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

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<TKey, TValue>(this KeyValuePair<TKey, TValue> 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<NestedOuter> {
+				new NestedOuter { Value = 1 },
+				new NestedOuter { Value = 2 }
+			});
+			NestedDeconstruction_DiscardedElement(new KeyValuePair<object, DiscardData>("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<object, DiscardData> 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<NestedOuter> items)
+		{
+			Console.WriteLine("NestedDeconstruction_ForEach:");
+			foreach (var (x, (a, b)) in items)
+			{
+				Console.WriteLine(x + a + b);
+			}
 		}
 
 		public void Property_NoDeconstruction_SwappedAssignments()

ICSharpCode.Decompiler.Tests/TestCases/Pretty/DeconstructionTests.cs

@@ -70,6 +70,17 @@ namespace ICSharpCode.Decompiler.Tests.TestCases.Pretty
 			}
 		}
 
+		public struct StructDeconstructionSource<T, T2>
+		{
+			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<T, T2> GetStructSource<T, T2>()
+		{
+			return default(StructDeconstructionSource<T, T2>);
+		}
+
 		private ref T GetRef<T>()
 		{
 			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<string, int>();
+			Console.WriteLine(value);
+			Console.WriteLine(value2);
+		}
+
 		public void Property_NoConversion_Custom()
 		{
 			(Get(0).NMy, Get(1).My) = GetSource<MyInt?, MyInt>();

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;

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<DeconstructInstruction> delayedActions)
 			{
 				while (conversionStLocIndex < conversionStLocs.Count)