[Proposal]: Change ref safety errors to warnings in unsafe contexts

[jaredpar]

Change ref safety errors to warnings inside unsafe

• Proposed
• Prototype: Not Started
• Implementation: Not Started
• Specification: Not Started

Summary

The language will downgrade ref errors to a warning inside an unsafe context.

Motivation

The set of ref and features in C# are concerned with providing type safe access to memory references. This includes ref returns, ref fields, in, etc ... The features are very successful in providing the expressiveness required by our API authors and developers. The analysis though is naturally very conservative. Essentially there are many scenarios in which ref could be used safely but it is flagged as an error because the conservative nature of our analysis does not understand the use case.

Today this results in a hard error being produced and the language contains no escape hatch. Instead when developers confront these cases they must rely on runtime APIs. For example Unsafe.AsRef as a way to work around these limitations.

Relying on runtime APIs has a number of significant drawbacks:

• They cannot be relied on in multi-targeting scenarios. When a new API in net7.0 to support a C# 11 feature that cannot be used when multi-targeting between net7.0 and say net6.0.
• They require non-trivial JIT work. The APIs cannot be implemented in C# code. It must be implemented as a codegen / JIT trick and done so such that it's a wash. The API intent is to really have no impact on the final code, it's an annotation.
• APIs cannot easily express some aspects of the language. Consider for example that Unsafe.AsRef does not work for a ref struct and designing an API that does is virtually impossible due to lack of generic support.
• It hides the actual problems. These APIs effectively turn off language analysis so there
• It's ugly to read long streams of Unsafe.AsRef calls

The language already has a mechanism to allow for type and memory safety violations: unsafe context. It is already possible to violate memory safety using unsafe that would allow for exactly the same type of problems that ref safety means to prevent. When the language is viewed in its totality it's inconsistent that we allow effectively unlimited memory safety violations with pointers (where we have no tracking of what you can do) but don't allow for a much more constrained violation (when compiler can help identify the exact points and reasons for unsafety).

Downgrading ref safety errors to warnings allows for an escape hatch for developers. It still requires three levels of acknowledgement of the safety issues: /unsafe compiler switch, unsafe context and suppression of the emitted diagnostic. It also means curious developers can quickly see what violations exist by removing the warning suppressions.

Detailed design

The language will identify the errors which fall into the ref memory safety category. For each which "break glass in case of emergency" is required the compilre will provide a warning to pair with the error. In the case the diagnostic occurs in unsafe the warning will be used instead of the error.

Initially the error codes will include:

• Address of
• ref lifetime violations

Example of the impact this will have:

static void M1(ref A a)
{
    B b = CreateB(stackalloc byte[42]);
    M2(ref a, ref b);
}

static Span<byte> M2(ref A a, ref B b)
{
  // b never assigns through a so this is safe, we just can't represent it
}

ref struct A { }
ref struct B{ }

Drawbacks

It increase the power of unsafe

Alternatives

The alternative is we continue to lean on the runtime team to provide APIs which act as safety hatches for unsafe. This means more APIs like Unsafe.AsRef. This functions but has several downsides including cost to JIT team, cost to API design and that it doesn't work in multi-targeted scenarios.

Unresolved questions

The full set of errors that should fall into this category. There are several we know are in the initial set. The team would be open to more errors falling into this category given compelling scenarios existing.

The other question is whether readonly violations should fall under the same umbrella. Generally readonly is there for memory safety reasons, ensuring a piece of memory is not written to. Downgrading that to a warning would seemingly fit into the same category.

Design meetings

• https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-09-21.md#unsafer-unsafeness

Related

• stackalloc should be disallowed for structs with ref fields roslyn#63104
• Consider types with ref fields as managed and Allow pointers of managed types in some contexts roslyn#64064
• Relax some rules in unsafe context #6453

[stephentoub]

Thanks, Jared.

will provide a warning to pair with the error

Just so I understand, this means there will be two distinct diagnostic codes, one that's an error and one that's a warning, and the compiler will choose which to raise based on whether you're in an unsafe context?

[jaredpar]

Just so I understand, this means there will be two distinct diagnostic codes, one that's an error and one that's a warning, and the compiler will choose which to raise based on whether you're in an unsafe context?

Correct. Having a single diagnostic code means we essentially have to make it a warning and upgrade it to an error outside of unsafe. That means though it's a generally suppressible diagnostic because it started as a warning. Don't really have a concept of a warning in the compiler which is only suppressible in certain contexts.

This has come up a few times in the past and the decision was to use separate diagnostic IDs. True none of the other cases were as broad as this.

[BreyerW]

The other question is whether readonly violations should fall under the same umbrella. Generally readonly is there for memory safety reasons, ensuring a piece of memory is not written to. Downgrading that to a warning would seemingly fit into the same category.

Based on this i wonder if it would be ok to also extend this to private/internal members of objects? AFAIK CLR itself allows that sort of thing and this could be primarily for source generators especially serialization ones. Eg. System.Text.Json STILL doesnt support OOB de/serialization of entirely private members mainly bc of source-gen problems since you have to fallback to reflection (and that can be iffy for AOT scenarios). This could be an escape hatch for these types of source gen. Or is it too big of a hatch in this case so to speak?

[alrz]

The other question is whether readonly violations should fall under the same umbrella.

And init. Initializaiton may not always fit inside the initializer syntax. That's where you want to go unsafe and not get forced to downgrade to settable properties or some other mechanism altogether.

That being said, a unsafe <embedded_statement> syntax would be a nice to have: unsafe obj.InitOnly = value;.

[hamarb123]

One that should definitely be possible is being effectively able to escape scoped/unscoped safety. Ie. there should be an equivalent way to write something like ref T AsRef<T>(scoped in T source) in .NET 6 where the provided one is declared as ref T AsRef<T>(in T source) - this is important for those of us who multitarget & polyfill the compiler attributes to older versions when required.

Basically what I'm trying to say is we should be able to convert scoping errors to warnings in unsafe blocks since it would be important for allowing scoped to be fully usable in .NET 6 and lower (it would also be nice in .NET 7 since it allows you to bypass having to call a method which adds visual clutter to code if your code is already marked as unsafe).

[hamarb123]

The other question is whether readonly violations should fall under the same umbrella. Generally readonly is there for memory safety reasons, ensuring a piece of memory is not written to. Downgrading that to a warning would seemingly fit into the same category.

I'd like to add my support for allowing ignoring of readonly rules as well. It's not like it can't be done already using Unsafe.AsRef (other than for init properties which would require reflection / il weaving? - they should also probably be allowed under the same rules); it will be much clearer what the developer is trying to do in my opinion and reduces code clutter. And for init properties specifically, it would also be a new feature since you can't really set these at runtime in a sensible way at the moment (without reflection, which has performance, readability, and possibly maintainability downsides).

[hamarb123]

Also finally, I think it is important that we have a way to easily disable all of the warnings at once so users of this feature don't end up with like 10 #pragma warning disables in a row. Obviously the warnings should be individually disableable as well, but I think it would be good to be able to disable and restore them in bulk, which would still make it easy for the programmer to see what dodgy stuff they're doing exactly by just quickly commenting out 1 line, and also allows them to not have to acknowledge every single different violation if they so choose. I also think it would be good if you could then individually re-enable warnings for particular violations after disabling them as a group (in case they want to ensure they don't do a certain type of violation for whatever reason), but not sure if this is practical to implement or not.

(Sorry for the multiple comments, just wanted to seperate the seperate ideas / suggestions I have for this)

[miyu]

Would this allow using variables to be passed as ref values?

struct X : IDisposable { public void Dispose() {} }
void SomeHelper(ref X x) { }
	
void Main() {
    using var x = new X();
    SomeHelper(ref x); // !! CS1657 Cannot use 'x' as a ref or out value because it is a 'using variable'
}

I'd use this pattern frequently with upgradable lock guards. Currently, I just can't pass my lock guards into methods (e.g. to perform double-checked locking) which is quite annoying.

[jaredpar]

@miyu No. This scenario is effectively breaking the readonly-ness of your local. At the moment LDM decided against taking the change to this level. It wasn't a hard no but more of "let's gather scenarios and see if it's worth taking that step". I can add this scenario to the list.

[bernd5]

@miyu:
today you can do it this way:

using System;

struct X : IDisposable {
    int x;
    public void Dispose() {
        Console.WriteLine(x);
    } 
    public void Foo(){
        x++;
        Console.WriteLine("XXX");
        x++;
    }
}

class Bar{
    static void SomeHelper(ref X x)
    {
        x.Foo();
    }
	
    static void Main() {
        using var x = new X();
        ref var xref = ref System.Runtime.CompilerServices.Unsafe.AsRef(in x);
        SomeHelper(ref xref);
    }
}

[KalleOlaviNiemitalo]

Wondering about this kind of unsafe and incorrect code:

class C1 {}
class C2 {}

static class Program {
    static unsafe void Main() {
        C1 c1 = null;
        ref C2 r2 = ref *(C2 *)&c1;
    }
}

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net45</TargetFramework>
    <LangVersion>7.3</LangVersion>
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
  </PropertyGroup>
</Project>

With .NET SDK 6.0.410, I get error CS0208. With .NET SDK 7.0.107, I get warning CS8500 instead. Is it a bug that this depends on the SDK version rather than the C# language version? According to the 2022-09-21 meeting notes, the feature was "Approved for C# 11", but this demo uses C# 7.3.

[RikkiGibson]

@KalleOlaviNiemitalo Opened an issue on the Roslyn side. It's possible I don't have the full picture, but it seems like we have a compiler bug here.

[Neme12]

Given the change that pointers to managed structs are now allowed and considered a warning instead of an error, could someone pls explain why the warning exists - i.e. why are pointers to managed structs considered unsafe (or "unsafe" unsafe) and why was it originally an error? What issues can I run into if I use pointers to managed structs?

There is no documentation for the new warning and I can't find any info on it.

[RikkiGibson]

GC doesn't track objects through a pointer, so a pointer to a managed type could end up referring to a GC'd object.

[Neme12]

I see, thanks. It would be nice to have a documentation page for the new warning (CS8500) and explain this there. Right now, if I click on the warning in Visual Studio, it points me to a 404 page (https://msdn.microsoft.com/query/roslyn.query?appId=roslyn&k=k(CS8500)).

In my case, I have a managed struct and in the body of one of the struct's method, I'm using a pointer to it while fixing this. Is it fine in this case, since the struct should be kept alive while its method is being invoked and the struct being alive should keep its field alive as well?

[theunrepentantgeek]

"kept alive" != "kept stationary"

It's been a long time since I futzed around with pointers in C#, but I believe you've got a problem - while the memory for your struct won't be reclaimed while the method runs, it's possible the struct will be relocated as a part of heap compaction.

IIRC, this is one of the things the unsafe keyword does - pin things in memory so they don't get moved around.

[Neme12]

while the memory for your struct won't be reclaimed while the method runs, it's possible the struct will be relocated as a part of heap compaction.

I mentioned that I'm "fixing this", that is:

struct S
{
    unsafe void M()
    {
        fixed (S* p = &this)
        {

        }
    }
}

[theunrepentantgeek]

Ah. I interpreted "fixing this" as "I'm fussing around with the memory of the struct to fix something", not as "fixing it in place".

[jcouv]

Moved to C# 11 milestone as this change was done in 17.4 (PR dotnet/roslyn#64186)