mirror of https://github.com/astral-sh/ruff
7 Commits
| Author | SHA1 | Message | Date |
|---|---|---|---|
Brent Westbrook
|
79c949f0f7
|
Don't cache files with diagnostics (#19869)
Summary -- To take advantage of the new diagnostics, we need to update our caching model to include all of the information supported by `ruff_db`'s diagnostic type. Instead of trying to serialize all of this information, Micha suggested simply not caching files with diagnostics, like we already do for files with syntax errors. This PR is an attempt at that approach. This has the added benefit of trimming down our `Rule` derives since this was the last place the `FromStr`/`strum_macros::EnumString` implementation was used, as well as the (de)serialization macros and `CacheKey`. Test Plan -- Existing tests, with their input updated not to include a diagnostic, plus a new test showing that files with lint diagnostics are not cached. Benchmarks -- In addition to tests, we wanted to check that this doesn't degrade performance too much. I posted part of this new analysis in https://github.com/astral-sh/ruff/issues/18198#issuecomment-3175048672, but I'll duplicate it here. In short, there's not much difference between `main` and this branch for projects with few diagnostics (`home-assistant`, `airflow`), as expected. The difference for projects with many diagnostics (`cpython`) is quite a bit bigger (~300 ms vs ~220 ms), but most projects that run ruff regularly are likely to have very few diagnostics, so this may not be a problem practically. I guess GitHub isn't really rendering this as I intended, but the extra separator line is meant to separate the benchmarks on `main` (above the line) from this branch (below the line). | Command | Mean [ms] | Min [ms] | Max [ms] | |:--------------------------------------------------------------|----------:|---------:|---------:| | `ruff check cpython --no-cache --isolated --exit-zero` | 322.0 | 317.5 | 326.2 | | `ruff check cpython --isolated --exit-zero` | 217.3 | 209.8 | 237.9 | | `ruff check home-assistant --no-cache --isolated --exit-zero` | 279.5 | 277.0 | 283.6 | | `ruff check home-assistant --isolated --exit-zero` | 37.2 | 35.7 | 40.6 | | `ruff check airflow --no-cache --isolated --exit-zero` | 133.1 | 130.4 | 146.4 | | `ruff check airflow --isolated --exit-zero` | 34.7 | 32.9 | 41.6 | |:--------------------------------------------------------------|----------:|---------:|---------:| | `ruff check cpython --no-cache --isolated --exit-zero` | 330.1 | 324.5 | 333.6 | | `ruff check cpython --isolated --exit-zero` | 309.2 | 306.1 | 314.7 | | `ruff check home-assistant --no-cache --isolated --exit-zero` | 288.6 | 279.4 | 302.3 | | `ruff check home-assistant --isolated --exit-zero` | 39.8 | 36.9 | 42.4 | | `ruff check airflow --no-cache --isolated --exit-zero` | 134.5 | 131.3 | 140.6 | | `ruff check airflow --isolated --exit-zero` | 39.1 | 37.2 | 44.3 | I had Claude adapt one of the [scripts](https://github.com/sharkdp/hyperfine/blob/master/scripts/plot_whisker.py) from the hyperfine repo to make this plot, so it's not quite perfect, but maybe it's still useful. The table is probably more reliable for close comparisons. I'll put more details about the benchmarks below for the sake of future reproducibility. <img width="4472" height="2368" alt="image" src="https://github.com/user-attachments/assets/1c42d13e-818a-44e7-b34c-247340a936d7" /> <details><summary>Benchmark details</summary> <p> The versions of each project: - CPython: 6322edd260e8cad4b09636e05ddfb794a96a0451, the 3.10 branch from the contributing docs - `home-assistant`: 5585376b406f099fb29a970b160877b57e5efcb0 - `airflow`: 29a1cb0cfde9d99b1774571688ed86cb60123896 The last two are just the main branches at the time I cloned the repos. I don't think our Ruff config should be applied since I used `--isolated`, but these are cloned into my copy of Ruff at `crates/ruff_linter/resources/test`, and I trimmed the `./target/release/` prefix from each of the commands, but these are builds of Ruff in release mode. And here's the script with the `hyperfine` invocation: ```shell #!/bin/bash cargo build --release --bin ruff # git clone --depth 1 https://github.com/home-assistant/core crates/ruff_linter/resources/test/home-assistant # git clone --depth 1 https://github.com/apache/airflow crates/ruff_linter/resources/test/airflow bin=./target/release/ruff resources=./crates/ruff_linter/resources/test cpython=$resources/cpython home_assistant=$resources/home-assistant airflow=$resources/airflow base=${1:-bench} hyperfine --warmup 10 --export-json $base.json --export-markdown $base.md \ "$bin check $cpython --no-cache --isolated --exit-zero" \ "$bin check $cpython --isolated --exit-zero" \ "$bin check $home_assistant --no-cache --isolated --exit-zero" \ "$bin check $home_assistant --isolated --exit-zero" \ "$bin check $airflow --no-cache --isolated --exit-zero" \ "$bin check $airflow --isolated --exit-zero" ``` I ran this once on `main` (`baseline` in the graph, top half of the table) and once on this branch (`nocache` and bottom of the table). </p> </details> |
|
|
Brent Westbrook
|
10a1d9f01e
|
Unify `OldDiagnostic` and `Message` (#18391)
Summary
--
This PR unifies the remaining differences between `OldDiagnostic` and
`Message` (`OldDiagnostic` was only missing an optional `noqa_offset`
field) and
replaces `Message` with `OldDiagnostic`.
The biggest functional difference is that the combined `OldDiagnostic`
kind no
longer implements `AsRule` for an infallible conversion to `Rule`. This
was
pretty easy to work around with `is_some_and` and `is_none_or` in the
few places
it was needed. In `LintContext::report_diagnostic_if_enabled` we can
just use
the new `Violation::rule` method, which takes care of most cases.
Most of the interesting changes are in [this
range](
|
|
Dylan
|
74f64d3f96
|
Server: Allow `FixAll` action in presence of version-specific syntax errors (#16848)
The single flag `has_syntax_error` on `LinterResult` is replaced with two (private) flags: `has_valid_syntax` and `has_no_unsupported_syntax_errors`, which record whether there are `ParseError`s or `UnsupportedSyntaxError`s, respectively. Only the former is used to prevent a `FixAll` action. An attempt has been made to make consistent the usage of the phrases "valid syntax" (which seems to be used to refer only to _parser_ errors) and "syntax error" (which refers to both _parser_ errors and version-specific syntax errors). Closes #16841 |
|
Dhruv Manilawala
|
72b6c26101 |
Simplify `LinterResult`, avoid cloning `ParseError` (#11903)
## Summary Follow-up to #11902 This PR simplifies the `LinterResult` struct by avoiding the generic and not store the `ParseError`. This is possible because the callers already have access to the `ParseError` via the `Parsed` output. This also means that we can simplify the return type of `check_path` and avoid the generic `T` on `LinterResult`. ## Test Plan `cargo insta test` |
|
|
Dhruv Manilawala
|
e7b49694a7 |
Remove `E999` as a rule, disallow any disablement methods for syntax error (#11901)
## Summary This PR updates the way syntax errors are handled throughout the linter. The main change is that it's now not considered as a rule which involves the following changes: * Update `Message` to be an enum with two variants - one for diagnostic message and the other for syntax error message * Provide methods on the new message enum to query information required by downstream usages This means that the syntax errors cannot be hidden / disabled via any disablement methods. These are: 1. Configuration via `select`, `ignore`, `per-file-ignores`, and their `extend-*` variants ```console $ cargo run -- check ~/playground/ruff/src/lsp.py --extend-select=E999 --no-preview --no-cache Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.10s Running `target/debug/ruff check /Users/dhruv/playground/ruff/src/lsp.py --extend-select=E999 --no-preview --no-cache` warning: Rule `E999` is deprecated and will be removed in a future release. Syntax errors will always be shown regardless of whether this rule is selected or not. /Users/dhruv/playground/ruff/src/lsp.py:1:8: F401 [*] `abc` imported but unused | 1 | import abc | ^^^ F401 2 | from pathlib import Path 3 | import os | = help: Remove unused import: `abc` ``` 3. Command-line flags via `--select`, `--ignore`, `--per-file-ignores`, and their `--extend-*` variants ```console $ cargo run -- check ~/playground/ruff/src/lsp.py --no-cache --config=~/playground/ruff/pyproject.toml Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.11s Running `target/debug/ruff check /Users/dhruv/playground/ruff/src/lsp.py --no-cache --config=/Users/dhruv/playground/ruff/pyproject.toml` warning: Rule `E999` is deprecated and will be removed in a future release. Syntax errors will always be shown regardless of whether this rule is selected or not. /Users/dhruv/playground/ruff/src/lsp.py:1:8: F401 [*] `abc` imported but unused | 1 | import abc | ^^^ F401 2 | from pathlib import Path 3 | import os | = help: Remove unused import: `abc` ``` This also means that the **output format** needs to be updated: 1. The `code`, `noqa_row`, `url` fields in the JSON output is optional (`null` for syntax errors) 2. Other formats are changed accordingly For each format, a new test case specific to syntax errors have been added. Please refer to the snapshot output for the exact format for syntax error message. The output of the `--statistics` flag will have a blank entry for syntax errors: ``` 315 F821 [ ] undefined-name 119 [ ] syntax-error 103 F811 [ ] redefined-while-unused ``` The **language server** is updated to consider the syntax errors by convert them into LSP diagnostic format separately. ### Preview There are no quick fixes provided to disable syntax errors. This will automatically work for `ruff-lsp` because the `noqa_row` field will be `null` in that case. <img width="772" alt="Screenshot 2024-06-26 at 14 57 08" src="https://github.com/astral-sh/ruff/assets/67177269/aaac827e-4777-4ac8-8c68-eaf9f2c36774"> Even with `noqa` comment, the syntax error is displayed: <img width="763" alt="Screenshot 2024-06-26 at 14 59 51" src="https://github.com/astral-sh/ruff/assets/67177269/ba1afb68-7eaf-4b44-91af-6d93246475e2"> Rule documentation page: <img width="1371" alt="Screenshot 2024-06-26 at 16 48 07" src="https://github.com/astral-sh/ruff/assets/67177269/524f01df-d91f-4ac0-86cc-40e76b318b24"> ## Test Plan - [x] Disablement methods via config shows a warning - [x] `select`, `extend-select` - [ ] ~`ignore`~ _doesn't show any message_ - [ ] ~`per-file-ignores`, `extend-per-file-ignores`~ _doesn't show any message_ - [x] Disablement methods via command-line flag shows a warning - [x] `--select`, `--extend-select` - [ ] ~`--ignore`~ _doesn't show any message_ - [ ] ~`--per-file-ignores`, `--extend-per-file-ignores`~ _doesn't show any message_ - [x] File with syntax errors should exit with code 1 - [x] Language server - [x] Should show diagnostics for syntax errors - [x] Should not recommend a quick fix edit for adding `noqa` comment - [x] Same for `ruff-lsp` resolves: #8447 |
|
Micha Reiser
|
64700d296f
|
Remove ImportMap (#11234)
## Summary This PR removes the `ImportMap` implementation and all its routing through ruff. The import map was added in https://github.com/astral-sh/ruff/pull/3243 but we then never ended up using it to do cross file analysis. We are now working on adding multifile analysis to ruff, and revisit import resolution as part of it. ``` hyperfine --warmup 10 --runs 20 --setup "./target/release/ruff clean" \ "./target/release/ruff check crates/ruff_linter/resources/test/cpython -e -s --extend-select=I" \ "./target/release/ruff-import check crates/ruff_linter/resources/test/cpython -e -s --extend-select=I" Benchmark 1: ./target/release/ruff check crates/ruff_linter/resources/test/cpython -e -s --extend-select=I Time (mean ± σ): 37.6 ms ± 0.9 ms [User: 52.2 ms, System: 63.7 ms] Range (min … max): 35.8 ms … 39.8 ms 20 runs Benchmark 2: ./target/release/ruff-import check crates/ruff_linter/resources/test/cpython -e -s --extend-select=I Time (mean ± σ): 36.0 ms ± 0.7 ms [User: 50.3 ms, System: 58.4 ms] Range (min … max): 34.5 ms … 37.6 ms 20 runs Summary ./target/release/ruff-import check crates/ruff_linter/resources/test/cpython -e -s --extend-select=I ran 1.04 ± 0.03 times faster than ./target/release/ruff check crates/ruff_linter/resources/test/cpython -e -s --extend-select=I ``` I suspect that the performance improvement should even be more significant for users that otherwise don't have any diagnostics. ``` hyperfine --warmup 10 --runs 20 --setup "cd ../ecosystem/airflow && ../../ruff/target/release/ruff clean" \ "./target/release/ruff check ../ecosystem/airflow -e -s --extend-select=I" \ "./target/release/ruff-import check ../ecosystem/airflow -e -s --extend-select=I" Benchmark 1: ./target/release/ruff check ../ecosystem/airflow -e -s --extend-select=I Time (mean ± σ): 53.7 ms ± 1.8 ms [User: 68.4 ms, System: 63.0 ms] Range (min … max): 51.1 ms … 58.7 ms 20 runs Benchmark 2: ./target/release/ruff-import check ../ecosystem/airflow -e -s --extend-select=I Time (mean ± σ): 50.8 ms ± 1.4 ms [User: 50.7 ms, System: 60.9 ms] Range (min … max): 48.5 ms … 55.3 ms 20 runs Summary ./target/release/ruff-import check ../ecosystem/airflow -e -s --extend-select=I ran 1.06 ± 0.05 times faster than ./target/release/ruff check ../ecosystem/airflow -e -s --extend-select=I ``` ## Test Plan `cargo test` |
|
manunio
|
14d3fe6bfa
|
Add a idempotent fuzz_target for ruff_python_formatter (#9448)
Co-authored-by: Addison Crump <addison.crump@cispa.de> Co-authored-by: Addison Crump <me@addisoncrump.info> |