Declare readonly members for mutable structs #39637
Conversation
JamesNK
requested review from
BrennanConroy,
halter73,
Pilchie and
Tratcher
as code owners
|
|
||
| public int MethodEnd => (int)(uint)(_versionAndMethod >> 32); | ||
| public readonly int MethodEnd => (int)(uint)(_versionAndMethod >> 32); |
There was a problem hiding this comment.
Is this a public type is in an internal namespace? There is some discussion in the runtime about how changing a readonly member to non-readonly in a later version would be considered a breaking change: dotnet/runtime#1718 (comment), something to be aware of.
There was a problem hiding this comment.
TIL it is a breaking change to remove readonly from structs.
I don't see a scenario when these properties would ever not be readonly so I think this is fine.
|
What are the other implications of this? Any gotchas to be worried about? |
At the member level, there aren't really any gotchas other than compat. It's not a breaking change to make a member readonly, but it is a breaking change to remove readonly from a member, so applying readonly to public members on public types is signing up to have that member be readonly forever more. Other than the implications of that, and the tiny additional amount of metadata that comes from [IsReadOnly], there aren't really downsides. As an aside, note that the C# compiler implicitly marks get-only auto-props as readonly. You can see here for example that it's emitting [IsReadOnly] on the getter. |
|
|
||
| object IEnumerator.Current => _current; | ||
| readonly object IEnumerator.Current => _current; |
| @@ -243,11 +243,11 @@ internal Enumerator(HttpRequestHeaders collection) | |||
| : default; | |||
| } | |||
|
|
|||
| public KeyValuePair<string, StringValues> Current => _current; | |||
| public readonly KeyValuePair<string, StringValues> Current => _current; | |||
There was a problem hiding this comment.
The comment is about backwards compat which doesn't apply here because this type is internal. And about auto-properties which doesn't apply because these aren't auto-properties.
You're going to need to provide more detail.
There was a problem hiding this comment.
It was about this #39637 (comment)
|
@JamesNK looking through the comments looks like this is in relatively good shape. The only potential blocker here is the last comment from @davidfowl, and that thread seem to have died. @davidfowl can you please follow up? Thanks! |
JamesNK
force-pushed
the
jamesnk/struct-readonly-members
branch
from
059f764 to
42740b7
Compare
ghost
added this to the 
amcasey
added
area-networking
On Kestrel solution, apply https://docs.microsoft.com/en-us/dotnet/csharp/write-safe-efficient-code#declare-readonly-members-for-mutable-structs