Fix #3803: Crash on nested deconstruction of custom structs

[siegfriedpammer]

Fixes #3803. Fixes #3388.

Nested deconstruction (var (x, (a, b)) = o;, also the foreach form) crashed the decompiler — Debug builds assert in DeconstructInstruction.CheckInvariant, Release builds throw the reported ArgumentOutOfRangeException. This hits real-world assemblies, e.g. Microsoft.CodeAnalysis.LanguageServer.Protocol.dll shipped with the VS Code C# extension.

Root cause. In optimized code a deconstruction element that is used only once after the deconstruction is not stored to a temporary. DeconstructionTransform.MatchAssignments compensated for that, but only for trailing elements. A nested deconstruction copies the inner element to a temporary (the defensive copy for the inner Deconstruct receiver), which counts as the only matched assignment — leaving the leading elements without one, with their single load outside the new DeconstructInstruction. That violates the instruction's invariant that all pattern-variable loads are descendants of it, and the statement/expression builders crash on the malformed pattern.

Fix. The forwarding fixup now covers all unassigned elements (guarded by an IsDescendantOf check), and inserts the forwarding assignments in pattern order, since StatementBuilder.TranslateDeconstructionDesignation and ExpressionBuilder.ConstructTuple pair pattern variables with assignments positionally.

Second bug (pre-existing, surfaced by the fixture). Deconstructing a struct with a custom Deconstruct passes the receiver by reference; VisitDeconstructInstruction emitted the address operand as an invalid cast — var (x, y) = (S)(ref s); — on released 10.1.1 even without nesting. Now unwrapped like TranslateTarget does for member access targets.

Output shape note. The inner deconstruction currently decompiles as an explicit inner.Deconstruct(out var a, out var b); call rather than nested designation sugar — DeconstructionTransform never builds nested sub-patterns (the IL node and the statement/expression builders already support them, so reconstructing the sugar is a self-contained future feature in the transform).

Tests. Nested assignment, foreach, and the #3388 KeyValuePair-with-discard shapes added to the Correctness fixture: red before (all compiler-matrix variants crash at decompile), green after, with runtime output compared against the original — so element evaluation order is verified, not just compilability. Full decompiler suite: 3332 tests, 0 failures.

🤖 Generated with Claude Code

[christophwille]

Code review

What this does

Two independent fixes plus regression coverage:

1. DeconstructionTransform.MatchAssignments - the "element used once, never stored to a temporary" fixup only ran for trailing elements (i = previousIndex + 1 ..). A nested deconstruction stores the inner element to a temporary (the defensive copy for the inner Deconstruct receiver), which consumes the match, so leading elements were left with their single load outside the new DeconstructInstruction - violating the invariant that DeconstructInstruction.CheckInvariant -> ValidatePattern asserts. The fixup now covers every unassigned element, guarded by IsDescendantOf, and inserts in pattern order.
2. ExpressionBuilder.VisitDeconstructInstruction - the tested operand's address was emitted as (S)(ref s). Now unwrapped, consistent with the existing DirectionExpression unwrap sites in TranslateTarget / the LdObj path (which also match on the syntax shape alone, so the new site follows precedent).

Verification

Beyond reading the diff, I reproduced the before/after independently (macOS, net11.0, Debug):

• Copied the new fixture onto the parent commit (6d213669a) and rebuilt: 6 DeconstructionTests variants fail there with Debug.Fail in ValidatePattern at LoadInstructions[0].IsDescendantOf(this), plus DecompilerException: Error decompiling ... NestedDeconstruction_DiscardedElement. The fixture is genuinely red before the fix, not just an addition.
• The same variants pass on the branch.
• Full suite run on both trees with the failure sets diffed: 0 new failures, 6 fixed - exactly the crashing variants. (The remaining failures are macOS environment limits - net40 targets, legacy Roslyn reference assemblies, Windows-only ILAsm - and are identical on both sides.)

Ordering logic checks out: a forwarding assignment is itself stloc fresh(ldloc v) with v in deconstructionResultsLookup, so it resolves through the first branch of GetAssignmentIndex and later insertions position correctly against earlier ones and against the AddMissingAssignmentsForConversions fixups. Inserting in pattern order also incidentally corrects a latent mis-ordering the old append-at-end path could produce when a conversion-only element preceded a forwarded trailing element.

The delayed-action chain is safe: the forwarding actions are appended last in MatchAssignments, so by the time they run, addAssignment / tuple-access / conversion fixups have already reparented their instructions into deconstructInst.Assignments, which is what makes the IsDescendantOf guard meaningful. Element values are stable (StoreCount == 0), so moving a load earlier in time cannot change what it reads.

Findings

All minor - nothing blocking. Comments left at the relevant lines.

1. DeconstructionTransform.cs:410 - the TryGetValue miss returns silently after the transform has been committed, so it cannot abort; it would re-create exactly the malformed instruction this PR fixes. Currently unreachable, so an assert would document that better than a silent return.
2. DeconstructionTransform.cs:404 / :425 - tuple-element variables are never registered in deconstructionResultsLookup, so GetAssignmentIndex returns int.MaxValue for every assignment of a tuple deconstruction. Harmless only because the eager LoadCount check keeps forwarding from firing on that path - an undocumented coupling worth a line of comment.
3. Correctness/DeconstructionTests.cs:202 - the ExpressionBuilder fix has no coverage independent of the nesting fix, and no Pretty test pins the emitted shape.

Conventions

Commit message, Assisted-by: trailer, ASCII-only, en-US, no new files needing license headers, comments self-contained - all fine. The build is warning-clean.

[christophwille]

Minor: this bail-out cannot actually protect anything. Delayed actions run from TransformDeconstruction after context.Step, once replacement is already built and about to be written into the block - so returning here leaves the pattern variable's load outside the DeconstructInstruction, i.e. exactly the malformed shape this PR fixes, just without the transform having declined it.

In practice it is unreachable: MatchDeconstruction registers every element of deconstructionResults in the lookup before returning, and the tuple path is filtered out earlier by the LoadCount check. Given that, a Debug.Assert (or dropping the guard entirely) states the invariant instead of quietly swallowing a violation of it.

[christophwille]

Worth a line of comment here, because two non-obvious things have to line up for the tuple case to stay correct.

LoadCount is read eagerly, at match time, while the decision is applied later from the delayed action. For a tuple deconstruction the elements are the fresh E_i variables created in FindIndex, and their loads only come into existence when the delayed inst.ReplaceWith(new LdLoc(...)) runs - so LoadCount is still 0 here and forwarding never fires on that path. Same as before this PR, so no behaviour change.

That matters because E_i is never added to deconstructionResultsLookup (only MatchDeconstruction and MatchConversions populate it). If forwarding ever did fire for a tuple, GetAssignmentIndex would return int.MaxValue for every assignment in the block, insertPos would stay 0, and the forwarding assignment would land at the front - mispairing designators with assignments, since both builders consume them positionally. So the eager read is load-bearing, not incidental, and moving the check into the delayed action (where it reads more naturally) would silently break tuples.

[christophwille]

Coverage suggestion: this is currently the only thing exercising the ExpressionBuilder half of the PR, and only as a side effect of the nested case. A plain non-nested var (a, b) = someStruct; would pin that fix on its own - useful since the two fixes are independent and the struct one reproduces without nesting.

It would also explain why the bug survived this long. DeconstructionSource in both fixtures is a class, and every assignment-form deconstruction in Pretty/DeconstructionTests.cs goes through it or through tuples - so nothing took the address of a struct receiver. DeconstructDictionaryForEach does deconstruct a KeyValuePair, but the foreach shape is built by StatementBuilder.TranslateDeconstructionDesignation, which never translates Pattern.TestedOperand and so never reaches VisitDeconstructInstruction.

A Pretty case would additionally lock in the emitted shape (including the inner.Deconstruct(out var a, out var b); form noted in the description), so a future change in the transform that starts reconstructing nested designations shows up as an expected-output diff rather than silently.

[siegfriedpammer]

Drop this comment completely