Add CompilerGeneratedAttribute to record members #58542

Youssef1313 · 2021-12-31T16:15:57Z

Youssef1313 · 2022-01-02T05:32:59Z

src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordCopyCtor.cs

+        internal override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, ref ArrayBuilder<SynthesizedAttributeData> attributes)
+        {
+            base.AddSynthesizedAttributes(moduleBuilder, ref attributes);
+            Debug.Assert(IsImplicitlyDeclared);
+            var compilation = this.DeclaringCompilation;
+            AddSynthesizedAttribute(ref attributes, compilation.TrySynthesizeAttribute(WellKnownMember.System_Runtime_CompilerServices_CompilerGeneratedAttribute__ctor));
+            Debug.Assert(WellKnownMembers.IsSynthesizedAttributeOptional(WellKnownMember.System_Runtime_CompilerServices_CompilerGeneratedAttribute__ctor));
+        }


I can see a room for refactoring, since this block is repeated in many places (not only in this PR - but over the codebase)

For example, we can have a virtual property in Symbol:

protected virtual bool SynthesizeCompilerGeneratedAttribute => false;

and in the already-existing empty virtual AddSynthesizedAttributes method:

if (SynthesizeCompilerGeneratedAttribute) { // Add this repeated code block here. }

Let me know if the refactoring worth it, and (if yes) whether you want it in this PR or a separate one.

Youssef1313 · 2022-01-02T06:07:55Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Synthesized.cs

+                var equalityContract = record.GetMember("EqualityContract");
+                validateCompilerGeneratedAttribute(equalityContract);


Should both get_EqualityContract and EqualityContract members have the attribute? or only EqualityContract property? Or only get_EqualityContract (unlikely to be correct - but it's the current behavior)?
#Closed

@AlekseyTs Can you point out to the expected behavior here?

I think it's okay to only have the CompilerGenerated attribute on the property and not the accessor. It's implied that the accessor must be compiler-generated too (it wouldn't make sense to have an explicit accessor in an implicitly declared property).

Please capture the behavior (lack of attribute on accessor) in test

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Synthesized.cs

jcouv · 2022-01-18T18:29:56Z

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Synthesized.cs

@@ -652,6 +652,255 @@ internal class C2<[Attr] T2> { }
            Assert.Equal(new[] { "Attr" }, GetAttributeNames(typeParam.GetAttributes()));
        }

+        [Fact]
+        [WorkItem(46439, "https://github.com/dotnet/roslyn/issues/46439")]
+        public void RecordSynthesizedMembers()


nit: Consider also validating that methods provided explicitly by the user don't get the compiler-generated attribute. #Closed

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Synthesized.cs

jcouv

Done with review pass (iteration 6)

jcouv · 2022-01-18T22:51:24Z

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

+            var compilation = this.DeclaringCompilation;
+            AddSynthesizedAttribute(ref attributes, compilation.TrySynthesizeAttribute(WellKnownMember.System_Runtime_CompilerServices_CompilerGeneratedAttribute__ctor));
+            Debug.Assert(WellKnownMembers.IsSynthesizedAttributeOptional(WellKnownMember.System_Runtime_CompilerServices_CompilerGeneratedAttribute__ctor));
+        }


Seeing this code moved (from accessor to property) and looking at the PR this came from (#57917, reviewed by Aleksey and Fred), I think the previous decision was to put the attribute on the accessor. Let's keep it that way. Sorry about that. #Closed

#57917 was intended to be (mostly) refactoring only (no behavioral changes around the attribute waa introduced).

I'm more convinced with the attribute being on the property.

@AlekseyTs @333fred What do you think?

I'm not certain what precedent we have for where we put CompilerGenerated. For example, in top-level statements, if we generate a closure type we don't put a CompilerGenerated on it because the containing type has CompilerGenerated. If the algorithm is "put it on the highest possible point", then putting it on the property instead of the accessors makes sense. If that's not the algorithm, then I'd appreciate knowing the algorithm :).

Thinking about this more, if we put the attribute on the property, then what happens if someone derives from the record and overrides the accessor (or adds a setter)?
So I think the attribute should be left on the accessor.

Relevant spec

@jcouv That makes sense since CompilerGeneratedAttribute is inherited.

https://github.com/dotnet/runtime/blob/1d751ba8563fc099c6a75c41687d2f282a05f916/src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/CompilerGeneratedAttribute.cs#L6

Should I include the attribute on the property if the property isn't virtual (ie, the containing record is sealed)? or just keep it simple and leave on the accessor only?

Always putting it on the accessor only seems fine/simpler with me.

That logic makes sense to me as well.

That logic makes sense to me as well.

@333fred Are you referring to the "always-accessor" or the "depends property virtual-ness" logic?

I have the "depends on property virtual-ness" logic locally. Let me know if I should push the commit or revert to only-accessor approach.

I meant @jcouv's logic.

Youssef1313 · 2022-02-05T19:34:36Z

@333fred @jcouv This should hopefully be ready to merge.

jcouv · 2022-02-07T17:50:18Z

src/Compilers/CSharp/Test/Emit2/Attributes/AttributeTests_Synthesized.cs

+
+                var ctor = record.GetMembers(WellKnownMemberNames.InstanceConstructorName);
+                Assert.Equal(2, ctor.Length);
+                Assert.Equal(1, ctor[0].GetParameters().Length); // copy constructor


nit: you could do Assert.Equal("....", ctor[0].ToTestDisplayString()); instead

@jcouv I addressed the comment in last commit.

jcouv

LGTM Thanks (iteration 10)

jaredpar · 2022-02-07T18:04:48Z

src/Compilers/CSharp/Test/Emit2/Attributes/AttributeTests_Synthesized.cs

@@ -652,6 +652,279 @@ internal class C2<[Attr] T2> { }
            Assert.Equal(new[] { "Attr" }, GetAttributeNames(typeParam.GetAttributes()));
        }

+        [Fact]
+        [WorkItem(46439, "https://github.com/dotnet/roslyn/issues/46439")]
+        public void RecordSynthesizedMembers()


Do we have tests validating the behavior when the CompilerGenerated attribute is missing?

@jaredpar I added the test, but I'm not sure if there is an existing test already somewhere.

There seems to be a legitimate test failure with AttributeTests_Synthesized.AttributeIsMissing

@jcouv I'm not familiar with PEVerify issues/failures. Should I skip verification or there is something else to do?

jcouv

Still LGTM (iteration 11)

jcouv · 2022-02-08T18:05:43Z

@jaredpar for second approval.

jaredpar · 2022-02-08T18:05:52Z

src/Compilers/CSharp/Test/Emit2/Attributes/AttributeTests_Synthesized.cs

@@ -984,7 +984,7 @@ public StringBuilder Append(string s)

 ";
            var comp = CreateEmptyCompilation(source);
-            CompileAndVerify(comp, symbolValidator: validate);
+            CompileAndVerify(comp, symbolValidator: validate, verify: Verification.Skipped);


What was the verification error that was skipped here?

Instead of using an empty compilation, I'd recommend using CreateCompilation then MakeTypeMissing. Then I don't think we'll need to skip PE verification

Yeah that type of PE verification failure is generally a real failure. It makes me worried that we missed a case and are attempting to emit a reference to the attribute when it doesn't exist. Think @jcouv approach is correct here to use MakeTypeMissing that will help rule out that problem.

This worked:

[Fact] [WorkItem(46439, "https://github.com/dotnet/roslyn/issues/46439")] public void AttributeIsMissing() { string source = @" record struct R; "; var comp = CreateCompilation(source); comp.MakeTypeMissing(WellKnownType.System_Runtime_CompilerServices_CompilerGeneratedAttribute); var verifier = CompileAndVerify(comp, symbolValidator: validate); verifier.VerifyDiagnostics(); void validate(ModuleSymbol module) { var record = module.GlobalNamespace.GetTypeMember("R"); Assert.Equal(7, record.GetMembers().Length); // If a new record member is added, extend the test with its behavior regarding CompilerGeneratedAttribute. var ctor = record.GetMember(WellKnownMemberNames.InstanceConstructorName); Assert.Empty(ctor.GetAttributes()); var toString = record.GetMember(WellKnownMemberNames.ObjectToString); Assert.Empty(toString.GetAttributes()); var op_Equality = record.GetMember(WellKnownMemberNames.EqualityOperatorName); Assert.Empty(op_Equality.GetAttributes()); var op_Inequality = record.GetMember(WellKnownMemberNames.InequalityOperatorName); Assert.Empty(op_Inequality.GetAttributes()); var getHashCode = record.GetMember(WellKnownMemberNames.ObjectGetHashCode); Assert.Empty(getHashCode.GetAttributes()); var equals = record.GetMembers(WellKnownMemberNames.ObjectEquals); Assert.Equal(2, equals.Length); Assert.Empty(equals[0].GetAttributes()); Assert.Empty(equals[1].GetAttributes()); } }

Nice! I didn't know MakeTypeMissing is a thing!

kofifus · 2022-05-18T12:03:30Z

Looking in SharpLab this does not fixes the mentioned #46439 and #47613 as the following synthesized methods are still not decorated with [CompilerGenerated]:
The synthesized constructors, ToString, PrintMembers, operator !=, operator ==, GetHashCode, Equals, <Clone>$, Deconstruct

Youssef1313 · 2022-05-18T12:59:38Z

@kofifus Do you have a specific repro? I see the attribute in SharpLab:

kofifus · 2022-05-18T14:20:43Z

Oh I had to switch the branch from Default to Main !
Is that included in latest VS2022 preview ? doesn't seem to but I may have things wrong on my side ..

Youssef1313 · 2022-05-18T14:24:11Z

PR is set to 17.2 Preview 2

kofifus · 2022-05-18T14:49:36Z

Great (I think you mean 17.3)

kofifus · 2022-06-15T09:04:01Z

As far as I can tell this didn't make it to today's release of Version 17.3.0 Preview 2.0

ghost added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Dec 31, 2021

dotnet-issue-labeler bot added the Area-Compilers label Dec 31, 2021

Youssef1313 force-pushed the compiler-generated-attribute-records branch 2 times, most recently from 6ac551e to 7d5955a Compare December 31, 2021 17:14

Add CompilerGeneratedAttribute to record members

a9193c6

Youssef1313 force-pushed the compiler-generated-attribute-records branch from 7d5955a to a9193c6 Compare December 31, 2021 19:29

Youssef1313 marked this pull request as ready for review December 31, 2021 19:36

Youssef1313 requested a review from a team as a code owner December 31, 2021 19:36

Youssef1313 added 2 commits December 31, 2021 22:18

Update AttributeTests_Synthesized.cs

b6a6ddc

Add CompilerGenerated to copy ctor

6d92df1

Youssef1313 commented Jan 2, 2022

View reviewed changes

Add more tests

48ab46b

Youssef1313 commented Jan 2, 2022

View reviewed changes

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Synthesized.cs Outdated Show resolved Hide resolved

Youssef1313 added 2 commits January 2, 2022 09:01

Use MetadataImportOptions.All

f149f32

Skip PEVerify on Desktop builds

6078d4e

Youssef1313 commented Jan 2, 2022

View reviewed changes

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Synthesized.cs Outdated Show resolved Hide resolved

jcouv added the Feature - Records Records label Jan 2, 2022

jcouv self-assigned this Jan 18, 2022

jcouv reviewed Jan 18, 2022

View reviewed changes

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Synthesized.cs Outdated Show resolved Hide resolved

jcouv reviewed Jan 18, 2022

View reviewed changes

src/Compilers/CSharp/Test/Emit/Attributes/AttributeTests_Synthesized.cs Outdated Show resolved Hide resolved

jcouv reviewed Jan 18, 2022

View reviewed changes

Youssef1313 added 2 commits January 18, 2022 21:57

Merge branch 'main' into compiler-generated-attribute-records

305a01a

Address feedback

92d9a16

Youssef1313 requested a review from jcouv January 18, 2022 20:09

jcouv reviewed Jan 18, 2022

View reviewed changes

Update RecordTests.cs

8cdee37

runfoapp bot mentioned this pull request Jan 19, 2022

Failure in CSharpCodeActions.GFUFuzzyMatchAfterRenameTrackingAndAfterGenerateType #57423

Closed

Update per feedback

8fead9c

Youssef1313 requested review from jcouv and 333fred February 5, 2022 15:49

jcouv reviewed Feb 7, 2022

View reviewed changes

jcouv approved these changes Feb 7, 2022

View reviewed changes

jaredpar reviewed Feb 7, 2022

View reviewed changes

Address feedback

fe466b6

jcouv approved these changes Feb 7, 2022

View reviewed changes

skip verification

25bcf6a

jaredpar reviewed Feb 8, 2022

View reviewed changes

Update AttributeTests_Synthesized.cs

ca4c539

jaredpar approved these changes Feb 8, 2022

View reviewed changes

jcouv enabled auto-merge (squash) February 8, 2022 19:33

jcouv merged commit a3e3dc6 into dotnet:main Feb 8, 2022

ghost added this to the Next milestone Feb 8, 2022

Youssef1313 deleted the compiler-generated-attribute-records branch February 8, 2022 21:11

RikkiGibson modified the milestones: Next, 17.2.P2 Mar 1, 2022

Shane32 mentioned this pull request May 18, 2022

Support for CSharp 9 PublicApiGenerator/PublicApiGenerator#207

Open

jcouv mentioned this pull request Jun 15, 2022

record Equals/GetHashCode - use CompilerGenerated attribute #47613

Closed

Youssef1313 mentioned this pull request Jun 17, 2022

Revise CompilerGeneratedAttribute behavior when records are inherited #61991

Closed

kofifus mentioned this pull request Jun 22, 2022

record primary constructor does not have CompilerGenerated attribute #62073

Closed

		var equalityContract = record.GetMember("EqualityContract");
		validateCompilerGeneratedAttribute(equalityContract);

Add CompilerGeneratedAttribute to record members #58542

Add CompilerGeneratedAttribute to record members #58542

Conversation

Youssef1313 commented Dec 31, 2021 • edited Loading

Choose a reason for hiding this comment

Youssef1313 Jan 2, 2022 • edited by jcouv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jan 18, 2022 • edited Loading

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv Jan 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Youssef1313 commented Feb 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

jcouv commented Feb 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kofifus commented May 18, 2022

Youssef1313 commented May 18, 2022

kofifus commented May 18, 2022

Youssef1313 commented May 18, 2022

kofifus commented May 18, 2022

kofifus commented Jun 15, 2022

Youssef1313 commented Dec 31, 2021 •

edited

Loading

Youssef1313 Jan 2, 2022 •

edited by jcouv

Loading

jcouv Jan 18, 2022 •

edited

Loading

jcouv Jan 18, 2022 •

edited

Loading

jcouv Jan 20, 2022 •

edited

Loading

Youssef1313 Jan 21, 2022 •

edited

Loading