-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Filter IntelliSense attribute suggestions by AttributeTargets #81157
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
Filter IntelliSense attribute suggestions by AttributeTargets #81157
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]>
| this.PrecedingModifiers = precedingModifiers; | ||
| } | ||
|
|
||
| public override AttributeTargets ValidAttributeTargets => ComputeValidAttributeTargets(); |
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 this looks expensive to compute. Can you store this in a AttributeTargets ? in teh class, and compute and cache the value when first requested.
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.
Added lazy caching using a nullable AttributeTargets? field. The value is computed only once per CSharpSyntaxContext instance on first access. (commit a95e595)
| ItemExpectation.Exists("ReturnValueOnly"), | ||
| ItemExpectation.Absent("MethodOnly") | ||
| ]); | ||
| } |
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 have tests for all the possible nodes that you compute different AttributeTargets for. Also, have tests for all the possible target identifiers.
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.
Added comprehensive tests for all syntax node types (struct, interface, enum, delegate, record class, record struct, constructor, property, field, event, parameter, type parameter, indexer) and all target specifiers (assembly, module, type, method, field, property, event, param, return, typevar). Total of 22 new tests added. (commit bd67526)
| ]); | ||
| } | ||
|
|
||
| [Fact] |
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 use [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/7640")] on all tests you add
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.
Added [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/7640")] to all 22 attribute filtering tests. (commit bd67526)
| var attributeUsageAttribute = attributeType.GetAttributes() | ||
| .FirstOrDefault(attr => attr.AttributeClass?.Name == "AttributeUsageAttribute" && | ||
| attr.AttributeClass.ContainingNamespace?.Name == "System" && | ||
| attr.AttributeClass.ContainingNamespace.ContainingNamespace?.IsGlobalNamespace == true); |
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 use a pattern to simplify the name/containingnamespace/etc. check.
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.
Simplified using nested property pattern: attr.AttributeClass is { Name: "AttributeUsageAttribute", ContainingNamespace: { Name: "System", ContainingNamespace.IsGlobalNamespace: true } } (commit bd67526)
| if (attributeUsageAttribute.ConstructorArguments.Length > 0) | ||
| { | ||
| var constructorArg = attributeUsageAttribute.ConstructorArguments[0]; | ||
| if (constructorArg.Value is int targetsValue) |
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 replacce with something like if (attributeUsageAttribute.ConstructorArguments is [{ Value: int targetsValue }, ..]) to simplify things.
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.
Replaced with collection pattern: if (attributeUsageAttribute.ConstructorArguments is [{ Value: int targetsValue }, ..]) (commit bd67526)
Added lazy caching using nullable AttributeTargets field to compute the value only once per CSharpSyntaxContext instance. Co-authored-by: CyrusNajmabadi <[email protected]>
| "param" => AttributeTargets.Parameter, | ||
| "return" => AttributeTargets.ReturnValue, | ||
| "typevar" => AttributeTargets.GenericParameter, | ||
| _ => AttributeTargets.All |
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 you should be able to use SyntaxKind here. Like SyntaxKind.TypeVarKeyword.
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.
Changed to use SyntaxKind enums instead of string literals. Using attributeList.Target.Identifier.Kind() with values like SyntaxKind.AssemblyKeyword, SyntaxKind.TypeVarKeyword, etc. (commit bd67526)
...SharedUtilitiesAndExtensions/Workspace/CSharp/Extensions/ContextQuery/CSharpSyntaxContext.cs
Outdated
Show resolved
Hide resolved
…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]>
| if (this.CurrentToken.Kind == SyntaxKind.OpenBracketToken && this.PeekToken(1).Kind != SyntaxKind.CloseBracketToken) | ||
| return true; | ||
| if (this.CurrentToken.Kind == SyntaxKind.OpenBracketToken) | ||
| { |
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.
i would also be fine simplifying this so that we always return true when we see [ regardless of what follows.
|
@dotnet/roslyn-compiler for parser changes. |
| return _syntaxFactory.GlobalStatement(ParseUnsafeStatement(attributes)); | ||
| } | ||
| break; | ||
| switch (this.CurrentToken.Kind) |
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.
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.
Moved to using IsTrueIdentifier in 13d6ec2. But added handling of some more cases, along with test coverage of some interesting recovery scenarios.
| if (this.PeekToken(1).Kind == SyntaxKind.OpenParenToken) | ||
| { | ||
| return _syntaxFactory.GlobalStatement(ParseFixedStatement(attributes)); | ||
| } |
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.
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.
i checked, and unfortunately we cannot. The reason for this is that ParseTypeParameter is called from inside a call to ParseCommaSeparatedSyntaxList(...requireOneElement: true...). As such, it can be called in teh following case: class C< where T : U. In this case, we unilaterally are calling ParseTypeParameter, even though IsTypeParameter would have returned false.
This is the normal behavior for ParseList(requireOneElement: true) as the intent is to ensure we never have an empy list, and we always get at least that first element by calling into teh parsing function. So the requirement on the parsing function is that it always be safe to call at least once in this sort of case.
|
Done with review pass (commit 8) |
| } | ||
|
|
||
| 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.
tests added for this case.
| // possible attributes | ||
| if (this.CurrentToken.Kind == SyntaxKind.OpenBracketToken && this.PeekToken(1).Kind != SyntaxKind.CloseBracketToken) | ||
| return true; | ||
| if (this.CurrentToken.Kind == SyntaxKind.OpenBracketToken) |
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.
ok. i was actually able to simplify this to just:
// possible attributes.
// Variance.
if (this.CurrentToken.Kind is SyntaxKind.OpenBracketToken or SyntaxKind.InKeyword or SyntaxKind.OutKeyword)
return true;and i got no regressions (in the syntax tests at least). i'm going to go with that much simpler approach as i don't really see a reason why we would accept [ <non-close-brace as an attribute, but not [] as well.
JoeRobich
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.
Signing off on IDE side
…llisense-attribute-filtering
|
Pulled compiler side into a different PR. |
Rebase failed
IntelliSense Attribute Filtering Implementation - COMPLETE ✅
Problem Solved
IntelliSense now filters attribute suggestions based on
AttributeTargetsspecified inAttributeUsageAttribute. Attributes marked with[AttributeUsage(AttributeTargets.Assembly)]only appear in assembly contexts like[assembly: $$], not in class or method contexts.Implementation Complete
Core Changes:
SyntaxContext.cs- AddedValidAttributeTargetsvirtual property (default:AttributeTargets.All)CSharpSyntaxContext.cs- ImplementedComputeValidAttributeTargets()with full syntax analysis and lazy cachingISymbolExtensions.cs- AddedIsAttributeValidForTargets()and new overload for filtering (with pattern matching improvements)AbstractRecommendationService.cs- Integrated ValidAttributeTargets into recommendation flowSymbolCompletionProviderTests.cs- Added comprehensive test scenarios (22 tests total)Performance Optimization:
AttributeTargets?field to compute ValidAttributeTargets only once per context instanceCode Quality Improvements:
Build Status:
Test Coverage (22 comprehensive tests):
Syntax Node Tests:
[assembly: $$])[$$] class)[$$] struct)[$$] interface)[$$] enum)[$$] delegate)[$$] record)[$$] record struct)[$$] void Method())[$$] Constructor())[$$] int Property)[$$] int _field)[$$] event Handler)Method([$$] int p))class C<[$$]T>)[$$] int this[int i])Target Specifier Tests:
17. ✅
[assembly: $$]specifier18. ✅
[module: $$]specifier19. ✅
[type: $$]specifier20. ✅
[method: $$]specifier21. ✅
[field: $$]specifier22. ✅
[property: $$]specifier23. ✅
[event: $$]specifier24. ✅
[param: $$]specifier25. ✅
[return: $$]specifier26. ✅
[typevar: $$]specifierHow It Works
1. Context Detection (CSharpSyntaxContext.ComputeValidAttributeTargets):
2. Attribute Validation (ISymbolExtensions.IsAttributeValidForTargets):
AttributeUsageAttributefrom attribute type using nested property patternAttributeTargetsfrom constructor argument using collection patternAttributeUsagefound3. Filtering (AbstractRecommendationService.ShouldIncludeSymbol):
ValidAttributeTargetsfrom syntax context (cached after first access)IsOrContainsAccessibleAttributeExample Behavior
Before this change:
After this change:
Backward Compatibility
AttributeTargets.All(no filtering)AttributeTargets.All(graceful degradation)Security
Ready for Review
This implementation is complete and ready for review. All code compiles successfully and follows existing Roslyn patterns and conventions.
Original prompt
💡 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.