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.
SynthesizedRecordPropertySymbol [](start = 26, length = 31)
It looks like changes made in this file are going to conflict with #44883 #Closed
| 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.
// 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.
// PROTOTYPE: Shouldn't the parameters be owned by this symbol? [](start = 96, length = 63)
Yes, they should be. I think #44883 addresses that. #Closed
| existingOrAddedMembers.Add(prop); | ||
| if (isInherited && existingMember.IsAbstract) | ||
| { | ||
| addProperty(new SynthesizedRecordPropertySymbol(this, param, isOverride: true, diagnostics)); |
There was a problem hiding this comment.
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
|
|
||
| public GetAccessorSymbol(SynthesizedRecordPropertySymbol property, string paramName) : base(property) | ||
| { | ||
| Name = SourcePropertyAccessorSymbol.GetAccessorName( |
There was a problem hiding this comment.
Name [](start = 16, length = 4)
In case of an override, shouldn't we inherit the name of the accessor? #Closed
| base(property) | ||
| { | ||
| _property = property; | ||
| Name = SourcePropertyAccessorSymbol.GetAccessorName( |
There was a problem hiding this comment.
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.
object [](start = 9, length = 6)
Are we testing scenario with a type mismatch? #Closed
|
Done with review pass (iteration 1) #Closed |
| 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.
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.
} [](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.
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.
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.
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.
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.
// (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.
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.
// (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.
Can we execute and observe that runtime considers things implemented by property in B? #Closed
|
|
||
| public override ImmutableArray<TypeParameterSymbol> TypeParameters => ImmutableArray<TypeParameterSymbol>.Empty; | ||
|
|
||
| public override ImmutableArray<ParameterSymbol> Parameters => ImmutableArray.Create(SynthesizedParameterSymbol.Create( |
There was a problem hiding this comment.
SynthesizedParameterSymbol.Create [](start = 96, length = 33)
Every time allocating a new parameter. #Closed
| .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.
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.
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.
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.
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.
There was a problem hiding this comment.
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.
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.
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.
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) |
| addedCount++; | ||
| addProperty(property); | ||
| } | ||
| else if (existingMember is PropertySymbol { IsStatic: false, GetMethod: { } } prop |
There was a problem hiding this comment.
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
getandinitauto-property is created (see separateinitaccessor 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
| 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.
// (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.
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.
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.
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) |
|
@dotnet/roslyn-compiler, please review. |
| 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.
I don't understand this change, doesn't this correspond to the 'rtspecialname' metadata flag? #ByDesign
There was a problem hiding this comment.
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