From 90dd3276359eb8cd747a1978f02cf4599a2e3ed3 Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Fri, 5 Sep 2014 10:59:20 -0700 Subject: [PATCH] Changed logic for break/continue search in switch statements and loops. Now if a labeled break in a switch refers to its original switch statement, we also highlight the 'switch' keyword. Also added tests for loop/break/continue. --- src/services/services.ts | 105 ++++++++++++------ .../getOccurrencesLoopBreakContinue.ts | 77 +++++++++++++ .../getOccurrencesLoopBreakContinue2.ts | 77 +++++++++++++ .../getOccurrencesLoopBreakContinue3.ts | 77 +++++++++++++ .../getOccurrencesLoopBreakContinue4.ts | 77 +++++++++++++ .../getOccurrencesLoopBreakContinue5.ts | 77 +++++++++++++ ...etOccurrencesLoopBreakContinueNegatives.ts | 70 ++++++++++++ .../getOccurrencesSwitchCaseDefault2.ts | 13 ++- .../getOccurrencesSwitchCaseDefault3.ts | 25 +++++ 9 files changed, 561 insertions(+), 37 deletions(-) create mode 100644 tests/cases/fourslash/getOccurrencesLoopBreakContinue.ts create mode 100644 tests/cases/fourslash/getOccurrencesLoopBreakContinue2.ts create mode 100644 tests/cases/fourslash/getOccurrencesLoopBreakContinue3.ts create mode 100644 tests/cases/fourslash/getOccurrencesLoopBreakContinue4.ts create mode 100644 tests/cases/fourslash/getOccurrencesLoopBreakContinue5.ts create mode 100644 tests/cases/fourslash/getOccurrencesLoopBreakContinueNegatives.ts create mode 100644 tests/cases/fourslash/getOccurrencesSwitchCaseDefault3.ts diff --git a/src/services/services.ts b/src/services/services.ts index b0b4c21f43..3ef524c82e 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -1359,11 +1359,19 @@ module ts { } enum SearchMeaning { + None = 0x0, Value = 0x1, Type = 0x2, Namespace = 0x4 } + enum BreakContinueSearchType { + None = 0x0, + Unlabeled = 0x1, + Labeled = 0x2, + All = Unlabeled | Labeled + } + // A cache of completion entries for keywords, these do not change between sessions var keywordCompletions:CompletionEntry[] = []; for (var i = SyntaxKind.FirstKeyword; i <= SyntaxKind.LastKeyword; i++) { @@ -2318,49 +2326,45 @@ module ts { } } } + + // These track whether we can own unlabeled break/continues. + var breakSearchType = BreakContinueSearchType.All; + var continueSearchType = BreakContinueSearchType.All; - // This switch tracks whether or not we're traversing into a construct that takes - // ownership over unlabelled 'break'/'continue' statements. - var onlyCheckLabelled = false; - - forEachChild(loopNode.statement, function aggregateBreakContinues(node: Node) { - // This tracks the status of the flag before diving into the next node. - var lastOnlyCheckLabelled = onlyCheckLabelled; + (function aggregateBreakContinues(node: Node) { + // Remember the statuses of the flags before diving into the next node. + var prevBreakSearchType = breakSearchType; + var prevContinueSearchType = continueSearchType; switch (node.kind) { case SyntaxKind.BreakStatement: case SyntaxKind.ContinueStatement: - // If the 'break'/'continue' statement has a label, it must be one of our tracked labels. - if ((node).label) { - var labelName = (node).label.text; - if (isLabelledBy(loopNode, labelName)) { - pushKeywordIf(keywords, node.getFirstToken(), SyntaxKind.BreakKeyword, SyntaxKind.ContinueKeyword); - } - } - // If not, we are free to add it if we haven't lost ownership of unlabeled break/continue statements. - else if (!onlyCheckLabelled) { + if (ownsBreakOrContinue(loopNode, node, breakSearchType, continueSearchType)) { pushKeywordIf(keywords, node.getFirstToken(), SyntaxKind.BreakKeyword, SyntaxKind.ContinueKeyword); } break; - + case SyntaxKind.ForStatement: case SyntaxKind.ForInStatement: case SyntaxKind.DoStatement: case SyntaxKind.WhileStatement: - case SyntaxKind.SwitchStatement: - onlyCheckLabelled = true; + continueSearchType = BreakContinueSearchType.Labeled; // Fall through - default: - // Do not cross function boundaries. - if (!isAnyFunction(node)) { - forEachChild(node, aggregateBreakContinues); - } + case SyntaxKind.SwitchStatement: + breakSearchType = BreakContinueSearchType.Labeled; } - // Restore the last state. - onlyCheckLabelled = lastOnlyCheckLabelled; - }); - return map(keywords, keywordToReferenceEntry); + // Do not cross function boundaries. + if (!isAnyFunction(node)) { + forEachChild(node, aggregateBreakContinues); + } + + // Restore the last state. + breakSearchType = prevBreakSearchType; + continueSearchType = prevContinueSearchType; + })(loopNode.statement); + + return map(keywords, getReferenceEntryFromNode); } function getSwitchCaseDefaultOccurrences(switchStatement: SwitchStatement) { @@ -2368,38 +2372,50 @@ module ts { pushKeywordIf(keywords, switchStatement.getFirstToken(), SyntaxKind.SwitchKeyword); - // Go through each clause in the switch statement, collecting the clause keywords. + // Types of break statements we can grab on to. + var breakSearchType = BreakContinueSearchType.All; + + // Go through each clause in the switch statement, collecting the case/default keywords. forEach(switchStatement.clauses, clause => { pushKeywordIf(keywords, clause.getFirstToken(), SyntaxKind.CaseKeyword, SyntaxKind.DefaultKeyword); // For each clause, also recursively traverse the statements where we can find analogous breaks. forEachChild(clause, function aggregateBreakKeywords(node: Node): void { + // Back the old search value up. + var oldBreakSearchType = breakSearchType; + switch (node.kind) { case SyntaxKind.BreakStatement: // If the break statement has a label, it cannot be part of a switch block. - if (!(node).label) { + if (ownsBreakOrContinue(switchStatement, + node, + breakSearchType, + /*continuesSearchType*/ BreakContinueSearchType.None)) { pushKeywordIf(keywords, node.getFirstToken(), SyntaxKind.BreakKeyword); } - // Fall through + break; case SyntaxKind.ForStatement: case SyntaxKind.ForInStatement: case SyntaxKind.DoStatement: case SyntaxKind.WhileStatement: case SyntaxKind.SwitchStatement: - return; + breakSearchType = BreakContinueSearchType.Labeled; } // Do not cross function boundaries. if (!isAnyFunction(node)) { forEachChild(node, aggregateBreakKeywords); } + + // Restore the last state. + breakSearchType = oldBreakSearchType; }); }); return map(keywords, getReferenceEntryFromNode); } - function getBreakOrContinueStatementOccurences(breakOrContinueStatement: BreakOrContinueStatement): ReferenceEntry[]{ + function getBreakOrContinueStatementOccurences(breakOrContinueStatement: BreakOrContinueStatement): ReferenceEntry[] { for (var owner = node.parent; owner; owner = owner.parent) { switch (owner.kind) { case SyntaxKind.ForStatement: @@ -2413,8 +2429,8 @@ module ts { } break; case SyntaxKind.SwitchStatement: - // A switch statement can only be the owner of an unlabeled break statement. - if (breakOrContinueStatement.kind === SyntaxKind.BreakStatement && !breakOrContinueStatement.label) { + // A switch statement can only be the owner of an break statement. + if (breakOrContinueStatement.kind === SyntaxKind.BreakStatement && (!breakOrContinueStatement.label || isLabelledBy(owner, breakOrContinueStatement.label.text))) { return getSwitchCaseDefaultOccurrences(owner); } break; @@ -2429,6 +2445,25 @@ module ts { return undefined; } + // Note: 'statement' must be a descendant of 'root'. + // Reasonable logic for restricting traversal prior to arriving at the + // 'statement' node is beyond the scope of this function. + function ownsBreakOrContinue(root: Node, + statement: BreakOrContinueStatement, + breakSearchType: BreakContinueSearchType, + continueSearchType: BreakContinueSearchType): boolean { + var searchType = statement.kind === SyntaxKind.BreakStatement ? + breakSearchType : + continueSearchType; + + if (statement.label) { + return isLabelledBy(root, statement.label.text); + } + else { + return !!(searchType & BreakContinueSearchType.Unlabeled); + } + } + // returns true if 'node' is defined and has a matching 'kind'. function hasKind(node: Node, kind: SyntaxKind) { return node !== undefined && node.kind === kind; diff --git a/tests/cases/fourslash/getOccurrencesLoopBreakContinue.ts b/tests/cases/fourslash/getOccurrencesLoopBreakContinue.ts new file mode 100644 index 0000000000..aad909f973 --- /dev/null +++ b/tests/cases/fourslash/getOccurrencesLoopBreakContinue.ts @@ -0,0 +1,77 @@ +/// + +////var arr = [1, 2, 3, 4]; +////label1: [|for|] (var n in arr) { +//// [|break|]; +//// [|continue|]; +//// [|br/**/eak|] label1; +//// [|continue|] label1; +//// +//// label2: for (var i = 0; i < arr[n]; i++) { +//// [|break|] label1; +//// [|continue|] label1; +//// +//// break; +//// continue; +//// break label2; +//// continue label2; +//// +//// function foo() { +//// label3: while (true) { +//// break; +//// continue; +//// break label3; +//// continue label3; +//// +//// // these cross function boundaries +//// break label1; +//// continue label1; +//// break label2; +//// continue label2; +//// +//// label4: do { +//// break; +//// continue; +//// break label4; +//// continue label4; +//// +//// break label3; +//// continue label3; +//// +//// switch (10) { +//// case 1: +//// case 2: +//// break; +//// break label4; +//// default: +//// continue; +//// } +//// +//// // these cross function boundaries +//// break label1; +//// continue label1; +//// break label2; +//// continue label2; +//// () => { break; } +//// } while (true) +//// } +//// } +//// } +////} +//// +////label5: while (true) break label5; +//// +////label7: while (true) continue label5; + +test.ranges().forEach(r => { + goTo.position(r.start); + + test.ranges().forEach(range => { + verify.occurrencesAtPositionContains(range, false); + }); +}); + +goTo.marker(); +test.ranges().forEach(range => { + verify.occurrencesAtPositionContains(range, false); +}); \ No newline at end of file diff --git a/tests/cases/fourslash/getOccurrencesLoopBreakContinue2.ts b/tests/cases/fourslash/getOccurrencesLoopBreakContinue2.ts new file mode 100644 index 0000000000..cae9845e06 --- /dev/null +++ b/tests/cases/fourslash/getOccurrencesLoopBreakContinue2.ts @@ -0,0 +1,77 @@ +/// + +////var arr = [1, 2, 3, 4]; +////label1: for (var n in arr) { +//// break; +//// continue; +//// break label1; +//// continue label1; +//// +//// label2: [|f/**/or|] (var i = 0; i < arr[n]; i++) { +//// break label1; +//// continue label1; +//// +//// [|break|]; +//// [|continue|]; +//// [|break|] label2; +//// [|continue|] label2; +//// +//// function foo() { +//// label3: while (true) { +//// break; +//// continue; +//// break label3; +//// continue label3; +//// +//// // these cross function boundaries +//// break label1; +//// continue label1; +//// break label2; +//// continue label2; +//// +//// label4: do { +//// break; +//// continue; +//// break label4; +//// continue label4; +//// +//// break label3; +//// continue label3; +//// +//// switch (10) { +//// case 1: +//// case 2: +//// break; +//// break label4; +//// default: +//// continue; +//// } +//// +//// // these cross function boundaries +//// break label1; +//// continue label1; +//// break label2; +//// continue label2; +//// () => { break; +//// } while (true) +//// } +//// } +//// } +////} +//// +////label5: while (true) break label5; +//// +////label7: while (true) continue label5; + +test.ranges().forEach(r => { + goTo.position(r.start); + + test.ranges().forEach(range => { + verify.occurrencesAtPositionContains(range, false); + }); +}); + +goTo.marker(); +test.ranges().forEach(range => { + verify.occurrencesAtPositionContains(range, false); +}); \ No newline at end of file diff --git a/tests/cases/fourslash/getOccurrencesLoopBreakContinue3.ts b/tests/cases/fourslash/getOccurrencesLoopBreakContinue3.ts new file mode 100644 index 0000000000..571114ea15 --- /dev/null +++ b/tests/cases/fourslash/getOccurrencesLoopBreakContinue3.ts @@ -0,0 +1,77 @@ +/// + +////var arr = [1, 2, 3, 4]; +////label1: for (var n in arr) { +//// break; +//// continue; +//// break label1; +//// continue label1; +//// +//// label2: for (var i = 0; i < arr[n]; i++) { +//// break label1; +//// continue label1; +//// +//// break; +//// continue; +//// break label2; +//// continue label2; +//// +//// function foo() { +//// label3: [|w/**/hile|] (true) { +//// [|break|]; +//// [|continue|]; +//// [|break|] label3; +//// [|continue|] label3; +//// +//// // these cross function boundaries +//// break label1; +//// continue label1; +//// break label2; +//// continue label2; +//// +//// label4: do { +//// break; +//// continue; +//// break label4; +//// continue label4; +//// +//// [|break|] label3; +//// [|continue|] label3; +//// +//// switch (10) { +//// case 1: +//// case 2: +//// break; +//// break label4; +//// default: +//// continue; +//// } +//// +//// // these cross function boundaries +//// break label1; +//// continue label1; +//// break label2; +//// continue label2; +//// () => { break; } +//// } while (true) +//// } +//// } +//// } +////} +//// +////label5: while (true) break label5; +//// +////label7: while (true) continue label5; + +test.ranges().forEach(r => { + goTo.position(r.start); + + test.ranges().forEach(range => { + verify.occurrencesAtPositionContains(range, false); + }); +}); + +goTo.marker(); +test.ranges().forEach(range => { + verify.occurrencesAtPositionContains(range, false); +}); \ No newline at end of file diff --git a/tests/cases/fourslash/getOccurrencesLoopBreakContinue4.ts b/tests/cases/fourslash/getOccurrencesLoopBreakContinue4.ts new file mode 100644 index 0000000000..587fb1a093 --- /dev/null +++ b/tests/cases/fourslash/getOccurrencesLoopBreakContinue4.ts @@ -0,0 +1,77 @@ +/// + +////var arr = [1, 2, 3, 4]; +////label1: for (var n in arr) { +//// break; +//// continue; +//// break label1; +//// continue label1; +//// +//// label2: for (var i = 0; i < arr[n]; i++) { +//// break label1; +//// continue label1; +//// +//// break; +//// continue; +//// break label2; +//// continue label2; +//// +//// function foo() { +//// label3: while (true) { +//// break; +//// continue; +//// break label3; +//// continue label3; +//// +//// // these cross function boundaries +//// break label1; +//// continue label1; +//// break label2; +//// continue label2; +//// +//// label4: [|do|] { +//// [|break|]; +//// [|continue|]; +//// [|break|] label4; +//// [|continue|] label4; +//// +//// break label3; +//// continue label3; +//// +//// switch (10) { +//// case 1: +//// case 2: +//// break; +//// [|break|] label4; +//// default: +//// [|continue|]; +//// } +//// +//// // these cross function boundaries +//// break label1; +//// continue label1; +//// break label2; +//// continue label2; +//// () => { break; } +//// } [|wh/**/ile|] (true) +//// } +//// } +//// } +////} +//// +////label5: while (true) break label5; +//// +////label7: while (true) continue label5; + +test.ranges().forEach(r => { + goTo.position(r.start); + + test.ranges().forEach(range => { + verify.occurrencesAtPositionContains(range, false); + }); +}); + +goTo.marker(); +test.ranges().forEach(range => { + verify.occurrencesAtPositionContains(range, false); +}); \ No newline at end of file diff --git a/tests/cases/fourslash/getOccurrencesLoopBreakContinue5.ts b/tests/cases/fourslash/getOccurrencesLoopBreakContinue5.ts new file mode 100644 index 0000000000..d558e6f385 --- /dev/null +++ b/tests/cases/fourslash/getOccurrencesLoopBreakContinue5.ts @@ -0,0 +1,77 @@ +/// + +////var arr = [1, 2, 3, 4]; +////label1: for (var n in arr) { +//// break; +//// continue; +//// break label1; +//// continue label1; +//// +//// label2: for (var i = 0; i < arr[n]; i++) { +//// break label1; +//// continue label1; +//// +//// break; +//// continue; +//// break label2; +//// continue label2; +//// +//// function foo() { +//// label3: while (true) { +//// break; +//// continue; +//// break label3; +//// continue label3; +//// +//// // these cross function boundaries +//// break label1; +//// continue label1; +//// break label2; +//// continue label2; +//// +//// label4: do { +//// break; +//// continue; +//// break label4; +//// continue label4; +//// +//// break label3; +//// continue label3; +//// +//// switch (10) { +//// case 1: +//// case 2: +//// break; +//// break label4; +//// default: +//// continue; +//// } +//// +//// // these cross function boundaries +//// break label1; +//// continue label1; +//// break label2; +//// continue label2; +//// () => { break; } +//// } while (true) +//// } +//// } +//// } +////} +//// +////label5: [|while|] (true) [|br/**/eak|] label5; +//// +////label7: while (true) continue label5; + +test.ranges().forEach(r => { + goTo.position(r.start); + + test.ranges().forEach(range => { + verify.occurrencesAtPositionContains(range, false); + }); +}); + +goTo.marker(); +test.ranges().forEach(range => { + verify.occurrencesAtPositionContains(range, false); +}); \ No newline at end of file diff --git a/tests/cases/fourslash/getOccurrencesLoopBreakContinueNegatives.ts b/tests/cases/fourslash/getOccurrencesLoopBreakContinueNegatives.ts new file mode 100644 index 0000000000..0127245dd5 --- /dev/null +++ b/tests/cases/fourslash/getOccurrencesLoopBreakContinueNegatives.ts @@ -0,0 +1,70 @@ +/// + +////var arr = [1, 2, 3, 4]; +////label1: for (var n in arr) { +//// break; +//// continue; +//// break label1; +//// continue label1; +//// +//// label2: for (var i = 0; i < arr[n]; i++) { +//// break label1; +//// continue label1; +//// +//// break; +//// continue; +//// break label2; +//// continue label2; +//// +//// function foo() { +//// label3: while (true) { +//// break; +//// continue; +//// break label3; +//// continue label3; +//// +//// // these cross function boundaries +//// br/*1*/eak label1; +//// cont/*2*/inue label1; +//// bre/*3*/ak label2; +//// c/*4*/ontinue label2; +//// +//// label4: do { +//// break; +//// continue; +//// break label4; +//// continue label4; +//// +//// break label3; +//// continue label3; +//// +//// switch (10) { +//// case 1: +//// case 2: +//// break; +//// break label4; +//// default: +//// continue; +//// } +//// +//// // these cross function boundaries +//// br/*5*/eak label1; +//// co/*6*/ntinue label1; +//// br/*7*/eak label2; +//// con/*8*/tinue label2; +//// () => { b/*9*/reak; } +//// } while (true) +//// } +//// } +//// } +////} +//// +////label5: while (true) break label5; +//// +////label7: while (true) co/*10*/ntinue label5; + +test.markers().forEach(m => { + goTo.position(m.position); + + verify.occurrencesAtPositionCount(0); +}); diff --git a/tests/cases/fourslash/getOccurrencesSwitchCaseDefault2.ts b/tests/cases/fourslash/getOccurrencesSwitchCaseDefault2.ts index a8c91f530c..dc0db1f4b3 100644 --- a/tests/cases/fourslash/getOccurrencesSwitchCaseDefault2.ts +++ b/tests/cases/fourslash/getOccurrencesSwitchCaseDefault2.ts @@ -10,7 +10,7 @@ //// [|cas/*3*/e|] 2: //// [|b/*4*/reak|]; //// [|defaul/*5*/t|]: -//// break foo; +//// [|break|] foo; //// } //// case 0xBEEF: //// default: @@ -19,9 +19,18 @@ ////} +test.ranges().forEach(r => { + goTo.position(r.start); + + test.ranges().forEach(range => { + verify.occurrencesAtPositionContains(range, false); + }); +}); + + for (var i = 1; i <= test.markers().length; i++) { goTo.marker("" + i); - verify.occurrencesAtPositionCount(5); + verify.occurrencesAtPositionCount(6); test.ranges().forEach(range => { verify.occurrencesAtPositionContains(range, false); diff --git a/tests/cases/fourslash/getOccurrencesSwitchCaseDefault3.ts b/tests/cases/fourslash/getOccurrencesSwitchCaseDefault3.ts new file mode 100644 index 0000000000..959ce9d993 --- /dev/null +++ b/tests/cases/fourslash/getOccurrencesSwitchCaseDefault3.ts @@ -0,0 +1,25 @@ +/// + +////foo: [|switch|] (1) { +//// [|case|] 1: +//// [|case|] 2: +//// [|break|]; +//// [|case|] 3: +//// switch (2) { +//// case 1: +//// [|break|] foo; +//// continue; // invalid +//// default: +//// break; +//// } +//// [|default|]: +//// [|break|]; +////} + +test.ranges().forEach(r => { + goTo.position(r.start); + + test.ranges().forEach(range => { + verify.occurrencesAtPositionContains(range, false); + }); +});