interpreter: give the frame getset accessors named descr_typecheck wrappers - #1515
Conversation
…appers `init_frame_type` registered each `GetSetProperty` accessor as an anonymous closure literal. `typedef.py _make_descr_typecheck_wrapper` generates one named function per property and role, so the 15 accessor bodies move to `pyframe.rs` as `descr_typecheck_*` items and the registration passes the path. The bodies are carried over unchanged apart from `frame_ptr(args[1])`, which becomes `args.get(1).copied().unwrap_or(pyre_object::PY_NULL) as *mut PyFrame` now that the helper nested in `init_frame_type` is out of scope. `clear` and `__repr__` keep their closures: upstream registers those through `interp2app`, which does not build a typecheck wrapper. Assisted-by: Claude
|
Warning Review limit reachedNext included review available in 21 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
There was a problem hiding this comment.
💡 Codex Review
https://github.com/youknowone/pyre/blob/3eaa3b3623d81f426dfd4761093d7765e5f30a8f/pyre-interpreter/src/typedef.rs#L8168
Give f_locals its named wrapper too
This named-registration sequence skips f_locals immediately below, leaving its getter as the sole anonymous closure among the frame getsets. Upstream PyFrame.typedef registers f_locals through GetSetProperty(PyFrame.fget_getdictscope), so _make_descr_typecheck_wrapper generates a named descr_typecheck_fget_getdictscope just as it does for the accessors converted here; add the corresponding free wrapper and register it to complete the structural conversion.
AGENTS.md reference: AGENTS.md:L223-L226
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🤖 Codex parity reviewStatic analysis of this diff vs the local RPython/PyPy sources (commit 3eaa3b3). Files in the reviewed diff1. Regressions to PyPy parity introduced by this patchNone. 2. Other mismatches introduced by this patchNone. 3. Pre-existing mismatches (already present before this patch)None. 4. Structural adaptations
|
init_frame_typeregistered eachGetSetPropertyaccessor as an anonymousclosure literal. Upstream does not:
typedef.py _make_descr_typecheck_wrappergenerates one named function per property and role —
descr_typecheck_%sover the wrapped method — memoised by
@specialize.memo()so each is aprebuilt constant, and
GetSetProperty.__init__binds that asfget/fset/
fdel.This moves the 15 accessor bodies to
pyframe.rsasdescr_typecheck_*itemsand has the registration pass the path.
Bodies are carried over unchanged apart from
frame_ptr(args[1]), whichbecomes
args.get(1).copied().unwrap_or(pyre_object::PY_NULL) as *mut PyFramenow that the helper nested in
init_frame_typeis out of scope.clearand__repr__keep their closures: upstream registers those throughinterp2app, which does not build a typecheck wrapper.This is structural parity — the reachability gain is NOT demonstrated
Naming these was proposed on the theory that the JIT cannot see an anonymous
closure. Measured on the shipped LLBC, that is false. charon emits, per
closure, a
closureitem plus anas_fnFunDecl with a stable id:So the front end already had these bodies. Whatever the getset path costs the
JIT, it is not front-end visibility — the getset
fgetis reached through anindirect call off the descriptor, and upstream resolves that through the
graph set the annotator attaches to
indirect_call(call.py graphs_from).Whether pyre resolves it is a separate question this PR does not touch, and no
jitstats move is claimed.
The justification here is that upstream builds a named wrapper per property and
pyre inlined an anonymous one. Nothing more.
Conflict resolved against main, not taken from one side
#1512's neighbourhood aside, main rewrote thef_lastigetter body while thiswas in flight — the not-yet-started generator frame resting after
RETURN_GENERATORrather than on theRESUME. Taking either side whole wouldhave dropped one of the two changes, so
typedef.rstakes the wrapper path andthe wrapper carries main's new body. Main's 44-line explanation moved with
it: that block ends "so both adaptations live here rather than in
PyFrame::fget_f_lasti", which stops being true if the body moves and the prosedoes not.
Verification
cargo check -p pyre-interpreter --features dynasmon this commit — clean(one pre-existing
dead_codewarning onerror.rs write_report_line, nottouched here).
cargo fmt --all -- --check— clean.measurement; CI is the gate.
🤖 Generated with Claude Code
https://claude.ai/code/session_017wapwfqfqNe7kFcxQRx85P