Align addition of a synthesized EqualityContract in records with the latest design. by AlekseyTs · Pull Request #45831 · dotnet/roslyn

AlekseyTs · 2020-07-09T16:15:57Z

Related to #45296.

From specification:
If the record is derived from object, the record type includes a synthesized readonly property equivalent to a property declared as follows:

protected Type EqualityContract { get; };

The property is virtual unless the record type is sealed.
The property can be declared explicitly. It is an error if the explicit declaration does not match the expected signature or accessibility, or if the explicit declaration doesn't allow overiding it in a derived type and the record type is not sealed.

If the record type is derived from a base record type Base, the record type includes a synthesized readonly property equivalent to a property declared as follows:

protected override Type EqualityContract { get; };

The property can be declared explicitly. It is an error if the explicit declaration does not match the expected signature or accessibility, or if the explicit declaration doesn't allow overriding it in a derived type and the record type is not sealed. It is an error if either synthesized, or explicitly declared property doesn't override a property with this signature in the record type Base (for example, if the property is missing in the Base, or sealed, or not virtual, etc.).
The synthesized property returns typeof(R) where R is the record type.

…latest design. Related to dotnet#45296. From specification: If the record is derived from `object`, the record type includes a synthesized readonly property equivalent to a property declared as follows: ```C# protected Type EqualityContract { get; }; ``` The property is `virtual` unless the record type is `sealed`. The property can be declared explicitly. It is an error if the explicit declaration does not match the expected signature or accessibility, or if the explicit declaration doesn't allow overiding it in a derived type and the record type is not `sealed`. If the record type is derived from a base record type `Base`, the record type includes a synthesized readonly property equivalent to a property declared as follows: ```C# protected override Type EqualityContract { get; }; ``` The property can be declared explicitly. It is an error if the explicit declaration does not match the expected signature or accessibility, or if the explicit declaration doesn't allow overiding it in a derived type and the record type is not `sealed`. It is an error if either synthesized, or explicitly declared property doesn't override a property with this signature in the record type `Base` (for example, if the property is missing in the `Base`, or sealed, or not virtual, etc.). The synthesized property returns `typeof(R)` where `R` is the record type.

RikkiGibson · 2020-07-09T17:23:25Z

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs

+                     ReportProtectedMemberInSealedTypeError(location, diagnostics))
            {
-                diagnostics.Add(AccessCheck.GetProtectedMemberInSealedTypeError(ContainingType), location, this);
+                ;


Is this empty statement intended? #Closed

Is this empty statement intended?

Yes.

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

RikkiGibson · 2020-07-09T17:45:49Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs

+";
+            var comp = CreateCompilation(source, parseOptions: TestOptions.RegularPreview, options: TestOptions.ReleaseDll);
+            comp.VerifyEmitDiagnostics(
+                // (4,27): error CS8874: Record member 'A.EqualityContract' must return 'Type'.


nit: it seems like 'Type' here will be 'System.Type' in the actual diagnostic message #Resolved

nit: it seems like 'Type' here will be 'System.Type' in the actual diagnostic message

This is exactly what the helper printed.

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

AlekseyTs · 2020-07-09T18:13:40Z

@cston, @jcouv, @dotnet/roslyn-compiler Please review, need a second sign-off.

jcouv · 2020-07-09T20:34:27Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

-                // https://github.com/dotnet/roslyn/issues/44903: Check explicit member has expected signature.
+                PropertySymbol equalityContract;
+
                if (!memberSignatures.TryGetValue(targetProperty, out Symbol? existingEqualityContractProperty))


FYI, added a note to test plan to confirm nullability expectations on this signature (if I implement protected virtual Type? EqualityContract { get; }, should the compiler give a warning?) #Resolved

jcouv · 2020-07-09T20:42:17Z

src/Compilers/CSharp/Portable/Symbols/Source/SourcePropertySymbolBase.cs

            }
-            else if (ContainingType.IsSealed && this.DeclaredAccessibility.HasProtected() && !this.IsOverride)
+            else if (ContainingType.IsSealed && this.DeclaredAccessibility.HasProtected() && !this.IsOverride &&
+                     ReportProtectedMemberInSealedTypeError(location, diagnostics))


ReportProtectedMemberInSealedTypeError [](start = 21, length = 38)

Consider keeping the diagnostic here, but just having a boolean property ShouldReportProtectedMemberInSealedTypeError (true for this type, false for derived type) #Resolved

jcouv · 2020-07-09T20:44:57Z

...ers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEqualityContractProperty.cs

            {
-                _property = property;
-                Name = name;
+                return null;


Can we reach this? If not, consider throwing as unreachable instead. #Resolved

Can we reach this? If not, consider throwing as unreachable instead.

Yes, this method is called for both accessors, it is up to the method to decide if it should create one.

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

jcouv · 2020-07-09T20:48:36Z

...ers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEqualityContractProperty.cs


-            internal override ImmutableArray<string> GetAppliedConditionalSymbols()
-                => ImmutableArray<string>.Empty;
+                if (overridden is object &&


overridden is object [](start = 20, length = 20)

nit: is this check necessary given that we've already checked .IsOverride? #Resolved

nit: is this check necessary given that we've already checked .IsOverride?

Yes, there could be nothing to override, even when we want to, and IsOverride is simply a reflection of the intent

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

jcouv · 2020-07-09T20:49:12Z

...ers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEqualityContractProperty.cs

+                       hasBody: false,
+                       hasExpressionBody: false,
+                       isIterator: false,
+                       modifiers: new SyntaxTokenList(),


singleton here too? #Resolved

singleton here too?

Same here, it is a struct

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

jcouv · 2020-07-09T20:55:46Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs

+            comp.VerifyDiagnostics();
+            CompileAndVerify(comp, expectedOutput:
+@"
+B.EqualityContract


I didn't understand why there are two calls to EqualityContract #Resolved

I didn't understand why there are two calls to EqualityContract

From specification for record equals method:
If there is a base record type, the value of base.Equals(other) (a non-virtual call to public virtual bool Equals(Base? other)); otherwise the value of EqualityContract == other.EqualityContract.

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

jcouv · 2020-07-09T21:01:27Z

src/Compilers/CSharp/Test/Semantic/Semantics/RecordTests.cs

+                //     protected override System.Type EqualityContract
+                Diagnostic(ErrorCode.ERR_DoesNotOverrideBaseEqualityContract, "EqualityContract").WithArguments("G.EqualityContract", "B").WithLocation(21, 36)
+                );
+        }


I couldn't find a test verifying IL for EqualityContract implementation. Consider adding one #Resolved

I couldn't find a test verifying IL for EqualityContract implementation. Consider adding one

I am not changing anything about emitted IL and I assume that that kind of testing has been already done, if necessary.

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

I'm saying there doesn't seem to be any (that I could find), so may be a good occasion to cover, while in the area.

In reply to: 452488774 [](ancestors = 452488774,452487751)

I'm saying there doesn't seem to be any (that I could find), so may be a good occasion to cover, while in the area.

Given simplicity of what this property does, I do not believe IL verification is warranted. Behavior tests are good enough. I will add assert in a couple of tests to verify the actual value the property returns.

In reply to: 452489813 [](ancestors = 452489813,452488774,452487751)

jcouv · 2020-07-09T21:03:24Z

src/Compilers/CSharp/Portable/Symbols/Source/SourceMemberContainerSymbol.cs

+                    }
+
+
+                    reportNotOverridableAPIInRecord(equalityContract, diagnostics);


nit: double empty line above #Resolved

jcouv · 2020-07-09T21:03:29Z

...ers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEqualityContractProperty.cs


-        internal override bool MustCallMethodsDirectly => false;
+        protected override SyntaxTokenList GetModifierTokens(SyntaxNode syntax)
+            => new SyntaxTokenList();


nit: could we use a singleton rather than allocate? #Closed

nit: could we use a singleton rather than allocate?

No allocation here, it is a struct

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

Ah, thanks

In reply to: 452489159 [](ancestors = 452489159,452488746)

jcouv

LGTM Thanks (iteration 1)

AlekseyTs added Area-Compilers Feature - Records Records labels Jul 9, 2020

AlekseyTs requested review from a team, RikkiGibson, cston and jcouv July 9, 2020 16:15

RikkiGibson approved these changes Jul 9, 2020

View reviewed changes

jcouv reviewed Jul 9, 2020

View reviewed changes

jcouv approved these changes Jul 9, 2020

View reviewed changes

jcouv self-assigned this Jul 9, 2020

PR feedback

2073687

AlekseyTs merged commit 32da307 into dotnet:master Jul 10, 2020

ghost added this to the Next milestone Jul 10, 2020

JoeRobich modified the milestones: Next, 16.8.P1 Jul 20, 2020

		}


		reportNotOverridableAPIInRecord(equalityContract, diagnostics);

Conversation

AlekseyTs commented Jul 9, 2020

Uh oh!

RikkiGibson Jul 9, 2020 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Jul 9, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Jul 9, 2020

Uh oh!

jcouv Jul 9, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 9, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 9, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 9, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 9, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 9, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 9, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 9, 2020 • edited by AlekseyTs Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Jul 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

RikkiGibson Jul 9, 2020 •

edited by jcouv

Loading

RikkiGibson Jul 9, 2020 •

edited by AlekseyTs

Loading

jcouv Jul 9, 2020 •

edited by AlekseyTs

Loading

jcouv Jul 9, 2020 •

edited by AlekseyTs

Loading

jcouv Jul 9, 2020 •

edited by AlekseyTs

Loading

jcouv Jul 9, 2020 •

edited by AlekseyTs

Loading

jcouv Jul 9, 2020 •

edited by AlekseyTs

Loading

jcouv Jul 9, 2020 •

edited by AlekseyTs

Loading

jcouv Jul 9, 2020 •

edited by AlekseyTs

Loading

jcouv Jul 9, 2020 •

edited by AlekseyTs

Loading

jcouv Jul 9, 2020 •

edited

Loading