-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Improve error messages for keywords used as identifiers in LINQ let clauses #80695
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
base: main
Are you sure you want to change the base?
Conversation
Modified ParseLetClause to detect keywords followed by '=' and use EatTokenEvenWithIncorrectKind to produce ERR_IdentifierExpectedKW error instead of ERR_IdentifierExpected, reducing cascading errors. Co-authored-by: CyrusNajmabadi <[email protected]>
Improved code readability by extracting the condition into a well-named helper method as suggested in code review. Co-authored-by: CyrusNajmabadi <[email protected]>
this.ParseExpressionCore()); | ||
} | ||
|
||
private bool IsReservedKeywordFollowedByEquals() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot inline this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit fe41692 (inlined the method directly into the if condition).
SyntaxToken identifier; | ||
if (IsReservedKeywordFollowedByEquals()) | ||
{ | ||
var keyword = this.EatTokenEvenWithIncorrectKind(SyntaxKind.IdentifierToken); | ||
identifier = ConvertToIdentifier(keyword); | ||
} | ||
else | ||
{ | ||
identifier = this.ParseIdentifierToken(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot conver this to a simple conditional expression. placce the ?
and :
on new lines with the following expresison on the same line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 570ed57 - converted to ternary operator with ?
and :
on new lines.
Address code review feedback by inlining the helper method directly into the if condition in ParseLetClause. Co-authored-by: CyrusNajmabadi <[email protected]>
if (IsReservedKeywordFollowedByEquals()) | ||
{ | ||
var keyword = this.EatTokenEvenWithIncorrectKind(SyntaxKind.IdentifierToken); | ||
identifier = ConvertToIdentifier(keyword); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot instead of this pair, use EatTokenAsKind instead. We want a missing token here with the keyword attached as a skipped token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 570ed57 - now using EatTokenAsKind
which creates a missing identifier token with the keyword attached as skipped syntax.
- Converted if/else to ternary operator with ? and : on new lines - Changed from EatTokenEvenWithIncorrectKind + ConvertToIdentifier to EatTokenAsKind for better error recovery - Updated test to expect missing identifier token with keyword as skipped syntax Co-authored-by: CyrusNajmabadi <[email protected]>
{ | ||
Debug.Assert(this.CurrentToken.ContextualKind == SyntaxKind.LetKeyword); | ||
var letKeyword = this.EatContextualToken(SyntaxKind.LetKeyword); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Roslyn disallows blank lines with spaces on them. Please update the copilot contributing document to mention that this should be avoided as it will cause linting errors that will have to be fixed up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in commit 2ce564d. Fixed the blank lines with spaces in ParseLetClause and updated .github/copilot-instructions.md
to include a note about this requirement under the Formatting section.
- Removed spaces from blank lines in ParseLetClause (lines 13908, 13914) - Updated .github/copilot-instructions.md to warn about blank lines with whitespace causing linting errors Co-authored-by: CyrusNajmabadi <[email protected]>
Co-authored-by: Joey Robichaud <[email protected]>
@copilot QueryWithMultipleParseErrorsAndScriptParseOption is failing now (since parsing improved in that test case). Can you determine why the test is failing and update the expected results? |
Updated the test to expect the complete query expression 'from i in expr1 let namespace = expr1 select i' instead of the truncated 'from i in expr1 let ' since the parser now recovers better and continues parsing after encountering the keyword. Also removed redundant line from copilot-instructions.md formatting section. Co-authored-by: CyrusNajmabadi <[email protected]>
Done in commit 9406664. The test was failing because the parser now recovers better - instead of stopping after "let ", it continues parsing to "from i in expr1 let namespace = expr1 select i". Updated the test to expect the complete query expression. |
@dotnet/roslyn-compiler this is ready fro review. |
Fix error messages for using a keyword instead of an identifier in LINQ let clauses
Issue Summary
When using a reserved keyword (like
params
) as an identifier in a LINQlet
clause, the compiler previously produced many confusing error messages (8-10 errors). The goal was to:Implementation
Modified
LanguageParser.ParseLetClause()
to:=
(the expected pattern for let clause) using a conditional expressionEatTokenAsKind
to create a missing identifier token with the keyword attached as skipped syntaxThis results in just 1 clear error message instead of 8-10 confusing ones.
Changes Made
src/Compilers/CSharp/Portable/Parser/LanguageParser.cs
:ParseLetClause
method using conditional (ternary) expression with?
and:
on new linesEatTokenAsKind
to create missing identifier token with keyword as skipped syntaxsrc/Compilers/CSharp/Test/Syntax/Parsing/ExpressionParsingTests.cs
:LetClauseWithKeywordAsIdentifier
src/Compilers/CSharp/Test/Semantic/Semantics/QueryTests.cs
:QueryWithMultipleParseErrorsAndScriptParseOption
test to expect improved parsing.github/copilot-instructions.md
:Test Results
Before/After
Before: Using
params
in a let clause produced 8+ errors including CS1001, CS1003, CS1525, CS0742, etc.After: Using
params
in a let clause produces 1 clear CS1041 error: "Identifier expected; 'params' is a keyword"Fixes #10446
Original prompt
Fixes #11418
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.