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: allow positional members to be implemented with fields #52480

Merged
merged 6 commits into from
Apr 15, 2021
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
5 changes: 4 additions & 1 deletion src/Compilers/CSharp/Portable/CSharpResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -6419,7 +6419,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<value>Only records may inherit from records.</value>
</data>
<data name="ERR_BadRecordMemberForPositionalParameter" xml:space="preserve">
<value>Record member '{0}' must be a readable instance property of type '{1}' to match positional parameter '{2}'.</value>
<value>Record member '{0}' must be a readable instance property or field of type '{1}' to match positional parameter '{2}'.</value>
</data>
<data name="ERR_NoCopyConstructorInBaseType" xml:space="preserve">
<value>No accessible copy constructor found in base type '{0}'.</value>
Expand Down Expand Up @@ -6570,6 +6570,9 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="IDS_FeatureWithOnStructs" xml:space="preserve">
<value>with on structs</value>
</data>
<data name="IDS_FeaturePositionalFieldsInRecords" xml:space="preserve">
<value>positional fields in records</value>
</data>
<data name="IDS_FeatureVarianceSafetyForStaticInterfaceMembers" xml:space="preserve">
<value>variance safety for static interface members</value>
</data>
Expand Down
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/MessageID.cs
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ internal enum MessageID
IDS_FeatureMixedDeclarationsAndExpressionsInDeconstruction = MessageBase + 12793,
IDS_FeatureRecordStructs = MessageBase + 12794,
IDS_FeatureWithOnStructs = MessageBase + 12795,
IDS_FeaturePositionalFieldsInRecords = MessageBase + 12796,
}

// Message IDs may refer to strings that need to be localized.
Expand Down Expand Up @@ -328,6 +329,7 @@ internal static LanguageVersion RequiredVersion(this MessageID feature)
case MessageID.IDS_FeatureRecordStructs:
case MessageID.IDS_FeatureWithOnStructs: // semantic check
case MessageID.IDS_FeatureMixedDeclarationsAndExpressionsInDeconstruction: // semantic check
case MessageID.IDS_FeaturePositionalFieldsInRecords: // semantic check
return LanguageVersion.Preview;
// C# 9.0 features.
case MessageID.IDS_FeatureLambdaDiscardParameters: // semantic check
Expand Down
75 changes: 75 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/SignatureOnlyFieldSymbol.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
// 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.

using System.Collections.Immutable;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.CSharp.Symbols
{
/// <summary>
/// A representation of a field symbol that is intended only to be used for comparison purposes
/// (esp in MemberSignatureComparer).
/// </summary>
internal sealed class SignatureOnlyFieldSymbol : FieldSymbol
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 13, 2021

Choose a reason for hiding this comment

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

SignatureOnlyFieldSymbol [](start = 26, length = 24)

It feels like we can achieve the goal without this type. #Closed

{
private readonly string _name;
private readonly TypeSymbol _containingType;
private readonly TypeWithAnnotations _type;

public SignatureOnlyFieldSymbol(
string name,
TypeSymbol containingType,
TypeWithAnnotations type)
{
_type = type;
_containingType = containingType;
_name = name;
}

public override string Name => _name;

internal override TypeWithAnnotations GetFieldType(ConsList<FieldSymbol> fieldsBeingBound) => _type;

public override Symbol ContainingSymbol => _containingType;

#region Not used by MemberSignatureComparer
public override bool IsReadOnly => throw ExceptionUtilities.Unreachable;

public override bool IsStatic => throw ExceptionUtilities.Unreachable;

internal override bool HasSpecialName => throw ExceptionUtilities.Unreachable;

public override ImmutableArray<Location> Locations => throw ExceptionUtilities.Unreachable;

public override ImmutableArray<SyntaxReference> DeclaringSyntaxReferences => throw ExceptionUtilities.Unreachable;

public override Accessibility DeclaredAccessibility => throw ExceptionUtilities.Unreachable;

internal override ObsoleteAttributeData ObsoleteAttributeData => throw ExceptionUtilities.Unreachable;

public override AssemblySymbol ContainingAssembly => throw ExceptionUtilities.Unreachable;

internal override ModuleSymbol ContainingModule => throw ExceptionUtilities.Unreachable;

public override FlowAnalysisAnnotations FlowAnalysisAnnotations => throw ExceptionUtilities.Unreachable;

public override Symbol AssociatedSymbol => throw ExceptionUtilities.Unreachable;

public override bool IsVolatile => throw ExceptionUtilities.Unreachable;

public override bool IsConst => throw ExceptionUtilities.Unreachable;

internal override bool HasRuntimeSpecialName => throw ExceptionUtilities.Unreachable;

internal override bool IsNotSerialized => throw ExceptionUtilities.Unreachable;

internal override MarshalPseudoCustomAttributeData MarshallingInformation => throw ExceptionUtilities.Unreachable;

internal override int? TypeLayoutOffset => throw ExceptionUtilities.Unreachable;

internal override ConstantValue GetConstantValue(ConstantFieldsInProgress inProgress, bool earlyDecodingWellKnownAttributes) => throw new System.NotImplementedException();

#endregion Not used by MemberSignatureComparer
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3466,7 +3466,6 @@ private void AddSynthesizedRecordMembersIfNecessary(MembersAndInitializersBuilde
{
switch (member)
{
case FieldSymbol:
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 13, 2021

Choose a reason for hiding this comment

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

case FieldSymbol: [](start = 19, length = 18)

It looks like for fields the only thing we are going to compare is the Name. I think we should consider simply using a separate dictionary for that and avoiding an introduction of SignatureOnlyFieldSymbol with the only purpose to lookup a field by name. #Closed

case EventSymbol:
case MethodSymbol { MethodKind: not (MethodKind.Ordinary or MethodKind.Constructor) }:
continue;
Expand Down Expand Up @@ -3542,8 +3541,10 @@ private void AddSynthesizedRecordMembersIfNecessary(MembersAndInitializersBuilde

return;

void addDeconstruct(SynthesizedRecordConstructor ctor, ImmutableArray<PropertySymbol> properties)
void addDeconstruct(SynthesizedRecordConstructor ctor, ImmutableArray<Symbol> positionalMembers)
{
Debug.Assert(positionalMembers.All(p => p is PropertySymbol or FieldSymbol));

var targetMethod = new SignatureOnlyMethodSymbol(
WellKnownMemberNames.DeconstructMethodName,
this,
Expand All @@ -3563,7 +3564,7 @@ void addDeconstruct(SynthesizedRecordConstructor ctor, ImmutableArray<PropertySy

if (!memberSignatures.TryGetValue(targetMethod, out Symbol? existingDeconstructMethod))
{
members.Add(new SynthesizedRecordDeconstruct(this, ctor, properties, memberOffset: members.Count, diagnostics));
members.Add(new SynthesizedRecordDeconstruct(this, ctor, positionalMembers, memberOffset: members.Count, diagnostics));
}
else
{
Expand Down Expand Up @@ -3722,9 +3723,9 @@ void addToStringMethod(MethodSymbol printMethod)
}
}

ImmutableArray<PropertySymbol> addProperties(ImmutableArray<ParameterSymbol> recordParameters)
ImmutableArray<Symbol> addProperties(ImmutableArray<ParameterSymbol> recordParameters)
{
var existingOrAddedMembers = ArrayBuilder<PropertySymbol>.GetInstance(recordParameters.Length);
var existingOrAddedMembers = ArrayBuilder<Symbol>.GetInstance(recordParameters.Length);
int addedCount = 0;
foreach (ParameterSymbol param in recordParameters)
{
Expand All @@ -3743,12 +3744,42 @@ ImmutableArray<PropertySymbol> addProperties(ImmutableArray<ParameterSymbol> rec
if (!memberSignatures.TryGetValue(targetProperty, out var existingMember))
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
{
existingMember = OverriddenOrHiddenMembersHelpers.FindFirstHiddenMemberIfAny(targetProperty, memberIsFromSomeCompilation: true);
isInherited = true;
if (existingMember is FieldSymbol)
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 13, 2021

Choose a reason for hiding this comment

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

if (existingMember is FieldSymbol) [](start = 24, length = 34)

The logic feels too convoluted and I think it can be significantly simplified as follows:

  1. First, we look for a matching property in the current record;
  2. Then we look for a matching field in the current record;
  3. Then we look for a first hidden member in base #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.

We shouldn't look at the base last though. A property from the base should win over a field from the current type, to avoid breaking existing scenario (below).
That said, I think we can achieve that by calling FindFirstHiddenMemberIfAny only once and keeping the result.

public record Base 
{
    public int X { get; set; } 
}
public record C(int X) : Base
{
    public new int X;
}

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

Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't look at the base last though. A property from the base should win over a field from the current type, to avoid breaking existing scenario (below).

I don't think this behavior is sound, even though it avoids a breaking change.


In reply to: 612935164 [](ancestors = 612935164,612815137)

{
// field from base type should not be considered before we look at fields from current type
existingMember = null;
}
else
{
isInherited = true;
Copy link
Contributor

@RikkiGibson RikkiGibson Apr 13, 2021

Choose a reason for hiding this comment

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

I didn't understand why we avoid setting this for fields. In this case, the field is also inherited, right? We don't use it, but at the same time it seems simpler if we set it unconditionally. #Resolved

Copy link
Member Author

@jcouv jcouv Apr 13, 2021

Choose a reason for hiding this comment

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

We shouldn't set isInherited to true when a field was found from base types here, because we may find a field on the current type (which would be preferred and is not inherited). #Resolved

}
}

if (existingMember is null)
{
var targetField = new SignatureOnlyFieldSymbol(param.Name,
this,
param.TypeWithAnnotations);

if (!memberSignatures.TryGetValue(targetField, out existingMember))
{
existingMember = OverriddenOrHiddenMembersHelpers.FindFirstHiddenMemberIfAny(targetField, memberIsFromSomeCompilation: true);
Copy link
Contributor

@RikkiGibson RikkiGibson Apr 13, 2021

Choose a reason for hiding this comment

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

Does this method return a different value when called with this targetField than when it is called with the targetProperty above?
#Resolved

if (existingMember is not FieldSymbol)
{
existingMember = null;
}
}
}

if (existingMember is null)
{
addProperty(new SynthesizedRecordPropertySymbol(this, syntax, param, isOverride: false, diagnostics));
}
else if (existingMember is FieldSymbol { IsStatic: false } field)
Copy link
Contributor

@AlekseyTs AlekseyTs Apr 13, 2021

Choose a reason for hiding this comment

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

existingMember is FieldSymbol { IsStatic: false } field [](start = 29, length = 55)

Are we making sure the type matches? Where? #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.

That was missing. Thanks


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

{
Binder.CheckFeatureAvailability(syntax, MessageID.IDS_FeaturePositionalFieldsInRecords, diagnostics);
existingOrAddedMembers.Add(field);
}
else if (existingMember is PropertySymbol { IsStatic: false, GetMethod: { } } prop
&& prop.TypeWithAnnotations.Equals(param.TypeWithAnnotations, TypeCompareKind.AllIgnoreOptions))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,9 @@
// 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.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics;
using System.Linq;
using System.Reflection;
using Microsoft.Cci;
using Microsoft.CodeAnalysis.PooledObjects;
using Roslyn.Utilities;

Expand All @@ -16,19 +13,19 @@ namespace Microsoft.CodeAnalysis.CSharp.Symbols
internal sealed class SynthesizedRecordDeconstruct : SynthesizedRecordOrdinaryMethod
{
private readonly SynthesizedRecordConstructor _ctor;
private readonly ImmutableArray<PropertySymbol> _properties;
private readonly ImmutableArray<Symbol> _positionalMembers;

public SynthesizedRecordDeconstruct(
SourceMemberContainerTypeSymbol containingType,
SynthesizedRecordConstructor ctor,
ImmutableArray<PropertySymbol> properties,
ImmutableArray<Symbol> positionalMembers,
int memberOffset,
BindingDiagnosticBag diagnostics)
: base(containingType, WellKnownMemberNames.DeconstructMethodName, hasBody: true, memberOffset, diagnostics)
{
Debug.Assert(properties.All(prop => prop.GetMethod is object));
Debug.Assert(positionalMembers.All(p => p is PropertySymbol { GetMethod: not null } or FieldSymbol));
_ctor = ctor;
_properties = properties;
_positionalMembers = positionalMembers;
}

protected override DeclarationModifiers MakeDeclarationModifiers(DeclarationModifiers allowedModifiers, BindingDiagnosticBag diagnostics)
Expand Down Expand Up @@ -63,29 +60,45 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState,
{
var F = new SyntheticBoundNodeFactory(this, ContainingType.GetNonNullSyntaxNode(), compilationState, diagnostics);

if (ParameterCount != _properties.Length)
if (ParameterCount != _positionalMembers.Length)
{
// There is a mismatch, an error was reported elsewhere
F.CloseMethod(F.ThrowNull());
return;
}

var statementsBuilder = ArrayBuilder<BoundStatement>.GetInstance(_properties.Length + 1);
for (int i = 0; i < _properties.Length; i++)
var statementsBuilder = ArrayBuilder<BoundStatement>.GetInstance(_positionalMembers.Length + 1);
for (int i = 0; i < _positionalMembers.Length; i++)
{
var parameter = Parameters[i];
var property = _properties[i];
var positionalMember = _positionalMembers[i];

if (!parameter.Type.Equals(property.Type, TypeCompareKind.AllIgnoreOptions))
var type = positionalMember switch
{
PropertySymbol property => property.Type,
FieldSymbol field => field.Type,
_ => throw ExceptionUtilities.Unreachable
};

if (!parameter.Type.Equals(type, TypeCompareKind.AllIgnoreOptions))
{
// There is a mismatch, an error was reported elsewhere
statementsBuilder.Free();
F.CloseMethod(F.ThrowNull());
return;
}

// parameter_i = property_i;
statementsBuilder.Add(F.Assignment(F.Parameter(parameter), F.Property(F.This(), property)));
switch (positionalMember)
{
case PropertySymbol property:
// parameter_i = property_i;
statementsBuilder.Add(F.Assignment(F.Parameter(parameter), F.Property(F.This(), property)));
break;
case FieldSymbol field:
// parameter_i = field_i;
statementsBuilder.Add(F.Assignment(F.Parameter(parameter), F.Field(F.This(), field)));
break;
}
}

statementsBuilder.Add(F.Return());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ private static SyntaxKind GetTypeDeclarationKeywordKind(SyntaxKind kind)
case SyntaxKind.InterfaceDeclaration:
return SyntaxKind.InterfaceKeyword;
case SyntaxKind.RecordDeclaration:
case SyntaxKind.RecordStructDeclaration:
return SyntaxKind.RecordKeyword;
default:
throw ExceptionUtilities.UnexpectedValue(kind);
Expand Down Expand Up @@ -114,7 +115,8 @@ public static TypeDeclarationSyntax TypeDeclaration(
return SyntaxFactory.InterfaceDeclaration(attributes, modifiers, keyword, identifier, typeParameterList, baseList, constraintClauses, openBraceToken, members, closeBraceToken, semicolonToken);
case SyntaxKind.RecordDeclaration:
return SyntaxFactory.RecordDeclaration(attributes, modifiers, keyword, classKeyword: default, identifier, typeParameterList, parameterList: null, baseList, constraintClauses, openBraceToken, members, closeBraceToken, semicolonToken);
// PROTOTYPE(record-structs): we likely need to handle RecordStructDeclaration
case SyntaxKind.RecordStructDeclaration:
return SyntaxFactory.RecordStructDeclaration(attributes, modifiers, keyword, structKeyword: SyntaxFactory.Token(SyntaxKind.StructKeyword), identifier, typeParameterList, parameterList: null, baseList, constraintClauses, openBraceToken, members, closeBraceToken, semicolonToken);
default:
throw new ArgumentException("kind");
RikkiGibson marked this conversation as resolved.
Show resolved Hide resolved
}
Expand Down
9 changes: 7 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 7 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 7 additions & 2 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading