Make ValueTuple members that can be made readonly readonly

[Neme12]

I applied readonly to all methods except for GetHashCode and ToString, which can't be made readonly (or can be, but it would make copies of the items) because they forward to methods on the individual items that could potentially modify them.

[teo-tsirpanis]

You also have to update the reference assembly in https://github.com/dotnet/runtime/blob/64243bbf5e9ee53c0c4c5678f2cd8c7f1c9b4f6f/src/libraries/System.Runtime/ref/System.Runtime.cs#L7116-L7294.

[Neme12]

Thanks.

[neon-sunset]

Doesn't this break C# tuple contract? For example

// Valid code
var tuple = (1, 2);
tuple.Item2 = 3;

[huoyaoyuan]

Doesn't this break C# tuple contract? For example

Only the 0-ary ValueTuple is marked as readonly as a whole type. The members of others are still writable.

[ghost]

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

Issue Details

---

I applied readonly to all methods except for GetHashCode and ToString, which can't be made readonly (or can be, but it would make copies of the items) because they forward to methods on the individual items that could potentially modify them.

Author:	Neme12
Assignees:	-
Labels:	area-System.Runtime, community-contribution, needs-area-label
Milestone:	-

[jozkee]

Thanks for working on this.
How does GetHashCode() and ToString() create a defensive copy? sharplab.io shows identical asm.

Also, there have been other issues filed where this is being discussed. Just referencing them here.

• ValueTuple should use readonly methods #29403 (specific to ValueTuple)
• Determine Libraries strategy for using the readonly members annotation #1718 (more general approach)
• Methods "readonly" annotation #46675 (comment)

[adamsitnik]

@dotnet/fxdc does applying readonly to a type and/or method requires going through API review (removing it would cause a breaking change)?

@stephentoub @jkotas what is our current strategy for similar changes? I saw #46675 (comment)

annotate methods on non-readonly structs which do not and will never mutate the state of the instance

And I believe that most of the methods marked as readonly in this PR (Equals, indexer, GetHashCode) will never mutate. But it's not very clear to me what we would gain from it.

[stephentoub]

does applying readonly to a type and/or method requires going through API review (removing it would cause a breaking change)?

We've generally preferred to do so, yes, ideally in bulk.

it's not very clear to me what we would gain from it

The main consumer gain for us marking something readonly is it can avoid a defensive copy when a value is passed via in.

Let's say you have:

(long, long, long, long) value = ...;
Foo(ref value);
...
void Foo(ref (long, long, long, long) data)
{
    data.GetHashCode();
}

The compiler doesn't need to do anything special here around preventing mutation. But if instead the code was:

(long, long, long, long) value = ...;
Foo(in value);
...
void Foo(in (long, long, long, long) data)
{
    data.GetHashCode();
}

The in is promising that the caller's value won't be mutated, which means when compiling Foo, the compiler needs to ensure that data isn't mutated, but the compiler doesn't know what GetHashCode does... it could be writing to this, in which case it would be mutating data and thus value. So if GetHashCode isn't readonly (either directly or because the struct on which it's declared is readonly), the compiler needs to make a defensive copy, and it's compiled instead as if it were:

void Foo(ref (long, long, long, long) data)
{
    (long, long, long, long) copy = data;
    copy.GetHashCode();
}

Whether that has any actual performance impact depends on a variety of factors, e.g. if the method being called is inlined, there's a good chance the JIT will eliminate the copy.

[jaredpar]

Putting readonly on explicit interface implementations isn't going to buy you anything. These can only be accessed via interface references and those don't have defensive copy semantics.

I'm actually a bit surprised we allow this syntax.

[tannergooding]

The main consumer gain for us marking something readonly is it can avoid a defensive copy when a value is passed via in

The other notable case is static readonly (int, int) s_value = ...; and then doing s_value.Method() or similar.

---

That being said, this is all generic code and we're just trading one copy for another copy here.

That is, today, if you have readonly (int, int) value = ... and do say value.GetHashCode() you get a copy of the full value and then GetHashCode() is called on that copy.

If you instead mark GetHashCode() as readonly, then it cannot statically determine that Item1.GetHashCode() won't mutate and Roslyn will insert independent copies of both Item1 and Item2 before calling their own respective GetHashCode() methods.

In the case where all types are unmanaged, the JIT can optimize the former copy to a block copy, while it may needs to preserve the per item copies in the latter case. Which is better is ultimately scenario dependent, but I don't think this is something we can really "fix", since we can't actually remove the copies without unsafe code.

[tannergooding]

The below sharplab link gives a simplistic example of a struct where Item1/Item2 are a known concrete type vs when they are generic and show the copies that are inserted by Roslyn

https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKBoG0BlAC2ygAcAZbYAOgCUArgDsMAS3wweAKVEYA4jCEwoosAAoMATzYwIAMzWiRASjQACLTv2GTxgLo1iAZjO4MUAWAxmAagBUAHj9ycz9SAD4aAG8aMzizZzNgswBJDBh8cgBuWPjEsNT0/FIc2mp4hJdYbAATCCEAG00zCAA3ZRUamDMjbwUMAAk8JgBhCC61Y1y4mPKKvIB2MyHcUfHJMfxgIxg1NIzyAH4efpW1ieMzQ8OzAAY0afmn55f4/eLj0+Gxi6ub2+MpQqAF8aKCyok3B4vL4/AB9FIiJykBFI0jRR6JXqFA5AvIubHvEo0TFVGC1epNFrtKCdbrYr6rH67KZzGaPCrEJZnZk8TbbJR7IrkE4wQbfdaTB5s16y2VE0XipmS4yAx7g4FAA==

[adamsitnik]

does applying readonly to a type and/or method requires going through API review (removing it would cause a breaking change)?

We've generally preferred to do so, yes, ideally in bulk.

In such a case I am going to convert this PR to a draft and create a new API proposal based on the ref file changes.

[huoyaoyuan]

Historically, #44640 and #44629 didn't go through API review.
This one should be really trivial though.

[ghost]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[terrajobst]

Before we close this, is this intended to still be one?

From an API review standpoint, this affects public surface area and should normally go through API review but my care level is relatively low. Unless anyone on @dotnet/fxdc objects, I'm OK with letting these kind of changes be handled by PR review alone.

[ghost]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.