Skip to content

Better parse when lambda expressions contain default parameter values.#58690

Merged
CyrusNajmabadi merged 22 commits intodotnet:mainfrom
CyrusNajmabadi:lambdaDefaultValue
Jan 11, 2022
Merged

Better parse when lambda expressions contain default parameter values.#58690
CyrusNajmabadi merged 22 commits intodotnet:mainfrom
CyrusNajmabadi:lambdaDefaultValue

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

Followup to #58644

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner January 6, 2022 22:07
@ghost ghost added the Area-Compilers label Jan 6, 2022
@RikkiGibson RikkiGibson self-assigned this Jan 6, 2022
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

@RikkiGibson this is ready for review.

Copy link
Copy Markdown
Member

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

That was easy!

// it. However, that should never happen as ScanParenthesizedLambda allows `!!` only.
Debug.Assert(equalsToken == null);
// If we didn't already consume an equals sign as part of !!=, then try to scan one out now. Note: this is
// not legal code. But we detect it so that we can give the user a good message, and so we don't completely
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"don't go completely"?

@RikkiGibson
Copy link
Copy Markdown
Member

@dotnet/roslyn-compiler for a second review on a relatively simple change

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner January 11, 2022 18:20
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

@dotnet/roslyn-compiler for small parser change that produces better error recovery results. thanks!

Comment on lines +13119 to +13121
// This allows us to handle this code without ever showing it the binding phases. We could consider
// actually binding this in the future if it would be helpful and if we're ok paying the testing cost of
// checking this at the semantic layers.
Copy link
Copy Markdown
Contributor

@cston cston Jan 11, 2022

Choose a reason for hiding this comment

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

Consider dropping the end of the comment - we typically bind code the user wrote, even in error situations, so the cost shouldn't be an issue.

Suggested change
// This allows us to handle this code without ever showing it the binding phases. We could consider
// actually binding this in the future if it would be helpful and if we're ok paying the testing cost of
// checking this at the semantic layers.
// This allows us to handle this code without showing it to the binding phases. We could consider
// actually binding this in the future.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

so the cost shouldn't be an issue.

In this case, i mentioned that it's "testing cost" :) In other words, i'd like to have the better error recovery here, but i don't really want to write the tons of semantic tests that validate this :)

@cston
Copy link
Copy Markdown
Contributor

cston commented Jan 11, 2022

                    attributes, modifiers.ToList(), type: null, this.EatToken(SyntaxKind.ArgListKeyword), exclamationExclamationToken: null, @default: null);

I'm not sure it's worth handling __arglist!! and __arglist = value. The resulting code seems more complicated for a very unlikely scenario.


Refers to: src/Compilers/CSharp/Portable/Parser/LanguageParser.cs:4395 in 0613817. [](commit_id = 0613817, deletion_comment = True)

Co-authored-by: Charles Stoner <10732005+cston@users.noreply.github.com>
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

I'm not sure it's worth handling __arglist!! and __arglist = value. The resulting code seems more complicated for a very unlikely scenario.

It didn't bother me to support. I'm a fan of the parser not going off the rails for sensible constructs. I do agree it's exceedingly unlikely for this code to happen though (who even uses __arglist), so i can revert this though if you'd like. :)

Copy link
Copy Markdown
Contributor

@ryzngard ryzngard left a comment

Choose a reason for hiding this comment

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

Test update lgtm

@CyrusNajmabadi CyrusNajmabadi merged commit 12be03b into dotnet:main Jan 11, 2022
@ghost ghost added this to the Next milestone Jan 11, 2022
@CyrusNajmabadi CyrusNajmabadi deleted the lambdaDefaultValue branch February 1, 2022 18:25
@RikkiGibson RikkiGibson modified the milestones: Next, 17.2.P1 Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants