Skip to content

fix: parse errors must not leak ruff AST Debug formatting#390

Open
smigolsmigol wants to merge 3 commits intopydantic:mainfrom
smigolsmigol:fix/parse-error-no-ast-debug-leak
Open

fix: parse errors must not leak ruff AST Debug formatting#390
smigolsmigol wants to merge 3 commits intopydantic:mainfrom
smigolsmigol:fix/parse-error-no-ast-debug-leak

Conversation

@smigolsmigol
Copy link
Copy Markdown

@smigolsmigol smigolsmigol commented Apr 23, 2026

parse_identifier and parse_unpack_target_impl format the offending AST node through Rust Debug when rejecting a target. The resulting error message is the full Debug representation of the ruff_python_ast node - struct name, node_index, range, nested value, ctx, etc. Anything that surfaces MontyException::message() reads that text verbatim.

What the error looks like today

>>> pydantic_monty.Monty("*a = [1, 2]").run()
# MontySyntaxError: Expected name, got Starred(ExprStarred { node_index: NodeIndex(None),
#   range: 0..2, value: Name(ExprName { node_index: NodeIndex(None),
#   range: 1..2, id: Name("a"), ctx: Store }), ctx: Store })

Same shape for *x.y = 1, *x[0] = 1, for x.y in [1]: pass. Each dumps the full nested AST struct instead of a human description.

Fix

Adds describe_expr_kind(&AstExpr) -> &'static str: a match over all 33 AstExpr variants returning a short human description ("starred expression", "attribute", "subscript", "list comprehension", and so on). The two call sites format this string instead of the Debug of the node.

before: "Expected name, got Starred(ExprStarred { node_index: ... })"
after:  "Expected name, got starred expression"

before: "invalid unpacking target: Attribute(ExprAttribute { ... })"
after:  "invalid unpacking target: attribute"

Exhaustive match, no catch-all: if ruff adds a new Expr variant, the compiler forces an update instead of silently falling back to Debug.

Tests

New in crates/monty/tests/parse_errors.rs, one per trigger shape:

  • starred_name_target_has_clean_message - *a = [1, 2]
  • starred_attribute_target_has_clean_message - *x.y = 1
  • starred_subscript_target_has_clean_message - *x[0] = 1
  • for_loop_attribute_target_has_clean_message - for x.y in [1]: pass

Each snapshots the full error message via insta::assert_snapshot! so any regression that reintroduces Debug formatting of AST nodes (struct names, node_index, range, ctx: Store, etc.) fails the snapshot diff loudly.

Not covered

for x.y in [1]: pass is valid Python. CPython 3.11 through 3.14 accept it. Monty rejects it at parse_unpack_target_impl because attribute / subscript targets are not implemented for for-loops. That rejection is a separate issue; this PR only cleans up the error message emitted along the existing path. Separate ticket.

CPython's own messages ("starred assignment target must be in a list or tuple", etc.) are not adopted verbatim here to keep the diff minimal. Switching to CPython prose verbatim is a separate follow-up.

devin-ai-integration[bot]

This comment was marked as resolved.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 23, 2026

Codecov Report

❌ Patch coverage is 18.42105% with 31 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
crates/monty/src/parse.rs 18.42% 31 Missing ⚠️

📢 Thoughts on this report? Let us know!

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 23, 2026

Merging this PR will not alter performance

✅ 16 untouched benchmarks
⏩ 15 skipped benchmarks1


Comparing smigolsmigol:fix/parse-error-no-ast-debug-leak (51d36f3) with main (5c7cf2b)

Open in CodSpeed

Footnotes

  1. 15 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

Copy link
Copy Markdown
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

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

overall LGTM. a few minor points, more generally I would use insta for tests rather than a custom test function.

Comment thread crates/monty/src/parse.rs Outdated
Comment on lines +1253 to +1255
let kind = describe_expr_kind(&other);
Err(ParseError::syntax(
format!("Expected name, got {kind}"),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
let kind = describe_expr_kind(&other);
Err(ParseError::syntax(
format!("Expected name, got {kind}"),
Err(ParseError::syntax(
format!("Expected name, got {}", describe_expr_kind(&other);),

Comment thread crates/monty/src/parse.rs Outdated
Comment on lines +1357 to +1359
let kind = describe_expr_kind(&other);
Err(ParseError::syntax(
format!("invalid unpacking target: {kind}"),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
let kind = describe_expr_kind(&other);
Err(ParseError::syntax(
format!("invalid unpacking target: {kind}"),
Err(ParseError::syntax(
format!("invalid unpacking target: {}", describe_expr_kind(&other)),

Comment thread crates/monty/tests/parse_errors.rs Outdated

/// Asserts an error message is present, contains the expected prose,
/// and does not leak any telltale `ruff_python_ast` Debug tokens.
fn assert_no_debug_leak(msg: Option<&str>, expected_substr: &str) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would remove this and instead use insta to check the full error text.

@smigolsmigol
Copy link
Copy Markdown
Author

smigolsmigol commented Apr 24, 2026

Addressed all three points :

inlined the describe_expr_kind calls at both sites, switched the 4 error-message tests to insta::assert_snapshot! with a top-level use insta::assert_snapshot; import to match the convention from #375, and removed the custom helper.
Also renamed the first test to starred_name_target_* for consistency with the sibling starred_attribute_target_* / starred_subscript_target_* names. 42/42 parse_errors tests pass locally, fmt + clippy clean.

Thanks for the review.

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.

2 participants