Add parser test demonstrating recovery from missing type arguments in generic names. #81062
Conversation
… argument Co-authored-by: CyrusNajmabadi <[email protected]>
| else | ||
| { | ||
| types.Add(this.ParseTypeArgument()); | ||
| } |
There was a problem hiding this comment.
@copilot switch this to a conditinoal expression like so:
types.Add(this.CurrentToken.Kind == SyntaxKind.CommaToken
? _syntaxFactory.OmittedTypeArgument(SyntaxFactory.Token(SyntaxKind.OmittedTypeArgumentToken))
: this.ParseTypeArgument());
Do the same in the other code change you had below.
There was a problem hiding this comment.
Done in commit cea68f0. Refactored both locations to use conditional expressions.
…progress) Co-authored-by: CyrusNajmabadi <[email protected]>
| { | ||
| M<,int>(); // First argument omitted | ||
| M<string,>(); // Second argument omitted | ||
| M<,int,>(); // First and third arguments omitted (if we had 3 params) |
There was a problem hiding this comment.
@copilot add variants with ,, as well.
Also, we should validate that we get proper binding diagnostics in these cases. We don't want this to accidentally be accepted.
Co-authored-by: CyrusNajmabadi <[email protected]>
| EOF(); | ||
| } | ||
|
|
||
| [Fact] |
There was a problem hiding this comment.
@copilot the new tests (and any tests you modified) should have [Fact, WorkItem("...")] to associate them with the issue being fixed.
For tests that now no longer report a syntax error, we need to make a compilation and verify diagnostics on it to make sure that we are reporting a binding error.
There was a problem hiding this comment.
Done in commit 033c5d5.
Added [WorkItem("https://github.com/dotnet/roslyn/issues/71379")] to:
- The new test:
ParseGenericNameInvocationWithPartiallyOmittedTypeArguments - Modified tests in
SeparatedSyntaxListParsingTests:TypeArguments - Modified tests in
TypeArgumentListParsingTests:TestGenericWithExtraCommasAndMissingTypes1throughTestGenericWithExtraCommasAndMissingTypes8
Also added binding diagnostics validation (via CreateCompilation().VerifyDiagnostics()) to all these tests to ensure proper semantic errors are reported even though syntax errors are eliminated.
Co-authored-by: CyrusNajmabadi <[email protected]>
Co-authored-by: CyrusNajmabadi <[email protected]>
| EOF(); | ||
|
|
||
| // Validate binding diagnostics for cases that now parse without syntax errors | ||
| var source = @" |
There was a problem hiding this comment.
@copilot use raw string literals for all new tests you add.
There was a problem hiding this comment.
Done in commit d4db8a1. Converted the binding diagnostic test source in SeparatedSyntaxListParsingTests.TypeArguments from verbatim string literal to raw string literal and updated all line numbers accordingly.
| Diagnostic(ErrorCode.ERR_SyntaxError, "U").WithArguments(",").WithLocation(11, 9), | ||
| // (8,5): error CS0246: The type or namespace name 'A<,,>' could not be found (are you missing a using directive or an assembly reference?) | ||
| // A<,,> a5; | ||
| Diagnostic(ErrorCode.ERR_SingleTypeNameNotFound, "A<,,>").WithArguments("A<,,>").WithLocation(8, 5), |
There was a problem hiding this comment.
@copilot since these are binding tests now, we should actually ensure that there is a class A<T> (and multiple type arg variants) to make sure that even if we bind to that type, we still fail because of the omitted type arguments.
There was a problem hiding this comment.
The binding diagnostic validation was removed from the new test since the approach changed. The existing tests in TypeArgumentListParsingTests.cs (TestGenericWithExtraCommasAndMissingTypes1-8) already cover these scenarios with the correct parse-time diagnostics. Since we're now avoiding binding changes per the review feedback, the focus is on parse-time error recovery rather than binding validation.
| // Allow for a comma immediately after '<', treating it as an omitted first type argument. | ||
| // This handles cases like `M<,T>` where the first type argument is missing. | ||
| types.Add(this.CurrentToken.Kind == SyntaxKind.CommaToken | ||
| ? _syntaxFactory.OmittedTypeArgument(SyntaxFactory.Token(SyntaxKind.OmittedTypeArgumentToken)) |
There was a problem hiding this comment.
@copilot i don't think we want OmittedTypeArgumentToken here. I htink we want to try to parse out a type (which ideally then gives a good message) and insert that missing tyep in the tree. That way we don't get totally thrown off, but give a good error about something missing. And that way we don't have to worry about binding changes. please make that change here and below, and update all tests accordingly.
There was a problem hiding this comment.
Done in commit e0dbd9c. Reverted to unconditionally calling ParseTypeArgument() instead of creating OmittedTypeArgument nodes. This produces better error messages (ERR_TypeExpected) and inserts missing nodes in the tree for recovery, avoiding binding changes.
Co-authored-by: CyrusNajmabadi <[email protected]>
…ssages Co-authored-by: CyrusNajmabadi <[email protected]>
CyrusNajmabadi
changed the title
|
@dotnet/roslyn-compiler Test only change to demonstrate issue no longer repros. |
Add parser test demonstrating recovery from missing type arguments in generic names.