From 716d7f6a5b0e5c1244e983d18aa7525451e3ad2e Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Sat, 1 Aug 2026 16:46:21 +0200 Subject: [PATCH] Fix #3803: Crash on nested deconstruction of custom structs Optimized code stores no temporary for a deconstruction element that is used only once after the deconstruction. MatchAssignments handled that for trailing elements, but a nested deconstruction copies the inner element to a temporary, so the elements preceding it are also left without an assignment; their external load then violated the DeconstructInstruction invariant that all pattern variable loads are descendants of the instruction. The forwarding fixup now covers all unassigned elements and inserts in pattern order, because the statement and expression builders pair pattern variables with assignments positionally. This also fixes the nested tuple deconstruction crash reported in #3388. Also unwrap the address of the tested operand in VisitDeconstructInstruction: deconstructing a struct passes the receiver by reference, which was emitted as an invalid cast, 'var (x, y) = (S)(ref s);', even without nesting. Fixes #3388. Assisted-by: Claude:claude-fable-5:Claude Code --- .../Correctness/DeconstructionTests.cs | 84 +++++++++++++++++++ .../TestCases/Pretty/DeconstructionTests.cs | 23 +++++ .../CSharp/ExpressionBuilder.cs | 6 ++ .../IL/Transforms/DeconstructionTransform.cs | 68 +++++++++++---- 4 files changed, 166 insertions(+), 15 deletions(-) diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Correctness/DeconstructionTests.cs b/ICSharpCode.Decompiler.Tests/TestCases/Correctness/DeconstructionTests.cs index a0924b137..15c49cb9a 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Correctness/DeconstructionTests.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/Correctness/DeconstructionTests.cs @@ -6,6 +6,15 @@ using System.Threading.Tasks; namespace ICSharpCode.Decompiler.Tests.TestCases.Correctness { + static class KeyValuePairExtensions + { + public static void Deconstruct(this KeyValuePair pair, out TKey key, out TValue value) + { + key = pair.Key; + value = pair.Value; + } + } + class DeconstructionTests { public static void Main() @@ -137,6 +146,81 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Correctness NullReferenceException_RefLocalReferencesArrayElement_Deconstruction(out _, null); DeconstructTupleSameVar(("a", "b")); DeconstructTupleListForEachSameVar(new List<(string, string)> { ("a", "b") }); + StructDeconstruction_Assignment(new NestedInner { Value = 7 }); + NestedDeconstruction_Assignment(new NestedOuter { Value = 42 }); + NestedDeconstruction_ForEach(new List { + new NestedOuter { Value = 1 }, + new NestedOuter { Value = 2 } + }); + NestedDeconstruction_DiscardedElement(new KeyValuePair("key", default(DiscardData))); + } + + public struct DiscardData + { + public void Deconstruct(out object o1, out object o2) + { + Console.WriteLine("DiscardData.Deconstruct"); + o1 = 1; + o2 = 2; + } + } + + public void NestedDeconstruction_DiscardedElement(KeyValuePair pair) + { + Console.WriteLine("NestedDeconstruction_DiscardedElement:"); + var (key, (value, _)) = pair; + Console.WriteLine(key); + Console.WriteLine(value); + } + + public struct NestedInner + { + public int Value; + + public void Deconstruct(out int a, out int b) + { + Console.WriteLine("NestedInner.Deconstruct"); + a = Value + 1; + b = Value + 2; + } + } + + public struct NestedOuter + { + public int Value; + + public void Deconstruct(out int x, out NestedInner inner) + { + Console.WriteLine("NestedOuter.Deconstruct"); + x = Value; + inner = new NestedInner { Value = Value * 10 }; + } + } + + public void StructDeconstruction_Assignment(NestedInner s) + { + Console.WriteLine("StructDeconstruction_Assignment:"); + var (a, b) = s; + Console.WriteLine(a); + Console.WriteLine(b); + } + + public void NestedDeconstruction_Assignment(NestedOuter o) + { + Console.WriteLine("NestedDeconstruction_Assignment:"); + var (x, (a, b)) = o; + Console.WriteLine(x); + Console.WriteLine(a); + Console.WriteLine(b); + } + + public void NestedDeconstruction_ForEach(IEnumerable items) + { + Console.WriteLine("NestedDeconstruction_ForEach:"); + foreach (var (x, (a, b)) in items) + { + Console.WriteLine(x + a + b); + } } public void Property_NoDeconstruction_SwappedAssignments() diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/DeconstructionTests.cs b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/DeconstructionTests.cs index c5f029aca..1e1802eb2 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/DeconstructionTests.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/DeconstructionTests.cs @@ -70,6 +70,17 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty } } + public struct StructDeconstructionSource + { + public int Dummy { get; set; } + + public void Deconstruct(out T a, out T2 b) + { + a = default(T); + b = default(T2); + } + } + private class AssignmentTargets { public int IntField; @@ -128,6 +139,11 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty return null; } + private StructDeconstructionSource GetStructSource() + { + return default(StructDeconstructionSource); + } + private ref T GetRef() { throw new NotImplementedException(); @@ -299,6 +315,13 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty Console.WriteLine(myInt4); } + public void LocalVariable_NoConversion_Struct_Custom() + { + var (value, value2) = GetStructSource(); + Console.WriteLine(value); + Console.WriteLine(value2); + } + public void Property_NoConversion_Custom() { (Get(0).NMy, Get(1).My) = GetSource(); diff --git a/ICSharpCode.Decompiler/CSharp/ExpressionBuilder.cs b/ICSharpCode.Decompiler/CSharp/ExpressionBuilder.cs index ddd5da25b..bfb457be7 100644 --- a/ICSharpCode.Decompiler/CSharp/ExpressionBuilder.cs +++ b/ICSharpCode.Decompiler/CSharp/ExpressionBuilder.cs @@ -4885,6 +4885,12 @@ namespace ICSharpCode.Decompiler.CSharp { IType rhsType = inst.Pattern.Variable.Type; var rhs = Translate(inst.Pattern.TestedOperand, rhsType); + if (rhs.Expression is DirectionExpression dirExpr) + { + // Deconstructing a value type takes the address of the deconstructed value: + // (ref x) => x + rhs = rhs.UnwrapChild(dirExpr.Expression); + } rhs = rhs.ConvertTo(rhsType, this); // TODO allowImplicitConversion var assignments = inst.Assignments.Instructions; int assignmentPos = 0; diff --git a/ICSharpCode.Decompiler/IL/Transforms/DeconstructionTransform.cs b/ICSharpCode.Decompiler/IL/Transforms/DeconstructionTransform.cs index 9dab0c7de..f9938f3ed 100644 --- a/ICSharpCode.Decompiler/IL/Transforms/DeconstructionTransform.cs +++ b/ICSharpCode.Decompiler/IL/Transforms/DeconstructionTransform.cs @@ -19,6 +19,7 @@ using System; using System.Collections.Generic; using System.Collections.Immutable; +using System.Diagnostics; using System.Linq; using System.Resources; @@ -390,27 +391,64 @@ namespace ICSharpCode.Decompiler.IL.Transforms if (deconstructionResults != null) { - int i = previousIndex + 1; - while (i < deconstructionResults.Length) + foreach (var v in deconstructionResults) { - var v = deconstructionResults[i]; - // this should only happen in release mode, where usually the last deconstruction element - // is not stored to a temporary, if it is used directly (and only once!) - // after the deconstruction. - if (v?.LoadCount == 1) - { - delayedActions += (DeconstructInstruction deconstructInst) => { - var freshVar = context.Function.RegisterVariable(VariableKind.StackSlot, v.Type); - deconstructInst.Assignments.Instructions.Add(new StLoc(freshVar, new LdLoc(v))); - v.LoadInstructions[0].Variable = freshVar; - }; - } - i++; + // In optimized code a deconstruction element is not stored to a temporary, + // if it is used directly (and only once!) after the deconstruction. This + // happens for trailing elements, but also for leading elements, e.g., when + // a nested deconstruction copies the inner element to a temporary before + // the elements preceding it are used. Forward such elements through a fresh + // variable assigned inside the deconstruction, so that every pattern + // variable's load is a descendant of the deconstruct instruction. + // The assignment is inserted in pattern order, because StatementBuilder and + // ExpressionBuilder pair pattern variables with assignments positionally. + // LoadCount must be read eagerly, at match time: for a tuple deconstruction + // the elements are the fresh "E_i" variables created in FindIndex, whose + // loads only materialize when the delayed ReplaceWith actions run, so + // LoadCount is still 0 here and forwarding never fires on that path. That + // is load-bearing, not incidental: the fresh variables are never registered + // in deconstructionResultsLookup, so GetAssignmentIndex could not position + // a forwarding assignment among a tuple's assignments. + if (v?.LoadCount != 1) + continue; + delayedActions += (DeconstructInstruction deconstructInst) => { + var load = v.LoadInstructions[0]; + if (load.IsDescendantOf(deconstructInst)) + return; + // MatchDeconstruction registered every deconstruction result in the + // lookup, and the tuple path never gets here (see above); a miss would + // leave the load outside the deconstruct instruction, i.e. a malformed + // pattern, because the transform is already committed at this point. + bool isDeconstructionResult = deconstructionResultsLookup.TryGetValue(v, out int index); + Debug.Assert(isDeconstructionResult); + var freshVar = context.Function.RegisterVariable(VariableKind.StackSlot, v.Type); + var instructions = deconstructInst.Assignments.Instructions; + int insertPos = 0; + while (insertPos < instructions.Count && GetAssignmentIndex(instructions[insertPos]) < index) + insertPos++; + instructions.Insert(insertPos, new StLoc(freshVar, new LdLoc(v))); + load.Variable = freshVar; + }; } } return startPos != pos; + int GetAssignmentIndex(ILInstruction inst) + { + if (DeconstructInstruction.IsAssignment(inst, context.TypeSystem, out _, out var value) + && value.MatchLdLoc(out var inputVariable)) + { + if (deconstructionResultsLookup.TryGetValue(inputVariable, out int index)) + return index; + // Forwarding assignments produced for conversions load a fresh variable; + // their pattern index is that of the conversion output they store to. + if (inst is StLoc stLoc && deconstructionResultsLookup.TryGetValue(stLoc.Variable, out index)) + return index; + } + return int.MaxValue; + } + void AddMissingAssignmentsForConversions(int index, ref Action delayedActions) { while (conversionStLocIndex < conversionStLocs.Count)