-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Improve error recovery for incomplete attribute in a type-parameter. #81231
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 error recovery for incomplete attribute in a type-parameter. #81231
Conversation
- Add ValidAttributeTargets property to SyntaxContext base class - Implement ComputeValidAttributeTargets in CSharpSyntaxContext - Update IsOrContainsAccessibleAttribute to filter by AttributeTargets - Add comprehensive tests for attribute filtering scenarios Co-authored-by: CyrusNajmabadi <[email protected]>
Added lazy caching using nullable AttributeTargets field to compute the value only once per CSharpSyntaxContext instance. Co-authored-by: CyrusNajmabadi <[email protected]>
…xtensions/ContextQuery/CSharpSyntaxContext.cs
- Added WorkItem attribute to all attribute filtering tests - Added tests for all syntax node types (struct, interface, enum, delegate, record, constructor, property, field, event, parameter, type parameter, indexer) - Added tests for all target specifiers (assembly, module, type, method, field, property, event, param, return, typevar) - Simplified AttributeUsageAttribute lookup using nested property pattern - Simplified constructor argument check using collection pattern - Changed target identifier check to use SyntaxKind instead of string literals Co-authored-by: CyrusNajmabadi <[email protected]>
…ing-with-compiler
|
@dotnet/roslyn-compiler this is ready for review. |
| return false; | ||
|
|
||
| // possible attributes | ||
| if (this.CurrentToken.Kind == SyntaxKind.OpenBracketToken && this.PeekToken(1).Kind != SyntaxKind.CloseBracketToken) |
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.
this blocks good error recovery in the Foo<[]T> case. There isn't a good reason for this, as [] isn't a suitable start of some other construct. In all other attribute cases, running into [] is supported, just not for type params.
This simply removes that restriction, and gives consistent support for attributes here like everywhere else.
AlekseyTs
left a comment
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.
LGTM (commit 13)
|
@RikkiGibson ptal :) |
|
@jjonescz ptal :) |
| } | ||
|
|
||
| if (this.IsCurrentTokenWhereOfConstraintClause() || | ||
| this.IsCurrentTokenPartialKeywordOfPartialMemberOrType()) |
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.
It's not obvious why "partial" is getting special treatment. Should we similarly care about other modifier keywords?
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.
partial/where refer to the special identifiers that IsTrueIdentifier returns false for if it sees that these should not actually be treated as identifiers but as contextual keywords.
They're treated specially here as they are very reasonable contextual keywords that would follow a type param being written.
| N(SyntaxKind.EndOfFileToken); | ||
| } | ||
| EOF(); | ||
| } |
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.
Consider testing < public or <[] public
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.
Sure: fd5d3d1
jcouv
left a comment
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 with review pass (commit 13)
…llisense-attribute-filtering-with-compiler
jcouv
left a comment
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.
LGTM Thanks (commit 15)
Followup to #81157