Skip to content

Store SyntaxError members in struct fields instead of the instance dict#8348

Open
devyubin wants to merge 2 commits into
RustPython:mainfrom
devyubin:syntax-error-struct-fields
Open

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

Conversation

@devyubin

@devyubin devyubin commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Part of #8345

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 fields.
    • Added stricter validation for supported location argument shapes, including proper end-position constraints.
    • Improved exception instantiation compatibility when exception types have different internal payload layouts.

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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: 694d268c-967c-4208-90b5-2236a88a00f1

📥 Commits

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

📒 Files selected for processing (1)
  • crates/vm/src/exceptions.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • 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, and initializes positional location data directly.
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

Suggested labels: z-ca-2026

🚥 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 clearly summarizes the main change: moving SyntaxError members from instance-dict storage to 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.

@github-actions

Copy link
Copy Markdown
Contributor

📦 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

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Copy Markdown
Contributor

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
@devyubin
devyubin force-pushed the syntax-error-struct-fields branch from cca42e7 to 172292a Compare July 22, 2026 23:55
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
@youknowone

Copy link
Copy Markdown
Member

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants