Skip to content

Fix finding attribute data for syntax for assembly/module symbols #72535

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

Merged
merged 1 commit into from
Jul 20, 2022

Conversation

jkoritzinsky
Copy link
Member

Correctly handle attributes with assembly or module targets, and use 'null' fallback for unknown scenarios.

Unblocks dotnet/sdk and dotnet/aspnetcore intake of dotnet/runtime.

Correctly handle attributes with assembly or module targets, and use 'null' fallback for unknown scenarios.
@jkoritzinsky jkoritzinsky added area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature labels Jul 20, 2022
@ghost
Copy link

ghost commented Jul 20, 2022

Tagging subscribers to this area: @dotnet/interop-contrib
See info in area-owners.md if you want to be subscribed.

Issue Details

Correctly handle attributes with assembly or module targets, and use 'null' fallback for unknown scenarios.

Unblocks dotnet/sdk and dotnet/aspnetcore intake of dotnet/runtime.

Author: jkoritzinsky
Assignees: -
Labels:

area-System.Runtime.InteropServices, source-generator

Milestone: -

Comment on lines +35 to +42
case SyntaxKind.ReturnKeyword:
return ((IMethodSymbol)targetSymbol).GetReturnTypeAttributes().First(attributeSyntaxLocationMatches);
case SyntaxKind.AssemblyKeyword:
return targetSymbol.ContainingAssembly.GetAttributes().First(attributeSyntaxLocationMatches);
case SyntaxKind.ModuleKeyword:
return targetSymbol.ContainingModule.GetAttributes().First(attributeSyntaxLocationMatches);
default:
return null;
Copy link
Member

Choose a reason for hiding this comment

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

I didn't look at the context of the callers or what's possible and what's not.

But there are other attribute targets to consider (only if they're relevant):

https://github.com/dotnet/roslyn/blob/1c480b07dbd55e2dce5580fc6a3e5b468d53c64d/src/Compilers/CSharp/Test/Emit2/Attributes/AttributeTests_Locations.cs#L87-L97

Copy link
Member Author

Choose a reason for hiding this comment

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

The rest of them aren't relevant enough to cover here. Returning null is fine. Technically, we don't need to handle assembly and module as well as this PR does, but I'd rather cleanly handle them than returning null if possible.

If your PR to add IAttributeOperation ever gets merged, we're going to switch to use that API from Roslyn and work with the IOperation tree instead of the syntax tree (and effectively obsolete all usages of this class).

@jkoritzinsky
Copy link
Member Author

Timeouts in the System.Net.Http.Functional test suites are unrelated. Installer test is a known issue (linked automatically by Build Analysis)

@jkoritzinsky jkoritzinsky merged commit ddded2f into dotnet:main Jul 20, 2022
@jkoritzinsky jkoritzinsky deleted the other-target-attributes branch July 20, 2022 21:05
@ghost ghost locked as resolved and limited conversation to collaborators Aug 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime.InteropServices source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants