-
Notifications
You must be signed in to change notification settings - Fork 4k
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: override abstract inherited properties #45336
Conversation
@@ -16,15 +16,13 @@ namespace Microsoft.CodeAnalysis.CSharp.Symbols | |||
internal sealed class SynthesizedRecordPropertySymbol : SourceOrRecordPropertySymbol |
There was a problem hiding this comment.
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
|
||
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? |
There was a problem hiding this comment.
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 TypeWithAnnotations ReturnTypeWithAnnotations => _property.TypeWithAnnotations; | ||
|
||
public override ImmutableArray<ParameterSymbol> Parameters => _property.Parameters; // PROTOTYPE: Shouldn't the parameters be owned by this symbol? |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
existingOrAddedMembers.Add(prop); | ||
if (isInherited && existingMember.IsAbstract) | ||
{ | ||
addProperty(new SynthesizedRecordPropertySymbol(this, param, isOverride: true, diagnostics)); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
public GetAccessorSymbol(SynthesizedRecordPropertySymbol property, string paramName) : base(property) | ||
{ | ||
Name = SourcePropertyAccessorSymbol.GetAccessorName( |
There was a problem hiding this comment.
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
{ | ||
_property = property; | ||
Name = SourcePropertyAccessorSymbol.GetAccessorName( |
There was a problem hiding this comment.
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
public abstract object P5 { init; } | ||
public abstract object P6 { set; } | ||
} | ||
record B(object P1, object P2, object P3, object P4, object P5, object P6) : A; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
object [](start = 9, length = 6)
Are we testing scenario with a type mismatch? #Closed
Done with review pass (iteration 1) #Closed |
@@ -98,46 +97,42 @@ internal sealed class SynthesizedRecordPropertySymbol : SourceOrRecordPropertySy | |||
|
|||
public override SyntaxList<AttributeListSyntax> AttributeDeclarationSyntaxList => new SyntaxList<AttributeListSyntax>(); | |||
|
|||
private sealed class GetAccessorSymbol : SynthesizedInstanceMethodSymbol | |||
private abstract class AccessorSymbol : SynthesizedInstanceMethodSymbol |
There was a problem hiding this comment.
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
IL_0033: add | ||
IL_0034: ret | ||
}"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} [](start = 8, length = 1)
It might worth testing scenario when an abstract property gets shadowed in the intermediate base by some other member with the same name. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test but avoided overriding because the intermediate base members result in errors.
In reply to: 443766696 [](ancestors = 443766696)
I think this should always be true. There is really no relationship like that betwee a property and an accessor., I believe. #Closed Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEqualityContractProperty.cs:145 in d1f9199. [](commit_id = d1f9199, deletion_comment = False) |
It looks like there are the same issues around custom modifiers for an override case. #Closed Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEqualityContractProperty.cs:15 in d1f9199. [](commit_id = d1f9199, deletion_comment = False) |
Same problem as with other accessor types #Closed Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEqualityContractProperty.cs:117 in d1f9199. [](commit_id = d1f9199, deletion_comment = False) |
In one place this type is generalized, in others it is not. For example, is this correct for a static acessor? #Closed Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEqualityContractProperty.cs:155 in d1f9199. [](commit_id = d1f9199, deletion_comment = False) |
"System.Object B.P1 { get; init; }", | ||
"System.Object B.P2 { get; init; }", | ||
"System.Object B.P3 { get; init; }", | ||
"System.Object B.P4 { get; init; }", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
System.Object B.P4 { get; init; } [](start = 17, length = 33)
Where P3 and P4 came from? #Closed
var expectedMembers = new[] | ||
{ | ||
"System.Type B.EqualityContract { get; }", | ||
"System.Object B.P1 { get; init; }", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
System.Object [](start = 17, length = 13)
Should this be string? #Closed
{ | ||
"System.Type B.EqualityContract { get; }", | ||
"System.Object B.P1 { get; init; }", | ||
"System.Object B.P2 { get; init; }", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
System.Object [](start = 17, length = 13)
Should this be string? #Closed
// (12,8): error CS0534: 'C' does not implement inherited abstract member 'A.X.init' | ||
// record C(object X, object Y) : B; | ||
Diagnostic(ErrorCode.ERR_UnimplementedAbstractMethod, "C").WithArguments("C", "A.X.init").WithLocation(12, 8), | ||
// (12,8): error CS0534: 'C' does not implement inherited abstract member 'A.Y.init' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// (12,8): error CS0534: 'C' does not implement inherited abstract member 'A.Y.init' [](start = 16, length = 84)
It looks like a duplicate error is reported. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error is for A.Y.init
, the previous error is for A.X.init
.
In reply to: 443958403 [](ancestors = 443958403)
// (12,8): error CS0534: 'C' does not implement inherited abstract member 'A.X.get' | ||
// record C(object X, object Y) : B; | ||
Diagnostic(ErrorCode.ERR_UnimplementedAbstractMethod, "C").WithArguments("C", "A.X.get").WithLocation(12, 8), | ||
// (12,8): error CS0534: 'C' does not implement inherited abstract member 'A.Y.get' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// (12,8): error CS0534: 'C' does not implement inherited abstract member 'A.Y.get' [](start = 16, length = 83)
Here is another one #Closed
@"record B(object P) : A;"; | ||
var comp = CreateCompilation(sourceB, new[] { refA }, parseOptions: TestOptions.RegularPreview); | ||
comp.VerifyDiagnostics(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we execute and observe that runtime considers things implemented by property in B? #Closed
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( |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.property instance object modopt(uint16) P() | ||
{ | ||
.get instance object modopt(uint16) A::get_P() | ||
.set instance void modreq(System.Runtime.CompilerServices.IsExternalInit) A::set_P(object modopt(uint16)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modreq(System.Runtime.CompilerServices.IsExternalInit) [](start = 23, length = 55)
Please add a modopt here in addition to the modreq. In definition too. #Closed
{ | ||
var parameterType = setMethod.Parameters[0].TypeWithAnnotations; | ||
Assert.True(setMethod.OverriddenMethod.Parameters[0].TypeWithAnnotations.Equals(parameterType, TypeCompareKind.ConsiderEverything)); | ||
AssertEx.Equal(expectedModifiers, parameterType.CustomModifiers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AssertEx.Equal(expectedModifiers, parameterType.CustomModifiers); [](start = 20, length = 65)
Should also assert modifiers on return type. #Closed
var sourceB = | ||
@"record B(object P) : A;"; | ||
var comp = CreateCompilation(sourceB, new[] { refA }, parseOptions: TestOptions.RegularPreview); | ||
comp.VerifyDiagnostics(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comp.VerifyDiagnostics(); [](start = 12, length = 25)
Can we execute and observe that runtime considers things implemented by property in B? #Closed
Done with review pass (iteration 15), tests are not looked at. #Closed |
string name, | ||
Location location, | ||
TypeWithAnnotations typeOpt, | ||
ImmutableArray<ParameterSymbol> parametersOpt, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parametersOpt [](start = 44, length = 13)
This could either be empty, or default. Consider passing a boolean "hasParameters" instead. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsDefault captures that.
Why give an illusion that you can pass a non-empty array?
In reply to: 446549153 [](ancestors = 446549153,446545495)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't often allow a default
value for Parameters
.
In reply to: 446555527 [](ancestors = 446555527,446549153,446545495)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't often allow a default value for Parameters.
Sorry, I am not following. Let me rephrase the question. Why use an array type to communicate a boolean state? Things simply wouldn't work properly if one passes an array with actual parameter symbols in it. No one makes sure the symbols would be linked properly. So, why give consumer an illusion that the constructor can properly deal with the situation?
In reply to: 446569515 [](ancestors = 446569515,446555527,446549153,446545495)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, thanks.
In reply to: 446582904 [](ancestors = 446582904,446569515,446555527,446549153,446545495)
There are minimal changes to this file, some of which are responses to earlier comments. In reply to: 650563133 [](ancestors = 650563133) Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEqualityContractProperty.cs:15 in 63b0dae. [](commit_id = 63b0dae, deletion_comment = False) |
This is all throw away work, I am going to rewrite this anyway. Up to you. In reply to: 650598383 [](ancestors = 650598383,650563133) Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEqualityContractProperty.cs:15 in 63b0dae. [](commit_id = 63b0dae, deletion_comment = False) |
It doesn't look like the change went for the better. In reply to: 647868847 [](ancestors = 647868847) Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEqualityContractProperty.cs:145 in d1f9199. [](commit_id = d1f9199, deletion_comment = False) |
|
||
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 |
There was a problem hiding this comment.
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
andinit
auto-property is created (see separateinit
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
@@ -3227,15 +3230,15 @@ public void Inheritance_08() | |||
}"; | |||
var comp = CreateCompilation(source); | |||
comp.VerifyDiagnostics( | |||
// (11,8): error CS0534: 'C' does not implement inherited abstract member 'B.Y.get' | |||
// (11,14): error CS0546: 'C.X.init': cannot override because 'A.X' does not have an overridable set accessor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// (11,14): error CS0546: 'C.X.init': cannot override because 'A.X' does not have an overridable set accessor [](start = 16, length = 109)
It doesn't look like this error follows specification from the #44618.
It says:
Each "matching" inherited abstract property's get accessor is overridden.
However it doesn't say anything about init/set accessors. From reading the specification I am getting an impression that we shouldn't be reporting an error for this scenario, we override the getter and leave the init accessor not overriding anything. This, however, isn't something that you can express in a regular C# today. So, it would make sense to not support this for synthesized properties as well. Please, include amended specification in the description of this PR, so that reviewers could review against it. #Resolved
{ | ||
B b = new B(3); | ||
WriteLine(b.P); | ||
WriteLine(((A)b).P); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WriteLine(((A)b).P); [](start = 8, length = 20)
It doesn't look like we are exercising overriding of the setter #Closed
{ | ||
B b = new B(3); | ||
WriteLine(b.P); | ||
WriteLine(((A)b).P); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WriteLine(((A)b).P); [](start = 8, length = 20)
It doesn't look like we are exercising overriding of the setter #Closed
{ | ||
B b = new B(3); | ||
WriteLine(b.P); | ||
WriteLine(((A)b).P); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WriteLine(((A)b).P); [](start = 8, length = 20)
It doesn't look like we are exercising overriding of the setter #Closed
Done with review pass (iteration 17) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (iteration 20)
@dotnet/roslyn-compiler, please review. |
@@ -144,7 +147,7 @@ public GetAccessorSymbol(SynthesizedRecordEqualityContractProperty property) | |||
|
|||
public override ImmutableHashSet<string> ReturnNotNullIfParameterNotNull => ImmutableHashSet<string>.Empty; | |||
|
|||
internal override bool HasSpecialName => _property.HasSpecialName; | |||
internal override bool HasSpecialName => true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this change, doesn't this correspond to the 'rtspecialname' metadata flag? #ByDesign
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HasSpecialName
corresponds to the specialname
metadata bit. Property accessors should set that bit.
In reply to: 447926980 [](ancestors = 447926980)
From the records spec:
Fixes #44618