Parse incomplete code differently to better reflect user intent. #15885

CyrusNajmabadi · 2016-12-14T00:59:14Z

This is a port of a parsing strategy we took in TypeScript to better deal with code as the user is typing it. Note that this is a change in how we parse code in error, and it involves heuristics to make the tree better match the user intent. The code in question is code like:

Task.
await Task.Delay(...);

Today, the C# parser eagerly parses this as a local declaration of the form "Task.await Task". i.e. the "Type" is "Task.await" and the VariableDeclarator is "Task". This clearly doesn't match what the user intends, and it messes up higher layers of the stack.

Specifically, because "Task." is a QualifiedName, that changes how we treat it (when it really should be a MemberAccessExpression). Similarly, because it looks like we're declaring a local called 'Task', we introduce a bogus LocalSymbol into scope, which messes up binding of names like "Task" (it finds the local instead of the type).

The fix is to tweak how we parse here. We specifically look for the pattern:

Id dot new-line
Id id

And we do not think of it as a LocalDeclaration unless we see a following token that more definitely demonstrates that it is local-variable. i.e.

X.
Y z;

In this case, there is no syntax error, so we have to accept this code as being a local variable declaration.

CyrusNajmabadi · 2016-12-14T00:59:36Z

Tagging @dotnet/roslyn-compiler

@agocke To make sure i didn't break parsing of LocalFunctions.

CyrusNajmabadi · 2016-12-14T00:59:51Z

@dotnet/roslyn-compiler Where is the best place to put tests for this?

CyrusNajmabadi · 2016-12-14T01:02:18Z

I'm going to add tests to ExpressionParsingTests for this.

CyrusNajmabadi · 2016-12-14T01:27:03Z

Note: in these error situations we use the presence of the newline as a strong indicator of user intent. This works quite well in practice and makes the code parse intuitively as the user thinks it should.

CyrusNajmabadi · 2016-12-14T01:33:38Z

src/Compilers/CSharp/Test/Syntax/Parsing/ExpressionParsingTests.cs

+                                        N(SyntaxKind.IdentifierToken, "Task");
+                                    }
+                                    N(SyntaxKind.DotToken);
+                                    M(SyntaxKind.IdentifierName);


note the missing identifier here. we now parse this as Task.<missing> as would be expected from the given source.

CyrusNajmabadi · 2016-12-14T01:36:19Z

See https://github.com/Microsoft/TypeScript/blob/master/src/compiler/parser.ts#L1923 as an example of how we dealt with this in TypeScript.

This issue was more pronounced in TS/JS as the following is legal in their language:

foo.any_keyword

So we would commonly have the first keyword of the next line getting sucked into the parse of the previous unterminated construct. This was an elegant way to address the issue low in the stack so that all higher levels worked well.

CyrusNajmabadi · 2016-12-14T01:36:29Z

Tagging @gafter for parser changes.

CyrusNajmabadi · 2016-12-14T01:37:04Z

Ok, tests have been added.

gafter · 2016-12-14T19:56:25Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                        //      X.Y z;
+                        //      X.Y z = ...
+                        //      X.Y z, ...  
+                        //      X.Y z( ...      (local function) 


There is also the case z[ which is an erroneous local array declaration.

Fort that case, I think the new error handling heuristic will actually produce a better result :-)

Could you please add a test case showing that?

Test added.

gafter · 2016-12-14T20:10:11Z

@CyrusNajmabadi master is in ask mode for RC3. Are you confident this would pass ask mode?

CyrusNajmabadi · 2016-12-14T20:17:27Z

Fairly confident yes. This is a pretty reasonable situation and the experience is currently poor

gafter

This looks good to me.

It would be nice to see the errors associated with the parse tests. I'll enhance the tests with that info in a separate PR.

gafter · 2016-12-14T22:56:12Z

@dotnet/roslyn-compiler Can we have a second review here, please?

AlekseyTs · 2016-12-14T23:01:04Z

It would be nice to see the errors associated with the parse tests. I'll enhance the tests with that info in a separate PR.

Why do we need to do this in a separate PR? I think it is appropriate to do this in this PR, this will make it easier to review and sign-off on the issue. @gafter If you want to help @CyrusNajmabadi, you can push commit to this PR.

CyrusNajmabadi · 2016-12-16T20:47:54Z

I would also be fine with more commits pushed to this.

gafter · 2016-12-16T20:53:13Z

@CyrusNajmabadi Don't wait for me. I'm technically on vacation this month, and this is not the next thing I would do. If you and/or @AlekseyTs think that needs to be done as part of this PR (I don't), someone other than me will have to do it.

CyrusNajmabadi · 2016-12-16T20:57:08Z

Oh ok. I'll do taht and let you know when it's done.

CyrusNajmabadi · 2016-12-16T21:20:02Z

@AlekseyTs Tests pushed.

CyrusNajmabadi · 2016-12-16T22:03:13Z

retest windows_eta_open_prtest please

CyrusNajmabadi · 2016-12-16T22:04:03Z

Tagging @srivatsn For approval once @AlekseyTs Has taken a look and signed off.

CyrusNajmabadi · 2016-12-16T22:04:51Z

src/EditorFeatures/Test2/IntelliSense/CSharpCompletionCommandHandlerTests.vb

@@ -2830,6 +2833,7 @@ class C
            End Using
        End Function

+        ' <WpfFact, Trait(Traits.Feature, Traits.Features.Completion)>


@rchande I tried to enable this test but it fails. Can you take a look and fix/enable the test?

jcouv · 2016-12-16T22:59:14Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+            {
+                var token1 = PeekToken(1);
+                if (token1.Kind == SyntaxKind.DotToken &&
+                    token1.TrailingTrivia.Any((int)SyntaxKind.EndOfLineTrivia))


Is this check sensitive to spaces?

jcouv

LGTM Thanks

AlekseyTs · 2016-12-17T00:17:09Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

+                               token4Kind == SyntaxKind.CommaToken ||
+                               token4Kind == SyntaxKind.OpenParenToken ||
+                               token4Kind == SyntaxKind.LessThanToken;
+                    }


Should we simply continue if this condition is true? #Closed

AlekseyTs · 2016-12-17T00:37:11Z

LGTM. However, please consider if it is better to let the old remaining logic in IsPossibleLocalDeclarationStatement to be applied to the situation that the new code recognized as a possible valid declaration.

CyrusNajmabadi · 2016-12-17T02:01:05Z

You're totally right aleksey. I'll make that change and resubmit

AlekseyTs · 2016-12-19T18:16:31Z

LGTM

CyrusNajmabadi · 2016-12-19T19:55:49Z

@gafter Are you ok with me merging this in? You assigned to yourself, so i wasn't sure if there was anything you wanted to do with this.

CyrusNajmabadi · 2016-12-19T21:34:00Z

Merging in as i see that @gafter LGTMed this.

…on-error code. Related to dotnet#15885 Fixes dotnet#17683

…on-error code. (#17692) Related to #15885 Fixes #17683

CyrusNajmabadi added 2 commits December 13, 2016 16:34

Improve completion in an expression before an 'await Task' expression.

0d05f0a

Parse incomplete code differently to better reflect user intent.

426677b

dnfclas added the cla-already-signed label Dec 14, 2016

CyrusNajmabadi force-pushed the awaitCompletion2 branch from a90b56a to 8e82403 Compare December 14, 2016 01:18

UPdate how we parse MemberAccess expressions as well.

0851ac8

CyrusNajmabadi force-pushed the awaitCompletion2 branch from 8e82403 to 0851ac8 Compare December 14, 2016 01:24

Formatting

ec74124

CyrusNajmabadi added 2 commits December 13, 2016 17:31

Add parsing tests.

24f4563

Disable test.

fb3f230

CyrusNajmabadi commented Dec 14, 2016

View reviewed changes

gafter reviewed Dec 14, 2016

View reviewed changes

gafter self-assigned this Dec 14, 2016

gafter added Area-Compilers Concept-Diagnostic Clarity The issues deals with the ease of understanding of errors and warnings. Feature Request labels Dec 14, 2016

gafter added this to the 2.0 (RC.3) milestone Dec 14, 2016

Add test.

3b21ccb

gafter approved these changes Dec 14, 2016

View reviewed changes

CyrusNajmabadi added 2 commits December 16, 2016 13:02

Merge branch 'master' into awaitCompletion2

32a294e

Adding tests.

77cc0c6

CyrusNajmabadi commented Dec 16, 2016

View reviewed changes

jcouv reviewed Dec 16, 2016

View reviewed changes

jcouv approved these changes Dec 16, 2016

View reviewed changes

AlekseyTs reviewed Dec 17, 2016

View reviewed changes

CyrusNajmabadi added 2 commits December 17, 2016 01:14

Merge remote-tracking branch 'upstream/master' into awaitCompletion2

3bf28e5

PR feedback.

6c6a6b5

srivatsn added the Approved to merge label Dec 19, 2016

CyrusNajmabadi merged commit 1338adc into dotnet:master Dec 19, 2016

CyrusNajmabadi deleted the awaitCompletion2 branch December 19, 2016 21:34

gafter added a commit to gafter/roslyn that referenced this pull request Mar 10, 2017

Refine syntax error recovery for partial code to not interfere with n…

d78fda7

…on-error code. Related to dotnet#15885 Fixes dotnet#17683

gafter mentioned this pull request Mar 10, 2017

Refine syntax error recovery for partial code to not interfere with non-error code. #17692

Merged

gafter added a commit that referenced this pull request Mar 15, 2017

Refine syntax error recovery for partial code to not interfere with n…

0bb7894

…on-error code. (#17692) Related to #15885 Fixes #17683

CyrusNajmabadi mentioned this pull request Feb 3, 2023

Improved parser recovery during typing #11817

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse incomplete code differently to better reflect user intent. #15885

Parse incomplete code differently to better reflect user intent. #15885

CyrusNajmabadi commented Dec 14, 2016

CyrusNajmabadi commented Dec 14, 2016

CyrusNajmabadi commented Dec 14, 2016

CyrusNajmabadi commented Dec 14, 2016

CyrusNajmabadi commented Dec 14, 2016

CyrusNajmabadi Dec 14, 2016

CyrusNajmabadi commented Dec 14, 2016

CyrusNajmabadi commented Dec 14, 2016

CyrusNajmabadi commented Dec 14, 2016

gafter Dec 14, 2016

CyrusNajmabadi Dec 14, 2016

gafter Dec 14, 2016

CyrusNajmabadi Dec 14, 2016

CyrusNajmabadi Dec 14, 2016

gafter commented Dec 14, 2016

CyrusNajmabadi commented Dec 14, 2016

gafter left a comment

gafter commented Dec 14, 2016

AlekseyTs commented Dec 14, 2016 •

edited

Loading

CyrusNajmabadi commented Dec 16, 2016

gafter commented Dec 16, 2016

CyrusNajmabadi commented Dec 16, 2016

CyrusNajmabadi commented Dec 16, 2016

CyrusNajmabadi commented Dec 16, 2016

CyrusNajmabadi commented Dec 16, 2016

CyrusNajmabadi Dec 16, 2016

jcouv Dec 16, 2016

jcouv left a comment

AlekseyTs Dec 17, 2016 •

edited

Loading

AlekseyTs commented Dec 17, 2016

CyrusNajmabadi commented Dec 17, 2016

AlekseyTs commented Dec 19, 2016

CyrusNajmabadi commented Dec 19, 2016

CyrusNajmabadi commented Dec 19, 2016

Parse incomplete code differently to better reflect user intent. #15885

Parse incomplete code differently to better reflect user intent. #15885

Conversation

CyrusNajmabadi commented Dec 14, 2016

CyrusNajmabadi commented Dec 14, 2016

CyrusNajmabadi commented Dec 14, 2016

CyrusNajmabadi commented Dec 14, 2016

CyrusNajmabadi commented Dec 14, 2016

Choose a reason for hiding this comment

CyrusNajmabadi commented Dec 14, 2016

CyrusNajmabadi commented Dec 14, 2016

CyrusNajmabadi commented Dec 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter commented Dec 14, 2016

CyrusNajmabadi commented Dec 14, 2016

gafter left a comment

Choose a reason for hiding this comment

gafter commented Dec 14, 2016

AlekseyTs commented Dec 14, 2016 • edited Loading

CyrusNajmabadi commented Dec 16, 2016

gafter commented Dec 16, 2016

CyrusNajmabadi commented Dec 16, 2016

CyrusNajmabadi commented Dec 16, 2016

CyrusNajmabadi commented Dec 16, 2016

CyrusNajmabadi commented Dec 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs Dec 17, 2016 • edited Loading

Choose a reason for hiding this comment

AlekseyTs commented Dec 17, 2016

CyrusNajmabadi commented Dec 17, 2016

AlekseyTs commented Dec 19, 2016

CyrusNajmabadi commented Dec 19, 2016

CyrusNajmabadi commented Dec 19, 2016

AlekseyTs commented Dec 14, 2016 •

edited

Loading

AlekseyTs Dec 17, 2016 •

edited

Loading