From 1a11df6f700d61ddda4d4ec4c5d88bb34a9d8bcb Mon Sep 17 00:00:00 2001 From: hatayama Date: Mon, 5 Oct 2026 10:04:38 +0900 Subject: [PATCH 1/2] Capture every closure struct a local function receives at a pause point A local function that captures variables gets them, and often the instance, in by-ref closure structs. The injection only passed one of those structs, and only when the local function was static. A local function compiled as an instance method of a lambda's closure class therefore showed neither `this` nor the variables of the method scope, an instance local function lost the enclosing locals, and a local function using two scopes lost one of them. - The injection now boxes every by-ref closure struct and passes them to Capture as a separate array, loading each from its own argument slot (shifted by one for the hidden `this` of an instance method). - The collector walks the instance holder and every struct, lists their variables before the instance state, and takes `this` from the first link it finds, so the count cap still favors variables over fields. - A closure struct argument is no longer reported as a ref parameter that cannot be captured, since its variables are captured through it. - A hit-when condition on a variable that only a struct carries is now evaluated instead of failing to find it. Closes #3072 --- .../HotReloadPausePointContractTests.cs | 73 ++++++ .../Tests/Editor/PausePointToolModeTests.cs | 8 +- .../SourcePausePointCaptureTests.cs | 129 ++++++++-- ...ourcePausePointTruncationAggregateTests.cs | 12 +- ...PatcherClosureFrameLocalFunctionFixture.cs | 89 +++++++ ...erClosureFrameLocalFunctionFixture.cs.meta | 11 + ...ourcePausePointClosureFrameCaptureTests.cs | 229 ++++++++++++++++++ ...PausePointClosureFrameCaptureTests.cs.meta | 11 + .../PausePoint/SourcePausePointCapture.cs | 13 +- .../SourcePausePointCaptureEligibility.cs | 45 +++- .../SourcePausePointClosureFrameArgument.cs | 50 +--- .../SourcePausePointInjectionEmitter.cs | 43 ++-- .../SourcePausePointVariableCollector.cs | 67 +++-- .../SourcePausePointVariableFormatter.cs | 2 +- 14 files changed, 669 insertions(+), 113 deletions(-) create mode 100644 Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherClosureFrameLocalFunctionFixture.cs create mode 100644 Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherClosureFrameLocalFunctionFixture.cs.meta create mode 100644 Assets/Tests/Editor/SourcePausePointPatcher/SourcePausePointClosureFrameCaptureTests.cs create mode 100644 Assets/Tests/Editor/SourcePausePointPatcher/SourcePausePointClosureFrameCaptureTests.cs.meta diff --git a/Assets/Tests/Editor/HotReload/HotReloadPausePointContractTests.cs b/Assets/Tests/Editor/HotReload/HotReloadPausePointContractTests.cs index aabeef07f6..53844a8c3e 100644 --- a/Assets/Tests/Editor/HotReload/HotReloadPausePointContractTests.cs +++ b/Assets/Tests/Editor/HotReload/HotReloadPausePointContractTests.cs @@ -801,6 +801,79 @@ public async Task Enable_OnHotReloadedLocalFunctionBody_CapturesInstanceAsThis() FormatCaptured(status)); } + /// + /// What: a pause point inside a local function of a hot-reloaded method, where a lambda + /// capturing the loop variable turns the loop scope into a closure class, captures the + /// instance as "this" with its fields and the variables of every enclosing scope, although + /// the instance and the method parameter travel in a by-ref closure struct that the loop + /// closure class does not link to. + /// + [Test] + public async Task Enable_OnHotReloadedLocalFunctionOfLoopClosure_CapturesInstanceAndEnclosingScopes() + { + string onDisk = File.ReadAllText(ResolveFixtureAbsolutePath()); + const string compiledBody = + "public int ComputeWithPrivate(int delta)\n {\n" + + " return _secret + delta;\n" + + " }"; + Assert.That(onDisk, Does.Contain(compiledBody), "Precondition: ComputeWithPrivate body must exist."); + string editedSource = onDisk.Replace( + compiledBody, + "public int ComputeWithPrivate(int delta)\n {\n" + + " int total = 0;\n" + + " for (int j = 0; j < 1; j++)\n {\n" + + " Func read = () => j;\n" + + " int Local()\n {\n" + + " int loopSum = _secret + delta + j;\n" + + " return loopSum;\n" + + " }\n\n" + + " total += Local() + read();\n" + + " }\n\n" + + " return total;\n" + + " }", + StringComparison.Ordinal); + int enableLine = FindLineNumber(editedSource, "return loopSum;"); + Assert.That(enableLine, Is.GreaterThan(0)); + + await HotReloadFromEditedSourceAsync(editedSource, "ContractLoopClosureLocalFunction.cs"); + + PausePointResponse enable = new PausePointUseCase().Enable(new EnablePausePointSchema + { + File = FixtureProjectRelativePath, + Line = enableLine, + TimeoutSeconds = 30, + Mode = UloopPausePointCaptureMode.Continuous + }); + Assert.That(enable.Success, Is.True, enable.Message + " / " + enable.RecommendedNextAction); + Assert.That(enable.RetargetedToHotReloadPatch, Is.True); + Assert.That(enable.ResolvedLine, Is.GreaterThan(0)); + + HotReloadE2EFixture fixture = new HotReloadE2EFixture(); + Assert.That(fixture.ComputeWithPrivate(5), Is.EqualTo(fixture.SecretForAssert + 5)); + + UloopPausePointSnapshot status = UloopPausePointRegistry.GetStatus(enable.Id); + Assert.That(status.IsHit, Is.True); + Assert.That(status.CapturedVariables.Any(v => v.Name == "this"), Is.True, FormatCaptured(status)); + Assert.That( + status.CapturedVariables.Any(v => v.Name == "_secret" && v.Scope == UloopCapturedVariableScope.InstanceField), + Is.True, + FormatCaptured(status)); + Assert.That( + status.CapturedVariables.Any(v => v.Name == "delta" && v.Value == "5"), + Is.True, + FormatCaptured(status)); + Assert.That( + status.CapturedVariables.Any(v => v.Name == "j" && v.Value == "0"), + Is.True, + FormatCaptured(status)); + Assert.That( + status.CapturedVariables.Any(v => v.Name == HotReloadShimMethodLookup.ShimReceiverParameterName), + Is.False, + FormatCaptured(status)); + // Read from the registry: the enable response turns an empty list into null. + Assert.That(status.NotCapturableVariables, Is.Empty); + } + /// /// What: RevertAll after enable on a hot-reloaded async body restores instrumentation /// onto the compiled MoveNext and hits with the original (non-edited) return value. diff --git a/Assets/Tests/Editor/PausePointToolModeTests.cs b/Assets/Tests/Editor/PausePointToolModeTests.cs index 88120f8cba..f89ffb2b54 100644 --- a/Assets/Tests/Editor/PausePointToolModeTests.cs +++ b/Assets/Tests/Editor/PausePointToolModeTests.cs @@ -289,7 +289,7 @@ public void Hit_WhenMarkerHasMaxPreviewElementsOverride_UsesOverrideForCollectio List values = Enumerable.Range(0, maxPreviewElements + 5).ToList(); SourcePausePointCapture.Capture( - "jump", null, Array.Empty(), new object[] { "scores", values }); + "jump", null, Array.Empty(), Array.Empty(), new object[] { "scores", values }); UloopPausePointSnapshot snapshot = UloopPausePointRegistry.GetStatus("jump"); Assert.That(snapshot.CapturedVariables.Single().Value.Split(',').Length, Is.EqualTo(maxPreviewElements)); @@ -314,7 +314,7 @@ public void Capture_WhenHitWhenDoesNotMatch_SkipsCaptureAndRecordsSkipCount() parseResult.Condition); SourcePausePointCapture.Capture( - "jump", null, Array.Empty(), new object[] { "speed", 5 }); + "jump", null, Array.Empty(), Array.Empty(), new object[] { "speed", 5 }); UloopPausePointSnapshot snapshot = UloopPausePointRegistry.GetStatus("jump"); @@ -344,7 +344,7 @@ public void Capture_WhenHitWhenEvaluationErrors_RecordsErrorAndCapturesFrame() parseResult.Condition); SourcePausePointCapture.Capture( - "jump", null, Array.Empty(), new object[] { "speed", 5 }); + "jump", null, Array.Empty(), Array.Empty(), new object[] { "speed", 5 }); UloopPausePointSnapshot snapshot = UloopPausePointRegistry.GetStatus("jump"); @@ -494,7 +494,7 @@ public void Hit_WhenMaxCallerFramesIsZero_ReturnsEmptyCallerFramesArray() "jump", 30, UloopPausePointCaptureMode.SingleShot, 20, 10, 0); SourcePausePointCapture.Capture( - "jump", null, Array.Empty(), Array.Empty()); + "jump", null, Array.Empty(), Array.Empty(), Array.Empty()); UloopPausePointSnapshot snapshot = UloopPausePointRegistry.GetStatus("jump"); Assert.That(snapshot.CallerFrames, Is.Empty); diff --git a/Assets/Tests/Editor/SourcePausePointCapture/SourcePausePointCaptureTests.cs b/Assets/Tests/Editor/SourcePausePointCapture/SourcePausePointCaptureTests.cs index 30f8131195..d7a136bda7 100644 --- a/Assets/Tests/Editor/SourcePausePointCapture/SourcePausePointCaptureTests.cs +++ b/Assets/Tests/Editor/SourcePausePointCapture/SourcePausePointCaptureTests.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Linq; using System.Reflection; +using System.Runtime.CompilerServices; using System.Threading.Tasks; using NUnit.Framework; @@ -46,7 +47,7 @@ public void Capture_WhenPausePointIsEnabled_RecordsFormattedVariablesInSnapshot( object[] parameters = { "damage", 3 }; object[] locals = { "speed", 5 }; - SourcePausePointCapture.Capture("jump", null, parameters, locals); + SourcePausePointCapture.Capture("jump", null, Array.Empty(), parameters, locals); UloopPausePointSnapshot snapshot = UloopPausePointRegistry.GetStatus("jump"); Assert.That(snapshot.IsHit, Is.True); @@ -62,7 +63,7 @@ public void Capture_WhenPausePointIsEnabled_EmitsPausePointHitVibeLog() UloopPausePointRegistry.Enable("jump", 30); VibeLogger.ClearMemoryLogs(); - SourcePausePointCapture.Capture("jump", null, Array.Empty(), Array.Empty()); + SourcePausePointCapture.Capture("jump", null, Array.Empty(), Array.Empty(), Array.Empty()); string logs = VibeLogger.GetLogsForAi("pause_point_hit"); Assert.That(logs, Does.Contain("pause_point_hit")); @@ -74,7 +75,7 @@ public void Capture_WhenPausePointIsEnabled_EmitsPausePointHitVibeLog() public void Capture_WhenPausePointIsNotArmed_DoesNotPauseOrRecordAHit() { // Verifies the IsArmed fast path no-ops when the marker was never enabled. - SourcePausePointCapture.Capture("never-enabled", null, Array.Empty(), Array.Empty()); + SourcePausePointCapture.Capture("never-enabled", null, Array.Empty(), Array.Empty(), Array.Empty()); UloopPausePointSnapshot snapshot = UloopPausePointRegistry.GetStatus("never-enabled"); Assert.That(snapshot.IsHit, Is.False); @@ -86,9 +87,9 @@ public void Capture_WhenPausePointWasAlreadyHit_IgnoresSecondCall() { // Verifies a one-shot marker disarms itself so a second pass through the same line no-ops. UloopPausePointRegistry.Enable("jump", 30); - SourcePausePointCapture.Capture("jump", null, Array.Empty(), Array.Empty()); + SourcePausePointCapture.Capture("jump", null, Array.Empty(), Array.Empty(), Array.Empty()); - SourcePausePointCapture.Capture("jump", null, Array.Empty(), Array.Empty()); + SourcePausePointCapture.Capture("jump", null, Array.Empty(), Array.Empty(), Array.Empty()); Assert.That(_pauseController.PauseCount, Is.EqualTo(1)); } @@ -101,8 +102,8 @@ public void Capture_WhenContinuousPausePointIsEnabled_RecordsEveryFormattedHit() { UloopPausePointRegistry.Enable("jump", 30, UloopPausePointCaptureMode.Continuous, 20); - SourcePausePointCapture.Capture("jump", null, Array.Empty(), new object[] { "speed", 1 }); - SourcePausePointCapture.Capture("jump", null, Array.Empty(), new object[] { "speed", 2 }); + SourcePausePointCapture.Capture("jump", null, Array.Empty(), Array.Empty(), new object[] { "speed", 1 }); + SourcePausePointCapture.Capture("jump", null, Array.Empty(), Array.Empty(), new object[] { "speed", 2 }); UloopPausePointSnapshot snapshot = UloopPausePointRegistry.GetStatus("jump"); @@ -123,7 +124,7 @@ public IEnumerator Capture_WhenCalledOffMainThread_RecordsHitOnNextMainThreadTic UloopPausePointRegistry.Enable("jump", 30); object[] locals = { "speed", 5 }; - Task.Run(() => SourcePausePointCapture.Capture("jump", null, Array.Empty(), locals)); + Task.Run(() => SourcePausePointCapture.Capture("jump", null, Array.Empty(), Array.Empty(), locals)); float timeoutTime = Time.realtimeSinceStartup + 5f; UloopPausePointSnapshot snapshot = UloopPausePointRegistry.GetStatus("jump"); @@ -146,7 +147,7 @@ public void Collect_WithNormalInstance_AddsThisEntryReferencingTheInstance() NormalInstanceFixture instance = new(); UloopPausePointCapturedVariableFrame frame = SourcePausePointVariableCollector.Collect( - instance, Array.Empty(), Array.Empty()); + instance, Array.Empty(), Array.Empty(), Array.Empty()); UloopPausePointCapturedVariableEntry thisEntry = frame.Entries.Single(entry => entry.Name == "this"); Assert.That(thisEntry.Scope, Is.EqualTo(UloopCapturedVariableScope.This)); @@ -163,7 +164,7 @@ public void Collect_OrdersThisAfterLocalsAndParametersButBeforeInstanceFields() object[] parameters = { "damage", 3 }; UloopPausePointCapturedVariableFrame frame = SourcePausePointVariableCollector.Collect( - instance, parameters, locals); + instance, Array.Empty(), parameters, locals); List names = frame.Entries.Select(entry => entry.Name).ToList(); int thisIndex = names.IndexOf("this"); @@ -185,7 +186,7 @@ public void Collect_WithStateMachineInstance_ResolvesThisToOuterInstanceNotState outerThisField.SetValue(stateMachine, outer); UloopPausePointCapturedVariableFrame frame = SourcePausePointVariableCollector.Collect( - stateMachine, Array.Empty(), Array.Empty()); + stateMachine, Array.Empty(), Array.Empty(), Array.Empty()); UloopPausePointCapturedVariableEntry thisEntry = frame.Entries.Single(entry => entry.Name == "this"); Assert.That(thisEntry.Scope, Is.EqualTo(UloopCapturedVariableScope.This)); @@ -201,7 +202,7 @@ public void Collect_WithStateMachineInstanceMissingOuterThis_AddsNoThisEntry() (object stateMachine, _) = CreateStateMachine(); UloopPausePointCapturedVariableFrame frame = SourcePausePointVariableCollector.Collect( - stateMachine, Array.Empty(), Array.Empty()); + stateMachine, Array.Empty(), Array.Empty(), Array.Empty()); Assert.That(frame.Entries.Any(entry => entry.Name == "this"), Is.False); } @@ -224,7 +225,7 @@ public void Collect_WithShimStateMachineHoistingTheReceiver_ResolvesThisToTheRec receiverField.SetValue(stateMachine, receiver); UloopPausePointCapturedVariableFrame frame = SourcePausePointVariableCollector.Collect( - stateMachine, Array.Empty(), Array.Empty()); + stateMachine, Array.Empty(), Array.Empty(), Array.Empty()); AssertReceiverSurfacesAsThis(frame, receiver); } @@ -238,7 +239,7 @@ public void Collect_WithShimClosureCapturingTheReceiver_ResolvesThisToTheReceive Func closure = ShimShapedFixture.CaptureReceiver(receiver, 3); UloopPausePointCapturedVariableFrame frame = SourcePausePointVariableCollector.Collect( - closure.Target, Array.Empty(), Array.Empty()); + closure.Target, Array.Empty(), Array.Empty(), Array.Empty()); AssertReceiverSurfacesAsThis(frame, receiver); Assert.That(frame.Entries.Any(entry => entry.Name == "bonus"), Is.True); @@ -254,7 +255,7 @@ public void Collect_WithShimNestedClosure_FollowsTheEnclosingClosureToTheReceive Func closure = ShimShapedFixture.CaptureReceiverInLoop(receiver, 2); UloopPausePointCapturedVariableFrame frame = SourcePausePointVariableCollector.Collect( - closure.Target, Array.Empty(), Array.Empty()); + closure.Target, Array.Empty(), Array.Empty(), Array.Empty()); AssertReceiverSurfacesAsThis(frame, receiver); Assert.That(frame.Entries.Any(entry => entry.Name == "x"), Is.True); @@ -271,7 +272,7 @@ public void Collect_WithShimAsyncLambdaStateMachine_ResolvesThisThroughItsClosur object stateMachine = CreateAsyncLambdaStateMachine(asyncLambda.Target); UloopPausePointCapturedVariableFrame frame = SourcePausePointVariableCollector.Collect( - stateMachine, Array.Empty(), Array.Empty()); + stateMachine, Array.Empty(), Array.Empty(), Array.Empty()); AssertReceiverSurfacesAsThis(frame, receiver); Assert.That(frame.Entries.Any(entry => ReferenceEquals(entry.Value, stateMachine)), Is.False); @@ -287,7 +288,7 @@ public void Collect_WithCompiledAsyncLambdaStateMachine_ResolvesThisToTheInstanc object stateMachine = CreateAsyncLambdaStateMachine(asyncLambda.Target); UloopPausePointCapturedVariableFrame frame = SourcePausePointVariableCollector.Collect( - stateMachine, Array.Empty(), Array.Empty()); + stateMachine, Array.Empty(), Array.Empty(), Array.Empty()); AssertReceiverSurfacesAsThis(frame, instance); Assert.That(frame.Entries.Any(entry => entry.Name == "bonus"), Is.True); @@ -330,11 +331,68 @@ public void Collect_WithNullInstance_AddsNoThisEntry() object[] locals = { "speed", 5 }; UloopPausePointCapturedVariableFrame frame = SourcePausePointVariableCollector.Collect( - null, Array.Empty(), locals); + null, Array.Empty(), Array.Empty(), locals); Assert.That(frame.Entries.Any(entry => entry.Name == "this"), Is.False); } + [Test] + public void Collect_WithFramesAndNullInstance_ListsEveryFrameVariableWithoutThis() + { + // Verifies a static local function's closure structs each contribute their variables + // as parameters, and none of them surfaces as "this". + object[] closureFrames = { new OuterScopeFrameShape { offset = 2 }, new InnerScopeFrameShape { inner = 3 } }; + + UloopPausePointCapturedVariableFrame frame = SourcePausePointVariableCollector.Collect( + null, closureFrames, Array.Empty(), Array.Empty()); + + Assert.That(frame.Entries.Select(entry => entry.Name), Is.EqualTo(new[] { "offset", "inner" })); + Assert.That(frame.Entries.All(entry => entry.Scope == UloopCapturedVariableScope.Parameter), Is.True); + } + + [Test] + public void Collect_WithNormalInstanceAndFrame_ListsFrameVariablesBeforeThis() + { + // Verifies a closure struct's variables land after locals and parameters but before + // "this" and the instance fields, so the count cap keeps prioritizing variables. + NormalInstanceFixture instance = new() { Health = 7 }; + object[] closureFrames = { new OuterScopeFrameShape { offset = 2 } }; + object[] parameters = { "damage", 3 }; + + UloopPausePointCapturedVariableFrame frame = SourcePausePointVariableCollector.Collect( + instance, closureFrames, parameters, Array.Empty()); + + Assert.That( + frame.Entries.Select(entry => entry.Name), + Is.EqualTo(new[] { "damage", "offset", "this", "Health" })); + Assert.That(frame.Entries.Single(entry => entry.Name == "this").Value, Is.SameAs(instance)); + } + + [Test] + public void Collect_WithShimReceiverInLaterFrame_ResolvesThisFromThatFrame() + { + // Verifies a closure-class holder plus several closure structs lists every holder's + // variables and finds "this" through the receiver field of a struct that is not the + // first one, without listing the receiver field itself. + AsyncStateMachineFixture receiver = new() { OuterField = 7 }; + LoopClosureShape holder = new() { j = 1 }; + object[] closureFrames = + { + new OuterScopeFrameShape { offset = 2 }, + new ShimReceiverFrameShape { __uloopInstance = receiver, delta = 5 }, + }; + + UloopPausePointCapturedVariableFrame frame = SourcePausePointVariableCollector.Collect( + holder, closureFrames, Array.Empty(), Array.Empty()); + + AssertReceiverSurfacesAsThis(frame, receiver); + foreach (string name in new[] { "j", "offset", "delta" }) + { + UloopPausePointCapturedVariableEntry entry = frame.Entries.Single(e => e.Name == name); + Assert.That(entry.Scope, Is.EqualTo(UloopCapturedVariableScope.Parameter), name); + } + } + [Test] public void Collect_WhenCountCapReachedBeforeThis_OmitsThisAndReportsTruncated() { @@ -351,7 +409,7 @@ public void Collect_WhenCountCapReachedBeforeThis_OmitsThisAndReportsTruncated() NormalInstanceFixture instance = new(); UloopPausePointCapturedVariableFrame frame = SourcePausePointVariableCollector.Collect( - instance, Array.Empty(), locals); + instance, Array.Empty(), Array.Empty(), locals); Assert.That(frame.Entries.Count, Is.EqualTo(SourcePausePointConstants.MaxCapturedVariableCount)); Assert.That(frame.Entries.Any(entry => entry.Name == "this"), Is.False); @@ -374,7 +432,7 @@ public void Collect_WhenVariableCountExceedsCap_ReportsTruncatedNamesUpToLimitAn } UloopPausePointCapturedVariableFrame frame = SourcePausePointVariableCollector.Collect( - null, Array.Empty(), locals); + null, Array.Empty(), Array.Empty(), locals); Assert.That(frame.Entries.Count, Is.EqualTo(SourcePausePointConstants.MaxCapturedVariableCount)); Assert.That(frame.Truncated, Is.True); @@ -390,7 +448,7 @@ public void Collect_WhenUnderCountCap_ReportsEmptyTruncatedNames() object[] locals = { "speed", 5, "damage", 3 }; UloopPausePointCapturedVariableFrame frame = SourcePausePointVariableCollector.Collect( - null, Array.Empty(), locals); + null, Array.Empty(), Array.Empty(), locals); Assert.That(frame.Truncated, Is.False); Assert.That(frame.TruncatedVariableCount, Is.EqualTo(0)); @@ -407,7 +465,7 @@ public void Collect_WithAutoPropertyBackingField_CapturesUnmangledPropertyNameAs instance.SetVerticalVelocity(4.5f); UloopPausePointCapturedVariableFrame frame = SourcePausePointVariableCollector.Collect( - instance, Array.Empty(), Array.Empty()); + instance, Array.Empty(), Array.Empty(), Array.Empty()); UloopPausePointCapturedVariableEntry entry = frame.Entries.Single(entry => entry.Name == "VerticalVelocity"); Assert.That(entry.Scope, Is.EqualTo(UloopCapturedVariableScope.InstanceField)); @@ -428,6 +486,33 @@ private sealed class NormalInstanceFixture public int Health; } + // Shapes of the compiler-generated holders a local function receives: a lambda's closure + // class, and the closure structs passed by reference. Field names follow the compiler's. + [CompilerGenerated] + private sealed class LoopClosureShape + { + public int j; + } + + [CompilerGenerated] + private struct OuterScopeFrameShape + { + public int offset; + } + + [CompilerGenerated] + private struct InnerScopeFrameShape + { + public int inner; + } + + [CompilerGenerated] + private struct ShimReceiverFrameShape + { + public AsyncStateMachineFixture __uloopInstance; + public int delta; + } + private sealed class AutoPropertyFixture { public float VerticalVelocity { get; private set; } diff --git a/Assets/Tests/Editor/SourcePausePointCapture/SourcePausePointTruncationAggregateTests.cs b/Assets/Tests/Editor/SourcePausePointCapture/SourcePausePointTruncationAggregateTests.cs index 1e9d63fa8f..24f3bf438b 100644 --- a/Assets/Tests/Editor/SourcePausePointCapture/SourcePausePointTruncationAggregateTests.cs +++ b/Assets/Tests/Editor/SourcePausePointCapture/SourcePausePointTruncationAggregateTests.cs @@ -27,7 +27,7 @@ public void CaptureFrame_WhenOnlyPreviewIsClipped_ReportsThatNameAndCountOne() object[] locals = { "longText", longValue, "hp", 42 }; (UloopPausePointCapturedVariableFrame frame, List variables, bool truncated) = - SourcePausePointCapture.CaptureFrame(null, Array.Empty(), locals); + SourcePausePointCapture.CaptureFrame(null, Array.Empty(), Array.Empty(), locals); Assert.That(truncated, Is.True); Assert.That(frame.Truncated, Is.True); @@ -51,7 +51,7 @@ public void CaptureFrame_WhenOnlyCountCapDropsVariables_ReportsTwentyNamesAndExa } (UloopPausePointCapturedVariableFrame frame, _, bool truncated) = - SourcePausePointCapture.CaptureFrame(null, Array.Empty(), locals); + SourcePausePointCapture.CaptureFrame(null, Array.Empty(), Array.Empty(), locals); Assert.That(truncated, Is.True); Assert.That(frame.TruncatedVariableCount, Is.EqualTo(25)); @@ -98,7 +98,7 @@ public void CaptureFrame_WhenPreviewClipAndCountCapCombine_UnionsNamesInCaptureO } (UloopPausePointCapturedVariableFrame frame, _, bool truncated) = - SourcePausePointCapture.CaptureFrame(null, Array.Empty(), locals); + SourcePausePointCapture.CaptureFrame(null, Array.Empty(), Array.Empty(), locals); Assert.That(truncated, Is.True); Assert.That(frame.TruncatedVariableCount, Is.EqualTo(4)); @@ -116,7 +116,7 @@ public void CaptureFrame_WhenNothingIsTruncated_ReportsEmptyAggregate() object[] locals = { "speed", 5, "damage", 3 }; (UloopPausePointCapturedVariableFrame frame, _, bool truncated) = - SourcePausePointCapture.CaptureFrame(null, Array.Empty(), locals); + SourcePausePointCapture.CaptureFrame(null, Array.Empty(), Array.Empty(), locals); Assert.That(truncated, Is.False); Assert.That(frame.Truncated, Is.False); @@ -136,7 +136,7 @@ public void CaptureFrame_WhenClippedValueHasEmptyName_CountsItWithoutReportingAN LogAssert.Expect(LogType.Assert, "name must not be null or empty"); (UloopPausePointCapturedVariableFrame frame, _, bool truncated) = - SourcePausePointCapture.CaptureFrame(null, Array.Empty(), locals); + SourcePausePointCapture.CaptureFrame(null, Array.Empty(), Array.Empty(), locals); Assert.That(truncated, Is.True); Assert.That(frame.Truncated, Is.True); @@ -159,7 +159,7 @@ public void CaptureFrame_WhenPreviewClipsExceedNameCap_ReportsFirstTwentyNamesAn } (UloopPausePointCapturedVariableFrame frame, _, bool truncated) = - SourcePausePointCapture.CaptureFrame(null, Array.Empty(), locals); + SourcePausePointCapture.CaptureFrame(null, Array.Empty(), Array.Empty(), locals); Assert.That(truncated, Is.True); Assert.That(frame.TruncatedVariableCount, Is.EqualTo(21)); diff --git a/Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherClosureFrameLocalFunctionFixture.cs b/Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherClosureFrameLocalFunctionFixture.cs new file mode 100644 index 0000000000..e2d1d474a3 --- /dev/null +++ b/Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherClosureFrameLocalFunctionFixture.cs @@ -0,0 +1,89 @@ +// FROZEN FIXTURE: content and line numbers are asserted by SourcePausePointClosureFrameCaptureTests. +// Do not reformat or edit this file; add a new fixture file instead. +using System; + +namespace io.github.hatayama.UnityCliLoop.Tests.SourcePausePointPatcherFixtures +{ + internal sealed class PatcherClosureFrameLocalFunctionFixture + { + private readonly int _value; + + public PatcherClosureFrameLocalFunctionFixture(int value) + { + _value = value; + } + + public int SumInLoopClosure(int delta) + { + int total = 0; + for (int j = 0; j < 3; j++) + { + Func read = () => j; + int Local() => _value + delta + j; + total += Local() + read(); + } + + return total; + } + + public int SumWithoutClosureClass(int delta) + { + int offset = delta * 2; + return Local(); + + int Local() + { + int sum = _value + offset; + return sum; + } + } + + public static int SumInStaticMethod(int delta) + { + int offset = delta * 2; + return Local(); + + int Local() + { + int sum = offset + 1; + return sum; + } + } + + public static int SumAcrossNestedScopes(int delta) + { + int offset = delta * 2; + if (delta > 0) + { + int inner = delta + 1; + return Local(); + + int Local() + { + int sum = offset + inner; + return sum; + } + } + + return 0; + } + + public int SumWithParameterAcrossNestedScopes(int delta) + { + int offset = delta * 2; + if (delta > 0) + { + int inner = delta + 1; + return Local(100); + + int Local(int extra) + { + int sum = _value + offset + inner + extra; + return sum; + } + } + + return 0; + } + } +} diff --git a/Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherClosureFrameLocalFunctionFixture.cs.meta b/Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherClosureFrameLocalFunctionFixture.cs.meta new file mode 100644 index 0000000000..3aa2134344 --- /dev/null +++ b/Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherClosureFrameLocalFunctionFixture.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 0c01bcd3c8d17403b8522eef32d050ff +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Assets/Tests/Editor/SourcePausePointPatcher/SourcePausePointClosureFrameCaptureTests.cs b/Assets/Tests/Editor/SourcePausePointPatcher/SourcePausePointClosureFrameCaptureTests.cs new file mode 100644 index 0000000000..471982a587 --- /dev/null +++ b/Assets/Tests/Editor/SourcePausePointPatcher/SourcePausePointClosureFrameCaptureTests.cs @@ -0,0 +1,229 @@ +using System; +using System.Linq; + +using NUnit.Framework; + +using io.github.hatayama.UnityCliLoop.FirstPartyTools; +using io.github.hatayama.UnityCliLoop.Runtime; +using io.github.hatayama.UnityCliLoop.Tests.SourcePausePointPatcherFixtures; + +namespace io.github.hatayama.UnityCliLoop.Tests.Editor +{ + /// + /// Verifies that a pause point inside a local function captures the variables and the + /// instance its enclosing scopes pass to it in by-ref closure structs, whichever shape the + /// compiler gave the local function. + /// + [TestFixture] + public sealed class SourcePausePointClosureFrameCaptureTests + { + private const string FixturePath = + "Assets/Tests/Editor/SourcePausePointPatcher/Fixtures/PatcherClosureFrameLocalFunctionFixture.cs"; + private const int LoopClosureLocalFunctionLine = 22; + private const int InstanceLocalFunctionReturnLine = 37; + private const int StaticLocalFunctionReturnLine = 49; + private const int NestedScopesLocalFunctionReturnLine = 64; + private const int ParameterNestedScopesLocalFunctionReturnLine = 82; + + private FakePausePointPauseController _pauseController; + private HotReloadSidePortScope _hotReloadSideScope; + + [SetUp] + public void SetUp() + { + _pauseController = new FakePausePointPauseController(); + UloopPausePointRegistry.ConfigureForTests(_pauseController, () => DateTime.UtcNow); + _hotReloadSideScope = new HotReloadSidePortScope(); + } + + [TearDown] + public void TearDown() + { + _hotReloadSideScope.Dispose(); + SourcePausePointPatcher.UnpatchAll(); + UloopPausePointRegistry.ResetForTests(); + } + + [Test] + public void LocalFunctionOfLoopClosureClass_CapturesThisAndEveryEnclosingScope() + { + // Verifies a local function compiled as an instance method of a loop closure class, + // with the instance and the method parameters in a by-ref closure struct, captures + // "this", the instance fields, the method parameter, and the loop variable. + UloopPausePointSnapshot snapshot = EnableAndHit( + "closure-frame-loop", LoopClosureLocalFunctionLine, + () => Assert.That(new PatcherClosureFrameLocalFunctionFixture(10).SumInLoopClosure(1), Is.EqualTo(39))); + + AssertCaptured(snapshot, "this", UloopCapturedVariableScope.This); + Assert.That(AssertCaptured(snapshot, "_value", UloopCapturedVariableScope.InstanceField).Value, Is.EqualTo("10")); + Assert.That(AssertCaptured(snapshot, "delta", UloopCapturedVariableScope.Parameter).Value, Is.EqualTo("1")); + Assert.That(AssertCaptured(snapshot, "j", UloopCapturedVariableScope.Parameter).Value, Is.EqualTo("0")); + } + + [Test] + public void LocalFunctionOfInstanceMethod_CapturesThisAndTheEnclosingLocals() + { + // Verifies a local function compiled as an instance method of the declaring class, + // with the enclosing locals in a by-ref closure struct, captures those locals + // alongside "this" and its own locals. + UloopPausePointSnapshot snapshot = EnableAndHit( + "closure-frame-instance", InstanceLocalFunctionReturnLine, + () => Assert.That(new PatcherClosureFrameLocalFunctionFixture(10).SumWithoutClosureClass(1), Is.EqualTo(12))); + + AssertCaptured(snapshot, "this", UloopCapturedVariableScope.This); + Assert.That(AssertCaptured(snapshot, "_value", UloopCapturedVariableScope.InstanceField).Value, Is.EqualTo("10")); + Assert.That(AssertCaptured(snapshot, "offset", UloopCapturedVariableScope.Parameter).Value, Is.EqualTo("2")); + Assert.That(AssertCaptured(snapshot, "sum", UloopCapturedVariableScope.Local).Value, Is.EqualTo("12")); + } + + [Test] + public void LocalFunctionOfStaticMethod_CapturesTheEnclosingLocalsWithoutThis() + { + // Verifies a static local function receiving only a by-ref closure struct still + // captures the enclosing locals and reports no "this". + UloopPausePointSnapshot snapshot = EnableAndHit( + "closure-frame-static", StaticLocalFunctionReturnLine, + () => Assert.That(PatcherClosureFrameLocalFunctionFixture.SumInStaticMethod(1), Is.EqualTo(3))); + + Assert.That(snapshot.CapturedVariables.Any(v => v.Name == "this"), Is.False, FormatCaptured(snapshot)); + Assert.That(AssertCaptured(snapshot, "offset", UloopCapturedVariableScope.Parameter).Value, Is.EqualTo("2")); + Assert.That(AssertCaptured(snapshot, "sum", UloopCapturedVariableScope.Local).Value, Is.EqualTo("3")); + } + + [Test] + public void LocalFunctionAcrossNestedScopes_CapturesEveryClosureFrame() + { + // Verifies a static local function receiving one closure struct per enclosing scope + // captures the variables of every scope, not only those of the first struct. + UloopPausePointSnapshot snapshot = EnableAndHit( + "closure-frame-nested", NestedScopesLocalFunctionReturnLine, + () => Assert.That(PatcherClosureFrameLocalFunctionFixture.SumAcrossNestedScopes(1), Is.EqualTo(4))); + + Assert.That(snapshot.CapturedVariables.Any(v => v.Name == "this"), Is.False, FormatCaptured(snapshot)); + Assert.That(AssertCaptured(snapshot, "offset", UloopCapturedVariableScope.Parameter).Value, Is.EqualTo("2")); + Assert.That(AssertCaptured(snapshot, "inner", UloopCapturedVariableScope.Parameter).Value, Is.EqualTo("2")); + Assert.That(AssertCaptured(snapshot, "sum", UloopCapturedVariableScope.Local).Value, Is.EqualTo("4")); + } + + [Test] + public void LocalFunctionWithParameterAcrossNestedScopes_CapturesEveryFrameAndParameter() + { + // Verifies an instance local function whose closure structs follow its own declared + // parameter captures that parameter and every struct, each read from its own argument. + UloopPausePointSnapshot snapshot = EnableAndHit( + "closure-frame-parameter", ParameterNestedScopesLocalFunctionReturnLine, + () => Assert.That( + new PatcherClosureFrameLocalFunctionFixture(10).SumWithParameterAcrossNestedScopes(1), + Is.EqualTo(114))); + + AssertCaptured(snapshot, "this", UloopCapturedVariableScope.This); + Assert.That(AssertCaptured(snapshot, "_value", UloopCapturedVariableScope.InstanceField).Value, Is.EqualTo("10")); + Assert.That(AssertCaptured(snapshot, "extra", UloopCapturedVariableScope.Parameter).Value, Is.EqualTo("100")); + Assert.That(AssertCaptured(snapshot, "offset", UloopCapturedVariableScope.Parameter).Value, Is.EqualTo("2")); + Assert.That(AssertCaptured(snapshot, "inner", UloopCapturedVariableScope.Parameter).Value, Is.EqualTo("2")); + Assert.That(AssertCaptured(snapshot, "sum", UloopCapturedVariableScope.Local).Value, Is.EqualTo("114")); + } + + [Test] + public void LocalFunctionAcrossNestedScopes_EvaluatesHitWhenOnFrameVariable() + { + // Verifies a hit-when condition on a variable that only a closure struct carries is + // evaluated against its value: a non-matching call is skipped, a matching one hits. + const string id = "closure-frame-hit-when"; + const string hitWhen = "inner == 3"; + UloopPausePointHitWhenParseResult parseResult = UloopPausePointHitWhenCondition.Parse(hitWhen); + Assert.That(parseResult.Condition, Is.Not.Null, parseResult.ErrorMessage); + UloopPausePointRegistry.Enable( + id, + 30, + UloopPausePointCaptureMode.SingleShot, + UloopPausePointRegistry.DefaultMaxHistory, + UloopPausePointRegistry.DefaultMaxPreviewElements, + UloopPausePointRegistry.DefaultMaxCallerFrames, + hitWhen, + parseResult.Condition); + ResolveAndPatch(id, ParameterNestedScopesLocalFunctionReturnLine); + PatcherClosureFrameLocalFunctionFixture fixture = new PatcherClosureFrameLocalFunctionFixture(10); + + fixture.SumWithParameterAcrossNestedScopes(1); + + UloopPausePointSnapshot skipped = UloopPausePointRegistry.GetStatus(id); + Assert.That(skipped.IsHit, Is.False, skipped.HitWhenErrorNote); + Assert.That(skipped.HitWhenSkippedCount, Is.EqualTo(1)); + Assert.That(skipped.HitWhenErrorNote, Is.Empty); + + fixture.SumWithParameterAcrossNestedScopes(2); + + UloopPausePointSnapshot hit = UloopPausePointRegistry.GetStatus(id); + Assert.That(hit.IsHit, Is.True); + Assert.That(AssertCaptured(hit, "inner", UloopCapturedVariableScope.Parameter).Value, Is.EqualTo("3")); + } + + [TestCase(LoopClosureLocalFunctionLine)] + [TestCase(InstanceLocalFunctionReturnLine)] + [TestCase(StaticLocalFunctionReturnLine)] + [TestCase(NestedScopesLocalFunctionReturnLine)] + [TestCase(ParameterNestedScopesLocalFunctionReturnLine)] + public void Resolve_InLocalFunctionTakingClosureStructs_ReportsNoNotCapturableParameter(int line) + { + // Verifies a closure struct argument, whose variables are captured through it, is not + // also reported as a ref parameter that cannot be captured. + SourcePausePointResolveResult resolveResult = SourcePausePointResolver.Resolve(FixturePath, line); + + Assert.That(resolveResult.Success, Is.True, resolveResult.ErrorMessage); + Assert.That(resolveResult.Resolution.NotCapturableVariables, Is.Empty); + } + + private static UloopPausePointSnapshot EnableAndHit(string id, int line, Action invoke) + { + UloopPausePointRegistry.Enable(id, 30); + ResolveAndPatch(id, line); + + invoke(); + + UloopPausePointSnapshot snapshot = UloopPausePointRegistry.GetStatus(id); + Assert.That(snapshot.IsHit, Is.True); + return snapshot; + } + + private static void ResolveAndPatch(string id, int line) + { + SourcePausePointResolveResult resolveResult = SourcePausePointResolver.Resolve(FixturePath, line); + Assert.That(resolveResult.Success, Is.True, resolveResult.ErrorMessage); + Assert.That(resolveResult.Resolution.ResolvedLine, Is.EqualTo(line)); + + SourcePausePointPatchResult patchResult = SourcePausePointPatcher.Patch(id, resolveResult.Resolution); + Assert.That(patchResult.Success, Is.True, patchResult.ErrorMessage); + } + + private static UloopCapturedVariable AssertCaptured(UloopPausePointSnapshot snapshot, string name, string scope) + { + UloopCapturedVariable variable = snapshot.CapturedVariables.FirstOrDefault(v => v.Name == name); + Assert.That(variable, Is.Not.Null, $"'{name}' was not captured: {FormatCaptured(snapshot)}"); + Assert.That(variable.Scope, Is.EqualTo(scope), FormatCaptured(snapshot)); + return variable; + } + + private static string FormatCaptured(UloopPausePointSnapshot snapshot) + { + return string.Join(", ", snapshot.CapturedVariables.Select(v => $"{v.Name}({v.Scope})={v.Value}")); + } + + private sealed class FakePausePointPauseController : IUloopPausePointPauseController + { + public int PauseCount { get; private set; } + public bool IsPlaying => true; + public bool IsPaused => PauseCount > 0; + + public void Pause() + { + PauseCount++; + } + + public void Resume() + { + PauseCount = 0; + } + } + } +} diff --git a/Assets/Tests/Editor/SourcePausePointPatcher/SourcePausePointClosureFrameCaptureTests.cs.meta b/Assets/Tests/Editor/SourcePausePointPatcher/SourcePausePointClosureFrameCaptureTests.cs.meta new file mode 100644 index 0000000000..fab17271f4 --- /dev/null +++ b/Assets/Tests/Editor/SourcePausePointPatcher/SourcePausePointClosureFrameCaptureTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: fa68c064a871a4f879ffb43289bec97a +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointCapture.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointCapture.cs index a8c3f5e4d9..44494644bc 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointCapture.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointCapture.cs @@ -14,9 +14,14 @@ namespace io.github.hatayama.UnityCliLoop.FirstPartyTools internal static class SourcePausePointCapture { public static void Capture( - string id, object instance, object[] parameterNamesAndValues, object[] localNamesAndValues) + string id, + object instance, + object[] closureFrames, + object[] parameterNamesAndValues, + object[] localNamesAndValues) { Debug.Assert(!string.IsNullOrEmpty(id), "id must not be null or empty"); + Debug.Assert(closureFrames != null, "closureFrames must not be null"); Debug.Assert(parameterNamesAndValues != null, "parameterNamesAndValues must not be null"); Debug.Assert(localNamesAndValues != null, "localNamesAndValues must not be null"); @@ -26,7 +31,7 @@ public static void Capture( } UloopPausePointCapturedVariableFrame frame = SourcePausePointVariableCollector.Collect( - instance, parameterNamesAndValues, localNamesAndValues); + instance, closureFrames, parameterNamesAndValues, localNamesAndValues); UloopPausePointHitWhenCondition condition = UloopPausePointRegistry.GetHitWhenCondition(id); if (condition != null) { @@ -83,11 +88,11 @@ private static void LogHit(UloopPausePointSnapshot snapshot, bool truncated) internal static (UloopPausePointCapturedVariableFrame Frame, List Variables, bool Truncated) CaptureFrame( - object instance, object[] parameterNamesAndValues, object[] localNamesAndValues, + object instance, object[] closureFrames, object[] parameterNamesAndValues, object[] localNamesAndValues, int maxPreviewElements = SourcePausePointConstants.MaxCollectionPreviewElementCount) { UloopPausePointCapturedVariableFrame frame = SourcePausePointVariableCollector.Collect( - instance, parameterNamesAndValues, localNamesAndValues); + instance, closureFrames, parameterNamesAndValues, localNamesAndValues); return FormatFrame(frame, maxPreviewElements); } diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointCaptureEligibility.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointCaptureEligibility.cs index 8277657729..08b4a0c544 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointCaptureEligibility.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointCaptureEligibility.cs @@ -16,6 +16,9 @@ namespace io.github.hatayama.UnityCliLoop.FirstPartyTools /// internal static class SourcePausePointCaptureEligibility { + private const string CompilerGeneratedAttributeFullName = + "System.Runtime.CompilerServices.CompilerGeneratedAttribute"; + internal static List CollectCapturableLocals( MethodDefinition method, int offset) @@ -164,7 +167,9 @@ private static bool IsCaptureExcluded(TypeReference type) // byref locals/parameters (ref, out, in) and pointers cannot be boxed; ref structs // (Span, and any user-defined "ref struct") cannot be boxed either. // Why derived from the reason: the excluded set and the reported set must stay the - // same set, so a parameter is never both captured and named as not capturable. + // same set, so a parameter is never both captured and named as not capturable. The one + // exception is a by-ref closure struct, which is excluded but not reported because its + // variables are captured through it (see SourcePausePointClosureFrameArgument). return DescribeNotCapturableReason(type).Length > 0; } @@ -221,6 +226,11 @@ internal static List CollectNotCapturableParameters(MethodDefinition met List results = new List(); foreach (ParameterDefinition parameter in method.Parameters) { + if (IsClosureFrameParameter(parameter.ParameterType)) + { + continue; + } + string reason = DescribeNotCapturableReason(parameter.ParameterType); if (reason.Length == 0) { @@ -302,6 +312,11 @@ internal static List CollectNotCapturableParametersFromReflection( for (int index = startIndex; index < runtimeParameters.Length; index++) { System.Reflection.ParameterInfo parameter = runtimeParameters[index]; + if (SourcePausePointClosureFrameArgument.FrameTypeOrNull(parameter.ParameterType) != null) + { + continue; + } + string reason = DescribeNotCapturableReasonFromReflection(parameter.ParameterType); if (reason.Length == 0) { @@ -314,6 +329,34 @@ internal static List CollectNotCapturableParametersFromReflection( return results; } + // Cecil counterpart of SourcePausePointClosureFrameArgument.FrameTypeOrNull. A closure + // struct is always declared in the module being read, so its reference is already a + // TypeDefinition and no assembly has to be resolved. + private static bool IsClosureFrameParameter(TypeReference type) + { + if (!(type is ByReferenceType byReferenceType)) + { + return false; + } + + if (!(byReferenceType.ElementType is TypeDefinition elementType) + || !elementType.IsValueType + || !elementType.HasCustomAttributes) + { + return false; + } + + foreach (CustomAttribute attribute in elementType.CustomAttributes) + { + if (attribute.AttributeType.FullName == CompilerGeneratedAttributeFullName) + { + return true; + } + } + + return false; + } + /// /// The reason a Cecil type is excluded from capture, or empty when it is capturable. /// diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointClosureFrameArgument.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointClosureFrameArgument.cs index 4a14373d6e..32714a8f52 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointClosureFrameArgument.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointClosureFrameArgument.cs @@ -1,54 +1,37 @@ using System; +using System.Collections.Generic; using System.Reflection; using System.Runtime.CompilerServices; -using io.github.hatayama.UnityCliLoop.ToolContracts; - namespace io.github.hatayama.UnityCliLoop.FirstPartyTools { /// - /// Finds the closure struct a static local function receives by reference, so a pause point in - /// it can capture the variables the local function shares with its enclosing method. + /// Finds the closure structs a local function receives by reference, so a pause point in it + /// can capture the variables and the instance it shares with its enclosing scopes. /// /// /// A local function that captures variables but is not converted to a delegate is compiled to a - /// method taking its enclosing scopes as by-ref struct arguments. When the enclosing method is a - /// hot-reload shim, the instance travels in that struct as the shim's receiver field, so the - /// local function is static and has no `this` of its own. + /// method taking one by-ref struct argument per enclosing scope it uses, after its own declared + /// parameters. This happens whether the local function itself is static, an instance method of + /// the declaring class, or an instance method of a lambda's closure class. The instance often + /// travels inside one of those structs, as `<>4__this` or as a hot-reload shim's receiver field. /// internal static class SourcePausePointClosureFrameArgument { - private const string OuterThisFieldName = "<>4__this"; - - /// - /// The GetParameters() index of the closure struct to capture, or -1 when there is none. - /// The struct that holds the instance wins, because the instance is what the pause point - /// most needs; otherwise the first one. - /// - internal static int FindIndexOrMinusOne(MethodBase method) + /// The GetParameters() indexes of every closure struct argument, in declaration order. + internal static List FindIndexes(MethodBase method) { System.Reflection.ParameterInfo[] parameters = method.GetParameters(); - int firstFrameIndex = -1; + List indexes = new List(); for (int index = 0; index < parameters.Length; index++) { - Type frameType = FrameTypeOrNull(parameters[index].ParameterType); - if (frameType == null) - { - continue; - } - - if (HoldsInstance(frameType)) + if (FrameTypeOrNull(parameters[index].ParameterType) != null) { - return index; - } - - if (firstFrameIndex < 0) - { - firstFrameIndex = index; + indexes.Add(index); } } - return firstFrameIndex; + return indexes; } /// The struct type behind a by-ref closure argument, or null for any other parameter. @@ -67,12 +50,5 @@ internal static Type FrameTypeOrNull(Type parameterType) return Attribute.IsDefined(elementType, typeof(CompilerGeneratedAttribute)) ? elementType : null; } - - private static bool HoldsInstance(Type frameType) - { - const BindingFlags instanceFields = BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic; - return frameType.GetField(HotReloadShimMethodLookup.ShimReceiverParameterName, instanceFields) != null - || frameType.GetField(OuterThisFieldName, instanceFields) != null; - } } } diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointInjectionEmitter.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointInjectionEmitter.cs index 5b44ccc836..db41f4d3c6 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointInjectionEmitter.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointInjectionEmitter.cs @@ -157,6 +157,7 @@ private static List BuildInjection(SourcePausePointPatchInjecti }; AppendInstanceLoad(emitted, injection, method); + AppendClosureFramesLoad(emitted, method); // InstanceFromFirstArgument stores absolute GetParameters() indexes (including the // hole left by skipping __uloopInstance at 0), so argOffset stays 0. Ordinary @@ -252,7 +253,7 @@ private static void AppendInstanceLoad(List emitted, SourcePaus if (injection.IsStatic) { - AppendStaticInstanceLoad(emitted, method); + emitted.Add(new CodeInstruction(OpCodes.Ldnull)); return; } @@ -276,24 +277,34 @@ private static void AppendInstanceLoad(List emitted, SourcePaus } } - // Why a static method may still pass an instance: a local function receives its enclosing - // scopes as a by-ref closure struct, and the collector walks that struct to the variables - // and the instance it holds, as it does for a lambda's closure class. The struct is copied - // into the box, so the capture is a snapshot and cannot write back into the frame. - private static void AppendStaticInstanceLoad(List emitted, MethodBase method) + // Why pass the by-ref closure structs separately from the instance: a local function + // receives the variables of its enclosing scopes, and often the instance, in those structs, + // and the collector walks them the way it walks a lambda's closure class. They cannot be + // captured as parameters because by-ref arguments cannot be boxed as they are; each struct + // is copied into a box, so the capture is a snapshot and cannot write back into the frame. + private static void AppendClosureFramesLoad(List emitted, MethodBase method) { - int frameIndex = SourcePausePointClosureFrameArgument.FindIndexOrMinusOne(method); - if (frameIndex < 0) + List frameIndexes = SourcePausePointClosureFrameArgument.FindIndexes(method); + // Fully qualify: ToolContracts also defines ParameterInfo (tool schema DTO). + System.Reflection.ParameterInfo[] parameters = method.GetParameters(); + // GetParameters() omits the hidden `this` that occupies argument slot 0 of an instance + // method. Decided by the patched method itself: a hot-reload shim taking the instance as + // its first declared parameter is static, so its indexes already match its slots. + int slotOffset = method.IsStatic ? 0 : 1; + + emitted.Add(new CodeInstruction(OpCodes.Ldc_I4, frameIndexes.Count)); + emitted.Add(new CodeInstruction(OpCodes.Newarr, typeof(object))); + for (int i = 0; i < frameIndexes.Count; i++) { - emitted.Add(new CodeInstruction(OpCodes.Ldnull)); - return; + int frameIndex = frameIndexes[i]; + Type frameType = SourcePausePointClosureFrameArgument.FrameTypeOrNull(parameters[frameIndex].ParameterType); + emitted.Add(new CodeInstruction(OpCodes.Dup)); + emitted.Add(new CodeInstruction(OpCodes.Ldc_I4, i)); + emitted.Add(CodeInstruction.LoadArgument(frameIndex + slotOffset, false)); + emitted.Add(new CodeInstruction(OpCodes.Ldobj, frameType)); + emitted.Add(new CodeInstruction(OpCodes.Box, frameType)); + emitted.Add(new CodeInstruction(OpCodes.Stelem_Ref)); } - - Type frameType = SourcePausePointClosureFrameArgument.FrameTypeOrNull( - method.GetParameters()[frameIndex].ParameterType); - emitted.Add(CodeInstruction.LoadArgument(frameIndex, false)); - emitted.Add(new CodeInstruction(OpCodes.Ldobj, frameType)); - emitted.Add(new CodeInstruction(OpCodes.Box, frameType)); } private static void AppendNameValueArray( diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointVariableCollector.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointVariableCollector.cs index fa5e19c788..84f4c24ded 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointVariableCollector.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointVariableCollector.cs @@ -24,9 +24,15 @@ internal static class SourcePausePointVariableCollector // "this", so this never collides with a captured local, parameter, or field. private const string ThisEntryName = "this"; + /// + /// Lists locals, then parameters, then the variables the compiler-generated holders carry, + /// then the instance as "this" followed by its fields. The holders are the instance when it + /// is a closure or state machine, and every closure struct in closureFrames. + /// public static UloopPausePointCapturedVariableFrame Collect( - object instance, object[] parameterNamesAndValues, object[] localNamesAndValues) + object instance, object[] closureFrames, object[] parameterNamesAndValues, object[] localNamesAndValues) { + Debug.Assert(closureFrames != null, "closureFrames must not be null"); Debug.Assert(parameterNamesAndValues != null, "parameterNamesAndValues must not be null"); Debug.Assert(localNamesAndValues != null, "localNamesAndValues must not be null"); Debug.Assert(parameterNamesAndValues.Length % 2 == 0, "parameterNamesAndValues must contain name/value pairs"); @@ -47,12 +53,9 @@ public static UloopPausePointCapturedVariableFrame Collect( entries, capturedNames, truncatedVariableNames, ref truncatedVariableCount, ref truncated, parameterNamesAndValues, UloopCapturedVariableScope.Parameter); - if (instance != null) - { - CollectInstanceFieldVariables( - instance, entries, capturedNames, truncatedVariableNames, ref truncatedVariableCount, - ref truncated); - } + CollectHolderAndInstanceVariables( + instance, closureFrames, entries, capturedNames, truncatedVariableNames, ref truncatedVariableCount, + ref truncated); return new UloopPausePointCapturedVariableFrame( entries, truncated, truncatedVariableNames, truncatedVariableCount); @@ -77,39 +80,59 @@ private static void AppendPairs( } } - private static void CollectInstanceFieldVariables( + private static void CollectHolderAndInstanceVariables( object instance, + object[] closureFrames, List entries, HashSet capturedNames, List truncatedVariableNames, ref int truncatedVariableCount, ref bool truncated) { - // Normal method: the paused instance itself is `this`, emitted before its fields so the - // count cap keeps prioritizing locals and parameters over instance state. - if (!IsCompilerGeneratedHolder(instance.GetType())) + // A normal instance is `this` itself. A compiler-generated instance (closure or state + // machine) is only a holder: the real `this` is found through its links, never the + // holder itself. + object receiver = instance != null && !IsCompilerGeneratedHolder(instance.GetType()) ? instance : null; + object outerThis = null; + if (instance != null && receiver == null) { - TryAppendEntry( - entries, capturedNames, truncatedVariableNames, ref truncatedVariableCount, ref truncated, - ThisEntryName, UloopCapturedVariableScope.This, instance); + object linkedOuterThis = CollectDirectFieldVariables( + instance, entries, capturedNames, truncatedVariableNames, ref truncatedVariableCount, + ref truncated, followOuterThis: true); + outerThis ??= linkedOuterThis; + } + + // Closure structs hold variables of the enclosing scopes, so they are listed before the + // instance state to keep the count cap prioritizing variables over fields. Every struct + // is walked even after one of them links to the instance, because each carries the + // variables of its own scope. + foreach (object closureFrame in closureFrames) + { + if (closureFrame == null) + { + continue; + } + + object linkedOuterThis = CollectDirectFieldVariables( + closureFrame, entries, capturedNames, truncatedVariableNames, ref truncatedVariableCount, + ref truncated, followOuterThis: true); + outerThis ??= linkedOuterThis; } - object outerThis = CollectDirectFieldVariables( - instance, entries, capturedNames, truncatedVariableNames, ref truncatedVariableCount, - ref truncated, followOuterThis: true); - if (outerThis == null) + object thisValue = receiver ?? outerThis; + if (thisValue == null) { return; } - // State machine or closure: the real `this` is the outer instance found through the - // compiler-generated holders, never a holder itself. Emit it before its fields. TryAppendEntry( entries, capturedNames, truncatedVariableNames, ref truncatedVariableCount, ref truncated, - ThisEntryName, UloopCapturedVariableScope.This, outerThis); + ThisEntryName, UloopCapturedVariableScope.This, thisValue); + // Why no link following: only compiler-generated holders link outward, and `this` is + // never one of them. CollectDirectFieldVariables( - outerThis, entries, capturedNames, truncatedVariableNames, ref truncatedVariableCount, + thisValue, entries, capturedNames, truncatedVariableNames, ref truncatedVariableCount, ref truncated, followOuterThis: false); } diff --git a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointVariableFormatter.cs b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointVariableFormatter.cs index a5c2353af9..96de8e6830 100644 --- a/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointVariableFormatter.cs +++ b/Packages/src/Editor/FirstPartyTools/PausePoint/SourcePausePointVariableFormatter.cs @@ -24,7 +24,7 @@ public static (List Variables, bool Truncated) Format( int maxCollectionPreviewElementCount = SourcePausePointConstants.MaxCollectionPreviewElementCount) { UloopPausePointCapturedVariableFrame frame = SourcePausePointVariableCollector.Collect( - instance, parameterNamesAndValues, localNamesAndValues); + instance, Array.Empty(), parameterNamesAndValues, localNamesAndValues); return FormatFrame(frame, maxCollectionPreviewElementCount); } From ce8309587c15c7f0574b6a69847b40ddcba33933 Mon Sep 17 00:00:00 2001 From: hatayama Date: Mon, 5 Oct 2026 11:21:41 +0900 Subject: [PATCH 2/2] Give the closure structs distinct values in the multi-struct tests With delta 1, offset and inner were both 2, so a capture that read every struct from the first struct's argument still produced the expected values. Calling with 2 makes them 4 and 3, so each struct must come from its own argument for the tests to pass. --- ...ourcePausePointClosureFrameCaptureTests.cs | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/Assets/Tests/Editor/SourcePausePointPatcher/SourcePausePointClosureFrameCaptureTests.cs b/Assets/Tests/Editor/SourcePausePointPatcher/SourcePausePointClosureFrameCaptureTests.cs index 471982a587..7905fe33eb 100644 --- a/Assets/Tests/Editor/SourcePausePointPatcher/SourcePausePointClosureFrameCaptureTests.cs +++ b/Assets/Tests/Editor/SourcePausePointPatcher/SourcePausePointClosureFrameCaptureTests.cs @@ -95,14 +95,16 @@ public void LocalFunctionAcrossNestedScopes_CapturesEveryClosureFrame() { // Verifies a static local function receiving one closure struct per enclosing scope // captures the variables of every scope, not only those of the first struct. + // Called with 2 so that offset (4) and inner (3) differ: with equal values, reading + // every struct from the first struct's argument would go unnoticed. UloopPausePointSnapshot snapshot = EnableAndHit( "closure-frame-nested", NestedScopesLocalFunctionReturnLine, - () => Assert.That(PatcherClosureFrameLocalFunctionFixture.SumAcrossNestedScopes(1), Is.EqualTo(4))); + () => Assert.That(PatcherClosureFrameLocalFunctionFixture.SumAcrossNestedScopes(2), Is.EqualTo(7))); Assert.That(snapshot.CapturedVariables.Any(v => v.Name == "this"), Is.False, FormatCaptured(snapshot)); - Assert.That(AssertCaptured(snapshot, "offset", UloopCapturedVariableScope.Parameter).Value, Is.EqualTo("2")); - Assert.That(AssertCaptured(snapshot, "inner", UloopCapturedVariableScope.Parameter).Value, Is.EqualTo("2")); - Assert.That(AssertCaptured(snapshot, "sum", UloopCapturedVariableScope.Local).Value, Is.EqualTo("4")); + Assert.That(AssertCaptured(snapshot, "offset", UloopCapturedVariableScope.Parameter).Value, Is.EqualTo("4")); + Assert.That(AssertCaptured(snapshot, "inner", UloopCapturedVariableScope.Parameter).Value, Is.EqualTo("3")); + Assert.That(AssertCaptured(snapshot, "sum", UloopCapturedVariableScope.Local).Value, Is.EqualTo("7")); } [Test] @@ -110,18 +112,19 @@ public void LocalFunctionWithParameterAcrossNestedScopes_CapturesEveryFrameAndPa { // Verifies an instance local function whose closure structs follow its own declared // parameter captures that parameter and every struct, each read from its own argument. + // Called with 2 so that offset (4) and inner (3) differ, as in the static case above. UloopPausePointSnapshot snapshot = EnableAndHit( "closure-frame-parameter", ParameterNestedScopesLocalFunctionReturnLine, () => Assert.That( - new PatcherClosureFrameLocalFunctionFixture(10).SumWithParameterAcrossNestedScopes(1), - Is.EqualTo(114))); + new PatcherClosureFrameLocalFunctionFixture(10).SumWithParameterAcrossNestedScopes(2), + Is.EqualTo(117))); AssertCaptured(snapshot, "this", UloopCapturedVariableScope.This); Assert.That(AssertCaptured(snapshot, "_value", UloopCapturedVariableScope.InstanceField).Value, Is.EqualTo("10")); Assert.That(AssertCaptured(snapshot, "extra", UloopCapturedVariableScope.Parameter).Value, Is.EqualTo("100")); - Assert.That(AssertCaptured(snapshot, "offset", UloopCapturedVariableScope.Parameter).Value, Is.EqualTo("2")); - Assert.That(AssertCaptured(snapshot, "inner", UloopCapturedVariableScope.Parameter).Value, Is.EqualTo("2")); - Assert.That(AssertCaptured(snapshot, "sum", UloopCapturedVariableScope.Local).Value, Is.EqualTo("114")); + Assert.That(AssertCaptured(snapshot, "offset", UloopCapturedVariableScope.Parameter).Value, Is.EqualTo("4")); + Assert.That(AssertCaptured(snapshot, "inner", UloopCapturedVariableScope.Parameter).Value, Is.EqualTo("3")); + Assert.That(AssertCaptured(snapshot, "sum", UloopCapturedVariableScope.Local).Value, Is.EqualTo("117")); } [Test]