dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.9k stars 4.63k forks source link

[API Proposal]: Missing `scoped ref readonly` and `in` reference handling at Unsafe reference #104909

Open redknightlois opened 1 month ago

redknightlois commented 1 month ago

Background and motivation

The Unsafe class provides various methods for working with references in an unsafe manner for ref T. However, there are missing operations for handling ref readonly references, such as AddByteOffset. Currently, there is a need to use Unsafe.AsRef to convert ref readonly to ref, which adds unnecessary steps.

Proposal:

I propose the addition of ref readonly reference handling operations to the Unsafe class where the guarantees are still valid in order to avoid users to use Unsafe.AsRef when those guarantees could introduce subtle bugs. Specifically, unless I am missing something, methods such as AddByteOffset should directly support ref readonly references without requiring a call to Unsafe.AsRef.

Extending the Unsafe class to support ref readonly aligns with the existing pattern used alongside the Vector API which supports the used of both ref and readonly ref in operations where no side-effects exist.

API Proposal

public static ref readonly T AddByteOffset<T>(scoped ref readonly T source, IntPtr byteOffset);
public static ref readonly T SubstractByteOffset<T>(scoped ref readonly T source, IntPtr byteOffset);
public static ref readonly T Add<T>(scoped ref readonly T source, int offset);
public static ref readonly T Substract<T>(scoped ref readonly T source, int offset);

API Usage

This is how I do this now:

internal static int CompareAvx256(scoped in byte p1, scoped in byte p2, int size)
{
    ref byte bpx = ref Unsafe.AsRef(in p1);
    ref byte bpy = ref Unsafe.AsRef(in p2);
    ref byte bpxEnd = ref Unsafe.AddByteOffset(ref bpx, size);
    ....    
}

We should be able to keep the ref readonly guarantee on those references.

internal static int CompareAvx256(scoped in byte p1, scoped in byte p2, int size)
{
    ref readonly byte bpx = ref readonly p1;
    ref readonly byte bpy = ref readonly p2;
    ref readonly byte bpxEnd = ref Unsafe.AddByteOffset(ref readonly bpx, size);
    ....    
}

Alternative Designs

No response

Risks

No response

dotnet-policy-service[bot] commented 1 month ago

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

stephentoub commented 1 month ago

I propose the addition of ref readonly reference handling operations to the Unsafe class

We can't overload on ref vs ref readonly, e.g. SharpLab

redknightlois commented 1 month ago

Good point that would require changing the public API to ref readonly instead.

huoyaoyuan commented 1 month ago

Good point that would require changing the public API to ref readonly instead.

If the return value is ref readonly, it can't be used as ref. Given it's already in Unsafe space, additional Unsafe.AsRef won't be too dangerous.

xtqqczze commented 1 month ago

Good point that would require changing the public API to ref readonly instead.

And this would be a breaking change.

tannergooding commented 1 month ago

Changing inputs to ref readonly isn't a breaking change, actually. We explicitly have compatibility so that migration from ref -> ref readonly and in -> ref readonly is safe.

If this were done it would have to be such that inputs are ref readonly and outputs are ref, which makes the signatures overall the most usable, but also makes them "more dangerous" since every operation works like ref T Unsafe.As<T>(ref readonly T value) is implicitly wrapping it.

There's both pros and cons to doing that and I don't think we've had an in-depth discussion on whether that is better or worse; but I imagine it will be contentious discussion to have with people landing on both sides of the coin.

redknightlois commented 1 month ago

I would certainly would be on the side of: "If it doesn't return ref readonly it defeats the purpose and be dangerous for no reason". It is clearly an overload situation that would either require support at the language level or entirely new method name.

tannergooding commented 1 month ago

We can't overload like that due to the representation in IL and because we want the transition from ref to ref readonly to be possible in a non-breaking manner

It's something where it's either going to stay "as is", where we'd have ref T Add(ref readonly T address, int offset), or where we'd need some new "matched mutability" language feature so that we could say that the return type has the same mutability as the input type.