Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve generic type argument list error recovery #69734

Merged
merged 29 commits into from
Apr 2, 2024
Merged

Improve generic type argument list error recovery #69734

Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
d7e9b38
Improve error recovery on missing > in method
Rekkonnect Aug 26, 2023
b4d81da
Add many more tests
Rekkonnect Aug 27, 2023
508a9c8
Recovery on properties
Rekkonnect Aug 27, 2023
1b5ff5b
Recovery on tuples and more tests
Rekkonnect Aug 27, 2023
54c64dd
Adjust tests
Rekkonnect Aug 27, 2023
4c92d12
Fix type argument list termination and tests
Rekkonnect Aug 28, 2023
a830762
Move another test
Rekkonnect Aug 28, 2023
bd3f3bf
Better recovery logic and another test
Rekkonnect Aug 28, 2023
a6d0ffe
Adjust test after improvement
Rekkonnect Aug 28, 2023
5246351
Apply review comments
Rekkonnect Aug 29, 2023
6dca754
Add more tests covering type arg list break
Rekkonnect Aug 29, 2023
604db40
Revert test adjustment
Rekkonnect Aug 29, 2023
8218e34
More comments and improve tests
Rekkonnect Sep 8, 2023
5a25a48
Recover removed test
Rekkonnect Sep 8, 2023
e3ed56a
Merge branch 'main' into generic-method-error-recovery
Rekkonnect Sep 8, 2023
5935734
Formatting expected diagnostics
Rekkonnect Sep 8, 2023
cb19e46
Review comments
Rekkonnect Sep 8, 2023
0349424
Add a lot more tests
Rekkonnect Sep 9, 2023
ef2a978
Add `this` and `operator`
Rekkonnect Sep 9, 2023
13194f0
Add comments for terminating tokens
Rekkonnect Sep 10, 2023
491d86b
Remove test failures for bad recoveries
Rekkonnect Sep 13, 2023
c71ea51
Fix spelling
Rekkonnect Sep 13, 2023
fa72e62
Handle pattern matching keywords + tests
Rekkonnect Sep 13, 2023
9601aa6
Fix tests
Rekkonnect Sep 15, 2023
7b0f635
Add more tests in method declarations
Rekkonnect Sep 15, 2023
80dc680
Review comments
Rekkonnect Sep 29, 2023
c5bcc66
Merge branch 'main' into generic-method-error-recovery
Rekkonnect Sep 30, 2023
0f2180c
Merge branch 'main' into generic-method-error-recovery
Rekkonnect Apr 2, 2024
dbceb8e
Tests + nits
Rekkonnect Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 87 additions & 3 deletions src/Compilers/CSharp/Portable/Parser/LanguageParser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5759,6 +5759,7 @@ private ScanTypeFlags ScanPossibleTypeArgumentList(
}

ScanTypeFlags result = ScanTypeFlags.GenericTypeOrExpression;
ScanTypeFlags lastScannedType = ScanTypeFlags.NotType;
333fred marked this conversation as resolved.
Show resolved Hide resolved

do
{
Expand All @@ -5777,7 +5778,8 @@ private ScanTypeFlags ScanPossibleTypeArgumentList(
return result;
}

switch (this.ScanType(out _))
lastScannedType = this.ScanType(out _);
switch (lastScannedType)
{
case ScanTypeFlags.NotType:
greaterThanToken = null;
Expand Down Expand Up @@ -5876,6 +5878,25 @@ private ScanTypeFlags ScanPossibleTypeArgumentList(

if (this.CurrentToken.Kind != SyntaxKind.GreaterThanToken)
{
// Error recovery after missing > token:

// In the case of an identifier, we assume that there could be a missing > token
// For example, we have reached C in X<A, B C
if (this.CurrentToken.Kind is SyntaxKind.IdentifierToken)
{
greaterThanToken = this.EatToken(SyntaxKind.GreaterThanToken);
return result;
}

// As for tuples, we do not expect direct invocation right after the parenthesis
// EXAMPLE: X<(string, string)(), where we imply a missing > token between )(
// as the user probably wants to invoke X by X<(string, string)>()
if (lastScannedType is ScanTypeFlags.TupleType && this.CurrentToken.Kind is SyntaxKind.OpenParenToken)
{
greaterThanToken = this.EatToken(SyntaxKind.GreaterThanToken);
return result;
}
Rekkonnect marked this conversation as resolved.
Show resolved Hide resolved

greaterThanToken = null;
return ScanTypeFlags.NotType;
}
Expand Down Expand Up @@ -5922,11 +5943,35 @@ private void ParseTypeArgumentList(out SyntaxToken open, SeparatedSyntaxListBuil
// remaining types & commas
while (true)
{
if (this.CurrentToken.Kind == SyntaxKind.GreaterThanToken)
if (this.CurrentToken.Kind is SyntaxKind.GreaterThanToken)
break;
333fred marked this conversation as resolved.
Show resolved Hide resolved

// We prefer early terminating the argument list over parsing until exhaustion
// for better error recovery
if (tokenBreaksTypeArgumentList(this.CurrentToken))
break;
333fred marked this conversation as resolved.
Show resolved Hide resolved

// We are currently past parsing a type and we encounter an unexpected identifier token
// followed by tokens that are not part of a type argument list
// Example: List<(string a, string b) Method() { }
// current token: ^^^^^^
if (this.CurrentToken.Kind is SyntaxKind.IdentifierToken && tokenBreaksTypeArgumentList(this.PeekToken(1)))
{
break;
}
else if (this.CurrentToken.Kind == SyntaxKind.CommaToken || this.IsPossibleType())

// This is for the case where we are in a this[] accessor, and the last one of the parameters in the parameter list
// is missing a > on its type
// Example: X this[IEnumerable<string parameter] =>
// current token: ^^^^^^^^^
if (this.CurrentToken.Kind is SyntaxKind.IdentifierToken
&& this.PeekToken(1).Kind is SyntaxKind.CloseBracketToken
&& tokenBreaksTypeArgumentList(this.PeekToken(2)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need the tokenBreaksTypeArgumentList(this.PeekToken(2)) check here? Is identifier + close bracket not enough of a check?

{
break;
}

if (this.CurrentToken.Kind == SyntaxKind.CommaToken || this.IsPossibleType())
{
types.AddSeparator(this.EatToken(SyntaxKind.CommaToken));
types.Add(this.ParseTypeArgument());
Expand All @@ -5938,6 +5983,45 @@ private void ParseTypeArgumentList(out SyntaxToken open, SeparatedSyntaxListBuil
}

close = this.EatToken(SyntaxKind.GreaterThanToken);

static bool tokenBreaksTypeArgumentList(SyntaxToken token)
{
var contextualKind = SyntaxFacts.GetContextualKeywordKind(token.ValueText);
switch (contextualKind)
{
case SyntaxKind.OrKeyword:
case SyntaxKind.AndKeyword:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

case SyntaxKind.NotKeyword:
return true;
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Sep 29, 2023 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return token.Kind
// Example: IEnumerable<string Method<T>()
is SyntaxKind.LessThanToken
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is confusing because it's unclear which less than token you're referring to.

// Example: Method<string(argument)
or SyntaxKind.OpenParenToken
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Example: Method(IEnumerable<string parameter)
or SyntaxKind.CloseParenToken
// Example: IEnumerable<string field;
or SyntaxKind.SemicolonToken
// Example: IEnumerable<string Property { get; set; }
or SyntaxKind.OpenBraceToken
// Example:
// {
// IEnumerable<string field
// }
or SyntaxKind.CloseBraceToken
// Examples:
// - IEnumerable<string field = null;
// - Method(IEnumerable<string parameter = null)
or SyntaxKind.EqualsToken
// Example: IEnumerable<string Property => null;
or SyntaxKind.EqualsGreaterThanToken
// Example: IEnumerable<string this[string key] { get; set; }
or SyntaxKind.ThisKeyword
// Example: static IEnumerable<string operator +(A left, A right);
or SyntaxKind.OperatorKeyword;
}
}

private PostSkipAction SkipBadTypeArgumentListTokens(SeparatedSyntaxListBuilder<TypeSyntax> list, SyntaxKind expected)
Expand Down
25 changes: 8 additions & 17 deletions src/Compilers/CSharp/Test/Semantic/Semantics/NullableReferenceTypesTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151967,27 +151967,18 @@ public override void M1<T>(C<System.Nullable<T?> x)
class C<T> {}
");
comp.VerifyDiagnostics(
// (11,32): error CS0305: Using the generic type 'C<T>' requires 1 type arguments
// (11,26): error CS0115: 'B.M1<T>(C<T??>)': no suitable method found to override
// public override void M1<T>(C<System.Nullable<T?> x)
Diagnostic(ErrorCode.ERR_BadArity, "C<System.Nullable<T?> x").WithArguments("C<T>", "type", "1").WithLocation(11, 32),
// (11,54): error CS1003: Syntax error, ',' expected
Diagnostic(ErrorCode.ERR_OverrideNotExpected, "M1").WithArguments("B.M1<T>(C<T??>)").WithLocation(11, 26),
// (11,54): error CS1003: Syntax error, '>' expected
// public override void M1<T>(C<System.Nullable<T?> x)
Diagnostic(ErrorCode.ERR_SyntaxError, "x").WithArguments(",").WithLocation(11, 54),
// (11,54): error CS0246: The type or namespace name 'x' could not be found (are you missing a using directive or an assembly reference?)
Diagnostic(ErrorCode.ERR_SyntaxError, "x").WithArguments(">").WithLocation(11, 54),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's going on here? What was C<System.Nullable<T?> x previously parsed as? And what is it now parsed as?

Copy link
Contributor Author

@Rekkonnect Rekkonnect Sep 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not mistaken, this was previously being parsed as having a missing comma before the x, with the type argument list closing on the ) with a missing > token and a missing identifier on the parameter. Therefore, it was parsing:

public override void M1<T>(C<System.Nullable<T?>, x> @identifier)

We are now parsing this with x being the identifier name, since we look ahead, and thus parse the following:

public override void M1<T>(C<System.Nullable<T?>> x)

// (11,54): error CS0453: The type 'T?' must be a non-nullable value type in order to use it as parameter 'T' in the generic type or method 'Nullable<T>'
// public override void M1<T>(C<System.Nullable<T?> x)
Diagnostic(ErrorCode.ERR_SingleTypeNameNotFound, "x").WithArguments("x").WithLocation(11, 54),
// (11,55): error CS1003: Syntax error, '>' expected
Diagnostic(ErrorCode.ERR_ValConstraintNotSatisfied, "x").WithArguments("System.Nullable<T>", "T", "T?").WithLocation(11, 54),
// (11,54): error CS0453: The type 'T' must be a non-nullable value type in order to use it as parameter 'T' in the generic type or method 'Nullable<T>'
// public override void M1<T>(C<System.Nullable<T?> x)
Diagnostic(ErrorCode.ERR_SyntaxError, ")").WithArguments(">").WithLocation(11, 55),
// (11,55): error CS1001: Identifier expected
// public override void M1<T>(C<System.Nullable<T?> x)
Diagnostic(ErrorCode.ERR_IdentifierExpected, ")").WithLocation(11, 55),
// (11,55): error CS0453: The type 'T?' must be a non-nullable value type in order to use it as parameter 'T' in the generic type or method 'Nullable<T>'
// public override void M1<T>(C<System.Nullable<T?> x)
Diagnostic(ErrorCode.ERR_ValConstraintNotSatisfied, "").WithArguments("System.Nullable<T>", "T", "T?").WithLocation(11, 55),
// (11,55): error CS0453: The type 'T' must be a non-nullable value type in order to use it as parameter 'T' in the generic type or method 'Nullable<T>'
// public override void M1<T>(C<System.Nullable<T?> x)
Diagnostic(ErrorCode.ERR_ValConstraintNotSatisfied, "").WithArguments("System.Nullable<T>", "T", "T").WithLocation(11, 55)
Diagnostic(ErrorCode.ERR_ValConstraintNotSatisfied, "x").WithArguments("System.Nullable<T>", "T", "T").WithLocation(11, 54)
);
}

Expand Down
Loading