Resolve test_inspect bytecode parity gaps#7926
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors compile-time scope classification to use explicit CompilerScope variants, maps specific class-related dunder identifiers to unknown symbol scope, consolidates class dunder emission via ChangesScope and Dunder Name Resolution Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: [x] lib: cpython/Lib/inspect.py dependencies:
dependent tests: (90 tests)
Legend:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/codegen/src/compile.rs`:
- Around line 20926-20955: The assertion is too weak because it only checks for
any Instruction::StoreDeref without verifying the deref targets; update the
third assert to resolve the StoreDeref's target index (from unit.arg/OpArg as
used elsewhere) back into the closure capture name (using class_code.freevars or
the appropriate captured-names table) and assert that the resolved name is
exactly "__firstlineno__" (i.e. check the deref slot maps to "__firstlineno__"
rather than just any StoreDeref), keeping the existing error message format and
using the same symbols: class_code.instructions, Instruction::StoreDeref,
unit.arg/OpArg, and class_code.freevars to locate the code to change.
- Around line 3261-3262: The fallback that maps underscore-prefixed identifiers
to SymbolScope::Unknown is too broad—replace the single check
name.starts_with('_') with a whitelist of the compiler-generated synthetic
dunders introduced in this patch: e.g., match exact names via a constant set
(SYNTHETIC_NAMES.contains(name)) and/or prefix list
(SYNTHETIC_PREFIXES.iter().any(|p| name.starts_with(p))) and only map those to
SymbolScope::Unknown; keep the rest of identifiers unchanged. Update the code
around the existing branch that returns SymbolScope::Unknown (the expression
using name and SymbolScope::Unknown) to use this whitelist logic and add or
adjust tests to cover both synthetic dunders and normal user names.
- Line 6717: Replace the call to self.store_name("__classdictcell__") with an
unconditional class-local store like you do for "__classdict__" (emit
Instruction::StoreDeref or the equivalent direct store-deref bytecode) so the
name is always written into the class namespace regardless of global/nonlocal
declarations; likewise find the parallel location handling "__classcell__" and
change its self.store_name("__classcell__") to the same direct StoreDeref-style
emission. Locate these in compile.rs near the class-creation handling (the site
where "__classdict__" is already handled with Instruction::StoreDeref) and
ensure both special slots bypass store_name() and are stored unconditionally
into the class namespace.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 6fdca45e-5dbf-4384-91c9-e0ba1346cec8
⛔ Files ignored due to path filters (1)
Lib/test/test_inspect/test_inspect.pyis excluded by!Lib/**
📒 Files selected for processing (1)
crates/codegen/src/compile.rs
| namei: classdictcell | ||
| } | ||
| ); | ||
| self.store_name("__classdictcell__")?; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In CPython, does the compiler always store the synthetic classcellinto the class locals namespace, even when the class body declaresglobal classcellornonlocal classcell?
💡 Result:
No—CPython does not always end up storing the synthetic classcell into the class-body locals namespace when the class body declares global classcell or nonlocal classcell. In CPython, the zero-argument super() mechanism is implemented by injecting a hidden class / classcell cell during compilation, but CPython also standardizes behavior so that class and (in 3.6+) classcell do not show up in the class locals while the class body executes [1]. Additionally, the generator/symbol-table machinery for class/classcell interacts “strangely” with explicit nonlocal class in nested namespaces: the injection behaves like a closure/free variable and can be accessed via nonlocal from a nested class, without being treated as a normal local in the class-body locals mapping [1]. This directly implies that declaring nonlocal classcell (analogous to the class case discussed) prevents classcell from being stored like a regular local entry in the class locals namespace. So, when you explicitly declare global classcell or nonlocal classcell, CPython’s compiler/symbol-table handling will not necessarily (and in these cases generally does not) place the synthetic classcell into the class locals mapping the way it would for a normal class body without such declarations; instead the injected cell remains hidden/handled specially for LOAD_BUILD_CLASS and zero-argument super() semantics [1].
Citations:
🏁 Script executed:
# First, check the file size and read the relevant sections
wc -l crates/codegen/src/compile.rsRepository: RustPython/RustPython
Length of output: 101
🏁 Script executed:
# Read around line 6717
sed -n '6710,6725p' crates/codegen/src/compile.rsRepository: RustPython/RustPython
Length of output: 690
🏁 Script executed:
# Read around line 6731
sed -n '6724,6738p' crates/codegen/src/compile.rsRepository: RustPython/RustPython
Length of output: 557
🏁 Script executed:
# Find the store_name method definition to understand its behavior
rg -n "fn store_name" crates/codegen/src/compile.rs | head -5Repository: RustPython/RustPython
Length of output: 134
🏁 Script executed:
# Search for any special handling of __classcell__ or __classdictcell__
rg -n "__classcell__|__classdictcell__" crates/codegen/src/compile.rsRepository: RustPython/RustPython
Length of output: 661
🏁 Script executed:
# Read the store_name implementation
sed -n '3146,3160p' crates/codegen/src/compile.rsRepository: RustPython/RustPython
Length of output: 684
🏁 Script executed:
# Search for any method that might do unconditional class-local stores
rg -n "store_.*local|store_cell|STORE_DEREF|STORE_GLOBAL" crates/codegen/src/compile.rs | head -20Repository: RustPython/RustPython
Length of output: 380
🏁 Script executed:
# Get broader context around line 6717 - understand the class body compilation
sed -n '6690,6745p' crates/codegen/src/compile.rsRepository: RustPython/RustPython
Length of output: 1960
🏁 Script executed:
# Find compile_name implementation
rg -n "fn compile_name" crates/codegen/src/compile.rsRepository: RustPython/RustPython
Length of output: 154
🏁 Script executed:
# Search for enum NameUsage to understand Store variant
rg -n "enum NameUsage" crates/codegen/src/compile.rs -A 10Repository: RustPython/RustPython
Length of output: 386
🏁 Script executed:
# Look for direct instruction emitters that might bypass name resolution
rg -n "Instruction::Store|PseudoInstruction::Store" crates/codegen/src/compile.rs | head -20Repository: RustPython/RustPython
Length of output: 1639
🏁 Script executed:
# Check how __static_attributes__ is handled (line 6707 shows it uses store_name too)
# Let's see if there's any special handling for class protocol names
rg -n "__staticattr|__firstlineno__|__module__|__qualname__" crates/codegen/src/compile.rs | head -20Repository: RustPython/RustPython
Length of output: 1309
🏁 Script executed:
# Confirm that __classdict__ uses direct STORE_DEREF
sed -n '6648,6656p' crates/codegen/src/compile.rsRepository: RustPython/RustPython
Length of output: 502
🏁 Script executed:
# Check what the exact instruction types are for these stores
rg -n "Instruction::(StoreDeref|StoreName|StoreGlobal|StoreFast)" crates/codegen/src/compile.rs | grep -E "6(6|7)[0-9]{2}:" | head -20Repository: RustPython/RustPython
Length of output: 234
Use unconditional class-local stores for __classcell__ and __classdictcell__ instead of routing through store_name().
These are class-creation protocol slots that must always be stored to the class namespace, regardless of global or nonlocal declarations in the class body. Routing them through store_name() causes them to honor such declarations, converting them to STORE_DEREF or STORE_GLOBAL, which prevents the class namespace from receiving the cells that __build_class__ expects. Note that __classdict__ is already correctly handled with a direct Instruction::StoreDeref (line 6652); apply the same pattern to both locations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/codegen/src/compile.rs` at line 6717, Replace the call to
self.store_name("__classdictcell__") with an unconditional class-local store
like you do for "__classdict__" (emit Instruction::StoreDeref or the equivalent
direct store-deref bytecode) so the name is always written into the class
namespace regardless of global/nonlocal declarations; likewise find the parallel
location handling "__classcell__" and change its
self.store_name("__classcell__") to the same direct StoreDeref-style emission.
Locate these in compile.rs near the class-creation handling (the site where
"__classdict__" is already handled with Instruction::StoreDeref) and ensure both
special slots bypass store_name() and are stored unconditionally into the class
namespace.
1 participant
Summary by CodeRabbit
Refactor
Tests