API proposal: Nullable.RefValue, RefValueOrDefault

[nagya]

EDIT: @stephentoub's proposal update from #1534 (comment) on 1/29/2022:

namespace System
{
    public static class Nullable
    {
+        public static ref readonly T GetValueRefOrDefaultRef<T>(in T? nullable) where T : struct => ref nullable.value;
    }
}

---

Proposed API

namespace System
{
    public static class Nullable
    {
        public static ref readonly T RefValue<T>(this in T? n) where T : struct;
        public static ref readonly T RefValueOrDefault<T>(this in T? n) where T : struct;
        public static ref readonly T RefValueOrDefault<T>(this in T? n, in T defaultValue) where T : struct;
    }
}

These parallel the Value property and the GetValueOrDefault methods of Nullable<T>, but return the value by reference. They're extension methods because of CS8170: Struct members cannot return 'this' or other instance members by reference.

They're useful for accessing the value wrapped by a Nullable<T> without incurring a copy.

Reference implementation

namespace System
{
    public static class Nullable
    {
        public static ref readonly T RefValue<T>(this in T? n) where T : struct
        {
            if (n.HasValue)
                return ref n.value;
            else
                throw new InvalidOperationException();
        }

        public static ref readonly T RefValueOrDefault<T>(this in T? n) where T : struct
        {
            return ref n.value;
        }
        
        public static ref readonly T RefValueOrDefault<T>(this in T? n, in T defaultValue) where T : struct
        {
            return ref n.HasValue ? ref n.value : ref defaultValue;
        }
    }
}

[nagya]

To ensure that the implementation does not to incur any copies either, I believe the HasValue property would need to be marked readonly, so the call to it wouldn't defensively copy the in parameter. Or its backing field be made internal and used directly instead.

[GrabYourPitchforks]

What if we put this on the existing MemoryMarshal class?

public static class MemoryMarshal
{
    public static ref T GetNullableValueRef(ref Nullable<T> value);
}

It just returns a ref to the underlying _value field. No checks are performed.

If we put it on that type, it would be one of the few completely type-safe APIs on that class. It's type-safe because even though the value of any given Nullable<T> cannot be publicly set once the Nullable<T> is created, if you had a mutable ref to a Nullable<T> you could've overwritten the entire thing anyway.

[nagya]

Sounds good. If needed, all the other variants could be easily implemented using GetNullableValueRef.

[GrabYourPitchforks]

I should correct my earlier statement: it wouldn't be completely type-safe because it would allow creation of a "null" Nullable<T> wrapped around a non-default T, but whatever. MemoryMarshal isn't intended to be safe anyway.

[stephentoub]

I suggest we simply do:

namespace System
{
    public static class Nullable
    {
+        public static ref readonly T GetValueRefOrDefaultRef<T>(in T? nullable) where T : struct => ref nullable.value;
    }
}

Everything else can be built on top of that: if you want the unsafety of a ref T instead of a ref readonly T, you can compose this with Unsafe.AsRef, and if you want a null ref or a ref to something else if !HasValue, you can write that yourself... this method simply provides a ref readonly to the underlying field as a building block for whatever else you may need.

For naming, it mirrors both Nullable<T>.GetValueOrDefault and CollectionsMarshal.GetValueRefOrNullRef in structure.

Putting it as a static on Nullable instead of on Nullable<T> makes it pay-for-play from an AOT perspective. Debatable whether it should be an extension method, but I'd argue it's a rare enough need that it shouldn't clutter up the IntelliSense for T? and should be left as a non-extension, and we already have the non-generic System.Nullable for other static helpers around Nullable<T>.

[rickbrew]

cc @Sergio0694 re: #64491

[Sergio0694]

Ah, well I swear this issue hadn't showed up when I tried searching before opening #64491, I'm sorry for the duplicate 😅

The new proposal Stephen mentioned seems perfect, it's all we need anyway to then build on top of it.
Happy to help implementing this if it's marked up for grabs once it's approved 👍

[GrabYourPitchforks]

Taking this via in might lead to unexpected results due to unintentionally allowing rvalues to be captured. See related discussion in #31170, where we opted to take a parameter as ref to avoid this possibility, even though we weren't mutating the value.

[stephentoub]

Taking this via in might lead to unexpected results due to unintentionally allowing rvalues to be captured.

Can you share an example?

[GrabYourPitchforks]

Can you share an example?

class SomeClass
{
    public int? SomeProperty { get; set; }
}

SomeClass c = new SomeClass() { SomeProperty = 42 };
ref readonly int theRef = ref Nullable.GetValueRef(c.SomeProperty);
c.SomeProperty = 84;
Console.WriteLine(theRef); // "42"

In #31170 there was a proposal for a "thou shalt not pass rvalues to this API" analyzer. That didn't really go anywhere at the time. But presumably that analyzer would be a way to bridge the pit of failure if we decide that such a pit really does exist here.

[terrajobst]

Video

• This API might open a pit of failure where the compiler made a copy before passing in the value, thus exposing a ref to the copy, not the original
  • Example: Nullable.GetValueRefOrDefaultRef(someObj.SomeProp)
  • We could add an analyzer but it might be very difficult to write an analyzer that fully understands all the places where the compiler makes a copy. However, we might be able to construct an analyzer to checks for known-safe cases, e.g. when passing in locals or fields and warns on everything else or on known unsafe cases. Either way, the analyzer could add value without having to duplicate the logic of the compiler. Another option is force the call site to specify in, which is a safe pattern. However, we don't want to add a global analyzer because that would result in us pushing for a given language use or code style and we don't believe that's our role. We could, however, have targeted analyzer for specific APIs. This is something where having a conversion with @dotnet/LDM would be a good idea.

namespace System
{
    public static partial class Nullable
    {
        public static ref readonly T GetValueRefOrDefaultRef<T>(in T? nullable) where T : struct;
    }
}

[jaredpar]

Another option is force the call site to specify in, which is a safe pattern

That seems to be solving a specific problem though. Consider that a similar problem exists when in is used in an extension method this parameter. In that case there is no place to insert an in without undoing the extension part. Also ref has endless silent copies associated with it for struct instance methods.

I think a more encompassing feature might be something like an attribute along the lines of [RequiresLocation]. This could be applied to struct instance methods or in / ref / out parameters. The impact would be at the callsite the matching argument must be a location.

[GSPP]

With respect to naming, I think that GetValueRefOrDefaultRef is not correct when taken literally. It does not return a ref to a default value. It returns a ref to a location that currently happens to hold a default value, but that might change.

Ideas:

• GetValueRef
• GetValueStorageRef
• GetInternalValueRef //I like this one because it sounds like an unsafe construct, which it is.

[stephentoub]

It returns a ref to a location that currently happens to hold a default value, but that might change.

This was all discussed in the API review meeting and the proposed name was still considered the least bad option.