VB: Fix state tracking in AbstractFlowPass.VisitBinaryConditionalExpression - #84705
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR updates Visual Basic flow analysis so the state after a binary conditional If(testExpr, elseExpr) correctly accounts for the possible execution of elseExpr (instead of discarding its effects), addressing a mis-tracking scenario that could lead to incorrect async state-machine spilling/corruption. It also adds a regression test that validates both runtime output and key IL shapes in Debug and Release builds.
Changes:
- Update
AbstractFlowPass.VisitBinaryConditionalExpressionto merge (IntersectWith) the post-ElseExpressionstate with the pre-visit saved state. - Add a regression test covering
If(GetNothingObj(), Await GetObjAsync())and verifying output + IL in both Debug and Release configurations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Compilers/VisualBasic/Portable/Analysis/FlowAnalysis/AbstractFlowPass.vb | Fixes flow-state merging for binary conditional expressions by intersecting the else-visited state with the saved pre-else state. |
| src/Compilers/VisualBasic/Test/Emit/CodeGen/CodeGenAsyncTests.vb | Adds a regression test ensuring locals aren’t corrupted across If(,) with Await in the second operand; verifies output and IL for Debug/Release. |
|
@dotnet/roslyn-compiler Please review |
| Dim savedState As LocalState = Me.State.Clone() | ||
| VisitRvalue(node.ElseExpression) | ||
| Me.SetState(savedState) | ||
| IntersectWith(Me.State, savedState) |
There was a problem hiding this comment.
When the TestExpression is constant, should we continue to ignore side-effects of ElseExpression?
There was a problem hiding this comment.
When the
TestExpressionis constant, should we continue to ignore side-effects ofElseExpression?
Where do we do that "ignore side-effects of ElseExpression"?
There was a problem hiding this comment.
Sorry, that was imprecise. I simply mean that we would be discarding state of the ElseExpression previously.
So for example something like
Dim a As Object = New Object()
Dim result = If("left", Await FallbackAsync())
Console.WriteLine(a Is Nothing)would previously not need to hoist a local in Release mode. Now it presumably will.
There was a problem hiding this comment.
Now it presumably will.
Perhaps that is inefficient, but correct. Optimizing handling of non-Nothing constants is outside of the scope for this PR. Given that the scenario is rather pathological, I doubt we would ever do this work without a good customer case.
|
@jjonescz, @dotnet/roslyn-compiler Please review |
Fixes #84517
Microsoft Reviewers: Open in CodeFlow