Follow-up to "Local function attributes emit"#39830
Conversation
| return _underlyingTypeParameter.GetAttributes(); | ||
| } | ||
|
|
||
| return ImmutableArray<CSharpAttributeData>.Empty; |
There was a problem hiding this comment.
return ImmutableArray.Empty; [](start = 12, length = 49)
This doesn't feel right. A type parameter of a generic type nested into a constructed type should return attributes from the original definition of the type parameter (please add a test). I think this logic should be moved to SynthesizedSubstitutedTypeParameterSymbol. Please check if VB has the same problem around emitting attributes when not supposed to. #Closed
There was a problem hiding this comment.
I ended up moving the GetAttributes() to the WrappedTypeParameterSymbol, it seems to better resemble what WrappedParameterSymbol does, and it felt like a similar default behavior was appropriate.
I added the test SubstitutedTypeParameter_Attributes which demonstrates that we carry over the type parameter attributes from the declaration after constructing the containing type.
I added a VB test which shows that the base method and return attributes are not inherited on a synthesized method. Type parameter attributes seem a little trickier because type parameter attributes don't seem to be available in VB. I could maybe try having the VB program override a method with a type parameter attribute which is defined somewhere else.
In reply to: 346603239 [](ancestors = 346603239)
There was a problem hiding this comment.
I could maybe try having the VB program override a method with a type parameter attribute which is defined somewhere else.
Yes please define the base class in C#. We have a helper called CreateCSharpCompilation (or similar) that VB tests use for scenarios like this. It should be possible to find examples.
In reply to: 346624109 [](ancestors = 346624109,346603239)
| [Theory] | ||
| [MemberData(nameof(OptimizationLevelTheoryData))] | ||
| [WorkItem(38801, "https://github.com/dotnet/roslyn/issues/38801")] | ||
| public void BaseMethodWrapper_DoNotInheritAttributes_TypeParameter(OptimizationLevel optimizationLevel) |
There was a problem hiding this comment.
BaseMethodWrapper_DoNotInheritAttributes_TypeParameter [](start = 20, length = 54)
Is this test failing without the change in SubstitutedTypeParameterSymbol? #Closed
|
Done with review pass (iteration 5). #Closed |
|
Done with review pass (iteration 7) #Closed |
| {"CompilerGeneratedAttribute", "CompilerGeneratedAttribute", "DebuggerHiddenAttribute"}) | ||
|
|
||
| AssertEx.SetEqual(expectedNames, GetAttributeNames(baseWrapper.GetAttributes())) | ||
| AssertEx.SetEqual({}, GetAttributeNames(baseWrapper.GetReturnTypeAttributes())) |
There was a problem hiding this comment.
SetEqual [](start = 65, length = 8)
Consider using Assert.Empty here and in the test below. #Closed
There was a problem hiding this comment.
And there is no reason to call GetAttributeNames when we expect no attributes.
In reply to: 347041733 [](ancestors = 347041733)
3 participants
Follow up to #39226
I merged before all of @AlekseyTs's comments were addressed. This PR is meant to address those comments.
I think emitting local function type parameters is actually working at least in some scenarios:
roslyn/src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenLocalFunctionTests.cs
Line 5212 in 6af5960
I'll take another look over the tests and try to remember where I thought the gap was. (Edit: it was that all synthesized methods were inheriting type parameter attributes from their base method. This PR corrects that so the type parameter attributes are only inherited when expected.)