Impl PyAttributeError args #5805
Conversation
WalkthroughThe updates unify the creation and handling of Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant VM
participant PyObject
participant Exception
User->>PyObject: Access or delete attribute
PyObject->>VM: Attribute not found
VM->>VM: new_no_attribute_error(obj, attr_name)
VM->>Exception: Create AttributeError with context
Exception-->>User: Raise AttributeError with name and obj
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ Context from checks skipped due to timeout of 90000ms (11)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
vm/src/vm/vm_new.rs (1)
138-150: Consider propagating theset_attribute_error_contextresult and adding a doc-comment
set_attribute_error_contextcurrently returns(), but if at some point it starts returning aPyResult<()>(mirroring CPython’sPyErr_SetObject–family helpers), this silentlet _ = …pattern will hide errors.
Two low-friction improvements:- self.set_attribute_error_context(&attribute_error, obj, name); + // Propagate in case the signature ever changes in the future + self.set_attribute_error_context(&attribute_error, obj, name);…and add a short rustdoc header so downstream contributors discover the helper quickly.
vm/src/builtins/descriptor.rs (1)
289-292: Avoid re-allocating the attribute name on every miss
member.nameis already an interned&'static PyStrInterned.
Converting it into a freshPyStreach time a slot miss occurs allocates and defeats interning.- vm.new_no_attribute_error(obj.clone(), vm.ctx.new_str(member.name.clone())) + vm.new_no_attribute_error(obj.clone(), vm.ctx.intern_str(member.name))This keeps the
PyStrshared and shaves a tiny allocation off a very hot path.vm/src/exceptions.rs (1)
1263-1288:PyAttributeError.__init__only recognises kwargs – consider positional fallbackCPython’s
AttributeErroraccepts positional arguments and still setsname/objwhen raised by the interpreter.
Right now a user doing:raise AttributeError("msg", "attr", SomeObject)will get
name == Noneandobj == None.
Optionally consumeargs.argswhenkwargsare empty to stay CPython-compatible:- zelf.set_attr( - "name", - vm.unwrap_or_none(args.kwargs.get("name").cloned()), - vm, - )?; + let name_obj = if let Some(v) = args.kwargs.get("name") { + v.clone() + } else { + args.args.get(1).cloned().unwrap_or_else(|| vm.ctx.none()) + }; + zelf.set_attr("name", name_obj, vm)?;Same for
obj.vm/src/protocol/object.rs (1)
188-194: Preserve originalKeyErroras the__cause__of the generatedAttributeError
KeyError→AttributeErrortranslation is correct, but the original exception information is lost.
Keeping it as the__cause__makes debugging easier and mirrors CPython’s behaviour for exception chaining.- vm.new_no_attribute_error(self.to_owned(), attr_name.to_owned()) + { + let exc = vm.new_no_attribute_error( + self.to_owned(), + attr_name.to_owned(), + ); + // pseudo-API: adjust to actual helper if available + exc.set_cause(Some(e.clone())); + exc + }If
VirtualMachine::new_no_attribute_erroralready exposes a helper to attach a cause (or if the VM has a genericwith_causehelper), prefer that instead of directly mutating the exception.
Otherwise, consider extending the helper so the call-sites stay concise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Lib/test/test_exceptions.py(0 hunks)vm/src/builtins/descriptor.rs(1 hunks)vm/src/exceptions.rs(1 hunks)vm/src/protocol/object.rs(1 hunks)vm/src/vm/method.rs(1 hunks)vm/src/vm/vm_new.rs(1 hunks)
💤 Files with no reviewable changes (1)
- Lib/test/test_exceptions.py
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Check Rust code with rustfmt and clippy
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
🔇 Additional comments (3)
vm/src/vm/method.rs (1)
80-83: Great simplification – logic now matches the new helperReplacing the bespoke construction with
vm.new_no_attribute_errorremoves duplication and guarantees thename/objfields are populated consistently with the rest of the VM.vm/src/protocol/object.rs (2)
197-199: Consistent use ofnew_no_attribute_errorlooks goodThe fallback to
AttributeErrorwhen the object has no__dict__is now consistent with the rest of the codebase.
202-205:generic_getattrrefactor is clear and idiomaticSwitching to
ok_or_elsewith the new helper keeps the control flow straightforward and avoids duplicated formatting logic. No further action needed.
27b9b5e to
86cdbcc
Compare
Summary by CodeRabbit