From ed119852256c69e104d8da4754ee1f68aa46a4d7 Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Tue, 10 Jul 2018 16:37:31 +0800 Subject: [PATCH 1/5] (GH-1417) Tokenize a document per line to avoid line endings Previously the folding provider would tokenize the entire document at a time. While this mostly works, it causes issues with regular expressions which use the `$` anchor. This anchor will interpret CRLF vs LF differently. While it may be possible to munge the document prior to tokenization, the prior art within the VS Code codebase shows that this is not the intended usage, i.e. lines should be tokenized, not an entire document. This commit changes the tokenization process to tokenize per line but still preserves the original folding behaviour. --- src/features/Folding.ts | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/features/Folding.ts b/src/features/Folding.ts index 2a793b00d7..e208935d9c 100644 --- a/src/features/Folding.ts +++ b/src/features/Folding.ts @@ -8,6 +8,7 @@ import * as vscode from "vscode"; import { DocumentSelector, LanguageClient, + Position, } from "vscode-languageclient"; import { IFeature } from "../feature"; import { ILogger } from "../logging"; @@ -193,8 +194,24 @@ export class FoldingProvider implements vscode.FoldingRangeProvider { // If the grammar hasn't been setup correctly, return empty result if (this.powershellGrammar == null) { return []; } - // Convert the document text into a series of grammar tokens - const tokens: ITokenList = this.powershellGrammar.tokenizeLine(document.getText(), null).tokens; + // Tokenize each line and build up an array of document-wide tokens + // Note that line endings (CRLF/LF/CR) have interpolation issues so don't + // tokenize an entire document if the line endings are variable. + const tokens: ITokenList = new Array(); + let tokenizationState = null; + for (let i = 0; i < document.lineCount; i++) { + const result = this.powershellGrammar.tokenizeLine(document.lineAt(i).text, tokenizationState); + const offset = document.offsetAt(new vscode.Position(i, 0)) ; + + for (const item of result.tokens) { + // Add the offset of the line to translate a character offset into + // a document based index + item.startIndex += offset; + item.endIndex += offset; + tokens.push(item); + } + tokenizationState = result.ruleStack; + } // Parse the token list looking for matching tokens and return // a list of LineNumberRange objects. Then filter the list and only return matches From 9bca79cfdc5ea48320318327c73ce0b794540085 Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Tue, 10 Jul 2018 16:36:01 +0800 Subject: [PATCH 2/5] (GH-1417) Add tests for CRLF documents Previously the test fixtures were failing on some Windows systems due to git changing the line endings. This commit adds a new test fixture, and runs the same tests on the same content document, but each with a different line-ending (CRLF and LF). --- test/features/folding.test.ts | 28 +++++++++-- test/fixtures/.gitattributes | 8 ++++ .../{folding.ps1 => folding-crlf.ps1} | 0 test/fixtures/folding-lf.ps1 | 47 +++++++++++++++++++ 4 files changed, 80 insertions(+), 3 deletions(-) create mode 100644 test/fixtures/.gitattributes rename test/fixtures/{folding.ps1 => folding-crlf.ps1} (100%) create mode 100644 test/fixtures/folding-lf.ps1 diff --git a/test/features/folding.test.ts b/test/features/folding.test.ts index bfa25ec11e..4afd4cbd80 100644 --- a/test/features/folding.test.ts +++ b/test/features/folding.test.ts @@ -36,10 +36,32 @@ suite("Features", () => { assert.notEqual(psGrammar, null); }); - test("Can detect all of the foldable regions in a document", async () => { - // Integration test against the test fixture 'folding.ps1' that contains + test("Can detect all of the foldable regions in a document with CRLF line endings", async () => { + // Integration test against the test fixture 'folding-crlf.ps1' that contains // all of the different types of folding available - const uri = vscode.Uri.file(path.join(fixturePath, "folding.ps1")); + const uri = vscode.Uri.file(path.join(fixturePath, "folding-crlf.ps1")); + const document = await vscode.workspace.openTextDocument(uri); + const result = await provider.provideFoldingRanges(document, null, null); + + const expected = [ + { start: 1, end: 6, kind: 1 }, + { start: 7, end: 46, kind: 3 }, + { start: 8, end: 13, kind: 1 }, + { start: 14, end: 17, kind: 3 }, + { start: 21, end: 23, kind: 1 }, + { start: 25, end: 35, kind: 3 }, + { start: 27, end: 31, kind: 3 }, + { start: 37, end: 39, kind: 3 }, + { start: 42, end: 45, kind: 3 }, + ]; + + assertFoldingRegions(result, expected); + }); + + test("Can detect all of the foldable regions in a document with LF line endings", async () => { + // Integration test against the test fixture 'folding-lf.ps1' that contains + // all of the different types of folding available + const uri = vscode.Uri.file(path.join(fixturePath, "folding-lf.ps1")); const document = await vscode.workspace.openTextDocument(uri); const result = await provider.provideFoldingRanges(document, null, null); diff --git a/test/fixtures/.gitattributes b/test/fixtures/.gitattributes new file mode 100644 index 0000000000..782904e062 --- /dev/null +++ b/test/fixtures/.gitattributes @@ -0,0 +1,8 @@ +# Set the default behavior, in case people don't have core.autocrlf set. +* text=auto + +# These test fixtures require crlf +folding-crlf.ps1 text eol=crlf + +# These test fixtures require lf +folding-lf.ps1 text eol=lf diff --git a/test/fixtures/folding.ps1 b/test/fixtures/folding-crlf.ps1 similarity index 100% rename from test/fixtures/folding.ps1 rename to test/fixtures/folding-crlf.ps1 diff --git a/test/fixtures/folding-lf.ps1 b/test/fixtures/folding-lf.ps1 new file mode 100644 index 0000000000..c023b75264 --- /dev/null +++ b/test/fixtures/folding-lf.ps1 @@ -0,0 +1,47 @@ +function short-func {}; +<# +.SYNOPSIS + Displays a list of WMI Classes based upon a search criteria +.EXAMPLE + Get-WmiClasses -class disk -ns rootcimv2" +#> +function New-VSCodeCannotFold { +<# +.SYNOPSIS + Displays a list of WMI Classes based upon a search criteria +.EXAMPLE + Get-WmiClasses -class disk -ns rootcimv2" +#> + $I = @' +cannot fold + +'@ + + # this won't be folded + + # This should be foldable + # This should be foldable + # This should be foldable + + #region This fools the indentation folding. + Write-Host "Hello" + # region + Write-Host "Hello" + # comment1 + Write-Host "Hello" + #endregion + Write-Host "Hello" + # comment2 + Write-Host "Hello" + # endregion + + $c = { + Write-Host "Hello" + } + + # Array fools indentation folding + $d = @( + 'element1', + 'elemet2' + ) +} From 848848a0880f9a85c66618367f7759f4ffab9173 Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Wed, 11 Jul 2018 09:59:38 +0800 Subject: [PATCH 3/5] (maint) Update folding test fixtures Previously some of the text in the test fixtures was confusing e.g. Text that says 'cannot fold' should read 'should fold'. This commit updates the text in the fixture to describe the expected behaviour for humans. --- test/fixtures/folding-crlf.ps1 | 28 ++++++++++++++-------------- test/fixtures/folding-lf.ps1 | 28 ++++++++++++++-------------- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/test/fixtures/folding-crlf.ps1 b/test/fixtures/folding-crlf.ps1 index c023b75264..6c53e785c1 100644 --- a/test/fixtures/folding-crlf.ps1 +++ b/test/fixtures/folding-crlf.ps1 @@ -1,31 +1,31 @@ -function short-func {}; +function short-func-not-fold {}; <# .SYNOPSIS - Displays a list of WMI Classes based upon a search criteria + This whole comment block should fold, not just the SYNOPSIS .EXAMPLE - Get-WmiClasses -class disk -ns rootcimv2" + This whole comment block should fold, not just the EXAMPLE #> -function New-VSCodeCannotFold { +function New-VSCodeShouldFold { <# .SYNOPSIS - Displays a list of WMI Classes based upon a search criteria + This whole comment block should fold, not just the SYNOPSIS .EXAMPLE - Get-WmiClasses -class disk -ns rootcimv2" + This whole comment block should fold, not just the EXAMPLE #> $I = @' -cannot fold +herestrings should fold '@ # this won't be folded - # This should be foldable - # This should be foldable - # This should be foldable + # This block of comments should be foldable as a single block + # This block of comments should be foldable as a single block + # This block of comments should be foldable as a single block #region This fools the indentation folding. Write-Host "Hello" - # region + # region Nested regions should be foldable Write-Host "Hello" # comment1 Write-Host "Hello" @@ -36,12 +36,12 @@ cannot fold # endregion $c = { - Write-Host "Hello" + Write-Host "Script blocks should be foldable" } # Array fools indentation folding $d = @( - 'element1', - 'elemet2' + 'should fold1', + 'should fold2' ) } diff --git a/test/fixtures/folding-lf.ps1 b/test/fixtures/folding-lf.ps1 index c023b75264..6c53e785c1 100644 --- a/test/fixtures/folding-lf.ps1 +++ b/test/fixtures/folding-lf.ps1 @@ -1,31 +1,31 @@ -function short-func {}; +function short-func-not-fold {}; <# .SYNOPSIS - Displays a list of WMI Classes based upon a search criteria + This whole comment block should fold, not just the SYNOPSIS .EXAMPLE - Get-WmiClasses -class disk -ns rootcimv2" + This whole comment block should fold, not just the EXAMPLE #> -function New-VSCodeCannotFold { +function New-VSCodeShouldFold { <# .SYNOPSIS - Displays a list of WMI Classes based upon a search criteria + This whole comment block should fold, not just the SYNOPSIS .EXAMPLE - Get-WmiClasses -class disk -ns rootcimv2" + This whole comment block should fold, not just the EXAMPLE #> $I = @' -cannot fold +herestrings should fold '@ # this won't be folded - # This should be foldable - # This should be foldable - # This should be foldable + # This block of comments should be foldable as a single block + # This block of comments should be foldable as a single block + # This block of comments should be foldable as a single block #region This fools the indentation folding. Write-Host "Hello" - # region + # region Nested regions should be foldable Write-Host "Hello" # comment1 Write-Host "Hello" @@ -36,12 +36,12 @@ cannot fold # endregion $c = { - Write-Host "Hello" + Write-Host "Script blocks should be foldable" } # Array fools indentation folding $d = @( - 'element1', - 'elemet2' + 'should fold1', + 'should fold2' ) } From f574a43ed058233760cd727667ef534aadee9650 Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Wed, 11 Jul 2018 10:07:59 +0800 Subject: [PATCH 4/5] (GH-1417) Add folding tests for double quote here strings Previously only single quoted here strings were tested for folding. This commit adds a test for the double quoted herestrings as well. --- test/features/folding.test.ts | 26 ++++++++++++++------------ test/fixtures/folding-crlf.ps1 | 5 +++++ test/fixtures/folding-lf.ps1 | 5 +++++ 3 files changed, 24 insertions(+), 12 deletions(-) diff --git a/test/features/folding.test.ts b/test/features/folding.test.ts index 4afd4cbd80..032ad8a29f 100644 --- a/test/features/folding.test.ts +++ b/test/features/folding.test.ts @@ -45,14 +45,15 @@ suite("Features", () => { const expected = [ { start: 1, end: 6, kind: 1 }, - { start: 7, end: 46, kind: 3 }, + { start: 7, end: 51, kind: 3 }, { start: 8, end: 13, kind: 1 }, { start: 14, end: 17, kind: 3 }, - { start: 21, end: 23, kind: 1 }, - { start: 25, end: 35, kind: 3 }, - { start: 27, end: 31, kind: 3 }, - { start: 37, end: 39, kind: 3 }, - { start: 42, end: 45, kind: 3 }, + { start: 19, end: 22, kind: 3 }, + { start: 26, end: 28, kind: 1 }, + { start: 30, end: 40, kind: 3 }, + { start: 32, end: 36, kind: 3 }, + { start: 42, end: 44, kind: 3 }, + { start: 47, end: 50, kind: 3 }, ]; assertFoldingRegions(result, expected); @@ -67,14 +68,15 @@ suite("Features", () => { const expected = [ { start: 1, end: 6, kind: 1 }, - { start: 7, end: 46, kind: 3 }, + { start: 7, end: 51, kind: 3 }, { start: 8, end: 13, kind: 1 }, { start: 14, end: 17, kind: 3 }, - { start: 21, end: 23, kind: 1 }, - { start: 25, end: 35, kind: 3 }, - { start: 27, end: 31, kind: 3 }, - { start: 37, end: 39, kind: 3 }, - { start: 42, end: 45, kind: 3 }, + { start: 19, end: 22, kind: 3 }, + { start: 26, end: 28, kind: 1 }, + { start: 30, end: 40, kind: 3 }, + { start: 32, end: 36, kind: 3 }, + { start: 42, end: 44, kind: 3 }, + { start: 47, end: 50, kind: 3 }, ]; assertFoldingRegions(result, expected); diff --git a/test/fixtures/folding-crlf.ps1 b/test/fixtures/folding-crlf.ps1 index 6c53e785c1..e9b05534ee 100644 --- a/test/fixtures/folding-crlf.ps1 +++ b/test/fixtures/folding-crlf.ps1 @@ -17,6 +17,11 @@ herestrings should fold '@ +$I = @" +double quoted herestrings should also fold + +"@ + # this won't be folded # This block of comments should be foldable as a single block diff --git a/test/fixtures/folding-lf.ps1 b/test/fixtures/folding-lf.ps1 index 6c53e785c1..e9b05534ee 100644 --- a/test/fixtures/folding-lf.ps1 +++ b/test/fixtures/folding-lf.ps1 @@ -17,6 +17,11 @@ herestrings should fold '@ +$I = @" +double quoted herestrings should also fold + +"@ + # this won't be folded # This block of comments should be foldable as a single block From ef0e3bde8d1143bc8f0c33b3b7bb0703c31ac449 Mon Sep 17 00:00:00 2001 From: Glenn Sarti Date: Wed, 11 Jul 2018 10:15:26 +0800 Subject: [PATCH 5/5] (maint) Refactor folding test fixtures Previously there was some duplication in the folder test fixtures. This commit refactors the expectation to be more DRY. This commit also adds an expectation on the line endings as we're depending on git to clone correctly. Without this we may get false positive results. --- test/features/folding.test.ts | 59 +++++++++++++++++------------------ 1 file changed, 28 insertions(+), 31 deletions(-) diff --git a/test/features/folding.test.ts b/test/features/folding.test.ts index 032ad8a29f..1f9ab98b4a 100644 --- a/test/features/folding.test.ts +++ b/test/features/folding.test.ts @@ -12,14 +12,14 @@ import { MockLogger } from "../test_utils"; const fixturePath = path.join(__dirname, "..", "..", "..", "test", "fixtures"); function assertFoldingRegions(result, expected): void { - assert.equal(result.length, expected.length); - for (let i = 0; i < expected.length; i++) { const failMessage = `expected ${JSON.stringify(expected[i])}, actual ${JSON.stringify(result[i])}`; assert.equal(result[i].start, expected[i].start, failMessage); assert.equal(result[i].end, expected[i].end, failMessage); assert.equal(result[i].kind, expected[i].kind, failMessage); } + + assert.equal(result.length, expected.length); } suite("Features", () => { @@ -36,14 +36,8 @@ suite("Features", () => { assert.notEqual(psGrammar, null); }); - test("Can detect all of the foldable regions in a document with CRLF line endings", async () => { - // Integration test against the test fixture 'folding-crlf.ps1' that contains - // all of the different types of folding available - const uri = vscode.Uri.file(path.join(fixturePath, "folding-crlf.ps1")); - const document = await vscode.workspace.openTextDocument(uri); - const result = await provider.provideFoldingRanges(document, null, null); - - const expected = [ + suite("For a single document", async () => { + const expectedFoldingRegions = [ { start: 1, end: 6, kind: 1 }, { start: 7, end: 51, kind: 3 }, { start: 8, end: 13, kind: 1 }, @@ -56,30 +50,33 @@ suite("Features", () => { { start: 47, end: 50, kind: 3 }, ]; - assertFoldingRegions(result, expected); - }); + test("Can detect all of the foldable regions in a document with CRLF line endings", async () => { + // Integration test against the test fixture 'folding-crlf.ps1' that contains + // all of the different types of folding available + const uri = vscode.Uri.file(path.join(fixturePath, "folding-crlf.ps1")); + const document = await vscode.workspace.openTextDocument(uri); + const result = await provider.provideFoldingRanges(document, null, null); - test("Can detect all of the foldable regions in a document with LF line endings", async () => { - // Integration test against the test fixture 'folding-lf.ps1' that contains - // all of the different types of folding available - const uri = vscode.Uri.file(path.join(fixturePath, "folding-lf.ps1")); - const document = await vscode.workspace.openTextDocument(uri); - const result = await provider.provideFoldingRanges(document, null, null); + // Ensure we have CRLF line endings as we're depending on git + // to clone the test fixtures correctly + assert.notEqual(document.getText().indexOf("\r\n"), -1); - const expected = [ - { start: 1, end: 6, kind: 1 }, - { start: 7, end: 51, kind: 3 }, - { start: 8, end: 13, kind: 1 }, - { start: 14, end: 17, kind: 3 }, - { start: 19, end: 22, kind: 3 }, - { start: 26, end: 28, kind: 1 }, - { start: 30, end: 40, kind: 3 }, - { start: 32, end: 36, kind: 3 }, - { start: 42, end: 44, kind: 3 }, - { start: 47, end: 50, kind: 3 }, - ]; + assertFoldingRegions(result, expectedFoldingRegions); + }); + + test("Can detect all of the foldable regions in a document with LF line endings", async () => { + // Integration test against the test fixture 'folding-lf.ps1' that contains + // all of the different types of folding available + const uri = vscode.Uri.file(path.join(fixturePath, "folding-lf.ps1")); + const document = await vscode.workspace.openTextDocument(uri); + const result = await provider.provideFoldingRanges(document, null, null); + + // Ensure we do not CRLF line endings as we're depending on git + // to clone the test fixtures correctly + assert.equal(document.getText().indexOf("\r\n"), -1); - assertFoldingRegions(result, expected); + assertFoldingRegions(result, expectedFoldingRegions); + }); }); }); });