Misc. changes following recent records PR #44912
Conversation
| @@ -104,9 +102,9 @@ 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) => true; | |||
There was a problem hiding this comment.
internal override bool IsMetadataNewSlot(bool ignoreInterfaceImplementationChanges = false) => true; [](start = 8, length = 100)
It looks like meaningful changes in this file a obsolete after #44852 #Closed
| ordinal: 0, | ||
| isStruct ? RefKind.In : RefKind.None)); | ||
| RefKind.None)); | ||
| ReturnTypeWithAnnotations = TypeWithAnnotations.Create(compilation.GetSpecialType(SpecialType.System_Boolean)); |
There was a problem hiding this comment.
compilation.GetSpecialType(SpecialType.System_Boolean) [](start = 67, length = 54)
We should use static helper from Binder, which reports use-site diagnostics.
| null : | ||
| F.ObjectNotEqual(other, F.Null(F.SpecialType(SpecialType.System_Object))); | ||
| Debug.Assert(!other.Type.IsStructType()); | ||
| retExpr = F.ObjectNotEqual(other, F.Null(F.SpecialType(SpecialType.System_Object))); |
There was a problem hiding this comment.
F.SpecialType [](start = 61, length = 13)
Is this going to report use-site error, if any?
We should use static helper from Binder, which reports use-site diagnostics. Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordObjEquals.cs:32 in b7e1fe9. [](commit_id = b7e1fe9, deletion_comment = False) |
We should use static helper from Binder, which reports use-site diagnostics. Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordObjEquals.cs:35 in b7e1fe9. [](commit_id = b7e1fe9, deletion_comment = False) |
|
Done with review pass (iteration 1) |
| @@ -54,7 +55,7 @@ public SynthesizedRecordEqualityContractProperty(NamedTypeSymbol containingType, | |||
|
|
|||
| public override bool IsStatic => false; | |||
|
|
|||
| public override bool IsVirtual => true; | |||
| public override bool IsVirtual { get; } | |||
There was a problem hiding this comment.
I didn't understand that change. Seems like it should always be virtual, so that derived record types could override the equality contract.
On the other hand, this makes me wonder, should IsSealed below be based on whether the record type is sealed?
There was a problem hiding this comment.
Never mind, the PR description explain and clarifies ("IsVirtual and IsOverride cannot both be true"). Thanks!
In reply to: 436283471 [](ancestors = 436283471)
|
Squash merging based on Chuck's suggestion |
Address feedback following #44882:
IsVirtualandIsOverridecannot both betruestructparameter case fromSynthesizedRecordEqualsRecordTests