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

Records: Support EqualityContract in Equals #44882

Merged
merged 8 commits into from
Jun 6, 2020
Merged
Show file tree
Hide file tree
Changes from 3 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
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

#nullable enable

using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using Microsoft.CodeAnalysis.PooledObjects;
Expand Down Expand Up @@ -249,18 +248,18 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState,
/// Contains methods related to synthesizing bound nodes in lowered form
/// that does not need any processing before passing to codegen
/// </summary>
internal static partial class MethodBodySynthesizer
internal static class MethodBodySynthesizer
{
/// <summary>
/// Given a set of fields, produce an expression that is true when all of the given fields on
/// `this` are equal to the fields on <paramref name="otherReceiver" /> according to the
/// default EqualityComparer.
/// </summary>
public static BoundExpression GenerateFieldEquals<TList>(
public static BoundExpression GenerateFieldEquals(
BoundExpression? initialExpression,
BoundExpression otherReceiver,
TList fields,
SyntheticBoundNodeFactory F) where TList : IReadOnlyList<FieldSymbol>
ArrayBuilder<FieldSymbol> fields,
SyntheticBoundNodeFactory F)
{
Debug.Assert(fields.Count > 0);

Expand Down
15 changes: 3 additions & 12 deletions src/Compilers/CSharp/Portable/Symbols/LexicalSortKey.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,15 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Generic;
using System.Diagnostics;
using System.Threading;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Text;

namespace Microsoft.CodeAnalysis.CSharp.Symbols
{
/// <summary>
/// A structure used to lexically order symbols. For performance, it's important that this be
/// a STRUCTURE, and be able to be returned from a symbol without doing any additional allocations (even
/// if nothing is cached yet.)
/// if nothing is cached yet).
/// </summary>
internal struct LexicalSortKey
{
Expand Down Expand Up @@ -43,18 +38,14 @@ public int Position

public static readonly LexicalSortKey NotInitialized = new LexicalSortKey() { _treeOrdinal = -1, _position = -1 };

// Put Record Equals right before synthesized constructors.
public static LexicalSortKey SynthesizedRecordEquals => new LexicalSortKey() { _treeOrdinal = int.MaxValue, _position = int.MaxValue - 5 };
public static LexicalSortKey SynthesizedRecordObjEquals => new LexicalSortKey() { _treeOrdinal = int.MaxValue, _position = int.MaxValue - 4 };

// Put other synthesized members right before synthesized constructors.
public static LexicalSortKey GetSynthesizedMemberKey(int offset) => new LexicalSortKey() { _treeOrdinal = int.MaxValue, _position = int.MaxValue - 2 - offset };

// Dev12 compiler adds synthetic constructors to the child list after adding all other members.
// Methods are emitted in the children order, but synthetic cctors would be deferred
// until later when it is known if they can be optimized or not.
// As a result the last emitted method tokens are synthetic ctor and then synthetic cctor (if not optimized)
// Since it is not too hard, we will try keeping the same order just to be easy on metadata diffing tools and such.
public static readonly LexicalSortKey SynthesizedRecordCopyCtor = new LexicalSortKey() { _treeOrdinal = int.MaxValue, _position = int.MaxValue - 3 };
public static readonly LexicalSortKey SynthesizedRecordCtor = new LexicalSortKey() { _treeOrdinal = int.MaxValue, _position = int.MaxValue - 2 };
public static readonly LexicalSortKey SynthesizedCtor = new LexicalSortKey() { _treeOrdinal = int.MaxValue, _position = int.MaxValue - 1 };
public static readonly LexicalSortKey SynthesizedCCtor = new LexicalSortKey() { _treeOrdinal = int.MaxValue, _position = int.MaxValue };

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2991,10 +2991,16 @@ private void AddSynthesizedRecordMembersIfNecessary(MembersAndInitializersBuilde
addCopyCtor();
addCloneMethod();

var thisEquals = addThisEquals();
var equalityContract = addEqualityContract();
Copy link
Member

Choose a reason for hiding this comment

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

var [](start = 12, length = 3)

nit: having type (PropertySymbol) would be helpful for readability here

var otherEqualsMethods = ArrayBuilder<MethodSymbol>.GetInstance(); // PROTOTYPE: We don't need to hold onto the other Equals methods. The values aren't used.
getOtherEquals(otherEqualsMethods, equalityContract);

var thisEquals = addThisEquals(equalityContract, otherEqualsMethods.Count == 0 ? null : otherEqualsMethods[0]);
Copy link
Member

Choose a reason for hiding this comment

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

, [](start = 59, length = 1)

nit: consider naming the second argument

addOtherEquals(otherEqualsMethods, equalityContract, thisEquals);
addObjectEquals(thisEquals);
addHashCode();

otherEqualsMethods.Free();
memberSignatures.Free();

return;
Expand All @@ -3017,7 +3023,7 @@ SynthesizedRecordConstructor addCtor(RecordDeclarationSyntax declWithParameters)

void addCopyCtor()
{
var ctor = new SynthesizedRecordCopyCtor(this, diagnostics);
var ctor = new SynthesizedRecordCopyCtor(this, memberOffset: members.Count);
if (!memberSignatures.ContainsKey(ctor))
{
members.Add(ctor);
Expand All @@ -3040,7 +3046,7 @@ void addProperties(ImmutableArray<ParameterSymbol> recordParameters)
{
var property = new SynthesizedRecordPropertySymbol(this, param, diagnostics);
if (!memberSignatures.ContainsKey(property) &&
!hidesInheritedMember(property, this))
getInheritedMember(property, this) is null)
{
members.Add(property);
members.Add(property.GetMethod);
Expand All @@ -3062,7 +3068,7 @@ void addProperties(ImmutableArray<ParameterSymbol> recordParameters)
#endif
}

static bool hidesInheritedMember(Symbol symbol, NamedTypeSymbol type)
static Symbol? getInheritedMember(Symbol symbol, NamedTypeSymbol type)
Copy link
Member

@jcouv jcouv Jun 6, 2020

Choose a reason for hiding this comment

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

getInheritedMember [](start = 27, length = 18)

What's the benefit of returning a Symbol rather than a bool? It looks like we're only using the value to test for null. #Pending

Copy link
Member Author

@cston cston Jun 6, 2020

Choose a reason for hiding this comment

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

Hmm, I was using the Symbol at one point, but not now. Reverted. #Pending

{
while ((type = type.BaseTypeNoUseSiteDiagnostics) is object)
{
Expand All @@ -3076,20 +3082,21 @@ static bool hidesInheritedMember(Symbol symbol, NamedTypeSymbol type)
out var hiddenBuilder);
if (hiddenBuilder is object)
{
var result = hiddenBuilder[0];
hiddenBuilder.Free();
return true;
return result;
}
if (bestMatch is object)
{
return true;
return bestMatch;
}
}
return false;
return null;
}

void addObjectEquals(MethodSymbol thisEquals)
{
var objEquals = new SynthesizedRecordObjEquals(this, thisEquals);
var objEquals = new SynthesizedRecordObjEquals(this, thisEquals, memberOffset: members.Count);
if (!memberSignatures.ContainsKey(objEquals))
{
// https://github.com/dotnet/roslyn/issues/44617: Don't add if the overridden method is sealed
Expand All @@ -3099,24 +3106,81 @@ void addObjectEquals(MethodSymbol thisEquals)

void addHashCode()
{
var hashCode = new SynthesizedRecordGetHashCode(this);
var hashCode = new SynthesizedRecordGetHashCode(this, memberOffset: members.Count);
if (!memberSignatures.ContainsKey(hashCode))
{
// https://github.com/dotnet/roslyn/issues/44617: Don't add if the overridden method is sealed
members.Add(hashCode);
}
}

MethodSymbol addThisEquals()
PropertySymbol addEqualityContract()
{
var property = new SynthesizedRecordEqualityContractProperty(this, isOverride: false);
// PROTOTYPE: Test with explicit EqualityContract property from source.
// PROTOTYPE: Handle inherited member of unexpected member kind or unexpected
// property signature (distinct type, not virtual, sealed, etc.)
if (getInheritedMember(property, this) is PropertySymbol inheritedProperty)
{
// PROTOTYPE: Should not have to re-create property!
property = new SynthesizedRecordEqualityContractProperty(this, isOverride: true);
}
members.Add(property);
members.Add(property.GetMethod);
return property;
}

MethodSymbol addThisEquals(PropertySymbol equalityContract, MethodSymbol? otherEqualsMethod)
{
var thisEquals = new SynthesizedRecordEquals(this);
var thisEquals = new SynthesizedRecordEquals(this, parameterType: this, isOverride: false, equalityContract, otherEqualsMethod, memberOffset: members.Count);
if (!memberSignatures.TryGetValue(thisEquals, out var existing))
{
members.Add(thisEquals);
return thisEquals;
}
return (MethodSymbol)existing;
}

static void getOtherEquals(ArrayBuilder<MethodSymbol> otherEqualsMethods, PropertySymbol equalityContract)
{
while ((equalityContract = equalityContract.OverriddenProperty) is object)
Copy link
Contributor

Choose a reason for hiding this comment

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

is there something specific we're trying to do by traversing up the equalityContract.OverriddenProperty chain instead of just up the BaseType chain?

{
var containingType = equalityContract.ContainingType;
var member = containingType.GetMembers("Equals").FirstOrDefault(m =>
{
if (m is MethodSymbol method)
{
var parameters = method.Parameters;
if (parameters.Length == 1 && parameters[0].Type.Equals(containingType, TypeCompareKind.AllIgnoreOptions))
{
return true;
}
}
return false;
});
// PROTOTYPE: Test with missing or unexpected Equals(Base) methods on base type.
if (member is MethodSymbol method)
{
otherEqualsMethods.Add(method);
}
}
}

void addOtherEquals(ArrayBuilder<MethodSymbol> otherEqualsMethods, PropertySymbol equalityContract, MethodSymbol thisEquals)
{
foreach (var otherEqualsMethod in otherEqualsMethods)
{
var method = new SynthesizedRecordEquals(
this,
parameterType: otherEqualsMethod.Parameters[0].Type,
Copy link
Member

Choose a reason for hiding this comment

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

Consider the following case

record A<T> { } 
record B : A<int> { }

Will otherEqualsMethod.Parameters[0].Type be the closed or open generic here?

Copy link
Member Author

@cston cston Jun 6, 2020

Choose a reason for hiding this comment

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

Great test case, thanks. It looks like it's handled correctly but I'll add a test.

Added in #44912.

isOverride: true,
equalityContract,
otherEqualsMethod: thisEquals,
memberOffset: members.Count);
// PROTOTYPE: Test with explicit strongly-typed Equals(Base) methods on derived record type.
members.Add(method);
}
}
}

private void AddSynthesizedConstructorsIfNecessary(ArrayBuilder<Symbol> members, ArrayBuilder<ArrayBuilder<FieldOrPropertyInitializer.Builder>> staticInitializers, DiagnosticBag diagnostics)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,23 +1,24 @@
// Licensed to the .NET Foundation under one or more agreements.
// 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 Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.PooledObjects;

namespace Microsoft.CodeAnalysis.CSharp.Symbols
{
internal sealed class SynthesizedRecordCopyCtor : SynthesizedInstanceConstructor
{
private readonly int _memberOffset;

public SynthesizedRecordCopyCtor(
SourceMemberContainerTypeSymbol containingType,
DiagnosticBag diagnostics)
int memberOffset)
: base(containingType)
{
_memberOffset = memberOffset;
Parameters = ImmutableArray.Create(SynthesizedParameterSymbol.Create(
this,
TypeWithAnnotations.Create(
Expand All @@ -29,12 +30,7 @@ public SynthesizedRecordCopyCtor(

public override ImmutableArray<ParameterSymbol> Parameters { get; }

internal override LexicalSortKey GetLexicalSortKey()
{
// We need a separate sort key because struct records will have two synthesized
// constructors: the record constructor, and the parameterless constructor
return LexicalSortKey.SynthesizedRecordCopyCtor;
}
internal override LexicalSortKey GetLexicalSortKey() => LexicalSortKey.GetSynthesizedMemberKey(_memberOffset);

internal override void GenerateMethodBodyStatements(SyntheticBoundNodeFactory F, ArrayBuilder<BoundStatement> statements, DiagnosticBag diagnostics)
{
Expand Down
Loading