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 3 commits
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
13 changes: 4 additions & 9 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 @@ -3509,7 +3509,6 @@ private void AddSynthesizedRecordMembersIfNecessary(MembersAndInitializersBuilde
}
}


if (isRecordClass)
{
addCopyCtor(primaryAndCopyCtorAmbiguity);
Expand All @@ -3534,12 +3533,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 +3666,7 @@ MethodSymbol addPrintMembersMethod()
else
{
printMembersMethod = (MethodSymbol)existingPrintMembersMethod;
if (this.IsSealed && this.BaseTypeNoUseSiteDiagnostics.IsObjectType())
if (!isRecordClass || (this.IsSealed && this.BaseTypeNoUseSiteDiagnostics.IsObjectType()))
{
if (printMembersMethod.DeclaredAccessibility != Accessibility.Private)
{
Expand All @@ -3690,7 +3685,7 @@ MethodSymbol addPrintMembersMethod()
diagnostics.Add(ErrorCode.ERR_SignatureMismatchInRecord, printMembersMethod.Locations[0], printMembersMethod, targetMethod.ReturnType);
}
}
else
else if (isRecordClass)
{
SynthesizedRecordPrintMembers.VerifyOverridesPrintMembersFromBase(printMembersMethod, diagnostics);
}
Expand Down
3 changes: 1 addition & 2 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceNamedTypeSymbol_Bases.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,7 @@ protected override void CheckBase(BindingDiagnosticBag diagnostics)

if (declaration.Kind == DeclarationKind.Record)
{
if (SynthesizedRecordClone.FindValidCloneMethod(localBase, ref useSiteInfo) is null ||
SynthesizedRecordPrintMembers.FindValidPrintMembersMethod(localBase, DeclaringCompilation) is null)
if (SynthesizedRecordClone.FindValidCloneMethod(localBase, ref useSiteInfo) is null)
{
diagnostics.Add(ErrorCode.ERR_BadRecordBase, baseLocation);
}
Expand Down
74 changes: 18 additions & 56 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 && !ContainingType.BaseTypeNoUseSiteDiagnostics.IsObjectType())
{
result |= DeclarationModifiers.Override;
}
Expand All @@ -48,21 +48,14 @@ protected override DeclarationModifiers MakeDeclarationModifiers(DeclarationModi
#endif
return result;

MethodSymbol? virtualPrintInBase()
#if DEBUG
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)

{
NamedTypeSymbol baseType = ContainingType.BaseTypeNoUseSiteDiagnostics;

if (!baseType.IsObjectType())
if (ContainingType.IsRecordStruct)
{
return FindValidPrintMembersMethod(baseType, ContainingType.DeclaringCompilation);
return modifiers == DeclarationModifiers.Private;
}

return null;
}

#if DEBUG
static bool modifiersAreValid(DeclarationModifiers modifiers)
{
if ((modifiers & DeclarationModifiers.AccessibilityMask) != DeclarationModifiers.Private &&
(modifiers & DeclarationModifiers.AccessibilityMask) != DeclarationModifiers.Protected)
{
Expand All @@ -88,10 +81,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,27 +107,29 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState,
return;
}

ArrayBuilder<BoundStatement>? block = printableMembers.IsEmpty ? null : ArrayBuilder<BoundStatement>.GetInstance();
ArrayBuilder<BoundStatement> block;
BoundParameter builder = F.Parameter(this.Parameters[0]);
if (ContainingType.BaseTypeNoUseSiteDiagnostics.IsObjectType())
if (ContainingType.BaseTypeNoUseSiteDiagnostics.IsObjectType() || ContainingType.IsRecordStruct)
{
if (printableMembers.IsEmpty)
{
// return false;
F.CloseMethod(F.Return(F.Literal(false)));
return;
}
block = ArrayBuilder<BoundStatement>.GetInstance();
Copy link
Contributor

@AlekseyTs AlekseyTs Mar 26, 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.

block = ArrayBuilder.GetInstance(); [](start = 20, length = 51)

Consider moving block's declaration and initialization after the entire if. #ByDesign

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll leave this as-is. Seems fine

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

}
else
{
MethodSymbol? printMethod = FindValidPrintMembersMethod(ContainingType.BaseTypeNoUseSiteDiagnostics, DeclaringCompilation);
if (printMethod is null)
MethodSymbol? basePrintMethod = OverriddenMethod;
if (basePrintMethod is null ||
basePrintMethod.ReturnType.SpecialType != SpecialType.System_Boolean)
{
F.CloseMethod(F.ThrowNull()); // an error was reported in base checks already
return;
}

var basePrintCall = F.Call(receiver: F.Base(ContainingType.BaseTypeNoUseSiteDiagnostics), printMethod, builder);
var basePrintCall = F.Call(receiver: F.Base(ContainingType.BaseTypeNoUseSiteDiagnostics), basePrintMethod, builder);
if (printableMembers.IsEmpty)
{
// return base.PrintMembers(builder);
Expand All @@ -142,13 +138,14 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState,
}
else
{
block = ArrayBuilder<BoundStatement>.GetInstance();
// if (base.PrintMembers(builder))
// builder.Append(", ")
block!.Add(F.If(basePrintCall, makeAppendString(F, builder, ", ")));
block.Add(F.If(basePrintCall, makeAppendString(F, builder, ", ")));
}
}

Debug.Assert(!printableMembers.IsEmpty && block is object);
Debug.Assert(!printableMembers.IsEmpty);

for (var i = 0; i < printableMembers.Length; i++)
{
Expand Down Expand Up @@ -227,41 +224,6 @@ static bool isPrintable(Symbol m)
}
}

internal static MethodSymbol? FindValidPrintMembersMethod(TypeSymbol containingType, CSharpCompilation compilation)
{
if (containingType.IsObjectType())
{
return null;
}

MethodSymbol? candidate = null;
var stringBuilder = TypeWithAnnotations.Create(compilation.GetWellKnownType(WellKnownType.System_Text_StringBuilder));

foreach (var member in containingType.GetMembers(WellKnownMemberNames.PrintMembersMethodName))
{
if (member is MethodSymbol { DeclaredAccessibility: Accessibility.Protected, IsStatic: false, ParameterCount: 1, Arity: 0 } method &&
method.ParameterTypesWithAnnotations[0].Equals(stringBuilder, TypeCompareKind.AllIgnoreOptions))
{
if (candidate is object)
{
// An ambiguity case, can come from metadata, treat as an error for simplicity.
return null;
}

candidate = method;
}
}

if (candidate is null ||
!(containingType.IsSealed || candidate.IsOverride || candidate.IsVirtual) ||
candidate.ReturnType.SpecialType != SpecialType.System_Boolean)
{
return null;
}

return candidate;
}

internal static void VerifyOverridesPrintMembersFromBase(MethodSymbol overriding, BindingDiagnosticBag diagnostics)
{
NamedTypeSymbol baseType = overriding.ContainingType.BaseTypeNoUseSiteDiagnostics;
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