Runtime: Nullable<T> must be a readonly struct

[VSadov]

Nullable<T> is known to be an immutable struct with sideeffect-free methods.
C# compiler certainly relies on the implied purity for the soundness of null-propagating math as well as in various optimizations.

Basically - while for other structs readonly is a good thing to have, if possible. I think the spec for Nullable<T> should _demand_ that Nullable<T> is formally readonly.

In particular we consider methods like HasValue not mutating and therefore callable directly on references to readonly fields (which is currently unverifiable). When verification rules are updated to understand readonly references, we would need to special case Nullable<T> methods. - Unless the type is readonly by the spec, then the behavior would be subsumed by the general treatment or readonly structs.

[VSadov]

CC: @stephentoub @jkotas @jaredpar @marek-safar @kumpera

[marek-safar]

Why only Nullable<T> ? Why not also other C# core types like System.Decimal.

Or perhaps someone could review all types including DateTime, DateTimeOffset, etc.

[jaredpar]

I think we should start by taking a look at every struct where all the fields are readonly. Such a struct should be incredibly likely to benefit from becoming a readonly struct.

[kumpera]

Are you guys planning or researching on using any future CoreCLR optimization around this feature?

[jaredpar]

I don't know if there are any CoreClr optimizations planned right now. We have already optimized it in the c# compiler though. This allows us to avoid senseless struct copies in a number of scenarios.

[VSadov]

I did not mean that only Nullable must be readonly. It would be helpful for other structs too - DateTime and the like.

Nullable is special because it is, well, very special. We already imply that it is immutable in so many scenarios - we should really make its readonliness a formally required trait since it is possible now.

[stephentoub]

https://github.com/dotnet/corefx/pull/24997
https://github.com/dotnet/coreclr/pull/14789
https://github.com/dotnet/corert/pull/4855

[stephentoub]

Why only Nullable ? Why not also other C# core types like System.Decimal. Or perhaps someone could review all types including DateTime, DateTimeOffset, etc.

I went through and annotated types where the fields were already readonly, plus a few types like nullable, but someone who's interested can do a follow-up sweep through types to fix up field annotations and then add the type-level annotation... presumably Decimal would be on that list. DateTimeOffset can't in its current implementation be made readonly, as one of the fields is written to from outside of the constructor, but some tweaks could likely be employed to cheat with Unsafe and fix that.

[danmosemsft]

@jaredpar why can the compiler not treat a struct with all readonly fields as if it was marked readonly? (At least for optimizations - I guess the marker also has value in flagging any changes to make fields writeable)

Analogous to static or abstract on all members implying it on the type.

[stephentoub]

@danmosemsft, the compiler doesn't necessarily know about the fields. For example, when writing code against DateTime, you're compiling against the DateTime from ref assembly, at which point there are no fields. And even if it did, the readonly-ness is part of the contract, so whereas it's not a breaking change to remove readonly from a field, it would be a breaking change if you removed readonly from a field and the compiler had previously used that knowledge in another compilation unit to treat that type as readonly.

[YohDeadfall]

@stephentoub I think that @danmosemsft asking about a bit different thing. The compiler can mark a struct as readonly if all its fields are readonly when compiling that struct. So the following ReadOnlyStruct struct's implementations are equivalent.

public struct ReadOnlyStruct
{
    public readonly int Value;
}

public readonly struct ReadOnlyStruct
{
    public readonly int Value;
}

[sharwell]

@VSadov I'm not understanding the benefits such a change would have on the C# language. Unlike ValueTuple<T> (and related), the Nullable<T> type exposes no writable properties and no fields. The API itself mandates purity without regard to the definition of the type itself.

📝 To be clear, I understand why we want to define Nullable<T> as a readonly struct. What I don't understand is why the C# language would need to require it be defined that way.

[stephentoub]

So the following ReadOnlyStruct struct's implementations are equivalent.

That's the second part of what I wrote:

the readonly-ness is part of the contract, so whereas it's not a breaking change to remove readonly from a field, it would be a breaking change if you removed readonly from a field and the compiler had previously used that knowledge in another compilation unit to treat that type as readonly.

If the readonly-ness is implict, then it becomes a breaking change to remove readonly from a field when every other field is also readonly, as someone could have taken a dependency on it even though it wasn't intended to be a publicly visible fact. This would also wreak havoc on ref assemblies, where every struct could be marked readonly implicitly due to not having any fields.

[VSadov]

@sharwell when compiler reduces code that involves Nullable to method calls it makes assumption that members like HasValue do not mutate the struct or have any sideeffects watsoever. That allows many optimizations where calls are moved forward/backward in time WRT other such calls, dropped when unnecessary, or introduced to reduce conditional branching, etc.

As particular example compiler may now skip making copies of the receiver of HasValue if receiver is a readonly variable - since it knows that Nullable must be immutable.

Indeed we could add checks in the code "is the Nullable struct we are dealing with here indeed readonly?", but that is kind of pointless - if it is not immutable the whole thing is badly broken.

So we simply assume that Nullable is a readonly struct, regardless of annotation since we already assume it is immutable/pure anyways.

It still would be nice if it had annotation, since it now can. And it would be nice if the spec would require (or strongly recommend) that too.

[VSadov]

@sharwell - basically I do not want to say anywhere in C# or in IL verification spec "and Nullable is considered to be a readonly struct". - Let's just make it so and not introduce any special cases.

[stephentoub]

@VSAdov, @jaredpar, @jkotas, see https://github.com/dotnet/corefx/pull/24997#issuecomment-346523578. Do we need to revert that change to nullable? How does that relate to the comments in the post here about the compiler treating it as immutable?

[stephentoub]

Do we need to revert that change to nullable?

I've done so in https://github.com/dotnet/coreclr/pull/15198 and https://github.com/dotnet/corefx/pull/25464.

[VSadov]

Methods that Nullable overrides appears to propagate mutations to inderlying value. I wonder if that is deliberate. I guess that could be to match behavior of fancy boxing, which also exposes underlying to mutations.

I think we should specialcase HasValue, Value, ValueOrDefault as nonmutating in the compiler and tweak proposed verification rules for readonly refs accordingly.

[VSadov]

Actually after thinking a bit about this, it looks like a bug/oversight in Nullable. And boxing/lifting would always copy.
Self-lifted ToString should do something like ‘this?.ToString() ?? “” ‘

Mutable Nullable is less convenient and is unlikely intentional. As a result methods like ToString can change the state. Even T being readonly or a primitive would not help much. ToString could still do anything - as gross as ‘this = null’.

I guess we are keeping it as is anyways..

[VSadov]

Perhaps we should ask ourselves - what are the chances that anyone relies on this kind of mutations though.

[stephentoub]

what are the chances that anyone relies on this kind of mutations though.

I expect relatively good. There are lots of types, even structs, that are observably immutable, but cache results generated in ToString/GetHashCode so that subsequent access is faster. Making it readonly defeats such caching and could have an observable impact on performance.

Even if it's not, though, this impacts public API signature. We couldn't quirk that, which means I can't imagine this would ever go back to desktop, which means we shouldn't do it here in public API.

[VSadov]

In most scenarios with lifted operations, the underlying value is fetched via Value or, more commonly, via GetValueOrDefault, so caching in the instance would not generally work inside Nullable.
It is hard to believe that accommodating mutations inside Nullable is intentional. It is more likely that noone thought of mutating ToString or GetHashCode.

Anyways. I think the compat is the overriding matter here.

I will look through our code and check if we make any assumptions about members other than HasValue/Value/GetValueOrDefault. I think we don't.

These particular members will still be treated as nonmutating. - It would be silly to make an outer copy before calling GetValueOrDefault which copies again.
(in fact the method could return ref readonly if we designed the type right now.)