Implement PyStackRef borrowed stack references for LOAD_FAST_BORROW#7290
Merged
youknowone merged 3 commits intoRustPython:mainfrom Mar 1, 2026
Merged
Implement PyStackRef borrowed stack references for LOAD_FAST_BORROW#7290youknowone merged 3 commits intoRustPython:mainfrom
youknowone merged 3 commits intoRustPython:mainfrom
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a tagged stack-reference type Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Contributor
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin worktree-pystackref |
youknowone
force-pushed
the
worktree-pystackref
branch
2 times, most recently
from
5c3dd36 to
936d118
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/vm/src/frame.rs (2)
2637-2650:⚠️ Potential issue | 🟡 MinorMirror the borrowed-stack yield invariant in instrumented mode.
YieldValueenforces a debug invariant against borrowed refs on stack, butInstrumentedYieldValuecurrently skips that check.Proposed parity fix
Instruction::InstrumentedYieldValue => { + debug_assert!( + self.state + .stack + .iter() + .flatten() + .all(|sr| !sr.is_borrowed()), + "borrowed refs on stack at yield point" + ); let value = self.pop_value(); if self.monitoring_mask & monitoring::EVENT_PY_YIELD != 0 { let offset = (self.lasti() - 1) * 2; monitoring::fire_py_yield(vm, self.code, offset, &value)?; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/frame.rs` around lines 2637 - 2650, InstrumentedYieldValue currently skips the debug invariant that YieldValue enforces against borrowed refs on the stack; after calling self.pop_value() in Instruction::InstrumentedYieldValue you should run the same borrowed-stack check/assert used by YieldValue (i.e., the debug assertion that ensures the popped value contains no borrowed refs) before calling monitoring::fire_py_yield, before any wrapping for COROUTINE, and before returning ExecutionResult::Yield; reuse the same helper/assert or inline the same check so behavior is identical to YieldValue (refer to pop_value, monitoring::fire_py_yield, lasti, code.flags / bytecode::CodeFlags::COROUTINE, PyAsyncGenWrappedValue, and ExecutionResult::Yield).
1814-1828:⚠️ Potential issue | 🟠 Major
LOAD_FAST_BORROWis still an owned-clone fallback.This path currently clones from
fastlocalsand callspush_value, so borrowed-stack behavior is not actually active yet. The in-code TODO also flags known corruption concerns that should be resolved (or explicitly scoped out) before merge.If helpful, I can draft a follow-up issue with a concrete rollout plan (guarded enablement + invariants + tests) for re-enabling true borrowed refs safely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/frame.rs` around lines 1814 - 1828, The handler labeled for LOAD_FAST_BORROW is still cloning and pushing an owned value (using self.fastlocals.lock()[idx].clone() and self.push_value), so either implement true borrowed semantics or make the fallback explicit: implement a push_borrowed helper (e.g., self.push_borrowed_ref(idx) or similar) that pushes a borrowed reference onto the stack without cloning and use it here, ensuring you access self.fastlocals without cloning the slot; otherwise, stop pretending borrowed behavior is active by routing this opcode to the existing owned LOAD_FAST logic (or renaming the handler) and update the TODO/comment accordingly; refer to the LOAD_FAST_BORROW usage, self.fastlocals.lock(), push_value, and self.code.varnames to locate and change the code.
🧹 Nitpick comments (1)
crates/vm/src/object/core.rs (1)
1404-1406: Remove decorative section separators.Please replace the banner-style separator comments with a normal short comment/doc-comment.
As per coding guidelines: "Do not add decorative section separators (e.g. `// -----------`, `// ===`, `/* *** */`). Use `///` doc-comments or short `//` comments only when they add value".Proposed cleanup
-// --------------------------------------------------------------------------- -// PyStackRef – tagged stack reference (owned or borrowed) -// --------------------------------------------------------------------------- +// PyStackRef: tagged stack reference (owned or borrowed)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/object/core.rs` around lines 1404 - 1406, Replace the decorative banner comment block above the PyStackRef declaration with a concise comment or doc-comment: remove the multi-line separator lines and change the text "// PyStackRef – tagged stack reference (owned or borrowed)" into a single short comment (e.g. "// PyStackRef — tagged stack reference (owned or borrowed)") or a proper doc-comment ("/// PyStackRef — tagged stack reference (owned or borrowed)") placed immediately above the PyStackRef type/struct so it stays documented without decorative separators.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/vm/src/frame.rs`:
- Around line 2637-2650: InstrumentedYieldValue currently skips the debug
invariant that YieldValue enforces against borrowed refs on the stack; after
calling self.pop_value() in Instruction::InstrumentedYieldValue you should run
the same borrowed-stack check/assert used by YieldValue (i.e., the debug
assertion that ensures the popped value contains no borrowed refs) before
calling monitoring::fire_py_yield, before any wrapping for COROUTINE, and before
returning ExecutionResult::Yield; reuse the same helper/assert or inline the
same check so behavior is identical to YieldValue (refer to pop_value,
monitoring::fire_py_yield, lasti, code.flags / bytecode::CodeFlags::COROUTINE,
PyAsyncGenWrappedValue, and ExecutionResult::Yield).
- Around line 1814-1828: The handler labeled for LOAD_FAST_BORROW is still
cloning and pushing an owned value (using self.fastlocals.lock()[idx].clone()
and self.push_value), so either implement true borrowed semantics or make the
fallback explicit: implement a push_borrowed helper (e.g.,
self.push_borrowed_ref(idx) or similar) that pushes a borrowed reference onto
the stack without cloning and use it here, ensuring you access self.fastlocals
without cloning the slot; otherwise, stop pretending borrowed behavior is active
by routing this opcode to the existing owned LOAD_FAST logic (or renaming the
handler) and update the TODO/comment accordingly; refer to the LOAD_FAST_BORROW
usage, self.fastlocals.lock(), push_value, and self.code.varnames to locate and
change the code.
---
Nitpick comments:
In `@crates/vm/src/object/core.rs`:
- Around line 1404-1406: Replace the decorative banner comment block above the
PyStackRef declaration with a concise comment or doc-comment: remove the
multi-line separator lines and change the text "// PyStackRef – tagged stack
reference (owned or borrowed)" into a single short comment (e.g. "// PyStackRef
— tagged stack reference (owned or borrowed)") or a proper doc-comment ("///
PyStackRef — tagged stack reference (owned or borrowed)") placed immediately
above the PyStackRef type/struct so it stays documented without decorative
separators.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.cspell.dict/cpython.txtcrates/vm/src/frame.rscrates/vm/src/lib.rscrates/vm/src/object/core.rscrates/vm/src/object/traverse.rs
…wed refs - Change PyStackRef.bits from usize to NonZeroUsize so Option<PyStackRef> has the same size as Option<PyObjectRef> via niche optimization - Revert LoadFastBorrow to use clone instead of actual borrowed refs to avoid borrowed refs remaining on stack at yield points - Add static size assertions for Option<PyStackRef> - Add stackref, fastlocal to cspell dictionary - Remove debug eprintln statements - Fix clippy warning for unused push_borrowed
youknowone
force-pushed
the
worktree-pystackref
branch
from
936d118 to
5c25fc7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/vm/src/object/core.rs (1)
1404-1406: Remove decorative separator comments in this module section.Please replace this separator with a normal doc-comment header (
///) or a short value-adding comment to match repo standards.As per coding guidelines: "Do not add decorative section separators (e.g.
// -----------,// ===,/* *** */). Use///doc-comments or short//comments only when they add value".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/object/core.rs` around lines 1404 - 1406, Replace the decorative separator comment above the PyStackRef section with a value-adding doc or inline comment: change the multi-line decorative block to a short /// doc-comment describing the PyStackRef type and its purpose (e.g., "/// PyStackRef: a tagged stack reference that may be owned or borrowed") or a concise // comment that explains why this section exists; update the comment immediately adjacent to the PyStackRef symbol so the module follows the repo's non-decorative comment guideline.crates/vm/src/frame.rs (1)
4096-4106:push_borrowedis dead code; the borrowed fast-path is disabled pending resolution of stack lifetime issues at yield/exception points.The
push_borrowedhelper (line 4113) is defined but never called. BothLOAD_FAST_BORROW(line 1870) andLoadFastBorrowLoadFastBorrow(line 1887) fall back to.clone()instead. The code comment at line 1868 explicitly notes this is intentional: the optimization is available but disabled until stack lifetime issues are resolved. Either wire it in when lifetime issues are fixed, or remove it to avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/frame.rs`:
- Around line 2501-2508: The debug_assert ensuring no borrowed refs on the stack
is only present in the Instruction::YieldValue suspend path but must also run
for Instruction::InstrumentedYieldValue to maintain the suspend invariant; add
the same check (or factor into a small helper like
assert_no_borrowed_stack(&self.state) and call it) before the frame suspension
in the InstrumentedYieldValue handling so that
self.state.stack.iter().flatten().all(|sr| !sr.is_borrowed()) is asserted in
both yield code paths.
---
Nitpick comments:
In `@crates/vm/src/object/core.rs`:
- Around line 1404-1406: Replace the decorative separator comment above the
PyStackRef section with a value-adding doc or inline comment: change the
multi-line decorative block to a short /// doc-comment describing the PyStackRef
type and its purpose (e.g., "/// PyStackRef: a tagged stack reference that may
be owned or borrowed") or a concise // comment that explains why this section
exists; update the comment immediately adjacent to the PyStackRef symbol so the
module follows the repo's non-decorative comment guideline.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.cspell.dict/cpython.txtcrates/vm/src/frame.rscrates/vm/src/lib.rscrates/vm/src/object/core.rscrates/vm/src/object/traverse.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- .cspell.dict/cpython.txt
- crates/vm/src/lib.rs
- crates/vm/src/object/traverse.rs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/vm/src/frame.rs (1)
1284-1286: Potential issue when borrowed refs are enabled: cloning propagates the borrow tag.When
push_borrowed()is eventually enabled, cloning anOption<PyStackRef>in the Copy instruction will propagate the borrowed tag, resulting in multiple borrowed refs to the same object on the stack. This is safe only if the compiler guarantees all copies are consumed within the same basic block before the source object can be invalidated.Consider adding a comment noting this invariant, or alternatively promoting borrowed refs to owned when copying:
// Example: promote borrowed to owned on copy to avoid lifetime issues let value = self.state.stack[stack_len - idx] .as_ref() .map(|sr| PyStackRef::new_owned(sr.to_pyobj()));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/vm/src/frame.rs` around lines 1284 - 1286, The Copy instruction currently clones an Option<PyStackRef> and will propagate any borrowed tag (via self.state.stack[...] .clone()) leading to multiple borrowed refs; either add a clear comment in the Copy handling documenting the invariant that all copies must be consumed within the same basic block if borrowed refs are allowed, or change the copy behavior to promote borrowed refs to owned before pushing (i.e., when reading from self.state.stack use as_ref().map(|sr| PyStackRef::new_owned(sr.to_pyobj())) to produce an owned PyStackRef and then call push_stackref_opt with that owned value to avoid creating multiple borrowed refs; reference the Copy handling, self.state.stack, push_stackref_opt, push_borrowed, and PyStackRef symbols when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/vm/src/frame.rs`:
- Around line 1284-1286: The Copy instruction currently clones an
Option<PyStackRef> and will propagate any borrowed tag (via
self.state.stack[...] .clone()) leading to multiple borrowed refs; either add a
clear comment in the Copy handling documenting the invariant that all copies
must be consumed within the same basic block if borrowed refs are allowed, or
change the copy behavior to promote borrowed refs to owned before pushing (i.e.,
when reading from self.state.stack use as_ref().map(|sr|
PyStackRef::new_owned(sr.to_pyobj())) to produce an owned PyStackRef and then
call push_stackref_opt with that owned value to avoid creating multiple borrowed
refs; reference the Copy handling, self.state.stack, push_stackref_opt,
push_borrowed, and PyStackRef symbols when making the change.
ℹ️ Review info
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/vm/src/frame.rscrates/vm/src/object/core.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/vm/src/object/core.rs
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 by CodeRabbit
Refactor
Chores