Fix #4059: treat a Deconstruct out argument as a definition - #4114
Draft
siegfriedpammer wants to merge 1 commit into
Draft
Fix #4059: treat a Deconstruct out argument as a definition#4114siegfriedpammer wants to merge 1 commit into
siegfriedpammer wants to merge 1 commit into
Conversation
The reaching-definitions analysis does not track address loads, so a variable that is only ever written through its address stays potentially uninitialized for its whole life. SplitVariables merges every such load into one live range, which is why csc lowering two deconstructions onto the same pair of temporaries left neither of them recognizable. IL cannot express that a method assigns through an address without reading it first - 'out' is a C# convention over 'ref' - so the address loads that qualify have to be named. A Deconstruct method is the one case that matters here, and C#'s definite assignment rules rule out a read; a Deconstruct method written in IL could read the argument, and that possibility is knowingly ignored. Assisted-by: Claude:claude-opus-5:Claude Code
Member
|
This needs proper investigation of how it interacts with the derived class (SplitVariables.GroupStores) -- and especially the partial support for ref locals that has. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4059.
Two deconstructions of the same source type in one method are not recognized: the first one alone
decompiles correctly, but as soon as a second follows, both fall back to raw
Deconstructcalls,because csc reuses the same two out-slot temporaries and hands them to the second call in the
opposite order.
Why splitting the temporaries did not happen
ReachingDefinitionsVisitordoes not track address loads, so a variable that is only ever writtenthrough its address - which is every
outslot - stays potentially uninitialized for its wholelife.
SplitVariables.GroupStoresmerges all such loads of one variable into a singlerepresentative, so the two deconstructions end up in one live range and neither can be matched.
The same holds for
outarguments in general; it only becomes visible forDeconstruct, wherepattern matching depends on the split.
The fix
An
ldlocain argument position > 0 of aDeconstructcall is treated as a definition: it killsthe definitions reaching it and becomes the one that reaches the following loads. That reuses the
existing store machinery, so it stays flow-sensitive - where two such calls join, both reach the
load and are merged again, which a flow-insensitive rule would get wrong.
IL cannot express an address that is assigned without being read first;
outis a C# conventionover
ref. So the qualifying address loads have to be named one by one, and aDeconstructmethod written in IL could read its argument before assigning it. That possibility is knowingly
ignored - C#'s definite assignment rules rule it out for anything a compiler emits.
SplitVariablesitself is unchanged: itsHandleLoadon such anldlocanow runs against thepost-store state and merges the instruction with itself.
Tests
The
TwoBackToBackDeconstructs_Customcase that #4059 was filed from is checked in commented out;this enables it. It fails on all eight compiler configurations before the change and passes after.
ICSharpCode.Decompiler.Tests: 3604 tests, no failures, 45 skipped (Windows-only configurations).Prepared by an AI agent (Claude, claude-opus-5, via Claude Code) and reviewed by @siegfriedpammer.