Skip to content

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.

Sign up for GitHub

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

Open
jjonescz wants to merge 4 commits into
base: main
Choose a base branch
from
+178 −3
Open

Include [RequiresUnsafe] in PublicAPI signatures #82731

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c309d5d
Include `[RequiresUnsafe]` in PublicAPI signatures
jjonescz Mar 12, 2026
4e123d7
Extend tests
jjonescz Mar 13, 2026
75e6271
Add public API shape follow up
jjonescz Mar 17, 2026
6a4ea7a
Add a follow up comment to use the public API
jjonescz Mar 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions src/RoslynAnalyzers/PublicApiAnalyzers/Core/Analyzers/DeclarePublicApiAnalyzer.Impl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,10 +594,11 @@ private static bool UsesOblivious(ISymbol symbol)
private ApiName GetApiName(ISymbol symbol)
{
var experimentName = getExperimentName(symbol);
var requiresUnsafe = getRequiresUnsafe(symbol);

return new ApiName(
getApiString(_compilation, symbol, experimentName, s_publicApiFormat),
getApiString(_compilation, symbol, experimentName, s_publicApiFormatWithNullability));
getApiString(_compilation, symbol, experimentName, requiresUnsafe, s_publicApiFormat),
getApiString(_compilation, symbol, experimentName, requiresUnsafe, s_publicApiFormatWithNullability));

static string? getExperimentName(ISymbol symbol)
{
Expand Down Expand Up @@ -625,7 +626,25 @@ private ApiName GetApiName(ISymbol symbol)
return null;
}

static string getApiString(Compilation compilation, ISymbol symbol, string? experimentName, SymbolDisplayFormat format)
static bool getRequiresUnsafe(ISymbol symbol)
{
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 } } } })
Copy link
Member

@333fred 333fred Mar 16, 2026

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?

Copy link
Member Author

@jjonescz jjonescz Mar 17, 2026 •
edited
Loading

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.

Copy link
Member

@333fred 333fred Mar 17, 2026

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.

Copy link
Member Author

@jjonescz jjonescz Mar 17, 2026

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

Copy link
Member Author

@jjonescz jjonescz Mar 17, 2026

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.

{
return true;
}
}

if (symbol is IMethodSymbol { AssociatedSymbol: { } associatedSymbol })
{
return getRequiresUnsafe(associatedSymbol);
}

return false;
}

static string getApiString(Compilation compilation, ISymbol symbol, string? experimentName, bool requiresUnsafe, SymbolDisplayFormat format)
{
string publicApiName = symbol.ToDisplayString(format);

Expand Down Expand Up @@ -667,6 +686,11 @@ static string getApiString(Compilation compilation, ISymbol symbol, string? expe
publicApiName = "[" + experimentName + "]" + publicApiName;
}

if (requiresUnsafe)
{
publicApiName = "[RequiresUnsafe]" + publicApiName;
}

return publicApiName;
}
}
Expand Down
115 changes: 115 additions & 0 deletions src/RoslynAnalyzers/PublicApiAnalyzers/UnitTests/DeclarePublicAPIAnalyzerTestsBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2941,6 +2941,121 @@ 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 TestRequiresUnsafeApiOnPropertyAsync()
Copy link
Member

@jcouv jcouv Mar 12, 2026

Choose a reason for hiding this comment

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

This is a test on accessors. Consider testing [RequiresUnsafe] on property too

Copy link
Member

@jcouv jcouv Mar 12, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Consider also having a test for extern method

Copy link
Member Author

@jjonescz jjonescz Mar 13, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@jjonescz jjonescz Mar 13, 2026

Choose a reason for hiding this comment

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

Consider also having a test for extern method

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.)

=> 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 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
}
}
Loading