Compare formatted and unformatted ASTs during formatter tests #8624
Conversation
| body: (&expr.body).into(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
(These aren't necessary, but I made the changes before learning that we need to normalize the AST, figured they're worth having anyway.)
| format_module_source(&content, options.clone()).expect("Formatting to succeed"); | ||
| let formatted = printed.as_code(); | ||
|
|
||
| ensure_stability_when_formatting_twice(formatted, options.clone(), input_path); |
There was a problem hiding this comment.
Not clear to me why we're doing this, we do this exact thing immediately before the if-else.
|
charliermarsh
force-pushed
the
charlie/norm
branch
from
c029549
to
b02918a
Compare
There was a problem hiding this comment.
have to introduce a new Normalized AST specifically for formatting, which is the main downside here (ongoing maintenance whenever we modify the AST).
That's a lot of duplicated code/work/analysis. Could we avoid avoid that by using an AST visitor that applies these change to the cloned AST nodes and then uses the regular comparable AST?
| .header("Unformatted", "Formatted") | ||
| .to_string(); | ||
| panic!( | ||
| r#"Reformatting the unformatted code of {} resulted in AST changes. |
There was a problem hiding this comment.
| r#"Reformatting the unformatted code of {} resulted in AST changes. | |
| r#"Reformatting the unformatted code of {} resulted in AST changes. Please open an issue at https://github.com/astral-sh/ruff/issues/new with your configuration and the failing code snippet |
or something along the lines
There was a problem hiding this comment.
I think it's fine to omit since this is in tests only.
|
I think that would require similar boilerplate, would it not? Since we'd need a visitor that's allowed to mutate the AST nodes (which doesn't currently exist). |
|
That's true, the mutability ruins this |
|
I could do it but we’d need a new visitor, I think. I can at least try. |
charliermarsh
force-pushed
the
charlie/norm
branch
from
b02918a
to
3654986
Compare
| WithItem, | ||
| }; | ||
|
|
||
| /// A trait for transforming ASTs. Visits all nodes in the AST recursively in evaluation-order. |
There was a problem hiding this comment.
Is it important that this modifies the tree in evaluation order or could we use pre-order instead (I was very surprised when I learned that our default visitor visits in evaluation and not pre-order)
Summary
This PR implements validation in the formatter tests to ensure that we don't modify the AST during formatting. Black has similar logic.
In implementing this, I learned that Black actually does modify the AST, and their test infrastructure normalizes the AST to wipe away those differences. Specifically, Black changes the indentation of docstrings, which does modify the AST; and it also inserts parentheses in
delstatements, which changes the AST too.Ruff also does both these things, so we also implement the same normalization using a new visitor that allows for modifying the AST.
Closes #8184.
Test Plan
cargo test