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.

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

Align addition of a synthesized override of Base.Equals(Base? other) in records with the latest design. #45601

Merged
merged 2 commits into from
Jul 2, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 4 additions & 1 deletion src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -6278,9 +6278,12 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<value>A copy constructor in a record must call a copy constructor of the base, or a parameterless object constructor if the record inherits from object.</value>
</data>
<data name="ERR_DoesNotOverrideMethodFromObject" xml:space="preserve">
<value>'{0}' does not override the method from 'object'.</value>
<value>'{0}' does not override expected method from 'object'.</value>
</data>
<data name="ERR_SealedGetHashCodeInRecord" xml:space="preserve">
<value>'{0}' cannot be sealed because containing 'record' is not sealed.</value>
</data>
<data name="ERR_DoesNotOverrideBaseEquals" xml:space="preserve">
<value>'{0}' does not override expected method from '{1}'.</value>
</data>
</root>
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1856,6 +1856,7 @@ internal enum ErrorCode
ERR_CopyConstructorMustInvokeBaseCopyConstructor = 8868,
ERR_DoesNotOverrideMethodFromObject = 8869,
ERR_SealedGetHashCodeInRecord = 8870,
ERR_DoesNotOverrideBaseEquals = 8871,

#endregion diagnostics introduced for C# 9.0
// Note: you will need to re-generate compiler code after adding warnings (eng\generate-compiler-code.cmd)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3002,7 +3002,7 @@ private void AddSynthesizedRecordMembersIfNecessary(MembersAndInitializersBuilde
getOtherEquals(otherEqualsMethods, equalityContract);

var thisEquals = addThisEquals(equalityContract, otherEqualsMethod: otherEqualsMethods.Count == 0 ? null : otherEqualsMethods[0]);
addOtherEquals(otherEqualsMethods, equalityContract, thisEquals);
addOtherEquals();
addObjectEquals(thisEquals);
addHashCode(equalityContract);

Expand Down Expand Up @@ -3148,15 +3148,15 @@ void addHashCode(PropertySymbol equalityContract)
ImmutableArray<CustomModifier>.Empty,
ImmutableArray<MethodSymbol>.Empty);

if (!memberSignatures.TryGetValue(targetMethod, out Symbol? contender))
if (!memberSignatures.TryGetValue(targetMethod, out Symbol? existingHashCodeMethod))
{
var hashCode = new SynthesizedRecordGetHashCode(this, equalityContract, memberOffset: members.Count, diagnostics);
members.Add(hashCode);
}
else
{
var method = (MethodSymbol)contender;
if (!SynthesizedRecordObjectMethod.VerifyOerridesMethodFromObject(method, diagnostics) && method.IsSealed && !IsSealed)
var method = (MethodSymbol)existingHashCodeMethod;
if (!SynthesizedRecordObjectMethod.VerifyOverridesMethodFromObject(method, SpecialType.System_Int32, diagnostics) && method.IsSealed && !IsSealed)
{
diagnostics.Add(ErrorCode.ERR_SealedGetHashCodeInRecord, method.Locations[0], method);
}
Expand Down Expand Up @@ -3210,21 +3210,12 @@ static void getOtherEquals(ArrayBuilder<MethodSymbol> otherEqualsMethods, Proper
}
}

void addOtherEquals(ArrayBuilder<MethodSymbol> otherEqualsMethods, PropertySymbol equalityContract, MethodSymbol thisEquals)
void addOtherEquals()
{
foreach (var otherEqualsMethod in otherEqualsMethods)
if (!BaseTypeNoUseSiteDiagnostics.IsObjectType())
{
var method = new SynthesizedRecordEquals(
this,
parameterType: otherEqualsMethod.Parameters[0].Type,
isOverride: true,
equalityContract,
otherEqualsMethod: thisEquals,
memberOffset: members.Count);
if (!memberSignatures.ContainsKey(method))
{
members.Add(method);
}
var method = new SynthesizedRecordBaseEquals(this, memberOffset: members.Count, diagnostics);
members.Add(method);
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ protected override ImmutableArray<TypeParameterSymbol> MakeTypeParameters(CSharp
}
}

protected override (TypeWithAnnotations ReturnType, ImmutableArray<ParameterSymbol> Parameters, bool IsVararg, ImmutableArray<TypeParameterConstraintClause> DeclaredConstraintsForOverrideOrImplement) MakeParametersAndBindReturnType(DiagnosticBag diagnostics)
protected override (TypeWithAnnotations ReturnType, ImmutableArray<ParameterSymbol> Parameters, bool IsVararg, ImmutableArray<TypeParameterConstraintClause> DeclaredConstraintsForOverrideOrImplementation) MakeParametersAndBindReturnType(DiagnosticBag diagnostics)
{
var syntax = GetSyntax();
var withTypeParamsBinder = this.DeclaringCompilation.GetBinderFactory(syntax.SyntaxTree).GetBinder(syntax.ReturnType, syntax, this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -243,7 +243,7 @@ protected override void MethodChecks(DiagnosticBag diagnostics)
return;
}

protected abstract (TypeWithAnnotations ReturnType, ImmutableArray<ParameterSymbol> Parameters, bool IsVararg, ImmutableArray<TypeParameterConstraintClause> DeclaredConstraintsForOverrideOrImplement) MakeParametersAndBindReturnType(DiagnosticBag diagnostics);
protected abstract (TypeWithAnnotations ReturnType, ImmutableArray<ParameterSymbol> Parameters, bool IsVararg, ImmutableArray<TypeParameterConstraintClause> DeclaredConstraintsForOverrideOrImplementation) MakeParametersAndBindReturnType(DiagnosticBag diagnostics);

protected abstract void ExtensionMethodChecks(DiagnosticBag diagnostics);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

#nullable enable

using System.Collections.Immutable;
using System.Diagnostics;
using System.Linq;

namespace Microsoft.CodeAnalysis.CSharp.Symbols
{
/// <summary>
/// If the record type is derived from a base record type Base, the record type includes
/// a synthesized override of the strongly-typed Equals(Base other). The synthesized
/// override is sealed. It is an error if the override is declared explicitly.
/// The synthesized override returns Equals((object?)other).
/// </summary>
internal sealed class SynthesizedRecordBaseEquals : SynthesizedRecordOrdinaryMethod
{
public SynthesizedRecordBaseEquals(SourceMemberContainerTypeSymbol containingType, int memberOffset, DiagnosticBag diagnostics)
: base(containingType, WellKnownMemberNames.ObjectEquals, memberOffset, diagnostics)
{
}

protected override DeclarationModifiers MakeDeclarationModifiers(DeclarationModifiers allowedModifiers, DiagnosticBag diagnostics)
{
const DeclarationModifiers result = DeclarationModifiers.Public | DeclarationModifiers.Override | DeclarationModifiers.Sealed;
Debug.Assert((result & ~allowedModifiers) == 0);
return result;
}

protected override (TypeWithAnnotations ReturnType, ImmutableArray<ParameterSymbol> Parameters, bool IsVararg, ImmutableArray<TypeParameterConstraintClause> DeclaredConstraintsForOverrideOrImplementation) MakeParametersAndBindReturnType(DiagnosticBag diagnostics)
{
var compilation = DeclaringCompilation;
var location = ReturnTypeLocation;
return (ReturnType: TypeWithAnnotations.Create(Binder.GetSpecialType(compilation, SpecialType.System_Boolean, location, diagnostics)),
Parameters: ImmutableArray.Create<ParameterSymbol>(
new SourceSimpleParameterSymbol(owner: this,
TypeWithAnnotations.Create(ContainingType.BaseTypeNoUseSiteDiagnostics, NullableAnnotation.Annotated),
ordinal: 0, RefKind.None, "", isDiscard: false, Locations)),
IsVararg: false,
DeclaredConstraintsForOverrideOrImplementation: ImmutableArray<TypeParameterConstraintClause>.Empty);
}

protected override int GetParameterCountFromSyntax() => 1;

protected override void MethodChecks(DiagnosticBag diagnostics)
{
base.MethodChecks(diagnostics);

var overridden = OverriddenMethod;

if (overridden is null)
{
return;
}

if (overridden is object &&
Copy link
Contributor

@RikkiGibson RikkiGibson Jul 2, 2020

Choose a reason for hiding this comment

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

This check seems redundant. #Pending

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check seems redundant.

Yes, the previous if can be removed.


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

!overridden.ContainingType.Equals(ContainingType.BaseTypeNoUseSiteDiagnostics, TypeCompareKind.AllIgnoreOptions))
{
diagnostics.Add(ErrorCode.ERR_DoesNotOverrideBaseEquals, Locations[0], this, ContainingType.BaseTypeNoUseSiteDiagnostics);
}
}

internal override void GenerateMethodBody(TypeCompilationState compilationState, DiagnosticBag diagnostics)
{
var F = new SyntheticBoundNodeFactory(this, this.SyntaxNode, compilationState, diagnostics);

try
{
var retExpr = F.Call(
F.This(),
ContainingType.GetMembersUnordered().OfType<SynthesizedRecordObjEquals>().Single(),
Copy link
Contributor

@RikkiGibson RikkiGibson Jul 2, 2020

Choose a reason for hiding this comment

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

Would it make sense to lookup members by name and then filter down to SynthesizedRecordObjEquals? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it make sense to lookup members by name and then filter down to SynthesizedRecordObjEquals?

The code is simpler this way and I don't think it would lead to any noticeable perf difference.


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

F.Convert(F.SpecialType(SpecialType.System_Object), F.Parameter(Parameters[0])));

F.CloseMethod(F.Block(F.Return(retExpr)));
}
catch (SyntheticBoundNodeFactory.MissingPredefinedMember ex)
{
diagnostics.Add(ex.Diagnostic);
F.CloseMethod(F.ThrowNull());
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -178,8 +178,17 @@ internal override IEnumerable<SecurityAttribute> GetSecurityInformation()
internal override void GenerateMethodBody(TypeCompilationState compilationState, DiagnosticBag diagnostics)
{
var F = new SyntheticBoundNodeFactory(this, this.GetNonNullSyntaxNode(), compilationState, diagnostics);
F.CurrentFunction = this;
F.CloseMethod(F.Block(F.Return(F.Typeof(ContainingType))));

try
{
F.CurrentFunction = this;
F.CloseMethod(F.Block(F.Return(F.Typeof(ContainingType))));
}
catch (SyntheticBoundNodeFactory.MissingPredefinedMember ex)
{
diagnostics.Add(ex.Diagnostic);
F.CloseMethod(F.ThrowNull());
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,87 +167,101 @@ internal override IEnumerable<SecurityAttribute> GetSecurityInformation()
internal override void GenerateMethodBody(TypeCompilationState compilationState, DiagnosticBag diagnostics)
{
var F = new SyntheticBoundNodeFactory(this, ContainingType.GetNonNullSyntaxNode(), compilationState, diagnostics);
var other = F.Parameter(Parameters[0]);
BoundExpression? retExpr;

if (IsOverride)
try
{
// This method is an override of a strongly-typed Equals method from a base record type.
// The definition of the method is as follows, and _otherEqualsMethod
// is the method to delegate to (see B.Equals(A), C.Equals(A), C.Equals(B) above):
//
// override bool Equals(Base other) => Equals(other as Derived);
retExpr = F.Call(
F.This(),
_otherEqualsMethod!,
F.As(other, ContainingType));
}
else
{
// This method is the strongly-typed Equals method where the parameter type is
// the containing type.
var other = F.Parameter(Parameters[0]);
BoundExpression? retExpr;

if (_otherEqualsMethod is null)
{
// There are no base record types.
// The definition of the method is as follows (see A.Equals(A) above):
//
// virtual bool Equals(T other) =>
// other != null &&
// EqualityContract == other.EqualityContract &&
// field1 == other.field1 && ... && fieldN == other.fieldN;

// other != null
Debug.Assert(!other.Type.IsStructType());
retExpr = F.ObjectNotEqual(other, F.Null(F.SpecialType(SpecialType.System_Object)));

// EqualityContract == other.EqualityContract
var contractsEqual = F.Binary(
BinaryOperatorKind.ObjectEqual,
F.SpecialType(SpecialType.System_Boolean),
F.Property(F.This(), _equalityContract),
F.Property(other, _equalityContract));

retExpr = retExpr is null ? contractsEqual : F.LogicalAnd(retExpr, contractsEqual);
}
else
if (IsOverride)
{
// There are base record types.
// This method is an override of a strongly-typed Equals method from a base record type.
// The definition of the method is as follows, and _otherEqualsMethod
// is the corresponding method on the nearest base record type to
// delegate to (see B.Equals(B), C.Equals(C) above):
// is the method to delegate to (see B.Equals(A), C.Equals(A), C.Equals(B) above):
//
// virtual bool Equals(Derived other) =>
// base.Equals((Base)other) &&
// field1 == other.field1 && ... && fieldN == other.fieldN;
// override bool Equals(Base other) => Equals(other as Derived);
retExpr = F.Call(
F.Base(_otherEqualsMethod.ContainingType),
F.This(),
_otherEqualsMethod!,
F.Convert(_otherEqualsMethod.Parameters[0].Type, other));
F.As(other, ContainingType));
}

// field1 == other.field1 && ... && fieldN == other.fieldN
// https://github.com/dotnet/roslyn/issues/44895: Should compare fields from non-record base classes.
var fields = ArrayBuilder<FieldSymbol>.GetInstance();
foreach (var f in ContainingType.GetFieldsToEmit())
else
{
if (!f.IsStatic)
// This method is the strongly-typed Equals method where the parameter type is
// the containing type.

if (_otherEqualsMethod is null)
{
fields.Add(f);
// There are no base record types.
// The definition of the method is as follows (see A.Equals(A) above):
//
// virtual bool Equals(T other) =>
// other != null &&
// EqualityContract == other.EqualityContract &&
// field1 == other.field1 && ... && fieldN == other.fieldN;

// other != null
Debug.Assert(!other.Type.IsStructType());
retExpr = F.ObjectNotEqual(other, F.Null(F.SpecialType(SpecialType.System_Object)));

// EqualityContract == other.EqualityContract
var contractsEqual = F.Call(receiver: null, F.WellKnownMethod(WellKnownMember.System_Type__op_Equality),
F.Property(F.This(), _equalityContract),
F.Property(other, _equalityContract));

retExpr = retExpr is null ? (BoundExpression)contractsEqual : F.LogicalAnd(retExpr, contractsEqual);
}
else
{
if (_otherEqualsMethod.ReturnType.SpecialType != SpecialType.System_Boolean)
{
// There was a problem with overriding, an error was reported elsewhere
F.CloseMethod(F.ThrowNull());
return;
}

// There are base record types.
// The definition of the method is as follows, and _otherEqualsMethod
// is the corresponding method on the nearest base record type to
// delegate to (see B.Equals(B), C.Equals(C) above):
//
// virtual bool Equals(Derived other) =>
// base.Equals((Base)other) &&
// field1 == other.field1 && ... && fieldN == other.fieldN;
retExpr = F.Call(
F.Base(_otherEqualsMethod.ContainingType),
_otherEqualsMethod!,
F.Convert(_otherEqualsMethod.Parameters[0].Type, other));
}

// field1 == other.field1 && ... && fieldN == other.fieldN
// https://github.com/dotnet/roslyn/issues/44895: Should compare fields from non-record base classes.
var fields = ArrayBuilder<FieldSymbol>.GetInstance();
foreach (var f in ContainingType.GetFieldsToEmit())
{
if (!f.IsStatic)
{
fields.Add(f);
}
}
if (fields.Count > 0)
{
retExpr = MethodBodySynthesizer.GenerateFieldEquals(
retExpr,
other,
fields,
F);
}
fields.Free();
}
if (fields.Count > 0)
{
retExpr = MethodBodySynthesizer.GenerateFieldEquals(
retExpr,
other,
fields,
F);
}
fields.Free();
}

F.CloseMethod(F.Block(F.Return(retExpr)));
F.CloseMethod(F.Block(F.Return(retExpr)));
}
catch (SyntheticBoundNodeFactory.MissingPredefinedMember ex)
{
diagnostics.Add(ex.Diagnostic);
F.CloseMethod(F.ThrowNull());
}
}
}
}
Loading