Runtime: Do a pass over CoreFX for the C# readonly members feature

[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).

It would be useful to have a pass over CoreFX and annotate methods on non-readonly structs which do not and will never mutate the state of the instance.

There are a number of these methods in the System.Numerics namespace which users also may try to pass around as in.

[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: https://github.com/dotnet/corefx/pull/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.