-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Record structs: add ToString and PrintMembers #52012
Conversation
/// | ||
/// | ||
/// For record struct: | ||
/// public static bool operator==(R r1, R r2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be left and right based on #51973?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will. That'll get picked up next time the feature branch is refreshed with changes from main branch.
4fb75c8
to
94d781b
Compare
@@ -1081,7 +1081,7 @@ record C | |||
"System.String! C.Y { get; init; }", | |||
"System.String! C.Y.get", | |||
"void C.Y.init", | |||
"System.String C.ToString()", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 we were missing the annotation on record's ToString #Resolved
@AlekseyTs @dotnet/roslyn-compiler for review. Thanks |
DeclarationModifiers.Private : | ||
DeclarationModifiers.Protected; | ||
|
||
if (virtualPrintInBase() is object) | ||
if (ContainingType.IsRecord && virtualPrintInBase() is object) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
virtualPrintInBase() [](start = 43, length = 20)
This logic doesn't feel right. If a record class doesn't inherit from object, the method should always have Override
flag. If base doesn't contain the right method to override, an appropriate error is going to be reported about that. #Closed
Is this true for a record struct? #Closed Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordPrintMembers.cs:42 in 21dd0d0. [](commit_id = 21dd0d0, deletion_comment = False) |
@@ -3671,7 +3667,7 @@ MethodSymbol addPrintMembersMethod() | |||
else | |||
{ | |||
printMembersMethod = (MethodSymbol)existingPrintMembersMethod; | |||
if (this.IsSealed && this.BaseTypeNoUseSiteDiagnostics.IsObjectType()) | |||
if ((this.IsSealed && this.BaseTypeNoUseSiteDiagnostics.IsObjectType()) || !isRecordClass) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.IsSealed [](start = 25, length = 13)
Is this true for a record struct? If so, consider checking !isRecordClass
first. #Closed
@@ -124,6 +130,14 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState, | |||
return; | |||
} | |||
} | |||
else if (ContainingType.IsRecordStruct) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ContainingType.IsRecordStruct [](start = 25, length = 29)
It looks like we can combine this condition with the previous if
#Closed
@@ -113,7 +119,7 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState, | |||
return; | |||
} | |||
|
|||
ArrayBuilder<BoundStatement>? block = printableMembers.IsEmpty ? null : ArrayBuilder<BoundStatement>.GetInstance(); | |||
ArrayBuilder<BoundStatement>? block = (printableMembers.IsEmpty && !ContainingType.IsRecordStruct) ? null : ArrayBuilder<BoundStatement>.GetInstance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
&& !ContainingType.IsRecordStruct [](start = 80, length = 33)
Addition of this condition looks suspicious. Are we going to return from the added if
below without releasing the builder? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
F.CloseMethod(F.Return(F.Literal(false))); | ||
return; | ||
} | ||
} | ||
else | ||
{ | ||
MethodSymbol? printMethod = FindValidPrintMembersMethod(ContainingType.BaseTypeNoUseSiteDiagnostics, DeclaringCompilation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FindValidPrintMembersMethod [](start = 48, length = 27)
Usage of this method here doesn't feel right. Should we simply get OverriddenMethod
here? #Closed
It is quite possible I am missing something, but it looks to me that we shouldn't be needing this method. I already commented on the two call sites in this file and I believe we should remove the other remaining call as well. Instead we should override Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordPrintMembers.cs:244 in 21dd0d0. [](commit_id = 21dd0d0, deletion_comment = False) |
// static private bool PrintMembers(System.Text.StringBuilder builder) => throw null; | ||
Diagnostic(ErrorCode.ERR_StaticAPIInRecord, "PrintMembers").WithArguments("C1.PrintMembers(System.Text.StringBuilder)").WithLocation(4, 25) | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} [](start = 8, length = 1)
Are we testing a scenario when synthesized PrintMembers
used explicitly by user code? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we testing a scenario when synthesized PrintMembers used explicitly by user code?
Updated ToString_UserDefinedPrintMembers to cover
It looks like it doesn't call "synthesized PrintMembers".
In reply to: 601913646 [](ancestors = 601913646,601724459)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added ToString_CallingSynthesizedPrintMembers
In reply to: 601929272 [](ancestors = 601929272,601913646,601724459)
Done with review pass (commit 1) #Closed |
Yes. The sealed modifier is applied for all structs in
In reply to: 807085029 [](ancestors = 807085029) Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordPrintMembers.cs:42 in 21dd0d0. [](commit_id = 21dd0d0, deletion_comment = False) |
{ | ||
if (printableMembers.IsEmpty) | ||
{ | ||
// return false; | ||
F.CloseMethod(F.Return(F.Literal(false))); | ||
return; | ||
} | ||
block = ArrayBuilder<BoundStatement>.GetInstance(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
block = ArrayBuilder.GetInstance(); [](start = 20, length = 51)
Consider moving block
's declaration and initialization after the entire if
. #ByDesign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -6781,9 +6879,6 @@ extends A | |||
"; | |||
var comp = CreateCompilationWithIL(new[] { source, IsExternalInitTypeDefinition }, ilSource: ilSource, parseOptions: TestOptions.Regular9); | |||
comp.VerifyEmitDiagnostics( | |||
// (2,19): error CS8864: Records may only inherit from object or another record | |||
// public record C : B | |||
Diagnostic(ErrorCode.ERR_BadRecordBase, "B").WithLocation(2, 19), | |||
// (4,29): error CS8871: 'C.PrintMembers(StringBuilder)' does not override expected method from 'B'. | |||
// protected override bool PrintMembers(System.Text.StringBuilder builder) => throw null; | |||
Diagnostic(ErrorCode.ERR_DoesNotOverrideBaseMethod, "PrintMembers").WithArguments("C.PrintMembers(System.Text.StringBuilder)", "B").WithLocation(4, 29) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ERR_DoesNotOverrideBaseMethod [](start = 37, length = 29)
I would expect the same error reported for a record like this:
public record C : B;
Do we have a test for a scenario like this? #Closed
It doesn't look like the override is added. It feels like we need it and are missing a unit-test for the error scenario when overriding skips immediate base. In reply to: 807125057 [](ancestors = 807125057) Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordPrintMembers.cs:244 in 21dd0d0. [](commit_id = 21dd0d0, deletion_comment = False) |
Done with review pass (commit 3) #Closed |
Done with review pass (commit 3), it looks like there are legitimate test failures. #Closed |
I thought it was covered by check in In reply to: 807855586 [](ancestors = 807855586,807125057) Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordPrintMembers.cs:244 in 21dd0d0. [](commit_id = 21dd0d0, deletion_comment = False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 4)
@@ -48,21 +48,14 @@ protected override DeclarationModifiers MakeDeclarationModifiers(DeclarationModi | |||
#endif | |||
return result; | |||
|
|||
MethodSymbol? virtualPrintInBase() | |||
#if DEBUG | |||
bool modifiersAreValid(DeclarationModifiers modifiers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when the surrounding #if DEBUG
directives are removed from here and the call site? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd emit non-product code into the shipping product.
In reply to: 603564256 [](ancestors = 603564256)
Spec: https://github.com/dotnet/csharplang/blob/main/proposals/record-structs.md (relevant section copied below for convenience)
Test plan: #51199