-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Proposal]: Records with sealed base ToString override #4174
Comments
I was about to open an issue for the the exact same problem, and discovered that you did it yesterday... Good timing! Apparently this restriction was carefully specified:
But in some scenarios, it's killing one of the biggest benefits of records: conciseness. Until this is fixed, I'm working around the issue using source generators, but it's annoying, because writing source generators is time-consuming. |
@333fred what's the status of this proposal? Has it been discussed in LDM yet? Is there any chance it could be included in C#10? |
@thomaslevesque this proposal is in the Any Time milestone. What that means is that it is not a priority for the compiler team at the moment, but we'd accept a PR implementing the feature from the community. |
@333fred thanks. |
@thomaslevesque https://aka.ms/dotnet-discord has a |
Grabbing the error IDs from SharpLab, it looks like there will be two errors that will need to stop being emitted in this situation:
The Roslyn codebase (excluding tests) only has two references to the enum member representing CS8870, one clearly producing the diagnostic for ToString and the other clearly for GetHashCode: http://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/Errors/ErrorCode.cs,7a7a4a52c24276bc,references The other error will need to be fixed not by skipping emitting a diagnostic but by skipping emitting synthesized ToString overrides. A test will probably be needed that shows that a ToString was actually generated as sealed in the base class. Not sure how much help this is, but here's a PR demonstrating emit changes and emit tests: dotnet/roslyn#43281 |
@jnm2 thanks, that's helpful. |
I'm wondering if this change should be applied just to |
Either way, why would that mean this change should also be applied to allow sealing of GetHashCode? What's the motivation? |
LDM has only approved ToString. And I question the motivation of the others. ToString is a nicer representation. Equals and GetHashcode are fundamental pieces of how records work. |
You're right, there's no motivation for sealing GetHashCode. I'm just trying to get my head around why Equals and GetHashCode are handled differently.
Indeed. Which is why I wonder why they don't have the same restrictions. I assume the reason Equals can't be overridden has to do with EqualityContract, but then, why allow GetHashCode to be overridden, since it's so closely related to Equals?
Good enough for me! |
|
I submitted a PR: dotnet/roslyn#52029 |
Ah, I see. I didn't realize that before. |
The feature is merged and will be in preview for a while before shipping for C# 10 (.NET 6). Thanks @thomaslevesque |
Records with sealed base ToString override
Summary
This proposal would lift the restriction on declaring sealed
ToString
overrides in non-sealed records and lift the restriction on a record inheriting from a base type that has a sealedToString
override. The semantics ofsealed override
are clear, they are appropriate for closed hierarchies whereToString
is controlled by the base, and the current errors seem to deny its otherwise consistent functioning out of an overabundance of caution.Motivation
There's currently no way to specify a
ToString
implementation once for all derived records. Because of this, the need to overrideToString
can have a jarring result. Consider this hand-rolled discriminated union based on records:Overriding
ToString
has no effect on derived records unless each derived record is given a manual override that returnsbase.ToString()
. Sealing the override would perfectly describe the intended result, but this is currently blocked by the compiler with errors CS0239 and CS8870.The simplest option currently available is much less pleasing. One factor is its repetition. Another factor is that the base record no longer contains a readable list of single-line members.
Detailed design
Non-sealed records would support sealed overrides of
ToString
the same way classes do rather than producing error "CS8870 'BaseRecord.ToString()' cannot be sealed because containing record is not sealed."Derived records would support inheriting when the base
ToString
is sealed by skipping the synthesizedToString
override rather than producing error "CS0239 'DerivedRecord.ToString()': cannot override inherited member 'BaseRecord.ToString()' because it is sealed."Drawbacks
Alternatives
A community source generator could be developed that would look for an attribute on the base record or the base record's
ToString
override and addpublic override string ToString() => base.ToString();
to each derived record that doesn't already declare aToString
member. This feels like a workaround at best. The semantics ofsealed override
are clear, they are appropriate for closed hierarchies whereToString
is controlled by the base, and the current errors seem to deny its otherwise consistent functioning out of an overabundance of caution.Unresolved questions
Design meetings
https://github.com/dotnet/csharplang/blob/main/meetings/2021/LDM-2021-02-10.md#sealed-record-ToString
The text was updated successfully, but these errors were encountered: