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 all commits
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
2 changes: 1 addition & 1 deletion src/Compilers/CSharp/Portable/Symbols/OverriddenOrHiddenMembersHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ internal static OverriddenOrHiddenMembersResult MakeInterfaceOverriddenOrHiddenM
///
/// In incorrect or imported code, it is possible that both currTypeBestMatch and hiddenBuilder will be populated.
/// </remarks>
internal static void FindOverriddenOrHiddenMembersInType(
private static void FindOverriddenOrHiddenMembersInType(
Symbol member,
bool memberIsFromSomeCompilation,
NamedTypeSymbol memberContainingType,
Expand Down
86 changes: 32 additions & 54 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 @@ -3065,26 +3065,32 @@ 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 syntax = param.GetNonNullSyntaxNode();
var property = new SynthesizedRecordPropertySymbol(this, syntax, param, isOverride: false, diagnostics);
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 27, 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 = 39, length = 31)

Consider using SignatureOnlyPropertySymbol to check for an existing member. I am planning to switch all checks like that in this function to use SignatureOnly symbols. I think this will eliminate some overhead and will add some predictability in terms of what is happening and when. #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 seems like a separable change.

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

if (!memberSignatures.TryGetValue(property, out var existingMember))
{
Debug.Assert(property.OverriddenOrHiddenMembers.OverriddenMembers.Length == 0); // property is not virtual and should not have overrides
existingMember = property.OverriddenOrHiddenMembers.HiddenMembers.FirstOrDefault();
Copy link
Contributor

@AlekseyTs AlekseyTs Jun 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

property.OverriddenOrHiddenMembers.HiddenMembers.FirstOrDefault(); [](start = 41, length = 66)

Do we care if the hidden member is not a property? #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.

Do we care if the hidden member is not a property?

Not currently we don't. We report an error if the property hides a distinct member. (That is existing behavior.)

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

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 && prop.IsAbstract)
{
addProperty(new SynthesizedRecordPropertySymbol(this, syntax, param, isOverride: true, diagnostics));
}
else
{
// Deconstruct() is specified to simply assign from this property to the corresponding out parameter.
existingOrAddedMembers.Add(prop);
}
}
else
{
Expand All @@ -3094,6 +3100,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 All @@ -3107,32 +3125,6 @@ ImmutableArray<PropertySymbol> addProperties(ImmutableArray<ParameterSymbol> rec
return existingOrAddedMembers.ToImmutableAndFree();
}

static Symbol? getInheritedMember(Symbol symbol, NamedTypeSymbol type)
{
while ((type = type.BaseTypeNoUseSiteDiagnostics) is object)
{
OverriddenOrHiddenMembersHelpers.FindOverriddenOrHiddenMembersInType(
symbol,
memberIsFromSomeCompilation: true,
memberContainingType: symbol.ContainingType,
currType: type,
out var bestMatch,
out bool hasSameKindNonMatch,
out var hiddenBuilder);
if (hiddenBuilder is object)
{
var result = hiddenBuilder[0];
hiddenBuilder.Free();
return result;
}
if (bestMatch is object)
{
return bestMatch;
}
}
return null;
}

void addObjectEquals(MethodSymbol thisEquals)
{
var objEquals = new SynthesizedRecordObjEquals(this, thisEquals, memberOffset: members.Count, diagnostics);
Expand All @@ -3149,23 +3141,9 @@ void addHashCode(PropertySymbol equalityContract)
}
}

static PropertySymbol? getInheritedEqualityContract(NamedTypeSymbol type)
{
while ((type = type.BaseTypeNoUseSiteDiagnostics) is object)
{
var members = type.GetMembers(SynthesizedRecordEqualityContractProperty.PropertyName);
// https://github.com/dotnet/roslyn/issues/44903: Check explicit member has expected signature.
if (members.FirstOrDefault(m => m is PropertySymbol property && property.ParameterCount == 0) is PropertySymbol property)
{
return property;
}
}
return null;
}

PropertySymbol addEqualityContract()
{
var property = new SynthesizedRecordEqualityContractProperty(this, isOverride: getInheritedEqualityContract(this) is object);
var property = new SynthesizedRecordEqualityContractProperty(this, isOverride: SynthesizedRecordClone.FindValidCloneMethod(BaseTypeNoUseSiteDiagnostics) is object);
// https://github.com/dotnet/roslyn/issues/44903: Check explicit member has expected signature.
if (!memberSignatures.ContainsKey(property))
{
Expand Down
108 changes: 91 additions & 17 deletions src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbol.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,23 +21,96 @@ internal static SourcePropertySymbol Create(SourceMemberContainerTypeSymbol cont
{
var nameToken = syntax.Identifier;
var location = nameToken.GetLocation();
return new SourcePropertySymbol(containingType, bodyBinder, syntax, nameToken.ValueText, location, diagnostics);
return Create(containingType, bodyBinder, syntax, nameToken.ValueText, location, diagnostics);
}

internal static SourcePropertySymbol Create(SourceMemberContainerTypeSymbol containingType, Binder bodyBinder, IndexerDeclarationSyntax syntax, DiagnosticBag diagnostics)
{
var location = syntax.ThisKeyword.GetLocation();
return new SourcePropertySymbol(containingType, bodyBinder, syntax, DefaultIndexerName, location, diagnostics);
return Create(containingType, bodyBinder, syntax, DefaultIndexerName, location, diagnostics);
}

private static SourcePropertySymbol Create(
SourceMemberContainerTypeSymbol containingType,
Binder bodyBinder,
BasePropertyDeclarationSyntax syntax,
string name,
Location location,
DiagnosticBag diagnostics)
{
GetAccessorDeclarations(
syntax,
diagnostics,
out bool isAutoProperty,
out bool hasAccessorList,
out bool accessorsHaveImplementation,
out bool isInitOnly,
out var getSyntax,
out var setSyntax);
// This has the value that IsIndexer will ultimately have, once we've populated the fields of this object.
bool isIndexer = syntax.Kind() == SyntaxKind.IndexerDeclaration;
var explicitInterfaceSpecifier = GetExplicitInterfaceSpecifier(syntax);
var modifiers = MakeModifiers(
containingType,
GetModifierTokensSyntax(syntax),
isExplicitInterfaceImplementation: explicitInterfaceSpecifier is object,
isIndexer: isIndexer,
accessorsHaveImplementation: accessorsHaveImplementation,
location,
diagnostics,
out _);
return new SourcePropertySymbol(
containingType,
bodyBinder,
syntax,
getSyntax: getSyntax,
setSyntax: setSyntax,
explicitInterfaceSpecifier,
modifiers,
isIndexer: isIndexer,
isAutoProperty: isAutoProperty,
hasAccessorList: hasAccessorList,
isInitOnly: isInitOnly,
name,
location,
diagnostics);
}

private SourcePropertySymbol(
SourceMemberContainerTypeSymbol containingType,
Binder bodyBinder,
BasePropertyDeclarationSyntax syntax,
CSharpSyntaxNode? getSyntax,
CSharpSyntaxNode? setSyntax,
ExplicitInterfaceSpecifierSyntax? explicitInterfaceSpecifier,
DeclarationModifiers modifiers,
bool isIndexer,
bool isAutoProperty,
bool hasAccessorList,
bool isInitOnly,
string name,
Location location,
DiagnosticBag diagnostics)
: base(containingType, bodyBinder, syntax, syntax.Type.GetRefKind(), name, location, diagnostics)
: base(
containingType,
bodyBinder,
syntax,
getSyntax: getSyntax,
setSyntax: setSyntax,
arrowExpression: GetArrowExpression(syntax),
explicitInterfaceSpecifier,
modifiers,
isIndexer: isIndexer,
hasInitializer: HasInitializer(syntax),
isAutoProperty: isAutoProperty,
hasAccessorList: hasAccessorList,
isInitOnly: isInitOnly,
syntax.Type.GetRefKind(),
name,
location,
typeOpt: default,
hasParameters: GetParameterListSyntax(syntax) is object,
diagnostics)
{
}

Expand All @@ -47,23 +120,26 @@ protected override Location TypeLocation
=> GetTypeSyntax(CSharpSyntaxNode).Location;

protected override SyntaxTokenList GetModifierTokens(SyntaxNode syntax)
=> GetModifierTokensSyntax(syntax);

private static SyntaxTokenList GetModifierTokensSyntax(SyntaxNode syntax)
=> ((BasePropertyDeclarationSyntax)syntax).Modifiers;

protected override ArrowExpressionClauseSyntax? GetArrowExpression(SyntaxNode syntax)
private static ArrowExpressionClauseSyntax? GetArrowExpression(SyntaxNode syntax)
=> syntax switch
{
PropertyDeclarationSyntax p => p.ExpressionBody,
IndexerDeclarationSyntax i => i.ExpressionBody,
_ => throw ExceptionUtilities.UnexpectedValue(syntax.Kind())
};

protected override bool HasInitializer(SyntaxNode syntax)
private static bool HasInitializer(SyntaxNode syntax)
=> syntax is PropertyDeclarationSyntax { Initializer: { } };

public override SyntaxList<AttributeListSyntax> AttributeDeclarationSyntaxList
=> ((BasePropertyDeclarationSyntax)CSharpSyntaxNode).AttributeLists;

protected override void GetAccessorDeclarations(
private static void GetAccessorDeclarations(
CSharpSyntaxNode syntaxNode,
DiagnosticBag diagnostics,
out bool isAutoProperty,
Expand Down Expand Up @@ -148,7 +224,8 @@ protected override void CheckForBlockAndExpressionBody(CSharpSyntaxNode syntax,
diagnostics);
}

protected override DeclarationModifiers MakeModifiers(
private static DeclarationModifiers MakeModifiers(
NamedTypeSymbol containingType,
SyntaxTokenList modifiers,
bool isExplicitInterfaceImplementation,
bool isIndexer,
Expand All @@ -157,7 +234,7 @@ protected override DeclarationModifiers MakeModifiers(
DiagnosticBag diagnostics,
out bool modifierErrors)
{
bool isInterface = this.ContainingType.IsInterface;
bool isInterface = containingType.IsInterface;
var defaultAccess = isInterface && !isExplicitInterfaceImplementation ? DeclarationModifiers.Public : DeclarationModifiers.Private;

// Check that the set of modifiers is allowed
Expand Down Expand Up @@ -201,7 +278,7 @@ protected override DeclarationModifiers MakeModifiers(
allowedModifiers |= DeclarationModifiers.Abstract;
}

if (ContainingType.IsStructType())
if (containingType.IsStructType())
{
allowedModifiers |= DeclarationModifiers.ReadOnly;
}
Expand All @@ -210,7 +287,7 @@ protected override DeclarationModifiers MakeModifiers(

var mods = ModifierUtils.MakeAndCheckNontypeMemberModifiers(modifiers, defaultAccess, allowedModifiers, location, diagnostics, out modifierErrors);

this.CheckUnsafeModifier(mods, diagnostics);
containingType.CheckUnsafeModifier(mods, location, diagnostics);

ModifierUtils.ReportDefaultInterfaceImplementationModifiers(accessorsHaveImplementation, mods,
defaultInterfaceImplementationModifiers,
Expand All @@ -235,7 +312,7 @@ protected override DeclarationModifiers MakeModifiers(
bool isGet,
CSharpSyntaxNode? syntaxOpt,
PropertySymbol? explicitlyImplementedPropertyOpt,
string aliasQualifierOpt,
string? aliasQualifierOpt,
bool isAutoPropertyAccessor,
bool isExplicitInterfaceImplementation,
DiagnosticBag diagnostics)
Expand All @@ -260,7 +337,7 @@ protected override DeclarationModifiers MakeModifiers(
protected override SourcePropertyAccessorSymbol CreateExpressionBodiedAccessor(
ArrowExpressionClauseSyntax syntax,
PropertySymbol? explicitlyImplementedPropertyOpt,
string aliasQualifierOpt,
string? aliasQualifierOpt,
bool isExplicitInterfaceImplementation,
DiagnosticBag diagnostics)
{
Expand Down Expand Up @@ -389,10 +466,7 @@ protected override bool HasPointerTypeSyntactically
}
}

protected override ExplicitInterfaceSpecifierSyntax? GetExplicitInterfaceSpecifier(SyntaxNode syntax)
=> ((BasePropertyDeclarationSyntax)syntax).ExplicitInterfaceSpecifier;

protected override BaseParameterListSyntax? GetParameterListSyntax(CSharpSyntaxNode syntax)
private static BaseParameterListSyntax? GetParameterListSyntax(CSharpSyntaxNode syntax)
=> (syntax as IndexerDeclarationSyntax)?.ParameterList;
}
}
}
Loading