Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Record structs: add ToString and PrintMembers #52012

Merged
merged 4 commits into from
Mar 29, 2021
Merged

Record structs: add ToString and PrintMembers #52012

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
21dd0d0
Record structs: add ToString and PrintMembers
jcouv Mar 20, 2021
6aaf5aa
Address feedback
jcouv Mar 25, 2021
00dc122
Remove FindValidPrintMembersMethod
jcouv Mar 25, 2021
e906957
Add MethodChecks and tests
jcouv Mar 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 4 additions & 8 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3534,12 +3534,8 @@ private void AddSynthesizedRecordMembersIfNecessary(MembersAndInitializersBuilde
diagnostics.Add(ErrorCode.WRN_RecordEqualsWithoutGetHashCode, thisEquals.Locations[0], declaration.Name);
}

// PROTOTYPE(record-structs): update for record structs
if (isRecordClass)
{
var printMembers = addPrintMembersMethod();
addToStringMethod(printMembers);
}
var printMembers = addPrintMembersMethod();
addToStringMethod(printMembers);

memberSignatures.Free();

Expand Down Expand Up @@ -3671,7 +3667,7 @@ MethodSymbol addPrintMembersMethod()
else
{
printMembersMethod = (MethodSymbol)existingPrintMembersMethod;
if (this.IsSealed && this.BaseTypeNoUseSiteDiagnostics.IsObjectType())
if ((this.IsSealed && this.BaseTypeNoUseSiteDiagnostics.IsObjectType()) || !isRecordClass)
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 25, 2021 •
edited
Loading

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

{
if (printMembersMethod.DeclaredAccessibility != Accessibility.Private)
{
Expand All @@ -3690,7 +3686,7 @@ MethodSymbol addPrintMembersMethod()
diagnostics.Add(ErrorCode.ERR_SignatureMismatchInRecord, printMembersMethod.Locations[0], printMembersMethod, targetMethod.ReturnType);
}
}
else
else if (isRecordClass)
{
SynthesizedRecordPrintMembers.VerifyOverridesPrintMembersFromBase(printMembersMethod, diagnostics);
}
Expand Down
24 changes: 19 additions & 5 deletions src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordPrintMembers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@ public SynthesizedRecordPrintMembers(

protected override DeclarationModifiers MakeDeclarationModifiers(DeclarationModifiers allowedModifiers, BindingDiagnosticBag diagnostics)
{
var result = (ContainingType.BaseTypeNoUseSiteDiagnostics.IsObjectType() && ContainingType.IsSealed) ?
var result = (ContainingType.IsRecordStruct || (ContainingType.BaseTypeNoUseSiteDiagnostics.IsObjectType() && ContainingType.IsSealed)) ?
DeclarationModifiers.Private :
DeclarationModifiers.Protected;

if (virtualPrintInBase() is object)
if (ContainingType.IsRecord && virtualPrintInBase() is object)
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 25, 2021 •
edited
Loading

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

{
result |= DeclarationModifiers.Override;
}
Expand Down Expand Up @@ -61,8 +61,13 @@ protected override DeclarationModifiers MakeDeclarationModifiers(DeclarationModi
}

#if DEBUG
static bool modifiersAreValid(DeclarationModifiers modifiers)
bool modifiersAreValid(DeclarationModifiers modifiers)
Copy link
Contributor

@RikkiGibson RikkiGibson Mar 29, 2021 •
edited by jcouv
Loading

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

Copy link
Member Author

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)

{
if (ContainingType.IsRecordStruct)
{
return modifiers == DeclarationModifiers.Private;
}

if ((modifiers & DeclarationModifiers.AccessibilityMask) != DeclarationModifiers.Private &&
(modifiers & DeclarationModifiers.AccessibilityMask) != DeclarationModifiers.Protected)
{
Expand All @@ -88,10 +93,11 @@ protected override (TypeWithAnnotations ReturnType, ImmutableArray<ParameterSymb
{
var compilation = DeclaringCompilation;
var location = ReturnTypeLocation;
var annotation = ContainingType.IsRecordStruct ? NullableAnnotation.Oblivious : NullableAnnotation.NotAnnotated;
return (ReturnType: TypeWithAnnotations.Create(Binder.GetSpecialType(compilation, SpecialType.System_Boolean, location, diagnostics)),
Parameters: ImmutableArray.Create<ParameterSymbol>(
new SourceSimpleParameterSymbol(owner: this,
TypeWithAnnotations.Create(Binder.GetWellKnownType(compilation, WellKnownType.System_Text_StringBuilder, diagnostics, location), NullableAnnotation.NotAnnotated),
TypeWithAnnotations.Create(Binder.GetWellKnownType(compilation, WellKnownType.System_Text_StringBuilder, diagnostics, location), annotation),
ordinal: 0, RefKind.None, "builder", isDiscard: false, Locations)),
IsVararg: false,
DeclaredConstraintsForOverrideOrImplementation: ImmutableArray<TypeParameterConstraintClause>.Empty);
Expand All @@ -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();
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 25, 2021 •
edited
Loading

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. That was indeed a problem.

In reply to: 601677092 [](ancestors = 601677092)

BoundParameter builder = F.Parameter(this.Parameters[0]);
if (ContainingType.BaseTypeNoUseSiteDiagnostics.IsObjectType())
{
Expand All @@ -124,6 +130,14 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState,
return;
}
}
else if (ContainingType.IsRecordStruct)
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 25, 2021 •
edited
Loading

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

{
if (printableMembers.IsEmpty)
{
F.CloseMethod(F.Return(F.Literal(false)));
return;
}
}
else
{
MethodSymbol? printMethod = FindValidPrintMembersMethod(ContainingType.BaseTypeNoUseSiteDiagnostics, DeclaringCompilation);
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 25, 2021 •
edited
Loading

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

Expand Down
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordToString.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ protected override (TypeWithAnnotations ReturnType, ImmutableArray<ParameterSymb
{
var compilation = DeclaringCompilation;
var location = ReturnTypeLocation;
return (ReturnType: TypeWithAnnotations.Create(Binder.GetSpecialType(compilation, SpecialType.System_String, location, diagnostics)),
var annotation = ContainingType.IsRecordStruct ? NullableAnnotation.Oblivious : NullableAnnotation.NotAnnotated;
return (ReturnType: TypeWithAnnotations.Create(Binder.GetSpecialType(compilation, SpecialType.System_String, location, diagnostics), annotation),
Parameters: ImmutableArray<ParameterSymbol>.Empty,
IsVararg: false,
DeclaredConstraintsForOverrideOrImplementation: ImmutableArray<TypeParameterConstraintClause>.Empty);
Expand Down
Loading