From fd623ccf4fed2311c5b38152ade39cddeb016c26 Mon Sep 17 00:00:00 2001 From: Siegfried Pammer Date: Sun, 6 Sep 2026 08:42:34 +0200 Subject: [PATCH] Fix #4059: treat a Deconstruct out argument as a definition 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 --- .../TestCases/Pretty/DeconstructionTests.cs | 20 +++++------ .../ReachingDefinitionsVisitor.cs | 35 +++++++++++++++++-- 2 files changed, 42 insertions(+), 13 deletions(-) diff --git a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/DeconstructionTests.cs b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/DeconstructionTests.cs index fe2cc83888..8dbdae4723 100644 --- a/ICSharpCode.Decompiler.Tests/TestCases/Pretty/DeconstructionTests.cs +++ b/ICSharpCode.Decompiler.Tests/TestCases/Pretty/DeconstructionTests.cs @@ -1240,17 +1240,15 @@ public void TupleFieldSource() Console.WriteLine(value2); } - // #4059: csc reuses the same out-slot temporaries for both calls, and hands them to the - // second one in the opposite order, so neither deconstruction is recognized. - //public void TwoBackToBackDeconstructs_Custom() - //{ - // var (value, value2) = GetSource(); - // Console.WriteLine(value); - // Console.WriteLine(value2); - // var (value3, value4) = GetSource(); - // Console.WriteLine(value3); - // Console.WriteLine(value4); - //} + public void TwoBackToBackDeconstructs_Custom() + { + var (value, value2) = GetSource(); + Console.WriteLine(value); + Console.WriteLine(value2); + var (value3, value4) = GetSource(); + Console.WriteLine(value3); + Console.WriteLine(value4); + } } } diff --git a/ICSharpCode.Decompiler/FlowAnalysis/ReachingDefinitionsVisitor.cs b/ICSharpCode.Decompiler/FlowAnalysis/ReachingDefinitionsVisitor.cs index c9b53d2db8..1c4d231c91 100644 --- a/ICSharpCode.Decompiler/FlowAnalysis/ReachingDefinitionsVisitor.cs +++ b/ICSharpCode.Decompiler/FlowAnalysis/ReachingDefinitionsVisitor.cs @@ -39,7 +39,9 @@ namespace ICSharpCode.Decompiler.FlowAnalysis /// * StLoc /// * TryCatchHandler (for the exception variable) /// * ReachingDefinitionsVisitor.UninitializedVariable for uninitialized variables. - /// Note that we do not keep track of LdLoca/references/pointers. + /// * LdLoca, but only where the address is known to be assigned without being read + /// first, see IsDefiniteAssignment. + /// Note that we do not otherwise keep track of LdLoca/references/pointers. /// The analysis will likely be wrong/incomplete for variables with AddressCount != 0. /// /// Note: this class does not store the computed information, because doing so @@ -304,6 +306,8 @@ public ReachingDefinitionsVisitor(ILFunction scope, BitSet analyzedVariables, Ca int expectedStoreCount = scope.Variables[vi].StoreInstructions.Count; // Extra store for the uninitialized state. expectedStoreCount += 1; + // Extra store for every address load that is a definite assignment. + expectedStoreCount += scope.Variables[vi].AddressInstructions.Count(IsDefiniteAssignment); Debug.Assert(stores.Count == expectedStoreCount); stores.CopyTo(allStores, si); // Add all stores except for the first (representing the uninitialized state) @@ -335,7 +339,7 @@ static List[] FindAllStoresByVariable(ILFunction scope, BitSet ac } foreach (var inst in scope.Descendants) { - if (inst.HasDirectFlag(InstructionFlags.MayWriteLocals)) + if (inst.HasDirectFlag(InstructionFlags.MayWriteLocals) || IsDefiniteAssignment(inst)) { cancellationToken.ThrowIfCancellationRequested(); ILVariable v = ((IInstructionWithVariableOperand)inst).Variable; @@ -395,6 +399,33 @@ protected internal override void VisitStLoc(StLoc inst) HandleStore(inst, inst.Variable); } + protected internal override void VisitLdLoca(LdLoca inst) + { + if (IsDefiniteAssignment(inst)) + HandleStore(inst, inst.Variable); + else + base.VisitLdLoca(inst); + } + + /// + /// Gets whether an address load definitely assigns the variable without reading it first, + /// which makes it a definition, just like a store. + /// + /// IL has no such instruction - 'out' is a C# convention over 'ref' - so this holds only + /// for the out arguments of a Deconstruct method, where C#'s definite assignment rules + /// rule out a read. A Deconstruct method written in IL could read the argument, and the + /// possibility is ignored here: without it, two deconstructions that the compiler lowered + /// onto the same temporaries stay in one live range and neither can be recognized. + /// + /// + static bool IsDefiniteAssignment(ILInstruction inst) + { + // Argument 0 is the receiver of an instance method or the 'this' argument of an + // extension method; IsDeconstructMethod requires every parameter after it to be out. + return inst is LdLoca { ChildIndex: > 0, Parent: CallInstruction call } + && MatchInstruction.IsDeconstructMethod(call.Method); + } + protected override void HandleMatchStore(MatchInstruction inst) { HandleStore(inst, inst.Variable);