Improve generic type argument list error recovery by Rekkonnect · Pull Request #69734 · dotnet/roslyn

Rekkonnect · 2023-08-27T09:39:01Z

Also improves error recovery in bad tuple type arguments of the form (a x, b y) c.

Some erroneous code cases were also adjusted; the parser now leans more towards parsing generics than comparison expressions, only to improve error recovery.

The parser's logic now is:

After a successfully parsed type argument, if we fail to find a comma separator, we look forward. If the next token is an identifier token, followed by either of the following: ( ) { } ; = => < this operator or and not, we then consider the type argument list as complete, with a missing > token closing it.
Otherwise, if the next token is an identifier token, followed by a ] token and any of the above tokens, we consider the type argument list as complete. This was specifically added to handle the parameter list in a this accessor.

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

jcouv

LGTM Thanks (iteration 9)

jcouv · 2023-08-28T19:51:01Z

@dotnet/roslyn-compiler for second review. Thanks

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

CyrusNajmabadi · 2023-08-28T20:25:27Z

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

+                    or SyntaxKind.OpenBraceToken
+                    or SyntaxKind.CloseBraceToken
+                    or SyntaxKind.EqualsToken
+                    or SyntaxKind.EqualsGreaterThanToken;


It would be nice to see examples of all of the above.

Are there tests for each of these cases?

I think some of those might be missing some tests, will make another pass

this comment still applies. i want to know why each of those cases are something that would break the argument list.

comment still applies. :)

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

src/Features/CSharpTest/AddUsing/AddUsingTests.cs

src/Features/CSharpTest/AddUsing/AddUsingTestsWithAddImportDiagnosticProvider.cs

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

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

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

CyrusNajmabadi · 2023-09-29T15:22:42Z

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

+                    case SyntaxKind.OrKeyword:
+                    case SyntaxKind.AndKeyword:
+                    case SyntaxKind.NotKeyword:
+                        return true;


can you give an example of where this arises (in a code comment). #Closed

CyrusNajmabadi · 2023-09-29T15:24:04Z

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

+                    // Example: IEnumerable<string Method<T>()
+                    is SyntaxKind.LessThanToken
+                    // Example: Method<string(argument)
+                    or SyntaxKind.OpenParenToken


i would break these into separate 'if' checks. Different ones warrant different comments. For example, for this, i would mention, this means we'll do a bad job with Method<string (int a, ... where the ( starts a tuple type argument. But that we're ok with taht as we don't want to look that far ahead, and we feel like it's more likely that it's a parameter list.

CyrusNajmabadi

Signing off, with small nit requests.

CyrusNajmabadi · 2023-10-01T00:41:08Z

@dotnet/roslyn-compiler for another set of eyes. thanks!

Code changed

jcouv

LGTM Thanks (iteration 27)

jaredpar · 2024-04-01T18:22:29Z

@333fred for the second review here.

333fred

Mostly this looks good to me. Just a few comments.

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

333fred · 2024-04-01T20:18:08Z

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

+                    // Example: x is IEnumerable<string or IList<int>
+                    case SyntaxKind.OrKeyword:
+                    // Example: x is IEnumerable<string and IDisposable
+                    case SyntaxKind.AndKeyword:


Do we have tests for tuple types where the tuple type name is and or or and the tuple is unterminated?

333fred · 2024-04-01T22:55:46Z

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

I think I'd like to see a few tests with missing tuple end ). For example: IEnumerable<(string Value, string Description Values { get; }

333fred · 2024-04-02T16:32:14Z

Thanks @Rekkonnect!

* upstream/main: (416 commits) Semantic search (dotnet#71268) Make more static Fix MEF import of IExternalCSharpCopilotCodeAnalysisService to allow null Make static Make private Add comments Add method name to TimeInQueue telemetry (dotnet#72841) switch to frozen Simplify Add test Downstream Use singular helper when creating checksumsw Use singular helper when creating checksumsw Remove ability for a project to change its language Revert "Avoid creating result temp for is-expressions (dotnet#72273)" (dotnet#72827) Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 2420199 Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 2420199 Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 2420199 Localized file check-in by OneLocBuild Task: Build definition ID 327: Build ID 2420199 Improve generic type argument list error recovery (dotnet#69734) ...

Rekkonnect added 5 commits August 26, 2023 21:13

Improve error recovery on missing > in method

d7e9b38

Add many more tests

b4d81da

Recovery on properties

508a9c8

Recovery on tuples and more tests

1b5ff5b

Adjust tests

54c64dd

ghost added Community The pull request was submitted by a contributor who is not a Microsoft employee. Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 27, 2023

Rekkonnect marked this pull request as ready for review August 27, 2023 09:42

Rekkonnect requested a review from a team as a code owner August 27, 2023 09:42

Rekkonnect changed the title ~~Generic method error recovery~~ Improve generic type argument list error recovery Aug 27, 2023

Fix type argument list termination and tests

4c92d12

Rekkonnect requested a review from a team as a code owner August 28, 2023 08:52

Rekkonnect added 3 commits August 28, 2023 12:56

Move another test

a830762

Better recovery logic and another test

bd3f3bf

Adjust test after improvement

a6d0ffe

jcouv self-assigned this Aug 28, 2023

jcouv reviewed Aug 28, 2023

View reviewed changes

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

jcouv approved these changes Aug 28, 2023

View reviewed changes

CyrusNajmabadi reviewed Aug 28, 2023

View reviewed changes

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

CyrusNajmabadi reviewed Aug 28, 2023

View reviewed changes

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

CyrusNajmabadi reviewed Aug 28, 2023

View reviewed changes

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

CyrusNajmabadi reviewed Aug 28, 2023

View reviewed changes

src/Features/CSharpTest/AddUsing/AddUsingTests.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Aug 28, 2023

View reviewed changes

src/Features/CSharpTest/AddUsing/AddUsingTestsWithAddImportDiagnosticProvider.cs Show resolved Hide resolved

333fred reviewed Aug 28, 2023

View reviewed changes

Rekkonnect added 3 commits August 29, 2023 09:55

Apply review comments

5246351

Add more tests covering type arg list break

6dca754

Revert test adjustment

604db40

CyrusNajmabadi reviewed Sep 29, 2023

View reviewed changes

CyrusNajmabadi approved these changes Sep 29, 2023

View reviewed changes

Rekkonnect added 2 commits September 30, 2023 00:53

Review comments

80dc680

Merge branch 'main' into generic-method-error-recovery

c5bcc66

jaredpar added this to the 17.9 milestone Oct 2, 2023

build-analysis bot mentioned this pull request Oct 5, 2023

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Closed

3 tasks

jcouv approved these changes Jan 30, 2024

View reviewed changes

jcouv requested a review from 333fred January 30, 2024 18:36

jaredpar modified the milestones: 17.9, 17.11 Apr 1, 2024

333fred reviewed Apr 1, 2024

View reviewed changes

Rekkonnect added 2 commits April 2, 2024 13:48

Merge branch 'main' into generic-method-error-recovery

0f2180c

Tests + nits

dbceb8e

Rekkonnect requested a review from 333fred April 2, 2024 15:42

333fred approved these changes Apr 2, 2024

View reviewed changes

333fred merged commit 399051e into dotnet:main Apr 2, 2024

Rekkonnect deleted the generic-method-error-recovery branch April 2, 2024 16:32

dotnet-policy-service bot modified the milestones: 17.11, Next Apr 2, 2024

This was referenced Apr 13, 2024

[Automated] PRs inserted in VS build main-34812.241 #73011

Closed

[Automated] PRs inserted in VS build feature.debugger.main-34815.163 #73037

Closed

dibarbet modified the milestones: Next, 17.11 P1 Apr 29, 2024

Conversation

Rekkonnect commented Aug 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Aug 28, 2023

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CyrusNajmabadi Sep 29, 2023 • edited by jcouv 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!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi commented Oct 1, 2023

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jaredpar commented Apr 1, 2024

Uh oh!

333fred left a comment

Choose a reason for hiding this comment

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!

333fred commented Apr 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Rekkonnect commented Aug 27, 2023 •

edited

Loading

CyrusNajmabadi Sep 29, 2023 •

edited by jcouv

Loading