Store SyntaxError members in struct fields instead of the instance dict#8348
Store SyntaxError members in struct fields instead of the instance dict#8348devyubin wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesException handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] test: cpython/Lib/test/test_exceptions.py (TODO: 22) dependencies: dependent tests: (no tests depend on exception) Legend:
|
a540419 to
cca42e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/vm/src/exceptions.rs`:
- Around line 2784-2792: Update the SyntaxError argument handling around msg and
location_tuple to coerce the second argument through the generic sequence
protocol rather than downcasting specifically to PyTuple; accept sequence inputs
such as lists, and propagate TypeError for non-sequences instead of silently
treating them as absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 068691d7-638d-45e0-860e-5eefd748527c
⛔ Files ignored due to path filters (1)
Lib/test/test_exceptions.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/vm/src/exceptions.rscrates/vm/src/vm/vm_new.rs
|
Thanks for taking on part of #8345 ! 😁 |
Convert PySyntaxError from a #[repr(transparent)] newtype into a #[repr(C)] struct holding msg, filename, lineno, offset, text, end_lineno, end_offset and print_file_and_line as PyAtomicRef fields exposed through pygetset, following the PyStopIteration/PyOSError pattern. Re-initialization now resets end_lineno/end_offset so a 4-tuple __init__ clears previously set values (gh-146250). msg is initialized in the constructor so subclasses whose __init__ slot is not reached (e.g. TabError) still render it. new_exception() now routes exception types with additional payload through invoke_exception() so the compile-error path constructs a fully initialized SyntaxError payload. Unmark test_exceptions' test_syntax_error_memory_leak. Assisted-by: Claude Code:claude-fable-5
cca42e7 to
172292a
Compare
The second argument to SyntaxError(msg, ...) is now coerced from any sequence like CPython's PySequence_Tuple, so a list works and a non-sequence raises TypeError, instead of the exact-tuple downcast that silently dropped both cases. Assisted-by: Claude Code:claude-fable-5
|
@kangdora could you please review this patch? you recently worked on this series. you must be the one who know this the best. Thank you in advance! |
kangdora
left a comment
There was a problem hiding this comment.
Congrats on your first contribution, and thank you for taking this on! 🎉
I went through the whole patch. The SyntaxError conversion itself looks correctly implemented to me. Things I specifically verified:
Traversevisits all 8 fields — a missed one here would be a GC cycle leak.- The location coercion via
try_to_valueis behaviorally equivalent to CPython'sPySequence_Tuple(tuple/list fast paths + iterator fallback,TypeErroron non-iterables), and the comment documents this well. - Re-init resets
end_lineno/end_offset, which is exactly the gh-146250 case — unskipping that test is justified. msgis now decoupled fromargs[0], andprint_file_and_lineno longer pollutes the instance dict — both closer to CPython than before.
Apart from SyntaxError itself, I'd like other opinions on the new_exception change.
Some change there is unavoidable — the existing debug_assert would now fire for SyntaxError. I see two options:
(1) The current global size-based fallback. It conveniently covers the remaining #8345 conversions ahead of time, but at two costs:
- it changes the contract for every caller, and
- it introduces a new panic path:
invoke_exceptionreturnsErrwhenever__init__raises, even with a perfectly valid type — e.g. a 5-element location tuple makesSyntaxError'sslot_initraiseTypeError, which theexpecthere turns into a panic with a misleading message ("invalid exception type"). If this fallback stays, propagating the error (or rewording the message) seems necessary.
(2) A dedicated constructor path for this type — the pattern already used by new_stop_iteration and OSErrorBuilder. This keeps the existing guard and avoids the panic path above.
I don't think this trade-off is mine to decide — @youknowone , could you weigh in?
| let base_exception = PyBaseException::new(args.args, vm); | ||
| Ok(Self { | ||
| base: PyException(base_exception), | ||
| msg: msg.into(), |
There was a problem hiding this comment.
Nit: setting msg here is redundant — slot_init sets it again — and diverges slightly from CPython, where __new__ leaves msg unset (SyntaxError.__new__(SyntaxError, "x").msg is None there, since only __init__ assigns it). msg: None.into() would match.
There was a problem hiding this comment.
You're right about the CPython __new__ behavior, but msg: None.into() brings back a CI failure from earlier in this PR.
Since TabError (a second-level subclass) never reaches PySyntaxError::slot_init, TabError("error", …) renders as TabError: <no detail available> in tracebacks, which breaks test_doctest's test_syntax_error_with_note.
Setting it in py_new is a workaround until slot inheritance works properly for second-level subclasses.
If invoke_exception fails because the exception type's __init__ raises (e.g. a 5-element SyntaxError location tuple), return that exception rather than panicking with a misleading message. Also document why PySyntaxError::py_new sets msg: second-level subclasses such as TabError never reach slot_init. Assisted-by: Claude Code:claude-fable-5
|
LGTM! 👍 Nice catch documenting the |
Part of #8345
Summary
SyntaxError's members were stored asNoneclass-attribute placeholders plus per-instance__dict__entries, soSyntaxError('m').__dict__was non-empty ({'print_file_and_line': None}, plusfilename/lineno/offset/textfor the(msg, tuple)form) unlike CPython, and they weren't backed by descriptors. They're now struct fields onPySyntaxError(#[repr(C)]+basefield, the same approach asSystemExitin #8282 andStopIterationin #8301), exposed via#[pygetset]— writable, deletable, independent fromargs, with an empty__dict__as in CPython.IndentationError,TabError, and_IncompleteInputErrorinherit the fields.Re-initialization now resets
end_lineno/end_offset, so a 4-element location tuple after a 6-element one no longer keeps stale values (matches CPython's gh-146250 fix):new_exceptionis also routed through the real constructor for exception types that carry additional payload (now includingSyntaxError), instead of building a base-sizedPyBaseException. Without this, the compile-error path (new_exception_msg(syntax_error, …), ~15 call sites) would allocate a wrong-sized payload — a debug-assert panic, and out-of-bounds reads in release.Test plan
Verified against CPython 3.14.6:
msg/filename/lineno/offset/text/end_lineno/end_offsetforSyntaxError(), the 4-tuple, and the 6-tuple forms; write/delete; re-init reset; empty__dict__;IndentationError/TabErrorinheritance; and the compile-error path (compile("1 +", ...)renders the caret and setslineno/msg).test_exceptions: no regressions —test_syntax_error_memory_leaknow passes; the singletest_badisinstancefailure is pre-existing onmain(an unrelated empty-messageRecursionError).test_syntax/test_compile: pass.cargo fmt/cargo clippy: clean (no new warnings).test_exceptions'stest_syntax_error_memory_leak(removes its@expectedFailure) — it now passes.Notes:
StopIteration.valuein Store StopIteration.value in a struct field instead of the instance dict #8301, the members aregetset_descriptors rather than CPython'smember_descriptors; both are data descriptors and behave the same for get/set/delete.msgis initialized in the constructor (not only__init__) soTabErrorstill renders its message.TabError's__init__slot is not reached during construction — a pre-existing limitation for two-level exception subclasses — so its location members (lineno/offset/…) are not populated from a directTabError(msg, tuple)call; the compile path sets them via attribute assignment, so raisedTabErrors are unaffected. Fixing that inheritance is out of scope here.Assisted-by: Claude Code:claude-fable-5
Summary by CodeRabbit
SyntaxErrorhandling for message, filename, line number, offset, source text, and end-position details.