[Proposal] Unsafe expression syntax

[Sergio0694]

Overview

This issue is about adding the expression syntax to unsafe contexts, in addition to the currently supported unsafe blocks.
This would be analogous to eg. checked expressions, and it would bring the following benefits:

• Simplify the syntax when only dealing with individual unsafe expressions.
• Remove unnecessary indentation levels.
• Reduce the scope of unsafe contexts in case where devs would be forced to move additional statements into the unsafe blocks, if they need to access the return value of the unsafe expression (as that would otherwise be out of scope).
• It would make unsafe blocks more consistent with checked/unchecked ones (the difference feels a bit weird today).

Example usage

Consider the following extension method (from here):

 public static unsafe ref T DangerousGetReferenceAt<T>(this Span<T> span, int i)
{
    ref T r0 = ref MemoryMarshal.GetReference(span);
    ref T ri = ref Unsafe.Add(ref r0, (IntPtr)(void*)(uint)i);

    return ref ri;
}

Currently I need to mark the entire method as unsafe, even though the only part requiring that context is just the (IntPtr)(void*) cast in the expression on the second like. Yes I could just use an unsafe block for the second statement, but that'd add another indentation level and make the whole code much more cluttered and harder to follow.

With support for unsafe expressions, the above could be rewritten as:

 public static ref T DangerousGetReferenceAt<T>(this Span<T> span, int i)
{
    ref T r0 = ref MemoryMarshal.GetReference(span);
    ref T ri = ref Unsafe.Add(ref r0, unsafe((IntPtr)(void*)(uint)i));

    return ref ri;
}

With the unsafe context being restricted to the single expression that actually needs it. This is even more useful when dealing with eg. multi-targeting, where only some code paths might require an unsafe expression, which would make the unsafe modifier applied to the entire method be redundant on those targets. A workaround here is to use an unsafe block, but as mentioned that both makes the code more verbose and with an extra indentation level. Consider the following (extracted from this type):

private readonly Span<T> span;
#if !SPAN_RUNTIME_SUPPORT
// If we have runtime Span<T> support, we save 4 bytes
// by storing the index in the length property of the span,
// and creating the span already starting from the right offset.
private readonly int index;
#endif

public ref T Value
{
    get
    {
#if SPAN_RUNTIME_SUPPORT
        return ref MemoryMarshal.GetReference(this.span);
#else
        ref T r0 = ref MemoryMarshal.GetReference(this.span);

        unsafe
        {
            return ref Unsafe.Add(ref r0, (IntPtr)(void*)(uint)this.index);
        }
#endif
    }
}

Compare this with just being able to write that second part on the same indentation level, like so:

public ref T Value
{
    get
    {
#if SPAN_RUNTIME_SUPPORT
        return ref MemoryMarshal.GetReference(this.span);
#else
        ref T r0 = ref MemoryMarshal.GetReference(this.span);
        ref T r1 = ref Unsafe.Add(ref r0, unsafe((IntPtr)(void*)(uint)this.index));

        return ref r1;
#endif
    }
}

Syntax changes

From what I can see in the specs (here), this would require a new unsafe_expression type to be added to support the new expression syntax. This would then also need to be added to the list of supported expressions in the primary_no_array_creation_expression category (just like checked_expression).

unsafe_expression
    : 'unsafe' '(' expression ')'
    ;

primary_no_array_creation_expression
    ...
    | unsafe_expression
    ;

[CyrusNajmabadi]

Yes I could just use an unsafe block for the second statement, but that'd add another indentation level and make the whole code much more cluttered and harder to follow.

I personally disagree with this. I would find that much less cluttered than the unsafe expression form. The former calls out clearly that there is something unsafe going on. With your example, i didn't even notice when it was in expression form.

And that's something very important to me. I want unsafe code to stick out liek a sore thumb. It should intentionally be very visible because it needs extra special attention.

[Sergio0694]

Hey, thank you for chiming in!

I want unsafe code to stick out liek a sore thumb. It should intentionally be very visible because it needs extra special attention.

I understand that point of view, but my counter point to that would be that I'm not sure that really matter so much at this point, where we can have technically unsafe code literally everywhere by using the various Unsafe/MemoryMarshal APIs, which don't require an unsafe context anyway. The idea of "clearly marking unsafe paths" has already gone out the window at that point, since when using those APIs you don't even need to enable the unsafe flag in your project at all anymore. So it just becomes a matter of convenience to make the code less cluttered, especially when there are other unsafe bits in there as well.

In that previous example, that whole snippet was technically unsafe. I'd argue that making just the part with the (IntPtr)(void*) cast "stick out" isn't really necessary since every single one of the other statements is unsafe as well. That part is not "more unsafe" than the other lines, so I feel like making that stand out so much is just redundant, and unnecessary clutter.

I guess my rationale here is: I think making this so explicit on purpose made perfect sense back when it was introduced, but I feel like now that we have all these other "safe-not-safe" APIs being available, it lost meaning in a way. I feel like giving users the ability to have the more compact syntax here would make life easier in these scenarios, with no real drawbacks in code safety 🤔

[tannergooding]

At the same point, you could just do:

public unsafe ref T Value
{
    get
    {
#if SPAN_RUNTIME_SUPPORT
        return ref MemoryMarshal.GetReference(this.span);
#else
        ref T r0 = ref MemoryMarshal.GetReference(this.span);
        return ref Unsafe.Add(ref r0, (IntPtr)(void*)(uint)this.index);
#endif
    }
}

This is already legal today and equally as "hidden".

[Sergio0694]

Yeah that works too, I just don't really like that as when you're on a target that takes the first branch the unsafe modifier there becomes completely unnecessary. I know it's not actually an issue per se (I mean, it still builds and produces the same code anyway), but I feel like restricting the unsafe context to just the places that actually need it would make the code easier to read/follow.

I mean, with the unsafe modifier right there in the property declaration it'd take me a bit to see why it's actually needed, not to mention it makes the code quite error prone as one could see that modifier being marked as unnecessary when the other target is selected, then remove it and possibly forget to test all the targets to be sure, and either break the build or just waste time there. I mean, I feel like it makes the whole thing less clear in general is all, at least in cases like this 🤔

I'd personally use unsafe in the entire method/property either if I actually have some unsafe locals or if the whole body is small enough and with the unsafe requirement being present in all targets (or in the single target, if not multi-targeting).

[Sergio0694]

On second thought, to address what both @CyrusNajmabadi and @tannergooding said, I'd actually be happy if there was just an option to allow unsafe expressions in an entire project. I mean, especially given how today you can do all sorts of "unsafe" things anyway (Unsafe class, etc.), I'd argue the restriction to just using pointers under an explicit unsafe context isn't really needed that much anymore. I'd love to just be able to use those types like any other type, without the need for any explicit preamble.

I mean, something like (100% made up name):

<AllowUnrestrictedUnsafeBlocks>false</AllowUnrestrictedUnsafeBlocks>

Adding this to the .csproj (in place of, or besides AllowUnsafeBlocks) would just let you declare/use any pointer type or unsafe expression in the entire project. I'd imagine I should create a new issue for this in the roslyn repo and link this one there? 🤔

[333fred]

I'd imagine I should create a new issue for this in the roslyn repo and link this one there?

No, that's still a language change.

As a general rule, we do not create dialects of C# controlled by a project file switch. If the normal language feature has to go from -100 points to 100 points to even be considered, a new dialect has to get to about 1,000,000 points. Nullable is the only such feature in recent memory, addressing a fundamental issue in C# that likely costs developers literally billions of dollars of time a year. I don't see explicitly allowing unsafe everywhere in the language as meeting that bar.

[Sergio0694]

I see, that makes perfect sense, thanks!

I guess I was just thinking about how to me the original explicit toggle for unsafe blocks has basically some (most?) of its meaning today considering all the other ways to still do crazy unsafe things with code that's technically not unsafe. I mean, ideally I'd like to just not have the unsafe restriction at all and being able to use all those features like any other, but that's another point ahahah

I'll leave this issue as is then, unsafe expressions would still be a pretty cool feature to have and they would still solve all the issues mentioned in the various examples and snippets I added in the original post 😊