Align _opcode.has_* functions with 3.14.4#7711
Conversation
_opcode.has_* functions with CPython_opcode.has_* functions with 3.14.4
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR updates CPython-compatibility opcode classification helpers in RustPython's stdlib module by expanding detection criteria for jump, free-variable, local-variable, and exception-handling opcodes to recognize additional variants, pseudo-opcodes, and instrumented versions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: (module 'dis test_peepholer' not found) Legend:
|
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 (1)
crates/stdlib/src/_opcode.rs (1)
163-172:⚠️ Potential issue | 🔴 CriticalMove
LoadDereftohas_freeto align with CPython 3.13.The current placement of
LoadDerefinhas_localcontradicts CPython 3.13+, whereLOAD_DEREFis classified indis.hasfree. Per CPython behavior,LOAD_DEREFaccesses free variables (closure variables) and must be included inhas_free(). WhileLoadFromDictOrDeref(a RustPython-specific optimization) correctly belongs inhas_free,LoadDerefshould also be classified there, not inhas_local.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/stdlib/src/_opcode.rs` around lines 163 - 172, The opcode classification is wrong: remove Opcode::LoadDeref from the has_local match and add it to the has_free match so LOAD_DEREF is treated as a "free" access like CPython 3.13; specifically, update the matches!() in has_free (the AnyOpcode::Real(Opcode::... ) list) to include Opcode::LoadDeref (alongside LoadFromDictOrDeref, DeleteDeref, MakeCell, StoreDeref) and delete Opcode::LoadDeref from the has_local match to keep the two functions consistent.
🤖 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/stdlib/src/_opcode.rs`:
- Around line 163-172: The opcode classification is wrong: remove
Opcode::LoadDeref from the has_local match and add it to the has_free match so
LOAD_DEREF is treated as a "free" access like CPython 3.13; specifically, update
the matches!() in has_free (the AnyOpcode::Real(Opcode::... ) list) to include
Opcode::LoadDeref (alongside LoadFromDictOrDeref, DeleteDeref, MakeCell,
StoreDeref) and delete Opcode::LoadDeref from the has_local match to keep the
two functions consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: ddcd032a-8c81-40b7-a0fe-c9d48ae834c8
⛔ Files ignored due to path filters (1)
Lib/test/test_dis.pyis excluded by!Lib/**
📒 Files selected for processing (1)
crates/stdlib/src/_opcode.rs
* Remove unused rust impl for formatting dis output * remove `examples/dis.rs` * Added tests * Update lock * Try to set snapshot dir * Remove verbose flag * Regenerate snapshots after #7711 * Revert "Bump insta from 1.46.3 to 1.47.2 (#7706)" This reverts commit e6d9ea6. * Debug info * Show diff as well * Show debug faster * CI: true env * Recert CI * Add `CI: true` in ci emv * Reapply "Bump insta from 1.46.3 to 1.47.2 (#7706)" This reverts commit 693ca8c. * simplify macro * trim on function side * Force insta workspace root * fix merge
Summary by CodeRabbit