Reconstruct nested deconstruction designations

[siegfriedpammer]

Builds on #3949 (merged), rebased onto current master.

After #3949, nested deconstruction decompiled without crashing but unsugared: var (x, (a, b)) = o; came out as a flat deconstruction followed by an explicit inner.Deconstruct(out var a, out var b); call. The IL pattern node (MatchInstruction with IsDeconstructCall sub-patterns), its invariants, and the statement/expression builders all already support nested patterns — only DeconstructionTransform never built them.

Change (two commits — the chain-matching mechanism, then the rule changes that let chains nest): in DeconstructionTransform, MatchDeconstruction now consumes chained Deconstruct calls — including the defensive copy Roslyn emits for struct elements — into nested match patterns, recursively. Leaves get flat indices in depth-first order, which is exactly the order StatementBuilder/ExpressionBuilder pair assignments with designators, so conversion/assignment matching runs unchanged on top. Three matching-rule adjustments follow from the chain being consumed: a call pattern no longer needs a matched assignment (single-use leaves are covered by the existing forwarding fixup); an unrelated assignment ends a call pattern instead of rejecting it (tuple patterns still reject — their element list is discovered from the assignments, ending early would misread a suffix); and a pattern is not rooted on an element of an enclosing deconstruction (blocks are processed back to front — the inner call is visited first and would otherwise sugar piecemeal, starving the outer call).

The unrelated-assignment rule is keyed on the pattern being call-rooted, not on nesting, so it also fixes flat custom deconstructions: var (a, b) = GetSource(...); followed by any unrelated assignment previously decompiled as an explicit Deconstruct call and now sugars (pinned by the LocalVariable_NoConversion_Custom_UnrelatedAssignmentAfter fixture).

One production change lands outside the transform, as its own commit carrying its two pointer-target Pretty fixtures: DeconstructInstruction.IsAssignment falls back to stobj.Type when a pointer target was materialized through a stack slot (whose type erases to unknown during target evaluation). This independently enables deconstruction into pointer targets — (*p, value) = tuple; — including plain tuples with no nesting at all.

Nested tuples too: the second half of the series extends the nesting to pure tuple chains (fixtures, then the temporary-consuming mechanism, then the deferral guard) — var (x, (a, b)) = GetTuple<int, (int, int)>();, which Roslyn lowers to one temporary per nested designation (parents before children) plus element reads in depth-first leaf order, previously decompiled as a flat deconstruction plus separate element statements. The matcher now consumes the temporaries into a tuple-node tree with flat depth-first leaf indices (the same design as the call nesting). An element variable that escapes the deconstruction (used after the statement) demotes back to a designator leaf via a blacklist-and-retry, and a dry-run guard — the tuple analogue of the call path's — defers inner flat matches to the enclosing attempt. Two IL realities drove the matcher details: earlier transforms rewrite non-escaping element reads from ldloca to ldloc, and stack-slot temporaries carry imprecise types (the container's element type is authoritative; the match variable is retyped so the IsDeconstructTuple invariant holds). One production change again lands outside the transform, in its own commit: TupleType.FromUnderlyingType threw NullReferenceException on non-tuple input instead of returning null as documented.

Scope: call-under-call and tuple-in-tuple nesting, any depth, assignment + foreach forms, discards, conversions on leaves. Mixed containers (a call pattern absorbing tuple elements, or a custom-deconstructed element inside a tuple pattern) intentionally stay flat, as do by-ref extension receivers (Deconstruct(this in S, ...)).

Tests: each feature's fixtures land ahead of its implementation and are red until it. For the call nesting: 20 Pretty fixtures (the flat unrelated-assignment form, struct/class inners, both-elements-nested, depth 3, inner discard, nested/typed/nullable conversions, System.Tuple sources, the two bail-out shapes ElementDeconstructedAfterBarrier/OuterElementUsedTwice, two foreach forms incl. KeyValuePair extension Deconstruct; the two pointer-target forms ride in the pointer commit) and 18 Correctness cases with printed Deconstruct calls pinning evaluation order at runtime, including member hiding (NestedDeconstruction_HiddenDeconstructMethod pins the BindsOnElementType gate), side-effecting LHS targets, checked/nullable conversions, in-parameter, conditional and generic-constraint sources, and the tuple-with-custom-element corruption repro. For the tuple nesting: 6 Pretty fixtures (depth 2 and 3, both elements nested, conversions on inner leaves, the escaping-element case pinned per config, foreach) and 5 Correctness cases with runtime-value pins. Every commit in the series builds, and at every point the only failing tests are the not-yet-implemented spec fixtures (verified per intermediate). Full decompiler suite at the head: 3332 tests, 0 failures.

Output polish for #3803/#3388-adjacent cases; closes nothing by itself.

🤖 Generated with Claude Code

[christophwille]

Code review

What this does

DeconstructionTransform now reconstructs nested designations. MatchDeconstruction consumes chained Deconstruct calls (including Roslyn's defensive copy for struct elements) into a recursive DeconstructionCall tree, BuildPatternMatch turns that into nested MatchInstruction sub-patterns, and leaves get flat depth-first indices so the existing conversion/assignment matching keeps working unchanged. Three matching rules relax as a consequence, plus IsConsumableByEnclosingDeconstruction defers an inner call to its enclosing one (blocks are walked back to front, so the inner call is reached first).

The IL-side design is sound: depth-first leaf indexing is exactly the order ConstructTuple / ConstructDesignation consume assignments; nested receiver variables end up with LoadCount == SubPatterns.Count, so MatchInstruction.HasDesignator stays false and ValidatePattern is satisfied; and DeconstructResultInstruction correctly keeps per-call indices while the lookup keeps flat ones - two separate index spaces that never mix.

Verification

Beyond reading the diff (macOS, net11.0, Debug):

• TDD claim holds. Built the test-only commit (f8694ae63) on its own: 6 DeconstructionTests variants fail there, all 6 green on the implementation commit. Red-then-green, as described.
• No regressions. Full suite on f96283358 and on the base 555e364d8, failure sets diffed: identical, zero new failures, on 3332 tests. (The environmental failures are macOS-only - net40 targets, legacy Roslyn reference assemblies, Windows ILAsm - and match on both sides.)
• Riskiest new path checked. The relaxed "unrelated assignment ends the pattern" rule can leave a later element without an assignment, so the Fix #3803: Crash on nested deconstruction of custom structs #3949 forwarding fixup pulls its load into the deconstruct instruction. Fed the transform an interleaved case (x = a; z = GetInt(); y = b;) and the output is correct - the element is forwarded through a fresh variable and statement order is preserved.
• The IsAssignment hunk is load-bearing. Reverted it in isolation: the 6 runnable variants fail, with Pointer_NoConversion_Tuple and Pointer_Nested_Custom regressing to unsugared form.

Test coverage is considerably better than the description advertises ("9 Pretty fixtures ... plus class-inner and depth-3 Correctness cases"): there are ~18 of each, including member hiding pinning BindsOnElementType, evaluation-order pinning with side-effecting LHS targets, checked/nullable conversions, in parameter and conditional sources, and the two bail-out paths (ElementDeconstructedAfterBarrier, OuterElementUsedTwice). Worth updating the description - the extra cases are the strongest argument for the PR.

Findings

Nothing blocking; no correctness defect found. Comments left at the relevant lines.

1. DeconstructionTransform.cs:591 - the biggest user-visible win in this PR is untested and unadvertised. allowUnrelatedAssignments is keyed on rootCall != null, not on nesting, so it also fixes flat deconstructions. Measured on the base branch: var (a, b) = GetSource<MyInt?, MyInt>(); int v = GetInt(); decompiles as an explicit Deconstruct call today, and sugars correctly with this PR. Every fixture for the rule is nested, so a later narrowing would silently regress the flat form.
2. DeconstructInstruction.cs:266 - second production change, outside the stated scope. The description says the change is "all in DeconstructionTransform". This hunk independently enables deconstruction into pointer targets, including for plain tuples with no nesting at all.
3. DeconstructionTransform.cs:345 - the bail-out comment overstates what the dry run proves. It establishes that the chain is consumable, not that the enclosing attempt will succeed; MatchConversions/MatchAssignments can still reject there, and the back-to-front walk gives no second chance. I could not construct a case that actually loses sugar, so this is about the comment, not a defect.

Minor: MatchDeconstruction (line 311) returns bool but the result is discarded at its only call site, which tests rootCall != null instead - void would read more honestly.

Conventions

Commit messages, Assisted-by: trailer, ASCII-only, en-US, self-contained comments, no new files needing license headers, warning-clean build. The two-commit split (spec, then implementation) matches the repo's TDD rule and genuinely verifies.

[Copilot]

Pull request overview

This PR enhances ILSpy’s decompiler pipeline by reconstructing nested deconstruction syntax (both custom Deconstruct-call chains and nested tuple designations) into a single structured DeconstructInstruction, allowing the C# statement/expression builders to emit sugared nested patterns like var (x, (a, b)) = ...; instead of flattening into explicit follow-up calls/element statements.

Changes:

• Extend DeconstructionTransform to consume chained/nested Deconstruct calls and nested tuple-designation temporaries into recursive match patterns with depth-first leaf indexing.
• Improve pointer-target deconstruction detection by fixing DeconstructInstruction.IsAssignment expected-type inference for stobj into pointer targets (including stack-slot-erased pointer types).
• Fix TupleType.FromUnderlyingType to avoid NullReferenceException on non-tuple input by correctly handling default/empty tuple element arrays.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
ICSharpCode.Decompiler/TypeSystem/TupleType.cs	Prevents NRE in FromUnderlyingType by using IsDefaultOrEmpty on tuple element arrays.
ICSharpCode.Decompiler/IL/Transforms/DeconstructionTransform.cs	Core change: matches and builds nested deconstruction patterns (call chains + tuple nesting) and integrates conversions/assignments.
ICSharpCode.Decompiler/IL/Instructions/DeconstructInstruction.cs	Improves assignment recognition for pointer-target stobj by falling back to stobj.Type.
ICSharpCode.Decompiler.Tests/TestCases/Pretty/DeconstructionTests.cs	Adds Pretty fixtures covering nested deconstruction shapes, tuple nesting, unrelated-assignment termination, and pointer targets.
ICSharpCode.Decompiler.Tests/TestCases/Correctness/DeconstructionTests.cs	Adds runtime-pinned correctness cases validating evaluation order and nested tuple/custom deconstruction behavior.

Suppressed comments (2)

ICSharpCode.Decompiler/IL/Transforms/DeconstructionTransform.cs:602

• ElementAtOrDefault(pos) on block.Instructions will enumerate via LINQ. A simple bounds check with indexing avoids the overhead and keeps this matching loop allocation-free.

			while (MatchTupleElementStore(block.Instructions.ElementAtOrDefault(pos),
				out var temp, out var container, out var containerType, out int index))

ICSharpCode.Decompiler/IL/Transforms/DeconstructionTransform.cs:782

• MatchAssignments uses ElementAtOrDefault(pos) in a loop; that LINQ call can be avoided by directly indexing with a bounds check.

			while (MatchAssignment(block.Instructions.ElementAtOrDefault(pos), out var targetType, out var valueInst, out var addAssignment))
			{

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[christophwille]

Correctness (confirmed): IsAssignment's stobj fallback replaces the UnknownType sentinel that rejected stores whose target type cannot be inferred as ByReferenceType. StObj.Type comes from the IL store opcode and is sign-agnostic relative to the true pointer element type, so a sign-mismatched pointer store now passes the implicit-conversion check that previously rejected the transform.

Failure scenario: For var (el, y) = t; *p = (uint)el; with (int, int) t and uint* p, the int-to-uint cast is a no-op in IL (stind.i4, so StObj.Type = int32). InferType of the pointer target yields PointerType, not ByReferenceType, so expectedType becomes int32 and IsCompatibleImplicitConversion sees an identity conversion. The transform sugars this into (*p, y) = t;, which does not recompile (CS0266: cannot implicitly convert int to uint). On master, expectedType was UnknownType, the conversion check failed, and correct unsugared code was emitted.

Found by high-effort multi-agent review; independently verified.

[christophwille]

Correctness (confirmed): BindsOnElementType only guards against instance-method hiding; it returns true (method.IsStatic) for any extension-method Deconstruct as long as no instance method of the same arity exists on the element's type chain. It therefore does not detect a more-specific competing extension method that a recompiled nested designation would bind to instead of the one the IL actually calls.

Failure scenario: IL calls extension E1.Deconstruct(this Base, out int, out int) on an element whose static type is Derived, while E2.Deconstruct(this Derived, out int, out int) is also in scope. The transform folds the explicit call into var (x, (a, b)) = o;, and recompiling that source resolves to E2 instead of E1 -- the decompiled program has different runtime behavior than the original assembly.

Found by high-effort multi-agent review; independently verified.

[christophwille]

Correctness (confirmed): MatchNestedTupleDesignations calls tupleNodes.Add(temp, node) where temp can already be present as the container node registered by ResolveTupleContainer, throwing ArgumentException (duplicate key) instead of bailing out. Reachable via a self-referential element store when a reference type named System.ValueTuple`2 (which IsTupleCompatible accepts by name) is instantiated recursively, e.g. ValueTuple<int, C> where C contains itself, so the elementType/temp.Type equivalence guard passes.

Failure scenario: Decompiling an assembly that ships its own class System.ValueTuple (polyfill or obfuscated code) containing t = t.Item2; makes DeconstructionTransform throw ArgumentException "An item with the same key has already been added", so the whole method fails to decompile with an error stub instead of falling back to the flat assignment output.

Found by high-effort multi-agent review; independently verified.

[christophwille]

Correctness (confirmed): The removed return startPos != pos invariant (a deconstruction requires at least one matched assignment) is exempted for call-rooted patterns containing a nested call, so a chain of explicit source-level Deconstruct() call statements whose out-results are never consumed is now folded into a deconstruction declaration.

Failure scenario: Source that literally writes o.Deconstruct(out var i, out var inner); inner.Deconstruct(out var a, out var b); with all results unused decompiles as a discard deconstruction like (_, (_, _)) = o; instead of the explicit call statements. Master kept the calls verbatim because zero matched assignments rejected the pattern; the decompiled output now misrepresents the shape of the original code.

Found by high-effort multi-agent review; independently verified.

[christophwille]

Correctness (confirmed): TryFindEnclosingTupleDesignation's backward walk steps over any MatchTupleElementStore match regardless of container, so it crosses into an adjacent preceding deconstruction's element stores. The dry run then starts at the wrong position, fails, and the nested designation is dismembered instead of reconstructed.

Failure scenario: Two back-to-back deconstructions with no intervening init statement (sources already materialized, e.g. parameters): var (a, inner1) = t1; var (b, (c, d)) = t2;. At the stloc c position, the walk starts from inner2's store and keeps stepping back over stloc inner1 and stloc a, landing enclosingPos at the first deconstruction's start. The dry run from there fails, so IsConsumableByEnclosingDeconstruction returns false and (c, d) = inner2 is transformed standalone. When the walk later reaches inner2's store, its uses are no longer tuple element reads, so nesting is impossible: output is var inner2 = t2.Item2; (var b, _) = t2; var (c, d) = inner2; instead of var (b, (c, d)) = t2; -- the nested reconstruction silently fails whenever two deconstruction statement runs are adjacent. The walk should only step back over stores belonging to the same container tree (e.g. same root container), not any tuple element store.

Found by high-effort multi-agent review; independently verified.

[christophwille]

Quality regression (confirmed): The deconstructionResults[0] == null check runs before the escaped-tuple-node retry, so a wrongly-nested first element aborts the match instead of being demoted via doNotNest and retried -- losing sugar master already produces.

Failure scenario: For var (inner, x) = GetTuple<(int, int), int>(); Console.WriteLine(inner.Item1 + inner.Item2); Console.WriteLine(x);, MatchNestedTupleDesignations consumes stloc inner as a nested-designation temporary (all of inner's uses are tuple element reads, satisfying AllUsesAreTupleElementReads), but the reads inside WriteLine are never consumed by the pattern, so leaves 0..1 stay unassigned and this check returns false before EscapedTupleNodes() can add inner to doNotNest and retry. The retry would succeed as a flat match. Master sugars this as var (inner, x) = t;; the PR emits raw Item1/Item2 element stores. Moving the escape check before the [0] == null check restores the match.

Found by high-effort multi-agent review; independently verified.

[christophwille]

Correctness (confirmed): AllUsesAreTupleElementReads only accepts the shallow LdObj(LdFlda(use)) shape, so Rest-chained element reads of long tuples (cardinality >= 8) disqualify a nested designation temporary.

Failure scenario: A nested tuple designation whose inner tuple has 8 or more elements, e.g. var (a, (e1, e2, e3, e4, e5, e6, e7, e8)) = t;. The inner temporary's reads of elements 8+ are lowered as ldobj(ldflda Item1(ldflda Rest(ldloca temp))): the use's parent is the Rest LdFlda whose own parent is another LdFlda, not LdObj, so AllUsesAreTupleElementReads returns false and MatchNestedTupleDesignations refuses the nesting -- even though MatchTupleFieldAccess (used by FindIndex/MatchTupleElementRead, whose doc comment explicitly says Rest chains are flattened) would consume those reads fine. The outer match then falls apart into the flat two-statement form instead of the nested designation. The helper should walk the LdFlda chain (or reuse MatchTupleElementRead on the enclosing LdObj) instead of checking only the direct parent pair.

Found by high-effort multi-agent review; independently verified.

[christophwille]

Correctness (plausible): With greedy per-position matching removed, an inner deconstruction position is skipped when a dry run says an enclosing match will consume it -- but the enclosing attempt runs only after all intermediate positions are visited by the whole transform pipeline, which can rewrite the block so the enclosing match no longer fires, and the skipped position gets no second chance. (The doc comment on IsConsumableByEnclosingDeconstruction itself concedes the dry run cannot promise the enclosing attempt still matches.)

Failure scenario: A nested shape where an intermediate statement between the inner and outer pattern is rewritten by another statement transform (e.g. inlining a single-use temp) after the dry run succeeds: the outer MatchDeconstructionSequence fails when finally attempted, and the inner deconstruction was already skipped, so the decompiled output regresses to raw Deconstruct calls / tuple element reads with compiler temporaries, where master at least produced the flat (a, b) = temp; sugar for the inner part.

Found by high-effort multi-agent review; verifier judged the mechanism real but could not construct a concrete trigger.

[christophwille]

Efficiency (confirmed): IsConsumableByEnclosingDeconstruction re-runs the full MatchDeconstructionSequence dry run for every inner statement of the same pattern, matching each nested deconstruction O(k) times.

Details: A nested deconstruction with k inner statements (nested Deconstruct calls, defensive copies, designation temp stores) triggers k separate full dry runs from the same enclosingPos -- each allocating the conversions Dictionary, conversionStLocs List, DeconstructionCall arrays, and delayedActions closures that are then discarded -- plus the final real match: k+1 full matches of one pattern, and each dry run can itself loop on escape retries. On methods dense with tuple/deconstruction code this is quadratic wasted work. Cheaper: since the back-to-front walk visits the k inner positions consecutively, memoize the last dry-run result (block, enclosingPos, endPos) and reuse it instead of re-matching.

Found by high-effort multi-agent review; independently verified.

[christophwille]

Correctness (confirmed): (Anchored here because the affected loop -- if (index <= previousIndex) return false; at ~line 726, just above -- is unchanged context the diff cannot comment on.) MatchConversions still hard-rejects a call-rooted pattern when an unrelated conversion-shaped statement follows the Deconstruct calls -- inconsistent with the new allowUnrelatedAssignments handling one phase later.

Failure scenario: A nested Deconstruct-call chain whose leaves are all single-use (the new anyAssignments == false + NestedCalls path), followed by an unrelated widening assignment such as long l = someInt;: MatchConversions matches the stloc-of-Conv shape, FindIndex(ldloc someInt) returns -1, and if (index <= previousIndex) return false; rejects the entire sequence instead of ending the conversion run -- so no deconstruction is built at all and the output shows raw explicit Deconstruct calls. The same trailing statement without a conv (e.g. int l = GetInt();) is handled gracefully by the new break in MatchAssignments. MatchConversions needs the same "index < 0 for a call-rooted pattern ends the run" treatment (stop consuming, leave the statement after the deconstruction) rather than returning false.

Found by high-effort multi-agent review; independently verified.

[christophwille]

Code review summary (high-effort multi-agent review)

Reviewed 17e6649ba...33c0e986d (5 changed files). 18 candidate findings were pooled from four independent finder passes; each was adversarially verified by a separate agent against the PR head. 17 survived, 1 was refuted; the 10 most severe are posted as inline comments at their code locations. All are in DeconstructionTransform.cs unless noted.

Correctness (recompilation / behavior)

1. DeconstructInstruction.cs:266 -- stobj fallback accepts sign-mismatched pointer stores. The fallback replaces the UnknownType sentinel; StObj.Type is sign-agnostic (from the store opcode), so (*p, y) = t; can be emitted where the original required an explicit (uint) cast -- output no longer recompiles (CS0266).
2. :544 -- BindsOnElementType misses competing extension methods. It guards only against instance-method hiding; a more-specific extension Deconstruct in scope means the folded nested designation rebinds to a different method than the IL calls -- changed runtime behavior.
3. :636 -- reachable ArgumentException (duplicate tupleNodes key). A self-referential element store on a by-name-matched System.ValueTuple polyfill/obfuscated class crashes the whole method's decompilation instead of falling back.
4. :211 -- unused explicit Deconstruct() call chains are folded into discard designations ((_, (_, _)) = o;), misrepresenting source that master reproduced verbatim.

Sugar-loss regressions (output quality)

5. :381 -- the backward walk in TryFindEnclosingTupleDesignation crosses into an adjacent preceding deconstruction's element stores, so nested reconstruction silently fails whenever two deconstruction runs are adjacent. Walk should stay within the same container tree.
6. :215 -- deconstructionResults[0] == null check runs before the escaped-tuple-node retry, so a wrongly-nested first element aborts instead of being demoted and retried -- a regression vs. master's flat sugar.
7. :726 (anchored at 737) -- MatchConversions hard-rejects the whole call-rooted pattern on a trailing unrelated conversion, inconsistent with the new allowUnrelatedAssignments break in MatchAssignments.
8. :645 -- AllUsesAreTupleElementReads misses Rest-chained reads, so nested designations with inner tuples of 8+ elements are refused.
9. :130 (plausible, no concrete trigger constructed) -- a skipped inner position gets no second chance if intermediate transforms rewrite the block so the enclosing match no longer fires; the dry-run doc comment itself concedes this window.

Efficiency

10. :318 -- IsConsumableByEnclosingDeconstruction re-runs the full dry run per inner statement (k+1 full matches of one pattern, with per-run allocations). Memoizing the last dry-run result over the consecutive back-to-front walk would remove the quadratic rework.

Refuted during verification: the concern that break-and-commit on unrelated assignments could leave unconsumed Deconstruct results ill-formed -- the LoadCount <= 1 constraint in MatchDeconstructionCall plus the forwarding fixup covers all cases.

Seven further low-severity confirmed findings (minor duplication and micro-inefficiencies) were dropped under the report cap.

🤖 Generated with Claude Code