Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a script that tests formatter stability on repositories #5055

Merged
merged 2 commits into from
Jun 19, 2023

Conversation

konstin
Copy link
Member

@konstin konstin commented Jun 13, 2023

Summary

We want to ensure that once formatted content stays the same when formatted again, which is known as formatter stability or formatter idempotency, and that the formatter prints syntactically valid code. As our test cases cover only a limited amount of code, this allows checking entire repositories.

This adds a new subcommand to ruff_dev which can be invoked as cargo run --bin ruff_dev -- check-formatter-stability <repo>. While initially only intended to check stability, it has also found cases where the formatter printed invalid syntax or panicked.

Test Plan

Running this on cpython is already identifying bugs (#5089)

@konstin
Copy link
Member Author

konstin commented Jun 13, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 13, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      6.2±0.01ms     6.5 MB/sec    1.00      6.2±0.02ms     6.5 MB/sec
formatter/numpy/ctypeslib.py               1.01   1322.5±7.76µs    12.6 MB/sec    1.00   1311.8±4.48µs    12.7 MB/sec
formatter/numpy/globals.py                 1.01    127.6±1.55µs    23.1 MB/sec    1.00    126.7±0.12µs    23.3 MB/sec
formatter/pydantic/types.py                1.00      2.6±0.01ms     9.9 MB/sec    1.00      2.6±0.01ms    10.0 MB/sec
linter/all-rules/large/dataset.py          1.00     13.3±0.06ms     3.1 MB/sec    1.00     13.4±0.15ms     3.0 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      3.3±0.00ms     5.1 MB/sec    1.00      3.3±0.01ms     5.1 MB/sec
linter/all-rules/numpy/globals.py          1.00    414.9±0.50µs     7.1 MB/sec    1.00    415.2±0.41µs     7.1 MB/sec
linter/all-rules/pydantic/types.py         1.00      5.7±0.02ms     4.4 MB/sec    1.00      5.7±0.01ms     4.4 MB/sec
linter/default-rules/large/dataset.py      1.00      6.7±0.01ms     6.1 MB/sec    1.00      6.7±0.02ms     6.1 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00   1434.3±2.76µs    11.6 MB/sec    1.01   1455.6±3.19µs    11.4 MB/sec
linter/default-rules/numpy/globals.py      1.00    158.9±0.32µs    18.6 MB/sec    1.00    159.3±0.74µs    18.5 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.0±0.01ms     8.4 MB/sec    1.00      3.0±0.01ms     8.4 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
formatter/large/dataset.py                 1.00      7.9±0.04ms     5.1 MB/sec    1.00      8.0±0.07ms     5.1 MB/sec
formatter/numpy/ctypeslib.py               1.00  1596.9±12.50µs    10.4 MB/sec    1.00  1599.0±10.91µs    10.4 MB/sec
formatter/numpy/globals.py                 1.00    155.9±1.62µs    18.9 MB/sec    1.01    157.7±4.12µs    18.7 MB/sec
formatter/pydantic/types.py                1.00      3.2±0.04ms     7.9 MB/sec    1.01      3.3±0.08ms     7.8 MB/sec
linter/all-rules/large/dataset.py          1.00     15.9±0.19ms     2.6 MB/sec    1.04     16.6±0.19ms     2.5 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.2±0.03ms     4.0 MB/sec    1.04      4.3±0.06ms     3.9 MB/sec
linter/all-rules/numpy/globals.py          1.00    429.2±5.64µs     6.9 MB/sec    1.02    437.7±7.55µs     6.7 MB/sec
linter/all-rules/pydantic/types.py         1.00      7.1±0.09ms     3.6 MB/sec    1.02      7.2±0.09ms     3.5 MB/sec
linter/default-rules/large/dataset.py      1.00      8.4±0.06ms     4.9 MB/sec    1.03      8.6±0.09ms     4.7 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.00  1750.6±11.18µs     9.5 MB/sec    1.03  1806.4±15.23µs     9.2 MB/sec
linter/default-rules/numpy/globals.py      1.00    185.9±1.63µs    15.9 MB/sec    1.02    189.3±4.35µs    15.6 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.8±0.02ms     6.7 MB/sec    1.02      3.9±0.02ms     6.6 MB/sec

@addisoncrump
Copy link
Contributor

Potentially would also be interesting to integrate this with the fuzzer for generating that initial corpus. The corpus I used is from a somewhat outdated python3 dataset currently, so it might not be representative of all modern features.

@konstin konstin marked this pull request as ready for review June 14, 2023 15:59
@konstin konstin added the internal An internal refactor or improvement label Jun 14, 2023
@konstin
Copy link
Member Author

konstin commented Jun 14, 2023

As someone who has little experience with fuzzer, what would it mean to use cpython for the initial corpus?

konstin added a commit that referenced this pull request Jun 15, 2023
## Summary

This fixes a number of problems in the formatter that showed up with
various files in the [cpython](https://github.com/python/cpython)
repository. These problems surfaced as unstable formatting and invalid
code. This is not the entirety of problems discovered through cpython,
but a big enough chunk to separate it. Individual fixes are generally
individual commits. They were discovered with #5055, which i update as i
work through the output

## Test Plan

I added regression tests with links to cpython for each entry, except
for the two stubs that also got comment stubs since they'll be
implemented properly later.
@addisoncrump
Copy link
Contributor

The corpus is the set of inputs on which the fuzzer tries to modify such that it triggers new code paths. If the fuzzer is provided with a corpus containing representative samples of inputs that can be handled by the program being tested, then it is more likely to mutate into an input that triggers unexpected behaviour. This is because the parser is less likely to outright reject the input and the fuzzer more likely to introduce new data to the input that triggers unexpected behaviour while processing the parsed input. Also, it means the fuzzer has less work to do to "discover" new functionality or features.

@konstin
Copy link
Member Author

konstin commented Jun 15, 2023

Thanks for the explanation! In this case cpython plus our own test fixture should be a great start

@addisoncrump
Copy link
Contributor

In that case, once this lands, I'll update the (re)init-fuzzer script to use cpython source as part of the corpus init.

@charliermarsh
Copy link
Member

@konstin - Is this ready for review? Tag me when ready :)

@konstin
Copy link
Member Author

konstin commented Jun 15, 2023

it is :)

match &args.command {
Command::GenerateAll(args) => generate_all::main(args)?,
Command::GenerateJSONSchema(args) => generate_json_schema::main(args)?,
match args.command {
Copy link
Member Author

Choose a reason for hiding this comment

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

i've changed this back and forth like three times 😅 i can also roll this back once again

@konstin konstin force-pushed the formatter-stability-repo-check branch from c4bbe44 to 243253c Compare June 19, 2023 11:21
@konstin konstin requested a review from MichaReiser June 19, 2023 11:21
Progress as of today, can check CPython and finds a bunch of errors (`cargo run --bin ruff_dev projects/cpython`). Needs proper integration, output status and a better PR description.
@konstin konstin force-pushed the formatter-stability-repo-check branch from 243253c to a1f99e1 Compare June 19, 2023 11:22
/// Control the verbosity of the output
#[derive(Copy, Clone, PartialEq, Eq, clap::ValueEnum, Default)]
pub(crate) enum Format {
// Filenames only
Copy link
Member

Choose a reason for hiding this comment

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

These appear in the Clap output, right? Can we make them actual rustdocs?

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks i missed that

@konstin konstin enabled auto-merge (squash) June 19, 2023 14:06
@konstin konstin merged commit b8d378b into main Jun 19, 2023
@konstin konstin deleted the formatter-stability-repo-check branch June 19, 2023 14:13
charliermarsh pushed a commit that referenced this pull request Jun 20, 2023
Following #5055, add cpython as a member of the fuzzer corpus
unconditionally.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal An internal refactor or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants