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 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 @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -3460,16 +3460,22 @@ private void AddSynthesizedRecordMembersIfNecessary(MembersAndInitializersBuilde
};

var memberSignatures = s_duplicateRecordMemberSignatureDictionary.Allocate();
var fieldsByName = PooledDictionary<string, Symbol>.GetInstance();
var membersSoFar = builder.GetNonTypeMembers(declaredMembersAndInitializers);
var members = ArrayBuilder<Symbol>.GetInstance(membersSoFar.Count + 1);
foreach (var member in membersSoFar)
{
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;
case FieldSymbol { Name: var fieldName }:
if (!fieldsByName.ContainsKey(fieldName))
{
fieldsByName.Add(fieldName, member);
}
Comment on lines +3474 to +3477
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it feels like this could be replaced with fieldsByName[fieldName] = member; (in error scenarios with multiple identically named fields, which one we pick feels unimportant).

Copy link
Member Author

Choose a reason for hiding this comment

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

If it's okay, I'll leave as-is. Thanks

continue;
}

if (!memberSignatures.ContainsKey(member))
Expand Down Expand Up @@ -3533,6 +3539,7 @@ private void AddSynthesizedRecordMembersIfNecessary(MembersAndInitializersBuilde
addToStringMethod(printMembers);

memberSignatures.Free();
fieldsByName.Free();

// We put synthesized record members first so that errors about conflicts show up on user-defined members rather than all
// going to the record declaration
Expand All @@ -3542,8 +3549,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 +3572,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 +3731,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 @@ -3739,16 +3748,26 @@ ImmutableArray<PropertySymbol> addProperties(ImmutableArray<ParameterSymbol> rec
ImmutableArray<CustomModifier>.Empty,
isStatic: false,
ImmutableArray<PropertySymbol>.Empty);

if (!memberSignatures.TryGetValue(targetProperty, out var existingMember))
if (!memberSignatures.TryGetValue(targetProperty, out var existingMember)
&& !fieldsByName.TryGetValue(param.Name, out existingMember))
{
existingMember = OverriddenOrHiddenMembersHelpers.FindFirstHiddenMemberIfAny(targetProperty, memberIsFromSomeCompilation: true);
isInherited = true;
}

// There should be an error if we picked a member that is hidden
// This will be fixed in C# 9 as part of 16.10. Tracked by https://github.com/dotnet/roslyn/issues/52630
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we plan to do this after feature merge?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I'll make the breaking change in 16.10 and both changes will meet in features/compiler.


if (existingMember is null)
{
addProperty(new SynthesizedRecordPropertySymbol(this, syntax, param, isOverride: false, diagnostics));
}
else if (existingMember is FieldSymbol { IsStatic: false } field
&& field.TypeWithAnnotations.Equals(param.TypeWithAnnotations, TypeCompareKind.AllIgnoreOptions))
{
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
6 changes: 4 additions & 2 deletions src/Compilers/CSharp/Portable/Syntax/TypeDeclarationSyntax.cs
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,9 +115,10 @@ 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");
throw ExceptionUtilities.UnexpectedValue(kind);
}
}
}
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.

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

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

Loading