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

Support struct records #43133

Merged
merged 5 commits into from
Apr 23, 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
3 changes: 2 additions & 1 deletion src/Compilers/CSharp/Portable/Compiler/MethodCompiler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -1153,7 +1153,8 @@ private void CompileMethod(
// don't emit if the resulting method would contain initializers with errors
if (!hasErrors && (hasBody || includeInitializersInBody))
{
Debug.Assert(!methodSymbol.IsImplicitInstanceConstructor || !methodSymbol.ContainingType.IsStructType());
Debug.Assert(!(methodSymbol.IsImplicitInstanceConstructor && methodSymbol.ParameterCount == 0) ||
Copy link
Member

@gafter gafter Apr 10, 2020

Choose a reason for hiding this comment

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

IsImplicitInstanceConstructor [](start = 52, length = 29)

I think the condition IsImplicitInstanceConstructor generally implies methodSymbol.ParameterCount == 0, so you've weakened the assertion in case there is a mismatch there. #Resolved

Copy link
Member Author

@agocke agocke Apr 10, 2020

Choose a reason for hiding this comment

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

Not as far as I can tell: http://sourceroslyn.io/#Microsoft.CodeAnalysis.CSharp/Symbols/MethodSymbol.cs,fb6aebe573e152e2,references

That is satisfied for every record constructor as well, so all record constructors are now implicit instance constructors #Resolved

Copy link
Member

Choose a reason for hiding this comment

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

Are there any other implicit instance constructors though? AFAIK, there's only the default parameterless one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Record constructors!

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough.

Copy link
Member

Choose a reason for hiding this comment

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

Thankfully there's only one use of this property (in this assert), so we don't have to worry about that contract being violated elsewhere.

!methodSymbol.ContainingType.IsStructType());

// Fields must be initialized before constructor initializer (which is the first statement of the analyzed body, if specified),
// so that the initialization occurs before any method overridden by the declaring class can be invoked from the base constructor
Expand Down
6 changes: 4 additions & 2 deletions src/Compilers/CSharp/Portable/Symbols/LexicalSortKey.cs
Original file line number Diff line number Diff line change
Expand Up @@ -44,14 +44,16 @@ 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 - 3 };
public static LexicalSortKey SynthesizedRecordObjEquals => new LexicalSortKey() { _treeOrdinal = int.MaxValue, _position = int.MaxValue - 2 };
public static LexicalSortKey SynthesizedRecordEquals => new LexicalSortKey() { _treeOrdinal = int.MaxValue, _position = int.MaxValue - 4 };
public static LexicalSortKey SynthesizedRecordObjEquals => new LexicalSortKey() { _treeOrdinal = int.MaxValue, _position = int.MaxValue - 3 };


// 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 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 @@ -143,6 +143,9 @@ public bool SetNullableContext(byte? value)
}
}

private static readonly ObjectPool<PooledDictionary<Symbol, Symbol>> s_duplicateMemberSignatureDictionary =
Copy link
Member

@gafter gafter Apr 10, 2020

Choose a reason for hiding this comment

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

s_duplicateMemberSignatureDictionary [](start = 77, length = 36)

Why do you need a custom pool instead of using PooledDictionary<Symbol, Symbol>.GetInstance()? #Resolved

Copy link
Member Author

@agocke agocke Apr 10, 2020

Choose a reason for hiding this comment

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

Need a custom comparer #Resolved

PooledDictionary<Symbol, Symbol>.CreatePool(MemberSignatureComparer.DuplicateSourceComparer);

protected SymbolCompletionState state;

private Flags _flags;
Expand Down Expand Up @@ -2922,20 +2925,26 @@ private void AddSynthesizedRecordMembersIfNecessary(MembersAndInitializersBuilde
BinderFactory binderFactory = this.DeclaringCompilation.GetBinderFactory(paramList.SyntaxTree);
var binder = binderFactory.GetBinder(paramList);

var memberSignatures = new HashSet<Symbol>(members, MemberSignatureComparer.DuplicateSourceComparer);
var memberSignatures = s_duplicateMemberSignatureDictionary.Allocate();
foreach (var member in members)
{
memberSignatures.Add(member, member);
}

var ctor = addCtor(paramList);
addProperties(ctor.Parameters);
addObjectEquals();
var thisEquals = addThisEquals();
addObjectEquals(thisEquals);
addHashCode();
addThisEquals();

memberSignatures.Free();

return;

SynthesizedRecordConstructor addCtor(ParameterListSyntax paramList)
{
var ctor = new SynthesizedRecordConstructor(this, binder, paramList, diagnostics);
if (!memberSignatures.Contains(ctor))
if (!memberSignatures.ContainsKey(ctor))
{
members.Add(ctor);
}
Expand All @@ -2952,7 +2961,7 @@ void addProperties(ImmutableArray<ParameterSymbol> recordParameters)
foreach (ParameterSymbol param in ctor.Parameters)
{
var property = new SynthesizedRecordPropertySymbol(this, param);
if (!memberSignatures.Contains(property))
if (!memberSignatures.ContainsKey(property))
{
members.Add(property);
members.Add(property.GetMethod);
Expand All @@ -2961,10 +2970,10 @@ void addProperties(ImmutableArray<ParameterSymbol> recordParameters)
}
}

void addObjectEquals()
void addObjectEquals(MethodSymbol thisEquals)
{
var objEquals = new SynthesizedRecordObjEquals(this);
if (!memberSignatures.Contains(objEquals))
var objEquals = new SynthesizedRecordObjEquals(this, thisEquals);
if (!memberSignatures.ContainsKey(objEquals))
333fred marked this conversation as resolved.
Show resolved Hide resolved
{
// PROTOTYPE: Don't add if the overridden method is sealed
members.Add(objEquals);
Expand All @@ -2974,32 +2983,27 @@ void addObjectEquals()
void addHashCode()
{
var hashCode = new SynthesizedRecordGetHashCode(this);
if (!memberSignatures.Contains(hashCode))
if (!memberSignatures.ContainsKey(hashCode))
{
// PROTOTYPE: Don't add if the overridden method is sealed
members.Add(hashCode);
}
}

void addThisEquals()
MethodSymbol addThisEquals()
{
var thisEquals = new SynthesizedRecordEquals(this);
if (!memberSignatures.Contains(thisEquals))
if (!memberSignatures.TryGetValue(thisEquals, out var existing))
{
members.Add(thisEquals);
return thisEquals;
}
return (MethodSymbol)existing;
}
}

private void AddSynthesizedConstructorsIfNecessary(ArrayBuilder<Symbol> members, ArrayBuilder<ImmutableArray<FieldOrPropertyInitializer>> staticInitializers, DiagnosticBag diagnostics)
{
switch (declaration.Kind)
{
case DeclarationKind.Class:
case DeclarationKind.Struct:
break;
}

//we're not calling the helpers on NamedTypeSymbol base, because those call
//GetMembers and we're inside a GetMembers call ourselves (i.e. stack overflow)
var hasInstanceConstructor = false;
Expand Down Expand Up @@ -3036,7 +3040,8 @@ private void AddSynthesizedConstructorsIfNecessary(ArrayBuilder<Symbol> members,
// NOTE: Per section 11.3.8 of the spec, "every struct implicitly has a parameterless instance constructor".
// We won't insert a parameterless constructor for a struct if there already is one.
// We don't expect anything to be emitted, but it should be in the symbol table.
if ((!hasParameterlessInstanceConstructor && this.IsStructType()) || (!hasInstanceConstructor && !this.IsStatic && !this.IsInterface))
if ((!hasParameterlessInstanceConstructor && this.IsStructType()) ||
(!hasInstanceConstructor && !this.IsStatic && !this.IsInterface))
{
members.Add((this.TypeKind == TypeKind.Submission) ?
new SynthesizedSubmissionConstructor(this, diagnostics) :
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,13 @@ public SynthesizedRecordConstructor(
addRefReadOnlyModifier: false);
}

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.SynthesizedRecordCtor;
}
333fred marked this conversation as resolved.
Show resolved Hide resolved

internal override void GenerateMethodBodyStatements(SyntheticBoundNodeFactory F, ArrayBuilder<BoundStatement> statements, DiagnosticBag diagnostics)
{
// Write assignments to backing fields
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,14 @@ internal sealed class SynthesizedRecordEquals : SynthesizedInstanceMethodSymbol
public SynthesizedRecordEquals(NamedTypeSymbol containingType)
{
ContainingType = containingType;
if (containingType.IsStructType())
{
// If the record type is a struct, the parameter is marked 'in'
containingType.DeclaringCompilation.EnsureIsReadOnlyAttributeExists(
diagnostics: null,
location: Location.None,
modifyCompilation: true);
}
}

public override string Name => "Equals";
Expand Down Expand Up @@ -152,7 +160,7 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState,
other,
recordProperties,
F);
retExpr = F.LogicalAnd(retExpr, comparisons);
retExpr = retExpr is null ? comparisons : F.LogicalAnd(retExpr, comparisons);
}
recordProperties.Free();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,16 @@ namespace Microsoft.CodeAnalysis.CSharp.Symbols
{
internal sealed class SynthesizedRecordObjEquals : SynthesizedInstanceMethodSymbol
{
private readonly MethodSymbol _typedRecordEquals;

public override NamedTypeSymbol ContainingType { get; }

public override ImmutableArray<ParameterSymbol> Parameters { get; }

public SynthesizedRecordObjEquals(NamedTypeSymbol containingType)

public SynthesizedRecordObjEquals(NamedTypeSymbol containingType, MethodSymbol typedRecordEquals)
{
_typedRecordEquals = typedRecordEquals;
ContainingType = containingType;
Parameters = ImmutableArray.Create<ParameterSymbol>(SynthesizedParameterSymbol.Create(
this,
Expand Down Expand Up @@ -120,13 +124,32 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState,
{
var F = new SyntheticBoundNodeFactory(this, ContainingType.GetNonNullSyntaxNode(), compilationState, diagnostics);

// return this.Equals(param as ContainingType);
var block = F.Block(ImmutableArray.Create<BoundStatement>(
F.Return(
F.InstanceCall(F.This(), "Equals",
F.As(F.Parameter(Parameters[0]), ContainingType)))));

F.CloseMethod(block);
var paramAccess = F.Parameter(Parameters[0]);

BoundExpression expression;
if (ContainingType.IsStructType())
{
// For structs:
//
// return param is ContainingType i ? this.Equals(in i) : false;
Copy link
Member

@alrz alrz Apr 11, 2020

Choose a reason for hiding this comment

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

param is ContainingType i && this.Equals(in i)?

expression = F.Conditional(
F.Is(paramAccess, ContainingType),
F.Call(
F.This(),
_typedRecordEquals,
ImmutableArray.Create<RefKind>(RefKind.In),
333fred marked this conversation as resolved.
Show resolved Hide resolved
ImmutableArray.Create<BoundExpression>(F.Convert(ContainingType, paramAccess))),
F.Literal(false),
F.SpecialType(SpecialType.System_Boolean));
}
else
{
// For classes:
// return this.Equals(param as ContainingType);
expression = F.InstanceCall(F.This(), "Equals", F.As(paramAccess, ContainingType));
}

F.CloseMethod(F.Block(ImmutableArray.Create<BoundStatement>(F.Return(expression))));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ public sealed override ImmutableArray<TypeParameterSymbol> TypeParameters
get { return ImmutableArray<TypeParameterSymbol>.Empty; }
}

internal sealed override LexicalSortKey GetLexicalSortKey()
internal override LexicalSortKey GetLexicalSortKey()
{
//For the sake of matching the metadata output of the native compiler, make synthesized constructors appear last in the metadata.
//This is not critical, but it makes it easier on tools that are comparing metadata.
Expand Down
Loading