-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Include [RequiresUnsafe] in PublicAPI signatures
#82731
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2941,6 +2941,155 @@ public async Task TestExperimentalApiWithInvalidArgumentAsync(string invalidArgu | |
| await test.RunAsync(); | ||
| } | ||
|
|
||
| private const string RequiresUnsafeAttributeSource = """ | ||
| namespace System.Runtime.CompilerServices | ||
| { | ||
| internal sealed class RequiresUnsafeAttribute : Attribute { } | ||
| } | ||
| """; | ||
|
|
||
| [Fact] | ||
| public Task TestRequiresUnsafeApiOnMethodAsync() | ||
| => VerifyRequiresUnsafeAdditionalFileFixAsync($$""" | ||
| using System.Runtime.CompilerServices; | ||
|
|
||
| {{EnabledModifierCSharp}} class {|{{AddNewApiId}}:{|{{AddNewApiId}}:C|}|} | ||
| { | ||
| [RequiresUnsafe] | ||
| {{EnabledModifierCSharp}} void {|{{AddNewApiId}}:M|}() { } | ||
| } | ||
| """, @"", @"", """ | ||
| C | ||
| C.C() -> void | ||
| [RequiresUnsafe]C.M() -> void | ||
| """); | ||
|
|
||
| [Fact] | ||
| public Task TestRequiresUnsafeApiOnExternMethodAsync() | ||
| => VerifyRequiresUnsafeAdditionalFileFixAsync($$""" | ||
| using System.Runtime.CompilerServices; | ||
|
|
||
| {{EnabledModifierCSharp}} class {|{{AddNewApiId}}:{|{{AddNewApiId}}:C|}|} | ||
| { | ||
| {{EnabledModifierCSharp}} extern void {|{{AddNewApiId}}:M1|}() { } | ||
| [RequiresUnsafe] | ||
| {{EnabledModifierCSharp}} extern void {|{{AddNewApiId}}:M2|}() { } | ||
| } | ||
| """, @"", @"", """ | ||
| C | ||
| C.C() -> void | ||
| extern C.M1() -> void | ||
| [RequiresUnsafe]extern C.M2() -> void | ||
| """); | ||
|
|
||
| [Fact] | ||
| public Task TestRequiresUnsafeApiOnPropertyAsync() | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a test on accessors. Consider testing [RequiresUnsafe] on property too
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider also having a test for
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This is
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Thanks. |
||
| => VerifyRequiresUnsafeAdditionalFileFixAsync($$""" | ||
| using System.Runtime.CompilerServices; | ||
|
|
||
| {{EnabledModifierCSharp}} class {|{{AddNewApiId}}:{|{{AddNewApiId}}:C|}|} | ||
| { | ||
| [RequiresUnsafe] | ||
| {{EnabledModifierCSharp}} int Property { {|{{AddNewApiId}}:get|}; {|{{AddNewApiId}}:set|}; } | ||
| } | ||
| """, @"", @"", """ | ||
| C | ||
| C.C() -> void | ||
| [RequiresUnsafe]C.Property.get -> int | ||
| [RequiresUnsafe]C.Property.set -> void | ||
| """); | ||
|
|
||
| [Fact] | ||
| public Task TestRequiresUnsafeApiOnPropertyAccessorsAsync() | ||
| => VerifyRequiresUnsafeAdditionalFileFixAsync($$""" | ||
| using System.Runtime.CompilerServices; | ||
|
|
||
| {{EnabledModifierCSharp}} class {|{{AddNewApiId}}:{|{{AddNewApiId}}:C|}|} | ||
| { | ||
| {{EnabledModifierCSharp}} int Property { [RequiresUnsafe] {|{{AddNewApiId}}:get|}; {|{{AddNewApiId}}:set|}; } | ||
| } | ||
| """, @"", @"", """ | ||
| C | ||
| C.C() -> void | ||
| C.Property.set -> void | ||
| [RequiresUnsafe]C.Property.get -> int | ||
| """); | ||
|
|
||
| [Fact] | ||
| public Task TestRequiresUnsafeApiOnEventAsync() | ||
| => VerifyRequiresUnsafeAdditionalFileFixAsync($$""" | ||
| using System; | ||
| using System.Runtime.CompilerServices; | ||
|
|
||
| {{EnabledModifierCSharp}} class {|{{AddNewApiId}}:{|{{AddNewApiId}}:C|}|} | ||
| { | ||
| [RequiresUnsafe] | ||
| {{EnabledModifierCSharp}} event EventHandler {|{{AddNewApiId}}:MyEvent|}; | ||
| } | ||
| """, @"", @"", """ | ||
| C | ||
| C.C() -> void | ||
| [RequiresUnsafe]C.MyEvent -> System.EventHandler | ||
| """); | ||
|
|
||
| private async Task VerifyRequiresUnsafeAdditionalFileFixAsync(string source, string? shippedApiText, string? oldUnshippedApiText, string newUnshippedApiText) | ||
| { | ||
| // The RequiresUnsafeAttribute is defined as internal in test source, so we need to provide | ||
| // internal API files and include the attribute type entries to satisfy the internal API analyzer. | ||
| // We put these entries in the Shipped file so they don't interfere with the Unshipped file diffs. | ||
| var internalApiForAttribute = """ | ||
| System.Runtime.CompilerServices.RequiresUnsafeAttribute | ||
| System.Runtime.CompilerServices.RequiresUnsafeAttribute.RequiresUnsafeAttribute() -> void | ||
| """; | ||
|
|
||
| var test = new CSharpCodeFixTest<DeclarePublicApiAnalyzer, DeclarePublicApiFix, DefaultVerifier>() | ||
| { | ||
| ReferenceAssemblies = ReferenceAssemblies.Net.Net80, | ||
| CompilerDiagnostics = CompilerDiagnostics.None, | ||
| }; | ||
|
|
||
| test.TestState.Sources.Add(source); | ||
| test.TestState.Sources.Add(RequiresUnsafeAttributeSource); | ||
|
|
||
| if (IsInternalTest) | ||
| { | ||
| // For internal tests, ShippedFileName/UnshippedFileName are InternalAPI files. | ||
| // Put the RequiresUnsafeAttribute entries in the shipped file. | ||
| test.TestState.AdditionalFiles.Add((ShippedFileName, internalApiForAttribute)); | ||
| test.TestState.AdditionalFiles.Add((UnshippedFileName, oldUnshippedApiText ?? "")); | ||
| test.TestState.AdditionalFiles.Add((DeclarePublicApiAnalyzer.PublicShippedFileName, "")); | ||
| test.TestState.AdditionalFiles.Add((DeclarePublicApiAnalyzer.PublicUnshippedFileName, "")); | ||
|
|
||
| test.FixedState.Sources.Add(source); | ||
| test.FixedState.Sources.Add(RequiresUnsafeAttributeSource); | ||
| test.FixedState.AdditionalFiles.Add((ShippedFileName, internalApiForAttribute)); | ||
| test.FixedState.AdditionalFiles.Add((UnshippedFileName, newUnshippedApiText)); | ||
| test.FixedState.AdditionalFiles.Add((DeclarePublicApiAnalyzer.PublicShippedFileName, "")); | ||
| test.FixedState.AdditionalFiles.Add((DeclarePublicApiAnalyzer.PublicUnshippedFileName, "")); | ||
| } | ||
| else | ||
| { | ||
| // For public tests, provide internal API files with the attribute entries pre-populated. | ||
| if (shippedApiText != null) | ||
| test.TestState.AdditionalFiles.Add((ShippedFileName, shippedApiText)); | ||
| if (oldUnshippedApiText != null) | ||
| test.TestState.AdditionalFiles.Add((UnshippedFileName, oldUnshippedApiText)); | ||
| test.TestState.AdditionalFiles.Add((DeclarePublicApiAnalyzer.InternalShippedFileName, internalApiForAttribute)); | ||
| test.TestState.AdditionalFiles.Add((DeclarePublicApiAnalyzer.InternalUnshippedFileName, "")); | ||
|
|
||
| test.FixedState.Sources.Add(source); | ||
| test.FixedState.Sources.Add(RequiresUnsafeAttributeSource); | ||
| test.FixedState.AdditionalFiles.Add((ShippedFileName, shippedApiText ?? string.Empty)); | ||
| test.FixedState.AdditionalFiles.Add((UnshippedFileName, newUnshippedApiText)); | ||
| test.FixedState.AdditionalFiles.Add((DeclarePublicApiAnalyzer.InternalShippedFileName, internalApiForAttribute)); | ||
| test.FixedState.AdditionalFiles.Add((DeclarePublicApiAnalyzer.InternalUnshippedFileName, "")); | ||
| } | ||
|
|
||
| test.DisabledDiagnostics.AddRange(DisabledDiagnostics); | ||
|
|
||
| await test.RunAsync(); | ||
| } | ||
|
|
||
| #endregion | ||
| } | ||
| } | ||
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.
Do we have a tracking issue for a public API here?
Uh oh!
There was an error while loading. Please reload this page.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, right. Yes, we do: #82791
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 will update the tracking comment to say that we should use that API when available.