[API Proposal]: RequiresUnsafeAttribute(bool requiresUnsafe) ctor

[EgorBo]

Background and motivation

We plan to ship analyzers and code fixers that will issue warnings for various patterns, such as APIs with unmanaged pointers in their signatures, LibraryImport attributes, and extern methods (among others). We need a better mechanism than #pragma warning to inform the analyzer that an API is safe. For example, all Math extern methods are pretty safe to require unsafe context on call sites:

#pragma warning disable IL5005 // whatever id for "'extern' must be [RequiresUnsafe]"
        [MethodImpl(MethodImplOptions.InternalCall)]
        public static extern double Sin(double a);
#pragma warning restore IL5005

with this API proposal it can be just:

        [RequiresUnsafe(false)]
        [MethodImpl(MethodImplOptions.InternalCall)]
        public static extern double Sin(double a);

API Proposal

namespace System.Diagnostics.CodeAnalysis
{
    public sealed class RequiresUnsafeAttribute : Attribute
    {
+       public RequiresUnsafeAttribute(bool requiresUnsafe = true) { }
    }
}

NOTE: since we've just introduced the attribute, I suspect it's not a breaking change to "remove" the default constructor.

API Usage

        [RequiresUnsafe(false)]
        [MethodImpl(MethodImplOptions.InternalCall)]
        public static extern double Sin(double a);

        [RequiresUnsafe] // 'true' by default
        [LibraryImport(RuntimeHelpers.QCall)]
        private static partial void _AddMemoryPressure(ulong bytesAllocated);

Alternative Designs

Alternatively, can be a new attribute with the opposite meaning, e.g.:

namespace System.Diagnostics.CodeAnalysis
{
    public sealed class DoesNotRequireUnsafeAttribute : Attribute { }
}

The new attribute would be simpler for us as it won't require changes in C# spec/Roslyn.

Risks

No response

[dotnet-policy-service]

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

[EgorBo]

cc @jkotas @agocke @tannergooding

[EgorBo]

Note: That e.g. in #125196 several APIs were "reverted" by hands to remain safe (e.g. IntPtr) - it means if the code fixer is invoked again - they'll be marked again, so we need a way to stop that: either pragma or this ctor.

[jkotas]

The new attribute would be simpler for us as it won't require changes in C# spec/Roslyn.

It depends on whether Roslyn is going to have hardcoded policy for defaults. If it is going to have hardcoded defaults, it will need to be aware of this attribute to override the default value produced by the policy.

If we are going to have hardcoded policy for defaults in Roslyn, we want RequiresUnsafe with bool argument. Also, I am wondering whether it would make sense to default to RequiresUnsafe for any method with unmanaged pointers in the signature as well in this case - aligns with historical C# precedent.

If we are not going to have hardcoded policy for defaults in Roslyn, a separate attribute like DoesNotRequireUnsafeAttribute may make more sense.

[jkotas]

cc @jjonescz

[jjonescz]

it won't require changes in C# spec/Roslyn.

Roslyn currently only recognizes parameterless RequiresUnsafe constructor, so if you remove that, the one with the default parameter won't be recognized. Even if you keep that one, the new [RequiresUnsafe(true)] wouldn't be recognized by Roslyn.

[EgorBo]

it won't require changes in C# spec/Roslyn.

Roslyn currently only recognizes parameterless RequiresUnsafe constructor, so if you remove that, the one with the default parameter won't be recognized. Even if you keep that one, the new [RequiresUnsafe(true)] wouldn't be recognized by Roslyn.

So which option would you prefer and how long would it take to change the behavior to respect the parameter?
IMO, [RequiresUnsafe(true)] option is preferrable vs yet another attribute.

[jjonescz]

So which option would you prefer and how long would it take to change the behavior to respect the parameter?

Given roslyn needs to support the new overload anyway, we can remove the old one. Sounds like a straightforward change too, should be easy to implement. I just need to know the final decision (this includes the decision whether roslyn should implicitly mark extern methods as RequiresUnsafe or not).

And of course this needs a spec change too. This new shape of the attribute wasn't seen by LDM. Maybe we should mark the attribute Experimental to allow us to discuss it in LDM and change in .NET 12 (if needed) when the whole feature is supposed to be released?

[jcouv]

We need a better mechanism than #pragma warning to inform the analyzer that an API is safe.

It would be good to clarify that OP assumes that extern does not implicit produce [RequiresUnsafe], which is a recent development and not yet confirmed with LDM.

Now to the question at hand: We already have a mechanism to indicate that unsafety is encapsulated/handled rather than propagated. It's the unsafe keyword.
Such use of unsafe on a signature reads a bit funny, but we already accepted that and it is consistent which the new meaning of unsafe under new memory safety rules.

Using unsafe modifier (rather than [RequiresUnsafe(false)]) works regardless of the default behavior we end up deciding for extern. In the view where extern is unsafe by default (ie. implies [RequiresUnsafe]), unsafe extern would not imply [RequiresUnsafe]. In the view where extern is safe, the analyzer would push users to either put [RequiresUnsafe] or unsafe.

[jkotas]

the analyzer would push users to either put [RequiresUnsafe] or unsafe.

Using unsafe keyword to mark P/Invoke as safe feels really counterintuitive.

[RenderMichael]

Since the unsafe keyword is being expanded anyway, what if it becomes another scope block the same way the recently-added extension is?

unsafe
{
    [MethodImpl(MethodImplOptions.InternalCall)]
    public static extern double Sin(double a);

    [LibraryImport(RuntimeHelpers.QCall)]
    private static partial void _AddMemoryPressure(ulong bytesAllocated);
}