Make System.Guid readonly

[Gnbrkm41]

Related: #1718

This PR makes System.Guid as readonly struct. While none of the APIs mutate the instances, implementations of GUID parsing, copy and comparisons involve taking refs of those fields and assigning new values, which is not allowed by C# with readonly fields.

To work around these with minimal code changes, Unsafe.AsRef is used whenever we need to pass something as ref. These were really just for bitwise-comparison or for MemoryMarshal.TryWrite.
I'm honestly surprised by the fact that MemoryMarshal.TryWrite takes ref T instead of in T as it does not mutate the passed reference.

For the parsing implementation, rather large portions of it were mutating the guid; I ended up type-punning by creating a structurally identical struct then Unsafe.Asing it.

Not sure if API review is required for this? All the public members should not mutate the instance already anyway (and would make this a mutable struct if any of them are, which is usually a bad idea)

[tannergooding]

Is the new code really better than marking just the non-mutating methods as readonly?

Could we get a list of which instance methods mutate vs which do not?

[bartonjs]

Is marking the fields as readonly going to break binary serialization?

[Gnbrkm41]

Just did a simple experiment of creating a struct with a non-readonly field, serialising it to a file, marking the field and the struct as readonly and deserialising it; It appears to work fine without exceptions. Though, I haven't really used binary serialisation so I could be wrong.

[danmoseley]

Please verify the reverse as well since that's also supported (although, if ->readonly works, surely <-readonly does too)

[Gnbrkm41]

The opposite seems to be true as well.

[Gnbrkm41]

Is the new code really better than marking just the non-mutating methods as readonly?

I'm not sure. None of the methods are documented to mutate the structure, and having every methods marked as readonly yet not having the type itself marked readonly because of its private implementation feels weird to me.

Could we get a list of which instance methods mutate vs which do not?

There actually is no instance methods that mutate. Only the ctors/static methods related with the creation of new Guid Instances actually mutate as part of the generation.

• Guid.NewGuid (Only on Unix)

TryParse/Parse, new Guid(string) and new Guid(ReadOnlyChar<char>) use these private methods that assign new values to fields:

• TryParseExactD
• TryParseExactN

[jkotas]

What would it take to refactor this cleanly, without introducing the MutableGuid type?

[jkotas]

Hmm, I guess the performance of parsing would regress without MutableGuid or it would need Unsafe hacks in more places.

[Gnbrkm41]

Does this need to be evaluated as breaking change if code like this requires compensating for this?

In this case, all the fields are private so existing user interop codes (in fact, any other code outside of the Guid type) must use unsafe methods (e.g. taking pointers, S.R.CS.Unsafe, S.R.IS.Marshal...) to access the values anyway, which wouldn't be affected by those readonly modifiers.

(Also, I agree that the delta looks pretty ugly.)
What would it take to refactor this cleanly, without introducing the MutableGuid type?

For the parsing I suppose we could Introduce a bunch of local variables, continue with the same logic & call new Guid(Int32, Int16, Int16, Byte, Byte, Byte, Byte, Byte, Byte, Byte, Byte) at the end. Not sure if the JIT would be able to turn it into something of similar efficiency though; Maybe I'm worried too much about the performance.

For the Guid.NewGuid maybe I should have turned it to something on the line of stackalloc byte[sizeof(Guid)] followed by a reinterpret-cast at the end.

[tannergooding]

having every methods marked as readonly yet not having the type itself marked readonly

We have some other types, such as the System.Numerics.Vector* types, that are in the same boat. They are functionally readonly, but due to the fields being publicly exposed we can't mark the entire struct as readonly.

However, marking the instance methods as readonly achieves the desired affect as removing hidden copies.

I think it might be worth keeping the code simpler, especially since it is performance oriented, and just marking the various instance methods as readonly instead.

[Gnbrkm41]

We have some other types, such as the System.Numerics.Vector* types, that are in the same boat. They are functionally readonly, but due to the fields being publicly exposed we can't mark the entire struct as readonly.

In this case though, none of the fields are exposed, so it wouldn't be a breaking change to mark the whole type as readonly (unlike the Vector types as the fields can be mutated in user code)

However, marking the instance methods as readonly achieves the desired affect as removing hidden copies.

While it is true that marking all the instance methods would achieve the same effect, Marking the whole type readonly conveys that Guid is a immutable structure (which it really is). We could have easily went with an implementation that does not require mutation of the internal structure and mark it as readonly, but we're not doing so for (possible) optimisation reasons during construction, and looking from outside they would look the same. Marking the type as non-readonly for just that does not seem right to me.

[Gnbrkm41]

I'll explore a bit more to see if there's alternative ways to approach this tomorrow (of course, while not compromising on the performance aspect).

Just wondering - is there an "easy way" to obtain JIT output for those framework methods?

[Gnbrkm41]

Gave another thought about it... and realised, wouldn't we need to do the same thing even if we opt to mark individual members as readonly, as C# wouldn't still allow mutation of the fields in readonly methods?

[bartonjs]

wouldn't we need to do the same thing even if we opt to mark individual members as readonly

The MutableGuid type would go away, as it's only used in static methods. The ctor usages of Unsafe.AsRef would go away... GetHashCode and Equals might still need Unsafe.AsRef.

[GrabYourPitchforks]

Would it also make sense to have a unit test that iterates over all the public members of Guid and verifies that they're annotated with [IsReadOnly]? That way if we add public API in the future there's less of a chance of us accidentally shipping a method that's not annotated appropriately. This might be a viable trade-off if we're not willing to annotate the entire type.

[Gnbrkm41]

It looks like it'll be worth some extra discussion perhaps. Closing for now & Will open an issue.