Include [RequiresUnsafe] in PublicAPI signatures#82731
Include [RequiresUnsafe] in PublicAPI signatures#82731jjonescz wants to merge 3 commits intodotnet:mainfrom
[RequiresUnsafe] in PublicAPI signatures#82731Conversation
| """); | ||
|
|
||
| [Fact] | ||
| public Task TestRequiresUnsafeApiOnPropertyAsync() |
There was a problem hiding this comment.
This is a test on accessors. Consider testing [RequiresUnsafe] on property too
There was a problem hiding this comment.
Consider also having a test for extern method
There was a problem hiding this comment.
This is a test on accessors. Consider testing [RequiresUnsafe] on property too
This is [RequiresUnsafe] on property, but the list of APIs only contains the accessors.
There was a problem hiding this comment.
Consider also having a test for
externmethod
Thanks. extern apparently already is put in the signature by this analyzer. I don't think we need to put the implicit RequiresUnsafe there though. (Sure if someone enables updated memory safety rules, all extern apis become unsafe and that's kind of breaking. But similarly if someone disables updated memory safety rules, all pointer methods become unsafe and that's also not something we present in the signature by this analyzer. Basically enabling/disabling the rules is a break on a different level, not on the API level.)
|
Andy mentioned a bunch of other attributes that should be tracked on public APIs. Should we open a tracking issue? |
|
@333fred for another review, thanks |
| { | ||
| foreach (var attribute in symbol.GetAttributes()) | ||
| { | ||
| if (attribute.AttributeClass is { Name: "RequiresUnsafeAttribute", ContainingSymbol: INamespaceSymbol { Name: "CompilerServices", ContainingNamespace: { Name: "Runtime", ContainingNamespace: { Name: nameof(System), ContainingNamespace.IsGlobalNamespace: true } } } }) |
There was a problem hiding this comment.
Do we have a tracking issue for a public API here?
There was a problem hiding this comment.
Thanks, let me add this follow up comment (if that's what you meant).
// https://github.com/dotnet/roslyn/issues/82546: Confirm the attribute shape in BCL API review.There was a problem hiding this comment.
No, I mean an api for not needing to filter for an attribute and just check an api on the symbol.
There was a problem hiding this comment.
I will update the tracking comment to say that we should use that API when available.
Feels like a feature request that the runtime team should create if they want, I don't think I know all the details. |
3 participants
Test plan: #81207
Microsoft Reviewers: Open in CodeFlow