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

Allow diagnostics to generate multi-edit fixes #3709

Merged
merged 3 commits into from
Mar 26, 2023

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Mar 24, 2023

Summary

This PR extends the autofix API to support multiple edits by introducing a basic Fix struct:

pub struct Fix {
    edits: Vec<Edit>,
}

...and propagating those changes throughout the codebase.

To demonstrate its usefulness, the crates/ruff/src/rules/pyupgrade/rules/replace_stdout_stderr.rs rule was also refactored to leverage this new API. (That rule needs to remove two keyword arguments, which may or may not be adjacent, and replace them with capture_output=True. Previously, this rule required that we included all intermediary content in the text edit. Now, we can model it as a replacement and a deletion.)

There are a few changes that need to be made downstream, to tools that depend on our JSON API (namely, the playground and the LSP, the latter of which can be done in a backwards-compatible way).

@charliermarsh charliermarsh marked this pull request as ready for review March 24, 2023 04:16
@charliermarsh
Copy link
Member Author

charliermarsh commented Mar 24, 2023

The test fixture updates have intentionally omitted for now, to keep this PR reviewable. (All test fixtures will change in a mechanical way, as we're moving from a single fix to a vector of fixes.)

@github-actions
Copy link
Contributor

github-actions bot commented Mar 24, 2023

PR Check Results

Ecosystem

✅ ecosystem check detected no changes.

Benchmark

Linux

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.00     15.6±0.30ms     2.6 MB/sec    1.01     15.7±0.30ms     2.6 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.00      4.0±0.10ms     4.1 MB/sec    1.02      4.1±0.11ms     4.0 MB/sec
linter/all-rules/numpy/globals.py          1.00   564.9±14.80µs     5.2 MB/sec    1.00   564.0±17.22µs     5.2 MB/sec
linter/all-rules/pydantic/types.py         1.00      6.8±0.14ms     3.7 MB/sec    1.02      7.0±0.15ms     3.7 MB/sec
linter/default-rules/large/dataset.py      1.00      8.2±0.13ms     4.9 MB/sec    1.00      8.2±0.11ms     4.9 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.01  1882.3±46.47µs     8.8 MB/sec    1.00  1869.1±39.64µs     8.9 MB/sec
linter/default-rules/numpy/globals.py      1.00    209.5±6.30µs    14.1 MB/sec    1.00    209.1±7.25µs    14.1 MB/sec
linter/default-rules/pydantic/types.py     1.00      3.9±0.08ms     6.5 MB/sec    1.00      3.9±0.10ms     6.5 MB/sec

Windows

group                                      main                                   pr
-----                                      ----                                   --
linter/all-rules/large/dataset.py          1.04     15.8±0.21ms     2.6 MB/sec    1.00     15.2±0.18ms     2.7 MB/sec
linter/all-rules/numpy/ctypeslib.py        1.03      4.2±0.07ms     4.0 MB/sec    1.00      4.1±0.05ms     4.1 MB/sec
linter/all-rules/numpy/globals.py          1.01    534.8±8.19µs     5.5 MB/sec    1.00    528.1±6.54µs     5.6 MB/sec
linter/all-rules/pydantic/types.py         1.03      6.9±0.09ms     3.7 MB/sec    1.00      6.7±0.07ms     3.8 MB/sec
linter/default-rules/large/dataset.py      1.06      8.7±0.07ms     4.7 MB/sec    1.00      8.2±0.08ms     5.0 MB/sec
linter/default-rules/numpy/ctypeslib.py    1.04  1889.4±29.80µs     8.8 MB/sec    1.00  1808.3±26.49µs     9.2 MB/sec
linter/default-rules/numpy/globals.py      1.02    208.4±6.66µs    14.2 MB/sec    1.00    203.5±7.27µs    14.5 MB/sec
linter/default-rules/pydantic/types.py     1.05      4.0±0.05ms     6.4 MB/sec    1.00      3.8±0.06ms     6.7 MB/sec

crates/ruff_wasm/Cargo.toml Show resolved Hide resolved
crates/ruff_diagnostics/src/fix.rs Outdated Show resolved Hide resolved
crates/ruff_diagnostics/src/fix.rs Outdated Show resolved Hide resolved
crates/ruff_diagnostics/src/fix.rs Outdated Show resolved Hide resolved
Comment on lines +39 to +58
impl From<Edit> for Fix {
fn from(edit: Edit) -> Self {
Self { edits: vec![edit] }
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: If this is used frequently, then I recommend adding a

pub fn from_edit(edit: Edit) -> Self {
}

function to Fix to make it more explicit. I always find From implementations hard to discover.

crates/ruff_diagnostics/src/fix.rs Show resolved Hide resolved
@charliermarsh charliermarsh force-pushed the charlie/edit branch 3 times, most recently from c6a8a56 to b526d51 Compare March 24, 2023 21:49
Base automatically changed from charlie/edit to main March 24, 2023 23:29
@charliermarsh charliermarsh force-pushed the charlie/multiple-edit branch 4 times, most recently from 3055da9 to 506511e Compare March 26, 2023 18:41
@charliermarsh charliermarsh force-pushed the charlie/multiple-edit branch 5 times, most recently from 726b3cd to f22598a Compare March 26, 2023 20:07
@charliermarsh charliermarsh merged commit e603382 into main Mar 26, 2023
@charliermarsh charliermarsh deleted the charlie/multiple-edit branch March 26, 2023 20:45
@fannheyward
Copy link
Contributor

@charliermarsh sorry to boring you on this, but this PR changes the ruff --format=json format, I think we should mention this in BREAKING_CHANGES.md.

The reason I mention this is, coc-pyright uses ruff's fix info to fix codes, but the format has been changed in this PR, coc-pyright will change to the new output format.

Thank you for your work on ruff!

@charliermarsh
Copy link
Member Author

@fannheyward - You're 100% right, I'm not sure how I missed that -- purely an oversight. I'll add it to BREAKING_CHANGES.md. Sorry about that, and thank you for supporting Ruff!

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.

4 participants