Fix a few param-nullchecking parsing bugs by RikkiGibson · Pull Request #58644 · dotnet/roslyn

RikkiGibson · 2022-01-05T18:39:47Z

Closes #58585

Thanks to Cyrus for the greater part of the work here.

Relates to test plan #36024

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

CyrusNajmabadi · 2022-01-05T23:28:12Z

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

-                            && this.PeekToken(3).Kind == SyntaxKind.ExclamationToken
-                            && after.GetTrailingTriviaWidth() == 0
-                            && this.PeekToken(3).GetLeadingTriviaWidth() == 0
+                            && token3.Kind == SyntaxKind.ExclamationToken


intentionally removed this restriction.

here we are accepting this during scanning so we can give a better parse with errors, right?

correct! it also means there's uniform 'token handling' that goes in before calling the helper. none of these scanners (or hte helper) care about having this requirement. it means they're all resilient to accidental spaces happening as you edit.

CyrusNajmabadi · 2022-01-05T23:28:16Z

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

-                        && this.PeekToken(1).Kind == SyntaxKind.ExclamationToken
-                        && this.CurrentToken.GetTrailingTriviaWidth() == 0
-                        && this.PeekToken(1).GetLeadingTriviaWidth() == 0)
+                        && this.PeekToken(1).Kind == SyntaxKind.ExclamationToken)


intentionally removed this restriction.

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

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

RikkiGibson · 2022-01-05T23:39:42Z

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

-                            && this.PeekToken(3).Kind == SyntaxKind.ExclamationToken
-                            && after.GetTrailingTriviaWidth() == 0
-                            && this.PeekToken(3).GetLeadingTriviaWidth() == 0
+                            && token3.Kind == SyntaxKind.ExclamationToken


here we are accepting this during scanning so we can give a better parse with errors, right?

RikkiGibson · 2022-01-05T23:44:14Z

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

-                && this.PeekToken(3).Kind == SyntaxKind.GreaterThanToken)
+            if (token1.Kind == SyntaxKind.ExclamationToken
+                && token2.Kind == SyntaxKind.ExclamationEqualsToken
+                && token3.Kind == SyntaxKind.GreaterThanToken)


It's not necessary to change it but I wonder if this if and the preceding ifs would combine nicely as a pattern switch statement, or even something like

if ((token1.Kind, token2.Kind, token3.Kind) is ('!', '!', '=>') or ('!=', '>', _) or ('!', '!=', '>')) return true;

except with the proper use of SyntaxKind enum and proper indenting of course.

yup. i def like that!

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

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

RikkiGibson · 2022-01-06T00:05:54Z

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

+            UsingDeclaration("Func<string[], string> func0 = (x[]) => x;", options: TestOptions.RegularPreview,
+                    // (1,36): error CS1001: Identifier expected
+                    // Func<string[], string> func0 = (x[]) => x;
+                    Diagnostic(ErrorCode.ERR_IdentifierExpected, ")").WithLocation(1, 36)); N(SyntaxKind.FieldDeclaration);


I don't mind this parse, but I was confused that it expected an identifier instead of ).

good point. let me look into that!

RikkiGibson · 2022-01-06T00:07:37Z

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

+            UsingDeclaration("Func<string, string> func0 = (y, x = null) => x;", options: TestOptions.RegularPreview,
+                    // (1,44): error CS1003: Syntax error, ',' expected
+                    // Func<string, string> func0 = (y, x = null) => x;
+                    Diagnostic(ErrorCode.ERR_SyntaxError, "=>").WithArguments(",", "=>").WithLocation(1, 44)); N(SyntaxKind.FieldDeclaration);


does make me wonder if there is more room to unify on parameter lists in parenthesized lambdas and non-lambdas, and if we could simplify the parser and move some errors like these to binding in the process.

this is definitely possible. the IsPossibleLambda will have to do much more speculative parsing to handle the = null places whatnot. I put these tests in though so we could see the improvement if we do that.

i'm open to doing more in this PR if you've like!

I think we should consider it a "design debt" issue to address in the future.

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

RikkiGibson · 2022-01-06T19:41:36Z

The test failure in Unix is due to a whitespace/line ending difference. Haven't quite figured out how to hammer it down.

edit: resolved by eliminating the problematic test scenario. I don't think the scenario is valuable enough to warrant further effort working around the cross-plat/test framework issue.

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

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

Co-authored-by: Charles Stoner <10732005+cston@users.noreply.github.com>

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

Co-authored-by: Charles Stoner <10732005+cston@users.noreply.github.com>

cston · 2022-01-06T22:11:24Z

src/Compilers/CSharp/Test/Semantic/Semantics/NullCheckedParameterTests.cs

@@ -129,59 +129,49 @@ public void NullCheckedBadSyntax()
 #pragma warning disable CS8893  


warning disable CS8893

Was this #pragma disabling the warning previously? Does it now? (Was there a reason to change the parameter default value from null in each of the methods?)

sigh yes, some renumbering must have broken it. I feel it's simpler to use a literal string here, so if you don't object I'd like to keep it as it is in this PR. But I will delete the pragma.

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

chsienki

LGTM

Fix a few param-nullchecking parsing bugs

5a4330a

ghost added the Area-Compilers label Jan 5, 2022

CyrusNajmabadi reviewed Jan 5, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jan 5, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Jan 5, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Outdated Show resolved Hide resolved

CyrusNajmabadi added 5 commits January 5, 2022 12:39

Unify !! parsing logic in one location

92fa509

In progress

d801fa2

Code changes

63799ac

Add tests

7eabbf5

Add comment

f5e3bd5

CyrusNajmabadi reviewed Jan 5, 2022

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/DeclarationParsingTests.cs Outdated Show resolved Hide resolved

CyrusNajmabadi added 4 commits January 5, 2022 15:32

Simplify

47e7eff

Disallow features with __arglist

6f2b564

formatting

a6c9e0b

formatting

f690273

RikkiGibson commented Jan 6, 2022

View reviewed changes

Address some feedback and fix tests

1d233bf

RikkiGibson marked this pull request as ready for review January 6, 2022 18:40

RikkiGibson requested a review from a team as a code owner January 6, 2022 18:40

RikkiGibson mentioned this pull request Jan 6, 2022

__arglist should not be allowed to have a default value #58672

Closed

jcouv added the Feature - Param Null Checking label Jan 6, 2022

RikkiGibson changed the base branch from main to release/dev17.1 January 6, 2022 18:56

cston reviewed Jan 6, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Outdated Show resolved Hide resolved

cston reviewed Jan 6, 2022

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/DeclarationParsingTests.cs Show resolved Hide resolved

cston reviewed Jan 6, 2022

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/DeclarationParsingTests.cs Show resolved Hide resolved

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

b52aa77

Co-authored-by: Charles Stoner <10732005+cston@users.noreply.github.com>

This comment has been minimized.

Sign in to view

cston reviewed Jan 6, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Show resolved Hide resolved

cston reviewed Jan 6, 2022

View reviewed changes

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs Outdated Show resolved Hide resolved

RikkiGibson and others added 2 commits January 6, 2022 14:03

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

13f0242

Co-authored-by: Charles Stoner <10732005+cston@users.noreply.github.com>

Address feedback and fix test

ae0c6bf

CyrusNajmabadi mentioned this pull request Jan 6, 2022

Better parse when lambda expressions contain default parameter values. #58690

Merged

cston reviewed Jan 6, 2022

View reviewed changes

Address feedback and fix error code ordering

49c6eab

cston reviewed Jan 6, 2022

View reviewed changes

src/Compilers/CSharp/Test/Syntax/Parsing/LambdaParameterParsingTests.cs Outdated Show resolved Hide resolved

'Lamda' -> 'Lambda'

9489ed5

cston approved these changes Jan 6, 2022

View reviewed changes

chsienki approved these changes Jan 6, 2022

View reviewed changes

Fix failing tests

92aec55

RikkiGibson enabled auto-merge (squash) January 7, 2022 00:01

regenerate compiler code

6249dec

RikkiGibson merged commit 0e5c679 into dotnet:release/dev17.1 Jan 7, 2022

runfoapp bot mentioned this pull request Jan 7, 2022

Timeout on Microsoft.VisualStudio.LanguageServices.CSharp.UnitTests.dll tests #55638

Open

RikkiGibson deleted the pnc-fix-parsing branch January 7, 2022 16:28

		@@ -129,59 +129,49 @@ public void NullCheckedBadSyntax()
		#pragma warning disable CS8893

Conversation

RikkiGibson commented Jan 5, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jan 5, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RikkiGibson commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chsienki left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

RikkiGibson commented Jan 5, 2022 •

edited by jcouv

Loading

RikkiGibson Jan 5, 2022 •

edited

Loading

RikkiGibson commented Jan 6, 2022 •

edited

Loading