When a synthetic ref local is captured, variables referenced in its initializer must be captured too#38196
Conversation
333fred
left a comment
There was a problem hiding this comment.
This still repros locally in both master and release/dev16.4-preview1 when I run csc.exe with /target:library /debug- /optimize+ test.cs, copying and pasting from my original repro. When I build csc.exe in debug mode itself, it hits the assertion in the UnexpectedValue method, and in release mode it just hits the windows error reporting.
…nitializer must be captured too.
| else | ||
| { | ||
| _variablesToHoist.Add(variable); | ||
| if (_variablesToHoist.Add(variable) && variable is LocalSymbol local && _boundRefLocalInitializers.TryGetValue(local, out var variableInitializer)) |
There was a problem hiding this comment.
_boundRefLocalInitializers [](start = 88, length = 26)
I didn't understand: where do we add values into Never mind. Found it #Closed_boundRefLocalInitializers?
| case BoundParameter { ParameterSymbol: var symbol }: | ||
| CaptureVariable(symbol, syntax); | ||
| break; | ||
| case BoundFieldAccess { FieldSymbol: { IsStatic: false, ContainingType: var containingType }, ReceiverOpt: { } receiver } when containingType.IsValueType: |
There was a problem hiding this comment.
{ } [](start = 123, length = 3)
nit: should probably use type name here #Pending
| case BoundLocal { LocalSymbol: var symbol }: | ||
| CaptureVariable(symbol, syntax); | ||
| break; | ||
| case BoundParameter { ParameterSymbol: var symbol }: |
There was a problem hiding this comment.
BoundParameter [](start = 21, length = 14)
do we have a test for the parameter case? #Pending
There was a problem hiding this comment.
Not one that would crash currently (without the fix), as I believe we always capture parameters that are used in any way. That's because the state machine starts in a "cold" state, so hoisting the parameter is necessary for it to reach its use site. So there is no way to construct an observable crash in the current compiler.
Though, hang on, I guess I could assign to the parameter before its use to repro the issue. I will attempt to construct a case that crashes currently and exercises this to fix it.
In reply to: 318840878 [](ancestors = 318840878)
There was a problem hiding this comment.
No, we hoist any parameter that is written to as well. I will add a test in any case so if we stop hoisting parameters just because they are written then this won't be a problem.
In reply to: 319306652 [](ancestors = 319306652,318840878)
333fred
left a comment
There was a problem hiding this comment.
LGTM (iteration 2), but could use a test case for the parameter as Julien mentioned.
Fixes #36443