Code provider folding is not folding here strings #1417

rkeithhill · 2018-07-10T03:22:23Z

System Details

Operating system name and version: Windows 10 1803
VS Code version: 1.25.0 32-bit
PowerShell extension version: daily build with PR (GH-1413) Resolve promise correctly in Folding feature #1416 pulled down
Output from $PSVersionTable:

Name                           Value
----                           -----
PSVersion                      5.1.17134.137
PSEdition                      Desktop
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
BuildVersion                   10.0.17134.137
CLRVersion                     4.0.30319.42000
WSManStackVersion              3.0
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1

Issue Description

Code folding via the folding provider seems to be working for everything except here strings:

FWIW the npm tests fail on my dev machine:

Attached Logs

7/9/2018 9:13:36 PM [NORMAL] - Visual Studio Code v1.25.0 32-bit
7/9/2018 9:13:36 PM [NORMAL] - PowerShell Extension v1.7.0
7/9/2018 9:13:36 PM [NORMAL] - Operating System: Windows 64-bit
7/9/2018 9:13:36 PM [DIAGNOSTIC] - Succesfully required c:\Program Files (x86)\Microsoft VS Code\resources\app/node_modules.asar/vscode-textmate
7/9/2018 9:13:36 PM [DIAGNOSTIC] - Loaded the vscode-textmate module
7/9/2018 9:13:36 PM [DIAGNOSTIC] - Created the textmate Registry
7/9/2018 9:13:36 PM [DIAGNOSTIC] - PowerShell grammar file specified as c:\Program Files (x86)\Microsoft VS Code\resources\app\extensions\powershell\syntaxes\powershell.tmLanguage.json
7/9/2018 9:13:36 PM [NORMAL] - Language server starting --
7/9/2018 9:13:36 PM [NORMAL] -     exe: C:\WINDOWS\Sysnative\WindowsPowerShell\v1.0\powershell.exe
7/9/2018 9:13:36 PM [NORMAL] -     args: C:\Users\Keith\GitHub\rkeithhill\PowerShellEditorServices\module\PowerShellEditorServices\Start-EditorServices.ps1 -HostName 'Visual Studio Code Host' -HostProfileId 'Microsoft.VSCode' -HostVersion '1.7.0'-AdditionalModules @('PowerShellEditorServices.VSCode') -BundledModulesPath 'C:\Users\Keith\GitHub\rkeithhill\PowerShellEditorServices\module'-EnableConsoleRepl -LanguageServicePipeName LanguageService_55b445f8a3da0175246c.pipe -DebugServicePipeName DebugService_55b445f8a3da0175246c.pipe -LogLevel 'Diagnostic' -LogPath 'C:\Users\Keith\GitHub\rkeithhill\vscode-powershell\logs\1531192416-someValue.sessionId\EditorServices.log' -SessionDetailsPath 'C:\Users\Keith\GitHub\rkeithhill\vscode-powershell\sessions\PSES-VSCode-21924-436123' -FeatureFlags @()
7/9/2018 9:13:36 PM [NORMAL] - Syntax Folding Provider registered
7/9/2018 9:13:37 PM [NORMAL] - powershell.exe started, pid: 29196
7/9/2018 9:13:41 PM [NORMAL] - Language server started.
7/9/2018 9:13:41 PM [NORMAL] - {"status":"started","debugServiceTransport":"NamedPipe","languageServiceTransport":"NamedPipe","debugServicePipeName":"DebugService_55b445f8a3da0175246c.pipe","languageServicePipeName":"LanguageService_55b445f8a3da0175246c.pipe"}
7/9/2018 9:13:41 PM [NORMAL] - Connecting to language service on pipe LanguageService_55b445f8a3da0175246c.pipe...
7/9/2018 9:13:41 PM [NORMAL] - Language service connected.

The text was updated successfully, but these errors were encountered:

rkeithhill · 2018-07-10T03:22:53Z

Ping @glennsarti

glennsarti · 2018-07-10T03:27:40Z

Hrmmm...can't repro on my own machine ...

rkeithhill · 2018-07-10T03:31:00Z

Could it be something to do with me running 32-bit VSCode? That'd be odd but not unheard of. Are you looking at this file via the extension debug host?

glennsarti · 2018-07-10T03:40:04Z

yes and I'm running 32bit too.

glennsarti · 2018-07-10T03:47:02Z

What's the green squiggle saying underneath your $I

rkeithhill · 2018-07-10T03:48:02Z

That the variable $I is never used. Do you have code analysis disabled?

glennsarti · 2018-07-10T03:50:12Z

So the test failure is on that particular piece of text. The plot thickens... AND I can repro that failure.

glennsarti · 2018-07-10T03:59:56Z

Oh that's super weird. The tokenizer didn't see the heredoc.

rjmholt · 2018-07-10T04:02:21Z

Yeah I wondered if it was a grammar problem...

rjmholt · 2018-07-10T04:02:51Z

In that case, we should open an issue in EditorSyntax and mark as Resolution-External

glennsarti · 2018-07-10T04:03:45Z

Nope...not so fast. I can repro the test failure, but not the actual failure.

glennsarti · 2018-07-10T04:05:38Z

Currently thinking this is a test fixture issue and it's loading the wrong powershell grammar file...

rkeithhill · 2018-07-10T04:09:43Z

I noticed when the VSCode instance pops up during the test, it is v1.24.1. Even though I have v1.25 installed.

glennsarti · 2018-07-10T04:20:50Z

@rkeithhill Can you please try changing the file from CRLF to LF to see if it makes a difference

rkeithhill · 2018-07-10T04:22:38Z

Hmm, that fixes it. So, uh, that's weird.

glennsarti · 2018-07-10T04:24:18Z

It also fixes my test fixture problem. My git config is always use LF, which is why I may not have seen this.

rkeithhill · 2018-07-10T04:37:00Z

You may have Git configured to use LF but the repo configures itself to use auto via * text=auto in .gitattributes. However, if you're on Linux/macOS, auto would select LF.

glennsarti · 2018-07-10T06:46:10Z

@rkeithhill Ok so, it looks like I'm using the tokenizeLine method wrong. I should really be feeding it a line by line, instead of jamming the whole file in there. e.g.

It first splits the document into a line array
https://github.com/Microsoft/vscode/blob/d2b6bbb46fbdf535e2c96b3e00ac56ac1d427a69/src/vs/workbench/parts/themes/test/electron-browser/themes.test.contribution.ts#L227
And then for each line it calls tokenize
https://github.com/Microsoft/vscode/blob/d2b6bbb46fbdf535e2c96b3e00ac56ac1d427a69/src/vs/workbench/parts/themes/test/electron-browser/themes.test.contribution.ts#L131-L162

This would mean the CRLF / LF problem would disappear.

glennsarti · 2018-07-10T08:25:20Z

However....this throws out the order of the tokens which I depend on.

I also tried regex-ing out the CRLF, but I can only replace them I can't strip them e.g.

        const content: string = document.getText().replace(/\r/g, " ");
        const tokens: ITokenList = this.powershellGrammar.tokenizeLine(content, null).tokens;

I can't remove them because later on I need to access the document text via character offset, which is then wrong because the tokenization comes from a "different" document content.

I've narrowed it down to the regex used to determine the start of the heredoc \@'(?=$)
https://github.com/PowerShell/EditorSyntax/blob/master/PowerShellSyntax.tmLanguage#L137
https://github.com/PowerShell/EditorSyntax/blob/master/PowerShellSyntax.tmLanguage#L160

The $ regex anchor matching changes depending on what's running it.

For anchors there's an additional consideration when CR and LF occur as a pair and the regex flavor treats both these characters as line breaks. Delphi, Java, and the JGsoft flavor treat CRLF as an indivisible pair. ^ matches after CRLF and $ matches before CRLF, but neither match in the middle of a CRLF pair. JavaScript and XPath treat CRLF pairs as two line breaks. ^ matches in the middle of and after CRLF, while $ matches before and in the middle of CRLF.
Ref - https://www.regular-expressions.info/anchors.html

So it may simply be a case of changing how it determines the end of the line. 🤷‍♂️

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).

glennsarti · 2018-07-10T08:39:40Z

Started a WIP PR, with tests and janky workaround.

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.

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).

Previously only single quoted here strings were tested for folding. This commit adds a test for the double quoted herestrings as well.

* (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. * (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). * (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. * (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. * (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.

glennsarti mentioned this issue Jul 10, 2018

(GH-1417) Fix code folding on CRLF documents #1418

Merged

3 tasks

rjmholt closed this as completed in #1418 Jul 11, 2018

Code provider folding is not folding here strings #1417

Code provider folding is not folding here strings #1417

Comments

rkeithhill commented Jul 10, 2018

System Details

Issue Description

Attached Logs

rkeithhill commented Jul 10, 2018

Uh oh!

glennsarti commented Jul 10, 2018

Uh oh!

rkeithhill commented Jul 10, 2018

Uh oh!

glennsarti commented Jul 10, 2018

Uh oh!

glennsarti commented Jul 10, 2018

Uh oh!

rkeithhill commented Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glennsarti commented Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glennsarti commented Jul 10, 2018

Uh oh!

rjmholt commented Jul 10, 2018

Uh oh!

rjmholt commented Jul 10, 2018

Uh oh!

glennsarti commented Jul 10, 2018

Uh oh!

glennsarti commented Jul 10, 2018

Uh oh!

rkeithhill commented Jul 10, 2018

Uh oh!

glennsarti commented Jul 10, 2018

Uh oh!

rkeithhill commented Jul 10, 2018

Uh oh!

glennsarti commented Jul 10, 2018

Uh oh!

rkeithhill commented Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glennsarti commented Jul 10, 2018

Uh oh!

glennsarti commented Jul 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glennsarti commented Jul 10, 2018

Uh oh!

rkeithhill commented Jul 10, 2018 •

edited

Loading

glennsarti commented Jul 10, 2018 •

edited

Loading

rkeithhill commented Jul 10, 2018 •

edited

Loading

glennsarti commented Jul 10, 2018 •

edited

Loading