Mark Value and HasValue of System.Nullable<T> as readonly

[bernd5]

accessing Value and HasValue of System Nullable in a readonly context causes a copy...

=> please mark these properties readonly
(best would be a static analyzer which marks as much readonly as possible)

[mingxwa]

Since Value and HasValue are get methods, how do you expect them to be marked as readonly? Do we need additional syntax? Could you provide some before-after code?

[tannergooding]

C# added readonly instance members in C# 8. You can see https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-8.0/readonly-instance-members and https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/readonly#readonly-member-examples for more details.

But, basically, this means we would do the following:

public partial struct Nullable<T> where T : struct
{
    public readonly bool HasValue
    {
        get { /* impl */ }
    }

    public readonly T Value
    {
        get { /* impl */ }
    }
}

This would mark (and enforce) that HasValue and Value do not mutate the underlying instance and would emit the appropriate metadata to avoid the mutation in various scenarios.

We have https://github.com/dotnet/corefx/issues/36586 as the meta tracking issue for updating framework APIs, in general, to use the new feature.

[mingxwa]

What about marking the entire struct Nullable as readonly?

[tannergooding]

We can't do that as T may itself be mutable (and therefore calling T.ToString(), for example, could mutate the nullable instance).

We had actually attempted to mark the entire struct readonly previously and had to revert it.

[tannergooding]

For reference: dotnet/coreclr#15198

[bernd5]

Hello I think the data stored in the nullable should never change. If you assign a non nullable to a nullable it should create a new nullable object which replaces the old one. No change of the old nullable. If you call something like ToSting on the wrapped object than it should create a temporary for that call (ToString should be readonly too).

It is the same like with int, int is readonly. You can’t change the number one. You can only replace that number with a different value.

[tannergooding]

While it might be nice if that was the behavior, that isn't the current behavior and hasnt been the behavior since Nullable<T> was introduced. It is a breaking change to modify it to be such now.

It also isnt the only struct which exists which needs to be partially mutable and is part of the reason we have readonly instance members (which allows certain methods to be readonly and avoid hidden copies).

[bernd5]

Unfortunately we have as far as I know currently no possibility to overload on readonlyness (like in C++) on member functions.
For this reason I think we can't mark the whole struct readonly. This would cause extra copies in non readonly contexts e.g. for ToString calls (most ToString() implementations are effectively readonly, but because this is new they are not marked as readonly).
But we should mark those members readonly which can work with readonly (no assignment to field / call).

The readonly modifier is/was intended for perfomance optimization. But in this case it causes the opposite. Every call to an readonly nullable field causes a copy...

[ltrzesniewski]

This would be a really nice change since it would enable passing nullable structs through in parameters without defensive copies being made for the most common cases.

GetValueOrDefault should also be marked readonly BTW.

Can I submit a PR for this, or does this need to be triaged/validated first?

[tannergooding]

We've taken past changes through API review.
CC. @stephentoub and @dotnet/fxdc who may have opinions

[GrabYourPitchforks]

API review - it looks like the Roslyn compiler already special-cases this, so perf is already in a good way. But we should probably do this anyway for non-Roslyn compiler scenarios, and so that the API surface matches the expectation.

Approved for these APIs specifically:

• get_Value
• get_HasValue
• GetValueOrDefault (both overloads)