disallow __new__, __init__#6446
Conversation
Sorry, something went wrong.
WalkthroughThe PR refactors construction/initialization wiring: macros now emit and respect with(Constructor/Initializer) items, and many types move from inline init/pyslot handlers to explicit Constructor/Initializer trait implementations; derive macro gains trait-aware code generation and validation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Sorry, something went wrong.
youknowone
force-pushed
the
disallow-new-init
branch
from
e83e3dd to
928e3c4
Compare
December 16, 2025 05:51
youknowone
force-pushed
the
disallow-new-init
branch
3 times, most recently
from
103206d to
88f591d
Compare
December 21, 2025 15:13
youknowone
force-pushed
the
disallow-new-init
branch
from
88f591d to
0d791f9
Compare
December 21, 2025 15:20
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm/src/exceptions.rs (1)
2026-2116:PyIncompleteInputError::slot_initdiscards SyntaxError location args
PySyntaxError::slot_inituses the second positional argument (a location tuple) to populatefilename,lineno,offset,text,end_lineno, andend_offset, which are then consumed bywrite_syntaxerrorandPySyntaxError.__str__.
PyIncompleteInputErroris a subclass ofPySyntaxError, but its currentslot_initonly sets:zelf.set_attr("name", vm.ctx.new_str("SyntaxError"), vm)?;and ignores the original
FuncArgs, meaning_IncompleteInputErrorinstances won’t get their location fields initialized even if they are constructed with the standard(msg, location_tuple)arguments. That likely breaks formatting and tracebacks for incomplete‑input errors.To preserve SyntaxError semantics while still forcing the name, you can delegate to
PySyntaxError::slot_initafter settingname, e.g.:Proposed fix delegating to `PySyntaxError::slot_init`
- impl Initializer for PyIncompleteInputError { - type Args = FuncArgs; - - fn slot_init(zelf: PyObjectRef, _args: FuncArgs, vm: &VirtualMachine) -> PyResult<()> { - zelf.set_attr("name", vm.ctx.new_str("SyntaxError"), vm)?; - Ok(()) - } + impl Initializer for PyIncompleteInputError { + type Args = FuncArgs; + + fn slot_init(zelf: PyObjectRef, args: FuncArgs, vm: &VirtualMachine) -> PyResult<()> { + // Force the public name while still populating all SyntaxError fields. + zelf.set_attr("name", vm.ctx.new_str("SyntaxError"), vm)?; + PySyntaxError::slot_init(zelf, args, vm) + } @@ - fn init(_zelf: PyRef<Self>, _args: Self::Args, _vm: &VirtualMachine) -> PyResult<()> { - unreachable!("slot_init is defined") - } + fn init(_zelf: PyRef<Self>, _args: Self::Args, _vm: &VirtualMachine) -> PyResult<()> { + unreachable!("slot_init is defined") + } }This keeps
_IncompleteInputErrorfully aligned withSyntaxError’s initialization while still branding it as a separate type.Also applies to: 2127-2141
🧹 Nitpick comments (4)
crates/derive-impl/src/pyclass.rs (1)
736-744: Consider using iterator methods for more idiomatic code.The
with_containshelper manually iterates and compares. This can be simplified using iterator methods.🔎 Suggested refactor
- let with_contains = |with_items: &[Ident], s: &str| { - // Check if Constructor is in the list - for ident in with_items { - if ident.to_string().as_str() == s { - return true; - } - } - false - }; + let with_contains = |with_items: &[Ident], s: &str| { + with_items.iter().any(|ident| ident == s) + };crates/vm/src/stdlib/ast/python.rs (1)
5-10: NodeAst: customslot_new+Initializerlook good; please confirmslot_initisn’t run twiceThe new
Initializerimpl andslot_initbody forNodeAstare clean and mirror the existing_fields-based initializer pattern fromio.rs, which is a nice move away from calling__init__directly.One thing to double‑check:
NodeAstis now declared withwith(Constructor, Initializer)andslot_newmanually callsSelf::slot_init(zelf.clone(), args, vm). If the type-call path for pyclasses withInitializeralso invokes thetp_init/slot_inithook afterslot_new, this could result in initialization happening twice per construction. If the runtime only callsslot_newfor this type (and nottp_init), a short comment here clarifying that assumption would help future readers.Optionally, since this
slot_initis identical to the one used elsewhere for_fields-driven AST-like types, consider a shared helper to avoid the two implementations drifting over time.Also applies to: 16-18, 24-27, 41-45, 52-88
crates/stdlib/src/sqlite.rs (1)
833-885: Connection/Cursor Initializer wiring looks sound; consider CPython parity for directCursorinstantiationThe new
InitializerforConnectioncorrectly handles re‑initialization: it flipsinitializedto false, resets factories, drops any existing db, opens a new one, and only then repopulates the connection state and marks it initialized again. That matches the comment’s intent of leaving a failed reinit in an uninitialized/ProgrammingError state.For
Cursor, the combination of:
py_newreturningnew_uninitialized(connection, vm)(inner = None), andInitializer::initlazily creating a defaultCursorInnerif missing,covers both subclassed cursors (via the type call path) and the internal
Connection.cursor()construction path (which usesCursor::newand never needsInitializer). That looks consistent with howcheck_cursor_stateandBase Cursor.__init__ not called.are used.One behavior worth explicitly confirming is the niche case of
sqlite3.Cursor(conn)from Python:Initializer::Args = FuncArgsandinitignores its arguments, so argument count/type for that call is effectively unchecked androw_factorywon’t be wired from the connection the wayConnection.cursor()does. If CPython parity there matters, you might want to parse(PyRef<Connection>,)ininitand/or error on invalid calls; otherwise, the current, simpler behavior is fine.Also applies to: 902-937, 939-944, 1543-1572, 1574-1601, 1922-1932, 1934-1953
crates/vm/src/exception_group.rs (1)
42-47: PyBaseExceptionGroupslot_newlooks correct; consider rejecting unexpected keyword argsThe new
Constructor::slot_newforPyBaseExceptionGroupdoes a nice job of mirroring CPython’s rules: strict two‑positional‑arg arity, type checks for the message and exceptions sequence, per‑item BaseException validation, and theExceptionGroupvsBaseExceptionGroupvs subclass selection with appropriate nesting restrictions.One behavioral edge case:
args.kwargsis currently ignored, so something likeBaseExceptionGroup("msg", excs, foo=1)would succeed here even though CPython treats unexpected keyword arguments to__new__as aTypeError. If you want to match CPython more closely, you could add an earlyif !args.kwargs.is_empty()check returning aTypeError(and keepslot_initas the documented no‑op).Also applies to: 53-63, 244-370
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
crates/derive-impl/src/pyclass.rs(7 hunks)crates/stdlib/src/sqlite.rs(2 hunks)crates/stdlib/src/ssl/error.rs(1 hunks)crates/vm/src/builtins/object.rs(4 hunks)crates/vm/src/exception_group.rs(2 hunks)crates/vm/src/exceptions.rs(14 hunks)crates/vm/src/stdlib/ast/python.rs(3 hunks)
🧰 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 Rust 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/ssl/error.rscrates/derive-impl/src/pyclass.rscrates/vm/src/exception_group.rscrates/vm/src/stdlib/ast/python.rscrates/stdlib/src/sqlite.rscrates/vm/src/builtins/object.rscrates/vm/src/exceptions.rs
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.py : In most cases, Python code should not be edited; bug fixes should be made through Rust code modifications only
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/derive-impl/src/pyclass.rs
📚 Learning: 2025-12-09T08:46:58.660Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6358
File: crates/vm/src/exception_group.rs:173-185
Timestamp: 2025-12-09T08:46:58.660Z
Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.
Applied to files:
crates/vm/src/exception_group.rscrates/vm/src/exceptions.rs
🧬 Code graph analysis (4)
crates/vm/src/exception_group.rs (1)
crates/vm/src/exceptions.rs (7)
slot_new(710-717)slot_new(1645-1666)args(568-570)args(574-574)class(45-47)new(547-555)new(1061-1063)
crates/vm/src/stdlib/ast/python.rs (2)
crates/vm/src/builtins/object.rs (7)
slot_init(121-123)class(26-28)class(399-401)class(403-405)value(470-470)init(125-127)init(557-559)crates/vm/src/types/slot.rs (1)
slot_init(947-972)
crates/vm/src/builtins/object.rs (2)
crates/vm/src/stdlib/io.rs (2)
slot_init(1465-1468)vm(3524-3526)crates/vm/src/types/slot.rs (1)
slot_init(947-972)
crates/vm/src/exceptions.rs (3)
crates/vm/src/builtins/object.rs (6)
slot_init(121-123)init(125-127)init(557-559)class(26-28)class(399-401)class(403-405)crates/vm/src/exception_group.rs (5)
slot_init(354-365)zelf(212-213)init(367-369)obj(195-195)obj(384-385)crates/vm/src/types/slot.rs (1)
slot_init(947-972)
⏰ 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). (1)
- GitHub Check: Run snippets and cpython tests (windows-latest)
🔇 Additional comments (7)
crates/stdlib/src/ssl/error.rs (1)
10-10: TheInitializerimport is correctly used by the#[pyexception]macro for generating exception trait implementations. Compilation succeeds without clippy warnings, confirming the import is necessary.crates/derive-impl/src/pyclass.rs (4)
66-66: LGTM: is_trait field addition.The new
is_traitfield correctly distinguishes between trait and impl contexts, enabling conditional validation downstream.
236-239: LGTM: Trait context initialization.Correctly sets
is_trait = truefor trait implementations while defaulting other fields.
906-921: LGTM: Validation logic correctly enforces the new restrictions.The validation properly:
- Only applies to non-trait impl blocks (line 908)
- Provides clear, actionable error messages directing users to the correct approach
- Aligns with the PR objective to disallow direct
__new__and__init__method definitions
817-824: LGTM: Correct generation of pyclass attribute with trait information.The generated
#[pyclass]attribute correctly includes allwith_items, ensuring that Constructor/Initializer traits are properly registered when auto-generated.crates/vm/src/builtins/object.rs (1)
5-10: PyBaseObject Initializer +__class__setter refactor LGTMThe no‑op
InitializerforPyBaseObjectis consistent withobject.__init__, and wiring it viawith(Constructor, Initializer)integrates cleanly with the new initialization model. The reworked__class__getter/setter usingPyObjectRefand the mutable/module checks preserves the previous semantics while generalizing the setter shape; no issues spotted here.Also applies to: 118-128, 250-252, 459-462, 464-498
crates/vm/src/exceptions.rs (1)
1374-1387: Initializer implementations for StopIteration/AttributeError/OSError/Unicode look consistent*The new Initializer/slot_init implementations for:
PyStopIteration(valueset from the first positional arg orNone),PyAttributeError(name/objtaken from kwargs, defaulting toNone),PyOSError(field population + args truncation wired throughslot_initand its getters/setters),PyUnicodeDecodeError,PyUnicodeEncodeError, andPyUnicodeTranslateError(typedArgstuples, attribute population, and detailed__str__),all integrate cleanly with the existing
PyBaseExceptionConstructor/Initializer model and match the expected data layout for these exceptions. I don’t see correctness issues in these blocks; the move away from direct__init__calls to trait‑based initialization looks solid here.Also applies to: 1423-1446, 1670-1731, 2178-2225, 2227-2328
Sorry, something went wrong.
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 (1)
crates/derive-impl/src/pyclass.rs (1)
718-739: Replaceexpect()with proper error handling.Line 724 uses
expect()which will panic ifwith()contains non-ident items. In proc-macro code, panics produce poor error messages for users. Useok_or_else()with a propersyn::Errorinstead.🔎 Proposed fix
for meta in nested { - with_items.push(meta.get_ident().expect("with() has non-ident item").clone()); + let ident = meta.get_ident().ok_or_else(|| { + syn::Error::new_spanned(meta, "#[pyclass(with(...))] arguments must be identifiers") + })?; + with_items.push(ident.clone()); }
♻️ Duplicate comments (1)
crates/vm/src/exceptions.rs (1)
1465-1478:PyImportError::__reduce__can panic when no args are present.The call
exc.get_arg(0).unwrap()will panic ifImportError()is constructed without arguments and then__reduce__is called (e.g., via pickle). This should handle the empty args case gracefully.Proposed fix to avoid panic
#[pymethod] fn __reduce__(exc: PyBaseExceptionRef, vm: &VirtualMachine) -> PyTupleRef { let obj = exc.as_object().to_owned(); + let args = exc.args(); + if args.is_empty() { + // Fall back to BaseException behavior when no args + if let Some(dict) = obj.dict().filter(|x| !x.is_empty()) { + return vm.new_tuple((obj.class().to_owned(), vm.ctx.empty_tuple.clone(), dict)); + } else { + return vm.new_tuple((obj.class().to_owned(), vm.ctx.empty_tuple.clone())); + } + } let mut result: Vec<PyObjectRef> = vec![ obj.class().to_owned().into(), - vm.new_tuple((exc.get_arg(0).unwrap(),)).into(), + vm.new_tuple((args[0].clone(),)).into(), ]; if let Some(dict) = obj.dict().filter(|x| !x.is_empty()) { result.push(dict.into()); } result.into_pytuple(vm) }
🧹 Nitpick comments (1)
crates/stdlib/src/sqlite.rs (1)
1937-1937: Optional: Consider documenting why_connectionparameter is unused.The
_connectionparameter is intentionally unused (note the_prefix), since theConnectionis already available viazelf.connection(set by theConstructorinnew_uninitialized). While this is valid, a brief comment explaining this design choice would improve code clarity.🔎 Suggested documentation improvement
- fn init(zelf: PyRef<Self>, _connection: Self::Args, _vm: &VirtualMachine) -> PyResult<()> { + fn init(zelf: PyRef<Self>, _connection: Self::Args, _vm: &VirtualMachine) -> PyResult<()> { + // Note: connection is already stored in zelf.connection via Constructor::py_new let mut guard = zelf.inner.lock();
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/derive-impl/src/pyclass.rscrates/stdlib/src/sqlite.rscrates/vm/src/exception_group.rscrates/vm/src/exceptions.rscrates/vm/src/stdlib/ast/python.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 Rust 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/stdlib/ast/python.rscrates/derive-impl/src/pyclass.rscrates/stdlib/src/sqlite.rscrates/vm/src/exception_group.rscrates/vm/src/exceptions.rs
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.py : In most cases, Python code should not be edited; bug fixes should be made through Rust code modifications only
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.rs : Use the macro system (`pyclass`, `pymodule`, `pyfunction`, etc.) when implementing Python functionality in Rust
Applied to files:
crates/derive-impl/src/pyclass.rs
📚 Learning: 2025-11-29T12:17:28.606Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-29T12:17:28.606Z
Learning: Applies to **/*.py : In most cases, Python code should not be edited; bug fixes should be made through Rust code modifications only
Applied to files:
crates/derive-impl/src/pyclass.rs
📚 Learning: 2025-12-09T08:46:58.660Z
Learnt from: youknowone
Repo: RustPython/RustPython PR: 6358
File: crates/vm/src/exception_group.rs:173-185
Timestamp: 2025-12-09T08:46:58.660Z
Learning: In crates/vm/src/exception_group.rs, the derive() method intentionally always creates a BaseExceptionGroup instance rather than preserving the original exception class type. This is a deliberate design decision that differs from CPython's behavior.
Applied to files:
crates/vm/src/exception_group.rscrates/vm/src/exceptions.rs
🧬 Code graph analysis (3)
crates/derive-impl/src/pyclass.rs (2)
crates/derive-impl/src/util.rs (2)
syn(202-202)with(334-346)crates/derive-impl/src/lib.rs (1)
pyclass(41-47)
crates/stdlib/src/sqlite.rs (4)
crates/vm/src/stdlib/io.rs (2)
flags(3247-3247)zelf(3415-3415)crates/vm/src/stdlib/ast/python.rs (1)
init(86-88)crates/vm/src/builtins/getset.rs (1)
zelf(110-110)crates/vm/src/builtins/tuple.rs (1)
zelf(284-288)
crates/vm/src/exceptions.rs (3)
crates/vm/src/exception_group.rs (5)
slot_init(356-367)zelf(212-213)init(369-371)obj(195-195)obj(386-387)crates/vm/src/builtins/object.rs (6)
slot_init(121-123)init(125-127)init(557-559)class(26-28)class(399-401)class(403-405)crates/vm/src/types/slot.rs (2)
slot_init(947-972)repr(1126-1129)
⏰ 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 (windows-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (22)
crates/vm/src/exception_group.rs (3)
46-46: LGTM! Trait imports and wiring annotation look correct.The addition of
ConstructorandInitializerimports and the#[pyexception(with(Constructor, Initializer))]annotation properly wire the trait-based construction pattern forPyBaseExceptionGroup.Also applies to: 53-53
244-351: Constructor implementation is comprehensive and follows CPython semantics.The
slot_newimplementation correctly:
- Validates exactly 2 positional arguments
- Ensures message is a string
- Rejects sets/frozensets/None as exceptions argument
- Validates non-empty sequence of BaseException instances
- Handles auto-conversion to ExceptionGroup when all exceptions are Exception subclasses
- Properly handles user-defined subclasses
The
py_newreturningunimplemented!("use slot_new")follows the established pattern for types that need access to the fullFuncArgsin construction.
353-372: Initializer implementation is correct as a no-op.The comment clearly explains why
slot_initis a no-op:__new__already sets up the correct args (message, exceptions_tuple), and this design allows subclasses to pass extra arguments without__init__complaining about argument count. This matches CPython's behavior.crates/vm/src/exceptions.rs (9)
1374-1387: LGTM! PyStopIteration Initializer implementation is correct.The
slot_initproperly sets thevalueattribute from the first positional argument (or None if not provided), andinitis correctly marked as unreachable.
1423-1446: LGTM! PyAttributeError Initializer implementation is correct.The
slot_initproperly extractsnameandobjfrom keyword arguments and sets them as attributes, matching CPython behavior.
1481-1505: LGTM! PyImportError Initializer implementation is correct.The
slot_initproperly:
- Extracts and removes
nameandpathfrom kwargs- Validates no unexpected keyword arguments remain
- Sets the attributes in the instance dict
- Delegates to
PyBaseException::slot_initfor base initialization
1669-1729: LGTM! PyOSError Initializer implementation handles complex initialization correctly.The
slot_initproperly:
- Sets errno/strerror when 2-5 args are provided
- Sets filename when 3+ args
- Handles Windows-specific winerror conversion (when 4+ args)
- Sets filename2 when exactly 5 args
- Truncates args to 2 when filename is present (CPython compatibility)
- Delegates to
PyBaseException::slot_initThe unsafe swaps are appropriately commented and occur during initialization when fields are guaranteed to be None.
2076-2116: LGTM! PySyntaxError Initializer implementation is correct.The
slot_initproperly:
- Sets
print_file_and_lineto None- Parses the location tuple (second argument) when present and sets filename, lineno, offset, text, end_lineno, end_offset attributes
- Delegates to
PyBaseException::slot_init
2127-2141: LGTM! PyIncompleteInputError Initializer properly delegates to parent.The
slot_initsets thenameattribute and correctly delegates toPySyntaxError::slot_initfor the rest of the initialization.
2207-2225: LGTM! PyUnicodeDecodeError Initializer implementation is correct.The
slot_initbinds the expected 5 arguments (encoding, object, start, end, reason) and sets them as attributes. The object is correctly converted to bytes before storing.
2262-2279: LGTM! PyUnicodeEncodeError Initializer implementation is correct.The
slot_initbinds the expected 5 arguments (encoding, object, start, end, reason) and sets them as attributes appropriately.
2312-2328: LGTM! PyUnicodeTranslateError Initializer implementation is correct.The
slot_initbinds the expected 4 arguments (object, start, end, reason) and sets them as attributes. Note this type has one fewer argument than the encode/decode variants since it doesn't need an encoding.crates/derive-impl/src/pyclass.rs (6)
66-66: LGTM!The
is_traitfield addition toImplContextis clean and necessary for differentiating trait vs. impl block behavior, which aligns with the PR objectives.
236-239: LGTM!Correctly initializes the context with
is_trait: truewhen processing trait blocks, enabling trait-aware code generation downstream.
748-772: Previous type qualification issue resolved.The missing type qualifications flagged in a previous review (lines 766-767) have been fixed.
VirtualMachineandPyResultare now fully qualified as::rustpython_vm::VirtualMachineand::rustpython_vm::PyResult<Self>.The conditional generation logic is correct: when
Constructoris not inwith(), a default implementation is generated andConstructoris added towith_items.
778-803: Previous type qualification issue resolved.The missing type qualifications flagged in a previous review (lines 797-798) have been fixed.
VirtualMachineandPyResult<()>are now fully qualified as::rustpython_vm::VirtualMachineand::rustpython_vm::PyResult<()>.The conditional generation for
Initializermirrors theConstructorlogic and is correct.
812-819: LGTM!The generated code structure correctly assembles the
with()attribute and outputs the trait implementations. The use of#(#with_items),*properly expands the accumulated items.
901-916: LGTM - Correctly implements PR objectives.This validation correctly disallows
__new__and__init__as#[pymethod]in impl blocks (non-trait contexts), while allowing them in trait definitions. The error messages are clear and guide users to use#[pyclass(with(Constructor))]and#[pyclass(with(Initializer))]instead.This aligns with the PR's goal of preventing direct calls to
__new__and__init__.crates/stdlib/src/sqlite.rs (1)
1543-1543: LGTM: Initializer trait added to Cursor's pyclass declaration.The addition of
Initializerto thewith(...)clause correctly enables trait-based initialization for the Cursor type, consistent with the PR's objective to move away from direct__init__calls.crates/vm/src/stdlib/ast/python.rs (3)
6-6: LGTM! Import additions support the new Initializer pattern.The added imports are necessary for the trait-based refactoring.
Also applies to: 9-9
16-16: LGTM! pyclass attribute updated to enable trait-based pattern.Adding
with(Constructor, Initializer)correctly wires the new trait implementations.
53-89: LGTM! Initializer implementation with proper validation and error handling.The
slot_initlogic correctly:
- Retrieves and validates
_fields- Checks positional argument counts
- Handles keyword arguments with duplicate detection
- Interns string keys for efficiency
The
unreachable!ininitsignals thatslot_initis the designated entry point for this trait implementation.
Sorry, something went wrong.
youknowone
merged commit fdd2ac3
into
RustPython:main
Dec 22, 2025
There are currently some code paths that use
__new__and__init__directly instead of going through the Constructor/Initializer. I plan to change these so that they raise an error. Before doing that, we’ll need to remove any code that actually relies on this behavior.close #6448
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.