Determine Libraries strategy for using the readonly members annotation

[tannergooding]

C# 8 has added a new feature called "readonly members". This feature allows you to indicate that an individual method on a non-readonly struct is itself "readonly" (i.e. that the method does not mutate the state of the instance).

Edit

We need to define a strategy for if/how/when/where this feature should be used across the .NET Libraries. Originally, the intent of this issue was to take a pass over the libraries and annotate methods on non-readonly structs which do not and will never mutate the state of the instance. It was noted that there are a number of these methods in the System.Numerics namespace which users also may try to pass around as in. However, after such a pass was taken by @hrrrrustic in #46675, we realized that we need to be more strategic about this effort.

Some of the aspects that need to be considered are:

1. For every API touched, we have to evaluate it through the lens of whether we'd ever want to allow mutation to occur -- it would be a breaking change to remove readonly later. For example, adding caching to a readonly method later would be a breaking change.
2. How we quantify the value that readonly provides across different areas relative to that possibility
3. If we can define and document API design guidance to follow both with these PRs and for all APIs going forward

[tannergooding]

CC. @stephentoub as I believe you made a similar pass for readonly structs.

[tannergooding]

I plan on looking at System.Numerics itself; but there are likely other areas that would benefit as well.

[tannergooding]

(The ability to do this work is also pending a compiler update to the repo).

[stephentoub]

I believe you made a similar pass for readonly structs.

I'd written a little Roslyn-based tool that just made structs readonly if all of their fields were readonly, and then I reviewed the changes it made to ensure they were appropriate (I didn't do the next step of having it look for fields that weren't readonly but could have been). A simple tool could similarly be written here that would look at all methods/properties on non-readonly structs and see whether they write to any of the struct's fields, pass this by ref, etc... there'd be some false positives and some false negatives, but I expect it would automate most of the work.

However, we'll want to be careful in what we annotate as readonly, as once we do, that method won't be able to modify the struct (at least not without hackery), and so we'll want to only do so when we're confident it'll never want to mutate.

[tannergooding]

I've got a PR up for the System.Numerics.Vectors project: dotnet/corefx#36663.

In this case, they are all structs that expose their fields publicly (so we can't mark them as readonly) but all instance methods are currently, have always been, and always should be non-mutating.

[tannergooding]

ToString and Equals are likely common methods that would be good to mark as readonly. Property getters are likely the other common case where there will be no controversy.

I think other methods will likely be a case by case basis.

[benaadams]

GetHashCode also?

[Joe4evr]

Would like to throw in a vote to make members of System.Guid readonly. There may be a deeper reason why its fields can't be readonly, but far as I can see, everything on it seems to only do non-mutating things.

[GrabYourPitchforks]

Guid's lack of annotations is almost certainly because of the mutation done to the instance as part of construction. But honestly this really should be an implementation detail and shouldn't stop us from annotating the type appropriately.

Edit: Also because constructs like Unsafe.Add(ref _a, ...) appear throughout the code base outside of the ctor. @tannergooding, this matches what you said earlier re: difficult to use the Unsafe APIs in these scenarios because they all take ref instead of in.

[Gnbrkm41]

Opened a PR to mark Guid as readonly. Do we need an API review session for this?

[tannergooding]

So far we have done API review sessions for most of these changes (but they have been pretty quick in each case). It is generally beneficial just to have the input from @dotnet/fxdc that the right changes are being made.

[jeffhandley]

I've updated the issue title and description to reflect what we learned in #46675 (comment).

[bartonjs]

We sort of have have guidance for new API:

• DO declare immutable value types with the readonly modifier.
• DO declare nonmutating methods on mutable value types with the readonly modifier.

And, I put the breakingness into the book:

• D.6.6 Adding readonly on a struct (OK, changes behavior on recompile)
• D.6.7 Removing readonly from a struct (Not OK, changes runtime behavior, changes behavior on recompile)
• D.11.14 Adding the readonly Modifier to a struct Method (OK, changes behavior on recompile)
• D.11.15 Removing the readonly Modifier from a struct Method (Not OK, changes runtime behavior, changes behavior on recompile)

The thing that we don't have a stance on is how to re-evaluate existing methods, or what level of mutation we're willing to lie about (e.g. I believe you can still do things like take a pointer to a readonly struct, at which point you have a pointer, so "readonly "is out the door). And since we never made the conscious choice with existing API we now have the "hmm, do we keep the door open here, or let it close?" problem, which is (largely) unique to existing API.

[tannergooding]

For mutable structs, readonly methods have the benefit of avoiding copies when called. This can happen largely in two places:

• You pass something by in
• You have a readonly MyStruct _field or static readonly MyStruct _field

For types like Complex, they should have been readonly in the first place and the fix here is to just make the entire struct readonly.
For types like Vector2/3/4, they never should have exposed public fields and so should have been marked readonly, but can't be. So we instead annotated the individual methods as readonly.

When looking at whether or not something should be marked readonly, I think we largely need to consider the usage scenario. If something is expected to be passed by in or expected to be used in a readonly or static readonly field, then key methods (particularly things like ToString, Equals, or GetHashCode which probably shouldn't mutate, particularly the last two given problems that could arise from that).

Guid is an example of something that should probably be a readonly struct but which isn't today. Especially in native Windows code, it is commonly passed as const GUID* (in Guid in C#).
We actually did make Guid readonly, I missed that 😄
Other examples likely need to be considered on a per type basis for how likely they are to be used in a particular scenario.

[GrabYourPitchforks]

I see struct types as falling into two general categories.

The first category is where the struct is an exchange type and represents some standalone datum. Tanner gave some good examples earlier: Complex, VectorX, Guid, etc. The rule of thumb that I use is that if making a byval copy of the struct also makes a standalone copy of the datum, then it falls under this category. We should add readonly annotations to such structs / members where feasible, especially since it's common in data exchange scenarios for people to have readonly refs to such data.

The second category is where the struct does not represent some standalone datum, but where it instead acts as a wrapper or mutator around something else. Some examples of this are enumerators (List<T>.Enumerator), readers (Utf8JsonReader), and builders (System.HashCode). The point of these APIs is to perform some sort of state change or other side effect-producing operation. These structs are not very useful if somebody keeps a readonly reference to them, and in practice nobody does so anyway.

For this second category, I don't think there's significant benefit to marking the APIs readonly. Assuming that it's not typical for application code to maintain readonly references to such types, nobody's really going to see the benefit anyway. And it ties our hands such that we'd need to guarantee that these methods remain non-mutating for all time. The tradeoff does not seem worth it.