Allow SyntaxError.msg to be writable and reflected in string formatting #6493
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThis PR makes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Co-authored-by: youknowone <69878+youknowone@users.noreply.github.com>
Co-authored-by: youknowone <69878+youknowone@users.noreply.github.com>
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
The change looks good.
Check about the comment for the test code location.
Lib/test/test_exceptions.py
Outdated
| def test_msg_attribute_writable(self): | ||
| err = SyntaxError("bad bad", ("bad.py", 1, 2, "abcdefg")) | ||
| err.msg = "changed" | ||
| self.assertEqual(err.msg, "changed") | ||
| self.assertEqual(str(err), "changed (bad.py, line 1)") | ||
| del err.msg | ||
| self.assertIsNone(err.msg) | ||
|
|
There was a problem hiding this comment.
We must not add our own test under Lib/test/.
Please move it to extra_tests/snippets/builtin_exceptions.py
Also, please add a sentence to copilot instruction not to make this happen again.
There was a problem hiding this comment.
Addressed in 9ba97e5: moved the SyntaxError msg regression check to extra_tests/snippets/builtin_exceptions.py and added guidance in DEVELOPMENT.md to keep RustPython-specific tests out of Lib/test.
Co-authored-by: youknowone <69878+youknowone@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
extra_tests/snippets/builtin_exceptions.py (1)
88-93: Consider testingstr(err)after deletion and args consistency.The test validates core functionality but could be more comprehensive:
- After
del err.msg, verify whatstr(err)returns (currently untested)- Verify that setting
msgalso updatesargs[0]to maintain consistencyEnhanced test with additional assertions
err = SyntaxError("bad bad", ("bad.py", 1, 2, "abcdefg")) err.msg = "changed" assert err.msg == "changed" +assert err.args[0] == "changed", "args[0] should be updated when msg is set" assert str(err) == "changed (bad.py, line 1)" del err.msg assert err.msg is None +# After deletion, str(err) should handle None gracefully +assert "None" in str(err) or "bad.py" in str(err), "str(err) should still format after msg deletion"crates/vm/src/exceptions.rs (1)
2075-2083: Clarify fallback logic or check for None after deletion.The comment states the fallback handles "if the msg attribute was deleted," but
syntax_error_set_msgsetsargs[0]toNoneon deletion rather than removing it. This meansget_attr("msg", vm)succeeds and returnsOk(None object), so theErr(_)branch never executes for deletion.After deletion,
str(err)will format as"None (file.py, line 1)"rather than falling back to base exception formatting.Consider either:
- Update the comment to reflect actual behavior: "Fallback if attribute lookup fails (e.g., attribute overridden incorrectly)"
- Check if msg is None and fall back:
Ok(obj) if !vm.is_none(&obj) => ...Option 2: Check for None and fall back
- let msg = match zelf.as_object().get_attr("msg", vm) { - Ok(obj) => obj - .str(vm) - .unwrap_or_else(|_| vm.ctx.new_str("<msg str() failed>")), - Err(_) => { - // Fallback to the base formatting if the msg attribute was deleted or attribute lookup fails for any reason. - return Py::<PyBaseException>::__str__(zelf, vm); - } + let msg = match zelf.as_object().get_attr("msg", vm) { + Ok(obj) if !vm.is_none(&obj) => obj + .str(vm) + .unwrap_or_else(|_| vm.ctx.new_str("<msg str() failed>")), + _ => { + // Fallback to the base formatting if msg is None/missing + return Py::<PyBaseException>::__str__(zelf, vm); + } };
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
DEVELOPMENT.mdcrates/vm/src/exceptions.rsextra_tests/snippets/builtin_exceptions.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/exceptions.rs
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: In most cases, Python code should not be edited; bug fixes should be made through Rust code modifications only
Follow PEP 8 style for custom Python code
Use ruff for linting Python code
Files:
extra_tests/snippets/builtin_exceptions.py
🧠 Learnings (8)
📚 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 Lib/**/*.py : Add a `# TODO: RUSTPYTHON` comment when modifications are made to files in the `Lib/` directory
Applied to files:
DEVELOPMENT.md
📚 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 Lib/test/**/*.py : When modifying test files in `Lib/test/`, use `unittest.skip("TODO: RustPython <reason>")` or `unittest.expectedFailure` with `# TODO: RUSTPYTHON <reason>` comment to mark unsupported features
Applied to files:
DEVELOPMENT.md
📚 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 Lib/**/*.py : Minimize modifications to CPython standard library files in the `Lib/` directory; modifications should be minimal and only to work around RustPython limitations
Applied to files:
DEVELOPMENT.md
📚 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: Run `cargo test --workspace --exclude rustpython_wasm` to execute Rust unit tests
Applied to files:
DEVELOPMENT.md
📚 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:
DEVELOPMENT.mdcrates/vm/src/exceptions.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 Lib/** : Do not edit the `Lib/` directory directly; the only allowed operation is copying files from CPython
Applied to files:
DEVELOPMENT.md
📚 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: Use `cargo run -- script.py` instead of the standard `python` command to run and test Python code with RustPython
Applied to files:
DEVELOPMENT.md
📚 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/exceptions.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). (9)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Check Rust code with clippy
- GitHub Check: Check the WASM package and demo
🔇 Additional comments (4)
DEVELOPMENT.md (1)
133-134: LGTM! Clear guidance for test placement.The documentation update clarifies where to place RustPython-specific regression tests, which is helpful for contributors.
crates/vm/src/exceptions.rs (3)
12-12: LGTM! Import addition is correct.The
PySetterValueimport is necessary for the newsyntax_error_set_msgsetter function.
997-1002: LGTM! The msg attribute is now writable.The change from
new_readonly_getsettonew_static_getsetwith a setter enables assignment and deletion of themsgattribute, resolving issue #6492.
1049-1066: Implementation correctly maintains msg/args[0] consistency.The setter ensures
args[0]is always updated whenmsgis modified. Note that deletion setsmsgtoNonerather than removing it entirely, which maintains the invariant thatargs[0](the message slot) always exists for SyntaxError.This design choice means
err.msgreturnsNoneafter deletion rather than raisingAttributeError, which aligns with the test expectations.
SyntaxError.msgwas exposed as read-only, causing AttributeError when tools like xonsh attempted to assign to it. The str formatting also ignored a reassigned or missingmsg.SyntaxError.msgthat updates the args tuple consistently for both assignment and deletion.SyntaxError.__str__now uses themsgattribute (with safe fallback) so string output reflects runtime changes tomsg.extra_tests/snippets/builtin_exceptions.pycovering assigning and deletingSyntaxError.msg, ensuringstr(err)reflects updates.Example:
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.