Add support for readonly members in structs #673
Conversation
BillWagner
force-pushed
the
readonly-members
branch
from
6504c59 to
956667a
Compare
BillWagner
force-pushed
the
readonly-members
branch
from
956667a to
15b4f03
Compare
standard/structs.md
Outdated
| @@ -502,6 +505,50 @@ Automatically implemented properties ([§15.7.4](classes.md#1574-automatically-i | |||
|
|
|||
| > *Note*: This access restriction means that constructors in structs containing automatically implemented properties often need an explicit constructor initializer where they would not otherwise need one, to satisfy the requirement of all fields being definitely assigned before any function member is invoked or the constructor returns. *end note* | |||
|
|
|||
| A *property_declaration* ([§14.7.1](classes.md#1471-general)) for an instance property in a *struct_declaration* may contain the *property_modifier* `readonly`. However, a static property shall not contain that modifier. | |||
There was a problem hiding this comment.
Does "However, a static property shall not contain that modifier" also cover the case where the readonly modifier is on an accessor of the static property?
struct S {
static int P {
readonly get => 0;
set {}
}
}
|
rebased on 09/26/2023, various links also fixed. |
jskeet
added
the
meeting: discuss
There was a problem hiding this comment.
Overall it feels like we may be missing a brief section explaining the purpose of readonly members on structs.
I'd also expect there to be a change somewhere to what happens when member access via a readonly field (in either a class or a struct) invokes a readonly member... shouldn't we be removing a defensive copying step there? Apologies if I've missed it... I look forward to it being pointed out :)
standard/structs.md
Outdated
| @@ -154,6 +153,7 @@ Structs differ from classes in several important ways: | |||
| - Instance field declarations for a struct are not permitted to include variable initializers ([§16.4.8](structs.md#1648-field-initializers)). | |||
| - A struct is not permitted to declare a parameterless instance constructor ([§16.4.9](structs.md#1649-constructors)). | |||
| - A struct is not permitted to declare a finalizer. | |||
| - Some kinds of members in structs are permitted to have the modifier `readonly` while that is not generally permitted for those same member kinds in classes. | |||
There was a problem hiding this comment.
While I can appreciate the desire not to repeat things, I suspect it's worth being more specific about which members can have readonly in structs. The "some" here feels really woolly.
There was a problem hiding this comment.
This would be addressed by my suggestion below, assuming I haven't missed anything.
See #1053 |
There was a problem hiding this comment.
I think we need to pull out bits into a common ”readonly members” section so we have a single place to define what being a readonly member means.
Though I kept it in my suggestion changes I think “is contained directly by a struct_declaration” is a bit unwieldly, can we not use something like “is a member of a struct_declaration” – I don't think membership in the general sense ("member of the judo club") is automatically read to include grandchildren. But as I say I left it as it…
|
It’s the next day and I’m commenting on my own introductory comment on my review as an important issue probably doesn't come across clearly in the review itself:
Unless I'm missing something the Standard is missing a clear explanation of semantics of read-only members, and those semantics some might find “surprising”. It is reasonable to assume that a read-only member reports on the current state of an object and does not modify it. C# does the latter but not necessarily the former... If a read-only member attempts to modify the state a reasonable assumption is that an error would be produced, the member is read-only after all. C# chooses a different approach: it makes a copy of the object, mutates the state of that copy, and then discards the copy. The C# choice could be “surprising”, something Roslyn seems to acknowledge by issuing a warning, but I don’t think the Standard is clear as it could be on the semantics as it is preoccupied with the “how” a compiler might achieve this result, the so called “defensive copies”. Such an description doesn’t need to be long and complicated, it doesn’t need to justify C#’s choice, it just needs to be clear. Refs: #673 (comment) & #1053 |
BillWagner
force-pushed
the
readonly-members
branch
from
7fb7df9 to
e58cbcd
Compare
|
rebased on dotnet/draft-v8 |
There was a problem hiding this comment.
This is a great example PR for our new process.
I didn't find any new issues in this review. The additional comments should provide a clear path to completion, or a couple possible paths.
I see either of these as possible:
- Review and build the list in the meeting. Merge, and create issues to address identified issues.
- Leave open at the meeting. Assign one person for edits, and a second for review and merge prior to the next meeting. Add one or more comments that note the expected plan for each open discussion offline. Edit and merge offline before the June meeting.
|
Could there be value in describing what a "readonly struct member" is, and apply that to each instance member type (method, property, indexer, event) without repeating the whole definition? |
Co-authored-by: Nigel-Ecma <[email protected]>
Co-authored-by: Nigel-Ecma <[email protected]>
RexJaeschke
removed
the
meeting: priority
|
On the 2025-05-14 call: After 20 minutes, and accepting/tweaking some changes, we decided this PR is not yet ready to be merged. Action: Jon and Bill will review the remaining issues offline. For each remaining comment, they will add a new comment suggesting the disposition. |
There was a problem hiding this comment.
@BillWagner I've looked through this and commented or thumbs-upped each one that sounded potentially blocking. Please could you:
- Commit suggestions that you're happy with
- Comment on my suggestions for deferring with an issue
- Reply on anything else that doesn't come under one of those
standard/structs.md
Outdated
| @@ -154,6 +153,7 @@ Structs differ from classes in several important ways: | |||
| - Instance field declarations for a struct are not permitted to include variable initializers ([§16.4.8](structs.md#1648-field-initializers)). | |||
| - A struct is not permitted to declare a parameterless instance constructor ([§16.4.9](structs.md#1649-constructors)). | |||
| - A struct is not permitted to declare a finalizer. | |||
| - Some kinds of members in structs are permitted to have the modifier `readonly` while that is not generally permitted for those same member kinds in classes. | |||
There was a problem hiding this comment.
This would be addressed by my suggestion below, assuming I haven't missed anything.
Co-authored-by: Nigel-Ecma <[email protected]> Co-authored-by: Jon Skeet <[email protected]>
Apply suggestions and edit recommendations from the May meeting.
I made three commits today (June 3)
Other open issues are described in #1339 I think this is ready to merge. |
BillWagner
added
the
meeting: priority
jskeet
dismissed
Nigel-Ecma’s stale review
Lots has changed since Nigel left comments, and we believe they've all been addressed.
|
Test failure: |
|
But conversions.md isn't changed by this PR, so I'll merge this and we'll need to fix it separately. |
The standard now includes the language for `readonly` instance members in `struct` types. See dotnet/csharpstandard#673. Remove the feature speclet from the learn site.
* remove readonly speclet The standard now includes the language for `readonly` instance members in `struct` types. See dotnet/csharpstandard#673. Remove the feature speclet from the learn site. * build error * warnings, part 2
|
@jskeet That is the error caused by #1334 (review) and fixed by #1336. |
No description provided.