Fix !! incremental parsing by Youssef1313 · Pull Request #59977 · dotnet/roslyn

Youssef1313 · 2022-03-06T09:40:58Z

Fixes #59580

cc @RikkiGibson @tmat

Relates to test plan #36024

CyrusNajmabadi · 2022-03-06T16:56:20Z

Look for a method that has 'Fabricated' in its name. That should be what we are updating.

Youssef1313 · 2022-03-06T16:57:54Z

There is IsFabricatedToken. I'll see if updating it fixes the issue.

CyrusNajmabadi

Needs a tweak. If you can't find the relevant method, LMK and i can hunt it down on monday. THanks!

CyrusNajmabadi

LGTM!

Youssef1313 · 2022-03-06T17:01:35Z

@CyrusNajmabadi Thanks for the hint!

CyrusNajmabadi · 2022-03-06T17:01:57Z

                {
                    case SyntaxKind.GreaterThanGreaterThanToken:
                    case SyntaxKind.GreaterThanGreaterThanEqualsToken:
+                    case SyntaxKind.ExclamationExclamationToken:


my bad on forgetting about this @Youssef1313 @RikkiGibson . The merging tokens bit totally should have triggered things for me!

Consider adding a note to MergeAdjacent to mention this method should be updated when used.

Consider adding a note to MergeAdjacent to mention this method should be updated when used.

It looks LanguageParser.ParseLambdaExpression() also uses MergeAdjacent() for SyntaxKind.EqualsGreaterThanToken. If there's a potential incremental parsing bug there, we should open an issue.

good catch!

Consider adding an assert to MergeAdjacent(): Debug.Assert(IsFabricatedToken(kind));

Logged #60002.

jcouv

LGTM Thanks (iteration 3)

jcouv · 2022-03-07T19:36:50Z

@RikkiGibson @cston for second review. Thanks

cston · 2022-03-07T19:52:06Z

@@ -432,7 +463,6 @@ public void TestStatementToGlobalStatementChange()
                            SyntaxKind.IdentifierName,
                            SyntaxKind.SemicolonToken);
        }


}

Consider adding back the blank line.

CyrusNajmabadi

changing my vote. we're jsut asking to eb be bitten by the other merged token. we should just fix the code now and not have some random tree corruption issue someone has to figure out months down the line :)

RikkiGibson · 2022-03-07T22:13:14Z

changing my vote. we're jsut asking to eb be bitten by the other merged token. we should just fix the code now and not have some random tree corruption issue someone has to figure out months down the line :)

making sure I understand your concern here. Are you asking that #60002 be addressed now and not afterwards? Since we do have #60005 open currently which should address it.

CyrusNajmabadi · 2022-03-07T22:19:54Z

#60005 open currently which should address it.

That adds the assert (which is good). But i see no code change that adds the merged token taht the parser is creating to the blnder-reader to make sure we don't try to reuse that. or is the theory there that that's ok because => is a token that the lexer would produce? if so, i could be offbase and maybe this change is totally fine, just very subtle.

CyrusNajmabadi

retracting my objection.

not clear enough if that is hte actual fix.

RikkiGibson · 2022-03-08T01:17:45Z

@Youssef1313 do you think it's reasonable to do #59977 (comment) as part of this PR, just to ensure it's all out of the way?

Youssef1313 · 2022-03-08T05:00:10Z

@RikkiGibson This one is now already merged. I'm not sure it will be easy to reproduce the problem with lambdas parsing.

The else part of this condition will eat the fabricated token.

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

Lines 13018 to 13031 in 347cd02

    
           if (equalsToken != null) 
        
           { 
        
               // we only get an equals token if we had !!=> or !=> (enforced by IsPossibleLambdaExpression). 
        
               // So we must have a greater than token following.  If not, some invariant is badly broken. 
        
               var greaterThan = this.EatToken(); 
        
               Debug.Assert(greaterThan.Kind == SyntaxKind.GreaterThanToken); 
        
               arrow = MergeAdjacent(equalsToken, greaterThan, SyntaxKind.EqualsGreaterThanToken); 
        
           } 
        
           else 
        
           { 
        
               // Case x=>, x => 
        
               arrow = this.EatToken(SyntaxKind.EqualsGreaterThanToken); 
        
           }

Per @CyrusNajmabadi explanation, this is in general not correct because the merged token can be moved to a place where it contextually have a different meaning and hence a different SyntaxKind. But for this specific case:

The else is needed because lexer can pass us EqualsGreaterThanToken. The = and > gets separated only when there is a !!=> with no space between !! and =>. The !!=> thing is handed to parser as !, !=, and >. Then parameter null checking will merge the !! and leave the = and > as individual tokens.

Since => cannot have a different SyntaxKind when moved from one place in the tree to the other, I think there is no bug (currently) here. A bug will occur if => started to have different meanings.

The question is:

Should we add it to IsFabricatedToken anyway just in case => had a different meaning in future. Or probably leave everything as is now until an issue is reported?

Youssef1313 added 2 commits March 6, 2022 11:29

Add failing test

eef482e

Fix !! incremental parsing

141af31

Youssef1313 requested a review from a team as a code owner March 6, 2022 09:40

ghost added Community The pull request was submitted by a contributor who is not a Microsoft employee. Area-Compilers labels Mar 6, 2022

CyrusNajmabadi reviewed Mar 6, 2022

View reviewed changes

Comment thread src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Outdated

CyrusNajmabadi reviewed Mar 6, 2022

View reviewed changes

Comment thread src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Outdated

CyrusNajmabadi suggested changes Mar 6, 2022

View reviewed changes

Address feedback

002c322

CyrusNajmabadi approved these changes Mar 6, 2022

View reviewed changes

CyrusNajmabadi reviewed Mar 6, 2022

View reviewed changes

RikkiGibson self-assigned this Mar 6, 2022

jcouv approved these changes Mar 7, 2022

View reviewed changes

jcouv self-assigned this Mar 7, 2022

cston reviewed Mar 7, 2022

View reviewed changes

Update IncrementalParsingTests.cs

6bb581c

RikkiGibson approved these changes Mar 7, 2022

View reviewed changes

RikkiGibson enabled auto-merge (squash) March 7, 2022 20:49

cston mentioned this pull request Mar 7, 2022

Verify incremental parsing merged EqualsGreaterThanToken in ParseLambdaExpression() #60002

Closed

cston approved these changes Mar 7, 2022

View reviewed changes

CyrusNajmabadi previously requested changes Mar 7, 2022

View reviewed changes

CyrusNajmabadi reviewed Mar 7, 2022

View reviewed changes

RikkiGibson merged commit 15bf05a into dotnet:main Mar 8, 2022

ghost added this to the Next milestone Mar 8, 2022

Youssef1313 deleted the 59580-exclamation-exclamation branch March 8, 2022 04:29

allisonchou modified the milestones: Next, 17.2.P3 Mar 28, 2022

This was referenced Apr 3, 2022

Multiple Parameter Null Checks in Func Causes Compiler Error #60547

Closed

Double bang shows unexpected error when added to last argument of constructor #60615

Closed

Conversation

Youssef1313 commented Mar 6, 2022 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi commented Mar 6, 2022

Uh oh!

Youssef1313 commented Mar 6, 2022

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

Youssef1313 commented Mar 6, 2022

Uh oh!

CyrusNajmabadi Mar 6, 2022

Choose a reason for hiding this comment

Uh oh!

jcouv Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

cston Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

cston Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

cston Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Mar 7, 2022

Uh oh!

cston Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

RikkiGibson commented Mar 7, 2022

Uh oh!

CyrusNajmabadi commented Mar 7, 2022

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

RikkiGibson commented Mar 8, 2022

Uh oh!

Youssef1313 commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Youssef1313 commented Mar 6, 2022 •

edited by jcouv

Loading

cston Mar 7, 2022 •

edited

Loading

Youssef1313 commented Mar 8, 2022 •

edited

Loading