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

Fix parsing of generics in casts. #69519

Merged
merged 8 commits into from
Aug 15, 2023
Merged

Fix parsing of generics in casts. #69519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e3c972a
initial ideal
CyrusNajmabadi Aug 15, 2023
f740a66
Fix parsing of generics in casts.
CyrusNajmabadi Aug 15, 2023
547de9c
Simplify
CyrusNajmabadi Aug 15, 2023
054b157
REvert
CyrusNajmabadi Aug 15, 2023
370627d
Merge remote-tracking branch 'upstream/main' into genericWork
CyrusNajmabadi Aug 15, 2023
c7f5197
Add test. Revert code change
CyrusNajmabadi Aug 15, 2023
d4ac003
Apply suggestions from code review
CyrusNajmabadi Aug 15, 2023
9bd7a2a
Merge remote-tracking branch 'upstream/main' into genericWork
CyrusNajmabadi Aug 15, 2023
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
20 changes: 9 additions & 11 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 @@ -5824,7 +5824,7 @@ private ScanTypeFlags ScanPossibleTypeArgumentList(
// Note: we check if we got 'MustBeType' which triggers for predefined types,
// (int, string, etc.), or array types (Goo[], A<T>[][] etc.), or pointer types
// of things that must be types (int*, void**, etc.).
isDefinitelyTypeArgumentList = DetermineIfDefinitelyTypeArgumentList(isDefinitelyTypeArgumentList);
isDefinitelyTypeArgumentList = isDefinitelyTypeArgumentList || this.CurrentToken.Kind is SyntaxKind.CommaToken or SyntaxKind.GreaterThanToken;
Copy link
Member Author

Choose a reason for hiding this comment

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

i inlined this method as i found it easier to reason about just looking at teh check directly in code.

Copy link
Member Author

Choose a reason for hiding this comment

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

note: tehre are further changes i'd like to make to this method for clarity/correctness/invariants. but i'm trying to keep the PR small for the purposes of fixing this important issue.

result = ScanTypeFlags.GenericTypeOrMethod;
break;

Expand All @@ -5850,8 +5850,8 @@ private ScanTypeFlags ScanPossibleTypeArgumentList(
// }

case ScanTypeFlags.NullableType:
// See above. If we have X<Y?, or X<Y?>, then this is definitely a type argument list.
isDefinitelyTypeArgumentList = DetermineIfDefinitelyTypeArgumentList(isDefinitelyTypeArgumentList);
// See above. If we have `X<Y?,` or `X<Y?>` then this is definitely a type argument list.
isDefinitelyTypeArgumentList = isDefinitelyTypeArgumentList || this.CurrentToken.Kind is SyntaxKind.CommaToken or SyntaxKind.GreaterThanToken;
if (isDefinitelyTypeArgumentList)
{
result = ScanTypeFlags.GenericTypeOrMethod;
Expand Down Expand Up @@ -5900,17 +5900,15 @@ private ScanTypeFlags ScanPossibleTypeArgumentList(
}

greaterThanToken = this.EatToken();
return result;
}

private bool DetermineIfDefinitelyTypeArgumentList(bool isDefinitelyTypeArgumentList)
{
if (!isDefinitelyTypeArgumentList)
// If we have `X<Y>)` then this would definitely be a type argument list.
isDefinitelyTypeArgumentList = isDefinitelyTypeArgumentList || this.CurrentToken.Kind is SyntaxKind.CloseParenToken;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you help me understand in which scenarios we are depending on this line versus line 5874 for correct behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

so 5874 related to incomplete nested generics. so if you had (X<Y<Z>) (note the lack of the last >) .

This line applies when you have (X<Y>) or (X<Y<Z>>)

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks!

if (isDefinitelyTypeArgumentList)
{
isDefinitelyTypeArgumentList = this.CurrentToken.Kind is SyntaxKind.CommaToken or SyntaxKind.GreaterThanToken;
result = ScanTypeFlags.GenericTypeOrMethod;
}

return isDefinitelyTypeArgumentList;
return result;
}

// ParseInstantiation: Parses the generic argument/parameter parts of the name.
Expand Down Expand Up @@ -11798,7 +11796,7 @@ private bool ScanCast(bool forPattern = false)

this.EatToken();

var type = this.ScanType(forPattern: forPattern);
var type = this.ScanType(forPattern);
if (type == ScanTypeFlags.NotType)
{
return false;
Expand Down
122 changes: 122 additions & 0 deletions src/Compilers/CSharp/Test/Syntax/Parsing/CollectionExpressionParsingTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// See the LICENSE file in the project root for more information.

using Microsoft.CodeAnalysis.CSharp.Test.Utilities;
using Roslyn.Test.Utilities;
using Xunit;
using Xunit.Abstractions;

Expand Down Expand Up @@ -5656,6 +5657,127 @@ public void CastVersusIndexAmbiguity30()
EOF();
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/69508")]
public void CastVersusIndexAmbiguity31()
{
UsingStatement("var x = (A<B>)[1];");

N(SyntaxKind.LocalDeclarationStatement);
{
N(SyntaxKind.VariableDeclaration);
{
N(SyntaxKind.IdentifierName);
{
N(SyntaxKind.IdentifierToken, "var");
}
N(SyntaxKind.VariableDeclarator);
{
N(SyntaxKind.IdentifierToken, "x");
N(SyntaxKind.EqualsValueClause);
{
N(SyntaxKind.EqualsToken);
N(SyntaxKind.CastExpression);
{
N(SyntaxKind.OpenParenToken);
N(SyntaxKind.GenericName);
{
N(SyntaxKind.IdentifierToken, "A");
N(SyntaxKind.TypeArgumentList);
{
N(SyntaxKind.LessThanToken);
N(SyntaxKind.IdentifierName);
{
N(SyntaxKind.IdentifierToken, "B");
}
N(SyntaxKind.GreaterThanToken);
}
}
N(SyntaxKind.CloseParenToken);
N(SyntaxKind.CollectionExpression);
{
N(SyntaxKind.OpenBracketToken);
N(SyntaxKind.ExpressionElement);
{
N(SyntaxKind.NumericLiteralExpression);
{
N(SyntaxKind.NumericLiteralToken, "1");
}
}
N(SyntaxKind.CloseBracketToken);
}
}
}
}
}
N(SyntaxKind.SemicolonToken);
}
EOF();
}

[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/69508")]
public void CastVersusIndexAmbiguity31_GlobalStatement()
{
UsingTree("var x = (A<B>)[1];");

N(SyntaxKind.CompilationUnit);
{
N(SyntaxKind.GlobalStatement);
{
N(SyntaxKind.LocalDeclarationStatement);
{
N(SyntaxKind.VariableDeclaration);
{
N(SyntaxKind.IdentifierName);
{
N(SyntaxKind.IdentifierToken, "var");
}
N(SyntaxKind.VariableDeclarator);
{
N(SyntaxKind.IdentifierToken, "x");
N(SyntaxKind.EqualsValueClause);
{
N(SyntaxKind.EqualsToken);
N(SyntaxKind.CastExpression);
{
N(SyntaxKind.OpenParenToken);
N(SyntaxKind.GenericName);
{
N(SyntaxKind.IdentifierToken, "A");
N(SyntaxKind.TypeArgumentList);
{
N(SyntaxKind.LessThanToken);
N(SyntaxKind.IdentifierName);
{
N(SyntaxKind.IdentifierToken, "B");
}
N(SyntaxKind.GreaterThanToken);
}
}
N(SyntaxKind.CloseParenToken);
N(SyntaxKind.CollectionExpression);
{
N(SyntaxKind.OpenBracketToken);
N(SyntaxKind.ExpressionElement);
{
N(SyntaxKind.NumericLiteralExpression);
{
N(SyntaxKind.NumericLiteralToken, "1");
}
}
N(SyntaxKind.CloseBracketToken);
}
}
}
}
}
N(SyntaxKind.SemicolonToken);
}
}
N(SyntaxKind.EndOfFileToken);
}
EOF();
}

[Fact]
public void SpreadOfQuery()
{
Expand Down