Repository navigation
fix: Pause points inside local functions now show this and every variable they use from the enclosing scopes - #3157
Merged
Conversation
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
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughPause point capture now passes closure frames to variable collection. The collector scans those frames to report captured variables and resolve ChangesClosure-frame pause point capture
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant SourcePausePointInjectionEmitter
participant SourcePausePointCapture
participant SourcePausePointVariableCollector
SourcePausePointInjectionEmitter->>SourcePausePointCapture: Pass instance, closure frames, parameters, and locals
SourcePausePointCapture->>SourcePausePointVariableCollector: Collect captured variables and resolve this
Merge Risk: ⚪ Minimal · up to Closure-frame values flow through pause-point capture in the expected slots, and the supplied tests cover nested and hot-reloaded local functions. No actionable merge-blocking risk remains beyond normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change exposes additional enclosing-scope values through existing pause-point functionality. No new externally reachable operation or security-control bypass was identified, and existing hit and cleanup behavior remains unchanged. End-to-end authorization and interrupted-capture behavior were not fully established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
this, the instance fields, and every variable the local function uses from its enclosing scopes, in compiled code and in hot-reloaded methods alike.User Impact
this, no instance fields, no method parameter. The same gap appeared in two more shapes found while fixing it:thisbut lost the enclosing method's locals;ifblock) showed only the variables of one of them.(ref/out/in parameter cannot be boxed)underNotCapturableVariablesfor these local functions, which looked like a capture bug.--hit-whencondition on such a variable could not find it, so the marker reported an evaluation error and hit regardless of the condition.thisresolves to the real instance, the unnamed entry is gone, and--hit-whenevaluates the condition against the captured value.Changes
thisof an instance method). Before, only one struct was passed, and only when the local function was static.thisand the instance fields (so the count cap keeps favoring variables), and takesthisfrom the first link to the instance it finds, whichever struct carries it.ref/out/inparameters are still reported as before.Verification
uloop compile: success, no new warnings.Red before the fix, green after:
SourcePausePointClosureFrameCaptureTests(new, 11 tests). It covers the issue's loop-closure shape, an instance method's local function, a static local function (regression guard for Pause point in a local function of a hot-reloaded method shows no this #3066, passing before and after), a local function using two scopes, an instance local function whose structs follow its own declared parameter, a--hit-whencondition on a struct-only variable, and theNotCapturableVariablesreport for each shape.Green:
SourcePausePointCaptureTests(23, including three new collector tests for multiple structs, ordering, and a receiver found in a later struct),HotReloadPausePointContractTests(25, including a new hot-reload test for the issue's loop-closure shape and the existing fix: Pause points in a hot-reloaded method's local functions now show the instance as this #3071 test), andSourcePausePointTruncationAggregateTests,SourcePausePointVariableFormatterTests,PausePointToolModeTests,SourcePausePointNotCapturableParametersTests,PausePointNotCapturableVariablesTests,SourcePausePointPatcherTests,SourcePausePointPostLineCaptureTests(140 together).CA1502 complexity check and the file-length check: no findings.
Mutation check: making every struct load from the first struct's argument slot fails 3 of the new tests (the second struct's variable reads 4 or a garbage value instead of 3). The multi-struct tests call with values that make each struct's variables differ, so a wrong slot cannot reproduce the expected values.
Not verified
this: that IL would load anintargument as a pointer and could crash the Editor. The test whose structs follow a declared parameter would read the wrong argument in that case.