From 24f6e41de10d3488985b5e00d1ebb206a3a510b4 Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Thu, 28 Aug 2014 17:14:57 -0700 Subject: [PATCH 1/5] Added getOccs support for if/else keywords, and some tests. --- src/services/services.ts | 85 ++++++++++++++++--- tests/cases/fourslash/getOccurrencesIfElse.ts | 36 ++++++++ .../cases/fourslash/getOccurrencesIfElse2.ts | 32 +++++++ .../cases/fourslash/getOccurrencesIfElse3.ts | 32 +++++++ .../cases/fourslash/getOccurrencesIfElse4.ts | 30 +++++++ .../getOccurrencesIfElseNegatives.ts | 29 +++++++ 6 files changed, 232 insertions(+), 12 deletions(-) create mode 100644 tests/cases/fourslash/getOccurrencesIfElse.ts create mode 100644 tests/cases/fourslash/getOccurrencesIfElse2.ts create mode 100644 tests/cases/fourslash/getOccurrencesIfElse3.ts create mode 100644 tests/cases/fourslash/getOccurrencesIfElse4.ts create mode 100644 tests/cases/fourslash/getOccurrencesIfElseNegatives.ts diff --git a/src/services/services.ts b/src/services/services.ts index 862ca387e8..ccc4924a9c 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -2168,6 +2168,11 @@ module ts { } switch (node.kind) { + case SyntaxKind.IfKeyword: + case SyntaxKind.ElseKeyword: + if (hasKind(node.parent, SyntaxKind.IfStatement)) { + return getIfElseOccurrences(node.parent); + } case SyntaxKind.TryKeyword: case SyntaxKind.CatchKeyword: case SyntaxKind.FinallyKeyword: @@ -2195,6 +2200,65 @@ module ts { return undefined; + function getIfElseOccurrences(ifStatement: IfStatement): ReferenceEntry[] { + var keywords: Node[] = []; + + // Traverse upwards through all parent if-statements linked by their else-branches. + while (hasKind(ifStatement.parent, SyntaxKind.IfStatement) && (ifStatement.parent).elseStatement === ifStatement) { + ifStatement = ifStatement.parent; + } + + // Now traverse back down through the else branches, aggregating if/else keywords of if-statements. + while (ifStatement) { + pushIfAndElseKeywords(); + + if (!hasKind(ifStatement.elseStatement, SyntaxKind.IfStatement)) { + break + } + + ifStatement = ifStatement.elseStatement; + } + + var result: ReferenceEntry[] = []; + + // Here we do a little extra. + // We'd like to make else/ifs on the same line to be highlighted together + for (var i = 0; i < keywords.length; i++) { + if (keywords[i].kind === SyntaxKind.ElseKeyword && i < keywords.length - 1) { + var elseKeyword = keywords[i]; + var ifKeyword = keywords[i + 1]; // this *should* always be an 'if' keyword. + + // Ensure that the keywords are only separated by trivia. + if (elseKeyword.end === ifKeyword.getFullStart()) { + var elseLine = sourceFile.getLineAndCharacterFromPosition(elseKeyword.end); + var ifLine = sourceFile.getLineAndCharacterFromPosition(ifKeyword.getStart()); + + if (elseLine.line === ifLine.line) { + result.push(new ReferenceEntry(filename, TypeScript.TextSpan.fromBounds(elseKeyword.getStart(), ifKeyword.end), /* isWriteAccess */ false)); + i++; // skip the next keyword + continue; + } + } + } + + result.push(keywordToReferenceEntry(keywords[i])); + } + + return result; + + function pushIfAndElseKeywords() { + var children = ifStatement.getChildren(); + pushKeywordIf(keywords, children[0], SyntaxKind.IfKeyword); + + // Generally the 'else' keyword is second-to-last, so we traverse backwards. + for (var i = children.length - 1; i >= 0; i--) { + if (pushKeywordIf(keywords, children[i], SyntaxKind.ElseKeyword)) { + break; + } + } + } + } + function getTryCatchFinallyOccurrences(tryStatement: TryStatement): ReferenceEntry[] { var keywords: Node[] = []; @@ -2208,7 +2272,7 @@ module ts { pushKeywordIf(keywords, tryStatement.finallyBlock.getFirstToken(), SyntaxKind.FinallyKeyword); } - return keywordsToReferenceEntries(keywords); + return map(keywords, keywordToReferenceEntry); } function getSwitchCaseDefaultOccurrences(switchStatement: SwitchStatement) { @@ -2244,7 +2308,7 @@ module ts { }); }); - return keywordsToReferenceEntries(keywords); + return map(keywords, keywordToReferenceEntry); } function getBreakStatementOccurences(breakStatement: BreakOrContinueStatement): ReferenceEntry[]{ @@ -2285,20 +2349,17 @@ module ts { return node && node.parent; } - function pushKeywordIf(keywordList: Node[], token: Node, ...expected: SyntaxKind[]): void { - if (!token) { - return; + function pushKeywordIf(keywordList: Node[], token: Node, ...expected: SyntaxKind[]): boolean { + if (token && contains(expected, token.kind)) { + keywordList.push(token); + return true; } - if (contains(expected, token.kind)) { - keywordList.push(token); - } + return false; } - function keywordsToReferenceEntries(keywords: Node[]): ReferenceEntry[]{ - return map(keywords, keyword => - new ReferenceEntry(filename, TypeScript.TextSpan.fromBounds(keyword.getStart(), keyword.end), /* isWriteAccess */ false) - ); + function keywordToReferenceEntry(keyword: Node): ReferenceEntry { + return new ReferenceEntry(filename, TypeScript.TextSpan.fromBounds(keyword.getStart(), keyword.end), /* isWriteAccess */ false); } } diff --git a/tests/cases/fourslash/getOccurrencesIfElse.ts b/tests/cases/fourslash/getOccurrencesIfElse.ts new file mode 100644 index 0000000000..ec9533bf5b --- /dev/null +++ b/tests/cases/fourslash/getOccurrencesIfElse.ts @@ -0,0 +1,36 @@ +/// + +////[|if|] (true) { +//// if (false) { +//// } +//// else { +//// } +//// if (true) { +//// } +//// else { +//// if (false) +//// if (true) +//// var x = undefined; +//// } +////} +////[|else i/**/f|] (null) { +////} +////[|else /* whar garbl */ if|] (undefined) { +////} +////[|else|] +////[|if|] (false) { +////} +////[|else|] { } + +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/getOccurrencesIfElse2.ts b/tests/cases/fourslash/getOccurrencesIfElse2.ts new file mode 100644 index 0000000000..76df97179c --- /dev/null +++ b/tests/cases/fourslash/getOccurrencesIfElse2.ts @@ -0,0 +1,32 @@ +/// + +////if (true) { +//// [|if|] (false) { +//// } +//// [|else|]{ +//// } +//// if (true) { +//// } +//// else { +//// if (false) +//// if (true) +//// var x = undefined; +//// } +////} +////else if (null) { +////} +////else /* whar garbl */ if (undefined) { +////} +////else +////if (false) { +////} +////else { } + + +test.ranges().forEach(r => { + goTo.position(r.start); + + test.ranges().forEach(range => { + verify.occurrencesAtPositionContains(range, false); + }); +}); \ No newline at end of file diff --git a/tests/cases/fourslash/getOccurrencesIfElse3.ts b/tests/cases/fourslash/getOccurrencesIfElse3.ts new file mode 100644 index 0000000000..92a31800e9 --- /dev/null +++ b/tests/cases/fourslash/getOccurrencesIfElse3.ts @@ -0,0 +1,32 @@ +/// + +////if (true) { +//// if (false) { +//// } +//// else { +//// } +//// [|if|] (true) { +//// } +//// [|else|] { +//// if (false) +//// if (true) +//// var x = undefined; +//// } +////} +////else if (null) { +////} +////else /* whar garbl */ if (undefined) { +////} +////else +////if (false) { +////} +////else { } + + +test.ranges().forEach(r => { + goTo.position(r.start); + + test.ranges().forEach(range => { + verify.occurrencesAtPositionContains(range, false); + }); +}); \ No newline at end of file diff --git a/tests/cases/fourslash/getOccurrencesIfElse4.ts b/tests/cases/fourslash/getOccurrencesIfElse4.ts new file mode 100644 index 0000000000..c110521b93 --- /dev/null +++ b/tests/cases/fourslash/getOccurrencesIfElse4.ts @@ -0,0 +1,30 @@ +/// + +////if (true) { +//// if (false) { +//// } +//// else { +//// } +//// if (true) { +//// } +//// else { +//// /*1*/if (false) +//// /*2*/i/*3*/f (true) +//// var x = undefined; +//// } +////} +////else if (null) { +////} +////else /* whar garbl */ if (undefined) { +////} +////else +////if (false) { +////} +////else { } + + +for (var i = 1; i <= test.markers().length; i++) { + goTo.marker("" + i); + + verify.occurrencesAtPositionCount(1); +} \ No newline at end of file diff --git a/tests/cases/fourslash/getOccurrencesIfElseNegatives.ts b/tests/cases/fourslash/getOccurrencesIfElseNegatives.ts new file mode 100644 index 0000000000..68cbf5f187 --- /dev/null +++ b/tests/cases/fourslash/getOccurrencesIfElseNegatives.ts @@ -0,0 +1,29 @@ +/// + +////if/*1*/ (true) { +//// if/*2*/ (false) { +//// } +//// else/*3*/ { +//// } +//// if/*4*/ (true) { +//// } +//// else/*5*/ { +//// if/*6*/ (false) +//// if/*7*/ (true) +//// var x = undefined; +//// } +////} +////else/*8*/ if (null) { +////} +////else/*9*/ /* whar garbl */ if/*10*/ (undefined) { +////} +////else/*11*/ +////if/*12*/ (false) { +////} +////else/*13*/ { } + + +for (var i = 1; i <= test.markers().length; i++) { + goTo.marker("" + i); + verify.occurrencesAtPositionCount(0); +} From 0632d0c38c286e44d0324f3117f92a102ca73401 Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Fri, 29 Aug 2014 13:08:48 -0700 Subject: [PATCH 2/5] Addressed CR feedback, no longer highlighting elseifs with comments between. --- src/services/services.ts | 50 ++++++++++--------- tests/cases/fourslash/getOccurrencesIfElse.ts | 4 +- .../cases/fourslash/getOccurrencesIfElse2.ts | 2 +- .../cases/fourslash/getOccurrencesIfElse3.ts | 2 +- .../cases/fourslash/getOccurrencesIfElse4.ts | 2 +- .../getOccurrencesIfElseNegatives.ts | 8 +-- 6 files changed, 35 insertions(+), 33 deletions(-) diff --git a/src/services/services.ts b/src/services/services.ts index ccc4924a9c..ca59140ffb 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -2210,7 +2210,15 @@ module ts { // Now traverse back down through the else branches, aggregating if/else keywords of if-statements. while (ifStatement) { - pushIfAndElseKeywords(); + var children = ifStatement.getChildren(); + pushKeywordIf(keywords, children[0], SyntaxKind.IfKeyword); + + // Generally the 'else' keyword is second-to-last, so we traverse backwards. + for (var i = children.length - 1; i >= 0; i--) { + if (pushKeywordIf(keywords, children[i], SyntaxKind.ElseKeyword)) { + break; + } + } if (!hasKind(ifStatement.elseStatement, SyntaxKind.IfStatement)) { break @@ -2221,42 +2229,36 @@ module ts { var result: ReferenceEntry[] = []; - // Here we do a little extra. - // We'd like to make else/ifs on the same line to be highlighted together + // We'd like to highlight else/ifs together if they are only separated by spaces/tabs + // (i.e. the keywords are separated by no comments, no newlines). for (var i = 0; i < keywords.length; i++) { if (keywords[i].kind === SyntaxKind.ElseKeyword && i < keywords.length - 1) { var elseKeyword = keywords[i]; var ifKeyword = keywords[i + 1]; // this *should* always be an 'if' keyword. - // Ensure that the keywords are only separated by trivia. - if (elseKeyword.end === ifKeyword.getFullStart()) { - var elseLine = sourceFile.getLineAndCharacterFromPosition(elseKeyword.end); - var ifLine = sourceFile.getLineAndCharacterFromPosition(ifKeyword.getStart()); + var shouldHighlightNextKeyword = true; - if (elseLine.line === ifLine.line) { - result.push(new ReferenceEntry(filename, TypeScript.TextSpan.fromBounds(elseKeyword.getStart(), ifKeyword.end), /* isWriteAccess */ false)); - i++; // skip the next keyword - continue; + // Avoid recalculating getStart() by iterating backwards. + for (var j = ifKeyword.getStart() - 1; j >= elseKeyword.end; j--) { + var c = sourceFile.text.charCodeAt(j); + if (c !== CharacterCodes.space && c !== CharacterCodes.tab) { + shouldHighlightNextKeyword = false; + break; } } + + if (shouldHighlightNextKeyword) { + result.push(new ReferenceEntry(filename, TypeScript.TextSpan.fromBounds(elseKeyword.getStart(), ifKeyword.end), /* isWriteAccess */ false)); + i++; // skip the next keyword + continue; + } } + // Ordinary case: just highlight the keyword. result.push(keywordToReferenceEntry(keywords[i])); } return result; - - function pushIfAndElseKeywords() { - var children = ifStatement.getChildren(); - pushKeywordIf(keywords, children[0], SyntaxKind.IfKeyword); - - // Generally the 'else' keyword is second-to-last, so we traverse backwards. - for (var i = children.length - 1; i >= 0; i--) { - if (pushKeywordIf(keywords, children[i], SyntaxKind.ElseKeyword)) { - break; - } - } - } } function getTryCatchFinallyOccurrences(tryStatement: TryStatement): ReferenceEntry[] { @@ -2350,7 +2352,7 @@ module ts { } function pushKeywordIf(keywordList: Node[], token: Node, ...expected: SyntaxKind[]): boolean { - if (token && contains(expected, token.kind)) { + if (token && contains(expected, token.kind)) { keywordList.push(token); return true; } diff --git a/tests/cases/fourslash/getOccurrencesIfElse.ts b/tests/cases/fourslash/getOccurrencesIfElse.ts index ec9533bf5b..96c70d404a 100644 --- a/tests/cases/fourslash/getOccurrencesIfElse.ts +++ b/tests/cases/fourslash/getOccurrencesIfElse.ts @@ -13,9 +13,9 @@ //// var x = undefined; //// } ////} -////[|else i/**/f|] (null) { +////[|else i/**/f|] (null) { ////} -////[|else /* whar garbl */ if|] (undefined) { +////[|else|] /* whar garbl */ [|if|] (undefined) { ////} ////[|else|] ////[|if|] (false) { diff --git a/tests/cases/fourslash/getOccurrencesIfElse2.ts b/tests/cases/fourslash/getOccurrencesIfElse2.ts index 76df97179c..012e7a96ef 100644 --- a/tests/cases/fourslash/getOccurrencesIfElse2.ts +++ b/tests/cases/fourslash/getOccurrencesIfElse2.ts @@ -13,7 +13,7 @@ //// var x = undefined; //// } ////} -////else if (null) { +////else if (null) { ////} ////else /* whar garbl */ if (undefined) { ////} diff --git a/tests/cases/fourslash/getOccurrencesIfElse3.ts b/tests/cases/fourslash/getOccurrencesIfElse3.ts index 92a31800e9..b0ca361564 100644 --- a/tests/cases/fourslash/getOccurrencesIfElse3.ts +++ b/tests/cases/fourslash/getOccurrencesIfElse3.ts @@ -13,7 +13,7 @@ //// var x = undefined; //// } ////} -////else if (null) { +////else if (null) { ////} ////else /* whar garbl */ if (undefined) { ////} diff --git a/tests/cases/fourslash/getOccurrencesIfElse4.ts b/tests/cases/fourslash/getOccurrencesIfElse4.ts index c110521b93..6674625274 100644 --- a/tests/cases/fourslash/getOccurrencesIfElse4.ts +++ b/tests/cases/fourslash/getOccurrencesIfElse4.ts @@ -13,7 +13,7 @@ //// var x = undefined; //// } ////} -////else if (null) { +////else if (null) { ////} ////else /* whar garbl */ if (undefined) { ////} diff --git a/tests/cases/fourslash/getOccurrencesIfElseNegatives.ts b/tests/cases/fourslash/getOccurrencesIfElseNegatives.ts index 68cbf5f187..601eba63c6 100644 --- a/tests/cases/fourslash/getOccurrencesIfElseNegatives.ts +++ b/tests/cases/fourslash/getOccurrencesIfElseNegatives.ts @@ -13,7 +13,7 @@ //// var x = undefined; //// } ////} -////else/*8*/ if (null) { +////else/*8*/ if (null) { ////} ////else/*9*/ /* whar garbl */ if/*10*/ (undefined) { ////} @@ -23,7 +23,7 @@ ////else/*13*/ { } -for (var i = 1; i <= test.markers().length; i++) { - goTo.marker("" + i); +test.markers().forEach(m => { + goTo.position(m.position, m.fileName) verify.occurrencesAtPositionCount(0); -} +}); From 0a5c12c7ab774a99dfd7521ec01437c92bf2760d Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Fri, 29 Aug 2014 13:44:54 -0700 Subject: [PATCH 3/5] Added test case for broken if-elses. --- .../fourslash/getOccurrencesIfElseBroken.ts | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) create mode 100644 tests/cases/fourslash/getOccurrencesIfElseBroken.ts diff --git a/tests/cases/fourslash/getOccurrencesIfElseBroken.ts b/tests/cases/fourslash/getOccurrencesIfElseBroken.ts new file mode 100644 index 0000000000..a9e1e94f42 --- /dev/null +++ b/tests/cases/fourslash/getOccurrencesIfElseBroken.ts @@ -0,0 +1,24 @@ +/// + + +////[|if|] (true) { +//// var x = 1; +////} +////[|else if|] () +////[|else if|] +////[|else|] /* whar garbl */ [|if|] (i/**/f (true) { } else { }) +////else + +// It would be nice if in the future, +// We could include that last 'else'. + +test.ranges().forEach(r => { + goTo.position(r.start); + + test.ranges().forEach(range => { + verify.occurrencesAtPositionContains(range, false); + }); +}); + +goTo.marker(); +verify.occurrencesAtPositionCount(2); \ No newline at end of file From 38d7ba612f1fa3fe3f4c7364e4578589b9d7a681 Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Fri, 29 Aug 2014 13:48:00 -0700 Subject: [PATCH 4/5] Added missing break statement. --- src/services/services.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/services/services.ts b/src/services/services.ts index ca59140ffb..9d653ab2ca 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -2173,6 +2173,7 @@ module ts { if (hasKind(node.parent, SyntaxKind.IfStatement)) { return getIfElseOccurrences(node.parent); } + break; case SyntaxKind.TryKeyword: case SyntaxKind.CatchKeyword: case SyntaxKind.FinallyKeyword: From 3f3dd29461c506e333044fbe274a1f6d14fd83c9 Mon Sep 17 00:00:00 2001 From: Daniel Rosenwasser Date: Fri, 29 Aug 2014 14:48:20 -0700 Subject: [PATCH 5/5] Use isWhitespace in getIfElseOccurrences. --- src/services/services.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/services/services.ts b/src/services/services.ts index 9d653ab2ca..e5bdd0bf75 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -2230,7 +2230,7 @@ module ts { var result: ReferenceEntry[] = []; - // We'd like to highlight else/ifs together if they are only separated by spaces/tabs + // We'd like to highlight else/ifs together if they are only separated by whitespace // (i.e. the keywords are separated by no comments, no newlines). for (var i = 0; i < keywords.length; i++) { if (keywords[i].kind === SyntaxKind.ElseKeyword && i < keywords.length - 1) { @@ -2241,8 +2241,7 @@ module ts { // Avoid recalculating getStart() by iterating backwards. for (var j = ifKeyword.getStart() - 1; j >= elseKeyword.end; j--) { - var c = sourceFile.text.charCodeAt(j); - if (c !== CharacterCodes.space && c !== CharacterCodes.tab) { + if (!isWhiteSpace(sourceFile.text.charCodeAt(j))) { shouldHighlightNextKeyword = false; break; }