LibraryImport generated stubs should be implicitly RequiresUnsafe#125802
LibraryImport generated stubs should be implicitly RequiresUnsafe#125802
Conversation
|
Tagging subscribers to this area: @dotnet/area-meta |
Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
…urce Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
|
What's the rationale behind every |
…for regular stubs Co-authored-by: jkoritzinsky <1571408+jkoritzinsky@users.noreply.github.com>
|
LibraryImport methods don't resolve all of the reasons that DllImports are unsafe. Primarily, LibraryImport (like DllImport) cannot guarantee that the provided signature is an accurate match to the target native method. We could introduce a mechanism to request that a LibraryImport is safe (basically having the user promise that they got it right) but until then LibraryImport doesn't solve all of the reasons that DllImport requires unsafe. |
Of course. But this is swinging the pendulum to the other extreme and saying that because we don't know what's on the other end of the FFI, every single consumer must assume every single LibraryImport is unsafe. Is this what was agreed on with @agocke et al? My expectation is this is going to lead to developers wrapping [LibraryImport]s with yet another stub that serves purely to contain the virality of the |
I actually don't think it fundamentally resolves any of the reasons that That is, they remain equivalent to a
I think this is just the expectation of all unsafe code and is why all We expect the wrapper to exist to make the assertion that there is no memory unsafe operations occurring. That all the potential unsafety of crossing the managed -> unmanaged boundary is handled. Since a LibraryImport can make no more guarantees of this than any other extern method, we should likely treat them the same as well. |
|
If I were writing the wrapper that LibraryImport is writing, then I could choose to do so in a way that stops the virality, by using But I'm not writing the wrapper, the library import generator is. And there's nothing being exposed that let's me do the equivalent of moving the unsafety into the body. Which means I'm the stuck having to write yet another wrapper around its wrapper. I'm not sold on the notion that because we don't know what the thing being called is doing it's inherently unsafe. But assuming I buy that premise, we should at least make it trivial to annotate the [LibraryImport] in such a way that the virality is contained so I don't have to write yet another wrapper. |
I do not think that's the plan. FCalls and other similar methods in CoreLib implemented using runtime magic are not going to be implicitly unsafe. We can have analyzers that tell you to mark it as unsafe that you can suppress as needed (or have I think similar strategy can work for DllImport/LibraryImport too. |
The language has implemented it such that all That is, at the IL level functionally any method which is not abstract and has a null method body (i.e. is If we wanted a way to annotate that
The exact same logic applies to |
Then we can have the same switch for DllImport as well. I don't know why the same logic applying to DllImport means we can't have nice things.
Which is why I'm highlighting it. It's the thing we tell developers to use now, so it's the thing we should focus on ensuring has a good experience. |
@jjonescz I thought that we agreed on that the compiler is just going to look at RequiresUnsafeAttribute only, and it won't try to be smart about I do not want the runtime to be dealing with the problems created by safe wrappers over |
|
I'm not against being able to mark I think that escaping managed control (which from a user visible perspective is what
I'm not convinced there is a "good experience", or at least not an experience that helps lead users to a pit of success here. In my experience people do not write correct interop bindings regardless of what approach they use. Even tools like cswin32 have regularly gotten things wrong and have to have the various edges called out and fixed. This also applies to the runtime (one of the reasons we eventually created the LibraryImport generator), and other well known tools that are believed to be "robust". Rather, I find that devs are likely to assume that using Additionally, due to the way |
|
Tanner's right on the current status of the language feature: extern methods are effectively [RequiresUnsafe]. I also agree that it seems like LibraryImport doesn't meet the currently described rules for when it's OK to suppress the unsafety -- it hasn't discharged the validation obligation. That's still on the user. However, I also agree w/ Stephen that it seems like it would be nice to have a simple gesture to say "this is fine". My suggestion in my team meeting today was, "LibraryImport can have a Safe = true property that people can set which will automatically suppress the warnings". This wasn't a very popular position 😆 (too ugly). But it seems like a cheap way to mark things safe would be nice. Open to suggestions. |
Developers (or AI agents) will look for the path-of-least-resistance to suppressing the failures. I expect all we'll be doing by forcing them to write such a wrapper is increasing their annoyance and the amount of code they need to maintain.
Not necessarily. LibraryImport could start emitting wrappers in such cases; it doesn't mean DllImport would need the same mechanism. I don't really care whether DllImport has the same mechanism or not. What I care about is use of LibraryImport being made even harder than it already is, and forcing developers to write wrapper methods is making it harder than it already is. |
|
(The Comment button is way too close to the Close with comment button.) |
Since it'll already be everywhere in this system, I like Jan's suggestion of just using what we'll already have: (I still don't agree though that [LibraryImport]s should all be implicitly unsafe.) |
We have split the discharging of the obligations into two independent decisions:
There is nothing in the C# language that enforces that these two decisions are coherent. It is going to be on the human review aided by analyzers or AI to make sure that it is right. extern methods do not have C# implementation so (2) does not apply to them. Given that (1) and (2) are indepedent, (1) should not depend on how the method is implemented and thus it should not be different between methods implemented in C# vs. method implemented via runtime magic.
IMHO, the |
Yes, we discussed this and the conclusion was that we are not going to be smart about |
|
Seeing the fallout of the design, I think we want to revisit it. make the compiler even less smart and be more like latest Rust:
|
Opened dotnet/csharplang#10051 for discussion.
That sounds like libraries-only change, right? Compiler wouldn't need that for anything. |
From the doc linked (my emphasis):
So if the idea is to make it like rust, the changes recommended in dotnet/csharplang#10051 should have a way to mark as "safe" somehow, and error (or warn) if not marked TL;DR, shouldn't the PR have some way to specify "safe" is what I'm saying. |
This does not need to be part of the language spec. It can be left to the analyzers. |
Wouldn't it be better to be in the language? Analysers are optional to run and have to be opted-in (in some shape or form) last time I checked (maybe I am wrong here though?). Or would this be a special one that runs always regardless of settings when using new unsafe? Also, imo, having |
What about pinvokes/LibraryImports public APIs from pre-MemorySafetyRulesAttribute compiled assemblies? Do we treat them as unsafe when we call them from new code? |
Maybe? If we were to make the analyzer-like rules part of the language, we would want to make all of them part of the language. For example, method with unmanaged pointers in the signature are safe by default as far as C# is concerned with the current proposal. We are going to have an analyzer that warns you about marking the method as RequiresUnsafe. |
I personally would have liked to see it be an error to be unspecified (whether safe or unsafe) if one of the known attributes is applied (which should be done in a user-joinable system imo, e.g., via some other attribute on the attribute type, or similar). But I suppose if just a warning is preferred, then I guess having it as an analyser works as well as anything else. Also, I personally would think that adding something like |
I thought this analyzer was about providing a fixer to preserve existing behavior when enabling new rules and not some blanket warning on all code about "you have a pointer in the signature, you should mark it I'm still personally not in favor of requiring users to annotate extern methods themselves to indicate they're unsafe either. It being implicit with an explicit opt-out feels much better (and more closely mirrors the rust changes called out). I strongly believe it effectively being "presumed safe by default" will be a big pit of failure for typical users, knowing how they've handled unsafe code, interop, and other concepts for the last 25 years. |
It makes the language spec simple. It is the only reason why we have done it this way. I care about default end-to-end experience that the analyzers are an integral part of. The default end-to-end experience needs to warn about safe methods with pointers in the signatures (unless they are explicitly marked as safe in some way). Dtto for extern methods. |
I would think it makes more sense to do it based on whether the method has an
The difference for extern methods and pointer parameters, is that to do anything unsafe with a pointer parameter, you need to have |
|
It feels like we should really have some Arguably the biggest part of the end to end experience is how users writing unsafe code today will see the feature and how they have to think about things when enabling it. If we are forcing them to This feels like the design and lesser pit of failure is that we believe the majority of pointers are still unsafe and so they should be unsafe by default. With users annotating the few exceptions as being safe when they are. The same would then be my expectation of how |
There is a warning. You either do what it says, or use pragma/msbuild property to silence it -- isn't this the normal UX? |
No - normal UX for getting "is it unsafe or not" today is adding unsafe keyword somewhere with an error to force you, not adding a pragma to work around a warning. I can tell you now that if it overwarns a lot and the only solution is a pragma, I will probably disable it project-wide making it pointless (as I do with most things that overwarn a lot), and if it underwarns a lot, people will use/interpret it as proof that "it's not unsafe, see it warns I should mark it as |
Presumably, you will not be able to disable it if at some point (when the feature is mature enough) the warning is promoted to an error. |
I mean, if I can't disable it via NoWarn, then I also can't disable it via pragma, and it's probably not an analyser. Meaning a different way will have to be added, something like what @tannergooding has suggested. |
That is the normal UX for warnings yes. I'm talking about there being an issue in creating an experience where the default when enabling the feature is the dev having to suppress tons of warnings, which is counterintuitive. That is, this feels like something where we're changing semantics of the language and then immediately going "no the new language semantics are probably incorrect". This creates confusion and expects users to annotate with Given the most popular binding libraries are fairly large, I expect this is going to be a large number of suppressions. Enough so I imagine many users will consider simply not enabling the feature or will just disable the analyzer instead to help keep their code clean and maintainable. This simply feels like a pit of failure in the design. I would think a significantly better, but not too different setup would be:
This makes it so that the language and tooling are not in disagreement. It helps push users into a pit of success by matching the likely case as the default. It provides an easy to read, write, search, and understand experience for (ideally minority of) cases that do deviate from the default. It does not provide an environment that encourages mass suppression or avoidance of enabling the feature. |
|
Put another and maybe simpler way. We reject many analyzers due to the risk of their "false positive" ratio being too high. This feels like we're trying to push a design that is maximizing false positive warnings for users how have been writing unsafe code for the last 25 years. |
I expect that the default flow to enable new unsafe is going to be enable the feature and run the auto-fixer. The auto-fixer will add annotations as necessary to make your project compile, and adds a bunch of TODOs for you to go through and review manually.
The unsafe evolution is breaking by design. It is different from nearly all other features where we try to minimize the breaks. Our instincts are honed around building non-breaking features. We should watch for our instincts failing us here and leading to poor design choices. |
|
The design we had landed on seemed like a good middle-ground, but some of the points around the analyzer behavior and suggestions for changing what
I expect this will have a lot of negative feedback from the devs we want to start using the feature to annotate their own unsafe code. I believe it will be completely unactionable in a repo the size of The rules also make it difficult for alternative tooling, like
It's much less breaking than it was originally and is much closer in design to We are not changing the semantics of the I think the feature really comes down to two aspects:
I then predict that those in camp 1 are going to need to enable the feature and likely be unaware what makes things unsafe and possibly even how to handle that unsafety. In many cases they were likely using those APIs to avoid And then those in camp 2 are going to want to enable the feature and generally have an idea of what needs to be done. They are likely to want to start by keeping things exactly as they are today for their own exposed surface area and to simply add I expect:
|
Sure, but I do not see how we get the current unsafe mess sorted out. If folks do not want to do the work to opt in into unsafe v2, they can do so - but it is likely going to mean that their package is going to be viewed as less trustworthy over time. |
|
I think there's some confusion on Rust's model as of 2024, so let me put in some more detail: This is the current state: unsafe extern "C" {
// sqrt (from libm) may be called with any `f64`
pub safe fn sqrt(x: f64) -> f64;
// strlen (from libc) requires a valid pointer,
// so we mark it as being an unsafe fn
pub unsafe fn strlen(p: *const std::ffi::c_char) -> usize;
// this function doesn't say safe or unsafe, so it defaults to unsafe
pub fn free(p: *mut core::ffi::c_void);
pub safe static IMPORTANT_BYTES: [u8; 256];
}Every "extern" block, which is where you declare extern stubs, now requires the Then, each of the stubs may have
The stub may also be If the stub is unspecified, it's assumed that the implementation may have important preconditions, i.e. it's unsafe. If we were to try to map this to our current model I think it would look something like this: All extern functions must have Extern methods could also be marked as We could decide differently on any of these points, of course, but I believe this is the closest mapping between the C# model and the Rust model. |
I think that this model of Once we solve how to represent safe/unsafe for |
|
Wrt Rust 2024 model: I believe that the motivation for the change was that the unsafe annotation is in your face for externs. The mechanics are less interesting, they leverage the dual purpose of If we make externs RequiresUnsafe by default, we may be copying some of the Rust mechanics, but we are missing the actual point since RequiresUnsafe won't be in your face. |
|
@jkotas it's not clear to me which part you're commenting on. In the Rust model the new requirement about How do you envision the C# model differing? Is it simply the lack of an |
The main issue I see is not accounting enough for the practical experience users in both camps will encounter when turning on this feature. I think most of this is resolved by addressing 2 main points.
The thing that seems simplest to me to address these issues, without overly complicating the analyzer/fixer and user experience, is that we provide both We then provide three fixers. The first fixer is about annotating members with the attribute. It would provide the below 3 options. This then doesn't result in unnecessary warnings to user code, ensures everything that is likely unsafe is definitively attributed, and lets users pick what is most appropriate for their codebase. We could optionally separate this between
The second fixer is then about adding The final fixer is about helping users move towards the recommended/best practices around unsafe and likely needs to be controlled via some
I'm actually doubtful of this, for the same reason that's its not caused packages which didn't opt into features like NRT to be viewed this way. That is to say there are several large/popular packages that still haven't enabled NRT and most users simply do not care or are not significantly impacted. Libraries that haven't opted-in will continue having their historical experience, which means that consumers of these libraries won't see any new negatives and so they'll have no real reason to push for these libraries to enable the feature. They may not even realize that a package hasn't opted in. So while we'll opt the core libraries in and many packages will also opt in, I fully expect that several large libraries/packages will simply not if the UX and cost to benefit ratio isn't there to justify it. The historical experience is also largely correct or "good enough" for most code. Unsafe code is safe when used safely after all and many bugs are quickly found. The point of this feature is to help highlight unsafety users might not be aware about which can help expose latent bugs or other issues. -- Much as NRT is about helping surface latent issues where users are incorrectly passing in null or not handling a potential null return. |
[LibraryImport]stubs call native code whose signature the compiler cannot validate, making them inherently unsafe. Generated stubs were not annotated with[RequiresUnsafe], causing inconsistency with forwarder stubs (raw[DllImport]extern methods) and making the unsafe nature invisible to tools like ILLink'sRequiresUnsafeAnalyzer.Changes
RequiresUnsafeAttribute→publicsrc/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/RequiresUnsafeAttribute.cs:internal→publicso generated user code can reference itsrc/libraries/System.Runtime/ref/System.Runtime.cs: Added to reference assembly for API compatGenerator infrastructure
StubEnvironment: AddedRequiresUnsafeAttrTypelazy lookup propertyEnvironmentFlags: AddedRequiresUnsafeAvailable = 0x4flagTypeNames/NameSyntaxes: AddedSystem_Diagnostics_CodeAnalysis_RequiresUnsafeAttributeconstant and syntax helperLibraryImportGeneratorstub emissionCalculateStubInformation: SetsRequiresUnsafeAvailableflag when attribute type is available in the compilation. Skips if the user already applied[RequiresUnsafe]on their declaration (prevents CS0579 duplicate attribute error).GenerateSource: Injects[RequiresUnsafe]intoSignatureContext.AdditionalAttributesfor regular (marshalling) stubs.PrintForwarderStub: Explicitly adds[RequiresUnsafe]to forwarder (pure DllImport) stubs.The flag is the single source of truth for both stub paths. Availability is checked at compile time via
Compilation.GetTypeByMetadataName, so older TFMs without the attribute are handled gracefully.Tests
Added
RequiresUnsafeAddedandRequiresUnsafeAddedOnForwardingStubtoAdditionalAttributesOnStub.cs, verifying the attribute is emitted on both stub kinds with the expectedglobal::qualified syntax.Original prompt
📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.