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 2 commits into
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesException handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 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! |
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 fields.