runtime: Add [CallerMustBeUnsafe] attribute to denote APIs which should be called in an unsafe block
Per https://github.com/dotnet/roslyn-analyzers/issues/972, there’s an outstanding feature for a Roslyn analyzer which would require a caller to use an unsafe block to make an API call that’s pointer-equivalent dangerous but which doesn’t take pointers.
For example:
public static Span<T> ToMutableSpan(ReadOnlySpan<T> span)
{
// When the Roslyn analyzer is active, the following code will produce a warning
// unless it is wrapped within an "unsafe" block.
return MemoryMarshal.CreateSpan(ref Unsafe.AsRef(in MemoryMarshal.GetReference(span)), span.Length);
}
Whether that feature exists as a standalone analyzer or whether it gets moved into Roslyn proper (see https://github.com/dotnet/roslyn/issues/8663), we still need to define the attribute in corefx and apply it to the appropriate methods.
API proposal
namespace System.Runtime.CompilerServices
{
[AttributeUsage(AttributeTargets.Field | AttributeTargets.Method | AttributeTargets.Property, AllowMultiple = false, Inherited = true)]
public sealed class CallerMustBeUnsafeAttribute : Attribute
{
}
}
In this proposal I’m not applying AttributeTargets.Class or AttributeTargets.Struct because I don’t want constructs like Type t = typeof(SomeType) to require an unsafe block. Technically AttributeTargets.Property isn’t needed because the property getter / setter can be annotated (same with events), but allowing it on properties seems like a decent convenience.
The prime candidates to annotate are most methods on the MemoryMarshal type, some constructor-bypassing logic in FormatterServices, and any “fast” object factories which allow bypassing normal constructor validation.
Edit: I suppose since we’ve said that we see ArrayPool<T> as a malloc equivalent and since it now returns uninitialized memory we should annotate its methods as well, but due to how widely used that type is that risks opening a huge can of worms.
About this issue
- Original URL
- State: open
- Created 5 years ago
- Reactions: 25
- Comments: 16 (15 by maintainers)
Commits related to this issue
- Proposal for fixing unsafe — committed to jaredpar/csharplang by jaredpar 3 years ago
How about
[UnsafeToCall]?Discussion on this topic in Discord just now came up with names like
[RequiresUnsafeContext]and[UnsafeCallersOnly](the latter having nice parity with the recent[UnmanagedCallersOnly]).I’m actually liking
[UnsafeCallersOnly]for that reason, so I’m putting my vote in that hat.Note the cited RegexGenerator.Emitter.cs already does that for a few warnings; this would “just” be one (or more) more. https://github.com/dotnet/runtime/blob/b5bb3c9e5e0cb0ddfe99eec38e66ac1dc8f1b57f/src/libraries/System.Text.RegularExpressions/gen/RegexGenerator.Emitter.cs#L30-L39
That said, I also opened an issue about trying to drive that list to zero 😄 https://github.com/dotnet/runtime/issues/61666
I also think that mixing language-safe/unsafe concepts with API safe/unsafe is not very good.
The
unsafekeyword in C# denotes that you agree to loose promises that the C# language gives you otherwise, thus being able to use additional syntax and language features. It means that the compiler cannot protect you at language level that what you do is okay.Now there are a lot of APIs that do things that can break you in recoverable and non-recoverable ways. For methods that shift sanity-checking on the programmer, these have been traditionally called
UnsafeXYZor are on theUnsafeclass. But nearly everything that goes a little bit deeper than traditional safe APIs can end up bad (e.g. function pointers returned by the marshaller with the referenced delegate already been garbage collected).Mixing the language concern with the API concern is theoretically possible, but then it needs to be done for all sorts of APIs and possibly still can’t protect you from the results of using these APIs outside of unsafe contexts; think modifying a mutable span of a string - you would need an unsafe block to do that casts but if you later on use that mutable span elsewhere to do modifications you might be causing bad things to happen (and hours spent debugging because this will look like spooky action at a difference with interned strings). Many programmers work on the same project and if one opts into some unsafe actions for implementing a safe api, it doesn’t mean that other programmers using safe APIs won’t then be able to run into issues using this safe api in a different way.
Another concern of mine is that an “you need ‘unsafe’ here” diagnostic will only make people enable unsafe blocks and use the language feature without thinking too much about it. Because that will be the StackOverflow answer explaining to you how to use that method. No thinking about how that will impact your program involved.
Regarding naming: Should this be exclusive to C# or is there an expectation that the presence of a
CallerMustBeUnsafeattribute has an effect (produces diagnostic) on languages that do not have a language-level concept of „unsafe“ like C#?If not so, then I’d favor naming it agnostic to language constructs - e.g.
[UnsafeOperation]/[MemoryUnsafeOperation]- that could have semantically meaningful diagnostics and resolutions („suppress warning about calling members that perform unsafe operations“) regardless of the language used. So only the C# analyzer would require ausingblock and/or AllowUnsafeBlocks options while analyzers / linters for other languages could use the same message suppression mechanism as they would for any other diagnostic.Either excluding generated code, or just having the generator throw the pragma out saying it promises that it knows what it’s talking about, seem fine.
The safety of an API may also varies. For example
Unsafe.As, it can be super dangerous likeAs<T[], SZArrayHelper<T>>, or super safe likeAs<float, int>And some of the Unsafe methods?