Store SyntaxError members in struct fields instead of the instance dict by devyubin · Pull Request #8348 · RustPython/RustPython · GitHub

Part of #8345

This PR follows our AI policy

Summary

SyntaxError's members were stored as None class-attribute placeholders plus per-instance __dict__ entries, so SyntaxError('m').__dict__ was non-empty ({'print_file_and_line': None}, plus filename/lineno/offset/text for the (msg, tuple) form) unlike CPython, and they weren't backed by descriptors. They're now struct fields on PySyntaxError (#[repr(C)] + base field, the same approach as SystemExit in #8282 and StopIteration in #8301), exposed via #[pygetset] — writable, deletable, independent from args, with an empty __dict__ as in CPython. IndentationError, TabError, and _IncompleteInputError inherit 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):

e = SyntaxError("msg", ("file.py", 1, 2, "txt", 2, 3))
e.__init__("new", ("new.py", 4, 5, "t"))
print(e.end_lineno, e.end_offset)
# before: 2 3    after: None None   (matches CPython)

new_exception is also routed through the real constructor for exception types that carry additional payload (now including SyntaxError), instead of building a base-sized PyBaseException. 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_offset for SyntaxError(), the 4-tuple, and the 6-tuple forms; write/delete; re-init reset; empty __dict__; IndentationError/TabError inheritance; and the compile-error path (compile("1 +", ...) renders the caret and sets lineno/msg).

test_exceptions: no regressions — test_syntax_error_memory_leak now passes; the single test_badisinstance failure is pre-existing on main (an unrelated empty-message RecursionError).
test_syntax / test_compile: pass.
cargo fmt / cargo clippy: clean (no new warnings).
Unmarks test_exceptions's test_syntax_error_memory_leak (removes its @expectedFailure) — it now passes.

Notes:

Like StopIteration.value in Store StopIteration.value in a struct field instead of the instance dict #8301, the members are getset_descriptors rather than CPython's member_descriptors; both are data descriptors and behave the same for get/set/delete.
msg is initialized in the constructor (not only __init__) so TabError still 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 direct TabError(msg, tuple) call; the compile path sets them via attribute assignment, so raised TabErrors are unaffected. Fixing that inheritance is out of scope here.

Assisted-by: Claude Code:claude-fable-5

Summary by CodeRabbit

Bug Fixes
- Improved SyntaxError handling for message, filename, line number, offset, source text, and end-position details.
- Added stricter validation for how location information is provided, including correct handling of supported shapes and end-position constraints.
- Improved exception creation compatibility when exception types use different internal payload layouts.

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info ⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: e671f40a-9c4d-4cfd-969b-1e2d65d0058f

📥 Commits

Reviewing files that changed from the base of the PR and between c0fccaa and fc3a36f.

📒 Files selected for processing (2)

crates/vm/src/exceptions.rs
crates/vm/src/vm/vm_new.rs

🚧 Files skipped from review as they are similar to previous changes (2)

crates/vm/src/vm/vm_new.rs
crates/vm/src/exceptions.rs

📝 Walkthrough

Walkthrough

Changes

Exception handling

Layer / File(s)	Summary
SyntaxError fields and initialization crates/vm/src/exceptions.rs	PySyntaxError now stores message and source-location attributes explicitly, exposes them through getsets, initializes positional location data directly, and updates exception registration and message handling.
Exception layout routing crates/vm/src/vm/vm_new.rs	VirtualMachine::new_exception routes exception types with incompatible basicsizes through invoke_exception.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

RustPython/RustPython#7015: Updates VM-side SyntaxError construction for related location attributes.
RustPython/RustPython#7928: Refactors related SyntaxError creation and message handling.
RustPython/RustPython#8327: Refactors invoke_exception usage at the exception-construction call site.

Suggested labels: z-ca-2026

Suggested reviewers: youknowone

🚥 Pre-merge checks | ✅ 5 ✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: moving SyntaxError members from instance dict storage into struct fields.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[x] test: cpython/Lib/test/test_exceptions.py (TODO: 22)
[ ] test: cpython/Lib/test/test_baseexception.py
[x] test: cpython/Lib/test/test_except_star.py (TODO: 1)
[x] test: cpython/Lib/test/test_exception_group.py (TODO: 5)
[x] test: cpython/Lib/test/test_exception_hierarchy.py (TODO: 2)
[x] test: cpython/Lib/test/test_exception_variations.py

dependencies:

dependent tests: (no tests depend on exception)

Legend:

[+] path exists in CPython
[x] up-to-date, [ ] outdated

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

📥 Commits

Reviewing files that changed from the base of the PR and between dd9bce5 and a540419.

⛔ Files ignored due to path filters (1)

Lib/test/test_exceptions.py is excluded by !Lib/**

📒 Files selected for processing (2)

crates/vm/src/exceptions.rs
crates/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

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!

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:

Traverse visits all 8 fields — a missed one here would be a GC cycle leak.
The location coercion via try_to_value is behaviorally equivalent to CPython's PySequence_Tuple (tuple/list fast paths + iterator fallback, TypeError on 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.
msg is now decoupled from args[0], and print_file_and_line no 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_exception returns Err whenever __init__ raises, even with a perfectly valid type — e.g. a 5-element location tuple makes SyntaxError's slot_init raise TypeError, which the expect here 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?

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.

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 msg set in py_new.

unwrap_or_else is wrong folding here.
The result of invoke_exception return error instance of given exc_type. But unwrapped error is not. it is error caused during the process of raising error.

in my opinion, new_exception must not call invoke_exception for a few reason
new_exception is fast exception creation path for types which doesn't require python invoke. the reason why this function can skip returning Err is based on the restriction.
If we allow to invoke exception initializer, the restriction goes broken. we have to keep this as thin and fast path.

Note: it doesn't mean i am justifying the name new_exception and invoke_exception. if anyone can suggest good names that shows this difference, welcome.

Agreed on keeping new_exception thin.

I prototyped a dedicated payload path that skips PyType::call, for the internal builders that know their type at compile time:

pub fn new_payload_exception<T>(&self, cls: PyTypeRef, args: FuncArgs) -> PyResult<PyRef<T>>
where
    T: Constructor<Args = FuncArgs> + Initializer,
{
    let payload = T::py_new(&cls, args.clone(), self)?;
    let exc = payload
        .into_ref_with_type_lazy_dict(self, cls)
        .expect("new_payload_exception: cls is not a matching subtype of T");
    T::slot_init(exc.as_object().to_owned(), args, self)?;
    Ok(exc)
}

I verified it by routing both StopIteration and OSError through it — behaves identically, vm tests pass, and it simplifies OSErrorBuilder. new_exception stays untouched, and invoke_exception still handles the runtime-typed / user-subclass path.

If you're on board with this direction, I'd open it as a separate draft to work on further.

sounds good, let's try it

Thanks for the work, @kangdora. I'll leave this as a draft until #8403 lands. After that, I'll rebase onto new_payload_exception, revert the new_exception change, and route the SyntaxError constructions through it while keeping the struct-field conversion.

devyubin force-pushed the syntax-error-struct-fields branch from a540419 to cca42e7 Compare July 22, 2026 13:18

coderabbitai Bot reviewed Jul 22, 2026

View reviewed changes

Comment thread crates/vm/src/exceptions.rs Outdated

devyubin force-pushed the syntax-error-struct-fields branch from cca42e7 to 172292a Compare July 22, 2026 23:55

kangdora reviewed Jul 24, 2026

View reviewed changes

moreal added the z-ca-2026 Tag to track Contribution Academy 2026 label Jul 24, 2026

youknowone requested changes Jul 26, 2026

View reviewed changes

kangdora mentioned this pull request Jul 27, 2026

Add new_payload_exception helper for constructing built-in payload exceptions #8403

Merged

1 task

devyubin marked this pull request as draft July 27, 2026 21:48

coderabbitai Bot mentioned this pull request Aug 1, 2026

builtins: exception, iterator and function attribute surface parity youknowone/pyre#950

Merged

+                          let base_exception = PyBaseException::new(args.args, vm);
+                          Ok(Self {
+                              base: PyException(base_exception),
+                              msg: msg.into(),

+                      if exc_type.slots.basicsize != core::mem::size_of::<PyBaseException>() {
+                          // If constructing the exception raises (e.g. __init__ rejects the
+                          // args), surface that exception instead of panicking.
+                          return self.invoke_exception(&exc_type, args).unwrap_or_else(|e| e);

Store SyntaxError members in struct fields instead of the instance dict by devyubin · Pull Request #8348 · RustPython/RustPython · GitHub

Uh oh!

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 intoRustPython:mainRustPython/RustPython:mainfrom devyubin:syntax-error-struct-fieldsdevyubin/RustPython:syntax-error-struct-fieldsCopy head branch name to clipboard

Conversation

devyubin commented Jul 22, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Uh oh!

github-actions Bot commented Jul 22, 2026

📦 Library Dependencies

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kangdora commented Jul 22, 2026

Uh oh!

youknowone commented Jul 23, 2026

Uh oh!

kangdora left a comment

Choose a reason for hiding this comment

Uh oh!

kangdora Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

devyubin Jul 26, 2026

Choose a reason for hiding this comment

Uh oh!

kangdora commented Jul 24, 2026

Uh oh!

youknowone Jul 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kangdora Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

youknowone Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

devyubin Jul 27, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Store SyntaxError members in struct fields instead of the instance dict#8348
devyubin wants to merge 3 commits into
RustPython:mainfrom
devyubin:syntax-error-struct-fields

devyubin commented Jul 22, 2026 •
edited by coderabbitai Bot
Loading

coderabbitai Bot commented Jul 22, 2026 •
edited
Loading

youknowone Jul 26, 2026 •
edited
Loading