Eliminate return-accumulator temps in EH/lock by sinking returns

src/ILInspector.Decompiler.Tests/CompileBackGateTests.cs

@@ -19,38 +19,45 @@ public class CompileBackGateTests
     /// Methods that still recompile to a different opcode stream — the open
     /// decompiler docket. Each is a tracked defect or a benign over-render; the gate
     /// tolerates these but fails if a NEW method joins the set. Shrink this list as
-    /// fixes land. Tracked defects include StaleFieldRead (issue #605), Shadowed
-    /// (a dropped this.field load), and the definite-assignment default-init
-    /// over-render (TryFinallyAdd, PowerOfTwo, CatchEverything, SwitchCase,
-    /// WhileLoop, ClassicLock, TryFinallyTwoReturns, ManualDisposeAsyncInFinally).
+    /// fixes land. Tracked defects include StaleFieldRead (issue #605),
+    /// branch-polarity in the string-switch lowering (DayNumber,
+    /// SmallStringSwitch), and benign codegen choices (BothPositive, NeitherOr,
+    /// ReverseCopy, the volatile field stub in ReadVolatileFlag).
     /// </summary>
     static readonly HashSet<string> KnownDiffs = new(StringComparer.Ordinal)
     {
         "BothPositive",
-        "CatchEverything",
-        "ClassicLock",
         "DayNumber",
-        "ManualDisposeAsyncInFinally",
         "NeitherOr",
-        "PowerOfTwo",
         "ReadVolatileFlag",
         "ReverseCopy",
         "SmallStringSwitch",
         "StaleFieldRead",
-        "SwitchCase",
-        "TryFinallyAdd",
-        "TryFinallyTwoReturns",
-        "WhileLoop",
     };
 
     /// <summary>
     /// Methods a prior compile-back fix turned opcode-exact. Pinning them guards the
     /// fix durably: CheckedAdd must keep the overflow check (#604), UnsignedShift
     /// must keep dropping the redundant width mask (#606), Shadowed must keep
-    /// qualifying the shadowed this.field load (#607), and .ctor must keep lifting
-    /// its field initializer ahead of the base call to the field declaration.
+    /// qualifying the shadowed this.field load (#607), .ctor must keep lifting
+    /// its field initializer ahead of the base call to the field declaration
+    /// (#614), and the return-accumulator elimination must keep sinking the
+    /// result temp out of an EH region or lock (TryFinallyAdd,
+    /// TryFinallyTwoReturns, CatchEverything, ClassicLock,
+    /// ManualDisposeAsyncInFinally).
     /// </summary>
-    static readonly string[] PinnedExact = { "CheckedAdd", "UnsignedShift", "Shadowed", ".ctor" };
+    static readonly string[] PinnedExact =
+    {
+        "CheckedAdd",
+        "UnsignedShift",
+        "Shadowed",
+        ".ctor",
+        "TryFinallyAdd",
+        "TryFinallyTwoReturns",
+        "CatchEverything",
+        "ClassicLock",
+        "ManualDisposeAsyncInFinally",
+    };
 
     static IReadOnlyList<CompileBack.CompileBackResult> EvaluateFixtures()
     {

src/ILInspector.Decompiler.Tests/IrImporterTests.cs

@@ -311,6 +311,40 @@ public void Shadowed_QualifiesFieldLoadWhenParameterShadows()
         Assert.Contains("this._shadowed", CSharpPrinter.PrintRaised(function).Output!);
     }
 
+    [Fact]
+    public void TryFinallyAdd_SinksReturnAccumulatorIntoTry()
+    {
+        // The result is computed inside the try and the ret sits after the
+        // finally, so the importer reconstructs a synthetic accumulator local.
+        // Sinking it back to `return x + 1;` inside the try keeps Roslyn from
+        // re-zero-initing the temp (a dead leading ldc.i4.0/stloc the original
+        // never emitted) — there must be no accumulator local in the output.
+        var function = ImportFixture(nameof(CfgSampleClass.TryFinallyAdd));
+        IrPasses.Run(function);
+
+        string output = CSharpPrinter.PrintRaised(function).Output!;
+        Assert.Equal(DecompilationFidelity.Full, function.Fidelity);
+        Assert.Contains("return x + 1;", output);
+        Assert.DoesNotContain("V_0", output);
+    }
+
+    [Fact]
+    public void TryFinallyTwoReturns_SinksBothReturnsIntoTry()
+    {
+        // Both the guarded `return x` and the fall-through `return -1` are
+        // spilled into one accumulator and returned after the finally. The
+        // pass must sink both back into the try as distinct returns, leaving no
+        // accumulator local behind.
+        var function = ImportFixture(nameof(CfgSampleClass.TryFinallyTwoReturns));
+        IrPasses.Run(function);
+
+        string output = CSharpPrinter.PrintRaised(function).Output!;
+        Assert.Equal(DecompilationFidelity.Full, function.Fidelity);
+        Assert.Contains("return x;", output);
+        Assert.Contains("return -1;", output);
+        Assert.DoesNotContain("V_0", output);
+    }
+
     [Fact]
     public void InParameter_SeesThroughModifier_ImportsAtFull()
     {

src/ILInspector.Decompiler/Pipeline/Passes/IrPass.cs

@@ -81,6 +81,13 @@ public static class IrPasses
         // printer lifts it to a signature initializer rather than an invalid
         // base(temp); body call (CS0175).
         new ConstructorChainArgumentPass(),
+        // Eliminate return-accumulator temporaries the compiler spilled across
+        // an EH region or lock (try { V = e; } finally { } return V; back to
+        // try { return e; }). Runs after structuring and the second inlining so
+        // the try/catch/finally/lock shells and their tail stores are fully
+        // formed; expression inlining leaves these temps standing because it
+        // refuses to move a value across a leave/region edge.
+        new ReturnSinkingPass(),
         // Raise any static function-pointer load still standing into &Method
         // (it feeds a calli, native callback, or delegate*-typed field — all of
         // which take a function pointer directly).

src/ILInspector.Decompiler/Pipeline/Passes/ReturnSinkingPass.cs

@@ -0,0 +1,214 @@
+namespace ILInspector.Decompiler.Pipeline;
+
+/// <summary>
+/// Eliminates a return-accumulator temporary — the inverse of the compiler's
+/// habit of spilling a method's result into a synthetic local when the value
+/// is produced inside an exception region or a lock and the
+/// <c>ret</c> sits after the region closes. The decompiler reconstructs
+/// <c>try { V = x + 1; } finally { ... } return V;</c>; the original source was
+/// <c>try { return x + 1; } finally { ... }</c>. Recompiling the temp form makes
+/// Roslyn re-zero-init the synthesized local, so the recompiled IL gains a dead
+/// leading <c>ldc.i4.0; stloc</c> the original never had.
+///
+/// The pass is sound by construction: it acts only on a local that is never
+/// address-taken and whose every load is the direct operand of a
+/// <see cref="Return"/>, and it applies a candidate only when every store and
+/// every load is consumed by the plan (all-or-nothing). A store feeding a
+/// return is rewritten into a <see cref="Return"/> in place — adjacent
+/// <c>StoreLocal V; return V;</c> pairs, and the terminal fall-through stores of
+/// a structured statement (try/finally try body, try/catch arms, lock body,
+/// if/else arms) the trailing <c>return V;</c> follows. Switch is deliberately
+/// excluded: a switch <em>expression</em> is lowered through its own result
+/// accumulator, so reproducing per-arm returns would diverge from the original.
+///
+/// This is the structural raiser for the pure-return-accumulator subset; the
+/// printer's definite-assignment dead-init drop (CSharpPrinter, PR #611) is the
+/// conservative fallback for every accumulator this pass deliberately leaves
+/// standing — switch-expression results, multi-use locals (a value used past
+/// its return), and control flow this pass does not model. Those keep their
+/// temp but lose the redundant <c>= default</c>; only here does the temp vanish
+/// entirely. The two compose without overlap: a local this pass sinks has no
+/// stores or loads left, so the printer never emits a declaration for it.
+/// </summary>
+public sealed class ReturnSinkingPass : IIrPass
+{
+    public string Name => "return-sinking";
+
+    public void Run(IrFunction function)
+    {
+        while (SinkOnce(function))
+        {
+        }
+    }
+
+    static bool SinkOnce(IrFunction function)
+    {
+        var stores = new Dictionary<int, List<StoreLocal>>();
+        var returnLoads = new Dictionary<int, List<Return>>();
+        var addressTaken = new HashSet<int>();
+        var escapingLoad = new HashSet<int>();
+
+        foreach (var node in function.Descendants)
+        {
+            switch (node)
+            {
+                case StoreLocal store:
+                    (stores.TryGetValue(store.Index, out var list) ? list : stores[store.Index] = []).Add(store);
+                    break;
+                case LoadLocalAddress address:
+                    addressTaken.Add(address.Index);
+                    break;
+                case LoadLocal load when load.Parent is Return ret && ReferenceEquals(ret.Value, load):
+                    (returnLoads.TryGetValue(load.Index, out var rl) ? rl : returnLoads[load.Index] = []).Add(ret);
+                    break;
+                case LoadLocal load:
+                    // A load that is not a return's operand: the local carries
+                    // its value somewhere else, so it is not a pure return
+                    // accumulator and must be left alone.
+                    escapingLoad.Add(load.Index);
+                    break;
+            }
+        }
+
+        foreach (var (index, indexStores) in stores)
+        {
+            if (addressTaken.Contains(index) || escapingLoad.Contains(index))
+                continue;
+            if (!returnLoads.TryGetValue(index, out var returns) || returns.Count == 0)
+                continue;
+            if (TryPlan(index, indexStores, returns) is { } plan)
+            {
+                Apply(plan);
+                return true;
+            }
+        }
+        return false;
+    }
+
+    sealed record Plan(List<(StoreLocal Store, Break? Break)> Folds, List<Return> Returns);
+
+    static Plan? TryPlan(int index, List<StoreLocal> indexStores, List<Return> returns)
+    {
+        var folds = new List<(StoreLocal, Break?)>();
+        var consumed = new HashSet<StoreLocal>(ReferenceEqualityComparer.Instance);
+
+        foreach (var ret in returns)
+        {
+            if (ret.Parent is not Block block || ret.ChildIndex == 0)
+                return null;
+            var prev = block.Children[ret.ChildIndex - 1];
+
+            if (prev is StoreLocal adjacent && adjacent.Index == index)
+            {
+                if (!consumed.Add(adjacent))
+                    return null;
+                folds.Add((adjacent, null));
+                continue;
+            }
+
+            var tails = CollectStmtTails(prev, index);
+            if (tails is null)
+                return null;
+            foreach (var tail in tails)
+            {
+                if (!consumed.Add(tail.Store))
+                    return null;
+                folds.Add(tail);
+            }
+        }
+
+        // Soundness: refuse unless the plan consumes every store of the local.
+        // Every load is already a return we delete, so this leaves no orphan
+        // read of an undefined accumulator and no store that should have been
+        // a return but was missed.
+        if (consumed.Count != indexStores.Count)
+            return null;
+
+        return new Plan(folds, returns);
+    }
+
+    static void Apply(Plan plan)
+    {
+        foreach (var (store, brk) in plan.Folds)
+        {
+            var value = (IrExpression)store.DetachChildren()[0];
+            brk?.Detach();
+            store.ReplaceWith(new Return(value));
+        }
+        foreach (var ret in plan.Returns)
+            ret.Detach();
+    }
+
+    /// <summary>
+    /// The terminal fall-through stores of <paramref name="index"/> a trailing
+    /// <c>return index</c> consumes when it follows the structured statement, or
+    /// null when the statement is not a clean accumulator sink (every reachable
+    /// fall-through tail must be a <c>StoreLocal index</c>).
+    /// </summary>
+    static List<(StoreLocal Store, Break? Break)>? CollectStmtTails(IrNode statement, int index)
+    {
+        switch (statement)
+        {
+            case TryFinally tryFinally:
+                // The finally cannot produce the returned value; only the try
+                // body's fall-through tail feeds the trailing return.
+                return CollectContainerTail(tryFinally.TryBody, index);
+            case Lock @lock:
+                return CollectContainerTail(@lock.Body, index);
+            case TryCatch tryCatch:
+            {
+                var result = CollectContainerTail(tryCatch.TryBody, index);
+                if (result is null)
+                    return null;
+                foreach (var clause in tryCatch.Clauses)
+                {
+                    var clauseTail = CollectContainerTail(clause.Body, index);
+                    if (clauseTail is null)
+                        return null;
+                    result.AddRange(clauseTail);
+                }
+                return result;
+            }
+            case Switch:
+                // Do not sink into a switch: csc lowers a switch *expression*
+                // through its own result accumulator (each arm stores, the
+                // epilogue loads and returns), so reproducing per-arm returns
+                // diverges from the original stream rather than matching it.
+                return null;
+            case IfStatement { HasElse: true } ifStatement:
+            {
+                var thenTail = CollectBlockTail(ifStatement.Then, index);
+                var elseTail = CollectBlockTail(ifStatement.Else!, index);
+                if (thenTail is null || elseTail is null)
+                    return null;
+                thenTail.AddRange(elseTail);
+                return thenTail;
+            }
+            default:
+                return null;
+        }
+    }
+
+    static List<(StoreLocal Store, Break? Break)>? CollectContainerTail(BlockContainer container, int index)
+    {
+        var blocks = container.Blocks;
+        return blocks.Count == 0 ? null : CollectBlockTail(blocks[^1], index);
+    }
+
+    static List<(StoreLocal Store, Break? Break)>? CollectBlockTail(Block block, int index)
+    {
+        if (block.Children.Count == 0)
+            return null;
+        var last = block.Children[^1];
+        if (last is StoreLocal store && store.Index == index)
+            return [(store, null)];
+        if (last is Break brk
+            && block.Children.Count >= 2
+            && block.Children[^2] is StoreLocal stored
+            && stored.Index == index)
+        {
+            return [(stored, brk)];
+        }
+        return CollectStmtTails(last, index);
+    }
+}