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.

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

Improve location that lambda errors are reported at. #69334

Merged
merged 21 commits into from
Aug 5, 2023

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Aug 2, 2023

Fixes #69322

This changes the place we report lambda/anonymous-delegate errors to be on the => and delegate token of each construct respectively, instead of reporting the error on the entire expression.

Lambdas are effectively unbounded in size, and reporting such a large error is both poor for the user experience (as it can squiggle a huge range of the buffer), it also often masks more important issues for the user to be aware of (like the actual error in the parameters or body that is causing the high level lambda problem).

@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 2, 2023
@CyrusNajmabadi CyrusNajmabadi changed the title WIP: Improve location that lambda errors are reported at. Improve location that lambda errors are reported at. Aug 2, 2023
@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review August 3, 2023 19:55
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner August 3, 2023 19:56
@CyrusNajmabadi
Copy link
Member Author

@dotnet/roslyn-compiler this is ready for review.

IAnonymousFunctionOperation (Symbol: lambda expression) (OperationKind.AnonymousFunction, Type: null, IsInvalid) (Syntax: '(int i) => { }')
IBlockOperation (0 statements) (OperationKind.Block, Type: null, IsInvalid) (Syntax: '{ }')
IBlockOperation (0 statements) (OperationKind.Block, Type: null) (Syntax: '{ }')
Copy link
Member

@jcouv jcouv Aug 4, 2023

Choose a reason for hiding this comment

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

I didn't follow, what caused the IOperation changes?

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Aug 4, 2023

Choose a reason for hiding this comment

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

Iop adds the isinvalid bit if the node overlaps a semantic diagnostic. Because the diagnostic got smaller, less nodes are marked as invalid.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Nice :-) Done with review pass (iteration 14). Only minor questions

@jcouv jcouv self-assigned this Aug 4, 2023
@CyrusNajmabadi CyrusNajmabadi requested a review from jcouv August 4, 2023 15:53
Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 18)

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner August 4, 2023 18:38
@CyrusNajmabadi
Copy link
Member Author

@dotnet/roslyn-compiler for a second pair of eyes. Thanks!

@CyrusNajmabadi CyrusNajmabadi disabled auto-merge August 4, 2023 19:00
@CyrusNajmabadi CyrusNajmabadi enabled auto-merge (squash) August 4, 2023 19:00
@CyrusNajmabadi CyrusNajmabadi merged commit 093fb4c into dotnet:main Aug 5, 2023
@ghost ghost added this to the Next milestone Aug 5, 2023
@CyrusNajmabadi CyrusNajmabadi deleted the lambdaErrorLocation branch August 5, 2023 03:48
Rekkonnect added a commit to Rekkonnect/roslyn that referenced this pull request Aug 5, 2023
@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
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve places where CS1661 is reported
4 participants