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.

Sign up for GitHub

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

Jump to bottom

Records: override abstract inherited properties #45336

Merged
merged 20 commits into from
Jun 30, 2020
Merged

Records: override abstract inherited properties #45336

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
e22ea5e
Records: override abstract inherited properties
cston Jun 16, 2020
e065ff0
PR feedback
cston Jun 22, 2020
d1f9199
Add test
cston Jun 22, 2020
942fae4
PR feedback
cston Jun 23, 2020
3af055e
Execute test code
cston Jun 23, 2020
8f43626
Add test
cston Jun 23, 2020
40c5f83
PR feedback
cston Jun 23, 2020
b9b5038
Merge remote-tracking branch 'upstream/master' into 44618
cston Jun 26, 2020
3028c82
Misc.
cston Jun 26, 2020
d94b13e
Use existing SourcePropertySymbolBase .ctor
cston Jun 26, 2020
03be993
Revert changes
cston Jun 26, 2020
c9af0db
Remove search for inherited EqualityContract property
cston Jun 27, 2020
7971a3b
Remove search for inherited properties
cston Jun 27, 2020
63b0dae
Misc.
cston Jun 27, 2020
7a0481b
Replace delegate parameters
cston Jun 27, 2020
697f85f
PR feedback
cston Jun 27, 2020
5d5316b
HasSpecialName
cston Jun 27, 2020
7f6fd4b
PR feedback
cston Jun 28, 2020
f852cf0
Merge remote-tracking branch 'upstream/master' into 44618
cston Jun 29, 2020
f29f2f8
Misc.
cston Jun 29, 2020
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
42 changes: 29 additions & 13 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3061,26 +3061,30 @@ ImmutableArray<PropertySymbol> addProperties(ImmutableArray<ParameterSymbol> rec
int addedCount = 0;
foreach (ParameterSymbol param in recordParameters)
{
var property = new SynthesizedRecordPropertySymbol(this, param, diagnostics);
_ = memberSignatures.TryGetValue(property, out var existingMember);
existingMember ??= getInheritedMember(property, this);
bool isInherited = false;
var property = new SynthesizedRecordPropertySymbol(this, param, isOverride: false, diagnostics);
if (!memberSignatures.TryGetValue(property, out var existingMember))
{
existingMember = getInheritedMember(property, this);
isInherited = true;
}
if (existingMember is null)
{
existingOrAddedMembers.Add(property);
members.Add(property);
members.Add(property.GetMethod);
members.Add(property.SetMethod);
members.Add(property.BackingField);

builder.InstanceInitializersForRecordDeclarationWithParameters.Insert(addedCount, new FieldOrPropertyInitializer.Builder(property.BackingField, paramList.Parameters[param.Ordinal]));
addedCount++;
addProperty(property);
}
else if (existingMember is PropertySymbol { IsStatic: false, GetMethod: { } } prop
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 28, 2020 •
edited by cston
Loading

Choose a reason for hiding this comment

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

GetMethod: { } [](start = 81, length = 14)

The current specification, that I think should be up to date given the time when the #44618 was created, and the rules stated in the issue disagree.

The issue:

For each record parameter of a record type declaration there is a corresponding public property member whose name and type are taken from the value parameter declaration. If no concrete (i.e. non-abstract) property with a get accessor and with this name and type is explicitly declared or inherited, it is produced by the compiler as follows:

For a record struct or a record class:

  • A public get and init auto-property is created (see separate init accessor specification). Its value is initialized during construction with the value of the corresponding primary constructor parameter. Each "matching" inherited abstract property's get accessor is overridden.

The specification:

A public get and init auto-property is created (see separate init accessor specification). Each "matching" inherited abstract accessor is overridden. The auto-property is initialized to the value of the corresponding primary constructor parameter.
For each record parameter of a record type declaration there is a corresponding public property member whose name and type are taken from the value parameter declaration.

For a record:

  • A public get and init auto-property is created (see separate init accessor specification). Each "matching" inherited abstract accessor is overridden. The auto-property is initialized to the value of the corresponding primary constructor parameter.

Could you please reconcile the differences? This is not blocking

#Resolved

&& prop.TypeWithAnnotations.Equals(param.TypeWithAnnotations, TypeCompareKind.AllIgnoreOptions))
{
// There already exists a member corresponding to the candidate synthesized property.
// The deconstructor is specified to simply assign from this property to the corresponding out parameter.
existingOrAddedMembers.Add(prop);
if (isInherited && existingMember.IsAbstract)
{
addProperty(new SynthesizedRecordPropertySymbol(this, param, isOverride: true, diagnostics));
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

addProperty(new SynthesizedRecordPropertySymbol(this, param, isOverride: true, diagnostics)); [](start = 28, length = 93)

It doesn't look like we are properly inheriting custom modifiers for the property and the accessors. We should do what is done for regular properties and their accessors. #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.

Added a test for now, and will wait for #44883.

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

}
else
{
// Deconstruct() is specified to simply assign from this property to the corresponding out parameter.
existingOrAddedMembers.Add(prop);
}
}
else
{
Expand All @@ -3090,6 +3094,18 @@ ImmutableArray<PropertySymbol> addProperties(ImmutableArray<ParameterSymbol> rec
param.TypeWithAnnotations,
param.Name);
}

void addProperty(SynthesizedRecordPropertySymbol property)
{
existingOrAddedMembers.Add(property);
members.Add(property);
members.Add(property.GetMethod);
members.Add(property.SetMethod);
members.Add(property.BackingField);

builder.InstanceInitializersForRecordDeclarationWithParameters.Insert(addedCount, new FieldOrPropertyInitializer.Builder(property.BackingField, paramList.Parameters[param.Ordinal]));
addedCount++;
}
}

#if DEBUG
Expand Down
138 changes: 39 additions & 99 deletions src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordPropertySymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,13 @@ namespace Microsoft.CodeAnalysis.CSharp.Symbols
internal sealed class SynthesizedRecordPropertySymbol : SourceOrRecordPropertySymbol
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

SynthesizedRecordPropertySymbol [](start = 26, length = 31)

It looks like changes made in this file are going to conflict with #44883 #Closed

{
private readonly ParameterSymbol _backingParameter;

internal override SynthesizedBackingFieldSymbol BackingField { get; }
public override MethodSymbol GetMethod { get; }
public override MethodSymbol SetMethod { get; }
public override NamedTypeSymbol ContainingType { get; }

public SynthesizedRecordPropertySymbol(
NamedTypeSymbol containingType,
ParameterSymbol backingParameter,
DiagnosticBag diagnostics)
public SynthesizedRecordPropertySymbol(NamedTypeSymbol containingType, ParameterSymbol backingParameter, bool isOverride, DiagnosticBag diagnostics)
: base(backingParameter.Locations[0])
{
ContainingType = containingType;
Expand All @@ -38,6 +36,7 @@ public SynthesizedRecordPropertySymbol(
hasInitializer: true);
GetMethod = new GetAccessorSymbol(this, name);
SetMethod = new InitAccessorSymbol(this, name, diagnostics);
IsOverride = isOverride;
}

public ParameterSymbol BackingParameter => _backingParameter;
Expand Down Expand Up @@ -68,7 +67,7 @@ public SynthesizedRecordPropertySymbol(

public override bool IsVirtual => false;

public override bool IsOverride => false;
public override bool IsOverride { get; }

public override bool IsAbstract => false;

Expand Down Expand Up @@ -98,46 +97,42 @@ public SynthesizedRecordPropertySymbol(

public override SyntaxList<AttributeListSyntax> AttributeDeclarationSyntaxList => new SyntaxList<AttributeListSyntax>();

private sealed class GetAccessorSymbol : SynthesizedInstanceMethodSymbol
private abstract class AccessorSymbol : SynthesizedInstanceMethodSymbol
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

AccessorSymbol [](start = 31, length = 14)

See also #44883 (comment), this might make it easier to deal with overriding. #Closed

{
private readonly SynthesizedRecordPropertySymbol _property;
protected readonly SynthesizedRecordPropertySymbol _property;

public override string Name { get; }
public abstract override string Name { get; }

public GetAccessorSymbol(SynthesizedRecordPropertySymbol property, string paramName)
protected AccessorSymbol(SynthesizedRecordPropertySymbol property)
{
_property = property;
Name = SourcePropertyAccessorSymbol.GetAccessorName(
paramName,
getNotSet: true,
isWinMdOutput: false /* unused for getters */);
}

public override MethodKind MethodKind => MethodKind.PropertyGet;
public abstract override MethodKind MethodKind { get; }

public override int Arity => 0;

public override bool IsExtensionMethod => false;

public override bool HidesBaseMethodsByName => false;
public override bool HidesBaseMethodsByName => false; // PROTOTYPE: Should this be true if it hides?
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

// PROTOTYPE: Should this be true if it hides? [](start = 66, length = 46)

This PR is targeting master, no PROTOTYPE comments allowed. Also, I believe C# never hides by name. #Closed


public override bool IsVararg => false;

public override bool ReturnsVoid => false;
public override bool ReturnsVoid => ReturnType.SpecialType == SpecialType.System_Void;

public override bool IsAsync => false;

public override RefKind RefKind => RefKind.None;

public override TypeWithAnnotations ReturnTypeWithAnnotations => _property.TypeWithAnnotations;
public abstract override TypeWithAnnotations ReturnTypeWithAnnotations { get; }

public override FlowAnalysisAnnotations ReturnTypeFlowAnalysisAnnotations => FlowAnalysisAnnotations.None;

public override ImmutableArray<TypeWithAnnotations> TypeArgumentsWithAnnotations => ImmutableArray<TypeWithAnnotations>.Empty;

public override ImmutableArray<TypeParameterSymbol> TypeParameters => ImmutableArray<TypeParameterSymbol>.Empty;

public override ImmutableArray<ParameterSymbol> Parameters => _property.Parameters;
public abstract override ImmutableArray<ParameterSymbol> Parameters { get; }

public override ImmutableArray<MethodSymbol> ExplicitInterfaceImplementations => ImmutableArray<MethodSymbol>.Empty;

Expand Down Expand Up @@ -187,12 +182,33 @@ internal override ImmutableArray<string> GetAppliedConditionalSymbols()
internal override IEnumerable<SecurityAttribute> GetSecurityInformation()
=> Array.Empty<SecurityAttribute>();

internal override bool IsMetadataNewSlot(bool ignoreInterfaceImplementationChanges = false) => false;
internal override bool IsMetadataNewSlot(bool ignoreInterfaceImplementationChanges = false) => !this.IsOverride;

internal override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false) => false;
internal override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false) => this.IsOverride;

internal override bool SynthesizesLoweredBoundBody => true;

internal abstract override void GenerateMethodBody(TypeCompilationState compilationState, DiagnosticBag diagnostics);
}

private sealed class GetAccessorSymbol : AccessorSymbol
{
public override string Name { get; }

public GetAccessorSymbol(SynthesizedRecordPropertySymbol property, string paramName) : base(property)
{
Name = SourcePropertyAccessorSymbol.GetAccessorName(
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Name [](start = 16, length = 4)

In case of an override, shouldn't we inherit the name of the accessor? #Closed

paramName,
getNotSet: true,
isWinMdOutput: false /* unused for getters */);
}

public override MethodKind MethodKind => MethodKind.PropertyGet;

public override TypeWithAnnotations ReturnTypeWithAnnotations => _property.TypeWithAnnotations;

public override ImmutableArray<ParameterSymbol> Parameters => _property.Parameters; // PROTOTYPE: Shouldn't the parameters be owned by this symbol?
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

// PROTOTYPE: Shouldn't the parameters be owned by this symbol? [](start = 96, length = 63)

Yes, they should be. I think #44883 addresses that. #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.

Will merge #44883 when that has been committed.

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


internal override void GenerateMethodBody(TypeCompilationState compilationState, DiagnosticBag diagnostics)
{
// Method body:
Expand All @@ -208,19 +224,17 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState,
}
}

private sealed class InitAccessorSymbol : SynthesizedInstanceMethodSymbol
private sealed class InitAccessorSymbol : AccessorSymbol
{
private readonly SynthesizedRecordPropertySymbol _property;

public override TypeWithAnnotations ReturnTypeWithAnnotations { get; }
public override string Name { get; }

public InitAccessorSymbol(
SynthesizedRecordPropertySymbol property,
string paramName,
DiagnosticBag diagnostics)
DiagnosticBag diagnostics) :
base(property)
{
_property = property;
Name = SourcePropertyAccessorSymbol.GetAccessorName(
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 22, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Name [](start = 16, length = 4)

In case of an override, shouldn't we inherit the name of the accessor? #Closed

paramName,
getNotSet: false,
Expand All @@ -243,87 +257,13 @@ public InitAccessorSymbol(

public override MethodKind MethodKind => MethodKind.PropertySet;

public override int Arity => 0;

public override bool IsExtensionMethod => false;

public override bool HidesBaseMethodsByName => false;

public override bool IsVararg => false;

public override bool ReturnsVoid => true;

public override bool IsAsync => false;

public override RefKind RefKind => RefKind.None;

public override FlowAnalysisAnnotations ReturnTypeFlowAnalysisAnnotations => FlowAnalysisAnnotations.None;

public override ImmutableArray<TypeWithAnnotations> TypeArgumentsWithAnnotations => ImmutableArray<TypeWithAnnotations>.Empty;

public override ImmutableArray<TypeParameterSymbol> TypeParameters => ImmutableArray<TypeParameterSymbol>.Empty;

public override ImmutableArray<ParameterSymbol> Parameters => ImmutableArray.Create(SynthesizedParameterSymbol.Create(
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

SynthesizedParameterSymbol.Create [](start = 96, length = 33)

Every time allocating a new parameter. #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.

This was not part of this PR and has changed in #44883.

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

this,
_property.TypeWithAnnotations,
ordinal: 0,
RefKind.None,
name: ParameterSymbol.ValueParameterName));

public override ImmutableArray<MethodSymbol> ExplicitInterfaceImplementations => ImmutableArray<MethodSymbol>.Empty;

public override ImmutableArray<CustomModifier> RefCustomModifiers => _property.RefCustomModifiers;

public override Symbol AssociatedSymbol => _property;

public override Symbol ContainingSymbol => _property.ContainingSymbol;

public override ImmutableArray<Location> Locations => _property.Locations;

public override Accessibility DeclaredAccessibility => _property.DeclaredAccessibility;

public override bool IsStatic => _property.IsStatic;

public override bool IsVirtual => _property.IsVirtual;

public override bool IsOverride => _property.IsOverride;

public override bool IsAbstract => _property.IsAbstract;

public override bool IsSealed => _property.IsSealed;

public override bool IsExtern => _property.IsExtern;

public override ImmutableHashSet<string> ReturnNotNullIfParameterNotNull => ImmutableHashSet<string>.Empty;

internal override bool HasSpecialName => _property.HasSpecialName;

internal override MethodImplAttributes ImplementationAttributes => MethodImplAttributes.Managed;

internal override bool HasDeclarativeSecurity => false;

internal override MarshalPseudoCustomAttributeData? ReturnValueMarshallingInformation => null;

internal override bool RequiresSecurityObject => false;

internal override CallingConvention CallingConvention => CallingConvention.HasThis;

internal override bool GenerateDebugInfo => false;

public override DllImportData? GetDllImportData() => null;

internal override ImmutableArray<string> GetAppliedConditionalSymbols()
=> ImmutableArray<string>.Empty;

internal override IEnumerable<SecurityAttribute> GetSecurityInformation()
=> Array.Empty<SecurityAttribute>();

internal override bool IsMetadataNewSlot(bool ignoreInterfaceImplementationChanges = false) => false;

internal override bool IsMetadataVirtual(bool ignoreInterfaceImplementationChanges = false) => false;

internal override bool SynthesizesLoweredBoundBody => true;

internal override void GenerateMethodBody(TypeCompilationState compilationState, DiagnosticBag diagnostics)
{
// Method body:
Expand Down
Loading