Replace CallMethod/LoadMethod opcodes with plain Call #6674
Conversation
📝 WalkthroughWalkthroughConsolidate call/method emission and introduce NULL-aware self_or_null semantics: replace legacy CallMethod/CallFunction* and LoadMethod with Changes
Sequence Diagram(s)sequenceDiagram
participant Compiler as Compiler (codegen)
participant Bytecode as Bytecode (instructions)
participant VM as VM (frame/exec)
participant Callable as Callable/Method
Compiler->>Bytecode: emit PushNull, LoadAttrMethod(attr), Call/CallKw/CallFunctionEx
Note over VM: Stack before call → [callable, NULL|self, args..., (kw_names?)]
Bytecode->>VM: execute LoadAttrMethod -> push method and self_or_null
VM->>VM: execute Call* -> pop callable, self_or_null, args
alt self_or_null is NULL
VM->>Callable: invoke callable(args)
else self_or_null is Some(self)
VM->>Callable: invoke callable(self, args)
end
Callable-->>VM: return value
VM->>VM: push return value
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
46de323 to
5a5c7be
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/codegen/src/compile.rs (1)
3712-3763: Stack layout mismatch:CallWithSelfKwexpects tuple of keyword names on top, but code pushes individual name/value pairs without accounting for the extra items.The instruction expects stack
[callable, self_or_null, args..., kwarg_names]wherekwarg_namesis a single tuple. Currently, the code pushes one string (name) + one value for each keyword (2 items per keyword), but only incrementsnargsbykeywords.len(). This creates a discrepancy: the VM will try to popnargsitems expecting aligned positional arguments and keyword values, but the keyword names aren't consolidated into a tuple.Follow
compile_keywords' pattern or the proposed fix: build a tuple of keyword names to place on top of the stack as a single item, ensuring the stack accounting matchesCallWithSelfKw's contract.
🧹 Nitpick comments (3)
crates/jit/src/instructions.rs (1)
480-505: Potential panic in argument processing.Line 486 uses
arg.into_value().unwrap()which will panic if the argument isJitValue::None,JitValue::Null,JitValue::Tuple, orJitValue::FuncRef. This mirrors the existing pattern inCallFunctionPositional(line 466), so it's consistent with the codebase, but both locations have this limitation.Consider returning
JitCompileError::NotSupportedinstead of panicking when an argument cannot be converted to a value:♻️ Suggested improvement
let mut args = Vec::new(); for _ in 0..nargs { let arg = self.stack.pop().ok_or(JitCompileError::BadBytecode)?; - args.push(arg.into_value().unwrap()); + args.push(arg.into_value().ok_or(JitCompileError::NotSupported)?); }crates/vm/src/frame.rs (2)
2180-2202: LGTM: execute_call_with_self correctly implements the self_or_null protocol.The implementation properly handles both cases:
- When
self_or_nullisSome(self_val)(method call): prepends self to arguments- When
self_or_nullisNone(NULL, regular call): uses arguments as-isThe logic is clear and correct.
♻️ Optional optimization: pre-allocate capacity for method calls
let final_args = if let Some(self_val) = self_or_null { // Method call: prepend self to args - let mut all_args = vec![self_val]; + let mut all_args = Vec::with_capacity(1 + args.args.len()); + all_args.push(self_val); all_args.extend(args.args);This avoids a potential reallocation when extending the vector.
2398-2401: The stack manipulation is correct but somewhat complex.The code swaps the top two stack elements and ensures
attr_valueis not NULL:
- Pop
func(Option) from top- Replace current top with
func, getting the old top asattr_value- Expect
attr_valueis not NULLThe net effect is:
[..., attr_value, func] → [..., func]withattr_valueextracted.While correct, this could be clearer with a comment explaining the swap operation.
💡 Optional: Add explanatory comment
+ // Swap top two elements: pop func, replace TOS with func, extract attr_value let func = self.pop_value_opt(); let attr_value = self .replace_top(func) .expect("replace_top encountered NULL");
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_xml_etree.pyis excluded by!Lib/**crates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_double_async_with.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
crates/codegen/src/compile.rscrates/compiler-core/src/bytecode.rscrates/jit/src/instructions.rscrates/stdlib/src/opcode.rscrates/vm/src/frame.rscrates/vm/src/vm/method.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/vm/src/vm/method.rscrates/stdlib/src/opcode.rscrates/jit/src/instructions.rscrates/codegen/src/compile.rscrates/compiler-core/src/bytecode.rscrates/vm/src/frame.rs
🔇 Additional comments (28)
crates/stdlib/src/opcode.rs (1)
76-76: LGTM!The addition of
Instruction::LoadAttrMethod { .. }tohas_nameis correct since this instruction usesco_namesto look up the attribute name, consistent withLoadAttrand other name-using instructions.crates/vm/src/vm/method.rs (1)
41-49: LGTM - Classmethod descriptor handling.The logic correctly differentiates classmethods from regular methods: classmethods need
descr_getto properly bind the class (not the instance), while regular method descriptors can use the fast path withis_method = true.crates/jit/src/instructions.rs (2)
506-509: LGTM!The
PushNullinstruction correctly pushesJitValue::Nullonto the stack to support the self-or-null calling convention.
30-30: LGTM - JitValue::Null variant additions.The
Nullvariant is consistently integrated across all relevant methods:
to_jit_type()correctly returnsNoneinto_value()correctly returnsNoneboolean_val()correctly returnsNotSupportedAlso applies to: 49-49, 56-56, 143-145
crates/compiler-core/src/bytecode.rs (4)
1084-1101: LGTM - New self-aware call instructions.The new instructions (
CallWithSelf,LoadAttrMethod,CallWithSelfKw,CallWithSelfEx) are well-documented with clear stack layout comments. The design aligns with the self-or-null calling convention for method dispatch optimization.
1946-1948: Stack effect for LoadAttrMethod is correct.
LoadAttrMethodpops the object and pushes both the method andself_or_null, resulting in a net stack effect of +1.
1982-1987: Stack effects for CallWithSelf instructions are correct.*The calculations properly account for:
CallWithSelf: popsnargs + self_or_null + callable, pushes resultCallWithSelfKw: additionally popskwargs_namestupleCallWithSelfEx: popsargs_tuple + optional kwargs_dict + self_or_null + callable
1046-1049: Reserved instruction slots maintain opcode numbering.The reserved slots (
Reserved125,Reserved126,Reserved127,Reserved135) correctly preserve opcode numbering for future use or compatibility.Also applies to: 1072-1073
crates/codegen/src/compile.rs (10)
1026-1054: Good:with/async withcleanup now matchesself_or_nullstack shape.Pushing
NULLbefore calling__exit__makes the call path consistent withCallWithSelf { nargs: 3 }.
1876-1887: Good:assertnow callsAssertionError(...)with an explicitNULLreceiver slot.This aligns
LoadGlobal(AssertionError)+PushNull+CallWithSelf { nargs: 0/1 }with the new call convention.
2096-2110: Decorator application stack contract looks correct (and preserves reverse-application order).
prepare_decorators()pushing[decorator, NULL]andapply_decorators()doingCallWithSelf { nargs: 1 }should consume[decorator, NULL, func]repeatedly, applying decorators inside-out as expected.
2147-2152: Good: type-param bound/default closure is invoked viaPushNull+CallWithSelf.Keeps the “immediate invoke” path consistent with the new calling convention.
3775-3789: Good: non-generic class creation path now consistently usesPushNull+compile_call_with_self.Nice reuse of the shared call dispatcher.
3934-3943: Good:withnormal-exit__exit__(None, None, None)call updated forself_or_null.
6113-6144: Call-site refactor looks coherent; please confirmLoadAttrMethodstack contract matchesCallWithSelf*.The split is clean:
- method call:
valuethenLoadAttrMethodthenCallWithSelf*- non-method call:
functhenPushNullthenCallWithSelf*Main thing to verify is that
LoadAttrMethodalways leaves the stack in the[callable, self_or_null]shape expected byCallWithSelf*(especially in edge cases like descriptors / fallback-to-attribute).
6429-6450: Good: comprehension invocation now supplies theNULLreceiver slot before passing the iterator arg.
6858-6874: Good:break/continueunwinding throughwithnow calls__exit__viaCallWithSelfwith an explicitNULL.
3324-3347: CallFunctionPositional is valid and correctly used; no blocker here.
CallFunctionPositionalremains an active, non-deprecated opcode. The stack rearrangement viaSwapto produce[closure, arg1, ..., argN]is correct for this instruction, which expects noself_or_nullslot. The comment "internal call where self_or_null doesn't apply" accurately describes this design choice: the closure is called as a direct function, not a method, so theself_or_nullslot is intentionally avoided for the >0 args case. The 0-arg case usingPushNull+CallWithSelf { nargs: 0 }is also valid and will execute correctly. Both paths will compile and run without stack-shape issues.crates/vm/src/frame.rs (10)
56-57: LGTM: Stack now supports NULL slots for the new calling convention.The change from
BoxVec<PyObjectRef>toBoxVec<Option<PyObjectRef>>enables NULL entries on the stack, which is essential for the self_or_null method call pattern. The comment clearly documents this purpose.Note: This adds a small memory overhead (one discriminant byte per stack slot), but it's necessary for the new calling semantics.
721-735: LGTM: New CallWithSelf instructions implement the self_or_null calling convention.*The three instruction variants (CallWithSelf, CallWithSelfKw, CallWithSelfEx) correctly delegate to
execute_call_with_selfand mirror the structure of the existing Call* instructions. The stack layout comments clearly document the expected state:[callable, self_or_null, arg1, ..., argN].
792-794: LGTM: CopyItem correctly handles Option-wrapped stack slots.The use of
push_value_optpreserves NULL slots during copy operations, which is consistent with the new stack model.
1450-1454: LGTM: PushNull instruction is simple and well-documented.The instruction correctly pushes NULL onto the stack for the self_or_null calling convention.
1809-1813: LGTM: New reserved instruction opcodes.The new reserved opcodes (125, 126, 127, 135) are appropriately handled with unreachable panics, consistent with other placeholder instructions.
2570-2589: LGTM: load_attr_method correctly sets up the stack for the self_or_null calling convention.The implementation properly distinguishes between method descriptors and regular attributes:
- Method descriptor (
PyMethod::Function): Pushes[func, self]- the self argument will be prepended inexecute_call_with_self- Regular attribute (
PyMethod::Attribute): Pushes[attr, NULL]- NULL indicates no self argumentThis correctly implements the calling convention where the stack layout is
[callable, self_or_null, ...]beforeCallWithSelf*instructions.
2774-2801: pop_multiple uses the same unreachable_unchecked pattern as pop_value.The implementation is consistent with
pop_value()- it usesunreachable_unchecked()when encountering NULL slots. The same safety considerations apply here as discussed in thepop_value()review comment.The comment on line 2774 clearly documents that this panics on NULL, which is correct for its use cases (instructions like BuildString, BuildTuple, etc. that expect non-NULL arguments).
2804-2828: LGTM: Stack accessor methods correctly handle NULL slots.The methods properly handle the Option-wrapped stack:
replace_top: Swaps withOption<PyObjectRef>, enabling NULL swapstop_valueandnth_value: Return&PyObjectand panic with clear messages on NULLThe panics in
top_valueandnth_valueare more debuggable thanunreachable_unchecked, which is good for troubleshooting compiler bugs.
2842-2855: LGTM: Debug formatting correctly displays NULL slots.The updated
Debugimplementation properly handles the new Option-wrapped stack, displaying NULL slots as"> NULL", which aids in debugging the self_or_null calling convention.
2639-2647: Consider the debuggability implications ofunreachable_unchecked()inpop_value().The
pop_value()method assumes the bytecode compiler never generates code that pops from a NULL stack slot. When this assumption is violated (e.g., due to a compiler bug), usingunreachable_unchecked()converts what would be a detectable panic into undefined behavior.Trade-offs:
- ✅ Performance: Eliminates runtime check in a frequently-called path
- ❌ Debuggability: Compiler bugs become silent UB instead of panics
NULL values are legitimately used in the stack (e.g.,
push_null()for missingselfin method calls), so this optimization assumes strict separation between slots where NULL is valid vs. where it must be non-NULL. If the bytecode compiler incorrectly generates apop_value()call on a NULL slot, the result is undefined behavior rather than a catchable error.Alternatives to consider:
- Use
panic!()for better runtime detection of compiler bugs- Add a
#[cfg(debug_assertions)]branch that panics while keeping the optimization in release builds- Add validation tests in the bytecode compiler to prevent NULL pops in non-NULL contexts
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/stdlib/src/opcode.rs (1)
48-52:has_arg()has an off-by-one error and excludes opcode 44.
WithHAVE_ARGUMENT = 44, the condition should beopcode >= Self::HAVE_ARGUMENTto match CPython semantics, where opcodes >= 44 require arguments. The currentopcode > Self::HAVE_ARGUMENTincorrectly treats opcode 44 as having no argument.Proposed fix
- pub fn has_arg(opcode: i32) -> bool { - Self::is_valid(opcode) && opcode > Self::HAVE_ARGUMENT - } + pub fn has_arg(opcode: i32) -> bool { + Self::is_valid(opcode) && opcode >= Self::HAVE_ARGUMENT + }crates/jit/src/instructions.rs (1)
460-485: Fix JIT CALL instruction: unwrap() panic and reversed argument orderTwo critical bugs in the CALL handler:
Panic on non-Value arguments:
into_value().unwrap()panics if the argument isNone,Null,Tuple, orFuncRefinstead of returning a proper error. Useok_or(JitCompileError::NotSupported)?instead.Arguments passed in reversed order: Popping from the stack in a loop builds arguments as
[argN, ..., arg1]instead of[arg1, ..., argN]. Call the Cranelift IR instruction with correct argument ordering by addingargs.reverse().The proposed fix addresses both issues:
- Replace
.unwrap()with proper error handling- Use
Vec::with_capacity()for efficiency- Add
args.reverse()to restore correct ordercrates/codegen/src/compile.rs (1)
3717-3785: Critical: genericclasscall path panics on**kwargsand rejects starred bases (*bases).
panic!("keyword argument name must be set")will crash the compiler on validclass C(**kw): ....- Compiling bases via
self.compile_expression(arg)?;will error onExpr::Starred(sinceExpr::Starredis rejected outside annotation context), so validclass C(*bases): ...breaks for generic classes only.This likely needs to share the non-generic
compile_call_inner()machinery (or equivalent) so generic and non-generic classdefs accept the same Python syntax surface.
🧹 Nitpick comments (5)
crates/compiler-core/src/bytecode.rs (2)
1899-1977: Stack-effect math looks correct; consider making bool→i32 conversions more readable.
Call,CallKw, andCallFunctionExnet effects match the documented stack layouts. For readability, you could replace(has_kwargs.get(arg) as i32)withi32::from(has_kwargs.get(arg)).
2141-2252: Disassembly output for RESERVED opcodes is inconsistent (minor).
Some reserved variants go throughw!(RESERVED)while others print a raw"RESERVED"without padding. Consider routing all reserved variants through the same formatting path for stable disassembly output.Proposed tweak (unify reserved formatting)
- Reserved125 | Reserved126 | Reserved127 | Reserved135 => { - write!(f, "RESERVED") - } + Reserved125 | Reserved126 | Reserved127 | Reserved135 => w!(RESERVED),crates/vm/src/frame.rs (2)
2371-2399:SET_FUNCTION_ATTRIBUTE: NULL-safety is a bit indirect; consider failing fast on NULL func.
Right now, a NULLfuncwould be placed back on stack and later triggertop_value()fatal. Consider unwrappingfuncimmediately (compiler bug) to keep the failure localized.Possible tightening
- let func = self.pop_value_opt(); + let func = Some(self.pop_value()); let attr_value = self .replace_top(func) .expect("replace_top encountered NULL");
1097-1101: Use thetargetvalue returned by descriptor resolution instead of discarding it.In
load_attr_method, thePyMethod::Functionpattern destructurestargetbut discards it with_, then pushesparentinstead. This pattern should use thetargetvalue that descriptor resolution explicitly returned, which is both more semantically clear and follows Rust idioms of not discarding values unnecessarily.Suggested fix
PyMethod::Function { target: _, func } => { - // Method descriptor found: push [method, self] + // Descriptor resolved: push [callable, self_or_null] where self_or_null is `target` self.push_value(func); - self.push_value(parent); // self + self.push_value(target); }crates/codegen/src/compile.rs (1)
3315-3352: Type-params scope call: stack shuffling is plausible but fragile—consider encapsulating.
ThisReverse/Swap/Reversesequence is easy to regress; a small helper (e.g., “emit_call_with_null_and_n_args(n)”) would make invariants clearer.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_xml_etree.pyis excluded by!Lib/**crates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_double_async_with.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
crates/codegen/src/compile.rscrates/compiler-core/src/bytecode.rscrates/jit/src/instructions.rscrates/stdlib/src/opcode.rscrates/vm/src/frame.rscrates/vm/src/vm/method.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/vm/src/vm/method.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/stdlib/src/opcode.rscrates/vm/src/frame.rscrates/compiler-core/src/bytecode.rscrates/jit/src/instructions.rscrates/codegen/src/compile.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests on wasm-wasi
🔇 Additional comments (21)
crates/compiler-core/src/bytecode.rs (1)
802-821: Good: opcode-level self_or_null calling convention is explicit and consistent with the new LOAD_ATTR_METHOD.
The stack layout comments forCall,CallKw,CallFunctionEx, andLoadAttrMethodline up with the new protocol and make downstream implementations easier to reason about.Also applies to: 1046-1092
crates/stdlib/src/opcode.rs (1)
64-83: Correct update:has_name()now includesLoadAttrMethod.
This keeps the stdlibopcodemodule aligned with the bytecode refactor away fromLoadMethod.crates/jit/src/instructions.rs (2)
24-59: Nice: explicitJitValue::Nullkeeps JIT stack model aligned with VM self_or_null semantics.
The “Null is non-materializable” handling into_jit_type,into_value, andboolean_valis consistent.Also applies to: 129-147
486-489:PushNullsupport in the JIT is the right primitive for this calling convention.crates/vm/src/frame.rs (7)
52-60: Good: Option-wrapped stack +push_null/pop_value_optcleanly model NULL-aware bytecode.
This is a coherent mechanical refactor and the helper APIs make the new invariant obvious.Also applies to: 2590-2627
708-722: Call protocol wiring looks right: args collected first, thenself_or_null, then callable.
execute_call()prependingselfwhen present matches the new stack contract forLOAD_ATTR_METHOD+CALL*.Also applies to: 1437-1441, 2162-2184
677-686: VerifyBUILD_STRING’sWtf8Bufcollection compiles + is intended.
let s: Wtf8Buf = parts.iter().map(|pyobj| pyobj.downcast_ref::<PyStr>().unwrap()).collect();depends on aFromIterator<&PyStr>(or compatible) impl. If it doesn’t exist, you’ll need to explicitly concatenate.as_wtf8()/.as_str()into a buffer.
1900-1957: Good: exception unwinding now discards NULL slots safely.
Popping toentry.depthwithout assumingSome(_)is exactly what the NULL-aware stack needs.
2812-2838: Debug output improvement is solid (NULL slots are visible).
This will help a lot when tracking stack-shape issues during the opcode transition.
2753-2774: Verify compiler invariants ensure stack slots are never NULL when usingpop_value(). The code deliberately pushes NULL (None) viapush_null(), then relies onexpect_unchecked()to extract values. In debug builds this panics safely, but in release buildsunwrap_unchecked()silently causes UB if a NULL slot is popped. This requires ironclad invariants: every call site usingpop_value()must guarantee the popped slot contains a value;pop_value_opt()should be used when NULL is possible. If the compiler ever incorrectly allows a NULL slot to be popped viapop_value(), the result is silent undefined behavior in release builds.
1-20: Runcargo fmt,cargo clippy, and targeted tests to verify this opcode-wide change.This review cannot be fully verified in the current environment. The import statements shown appear properly formatted and follow Rust conventions. However, verification requires:
cargo fmt --allto ensure code formatting compliancecargo clippy --all-targets --all-features -- -D warningsto check for lints- Targeted tests for
PushNull/LoadAttrMethod/Callopcode sequence integration, including@classmethodand JIT recursive call scenariosThe implementation of related opcodes and descriptors exists in the codebase, but complete validation depends on toolchain execution.
crates/codegen/src/compile.rs (10)
1871-1888: Assert raising:PushNullinsertion matches new call convention.
2097-2111: Decorator application:[decorator, NULL, arg]stack discipline is coherent.
The “push decorator then PushNull” scheme + calling in source order correctly applies decorators from the innermost outward via stack order.
2113-2155: Type-param bound/default closure call:PushNullbeforeCall { nargs: 0 }matches convention.
3786-3800: Non-genericclasscall: good reuse ofcompile_call_inner()+compile_call_with_self(), but currently inherits the Ex-opcode bug (see below).
3945-3960:withnormal-exit__exit__call:PushNullplacement is correct for[__exit__, NULL, None, None, None].
6440-6463: Comprehension call:PushNullbeforeCall { nargs: 1 }aligns with unified call convention.
6867-6885: Break/continue with-unwind: call shape updated, but confirm block-pop semantics vs return-unwind.
This path now doesPushNull+Call { nargs: 3 }, but (unlike return unwinding) doesn’t emitPopBlock. Please confirm the VM/exception-table model doesn’t require it here.
1-80: Housekeeping: run rustfmt + clippy for this PR branch.
As per coding guidelines, please ensurecargo fmtandcargo clippyare clean after these opcode/stack changes.
6124-6156: CallFunctionEx already correctly handles the self_or_null stack layout—no change needed.The CallFunctionEx opcode's documentation in both bytecode.rs and frame.rs explicitly shows it expects and handles the self_or_null slot:
Stack: [func, self_or_null, callargs, kwargs?]. The stack effect calculation confirms it consumes all four components (callable, self_or_null, args_tuple, optional kwargs dict). The code at line 6153 is correct as-is; compile_call ensures self_or_null is always on the stack before calling compile_call_with_self, and CallFunctionEx is the appropriate opcode for the Ex case. There is no CallWithSelfEx opcode to propose—CallFunctionEx is already the self-aware variant.Likely an incorrect or invalid review comment.
1027-1055: RemovePopBlockfrom With/AsyncWith exception unwinding or add it to break/continue path for consistency.The
unwind_fblock()method emitsInstruction::PopBlockfor With/AsyncWith blocks (line 1031), but thecompile_break_continue()method does not emit it when unwinding the same block type (line 6870). This is inconsistent with the pattern used elsewhere in the codebase: TryExcept, FinallyTry, and other block setup methods explicitly avoid emitting PopBlock with comments stating "exception table handles this". Since the VM treats PopBlock as a pseudo-instruction (per frame.rs), this instruction should either be removed from the exception unwinding path entirely, or compile_break_continue should emit it as well for consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/codegen/src/compile.rs (1)
3694-3777: Generic class creation:panic!on**kwargsis a functional regression (should compile or error gracefully).
keyword.arg == Noneis reachable for**kwargsin class bases/keywords; panicking in codegen is too harsh and likely breaks valid code. Prefer routing**kwargsthrough the Ex call path (the new “WithSelfEx” opcode) or returning a structuredCodegenError.Proposed direction (sketch)
- If
arguments.keywords.iter().any(|k| k.arg.is_none()):
- Build positional args as today (including
.generic_base)- Build kwargs map via
compile_keywords(&arguments.keywords)?- Emit the new Ex call opcode (with self slot already present due to
PushNullafterLoadBuildClass)- Else keep the
CallKwfast-path.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_xml_etree.pyis excluded by!Lib/**crates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_double_async_with.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
crates/codegen/src/compile.rscrates/compiler-core/src/bytecode.rscrates/jit/src/instructions.rscrates/stdlib/src/opcode.rscrates/vm/src/frame.rscrates/vm/src/vm/method.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/stdlib/src/opcode.rs
- crates/vm/src/vm/method.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/jit/src/instructions.rscrates/codegen/src/compile.rscrates/vm/src/frame.rscrates/compiler-core/src/bytecode.rs
🧬 Code graph analysis (2)
crates/codegen/src/compile.rs (2)
crates/compiler-core/src/bytecode.rs (3)
try_from(1123-1129)try_from(1150-1155)try_from(1164-1170)crates/vm/src/builtins/code.rs (5)
args(434-443)args(446-455)args(458-467)args(470-479)args(497-505)
crates/vm/src/frame.rs (2)
crates/vm/src/protocol/object.rs (1)
s(379-379)crates/vm/src/vm/method.rs (1)
get(22-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (32)
crates/jit/src/instructions.rs (3)
30-30: LGTM! Null variant properly integrated.The
JitValue::Nullvariant is consistently handled across all value operations:
to_jit_typecorrectly returnsNone(not JIT-representable)into_valuereturnsNone(no Cranelift Value)boolean_valreturnsNotSupported(cannot convert to boolean)This aligns with the new self_or_null calling convention where NULL is a placeholder, not an actual value.
Also applies to: 49-49, 56-56, 143-145
460-485: Verify: JIT only supports direct recursive calls, not method calls.The Call instruction now requires
self_or_nullto beNull, returningNotSupportedfor any other case. This means the JIT currently cannot compile method calls (whereself_or_nullwould beSome(self)).This is a reasonable limitation for the initial implementation, but consider:
- Adding a comment explaining this limitation
- Tracking this as a future enhancement (JIT support for method calls)
Based on the PR context, is method call support in JIT planned for future work? If so, consider adding a TODO comment.
486-489: LGTM! PushNull instruction correctly implemented.The instruction properly pushes a
JitValue::Nullonto the JIT stack, matching the VM'spush_nullsemantics for the self_or_null calling convention.crates/vm/src/frame.rs (12)
56-56: Major change: Stack is now NULL-aware.The stack type changed from
BoxVec<PyObjectRef>toBoxVec<Option<PyObjectRef>>, enabling explicit NULL slots for the self_or_null calling convention. This is a fundamental change that affects all stack operations throughout the file.
613-619: LGTM! BinarySubscript cleaned up.The instruction now properly uses
push_value(result)instead of direct stack manipulation, which automatically wraps the value inSome()for the NULL-aware stack.
677-686: LGTM! BuildString correctly handles NULL-aware stack.The instruction now collects parts using
pop_multiple(which panics on NULL) and builds the string from the collected parts. This is correct since the compiler should never emitBuildStringwith NULL arguments on the stack.
708-722: LGTM! Unified call semantics with self_or_null protocol.All call variants (
Call,CallKw,CallFunctionEx) now follow the unified calling convention:
- Pop
self_or_nullusingpop_value_opt()(returnsOption<PyObjectRef>)- If
Some(self_val), prepend to args (method call)- If
None(NULL), use args as-is (regular function call)This correctly implements the new self_or_null protocol described in the PR objectives.
Also applies to: 2163-2185
1437-1441: LGTM! PushNull instruction correctly implemented.The instruction properly pushes a NULL slot onto the stack using
push_null(), which wrapsNonein the Option-based stack representation.
1169-1171: LGTM! LoadAttrMethod implements the new method loading protocol.The instruction correctly distinguishes between:
- Method descriptors: Pushes
[func, self]for method calls- Regular attributes: Pushes
[attr, NULL]for attribute accessThis enables the unified
Callinstruction to handle both cases by checking theself_or_nullslot. The implementation aligns with the method resolution logic shown in the relevant code snippets fromcrates/vm/src/vm/method.rs.Also applies to: 2553-2572
2592-2627: LGTM! Stack operations properly handle NULL slots.The stack operation methods are well-designed:
push_value(obj)wraps inSome()for regular valuespush_null()pushesNonefor NULL slotspop_value_opt()returnsOption<PyObjectRef>for NULL-aware popspop_value()panics on NULL (for instructions that expect values)The separation between NULL-aware and NULL-asserting operations makes the API clear and helps catch compiler bugs in debug mode.
2854-2860: Verify: unchecked unwrap relies on compiler correctness.The
expect_uncheckedhelper usesunwrap_unchecked()in release builds for performance, relying on the compiler to never emit bytecode that puts NULL where a value is expected. This is a common optimization pattern but has safety implications:Debug builds: Panics with helpful message on NULL
Release builds: Undefined behavior if NULL is encounteredThis is acceptable if the compiler is thoroughly tested and the bytecode validator prevents invalid bytecode. However, consider:
- Is there a bytecode validator that runs before execution?
- Are there comprehensive tests covering all bytecode patterns with the new NULL semantics?
Please confirm that the compiler is designed to never emit NULL in value positions, or that there's validation to catch such cases.
767-781: LGTM! CopyItem correctly handles NULL slots.The instruction now preserves NULL slots when copying stack elements, using
push_value_opt()to maintain theOption<PyObjectRef>semantics. This is correct since NULL is a valid stack value in the new calling convention.
1650-1654: LGTM! WithExceptStart safely accesses exit.The instruction uses
expect_uncheckedto access__exit__from a known stack position. This is correct since the compiler guarantees__exit__is placed atstack_len - 4byBeforeWith, and it should never be NULL.
2754-2802: LGTM! Stack accessor methods correctly handle NULL slots.The accessor methods properly handle the NULL-aware stack:
pop_multiple: Panics on NULL (used for collecting arguments)replace_top: Preserves NULL (allows swapping NULL slots)top_value/nth_value: Panic on NULL (used for value access)The panic-on-NULL behavior for value accessors is correct since these are used in contexts where the compiler should guarantee a value, not NULL.
2813-2839: LGTM! Debug formatting shows NULL slots.The debug implementation now properly renders NULL slots as
"> NULL", making it easier to debug stack states and identify issues with the NULL-aware calling convention.crates/codegen/src/compile.rs (10)
1027-1055: With/AsyncWith unwind:PushNull+Call { nargs: 3 }stack shape looks correct.The
__exit__(None, None, None)emission now matches the[callable, self_or_null, args…]convention and preservespreserve_tossemantics via the surroundingSwaps.
1871-1894: Assert error path:PushNullplacement is correct for callingAssertionError(...).This aligns the stack for
Call { nargs: 0/1 }without relying on legacy call opcodes.
2097-2111: Decorator call convention: pushingNULLper decorator is consistent with laterCall { nargs: 1 }.The resulting stack evolution correctly applies decorators from the topmost decorator down.
2113-2154: Type-param bound/default thunk:PushNull+Call { nargs: 0 }is the right convention for immediate invocation.LGTM; the thunk creation + immediate call sequence is now self_or_null-safe.
3778-3792: Non-generic class creation: switching toPushNull+compile_call_with_self(...)is the right consolidation.Nice simplification vs duplicating call-shape logic here.
3937-3957: With-statement normal exit:PushNullinsertion matches the new call ABI.This keeps the
__exit__call consistent with unwind-time__exit__emission.
6116-6137:compile_call: method vs non-method split usingLoadAttrMethod/PushNullis consistent with the new ABI.This is the right place to centralize the “self_or_null slot” decision.
6432-6450: Comprehension call site:PushNull+Call { nargs: 1 }matches the new convention.LGTM with the iterator argument evaluation following the inserted NULL slot.
6862-6877: break/continue unwind throughwith:PushNull+Call { nargs: 3 }is consistent with other__exit__call sites.This keeps unwind-time
__exit__calls ABI-consistent.
6139-6147: The current code is correct.CallFunctionExalready handles theself_or_nullslot, as documented in the bytecode definition (stack layout:[func, self_or_null, callargs, kwargs?]) and confirmed by the runtime handler inframe.rs:2165which explicitly pops and conditionally prependsself_or_nullto the arguments. NoCallWithSelfExopcode exists, and no change to line 6145 is needed.crates/compiler-core/src/bytecode.rs (7)
802-821: LGTM: Clear instruction definitions with explicit self_or_null calling convention.The new Call, CallFunctionEx, and CallKw instructions are well-documented with clear stack layout comments that explicitly show the self_or_null slot positioning. This makes the new calling convention easy to understand for anyone working with these opcodes.
1046-1049: LGTM: Reserved opcodes properly positioned.The Reserved125-127 and Reserved135 opcodes are correctly positioned in the instruction enum and maintain the opcode numbering scheme. These placeholders are handled consistently throughout the codebase.
Also applies to: 1072-1073
1086-1089: LGTM: LoadAttrMethod properly defined for the new calling convention.The LoadAttrMethod instruction is correctly defined with clear documentation showing it pushes both the method and self_or_null onto the stack, establishing the calling convention expected by the new Call instructions.
1902-1907: LGTM: Correct stack effects for reserved and placeholder instructions.The stack effect calculations are accurate:
- Reserved opcodes correctly return 0 (never executed)
- PushNull correctly returns +1 (pushes NULL for self_or_null slot in plain function calls)
1937-1938: LGTM: Stack effects correctly implement the new calling convention.The stack effect calculations are mathematically correct and consistent with the documented stack layouts:
- LoadAttrMethod: Correctly pops 1 (obj) and pushes 2 (method + self_or_null), net +1
- Call: Correctly pops nargs + 2 (self_or_null + callable) and pushes 1, net -(nargs+1)
- CallKw: Correctly pops nargs + 3 (kw_names + self_or_null + callable) and pushes 1, net -(nargs+2)
- CallFunctionEx: Correctly pops has_kwargs + 3 (kwargs? + args + self_or_null + callable) and pushes 1, net -(has_kwargs+2)
Also applies to: 1971-1976
2142-2143: LGTM: Consistent display logic for reserved opcodes.All reserved opcodes (3, 17, 125-127, 135, 140, 142-148) consistently display as "RESERVED" in disassembly output, making it clear these are placeholder opcodes not used in execution.
Also applies to: 2250-2252
2193-2194: LGTM: Clear and consistent display names for new instructions.The display logic properly formats the new opcodes with clear, CPython-compatible names:
- Call → "CALL"
- CallFunctionEx → "CALL_FUNCTION_EX"
- CallKw → "CALL_KW"
- LoadAttrMethod → "LOAD_ATTR_METHOD"
All arguments are correctly formatted using the appropriate getters.
Also applies to: 2197-2197, 2233-2233
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/codegen/src/compile.rs (3)
3929-3944:withnormal-exit path: PushNull + Call(3) matches the new call convention.
Minor: the big comment block above still showsCALL 2for__exit__(None, None, None); consider updating it to avoid confusion.
3703-3770: Generic class build path breaks*basesand**kwargs(and panics on**kwargs).
for arg in &arguments.args { self.compile_expression(arg)?; }rejectsExpr::Starred(*bases) with error "cannot use starred expression here"panic!("keyword argument name must be set")crashes when**kwargsis present in class definitionsBoth
*basesand**kwargsare valid Python syntax that must be supported. Use theCallFunctionExinstruction withgather_elementsandcompile_keywords(which already handle these cases correctly) when unpacking is present, and preserve the fastCall/CallKwpath for simple cases.
3297-3337: Incorrect stack argument order when calling type-params closure with multiple arguments.For
num_typeparam_args >= 2, the current Swap/PushNull/Swap/Reverse sequence produces arguments in reverse order. For N=2, the code produces[closure, NULL, arg2, arg1]instead of[closure, NULL, arg1, arg2]. This breaks generic functions with multiple type parameters that have defaults.Note: The proposed fix in the diff also does not produce the correct result and requires further revision.
🧹 Nitpick comments (4)
crates/vm/src/frame.rs (2)
2553-2572: Verify the unusedtargetin the Function variant.In the
PyMethod::Function { target: _, func }arm, thetargetis extracted from thePyMethodbut immediately discarded via_. However, on line 2563,parent(the original object) is pushed asselfinstead oftarget.According to
PyMethod::get,targetwould be the same as the original object passed in, but the naming discrepancy is confusing. Consider usingtargetdirectly for clarity:- PyMethod::Function { target: _, func } => { + PyMethod::Function { target, func } => { // Method descriptor found: push [method, self] self.push_value(func); - self.push_value(parent); // self + self.push_value(target); // self }This makes the code self-documenting and avoids holding onto
parentlonger than needed.
2381-2384: Consider usingpop_value()for consistency.Since
funcon the stack should never be NULL at this point (it was just created byMakeFunction), usingpop_value_opt()is slightly misleading. Theexpect()on line 2384 confirms the expectation that the value is non-NULL.♻️ Suggested refactor for consistency
-let func = self.pop_value_opt(); -let attr_value = self - .replace_top(func) - .expect("replace_top encountered NULL"); +let func = self.pop_value(); +let attr_value = self + .replace_top(Some(func)) + .expect("replace_top encountered NULL");crates/codegen/src/compile.rs (2)
2091-2105: Decorator protocol is consistent:prepare_decoratorspushes NULL,apply_decoratorsusesCall(1).
The resulting runtime stack shape[decorator, NULL, decorated_obj]per decorator application matches the comment.
6128-6184:compile_call_helper: good consolidation, but double-check EX-call opcode naming + avoid unchecked arg count math.
- The helper emits
Instruction::CallFunctionEx { has_kwargs }; ensure this is the intended opcode in this PR (some parts mentionCallWithSelfEx).let count = u32::try_from(arguments.len()).unwrap() + additional_positional;can wrap on overflow in release builds; preferchecked_add+ a compilation error.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_xml_etree.pyis excluded by!Lib/**crates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_double_async_with.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
crates/codegen/src/compile.rscrates/compiler-core/src/bytecode.rscrates/jit/src/instructions.rscrates/stdlib/src/opcode.rscrates/vm/src/frame.rscrates/vm/src/vm/method.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/stdlib/src/opcode.rscrates/vm/src/vm/method.rscrates/jit/src/instructions.rscrates/compiler-core/src/bytecode.rscrates/codegen/src/compile.rscrates/vm/src/frame.rs
🧬 Code graph analysis (1)
crates/compiler-core/src/bytecode.rs (2)
crates/codegen/src/compile.rs (1)
name(1205-1207)crates/codegen/src/ir.rs (1)
idx(47-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Run tests under miri
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (20)
crates/stdlib/src/opcode.rs (1)
77-77: LGTM!The opcode variant rename from
LoadMethodtoLoadAttrMethodis consistent with the broader instruction consolidation in this PR.crates/vm/src/vm/method.rs (1)
41-49: Correct handling of classmethod descriptors.The logic correctly routes classmethods through
descr_getto ensure the method is bound to the class rather than the instance. ForPyMethodDescriptorwith theCLASSflag, the descriptor protocol is invoked to properly convertinstance → class, while other method descriptors continue to use direct method binding.crates/jit/src/instructions.rs (2)
30-30: LGTM - JitValue::Null aligns with the NULL-aware call protocol.Adding the
Nullvariant properly models the self_or_null slot in the new call convention.
460-489: JIT correctly implements the new call protocol with an expected limitation.The implementation correctly:
- Pops
self_or_nullafter collecting arguments (lines 469-473)- Requires
self_or_nullto beNullfor JIT-compiled recursive calls- Returns
NotSupportedfor method calls whereself_or_nullis notNullThis means JIT currently only supports simple function calls (via
PushNull→Call), not method calls. This is a reasonable constraint for the initial implementation since method calls would require runtime dispatch logic.crates/vm/src/frame.rs (5)
56-56: Core architectural change: NULL-aware stack.The stack type change from
BoxVec<PyObjectRef>toBoxVec<Option<PyObjectRef>>enables the new call protocol whereNonerepresents a NULL slot for theself_or_nullposition. This is a significant but well-contained change that aligns with the newCall/CallKw/LoadAttrMethodinstruction semantics.
2163-2185: Clean implementation of the self_or_null call protocol.The
execute_callimplementation correctly handles both cases:
Some(self_val): Method call - prepends self to argsNone: Regular function call - uses args as-isThis provides a unified call path that works for both method and function calls based on what
LoadAttrMethodpushed onto the stack.
2854-2860: Good performance optimization with debug safety net.The
expect_uncheckedhelper appropriately usesunwrap_unchecked()in release builds while maintaining safety checks in debug builds. This pattern is reasonable for hot paths where the compiler guarantees non-NULL values.
1437-1441: PushNull instruction correctly implemented.The
PushNullinstruction simply callspush_null()to place a NULL marker on the stack, which is used by the call protocol to indicate "no self argument".
708-722: Call instructions correctly delegate to execute_call.All three call variants (
Call,CallKw,CallFunctionEx) now share the sameexecute_callpath, with the difference only in how arguments are collected. This is a clean unification of the call handling logic.crates/compiler-core/src/bytecode.rs (4)
1046-1098: LGTM: Reserved placeholders and LoadAttrMethod properly defined.The reserved instruction slots (125-127 for old CallMethod* opcodes, 135 for LoadMethod) maintain opcode numbering stability during the transition. The new
LoadAttrMethodinstruction at opcode 141 is well-positioned and clearly documented.
2139-2252: LGTM: Display formatting is consistent and follows CPython conventions.The instruction display names (
CALL,CALL_KW,CALL_FUNCTION_EX,LOAD_ATTR_METHOD) align with CPython 3.13 opcode naming. Reserved placeholder instructions are uniformly displayed asRESERVED, which is clear and appropriate.
1899-1976: Stack effect calculations are correct.The stack depth calculations correctly account for the self_or_null protocol used in call instructions:
- Call:
-(nargs + 1)(pops callable, self_or_null, and nargs arguments; pushes result)- CallKw:
-(nargs + 2)(additionally pops kw_names_tuple)- CallFunctionEx:
-(has_kwargs + 2)(pops callable, self_or_null, args_tuple, and optionally kwargs)- LoadAttrMethod:
+1(pops obj, pushes method and self_or_null)- PushNull:
+1(pushes NULL)These stack effects are actively used in the
max_stackdepth()calculation incrates/codegen/src/ir.rs, which validates stack depth by iterating through all instructions and tracking cumulative stack changes with overflow/underflow checks.
802-821: New call instruction definitions are correctly implemented with proper stack semantics.The
Call,CallFunctionEx, andCallKwinstructions properly define the updated calling convention with theself_or_nullprotocol. Stack layout comments accurately describe the operand order, stack effects are correctly calculated, and both the compiler (codegen/src/compile.rs) and VM (vm/src/frame.rs) correctly emit and handle these instructions with the updated stack semantics. The deprecatedCallMethod*instructions have been completely removed from the codebase.crates/codegen/src/compile.rs (7)
1021-1049:with/async withunwind: PushNull + Call(3) stack shape looks correct.
Thepreserve_tosswap puts__exit__back on TOS beforePushNull, soCall { nargs: 3 }consumes[__exit__, NULL, None, None, None]and leaves the preserved TOS intact.
1866-1892: Assert raising path: PushNull + Call(0/1) is consistent with the new call convention.
This correctly createsAssertionError(...)via[callable, NULL, args...].
2106-2148: Type-param bound/default closure: immediatePushNull+Call(0)looks correct.
This ensures the bound/default thunk is invoked under the same[callable, NULL]contract as other non-method calls.
3771-3784: Non-generic class build:PushNull+compile_call_helper(2, ...)is the right direction.
This reuses the centralized call logic and should correctly support*bases/**kwargshere.
6108-6126:compile_call: method calls viaLoadAttrMethod, regular calls viaPushNulllook consistent.
This cleanly enforces a single stack contract across both cases.
6414-6434: Comprehension call site:PushNullbeforeCall(1)matches the unified call ABI.
6840-6858: Break/continue unwind throughwith:PushNull+Call(3)is consistent with other__exit__call sites.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/codegen/src/compile.rs (1)
3687-3784: Genericclasscreation with type parameters likely regresses support for*basesand**kwargs, and crashes codegen with a panic on the latter.
- Lines 3718:
for arg in &arguments.args { self.compile_expression(arg)?; }will fail withInvalidStarExprif any base isExpr::Starred(e.g.,class C[T](*bases): ...), whereasclass C(*bases): ...works viacompile_call_helper→gather_elements, which explicitly handles starred expressions.- Lines 3735–3741: When
keyword.arg.is_none()(indicating**kwargsunpacking), the code panics instead of routing through proper handling. The non-generic path correctly detects this viahas_double_starand callscompile_keywords, which usesCallFunctionEx. Example:class C[T](**kw): ...will panic at codegen time, whileclass C(**kw): ...works (confirmed inextra_tests/snippets/syntax_class.pyline 265).Align the generic path with the non-generic path: replace the manual loop and keyword handling with calls to
gather_elementsandcompile_keywords, and never panic on user input (returnCodegenErrorinstead). Add tests for these cases with type parameters.
🤖 Fix all issues with AI agents
In @crates/codegen/src/compile.rs:
- Around line 3304-3337: The current Swap/PushNull/Swap + Reverse sequence can
reorder arguments for num_typeparam_args >= 3; replace it with a stable "bubble
down" sequence so NULL is inserted under the closure without disturbing arg
order: after computing swap_depth and emitting Instruction::Swap { index:
swap_depth } to bring the closure to the top and emitting Instruction::PushNull,
emit a series of Instruction::Swap operations that iteratively bubble the NULL
down one slot at a time across the num_typeparam_args positions (i.e., perform
num_typeparam_args swaps with appropriate decreasing indices) and then call
Instruction::Call { nargs: num_typeparam_args as u32 }; remove the existing
single Swap+Reverse approach and use this loop-based swap sequence using the
same swap_depth and num_typeparam_args symbols.
🧹 Nitpick comments (1)
crates/vm/src/frame.rs (1)
2790-2798: Consider using expect_unchecked for consistency and safety.The
nth_valuemethod usesunreachable_unchecked()for NULL slots, which results in undefined behavior if a NULL is encountered in release builds. For consistency withtop_valueand better safety, consider usingexpect_unchecked(defined at line 2850) which provides debug assertions:♻️ Proposed refactor
#[inline] #[track_caller] fn nth_value(&self, depth: u32) -> &PyObject { let stack = &self.state.stack; - match &stack[stack.len() - depth as usize - 1] { - Some(obj) => obj, - None => unsafe { std::hint::unreachable_unchecked() }, - } + expect_unchecked( + stack[stack.len() - depth as usize - 1].as_ref(), + "nth_value but null found. This is a compiler bug.", + ) }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_xml_etree.pyis excluded by!Lib/**crates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_double_async_with.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
crates/codegen/src/compile.rscrates/compiler-core/src/bytecode.rscrates/jit/src/instructions.rscrates/stdlib/src/opcode.rscrates/vm/src/frame.rscrates/vm/src/vm/method.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/stdlib/src/opcode.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/vm/src/vm/method.rscrates/jit/src/instructions.rscrates/codegen/src/compile.rscrates/vm/src/frame.rscrates/compiler-core/src/bytecode.rs
🧬 Code graph analysis (1)
crates/codegen/src/compile.rs (1)
crates/vm/src/builtins/code.rs (5)
args(434-443)args(446-455)args(458-467)args(470-479)args(497-505)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests (macos-latest)
🔇 Additional comments (36)
crates/vm/src/vm/method.rs (1)
41-49: LGTM! Classmethod descriptor handling is well-structured.The conditional logic correctly differentiates between classmethod descriptors (which need
descr_getto convert the instance to a class) and regular method descriptors (which preserve the existingis_methodbehavior). This aligns with the broader self_or_null protocol introduced in this PR.crates/jit/src/instructions.rs (4)
30-30: LGTM! Null variant integrated consistently.The
JitValue::Nullvariant is properly integrated across all relevant methods (to_jit_type,into_value), treating it as a non-representable type similar toNone,Tuple, andFuncRef. This aligns with the new NULL-aware stack protocol.Also applies to: 49-49, 56-56
143-145: LGTM! Appropriate error handling for Null.The
boolean_valmethod correctly returnsNotSupportedforNull, treating it consistently with other non-boolean-convertible types likeTupleandFuncRef.
486-489: LGTM! PushNull instruction is straightforward.The new
PushNullinstruction correctly pushesJitValue::Nullonto the stack, supporting the explicit Null construction needed for the new calling convention.
460-485: The existing comment at line 469 already explains this is intentional: "should be Null for JIT-compiled recursive calls." This restriction is by design, not an oversight. No changes needed.Likely an incorrect or invalid review comment.
crates/compiler-core/src/bytecode.rs (6)
802-805: LGTM! Call instruction well-documented.The new
Callinstruction with its stack layout comment[callable, self_or_null, args...]clearly documents the calling convention. The instruction definition is clean and straightforward.
806-821: LGTM! CallFunctionEx and CallKw instructions are consistent.Both
CallFunctionExandCallKwinstructions include clear stack layout comments showing theself_or_nullslot, maintaining consistency with the new calling convention.
1899-1904: LGTM! Stack effects for reserved instructions and PushNull are correct.Reserved instructions appropriately return 0 stack effect, and
PushNullcorrectly returns +1 as it pushes one NULL value onto the stack.
1934-1936: LGTM! Stack effect calculations are correct for the new calling convention.The stack effects for
LoadAttrMethod,Call,CallKw, andCallFunctionExcorrectly account for theself_or_nullslot in the new calling convention:
LoadAttrMethod: pops obj, pushes method + self_or_null (net +1) ✓Call: pops callable + self_or_null + nargs, pushes result (net -nargs - 1) ✓CallKw: pops callable + self_or_null + nargs + kw_names, pushes result (net -nargs - 2) ✓CallFunctionEx: pops callable + self_or_null + args + kwargs?, pushes result (net -(has_kwargs) - 2) ✓Also applies to: 1968-1973
2139-2141: LGTM! Display formatting is consistent.The
fmt_dismethod correctly maps all new instructions (Call,CallKw,LoadAttrMethod) and reserved slots to their appropriate display names. The reserved instructions consistently show as "RESERVED".Also applies to: 2190-2190, 2194-2194, 2230-2230, 2247-2249
1046-1049: LoadAttrMethod is a distinct instruction with its own implementation, not a conversion to LOAD_ATTR.
LoadAttrMethodhas a separate implementation incrates/vm/src/frame.rs(line 1167) that callsload_attr_method(), not a conversion toLOAD_ATTR. It also has distinct stack semantics (documented at line 1934–1935 in bytecode.rs): it pops an object and pushes both a method and aself_or_nullslot, unlike regularLOAD_ATTR.Regarding reserved instruction slots:
Reserved140–148are clearly documented as "padding to keep RESUME at 149," whileReserved125–127lack explicit purpose comments. All reserved slots are designated as "dummy/placeholder instructions (never executed)" in the codebase. The total count is 12 for the two ranges mentioned (3 + 9), not 13.Likely an incorrect or invalid review comment.
crates/codegen/src/compile.rs (7)
1021-1049:with/async withunwind:PushNullplacement looks correct for the new[callable, self_or_null, args…]convention.
PushNulloccurs after anySwapused to preserve TOS, so the call consumes__exit__+NULL+ 3 args without disturbing the preserved value.
1865-1889:assertfailure path correctly addsPushNullbefore callingAssertionError(...).
This matches the new call ABI for bothnargs: 0andnargs: 1.
2091-2105: Decorator stack protocol is consistent ([decorator, NULL]thenCALL nargs=1).
The comment inapply_decoratorsmatches the actual stack shape produced byprepare_decorators.
2106-2147: Type-param bound/default closure execution correctly usesPushNull+Call { nargs: 0 }.
3932-4042: Other updated call-sites (withnormal exit, comprehension call, break/continue unwind) follow the newPushNull+Call { nargs }convention consistently.Also applies to: 6414-6434, 6844-6858
1-7460: Repo hygiene: please runcargo fmt+cargo clippyfor the Rust changes.Ensure all formatting and linting passes before finalizing this PR.
6108-6184: Stack order and instruction naming assumptions are correct. No issues found with thecompile_call/compile_call_helperrefactor.
LoadAttrMethodcorrectly leaves[method, self_or_null]on the stack (confirmed in bytecode definition with stack effect +1 after popping object).CallFunctionExis the correct instruction name; no rename occurred in this PR series.- Both regular calls (via
PushNull) and method calls (viaLoadAttrMethod) produce matching stack layouts forCallFunctionExconsumption.crates/vm/src/frame.rs (18)
56-56: LGTM: Stack type updated to support NULL slots.The stack field is now
BoxVec<Option<PyObjectRef>>, enabling the NULL-aware self_or_null call protocol. This is a foundational change that coordinates with the new instruction set.
613-619: LGTM: BinarySubscript correctly adapted for new stack semantics.The instruction properly pops two values, calls
get_item, and pushes the result. Thepush_valuemethod now internally wraps the object inSome, maintaining correctness.
677-684: LGTM: BuildString correctly adapted.The instruction collects PyStr objects from the stack and assembles them into a Wtf8Buf. The
pop_multiplemethod panics on NULL slots, which is appropriate here since the compiler guarantees non-NULL PyStr objects on the stack.
706-720: LGTM: Call instructions updated for new call protocol.The three call instructions (
Call,CallKw,CallFunctionEx) are correctly dispatched with appropriate argument collection. The stack comments accurately reflect the self_or_null protocol. These instructions coordinate with the updatedexecute_callmethod to handle method calls.
765-780: LGTM: CopyItem correctly handles NULL-aware slots.The instruction now copies
Option<PyObjectRef>slots directly usingpush_value_opt, which correctly propagates NULL slots when present.
1435-1439: LGTM: PushNull instruction added for call protocol.This new instruction pushes a NULL placeholder onto the stack, used in the self_or_null slot when calling regular attributes (non-methods). The implementation correctly calls
push_null().
1642-1664: LGTM: WithExceptStart correctly accesses exit with NULL guard.The instruction uses
expect_uncheckedto safely unwrap the__exit__slot from the stack. This is appropriate since__exit__is guaranteed to be non-NULL by theBeforeWithinstruction. The debug assertion provides additional safety.
1795-1797: LGTM: Reserved opcodes added for replaced instructions.The reserved opcodes (125-127) likely correspond to the removed
CallMethod/LoadMethodinstructions. Marking them as unreachable is correct to prevent accidental execution.
2540-2568: LGTM: Attribute access correctly split into load_attr and load_attr_method.The implementation properly distinguishes between:
load_attr: Regular attribute access (pop obj, push attr)load_attr_method: Method call preparation (pop obj, push [callable, self_or_null])The
load_attr_methodcorrectly usesPyMethod::getto determine if a method descriptor is found:
- Method descriptor: pushes [method, self]
- Regular attribute: pushes [attr, NULL]
This coordinates with the
execute_callmethod which expects [callable, self_or_null, ...].
2161-2183: LGTM: execute_call correctly implements self_or_null protocol.The method properly handles both method calls and regular attribute calls:
- Pops
self_or_null(Option) from TOS- Pops
callablefrom stack- If
self_or_nullisSome: prepends self to args (method call)- If
self_or_nullisNone(NULL): uses args as-is (regular call)The stack order and logic are correct.
2587-2623: LGTM: Push/pop API correctly refactored for NULL-aware stack.The helper methods provide a clear API:
push_value_opt(Option<PyObjectRef>): Pushes an optional valuepush_value(PyObjectRef): Pushes a value (wraps in Some internally)push_null(): Pushes a NULL placeholderpop_value_opt() -> Option<PyObjectRef>: Pops an optional valuepop_value() -> PyObjectRef: Pops a value (asserts non-NULL viaexpect_unchecked)The use of
expect_uncheckedinpop_valueprovides debug assertions while being optimized in release builds.
2750-2771: LGTM: pop_multiple correctly guards against NULL slots.The method drains multiple values from the stack and maps each
OptiontoPyObjectRefusingexpect_unchecked. This correctly panics if any NULL slot is encountered, which is appropriate since most instructions expect non-NULL values. Instructions that need to handle NULL should usepop_value_optdirectly.
2773-2778: LGTM: replace_top correctly handles Option-wrapped slots.The method swaps TOS with a provided
Option<PyObjectRef>, correctly supporting both Some and None values.
2809-2835: LGTM: Debug formatting updated for NULL-aware stack.The Debug implementation correctly formats
Option<PyObjectRef>slots, explicitly displaying NULL slots as "NULL". This is helpful for debugging the new NULL-aware stack semantics.
2850-2856: LGTM: expect_unchecked provides debug safety with release optimization.The helper function uses
expectin debug builds (with assertions) andunwrap_uncheckedin release builds (for performance). This is an appropriate trade-off since NULL slots in expected non-NULL positions indicate compiler bugs. The helper is used consistently throughout the file.
2259-2289: LGTM: execute_unpack_ex correctly wraps elements in Some.The method correctly unpacks sequences with variable-length middle sections, wrapping each element in
Somebefore pushing onto the NULL-aware stack.
2462-2492: LGTM: unpack_sequence correctly wraps elements in Some.The method correctly unpacks fixed-size sequences, wrapping each element in
Somebefore pushing onto the NULL-aware stack.
1920-2005: LGTM: unwind_blocks correctly handles NULL slots during stack cleanup.The exception unwinding logic correctly pops stack slots to the handler depth without checking for NULL. This is appropriate since the values are being discarded during cleanup. The implementation properly handles raising, returning, breaking, and continuing.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/vm/src/frame.rs (1)
2780-2797: Remove UB:nth_value()must not useunreachable_unchecked()on NULL.
If a NULL slot is ever observed here (malformed bytecode, logic bug, or future refactor), this becomes immediate undefined behavior. Preferfatal()(orexpect_unchecked) to keep failure as a controlled panic.Proposed fix
fn nth_value(&self, depth: u32) -> &PyObject { let stack = &self.state.stack; match &stack[stack.len() - depth as usize - 1] { Some(obj) => obj, - None => unsafe { std::hint::unreachable_unchecked() }, + None => self.fatal("tried to get stack item but got NULL"), } }crates/jit/src/instructions.rs (1)
460-485: Fix CALL argument order + removeunwrap()panics + return-type handling.The implementation has three critical bugs:
Reversed argument order: Stack pops in the loop collect args in reverse order (argN, ..., arg1), but Cranelift's
call()expects arguments in left-to-right order (arg1, ..., argN). This corrupts all function call arguments.Unprotected
unwrap():into_value()returnsNoneforNull,Tuple,FuncRef, andNonevariants. Callingunwrap()on these will panic instead of returning an error.Hardcoded return type: Line 479 always pushes
JitValue::Int(returns[0]), ignoring the actual function signature's return type. This should useself.sig.ret(which exists and is used inreturn_value()) combined withJitValue::from_type_and_value()(which is already defined and used elsewhere).Proposed fix
Instruction::Call { nargs } => { let nargs = nargs.get(arg); - let mut args = Vec::new(); + let mut args = Vec::with_capacity(nargs as usize); for _ in 0..nargs { let arg = self.stack.pop().ok_or(JitCompileError::BadBytecode)?; - args.push(arg.into_value().unwrap()); + let val = arg.into_value().ok_or(JitCompileError::NotSupported)?; + args.push(val); } + // Stack is [callable, self_or_null, arg1, ..., argN] with argN on top. + // We popped argN..arg1; reverse to pass arg1..argN to Cranelift. + args.reverse(); // Pop self_or_null (should be Null for JIT-compiled recursive calls) let self_or_null = self.stack.pop().ok_or(JitCompileError::BadBytecode)?; if !matches!(self_or_null, JitValue::Null) { return Err(JitCompileError::NotSupported); } match self.stack.pop().ok_or(JitCompileError::BadBytecode)? { JitValue::FuncRef(reference) => { let call = self.builder.ins().call(reference, &args); let returns = self.builder.inst_results(call); - self.stack.push(JitValue::Int(returns[0])); + let ret = *returns.get(0).ok_or(JitCompileError::NotSupported)?; + let ret_ty = self.sig.ret.clone().ok_or(JitCompileError::NotSupported)?; + self.stack.push(JitValue::from_type_and_value(ret_ty, ret)); Ok(()) } _ => Err(JitCompileError::BadBytecode), } }crates/codegen/src/compile.rs (1)
3702-3762: Critical: genericclasscall path likely breaks starred bases and**kwargs(and usespanic!).In the generic branch, bases are compiled via
for arg in &arguments.args { self.compile_expression(arg)?; }(Line 3716-3723). If any base is starred (*bases), that’s typically represented asExpr::Starred(...)andcompile_expressionwill error outside annotation context—whereas the normal call machinery handles it viagather_elements. Also,keyword.arg == None(**kwargs) currently hard-panics (Line 3741-3742) instead of compiling.Recommendation: avoid the ad-hoc base/keyword emission and reuse the same star/** machinery as calls (either by extending
compile_call_helperfor this classdef+generic_base case, or by building a positional tuple + optional kwargs map and using the newCallWithSelfEx-style opcode).
🤖 Fix all issues with AI agents
In @crates/codegen/src/compile.rs:
- Around line 3731-3756: Replace the panic!("keyword argument name must be set")
in the keyword iteration with returning a structured CodegenError (e.g.,
CodegenError::new(CodegenErrorType::SyntaxError(...)) or similar) so compiler
input errors don't abort the process; locate the block handling
arguments.keywords in compile.rs (the loop over for keyword in
&arguments.keywords) and when keyword.arg is None return the error with an
appropriate span/range for the keyword (use existing range/span available on
`keyword` or `arguments`), keeping the rest of the logic (pushing
ConstantData::Str, self.compile_expression, emit_load_const,
Instruction::CallKw) unchanged.
In @crates/vm/src/frame.rs:
- Around line 2379-2381: The SET_FUNCTION_ATTRIBUTE handling must not accept a
NULL `func`: replace the current use of pop_value_opt() with a non-optional pop
(e.g., call pop_value() instead of pop_value_opt()), or explicitly check the
popped Option and immediately treat a null/None as a bytecode/compiler bug by
failing fast (use expect_unchecked or an assert with a clear message). Ensure
this change is applied to the code that calls pop_value_opt(), replace_top(func)
and expect_unchecked so `func` is guaranteed non-null before continuing.
🧹 Nitpick comments (2)
crates/vm/src/frame.rs (1)
677-684: BuildString: consider avoidingunwrap()if this can be reached by non-compiler bytecode.
If stack contents can be malformed (e.g., unmarshaled),.downcast::<PyStr>().unwrap()will panic instead of raising a VM error.crates/compiler-core/src/bytecode.rs (1)
2136-2250: Disassembly formatting updates are coherent (CALL,CALL_KW,LOAD_ATTR_METHOD, reserved opcodes).
Minor: the reserved formatting path is slightly inconsistent (some go through the padded macro, some write"RESERVED"directly), but functionally fine.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_xml_etree.pyis excluded by!Lib/**crates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_double_async_with.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
crates/codegen/src/compile.rscrates/compiler-core/src/bytecode.rscrates/jit/src/instructions.rscrates/stdlib/src/opcode.rscrates/vm/src/frame.rscrates/vm/src/vm/method.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/stdlib/src/opcode.rs
- crates/vm/src/vm/method.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/jit/src/instructions.rscrates/codegen/src/compile.rscrates/compiler-core/src/bytecode.rscrates/vm/src/frame.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run tests under miri
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (32)
crates/jit/src/instructions.rs (3)
24-33: Nice separation ofNullvsNone(representability stays explicit).
Nullconsistently maps toNotSupportedfor typed lowering (to_jit_type/into_value), which keeps the JIT type model honest.Also applies to: 44-58
129-146:boolean_val()correctly rejectsNull(and other non-scalars).
This avoids accidentally treating the self-or-null sentinel as falsy/truthy in compiled control-flow.
486-489:PushNullinstruction is a clean fit for the self-or-null protocol.crates/vm/src/frame.rs (14)
52-60: Good: stack element type change is minimal and localized (Option<PyObjectRef>).
613-619:BinarySubscript: good switch topush_value()(keeps NULL-aware stack invariants centralized).
706-720: CALL/CALL_KW/CALL_FUNCTION_EX handlers match the new stack contract.
The “callable, self_or_null, …” comment aligns withexecute_call()poppingself_or_nullfirst after arg collection.
765-780: CopyItem:push_value_opt()is the right API now.
1167-1169:LoadAttrMethod+load_attr_method()wiring looks consistent: pushes[callable, self_or_null].
Nice that “attribute call” and “method call” become uniform for CALL*.Also applies to: 2549-2567
1435-1439:PushNullexecution path is clear and self-documenting.
1648-1653: WithExceptStart: good explicit NULL check for__exit__.
1938-1940: Unwind stack trimming: OK withOptionstack slots (NULLs discarded safely).
2160-2183:execute_call()correctly prependsselfonly when present.
2274-2287: UnpackEx: correct wrapping intoSome(...)when pushing back onto the Option stack.
2474-2480: UnpackSequence: correct wrapping intoSome(...)for stack push-back.
2586-2622: Stack helper APIs are coherent (push/pop/replace + “NULL means None slot”).
Given the move toOption, this is a solid centralization of invariants and error messages.Also applies to: 2749-2770, 2772-2777
2811-2824: Debug formatting of NULL stack slots is helpful for tracing.
2849-2855:expect_unchecked()is a reasonable pattern, but please ensure bytecode input is trusted.
If unmarshaled/untrusted bytecode can reach here,unwrap_unchecked()becomes a safety hazard (UB).crates/compiler-core/src/bytecode.rs (3)
795-822: Opcode additions look consistent with the new call protocol; please double-check CPython-number alignment.
This enum’s ordering is correctness-critical for marshal/disassembly compatibility.Also applies to: 1046-1076
1896-1974:stack_effect()updates for PushNull / LoadAttrMethod / Call look correct.*
The Call/CallKw/CallFunctionEx formulas match the documented “callable + self_or_null + …” layouts.
1103-1106: Good:LAST_INSTRUCTIONand size asserts remain intact after enum changes.crates/codegen/src/compile.rs (12)
1032-1049: Correctly updated__exit__(None, None, None)to the new Call stack protocol.The inserted
PushNullensures the call site matches[callable, self_or_null, args...]beforeCall { nargs: 3 }.
1871-1882: Assert-path now matches new call protocol (goodPushNullplacement).
LoadGlobal(AssertionError)+PushNull+ optional message +Call { nargs: 0/1 }looks consistent with the new stack layout.
2091-2104: Decorator push/apply sequence matches new protocol; comment is helpful.Pushing
PushNullper decorator and then applyingCall { nargs: 1 }per decorator should preserve correct order (top-most decorator applied first).
2140-2147: Type-param bound/default closure call updated correctly (PushNull+Call { nargs: 0 }).This aligns the “immediate invoke” path with the new self-or-null slot requirement.
3303-3337: Type-params closure invocation:Reverse/PushNullsequence is subtle—please add/confirm coverage for N=0/1/2 args.The stack juggling looks plausible, but it’s easy to regress; I’d like to see a focused bytecode snapshot test that covers:
- no defaults/kwdefaults (
nargs=0)- defaults only (
nargs=1)- defaults + kwdefaults (
nargs=2)
3771-3784: Non-genericclasscall path: good use ofcompile_call_helper(keeps star/ behavior consistent).**This is the shape I’d expect the generic branch to converge towards as well (modulo
generic_baseinsertion).
3933-3944:withnormal-exit__exit__call updated correctly for self_or_null.
PushNullbeforeCall { nargs: 3 }matches the updated calling convention.
6108-6126:compile_callnow cleanly distinguishes method calls vs regular calls.Using
LoadAttrMethodforobj.attr(...)andPushNullfor plainfunc(...)matches the new “explicit self_or_null” protocol.
6417-6439: Comprehension invocation updated correctly (PushNullbeforeCall { nargs: 1 }).This preserves
[callable, NULL, iter]just before the call.
6847-6861: Break/continue unwind forwith: correctPushNullinsertion before calling__exit__.This matches the same
with-exit calling convention used elsewhere.
1-20: Please ensurecargo fmt+cargo clippyare clean for this opcode migration.As per repo guidelines for
**/*.rs, this change is wide enough that a clippy/fmt pass is important to catch stale opcode variants and stack-protocol mismatches early.
6128-6187: Remove: TheCallFunctionExinstruction is valid and properly defined.The instruction enum in
crates/compiler-core/src/bytecode.rsdefinesCallFunctionEx { has_kwargs: Arg<bool> }as variant 54, with the correct stack layout already documented as[func, self_or_null, callargs, kwargs?]. There is noCallWithSelfExvariant, andCallFunctionExhas not been renamed or removed. The code will compile correctly as-is.
crates/codegen/src/compile.rs
Outdated
| if let Some(arguments) = arguments | ||
| && !arguments.keywords.is_empty() | ||
| { | ||
| let mut kwarg_names = vec![]; | ||
| for keyword in &arguments.keywords { | ||
| if let Some(name) = &keyword.arg { | ||
| self.emit_load_const(ConstantData::Str { | ||
| kwarg_names.push(ConstantData::Str { | ||
| value: name.as_str().into(), | ||
| }); | ||
| } else { | ||
| panic!("keyword argument name must be set"); | ||
| } | ||
| self.compile_expression(&keyword.value)?; | ||
| } | ||
| self.emit_load_const(ConstantData::Tuple { | ||
| elements: kwarg_names, | ||
| }); | ||
| emit!( | ||
| self, | ||
| Instruction::CallFunctionKeyword { | ||
| Instruction::CallKw { | ||
| nargs: nargs | ||
| + u32::try_from(arguments.keywords.len()) | ||
| .expect("too many keyword arguments") | ||
| } | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
Avoid panic! in compiler input paths; return a CodegenError instead.
panic!("keyword argument name must be set") turns valid Python syntax errors (or unsupported constructs) into compiler crashes. Prefer emitting a structured CodegenErrorType::SyntaxError(...) (or a dedicated error) with a proper range.
🤖 Prompt for AI Agents
In @crates/codegen/src/compile.rs around lines 3731 - 3756, Replace the
panic!("keyword argument name must be set") in the keyword iteration with
returning a structured CodegenError (e.g.,
CodegenError::new(CodegenErrorType::SyntaxError(...)) or similar) so compiler
input errors don't abort the process; locate the block handling
arguments.keywords in compile.rs (the loop over for keyword in
&arguments.keywords) and when keyword.arg is None return the error with an
appropriate span/range for the keyword (use existing range/span available on
`keyword` or `arguments`), keeping the rest of the logic (pushing
ConstantData::Str, self.compile_expression, emit_load_const,
Instruction::CallKw) unchanged.
crates/codegen/src/compile.rs
Outdated
| value: name.as_str().into(), | ||
| }); | ||
| } else { | ||
| panic!("keyword argument name must be set"); |
There was a problem hiding this comment.
You can use:
&keyword.arg.expect("...")| self.assertEqual(t.find('.//paragraph').text, | ||
| 'A new cultivar of Begonia plant named \u2018BCT9801BEG\u2019.') | ||
|
|
||
| @unittest.expectedFailure # TODO: RUSTPYTHON |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/codegen/src/compile.rs (1)
3702-3760: Genericclasscall: keyword handling will panic on**kwargs, and likely regresses valid syntax.
keyword.arg.expect(...)makesclass C(**kw): ...(andmetaclass=..., **kw) a hard panic. Also, this path doesn’t appear to have a*_EXfallback for**kwargslikecompile_call_helperdoes.Proposed direction (no full diff): handle `**kwargs` via the same map-building path as `compile_call_helper`
- Detect
arguments.keywords.iter().any(|k| k.arg.is_none()).- If true, build kwargs via
self.compile_keywords(&arguments.keywords)?;(supports**groups) and emit the new EX-call opcode (likelyInstruction::CallWithSelfEx { has_kwargs: true }) after packing positional args into a tuple.- Otherwise keep the
CallKwfast-path.This keeps parity with regular call compilation and avoids panics.
crates/vm/src/frame.rs (1)
52-60: Avoid UB on malformed/hostile bytecode: removeunwrap_unchecked()/unreachable_unchecked()from stack access.
expect_unchecked()usesunwrap_unchecked()in release, andnth_value()usesunreachable_unchecked()onNone. If a crafted code object (e.g., viamarshal) hits these paths, this can become memory-unsafe UB instead of a controlled panic/error.Safer alternatives (panic in all builds)
-fn expect_unchecked(optional: Option<PyObjectRef>, err_msg: &'static str) -> PyObjectRef { - if cfg!(debug_assertions) { - optional.expect(err_msg) - } else { - unsafe { optional.unwrap_unchecked() } - } -} +fn expect_unchecked(optional: Option<PyObjectRef>, err_msg: &'static str) -> PyObjectRef { + optional.expect(err_msg) +}fn nth_value(&self, depth: u32) -> &PyObject { let stack = &self.state.stack; match &stack[stack.len() - depth as usize - 1] { Some(obj) => obj, - None => unsafe { std::hint::unreachable_unchecked() }, + None => self.fatal("tried to get stack value but got NULL"), } }Also applies to: 2586-2622, 2749-2797, 2849-2855
crates/jit/src/instructions.rs (1)
460-489: FixCALLarg order and removeunwrap()panics in JIT call lowering.The stack layout for
CALLis[callable, self_or_null, args...], a LIFO stack where arguments are already in reverse order when popped. The current loop pops arguments without reversing, so the callee receives them in reversed order. Additionally,arg.into_value().unwrap()will panic when the stack value isNull,Tuple, orFuncRefrather than returningNotSupported.Proposed fix
Instruction::Call { nargs } => { let nargs = nargs.get(arg); - let mut args = Vec::new(); - for _ in 0..nargs { - let arg = self.stack.pop().ok_or(JitCompileError::BadBytecode)?; - args.push(arg.into_value().unwrap()); - } + let mut args = Vec::with_capacity(nargs as usize); + for _ in 0..nargs { + let arg = self.stack.pop().ok_or(JitCompileError::BadBytecode)?; + let val = arg.into_value().ok_or(JitCompileError::NotSupported)?; + args.push(val); + } + args.reverse(); // Pop self_or_null (should be Null for JIT-compiled recursive calls) let self_or_null = self.stack.pop().ok_or(JitCompileError::BadBytecode)?; if !matches!(self_or_null, JitValue::Null) { return Err(JitCompileError::NotSupported); }
🤖 Fix all issues with AI agents
In @crates/vm/src/frame.rs:
- Around line 2810-2824: The debug branch that matches Some(elem) if
elem.downcastable::<Self>() currently pushes the literal string "\n > {frame}"
— replace that with a real representation of the frame: downcast the element to
a &Self (e.g., via elem.downcast_ref::<Self>()), then format a meaningful
identifier (pointer, id, or Debug) into the string using format_args! or format!
(for example "\n > <frame {:p}>" or "\n > <frame {:?}>") so the output shows
the frame info; if downcasting fails or you prefer a minimal change, use a clear
marker like "\n > <frame>" instead of the literal "{frame}".
- Around line 677-684: The BuildString instruction currently uses
.downcast::<PyStr>().unwrap() after pop_multiple, which will panic on non-str
results; replace the unwrap with a safe downcast/match for each popped PyObject
from pop_multiple in the bytecode::Instruction::BuildString arm, and if any item
fails to downcast to PyStr return a TypeError via the VM (e.g., raise/return a
VM TypeError) instead of panicking; include a CPython-like error message
identifying the actual returned type (e.g., "__format__() must return str, not
<type>") and keep the successful path pushing vm.ctx.new_str(s).into().
🧹 Nitpick comments (1)
crates/compiler-core/src/bytecode.rs (1)
802-821: Clarify whetherLoadAttrMethodis truly “pseudo” vs a real executed opcode.Line 1072 says “pseudo instruction converted…”, but the opcode is fully represented (enum variant + stack effect + disassembly) and is executed in
crates/vm/src/frame.rs. Please align the comment with actual semantics (or document the conversion point if it’s transformed before execution/marshaling).Also applies to: 1046-1075
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_xml_etree.pyis excluded by!Lib/**crates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_double_async_with.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
crates/codegen/src/compile.rscrates/compiler-core/src/bytecode.rscrates/jit/src/instructions.rscrates/stdlib/src/opcode.rscrates/vm/src/frame.rscrates/vm/src/vm/method.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/stdlib/src/opcode.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/vm/src/vm/method.rscrates/jit/src/instructions.rscrates/codegen/src/compile.rscrates/compiler-core/src/bytecode.rscrates/vm/src/frame.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check the WASM package and demo
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run tests under miri
🔇 Additional comments (18)
crates/codegen/src/compile.rs (12)
1032-1049: With/AsyncWith unwind:PushNullplacement looks consistent with new[callable, self_or_null, args…]contract.
The resulting stack should be[..., __exit__, NULL, None, None, None]beforeCall { nargs: 3 }, which matches the comment.
1871-1882: Assert path: good update to pushNULLbefore callingAssertionError(...).
This keeps the stack layout aligned with the unifiedCallopcode.
2091-2104: Decorator protocol seems correct:prepare_decoratorspushesdecorator, NULLandapply_decoratorsconsumesdecorator, NULL, arg.
The stack discipline should work for chained decorators.
2140-2147: Type-param bound/default thunk:PushNull+Call { nargs: 0 }is consistent.
3772-3784: Non-genericclasscall: good switch toPushNull+compile_call_helper(2, ...)(andCall { nargs: 2 }when no args).
3932-3944:withnormal-exit__exit__(None, None, None)call:PushNulladdition matches the newCallABI.
6416-6439: Comprehension call site:PushNullbeforeCall { nargs: 1 }is consistent with the new ABI.
6844-6861:break/continueunwind throughwith:PushNullbefore calling__exit__matches the updated call ABI.
3304-3337: Type-params closure invocation: verifyReverse/PushNullsequence matchesReversesemantics and call ABI.This is very sensitive to exact
Instruction::Reverse { amount }behavior; a small mismatch will corrupt argument order.To verify this sequence:
- Confirm
Reverseimplementation in the bytecode instruction set—specifically how it reverses the top N stack elements- Verify the Call instruction ABI expectations for argument order:
[callable, self_or_null, arg1, ..., argN]- Trace through the sequence with concrete examples (e.g., N=2 args) to ensure the double-Reverse with PushNull in between produces the correct final order
1-50: Process: please ensurecargo fmtandcargo clippyare run for this opcode-heavy refactor.
As per coding guidelines, this should be done before merging.#!/bin/bash set -euo pipefail cargo fmt --all -- --check cargo clippy --all-targets --all-features -D warnings
6108-6126:compile_call: nice split between method calls (LoadAttrMethod) and regular calls (PushNull).Verified: both code paths correctly produce
[callable, self_or_null, args...]on the stack as expected byCALL.LoadAttrMethodpushes[method, self_or_null](per VM implementation atframe.rs:2549-2567), andPushNullpushesnullfor theself_or_nullslot. CALL instruction expectation and both VM and JIT implementations align on this stack layout.
6128-6187: Incorrect:CallFunctionExis the actual variant name—no rename toCallWithSelfExoccurred.The
CallFunctionExinstruction variant exists and is correctly named in the bytecode. There is no variant namedCallWithSelfExin the codebase. The code at line 6157 is correct.Additionally,
Arguments::len()returns the count of positional arguments, and thenargsvalue passed toCallKw(the count of positional arguments) is correct. The keyword names tuple is emitted separately on the stack before theCallKwinstruction.Likely an incorrect or invalid review comment.
crates/compiler-core/src/bytecode.rs (2)
1896-1974:stack_effect()updates forCall/CallKw/CallFunctionEx/LoadAttrMethod/PushNulllook internally consistent.The net effects match the documented stack layouts, including the
self_or_nullslot andkw_names_tuple/kwargsoptionality.
2136-2232: Disassembly mappings cover the new opcodes and reserved slots appropriately.
CALL,CALL_KW,CALL_FUNCTION_EX,LOAD_ATTR_METHOD, andPUSH_NULLare surfaced; reserved opcodes print asRESERVED.crates/jit/src/instructions.rs (2)
24-59:JitValue::Nullintegration looks consistent, but keep it out of “value-required” paths.
to_jit_type()/into_value()correctly treatNullas non-representable;boolean_val()rejects it.Also applies to: 129-147
486-489:PushNullopcode support is straightforward.crates/vm/src/frame.rs (1)
706-720: Call protocol integration (PushNull+LoadAttrMethod+Call*) is coherent.
LoadAttrMethodpushes[callable, self]or[callable, NULL].PushNullfillsself_or_nullfor non-method calls.execute_call()correctly prependsselfwhen present.Nice simplification vs older
CallMethod*split.Also applies to: 1435-1439, 2161-2183, 2549-2567
crates/vm/src/vm/method.rs (1)
5-10: EnsurePyMethodFlags::CLASSdescriptor path behaves sanely ifdescr_getis absent.Right now classmethod descriptors route to
descr_cls.slots.descr_get.load(). If that’s everNone, the call site will treat it like a plain attribute (sinceis_methodstays false), which may be a silent behavioral shift. Consider asserting/guarding (or falling back tois_method = true) ifCLASSis set butdescr_getis missing.Also applies to: 36-49
⛔ Skipped due to learnings
Learnt from: CR Repo: RustPython/RustPython PR: 0 File: .github/copilot-instructions.md:0-0 Timestamp: 2025-12-27T14:03:49.034Z Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
| bytecode::Instruction::BuildString { size } => { | ||
| let s = self | ||
| let s: Wtf8Buf = self | ||
| .pop_multiple(size.get(arg) as usize) | ||
| .as_slice() | ||
| .iter() | ||
| .map(|pyobj| pyobj.downcast_ref::<PyStr>().unwrap()) | ||
| .collect::<Wtf8Buf>(); | ||
| let str_obj = vm.ctx.new_str(s); | ||
| self.push_value(str_obj.into()); | ||
| .map(|pyobj| pyobj.downcast::<PyStr>().unwrap()) | ||
| .collect(); | ||
| self.push_value(vm.ctx.new_str(s).into()); | ||
| Ok(None) | ||
| } |
There was a problem hiding this comment.
BUILD_STRING now panics on non-str; should raise TypeError instead.
.downcast::<PyStr>().unwrap() will crash the VM if a bad __format__ implementation returns a non-str (CPython raises TypeError).
Proposed fix
bytecode::Instruction::BuildString { size } => {
- let s: Wtf8Buf = self
- .pop_multiple(size.get(arg) as usize)
- .map(|pyobj| pyobj.downcast::<PyStr>().unwrap())
- .collect();
+ let mut parts = Vec::with_capacity(size.get(arg) as usize);
+ for obj in self.pop_multiple(size.get(arg) as usize) {
+ let s = obj.downcast::<PyStr>().map_err(|obj| {
+ vm.new_type_error(format!(
+ "BUILD_STRING expected str, got {:.200}",
+ obj.class().name()
+ ))
+ })?;
+ parts.push(s);
+ }
+ let s: Wtf8Buf = parts.into_iter().collect();
self.push_value(vm.ctx.new_str(s).into());
Ok(None)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| bytecode::Instruction::BuildString { size } => { | |
| let s = self | |
| let s: Wtf8Buf = self | |
| .pop_multiple(size.get(arg) as usize) | |
| .as_slice() | |
| .iter() | |
| .map(|pyobj| pyobj.downcast_ref::<PyStr>().unwrap()) | |
| .collect::<Wtf8Buf>(); | |
| let str_obj = vm.ctx.new_str(s); | |
| self.push_value(str_obj.into()); | |
| .map(|pyobj| pyobj.downcast::<PyStr>().unwrap()) | |
| .collect(); | |
| self.push_value(vm.ctx.new_str(s).into()); | |
| Ok(None) | |
| } | |
| bytecode::Instruction::BuildString { size } => { | |
| let mut parts = Vec::with_capacity(size.get(arg) as usize); | |
| for obj in self.pop_multiple(size.get(arg) as usize) { | |
| let s = obj.downcast::<PyStr>().map_err(|obj| { | |
| vm.new_type_error(format!( | |
| "BUILD_STRING expected str, got {:.200}", | |
| obj.class().name() | |
| )) | |
| })?; | |
| parts.push(s); | |
| } | |
| let s: Wtf8Buf = parts.into_iter().collect(); | |
| self.push_value(vm.ctx.new_str(s).into()); | |
| Ok(None) | |
| } |
🤖 Prompt for AI Agents
In @crates/vm/src/frame.rs around lines 677 - 684, The BuildString instruction
currently uses .downcast::<PyStr>().unwrap() after pop_multiple, which will
panic on non-str results; replace the unwrap with a safe downcast/match for each
popped PyObject from pop_multiple in the bytecode::Instruction::BuildString arm,
and if any item fails to downcast to PyStr return a TypeError via the VM (e.g.,
raise/return a VM TypeError) instead of panicking; include a CPython-like error
message identifying the actual returned type (e.g., "__format__() must return
str, not <type>") and keep the successful path pushing vm.ctx.new_str(s).into().
| let state = self.state.lock(); | ||
| let stack_str = state.stack.iter().fold(String::new(), |mut s, elem| { | ||
| if elem.downcastable::<Self>() { | ||
| s.push_str("\n > {frame}"); | ||
| } else { | ||
| core::fmt::write(&mut s, format_args!("\n > {elem:?}")).unwrap(); | ||
| let stack_str = state.stack.iter().fold(String::new(), |mut s, slot| { | ||
| match slot { | ||
| Some(elem) if elem.downcastable::<Self>() => { | ||
| s.push_str("\n > {frame}"); | ||
| } | ||
| Some(elem) => { | ||
| core::fmt::write(&mut s, format_args!("\n > {elem:?}")).unwrap(); | ||
| } | ||
| None => { | ||
| s.push_str("\n > NULL"); | ||
| } | ||
| } | ||
| s | ||
| }); |
There was a problem hiding this comment.
Debug output prints literal {frame} instead of something meaningful.
This branch currently does s.push_str("\n > {frame}"), which looks unintended. Consider a literal marker (e.g., <frame>) or formatting the frame pointer/id.
Possible tweak
Some(elem) if elem.downcastable::<Self>() => {
- s.push_str("\n > {frame}");
+ s.push_str("\n > <frame>");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let state = self.state.lock(); | |
| let stack_str = state.stack.iter().fold(String::new(), |mut s, elem| { | |
| if elem.downcastable::<Self>() { | |
| s.push_str("\n > {frame}"); | |
| } else { | |
| core::fmt::write(&mut s, format_args!("\n > {elem:?}")).unwrap(); | |
| let stack_str = state.stack.iter().fold(String::new(), |mut s, slot| { | |
| match slot { | |
| Some(elem) if elem.downcastable::<Self>() => { | |
| s.push_str("\n > {frame}"); | |
| } | |
| Some(elem) => { | |
| core::fmt::write(&mut s, format_args!("\n > {elem:?}")).unwrap(); | |
| } | |
| None => { | |
| s.push_str("\n > NULL"); | |
| } | |
| } | |
| s | |
| }); | |
| let state = self.state.lock(); | |
| let stack_str = state.stack.iter().fold(String::new(), |mut s, slot| { | |
| match slot { | |
| Some(elem) if elem.downcastable::<Self>() => { | |
| s.push_str("\n > <frame>"); | |
| } | |
| Some(elem) => { | |
| core::fmt::write(&mut s, format_args!("\n > {elem:?}")).unwrap(); | |
| } | |
| None => { | |
| s.push_str("\n > NULL"); | |
| } | |
| } | |
| s | |
| }); |
🤖 Prompt for AI Agents
In @crates/vm/src/frame.rs around lines 2810 - 2824, The debug branch that
matches Some(elem) if elem.downcastable::<Self>() currently pushes the literal
string "\n > {frame}" — replace that with a real representation of the frame:
downcast the element to a &Self (e.g., via elem.downcast_ref::<Self>()), then
format a meaningful identifier (pointer, id, or Debug) into the string using
format_args! or format! (for example "\n > <frame {:p}>" or "\n > <frame
{:?}>") so the output shows the frame info; if downcasting fails or you prefer a
minimal change, use a clear marker like "\n > <frame>" instead of the literal
"{frame}".
stack is [Option<PyObjectRef>]
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/codegen/src/compile.rs (2)
3277-3331: Critical: type-params closure call builds the wrong stack (arg order + NULL placement).
Reverseflips.defaults/.kwdefaults, andPushNullat TOS will be treated as an argument, notself_or_null.Proposed fix (keep arg order; bubble NULL under args)
- if num_typeparam_args > 0 { - // Stack: [arg1, ..., argN, closure] - // Need: [closure, NULL, arg1, ..., argN] - let reverse_amount = (num_typeparam_args + 1) as u32; - emit!( - self, - Instruction::Reverse { - amount: reverse_amount - } - ); - // Stack: [closure, argN, ..., arg1] - emit!(self, Instruction::PushNull); - // Stack: [closure, argN, ..., arg1, NULL] - emit!( - self, - Instruction::Call { - nargs: num_typeparam_args as u32 - } - ); - } else { + if num_typeparam_args > 0 { + // Stack: [arg1, ..., argN, closure] + // Goal: [closure, NULL, arg1, ..., argN] + let n = num_typeparam_args as u32; + emit!(self, Instruction::Reverse { amount: n + 1 }); // [closure, argN, ..., arg1] + if n > 1 { + emit!(self, Instruction::Reverse { amount: n }); // [closure, arg1, ..., argN] + } + emit!(self, Instruction::PushNull); // [closure, arg1, ..., argN, NULL] + for _ in 0..n { + emit!(self, Instruction::Swap { index: 2 }); // bubble NULL beneath args + } + emit!(self, Instruction::Call { nargs: n }); + } else { // Stack: [closure] emit!(self, Instruction::PushNull); // Stack: [closure, NULL] // Call pops: args (0), then self_or_null (NULL), then callable (closure) emit!(self, Instruction::Call { nargs: 0 }); }
3648-3752: Major: genericclasscreation path likely breaks*basesand**kwargs(and may panic).
The manualarguments.args/arguments.keywordshandling bypassesgather_elements/compile_keywordssemantics andexpect()will panic on**kwargs.One possible direction: always build an args tuple + (optional) kwargs map and use `CallFunctionEx`
// Compile original bases - let base_count = if let Some(arguments) = arguments { - for arg in &arguments.args { - self.compile_expression(arg)?; - } - arguments.args.len() - } else { - 0 - }; - - // Load .generic_base as the last base - emit!(self, Instruction::LoadName(dot_generic_base)); - - let nargs = 2 + u32::try_from(base_count).expect("too many base classes") + 1; // function, name, bases..., generic_base - - // Handle keyword arguments - if let Some(arguments) = arguments - && !arguments.keywords.is_empty() - { - let mut kwarg_names = vec![]; - for keyword in &arguments.keywords { - let name = keyword - .arg - .as_ref() - .expect("keyword argument name must be set"); - kwarg_names.push(ConstantData::Str { - value: name.as_str().into(), - }); - self.compile_expression(&keyword.value)?; - } - self.emit_load_const(ConstantData::Tuple { - elements: kwarg_names, - }); - emit!( - self, - Instruction::CallKw { - nargs: nargs - + u32::try_from(arguments.keywords.len()) - .expect("too many keyword arguments") - } - ); - } else { - emit!(self, Instruction::Call { nargs }); - } + // Build positional args tuple for __build_class__(func, name, *bases, generic_base) + if let Some(arguments) = arguments { + let (size, unpack) = self.gather_elements(2, &arguments.args)?; + if unpack { + emit!(self, Instruction::BuildTupleFromTuples { size }); + } else { + emit!(self, Instruction::BuildTuple { size }); + } + // append generic_base + emit!(self, Instruction::LoadName(dot_generic_base)); + emit!(self, Instruction::BuildTuple { size: 1 }); + emit!(self, Instruction::BuildTupleFromTuples { size: 2 }); + + let has_kwargs = !arguments.keywords.is_empty(); + if has_kwargs { + self.compile_keywords(&arguments.keywords)?; + } + emit!(self, Instruction::CallFunctionEx { has_kwargs }); + } else { + emit!(self, Instruction::BuildTuple { size: 2 }); // (func, name) + emit!(self, Instruction::LoadName(dot_generic_base)); + emit!(self, Instruction::BuildTuple { size: 1 }); + emit!(self, Instruction::BuildTupleFromTuples { size: 2 }); + emit!(self, Instruction::CallFunctionEx { has_kwargs: false }); + }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Lib/test/test_xml_etree.pyis excluded by!Lib/**crates/codegen/src/snapshots/rustpython_codegen__compile__tests__nested_double_async_with.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
crates/codegen/src/compile.rscrates/compiler-core/src/bytecode.rscrates/jit/src/instructions.rscrates/stdlib/src/opcode.rscrates/vm/src/frame.rscrates/vm/src/vm/method.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/jit/src/instructions.rscrates/codegen/src/compile.rscrates/stdlib/src/opcode.rscrates/compiler-core/src/bytecode.rscrates/vm/src/vm/method.rscrates/vm/src/frame.rs
🧠 Learnings (1)
📚 Learning: 2025-11-10T06:27:41.954Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6243
File: vm/src/function/buffer.rs:123-129
Timestamp: 2025-11-10T06:27:41.954Z
Learning: In vm/src/function/buffer.rs, line 36 in the try_rw_bytes_like method intentionally uses TypeError (not BufferError) for the "buffer is not a read-write bytes-like object" error case, even though a similar error message in ArgMemoryBuffer::try_from_borrowed_object uses BufferError. The TypeError is the intended exception type for this specific code path.
Applied to files:
crates/vm/src/frame.rs
🧬 Code graph analysis (1)
crates/compiler-core/src/bytecode.rs (2)
crates/codegen/src/compile.rs (1)
name(1205-1207)crates/codegen/src/ir.rs (1)
idx(47-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check Rust code with clippy
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Run rust tests (windows-2025)
🔇 Additional comments (38)
crates/stdlib/src/opcode.rs (1)
77-77: LGTM: Instruction variant renamed to align with bytecode changes.The rename from
LoadMethodtoLoadAttrMethodcorrectly reflects the broader opcode consolidation across the codebase.crates/vm/src/vm/method.rs (2)
6-7: LGTM: Imports added for classmethod resolution enhancement.The new imports (
PyMethodDescriptorandPyMethodFlags) are correctly added to support the enhanced descriptor handling logic below.
41-49: LGTM: Proper classmethod descriptor resolution added.The logic correctly distinguishes classmethods (which need
descr_getfor instance-to-class conversion) from regular method descriptors (which maintain the existingis_method = truebehavior). The let-chain pattern safely handles the downcast and flag check.crates/jit/src/instructions.rs (5)
30-30: LGTM: Null variant added to support the new call protocol.The
Nullvariant correctly extendsJitValueto represent the NULL marker used in the self_or_null calling convention.
44-50: LGTM: Null correctly treated as non-representable type.The addition of
JitValue::Nullto the pattern that returnsNoneis correct, as NULL markers (like None, Tuple, FuncRef) don't map to primitive JIT types.
53-58: LGTM: Null correctly has no Cranelift value representation.Adding
JitValue::Nullto returnNoneis appropriate, as the NULL marker (like None, Tuple, FuncRef) doesn't have an underlying CraneliftValue.
129-146: LGTM: Null correctly unsupported for boolean conversion.Adding
JitValue::Nullto theNotSupportederror path is appropriate - NULL markers shouldn't be converted to booleans, consistent with Tuple and FuncRef handling.
460-489: LGTM: Call instruction updated for new calling convention with known limitation.The changes correctly implement the new call protocol:
- Renamed
CallFunctionPositionaltoCall- Added logic to pop and verify
self_or_nullisNull- Added
PushNullinstruction to push NULL markersThe JIT currently only supports calls with NULL self (function calls), not method calls with a real self object. This is indicated by returning
NotSupportedfor non-NULL self, which is a reasonable limitation for the initial implementation.crates/compiler-core/src/bytecode.rs (6)
954-956: LGTM: LoadAttrMethod instruction definition.The new
LoadAttrMethodinstruction is well-defined and appropriately placed in the pseudo-opcode range. The structure is consistent with similar attribute-loading instructions.
749-763: LGTM: Unified call instructions.The new
Call,CallKw, andCallFunctionExinstructions properly consolidate the call protocol. The argument types and opcode assignments are correct.
1778-1779: LGTM: LoadAttrMethod stack effect is correct.The stack effect calculation properly accounts for popping the object and pushing both the method and the self_or_null slot, resulting in a net increase of 1.
1813-1818: LGTM: Call stack effects are correctly calculated.The stack effects for
Call,CallKw, andCallFunctionExproperly account for the self_or_null protocol:
Call: pops nargs + self_or_null + callable, pushes result → net effect: -nargs - 1CallKw: additionally pops kw_names_tuple → net effect: -nargs - 2CallFunctionEx: pops kwargs (if any) + args_tuple + self_or_null + callable, pushes result → net effect: -has_kwargs - 2
1913-1913: LGTM: PushNull stack effect is correct.The stack effect of +1 properly reflects pushing a NULL sentinel value for the self_or_null slot in the call protocol.
2042-2042: LGTM: LoadAttrMethod display is consistent.The display format properly shows the instruction name and attribute index, consistent with other attribute instructions.
crates/vm/src/frame.rs (18)
56-56: LGTM: Stack type updated to support NULL slots.The stack type change from
BoxVec<PyObjectRef>toBoxVec<Option<PyObjectRef>>is essential for the NULL-aware call protocol. This allows representing the self_or_null slot asNonewhen no self argument is present.
613-619: LGTM: BinarySubscr correctly handles the new stack semantics.The instruction properly uses
pop_value()to unwrap theOption<PyObjectRef>stack slots, which is correct since subscript operations should never have NULL operands.
677-684: LGTM: BuildString correctly processes stack elements.The instruction properly uses
pop_multiplewhich unwraps theOptionstack slots and panics on NULL (as expected, since BuildString operands should never be NULL). The downcast and string building logic is sound.
706-720: LGTM: Call instructions properly delegate to execute_call.The three call variants correctly collect their arguments and delegate to
execute_call, which handles the unified self_or_null protocol. The stack layout comments accurately document the expected state.
765-780: LGTM: CopyItem preserves NULL slots correctly.The instruction properly uses
push_value_optto copy stack slots, which correctly handles NULL values (represented asNone). This ensures that NULL slots are preserved when copied.
1435-1439: LGTM: PushNull correctly implements NULL pushing.The instruction properly calls
push_null()to push aNonevalue onto the stack, implementing the NULL sentinel for the self_or_null slot in the call protocol.
1642-1664: LGTM: WithExceptStart correctly guards against NULL exit.The use of
expect_uncheckedto unwrap__exit__is appropriate here. The__exit__method should never be NULL in valid bytecode, so this catches compiler bugs in debug builds while maintaining performance in release builds.
1893-1896: LGTM: Exception unwinding safely discards stack slots.The unwinding logic correctly pops stack slots without requiring them to be non-NULL. This is appropriate since exception handlers must handle partially-constructed stack states.
2117-2139: LGTM: execute_call correctly implements the self_or_null protocol.The implementation properly handles both method calls and regular calls:
- When
self_or_nullisSome, it prepends the self argument to the call arguments (method call)- When
self_or_nullisNone(NULL), it uses the arguments as-is (regular attribute/function call)This correctly implements the unified call protocol described in the PR.
2496-2523: LGTM: load_attr_method correctly implements method loading with self_or_null.The implementation properly distinguishes between:
- Method descriptors (
PyMethod::Function): Pushes the unbound method and the self object- Regular attributes (
PyMethod::Attribute): Pushes the attribute value and a NULL sentinelThis correctly implements the self_or_null protocol, where the NULL sentinel indicates no self argument for the subsequent call.
2542-2578: LGTM: Stack manipulation primitives correctly implement NULL-aware operations.The updated stack manipulation methods properly handle the
Option<PyObjectRef>stack:
push_value_opt: Directly pushes anOption<PyObjectRef>push_value: WrapsPyObjectRefinSomebefore pushingpush_null: PushesNoneas the NULL sentinelpop_value_opt: ReturnsOption<PyObjectRef>for nullable operationspop_value: Unwraps withexpect_unchecked, catching NULL in debug buildsThese primitives form a sound foundation for the NULL-aware call protocol.
2705-2726: LGTM: pop_multiple correctly unwraps non-NULL stack slots.The implementation properly returns
PyObjectRefinstead ofOption<PyObjectRef>, usingexpect_uncheckedto unwrap each slot. This is appropriate for operations that expect all operands to be non-NULL, catching compiler bugs in debug builds while maintaining performance in release.
2729-2753: LGTM: Stack access methods correctly handle Option unwrapping.The implementations properly handle the
Option<PyObjectRef>stack:
replace_top: Correctly swapsOption<PyObjectRef>valuestop_value: Pattern matches and panics on NULL (compiler bug)nth_value: Usesunreachable_uncheckedfor NULL case (optimization hint)These methods are used in contexts where NULL is unexpected, so the panic/unreachable behavior is appropriate for catching compiler bugs.
2764-2790: LGTM: Debug implementation visualizes NULL slots.The updated debug output properly handles
Option<PyObjectRef>stack slots, displaying NULL entries as "NULL". This aids in debugging the NULL-aware call protocol.
2805-2811: LGTM: expect_unchecked helper balances safety and performance.The helper function provides a good balance:
- Debug builds: Uses
expect()to catch NULL values with informative messages (compiler bugs)- Release builds: Uses
unwrap_unchecked()for performance after the debug validationThis is an appropriate pattern for "should never happen" cases in compiled bytecode.
2418-2448: LGTM: unpack_sequence correctly wraps elements in Some.The implementation properly wraps each unpacked element in
Some()before pushing onto theOption<PyObjectRef>stack. This ensures all unpacked values are non-NULL as expected.
2215-2245: LGTM: execute_unpack_ex correctly wraps all elements.The implementation properly wraps all unpacked elements in
Some()before pushing onto the stack. Both the after-elements and before-elements are correctly wrapped, ensuring proper stack semantics.
2326-2353: LGTM: execute_set_function_attribute correctly manipulates the stack.The implementation properly:
- Pops the function using
pop_value_opt()(handles NULL-aware stack)- Replaces TOS with the function
- Uses
expect_uncheckedto unwrap the old TOS (attr_value), which should never be NULLThe stack manipulation is correct for the SET_FUNCTION_ATTRIBUTE operation.
crates/codegen/src/compile.rs (6)
1021-1049:__exit__callsites:PushNullplacement looks correct.
You’re pushingNULLimmediately after the__exit__callable and before the 3 args, which matches the new call stack contract.Also applies to: 3922-3943, 6839-6854
1866-1883:assert: correctPushNullinsertion beforeCall.
This alignsAssertionError(...)with the unified call protocol.
2091-2104: Decorators: stack protocol + application order look consistent.
prepare_decorators()leaves[decorator, NULL]pairs soapply_decorators()can repeatedlyCall(1)with the produced function value.
2106-2147: Type-param bound/default closure: correctPushNullbeforeCall(0).
6101-6180:compile_call/compile_call_helper: good consolidation; verifyCallFunctionExnaming + invariant.
- The
LoadAttrMethodvsPushNullsplit is clear and keeps argument emission shared.- Please verify the actual opcode variant name is still
Instruction::CallFunctionEx(vsCallWithSelfEx) in the updatedrustpython_compiler_core::bytecode::Instruction, and that all callers satisfy the “stack already has[callable, self_or_null]” precondition.
6409-6432: Comprehension call: correctPushNullbeforeCall(1).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/compiler-core/src/bytecode.rs (1)
1777-1819: Remove or explicitly handle legacyCallMethodPositional,CallMethodKeyword, andCallMethodExopcodes instack_effect.These enum variants (lines 924–932) exist but are never constructed or emitted anywhere in the codebase. In the
stack_effectmethod, they fall through to the catch-all_ => 0pattern (line 1914), silently reporting zero stack depth change when they should either be explicitly removed or properly specified. If any old bytecode contains these instructions, the stack analysis will be incorrect.Either:
- Remove the enum variants entirely if they're truly obsolete
- Change them to
Reservedvariants- Add explicit
stack_effectcases with correct formulas if they might appear in legacy bytecodecrates/codegen/src/compile.rs (1)
1021-1049: EnsurePushNullplacement matches the newCallstack contract inwithunwind.The new
PushNullbefore calling__exit__looks consistent with the stated[callable, self_or_null, args…]layout, but this path is extremely sensitive to stack order (especially withpreserve_tosswaps). Consider adding a short stack-shape comment right abovePushNullto capture the exact expected top-of-stack layout after the conditional swaps.
🧹 Nitpick comments (4)
crates/compiler-core/src/bytecode.rs (1)
2042-2043: Disassembly names added forLOAD_ATTR_METHODandPUSH_NULL—consider explicit formatting for legacy CallMethod too.*Nice to have these show up clearly in
fmt_dis. IfCallMethodPositional/Keyword/Exare meant to be reserved going forward, it’d be better for debugging to print an explicitRESERVED_*opcode name rather thanRUSTPYTHON_PLACEHOLDERwhen disassembling older artifacts.Also applies to: 2065-2066
crates/codegen/src/compile.rs (3)
2091-2104: Decorator application order looks correct; comment could be more explicit about why the loop is forward.Because decorators are pushed in source order but popped from the stack, iterating
for _ in decorator_liststill applies them in reverse (last decorator runs first). It may be worth stating that explicitly to prevent future “fixes” that reverse the loop and break behavior.
3291-3336: Manual stack shuffling for calling the type-params closure is correct but brittle.The
Swap/PushNullsequences fornum_typeparam_args1 and 2 look consistent with the target layout[closure, NULL, arg1, ...]. However, this is easy to regress and hard to audit.Consider extracting a small helper like
fn reorder_for_null_call(nargs: u32)(or a more general “insert NULL under top N args”) to centralize these swap recipes.
3641-3830: Class creation call emission: good migration toCall/CallKw/CallFunctionEx, but generic path duplicates call-assembly logic.
LoadBuildClass+PushNullis consistent with the new protocol.- The non-generic branch’s
compile_call_helper(2, arguments)is a nice reuse.- The generic branch re-implements a parallel call-assembly flow to append
.generic_baseas a base (including starred/**kwargshandling). That duplication is a maintenance risk (e.g., keyword validation / tuple-building behavior can drift fromcompile_call_helper).If feasible, consider refactoring so the generic branch also uses
compile_call_helper(e.g., by supporting an “extra trailing positional” expression like.generic_base, or by building anArguments-like view that includes it).
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/codegen/src/compile.rscrates/compiler-core/src/bytecode.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/codegen/src/compile.rscrates/compiler-core/src/bytecode.rs
🧬 Code graph analysis (1)
crates/compiler-core/src/bytecode.rs (1)
crates/codegen/src/ir.rs (1)
idx(47-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
🔇 Additional comments (7)
crates/compiler-core/src/bytecode.rs (1)
954-957: LoadAttrMethod opcode 254 is safely and consistently integrated. No conflicts or unsafe assumptions detected. Opcode 254 is exclusively assigned to LoadAttrMethod with proper stack_effect (1) and disassembly formatting (LOAD_ATTR_METHOD). The instruction is correctly emitted by codegen, executed in the frame interpreter, and handled throughout the codebase with no gaps.crates/codegen/src/compile.rs (6)
1866-1882: AssertionError call path now correctly follows thePushNull+Callconvention.
LoadGlobal(AssertionError)+PushNull+Call { nargs: 0/1 }matches the new protocol and is clearer than mixing old call opcodes.
2106-2147: Type-param bound/default closure call:PushNulladdition looks correct.After
make_closure(...)thePushNull+Call { nargs: 0 }sequence aligns with the new call convention.
3864-3990:withnormal-exit__exit__call updated correctly withPushNull.
PushNullbefore pushing the threeNones matches the newCall { nargs: 3 }contract.
6149-6228:compile_call+compile_call_helper: argument counting and opcode selection look consistent with the new call protocol.
- Method call path (
LoadAttrMethod) +compile_call_helper(0, ...)matches the intended[method, self_or_null]setup.- Regular call path now explicitly inserts
PushNull.CallFunctionExis selected for*argsor**kwargs, andCallKwis used for named keywords; both seem consistent with the emitted stack shapes.count = arguments.len() + additional_positionallooks right forCall/CallKwsincearguments.len()includes both positional args and keywords, andadditional_positionalaccounts for pre-pushed positional args (e.g.,__build_class__(func, name, ...)).Minor:
args_count/countcould be computed lazily only in theCall/CallKwbranches, but this is fine.
6457-6474: Comprehension wrapper call updated correctly withPushNull.
make_closure(...)followed byPushNulland thenCall { nargs: 1 }(after iterator setup) matches the new stack convention.
6884-6902: Break/continue unwinding throughwith:PushNullbefore calling__exit__matches the new call convention.This mirrors the normal-exit/return unwinds and keeps the call protocol consistent across control-flow exits.
ShaharNaveh
left a comment
There was a problem hiding this comment.
Please generate opcode metadata again.
We should add a step in the CI to validate that's updated tbh
|
adding one to lint step and autocommit will make sense |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/codegen/src/compile.rs (1)
6169-6228: Remove NameUsage::Store check for keyword argument names; use debug_assert for unreachable branch.The
check_forbidden_namecall withNameUsage::Storeat line 6187 incorrectly restricts keyword argument names. Keyword arguments are not assignment targets—the__debug__restriction applies only to assignments and deletions, not to parameter names. This causes RustPython to reject valid calls likef(__debug__=1)even though CPython allows them.Additionally, the
panic!("name must be set")at line 6065 is unreachable by construction: the branch only executes when!has_double_star, which means all keywords havearg.is_some(). Replace withdebug_assert!(keyword.arg.is_some())for safety in release builds.Proposed change
- for keyword in &arguments.keywords { - if let Some(name) = &keyword.arg { - self.check_forbidden_name(name.as_str(), NameUsage::Store)?; - } - }And:
} else { - // This means **kwargs! - panic!("name must be set"); + debug_assert!(false, "keyword.arg must be Some when has_double_star is false"); + unreachable!() }
🧹 Nitpick comments (3)
crates/codegen/src/compile.rs (3)
2091-2104: Decorator application stack discipline is consistent; consider clarifying the ordering contract.Pushing
PushNullright after each decorator expression and then emittingCall { nargs: 1 }N times correctly applies decorators from the top of stack downward (i.e., last decorator is applied first). A short invariant comment like “stack grows as [d1,NULL,d2,NULL,...,value]” would reduce footguns.
3304-3336: Type-params wrapper call: the stack shuffles look correct but are brittle—please add coverage.The swap sequences for
num_typeparam_args1 and 2 appear to correctly transform:
[arg1, closure] -> [closure, NULL, arg1][arg1, arg2, closure] -> [closure, NULL, arg1, arg2]Given how easy this is to regress, it’d be good to add a disassembly snapshot test covering:
- generic function with positional defaults only
- generic function with kwonly defaults only
- generic function with both (arg count = 2)
3701-3830: PushNull/Call migration correct; nargs math verified against bytecode spec.*
- No legacy call ops remain in compile.rs. ✓
- Generic path:
LoadBuildClass+PushNullis correct.CallFunctionExbranch correctly builds tuple and optional kwargs map.- "Simple case" branch:
CallKw { nargs }is correct—nargs includes all positional args and keyword values on stack, with kw_names_tuple as a separate item pushed immediately beforeCallKw. Per bytecode spec (-1 - (nargs) - 2 + 1), this pops kw_names_tuple + nargs items + self_or_null + callable. ✓- Stack math is correct in both class creation branches.
Consider adding snapshot tests for generic classes with starred bases, **kwargs, and mixed keyword args to cover these edge cases.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/codegen/src/compile.rscrates/compiler-core/src/bytecode.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/compiler-core/src/bytecode.rscrates/codegen/src/compile.rs
🔇 Additional comments (14)
crates/compiler-core/src/bytecode.rs (6)
945-948: LGTM!The
LoadAttrMethodinstruction is properly defined as a pseudo-opcode (254) with the appropriateNameIdxargument type, consistent with related instructions likeLoadAttrandLoadClosure.
1769-1770: LGTM!Stack effect calculation is correct: pops 1 (object) and pushes 2 (method + self_or_null), yielding a net effect of +1.
1804-1809: LGTM!The stack effect calculations correctly implement the CPython 3.13+ call protocol:
Call: popsnargs + 2(args + self_or_null + callable), pushes 1 resultCallKw: adds the kw_names tuple to the pop countCallFunctionEx: correctly handles the optional kwargs dict
1904-1905: LGTM!The stack effect of +1 is correct for
PushNull, which pushes a NULL placeholder for the call protocol.
2033-2033: LGTM!The display formatting for
LoadAttrMethodfollows the established pattern, using thenamemapper consistent withLoadAttr.
2056-2056: LGTM!Simple, no-argument display formatting consistent with other nullary instructions.
crates/codegen/src/compile.rs (8)
1021-1049: With/AsyncWith unwind: PushNull + Call nargs=3 stack shape looks correct.Given the stack comment (“[..., exit]”), inserting
PushNullbefore loadingNones aligns with the unified[callable, self_or_null, ...]protocol.
1866-1882: Assert statement now matches unified Call protocol.
LoadGlobal(AssertionError)+PushNull+Call { nargs: 0/1 }is consistent with “regular call” semantics.
2106-2147: Type-param bound/default closure invocation: PushNull placement is correct.After
make_closure(...)the stack is[closure];PushNull+Call { nargs: 0 }matches the new calling convention.
3864-3991: With statement normal-exit exit call updated correctly.
PushNullbefore the 3Nones +Call { nargs: 3 }matches the new call stack layout.
6149-6167: compile_call: LoadAttrMethod vs PushNull split is coherent with self_or_null semantics.
- Attribute calls:
LoadAttrMethod { idx }thencompile_call_helper(...)leverages[method, self_or_null].- Non-attribute calls: explicit
PushNullensures the slot exists.Would be good to ensure coverage for
obj.attr(...)whereattris not a descriptor method (soself_or_nullshould be NULL).
6458-6474: Comprehension invocation updated correctly with PushNull.
make_closure(...)+PushNull+ iterator +Call { nargs: 1 }aligns with the unified call protocol.
6884-6902: break/continue unwind through with: exit call now follows unified call protocol.The
PushNullinsertion beforeCall { nargs: 3 }matches other exit call sites and should keep the unwind behavior consistent.
6149-6228: Runcargo fmt --allandcargo clippy --all-targets --all-features -- -D warningsbefore merging.This code introduces stack-shape logic changes and contains multiple
panic!()andexpect()calls (lines 6176, 6179, 6214) that may warrant clippy review to ensure error handling aligns with Rust best practices.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/codegen/src/compile.rs (1)
6158-6237: Use explicit field counts instead ofArguments::len()—it may not include keyword arguments, causing VM stack corruption withCallKw.
Instruction::CallKw { nargs }popsnargsvalues from the stack incollect_keyword_args(). Currently,nargsderives fromarguments.len(), which is an external API detail that may count only positional arguments, not keywords. The class compilation code at line ~2080 shows the correct pattern:nargs + arguments.keywords.len(). Apply the same fix here.Proposed fix
fn compile_call_helper( &mut self, additional_positional: u32, arguments: &Arguments, ) -> CompileResult<()> { - let args_count = u32::try_from(arguments.len()).expect("too many arguments"); - let count = args_count - .checked_add(additional_positional) - .expect("too many arguments"); + let positional_count = u32::try_from(arguments.args.len()) + .expect("too many positional arguments") + .checked_add(additional_positional) + .expect("too many arguments"); + let keyword_count = + u32::try_from(arguments.keywords.len()).expect("too many keyword arguments"); + let total_count = positional_count + .checked_add(keyword_count) + .expect("too many arguments"); // Normal arguments: let (size, unpack) = self.gather_elements(additional_positional, &arguments.args)?; let has_double_star = arguments.keywords.iter().any(|k| k.arg.is_none()); @@ } else if !arguments.keywords.is_empty() { @@ - emit!(self, Instruction::CallKw { nargs: count }); + emit!(self, Instruction::CallKw { nargs: total_count }); } else { - emit!(self, Instruction::Call { nargs: count }); + emit!(self, Instruction::Call { nargs: positional_count }); } Ok(()) }
🧹 Nitpick comments (4)
crates/compiler-core/src/bytecode.rs (1)
1804-1809: Stack effects are mathematically correct for the new calling convention.The calculations properly account for:
- Call: -(nargs + 2) + 1 items
- CallKw: -(nargs + 3) + 1 items
- CallFunctionEx: -(3 or 4) + 1 items
📝 Optional: Clarify stack layout order in comments
The comments could be more explicit about the stack order for clarity. For example:
- // Call: pops nargs + self_or_null + callable, pushes result + // Call: pops nargs + self_or_null + callable (top to bottom: argN, ..., arg1, callable, self_or_null), pushes result Call { nargs } => -(nargs.get(arg) as i32) - 2 + 1, - // CallKw: pops kw_names_tuple + nargs + self_or_null + callable, pushes result + // CallKw: pops kw_names_tuple + nargs + self_or_null + callable (top to bottom: kw_names, argN, ..., arg1, callable, self_or_null), pushes result CallKw { nargs } => -1 - (nargs.get(arg) as i32) - 2 + 1, - // CallFunctionEx: pops kwargs(if any) + args_tuple + self_or_null + callable, pushes result + // CallFunctionEx: pops kwargs(if any) + args_tuple + self_or_null + callable (top to bottom: kwargs?, args_tuple, callable, self_or_null), pushes result CallFunctionEx { has_kwargs } => -1 - (has_kwargs.get(arg) as i32) - 2 + 1,crates/codegen/src/compile.rs (3)
2094-2112: Decorator stack discipline looks correct; consider making the “reverse application” intent explicit.
prepare_decorators()pushingdec, NULLpairs andapply_decorators()issuingCall { nargs: 1 }N times correctly applies decorators from the top of stack. For readability, you could iterate.iter().rev()inapply_decorators(even though the loop variable isn’t used) to match the doc comment’s “reverse order” wording.
3311-3346: Reduce footguns: the manual SWAP/NULL insertion for type-param-closure calls is correct but brittle.The reordering logic for
num_typeparam_args in {0,1,2}looks stack-correct, but this is the kind of sequence that’s easy to subtly break later. Consider extracting a small helper likeemit_call_with_null_and_n_args(n)/reorder_call_with_null(n)to centralize the swap patterns.
3711-3839: Class creation paths: NULL handling looks consistent; consider consolidating the “build_class call” assembly.Both generic and non-generic class creation now do
LoadBuildClassthenPushNull, which matches the new protocol. The generic branch’sCallFunctionEx/CallKw/Callsplit for starred bases / kwargs also looks coherent, but the call assembly is duplicated and complex (especially tuple-from-tuples building +.generic_base). If this stabilizes, a focused helper for “build_class + generic_base” would reduce the chance of future stack mistakes.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/_opcode_metadata.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/codegen/src/compile.rscrates/compiler-core/src/bytecode.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/codegen/src/compile.rscrates/compiler-core/src/bytecode.rs
🧬 Code graph analysis (1)
crates/compiler-core/src/bytecode.rs (2)
crates/codegen/src/compile.rs (1)
name(1208-1210)crates/codegen/src/ir.rs (1)
idx(47-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (10)
crates/compiler-core/src/bytecode.rs (4)
1769-1770: Stack effect correctly accounts for the new calling convention.The calculation is accurate: popping the object (-1) and pushing both the method and self_or_null marker (+2) results in a net effect of +1.
1904-1904: PushNull stack effect is correct.The +1 effect correctly represents pushing a NULL marker onto the stack for the new call protocol.
1992-1992: Display formatting is consistent and follows CPython naming conventions.The instruction display implementations correctly use the
w!macro consistently across all four instructions. TheLoadAttrMethodinstruction properly displays the name index parameter using the samename = idxpattern asLoadAttrand other similar instructions. All CPython instruction names (CALL, CALL_KW, LOAD_ATTR_METHOD, PUSH_NULL) match the Python 3.11+ instruction set conventions.The codegen properly emits all four instructions throughout compile.rs, and the VM has corresponding execution handlers in frame.rs that correctly process each instruction's stack protocol.
945-947: LGTM - LoadAttrMethod instruction definition and calling convention are correct.Opcode 254 is appropriately placed in the pseudo-opcode range, the parameter type matches similar attribute instructions, and the stack effects correctly implement the new calling convention:
- LoadAttrMethod pops the object and pushes method + self_or_null (net +1)
- Call, CallKw, and CallFunctionEx properly account for callable + self_or_null + arguments on the stack
- PushNull correctly adds the NULL sentinel for non-method calls
The opcode assignments (Call=53, CallKw=57) align with CPython 3.13's bytecode specification, and the self_or_null mechanism matches CPython's updated calling convention for consistent callable positioning.
crates/codegen/src/compile.rs (6)
1021-1052: ConfirmPushNullplacement matches the new CALL protocol inWith/AsyncWithunwind.The added
Instruction::PushNullbefore calling__exit__aligns with the new[callable, self_or_null, arg1..]protocol, and thepreserve_tosswap keeps the return value below the callable. I’d just ensure the VM’sCall { nargs: 3 }consumes__exit__+ NULL + 3 args and leaves the preserved value untouched (as intended).
1869-1895: Good:AssertionError(...)call now pushes NULL.Adding
Instruction::PushNullbeforeCall { nargs: 0/1 }makes this a “regular call” under the new protocol, consistent with other non-method calls.
2149-2156: Good: type-param bound/default closures are invoked with NULL.
make_closure(...)followed byPushNullthenCall { nargs: 0 }matches the new call ABI.
3981-4000: Good:withnormal-exit__exit__(None,None,None)now pushes NULL.
Instruction::PushNullbeforeCall { nargs: 3 }matches the new calling convention for the__exit__callable sitting on the stack.
6466-6489: Good: comprehension function invocation now pushes NULL.After
make_closure(...), insertingPushNullbefore pushing the iterator arg keeps the call stack shape consistent with other regular calls.
6893-6911: Good:break/continueunwind path calls__exit__using NULL.The inserted
PushNullinUnwindAction::Withmatches the same__exit__calling convention used elsewhere (unwind/normal exit).
|
Code has been automatically formatted The code in this PR has been formatted using git pull origin call |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/compiler-core/src/bytecode.rs (1)
974-1008: Consider using aconstarray ormatchfor O(1) opcode validation.The current implementation uses
custom_ops.contains(&value)which is O(n) on eachTryFromcall. While the list is small (14 elements), this function may be called frequently during bytecode deserialization.♻️ Optional: Use a const lookup table for O(1) validation
+// At module level or inside impl block +const fn is_custom_op(value: u8) -> bool { + matches!(value, 119..=124 | 128..=135) +} // In TryFrom implementation -let custom_ops: &[u8] = &[ - // ... all the explicit opcodes -]; - if (cpython_start..=cpython_end).contains(&value) || value == resume_id - || custom_ops.contains(&value) + || is_custom_op(value) || (pseudo_start..=pseudo_end).contains(&value)
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/compiler-core/src/bytecode.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/compiler-core/src/bytecode.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Check Rust code with clippy
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (6)
crates/compiler-core/src/bytecode.rs (6)
945-948: LGTM!The
LoadAttrMethodinstruction is correctly defined as a pseudo opcode (254) with the appropriateNameIdxargument, aligning with CPython'sLOAD_ATTRwith method semantics.
1804-1805: LGTM!The stack effect of +1 for
LoadAttrMethodis correct: it pops 1 object and pushes 2 values (method + self_or_null), resulting in a net effect of +1.
1839-1844: LGTM!The stack effects for the new call protocol are correctly calculated:
Call: popsnargs + 2(args + self_or_null + callable), pushes 1 resultCallKw: popsnargs + 3(kw_names + args + self_or_null + callable), pushes 1 resultCallFunctionEx: pops3 + has_kwargs(args_tuple + optional kwargs + self_or_null + callable), pushes 1 resultThe comments accurately describe the stack operations.
1939-1939: LGTM!Stack effect of +1 for
PushNullis correct—it pushes a NULL placeholder onto the stack for the call protocol's self_or_null semantics.
2068-2068: LGTM!The display formatting for
LoadAttrMethodcorrectly usesLOAD_ATTR_METHODas the opcode name and properly resolves the name index for display.
2091-2091: LGTM!The display formatting for
PushNullfollows the standard pattern for no-argument instructions.
| }); | ||
| let custom_end = u8::from(Self::Subscript); | ||
| // RustPython-only opcodes (explicit list to avoid gaps like 125-127) | ||
| let custom_ops: &[u8] = &[ |
There was a problem hiding this comment.
Nice, I did exactly this on #6675 lol. I'll rebase after this gets merged
* implementing call the correct way stack is [Option<PyObjectRef>] * fix again * list custom instructions * Auto-format: cargo fmt --all --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Summary by CodeRabbit
Refactor
New Features
✏️ Tip: You can customize this high-level summary in your review settings.