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

Use new record equality specification #43626

Merged
merged 5 commits into from
May 1, 2020
Merged
Show file tree
Hide file tree
Changes from 2 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 @@ -123,27 +123,16 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState,
F.Convert(manager.System_Object, boundLocal),
F.Null(manager.System_Object));

// prepare symbols
MethodSymbol equalityComparer_Equals = manager.System_Collections_Generic_EqualityComparer_T__Equals;
MethodSymbol equalityComparer_get_Default = manager.System_Collections_Generic_EqualityComparer_T__get_Default;
NamedTypeSymbol equalityComparerType = equalityComparer_Equals.ContainingType;

// Compare fields
for (int index = 0; index < anonymousType.Properties.Length; index++)
if (anonymousType.Properties.Length > 0)
{
// Prepare constructed symbols
TypeParameterSymbol typeParameter = anonymousType.TypeParameters[index];
FieldSymbol fieldSymbol = anonymousType.Properties[index].BackingField;
NamedTypeSymbol constructedEqualityComparer = equalityComparerType.Construct(typeParameter);

// Generate 'retExpression' = 'retExpression && System.Collections.Generic.EqualityComparer<T_index>.
// Default.Equals(this.backingFld_index, local.backingFld_index)'
retExpression = F.LogicalAnd(retExpression,
F.Call(F.StaticCall(constructedEqualityComparer,
equalityComparer_get_Default.AsMember(constructedEqualityComparer)),
equalityComparer_Equals.AsMember(constructedEqualityComparer),
F.Field(F.This(), fieldSymbol),
F.Field(boundLocal, fieldSymbol)));
var fields = ArrayBuilder<FieldSymbol>.GetInstance(anonymousType.Properties.Length);
foreach (var prop in anonymousType.Properties)
{
fields.Add(prop.BackingField);
}
retExpression = MethodBodySynthesizer.GenerateFieldEquals(retExpression, boundLocal, fields, F);
fields.Free();
}

// Final return statement
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,13 @@
// 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.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Linq;
using Microsoft.CodeAnalysis.CSharp.Emit;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.RuntimeMembers;
using Microsoft.CodeAnalysis.Text;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp.Symbols
{
Expand Down Expand Up @@ -253,6 +251,61 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState,
/// </summary>
internal static partial 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>(
BoundExpression? initialExpression,
BoundExpression otherReceiver,
TList fields,
SyntheticBoundNodeFactory F) where TList : IReadOnlyList<FieldSymbol>
{
Debug.Assert(fields.Count > 0);
Copy link
Member

@gafter gafter May 1, 2020

Choose a reason for hiding this comment

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

fields.Count > 0 [](start = 25, length = 16)

How is this justified? Please add a PROTOYPE, issue, or bullet in the test plan.


// Expression:
//
// System.Collections.Generic.EqualityComparer<T_1>.Default.Equals(this.backingFld_1, value.backingFld_1)
// ...
// && System.Collections.Generic.EqualityComparer<T_N>.Default.Equals(this.backingFld_N, value.backingFld_N)

// prepare symbols
var equalityComparer_get_Default = F.WellKnownMethod(
WellKnownMember.System_Collections_Generic_EqualityComparer_T__get_Default);
var equalityComparer_Equals = F.WellKnownMethod(
WellKnownMember.System_Collections_Generic_EqualityComparer_T__Equals);

NamedTypeSymbol equalityComparerType = equalityComparer_Equals.ContainingType;

BoundExpression? retExpression = initialExpression;

// Compare fields
foreach (var field in fields)
{
// Prepare constructed comparer
var constructedEqualityComparer = equalityComparerType.Construct(field.Type);

// System.Collections.Generic.EqualityComparer<T_index>.
// Default.Equals(this.backingFld_index, local.backingFld_index)'
BoundExpression nextEquals = F.Call(
F.StaticCall(constructedEqualityComparer,
equalityComparer_get_Default.AsMember(constructedEqualityComparer)),
equalityComparer_Equals.AsMember(constructedEqualityComparer),
F.Field(F.This(), field),
F.Field(otherReceiver, field));

// Generate 'retExpression' = 'retExpression && nextEquals'
retExpression = retExpression is null
? nextEquals
: F.LogicalAnd(retExpression, nextEquals);
}

RoslynDebug.AssertNotNull(retExpression);
Copy link
Member

Choose a reason for hiding this comment

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

AssertNotNull [](start = 24, length = 13)

This assertion does not appear justified.

Copy link
Member

Choose a reason for hiding this comment

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

Earlier fields is asserted to have more than 1 element, there will always be at least one iteration of the foreach.

Copy link
Member

Choose a reason for hiding this comment

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

How is there always at least one iteration of the foreach? According to the spec a record can have an empty parameter list.


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


return retExpression;
}

/// <summary>
/// Construct a body for a method containing a call to a single other method with the same signature (modulo name).
/// </summary>
Expand All @@ -263,12 +316,11 @@ internal static partial class MethodBodySynthesizer
internal static BoundBlock ConstructSingleInvocationMethodBody(SyntheticBoundNodeFactory F, MethodSymbol methodToInvoke, bool useBaseReference)
{
var argBuilder = ArrayBuilder<BoundExpression>.GetInstance();
//var refKindBuilder = ArrayBuilder<RefKind>.GetInstance();

RoslynDebug.AssertNotNull(F.CurrentFunction);
foreach (var param in F.CurrentFunction.Parameters)
{
argBuilder.Add(F.Parameter(param));
//refKindBuilder.Add(param.RefKind);
}

BoundExpression invocation = F.Call(useBaseReference ? (BoundExpression)F.Base(baseType: methodToInvoke.ContainingType) : F.This(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@
using System.Collections.Immutable;
using System.Reflection;
using Microsoft.Cci;
using Microsoft.CodeAnalysis.CSharp.Emit;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.PooledObjects;

namespace Microsoft.CodeAnalysis.CSharp.Symbols
Expand Down Expand Up @@ -144,25 +142,25 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState,
retExpr = F.ObjectNotEqual(other, F.Null(F.SpecialType(SpecialType.System_Object)));
}

var recordProperties = ArrayBuilder<FieldSymbol>.GetInstance();
var fields = ArrayBuilder<FieldSymbol>.GetInstance();
foreach (var member in ContainingType.GetMembers())
agocke marked this conversation as resolved.
Show resolved Hide resolved
{
// PROTOTYPE: Should generate equality on user-written members as well
agocke marked this conversation as resolved.
Show resolved Hide resolved
if (member is SynthesizedRecordPropertySymbol p)
if (member is FieldSymbol { IsStatic: false } f)
{
recordProperties.Add(p.BackingField);
fields.Add(f);
}
}
if (recordProperties.Count > 0)

if (fields.Count > 0)
{
var comparisons = EqualityMethodBodySynthesizer.GenerateEqualsComparisons(
ContainingType,
retExpr = MethodBodySynthesizer.GenerateFieldEquals(
retExpr,
other,
recordProperties,
fields,
F);
retExpr = retExpr is null ? comparisons : F.LogicalAnd(retExpr, comparisons);
}
recordProperties.Free();
fields.Free();

F.CloseMethod(F.Block(F.Return(retExpr)));
}
Expand Down
Loading