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

Fix parsing of generics in casts. #69519

Merged
merged 8 commits into from
Aug 15, 2023
Merged

Fix parsing of generics in casts. #69519

merged 8 commits into from
Aug 15, 2023

Conversation

CyrusNajmabadi
Copy link
Member

Fixes #69508

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner August 15, 2023 16:31
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Aug 15, 2023
CyrusNajmabadi
CyrusNajmabadi commented Aug 15, 2023
View reviewed changes
src/Compilers/CSharp/Portable/Parser/LanguageParser.cs
@@ -5824,7 +5824,7 @@ private ScanTypeArgumentListKind ScanTypeArgumentList(NameOptions options)
// 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.

@CyrusNajmabadi
Copy link
Member Author

@RikkiGibson @cston ptal asap.

@RikkiGibson RikkiGibson self-assigned this Aug 15, 2023
@CyrusNajmabadi CyrusNajmabadi requested a review from cston August 15, 2023 19:06
@CyrusNajmabadi
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

RikkiGibson
RikkiGibson approved these changes Aug 15, 2023
View reviewed changes
src/Compilers/CSharp/Portable/Parser/LanguageParser.cs
{
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!

@CyrusNajmabadi CyrusNajmabadi enabled auto-merge (squash) August 15, 2023 21:57
@CyrusNajmabadi CyrusNajmabadi merged commit 0877df3 into dotnet:main Aug 15, 2023
@ghost ghost added this to the Next milestone Aug 15, 2023
@CyrusNajmabadi CyrusNajmabadi deleted the genericWork branch August 16, 2023 02:04
@allisonchou allisonchou mentioned this pull request Aug 16, 2023
Closed
@dibarbet dibarbet modified the milestones: Next, 17.8 P2 Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RikkiGibson RikkiGibson RikkiGibson approved these changes

@cston cston cston approved these changes

Assignees

@RikkiGibson RikkiGibson

Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Milestone
17.8 P2
Development

Successfully merging this pull request may close these issues.

Collection expressions need an unnecessary brace if a cast expression is a generic type with a type alias
4 participants
@CyrusNajmabadi @RikkiGibson @cston @dibarbet