Add CompilerGeneratedAttribute to EqualityContract property#61993
Add CompilerGeneratedAttribute to EqualityContract property#61993jcouv merged 8 commits intodotnet:mainfrom
CompilerGeneratedAttribute to EqualityContract property#61993Conversation
…r to the property itself
|
I am not sure why this PR is supposed to close #61991. It looks like that issue discusses completely different scenario and, perhaps, should be closed "By Design" on its own. It is quite possible that the decision somehow contributes to a decision to make the change presented here, but they are still not the same. |
|
@Youssef1313 It looks like there are legitimate test failures |
| { | ||
| } | ||
|
|
||
| internal override void AddSynthesizedAttributes(PEModuleBuilder moduleBuilder, ref ArrayBuilder<SynthesizedAttributeData> attributes) |
There was a problem hiding this comment.
There shouldn't be any harm, it's just redundant. If the property is compiler-generated, then for sure the accessors are.
From the old PR:
This is also consistent with what's done in other compiler-generated symbols. For example:
Here, a synthesized field will not have the attribute if the containing type already has it.
So the attribute is usually put on the outer most compiler-generated symbol.
There was a problem hiding this comment.
If the property is compiler-generated, then for sure the accessors are.
I don't think this is a safe assumption in general. For C# compiler, sure. But in metadata accessors and properties are distinct and each has its own set of attributes. I can easily imagine a synthesized property that uses user-defined methods as accessors.
do not emit CompilerGenerated attributes for fields inside compiler generated types
This isn't the same though, I think. In the type system (and I am not talking about C# exclusively), a field is contained by a type, it cannot be defined if there is no type. So, it is relatively safe to assume that a user cannot define a field in a compiler generated type. Accessors, however, are not contained by a property (despite what C# syntax might lead you to believe), properties and accessors are siblings, they are just linked in a special way.
There was a problem hiding this comment.
Yeah I see that from the metadata perspective. I'll add the attribute on both the property and the accessors.
|
I think we need a clear explanation why do we want to make this change. It is quite possible I missed something, but I did not find it in the referenced issue. |
|
Done with review pass (commit 2) |
Yeah the original issue can just be closed as by-design. It's just the conversation there led to revising where |
CompilerGeneratedAttribute from EqualityContract get accessor to the property itselfCompilerGeneratedAttribute to EqualityContract property
|
Done with review pass (commit 4). It looks like there are legitimate test failures. |
|
Done with review pass (commit 6). It looks like there are still tests that require a baseline adjustment. |
|
@jcouv, @dotnet/roslyn-compiler For the second review. |
Closes #61991