Support SkipLocalsInitAttribute in local functions #41183
Conversation
RikkiGibson
force-pushed
the
lfa-skiplocalsinit
branch
5 times, most recently
from
7f7133b
to
5cd9fbf
Compare
| @@ -367,15 +367,6 @@ public override Symbol AssociatedSymbol | |||
| } | |||
| } | |||
|
|
|||
| public override bool AreLocalsZeroed | |||
There was a problem hiding this comment.
Deleted redundant implementation that was inadvertently left around after a merge. The implementation was already in SourceMethodSymbolWithAttributes. #ByDesign
RikkiGibson
force-pushed
the
lfa-skiplocalsinit
branch
from
5cd9fbf
to
cf0f6ff
Compare
| @@ -345,13 +345,12 @@ internal sealed override CSharpAttributeData EarlyDecodeWellKnownAttribute(ref E | |||
| return base.EarlyDecodeWellKnownAttribute(ref arguments); | |||
| } | |||
|
|
|||
| // PROTOTYPE(local-function-attributes): test local function with SkipLocalsInit | |||
| public override bool AreLocalsZeroed | |||
There was a problem hiding this comment.
override [](start = 15, length = 8)
Can we seal this implementation? #Closed
There was a problem hiding this comment.
I think we need to override it in SynthesizedMethodBaseSymbol.
In reply to: 371410335 [](ancestors = 371410335)
There was a problem hiding this comment.
I think we need to override it in SynthesizedMethodBaseSymbol.
SynthesizedMethodBaseSymbol inherits from SourceMemberMethodSymbol
In reply to: 371421097 [](ancestors = 371421097,371410335)
| int y = 4; | ||
| y = y + y + y; | ||
|
|
||
| void LF() |
There was a problem hiding this comment.
void LF() [](start = 12, length = 9)
It looks like we are missing a positive test for a scenario when a local function is nested into a lambda. #Closed
| @@ -33,6 +33,27 @@ protected static void ReportBadRefToken(TypeSyntax returnTypeSyntax, DiagnosticB | |||
| } | |||
| } | |||
|
|
|||
| protected bool AreContainingSymbolLocalsZeroed | |||
There was a problem hiding this comment.
You could do
protected bool AreContainingSymbolLocalsZeroed => ContainingSymbol switch
{
SourceMethodSymbol m => m.AreLocalsZeroed,
SourceNamedTypeSymbol t => t.AreLocalsZeroed,
_ => true
};#Resolved
There was a problem hiding this comment.
Does this handle the scenario where the ContainingSymbol is a SourceFieldSymbolFromDeclarator and the containing type has SkipLocalsInitAttribute? #Resolved
There was a problem hiding this comment.
I think when I was triaging this I was surprised to find this as the ContainingSymbol for some scenario or other, but I tried to create a test that would create a bad behavior here and couldn't do it, so I'm good with taking your suggestion. #Resolved
There was a problem hiding this comment.
Aleksey came up with a test case involving a field initializer containing a lambda, where the ContainingSymbol is a FieldSymbol and we have to instead look up to the ContainingType. So I reverted the suggestion. #Closed
There was a problem hiding this comment.
Were you thinking of a lambda inside a field initializer? #Resolved
There was a problem hiding this comment.
I could see that as being a reasonable scenario that would be broken with my code. #Resolved
| public void M() | ||
| { | ||
| [System.Runtime.CompilerServices.SkipLocalsInitAttribute] | ||
| void F() |
There was a problem hiding this comment.
void F() [](start = 8, length = 8)
I think we should also test scenarios when the attribute is applied to a containing member (including events/properties if allowed), or containing type, or containing module. #Closed
There was a problem hiding this comment.
Some of these scenarios already had tests, such as SkipLocalsInitAttributeOnMethodPropagatesToLocalFunction, but I added tests for the other scenarios you mentioned.
In reply to: 371418225 [](ancestors = 371418225)
|
Done with review pass (iteration 1) #Closed |
|
@dotnet/roslyn-compiler Please review #Closed |
| @@ -10789,6 +11284,45 @@ static int M(out int x) | |||
| Assert.False(verifier.HasLocalsInit("C..ctor")); | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void SkipLocalsInitFieldInitializer() | |||
There was a problem hiding this comment.
SkipLocalsInitFieldInitializer [](start = 20, length = 30)
It doesn't look like this test actually tests anything in field initializer. However, I think this is an interesting scenario to test. We could have a lambda with a local function within a field initializer. We also could have two constructors: one with an attribute and one without. It feels like the attributes on the constructors shouldn't have any effect on the lambda and the local function. But the attribute on the type/module should have an effect. Please add a test like this. #Resolved
There was a problem hiding this comment.
What's a more appropriate term for this test? "ImplicitConstructor"?
In reply to: 371973916 [](ancestors = 371973916)
There was a problem hiding this comment.
What's a more appropriate term for this test? "ImplicitConstructor"?
Yes, I think this term will describe the purpose of the test more accurately.
In reply to: 371989150 [](ancestors = 371989150,371973916)
Related to #38801