The parser currently uses single quotes to wrap tokens. This is
inconsistent with the rest of ruff/ty, which use backticks.
For example, see the inconsistent diagnostics produced in this simple
example: https://play.ty.dev/0a9d6eab-6599-4a1d-8e40-032091f7f50f
Consistently wrapping tokens in backticks produces uniform diagnostics.
Following the style decision of #723, in #2889 some quotes were already
switched into backticks.
This is also in line with Rust's guide on diagnostics
(https://rustc-dev-guide.rust-lang.org/diagnostics.html#diagnostic-structure):
> When code or an identifier must appear in a message or label, it
should be surrounded with backticks
Summary
--
This PR unifies the two different ways Ruff and ty construct syntax
errors. Ruff has been storing the primary message in the diagnostic
itself, while ty attached the message to the primary annotation:
```
> ruff check try.py
invalid-syntax: name capture `x` makes remaining patterns unreachable
--> try.py:2:10
|
1 | match 42:
2 | case x: ...
| ^
3 | case y: ...
|
Found 1 error.
> uvx ty check try.py
WARN ty is pre-release software and not ready for production use. Expect to encounter bugs, missing features, and fatal errors.
Checking ------------------------------------------------------------ 1/1 files
error[invalid-syntax]
--> try.py:2:10
|
1 | match 42:
2 | case x: ...
| ^ name capture `x` makes remaining patterns unreachable
3 | case y: ...
|
Found 1 diagnostic
```
I think there are benefits to both approaches, and I do like ty's
version, but I feel like we should pick one (and it might help with
#20901 eventually). I slightly prefer Ruff's version, so I went with
that. Hopefully this isn't too controversial, but I'm happy to close
this if it is.
Note that this shouldn't change any other diagnostic formats in ty
because
[`Diagnostic::primary_message`](98d27c4128/crates/ruff_db/src/diagnostic/mod.rs (L177))
was already falling back to the primary annotation message if the
diagnostic message was empty. As a result, I think this change will
partially resolve the FIXME therein.
Test Plan
--
Existing tests with updated snapshots
## Summary
This PR uses the new `Diagnostic` type for rendering formatter
diagnostics. This allows the formatter to inherit all of the output
formats already implemented in the linter and ty. For example, here's
the new `full` output format, with the formatting diff displayed using
the same infrastructure as the linter:
<img width="592" height="364" alt="image"
src="https://github.com/user-attachments/assets/6d09817d-3f27-4960-aa8b-41ba47fb4dc0"
/>
<details><summary>Resolved TODOs</summary>
<p>
~~There are several limitiations/todos here still, especially around the
`OutputFormat` type~~:
- [x] A few literal `todo!`s for the remaining `OutputFormat`s without
matching `DiagnosticFormat`s
- [x] The default output format is `full` instead of something more
concise like the current output
- [x] Some of the output formats (namely JSON) have information that
doesn't make much sense for these diagnostics
The first of these is definitely resolved, and I think the other two are
as well, based on discussion on the design document. In brief, we're
okay inheriting the default `OutputFormat` and can separate the global
option into `lint.output-format` and `format.output-format` in the
future, if needed; and we're okay including redundant information in the
non-human-readable output formats.
My last major concern is with the performance of the new code, as
discussed in the `Benchmarks` section below.
A smaller question is whether we should use `Diagnostic`s for formatting
errors too. I think the answer to this is yes, in line with changes
we're making in the linter too. I still need to implement that here.
</p>
</details>
<details><summary>Benchmarks</summary>
<p>
The values in the table are from a large benchmark on the CPython 3.10
code
base, which involves checking 2011 files, 1872 of which need to be
reformatted.
`stable` corresponds to the same code used on `main`, while
`preview-full` and
`preview-concise` use the new `Diagnostic` code gated behind `--preview`
for the
`full` and `concise` output formats, respectively. `stable-diff` uses
the
`--diff` to compare the two diff rendering approaches. See the full
hyperfine
command below for more details. For a sense of scale, the `stable`
output format
produces 1873 lines on stdout, compared to 855,278 for `preview-full`
and
857,798 for `stable-diff`.
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|:------------------|--------------:|---------:|---------:|-------------:|
| `stable` | 201.2 ± 6.8 | 192.9 | 220.6 | 1.00 |
| `preview-full` | 9113.2 ± 31.2 | 9076.1 | 9152.0 | 45.29 ± 1.54 |
| `preview-concise` | 214.2 ± 1.4 | 212.0 | 217.6 | 1.06 ± 0.04 |
| `stable-diff` | 3308.6 ± 20.2 | 3278.6 | 3341.8 | 16.44 ± 0.56 |
In summary, the `preview-concise` diagnostics are ~6% slower than the
stable
output format, increasing the average runtime from 201.2 ms to 214.2 ms.
The
`full` preview diagnostics are much more expensive, taking over 9113.2
ms to
complete, which is ~3x more expensive even than the stable diffs
produced by the
`--diff` flag.
My main takeaways here are:
1. Rendering `Edit`s is much more expensive than rendering the diffs
from `--diff`
2. Constructing `Edit`s actually isn't too bad
### Constructing `Edit`s
I also took a closer look at `Edit` construction by modifying the code
and
repeating the `preview-concise` benchmark and found that the main issue
is
constructing a `SourceFile` for use in the `Edit` rendering. Commenting
out the
`Edit` construction itself has basically no effect:
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|:----------|------------:|---------:|---------:|------------:|
| `stable` | 197.5 ± 1.6 | 195.0 | 200.3 | 1.00 |
| `no-edit` | 208.9 ± 2.2 | 204.8 | 212.2 | 1.06 ± 0.01 |
However, also omitting the source text from the `SourceFile`
construction
resolves the slowdown compared to `stable`. So it seems that copying the
full
source text into a `SourceFile` is the main cause of the slowdown for
non-`full`
diagnostics.
| Command | Mean [ms] | Min [ms] | Max [ms] | Relative |
|:-----------------|------------:|---------:|---------:|------------:|
| `stable` | 202.4 ± 2.9 | 197.6 | 207.9 | 1.00 |
| `no-source-text` | 202.7 ± 3.3 | 196.3 | 209.1 | 1.00 ± 0.02 |
### Rendering diffs
The main difference between `stable-diff` and `preview-full` seems to be
the diffing strategy we use from `similar`. Both versions use the same
algorithm, but in the existing
[`CodeDiff`](https://github.com/astral-sh/ruff/blob/main/crates/ruff_linter/src/source_kind.rs#L259)
rendering for the `--diff` flag, we only do line-level diffing, whereas
for `Diagnostic`s we use `TextDiff::iter_inline_changes` to highlight
word-level changes too. Skipping the word diff for `Diagnostic`s closes
most of the gap:
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|:---|---:|---:|---:|---:|
| `stable-diff` | 3.323 ± 0.015 | 3.297 | 3.341 | 1.00 |
| `preview-full` | 3.654 ± 0.019 | 3.618 | 3.682 | 1.10 ± 0.01 |
(In some repeated runs, I've seen as small as a ~5% difference, down
from 10% in the table)
This doesn't actually change any of our snapshots, but it would
obviously change the rendered result in a terminal since we wouldn't
highlight the specific words that changed within a line.
Another much smaller change that we can try is removing the deadline
from the `iter_inline_changes` call. It looks like there's a fair amount
of overhead from the default 500 ms deadline for computing these, and
using `iter_inline_changes(op, None)` (`None` for the optional deadline
argument) improves the runtime quite a bit:
| Command | Mean [s] | Min [s] | Max [s] | Relative |
|:---|---:|---:|---:|---:|
| `stable-diff` | 3.322 ± 0.013 | 3.298 | 3.341 | 1.00 |
| `preview-full` | 5.296 ± 0.030 | 5.251 | 5.366 | 1.59 ± 0.01 |
<hr>
<details><summary>hyperfine command</summary>
```shell
cargo build --release --bin ruff && hyperfine --ignore-failure --warmup 10 --export-markdown /tmp/table.md \
-n stable -n preview-full -n preview-concise -n stable-diff \
"./target/release/ruff format --check ./crates/ruff_linter/resources/test/cpython/ --no-cache" \
"./target/release/ruff format --check ./crates/ruff_linter/resources/test/cpython/ --no-cache --preview --output-format=full" \
"./target/release/ruff format --check ./crates/ruff_linter/resources/test/cpython/ --no-cache --preview --output-format=concise" \
"./target/release/ruff format --check ./crates/ruff_linter/resources/test/cpython/ --no-cache --diff"
```
</details>
</p>
</details>
## Test Plan
Some new CLI tests and manual testing
## Summary
Addresses
https://github.com/astral-sh/ruff/pull/20443#discussion_r2381237640 by
factoring out the `match` on the ruff output format in a way that should
be reusable by the formatter.
I didn't think this was going to work at first, but the fact that the
config holds options that apply only to certain output formats works in
our favor here. We can set up a single config for all of the output
formats and then use `try_from` to convert the `OutputFormat` to a
`DiagnosticFormat` later.
## Test Plan
Existing tests, plus a few new ones to make sure relocating the
`SHOW_FIX_SUMMARY` rendering worked, that was untested before. I deleted
a bunch of test code along with the `text` module, but I believe all of
it is now well-covered by the `full` and `concise` tests in `ruff_db`.
I also merged this branch into
https://github.com/astral-sh/ruff/pull/20443 locally and made sure that
the API actually helps. `render_diagnostics` dropped in perfectly and
passed the tests there too.
## Summary
This is the GitHub analog to #20117. This PR prepares to add a GitHub
output format to ty by moving the implementation from `ruff_linter` to
`ruff_db`. Hopefully this one is a bit easier to review
commit-by-commit. Almost all of the refactoring this time is in the
first commit, then the second commit adds the new `OutputFormat` variant
and moves the file into `ruff_db`. The third commit is just a small
touch up to use a private method that accommodates ty files so that we
can run the tests and update/move the snapshots.
I had to push a fourth commit to fix and test diagnostics without a
span/file.
## Test Plan
Existing tests
This stabilizes the behavior introduced in #16565 which (roughly) tries
to match an import like `import a.b.c` to an actual directory path
`a/b/c` in order to label it as first-party, rather than simply looking
for a directory `a`.
Mainly this affects the sorting of imports in the presence of namespace
packages, but a few other rules are affected as well.
The tests looked good. For the docs, I added a `## See also` section
pointing to
the closely-related F841 (unused-variable) and the corresponding section
to F841
pointing back to RUF059. It seems like you'd probably want both of these
active
or at least to know about the other when reading the docs.
- Renames functions to drop `expect_` from names.
- Make functions return `Option<LineColumn>` to appropriately signal
when range is not available.
- Update existing consumers to use `unwrap_or_default()`. Uncertain if
there are better fallback behaviors for individual consumers.
## Summary
This PR fixes#7352 by exposing the `show_fix_diff` option used in our
snapshot tests in the CLI. As the issue suggests, we plan to make this
the default output format in the future, so this is added to the `full`
output format in preview for now.
This turned out to be pretty straightforward. I just used our existing
`Applicability` settings to determine whether or not to print the diff.
The snapshot differences are because we now set
`Applicability::DisplayOnly` for our snapshot tests. This
`Applicability` is also used to determine whether or not the fix icon
(`[*]`) is rendered, so this is now shown for display-only fixes in our
snapshots. This was already the case previously, but we were only
setting `Applicability::Unsafe` in these tests and ignoring the
`Applicability` when rendering fix diffs. CLI users can't enable
display-only fixes, so this is only a test change for now, but this
should work smoothly if we decide to expose a `--display-only-fixes`
flag or similar in the future.
I also deleted the `PrinterFlags::SHOW_FIX_DIFF` flag. This was
completely unused before, and it seemed less confusing just to delete it
than to enable it in the right place and check it along with the
`OutputFormat` and `preview`.
## Test Plan
I only added one CLI test for now. I'm kind of assuming that we have
decent coverage of the cases where this shouldn't be firing, especially
the `output_format` CLI test, which shows that this definitely doesn't
affect non-preview `full` output. I'm happy to add more tests with
different combinations of options, if we're worried about any in
particular. I did try `--diff` and `--preview` and a few other
combinations manually.
And here's a screenshot using our trusty UP049 example from the design
discussion confirming that all the colors and other formatting still
look as expected:
<img width="786" height="629" alt="image"
src="https://github.com/user-attachments/assets/94e408bc-af7b-4573-b546-a5ceac2620f2"
/>
And one with an unsafe fix to see the footer:
<img width="782" height="367" alt="image"
src="https://github.com/user-attachments/assets/bbb29e47-310b-4293-b2c2-cc7aee3baff4"
/>
## Related issues and PR
- https://github.com/astral-sh/ruff/issues/7352
- https://github.com/astral-sh/ruff/pull/12595
- https://github.com/astral-sh/ruff/issues/12598
- https://github.com/astral-sh/ruff/issues/12599
- https://github.com/astral-sh/ruff/issues/12600
I think we could probably close all of these issues now. I think we've
either resolved or avoided most of them, and if we encounter them again
with the new output format, it would probably make sense to open new
ones anyway.
## Summary
This PR is a first step toward adding a GitLab output format to ty. It
converts the `GitlabEmitter` from `ruff_linter` to a `GitlabRenderer` in
`ruff_db` and updates its implementation to handle non-Ruff files and
diagnostics without primary spans. I tried to break up the changes here
so that they're easy to review commit-by-commit, or at least in groups
of commits:
- [preparatory changes in-place in `ruff_linter` and a `ruff_db`
skeleton](0761b73a61)
- [moving the code over with no implementation changes mixed
in](0761b73a61..8f909ea0bb)
- [tidying up the code now in
`ruff_db`](9f047c4f9f..e5e217fcd6)
This wasn't strictly necessary, but I also added some `Serialize`
structs instead of calling `json!` to make it a little clearer that we
weren't modifying the schema (e4c4bee35d).
I plan to follow this up with a separate PR exposing this output format
in the ty CLI, which should be quite straightforward.
## Test Plan
Existing tests, especially the two that show up in the diff as renamed
nearly without changes
Summary
--
This is a preparatory PR in support of #19919. It moves our `Diff`
rendering code from `ruff_linter` to `ruff_db`, where we have direct
access to the `DiagnosticStylesheet` used by our other diagnostic
rendering code. As shown by the tests, this shouldn't cause any visible
changes. The colors aren't exactly the same, as I note in a TODO
comment, but I don't think there's any existing way to see those, even
in tests.
The `Diff` implementation is mostly unchanged. I just switched from a
Ruff-specific `SourceFile` to a `DiagnosticSource` (removing an
`expect_ruff_source_file` call) and updated the `LineStyle` struct and
other styling calls to use `fmt_styled` and our existing stylesheet.
In support of these changes, I added three styles to our stylesheet:
`insertion` and `deletion` for the corresponding diff operations, and
`underline`, which apparently we _can_ use, as I hoped on Discord. This
isn't supported in all terminals, though. It worked in ghostty but not
in st for me.
I moved the `calculate_print_width` function from the now-deleted
`diff.rs` to a method on `OneIndexed`, where it was available everywhere
we needed it. I'm not sure if that's desirable, or if my other changes
to the function are either (using `ilog10` instead of a loop). This does
make it `const` and slightly simplifies things in my opinion, but I'm
happy to revert it if preferred.
I also inlined a version of `show_nonprinting` from the
`ShowNonprinting` trait in `ruff_linter`:
f4be05a83b/crates/ruff_linter/src/text_helpers.rs (L3-L5)
This trait is now only used in `source_kind.rs`, so I'm not sure it's
worth having the trait or the macro-generated implementation (which is
only called once). This is obviously closely related to our unprintable
character handling in diagnostic rendering, but the usage seems
different enough not to try to combine them.
f4be05a83b/crates/ruff_db/src/diagnostic/render.rs (L990-L998)
We could also move the trait to another crate where we can use it in
`ruff_db` instead of inlining here, of course.
Finally, this PR makes `TextEmitter` a very thin wrapper around a
`DisplayDiagnosticsConfig`. It's still used in a few places, though,
unlike the other emitters we've replaced, so I figured it was worth
keeping around. It's a pretty nice API for setting all of the options on
the config and then passing that along to a `DisplayDiagnostics`.
Test Plan
--
Existing snapshot tests with diffs
## Summary
Fixes#19881. While I was here, I also made a couple of related tweaks
to the output format. First, we don't need to strip the `SyntaxError: `
prefix anymore since that's not added directly to the diagnostic message
after #19644. Second, we can use `secondary_code_or_id` to fall back on
the lint ID for syntax errors, which changes the `check_name` from
`syntax-error` to `invalid-syntax`. And then the main change requested
in the issue, prepending the `check_name` to the description.
## Test Plan
Existing tests and a new screenshot from GitLab:
<img width="362" height="113" alt="image"
src="https://github.com/user-attachments/assets/97654ad4-a639-4489-8c90-8661c7355097"
/>
## Summary
This is a follow-up to
https://github.com/astral-sh/ruff/pull/19415#discussion_r2263456740 to
remove some unused code. As Micha noticed,
`GroupedEmitter::with_show_source` was only used in local unit tests[^1]
and was safe to remove. This allowed deleting `MessageCodeFrame` and a
lot more helper code previously shared with the `full` output format.
I also moved some other code from `text.rs` and `message/mod.rs` into
`grouped.rs` that is now only used for the `grouped` format. With a
little refactoring of the `concise` rendering logic in `ruff_db`, we
could probably remove `RuleCodeAndBody` too. The only difference I see
from the `concise` output is whether we print the filename next to the
row and column or not:
```shell
> ruff check --output-format concise
try.py:1:8: F401 [*] `math` imported but unused
> ruff check --output-format grouped
try.py:
1:8 F401 [*] `math` imported but unused
```
But I didn't try to do that here.
## Test Plan
Existing tests, with the source code no longer displayed. I also deleted
one test, as it was now a duplicate of the `default` test.
[^1]: "Local unit tests" as opposed to all of our linter snapshot tests,
as is the case for `TextEmitter::with_show_fix_diff`. We also want to
expose that to users eventually
(https://github.com/astral-sh/ruff/issues/7352), which I don't believe
is the case for the `grouped` format.
## Summary
This PR switches the `full` output format in Ruff over to use the
rendering code
in `ruff_db`. As proposed in the design doc, this involves a lot of
changes to the snapshot output.
I also had to comment out this assertion with a TODO to replace it after
https://github.com/astral-sh/ruff/issues/19688 because many of Ruff's
"file-level" annotations aren't actually file-level. They just happen to
occur at the start of the file, especially in tests with very short
snippets.
529d81daca/crates/ruff_annotate_snippets/src/renderer/display_list.rs (L1204-L1208)
I broke up the snapshot commits at the end into several blocks, but I
don't think it's enough to help with review. The first few (notebooks,
syntax errors, and test rules) are small enough to look at, but I
couldn't really think of other categories beyond that. I'm happy to
break those up or pick out specific examples beyond what I have below,
if that would help.
The minimal code changes are in this
[range](abd28f1e77),
with the snapshot commits following. Moving the `FullRenderer` and
updating the `EmitterFlags` aren't strictly necessary either. I even
dropped the renderer commit this morning but figured it made sense to
keep it since we have the `full` module for tests. I don't feel strongly
either way.
## Test Plan
I did actually click through all 1700 snapshots individually instead of
accepting them all at once, although I moved through them quickly. There
are a
few main categories:
### Lint diagnostics
```diff
-unused.py:8:19: F401 [*] `pathlib` imported but unused
+F401 [*] `pathlib` imported but unused
+ --> unused.py:8:19
|
7 | # Unused, _not_ marked as required (due to the alias).
8 | import pathlib as non_alias
- | ^^^^^^^^^ F401
+ | ^^^^^^^^^
9 |
10 | # Unused, marked as required.
|
- = help: Remove unused import: `pathlib`
+help: Remove unused import: `pathlib`
```
- The filename and line numbers are moved to the second line
- The second noqa code next to the underline is removed
### Syntax errors
These are much like the above.
```diff
- -:1:16: invalid-syntax: Expected one or more symbol names after import
+ invalid-syntax: Expected one or more symbol names after import
+ --> -:1:16
|
1 | from foo import
| ^
```
One thing I noticed while reviewing some of these, but I don't think is
strictly syntax-error-related, is that some of the new diagnostics have
a little less context after the error. I don't think this is a problem,
but it's one small discrepancy I hadn't noticed before. Here's a minor
example:
```diff
-syntax_errors.py:1:15: invalid-syntax: Expected one or more symbol names after import
+invalid-syntax: Expected one or more symbol names after import
+ --> syntax_errors.py:1:15
|
1 | from os import
| ^
2 |
3 | if call(foo
-4 | def bar():
|
```
And one of the biggest examples:
```diff
-E30_syntax_error.py:18:11: invalid-syntax: Expected ')', found newline
+invalid-syntax: Expected ')', found newline
+ --> E30_syntax_error.py:18:11
|
16 | pass
17 |
18 | foo = Foo(
| ^
-19 |
-20 |
-21 | def top(
|
```
Similarly, a few of the lint diagnostics showed that the cut indicator
calculation for overly long lines is also slightly different, but I
think that's okay too.
### Full-file diagnostics
```diff
-comment.py:1:1: I002 [*] Missing required import: `from __future__ import annotations`
+I002 [*] Missing required import: `from __future__ import annotations`
+--> comment.py:1:1
+help: Insert required import: `from __future__ import annotations`
+
```
As noted above, these will be much more rare after #19688 too. This case
isn't a true full-file diagnostic and will render a snippet in the
future, but you can see that we're now rendering the help message that
would have been discarded before. In contrast, this is a true full-file
diagnostic and should still look like this after #19688:
```diff
-__init__.py:1:1: A005 Module `logging` shadows a Python standard-library module
+A005 Module `logging` shadows a Python standard-library module
+--> __init__.py:1:1
```
### Jupyter notebooks
There's nothing particularly different about these, just showing off the
cell index again.
```diff
- Jupyter.ipynb:cell 3:1:7: F821 Undefined name `x`
+ F821 Undefined name `x`
+ --> Jupyter.ipynb:cell 3:1:7
|
1 | print(x)
- | ^ F821
+ | ^
|
```
Summary
--
This is the other commit I wanted to spin off from #19415, currently
stacked on #19644.
This PR suppresses blank snippets for empty ranges at the very beginning
of a file, and for empty ranges in non-existent files. Ruff includes
empty ranges for IO errors, for example.
f4e93b6335/crates/ruff_linter/src/message/text.rs (L100-L110)
The diagnostics now look like this (new snapshot test):
```
error[test-diagnostic]: main diagnostic message
--> example.py:1:1
```
Instead of [^*]
```
error[test-diagnostic]: main diagnostic message
--> example.py:1:1
|
|
```
Test Plan
--
A new `ruff_db` test showing the expected output format
[^*]: This doesn't correspond precisely to the example in the PR because
of some details of the diagnostic builder helper methods in `ruff_db`,
but you can see another example in the current version of the summary in
#19415.
## Summary
This PR is a spin-off from https://github.com/astral-sh/ruff/pull/19415.
It enables replacing the severity and lint name in a ty-style
diagnostic:
```
error[unused-import]: `os` imported but unused
```
with the noqa code and optional fix availability icon for a Ruff
diagnostic:
```
F401 [*] `os` imported but unused
F821 Undefined name `a`
```
or nothing at all for a Ruff syntax error:
```
SyntaxError: Expected one or more symbol names after import
```
Ruff adds the `SyntaxError` prefix to these messages manually.
Initially (d912458), I just passed a `hide_severity` flag through a
bunch of calls to get it into `annotate-snippets`, but after looking at
it again today, I think reusing the `None` severity/level gave a nicer
result. As I note in a lengthy code comment, I think all of this code
should be temporary and reverted when Ruff gets real severities, so
hopefully it's okay if it feels a little hacky.
I think the main visible downside of this approach is that we can't
style the asterisk in the fix availabilty icon in cyan, as in Ruff's
current output. It's part of the message in this PR and any styling gets
overwritten in `annotate-snippets`.
<img width="400" height="342" alt="image"
src="https://github.com/user-attachments/assets/57542ec9-a81c-4a01-91c7-bd6d7ec99f99"
/>
Hmm, I guess reusing `Level::None` also means the `F401` isn't red
anymore. Maybe my initial approach was better after all. In any case,
the rest of the PR should be basically the same, it just depends how we
want to toggle the severity.
## Test Plan
New `ruff_db` tests. These snapshots should be compared to the two tests
just above them (`hide_severity_output` vs `output` and
`hide_severity_syntax_errors` against `syntax_errors`).
## Summary
I was a bit stuck on some snapshot differences I was seeing in #19415,
but @BurntSushi pointed out that `annotate-snippets` already normalizes
tabs on its own, which was very helpful! Instead of applying this change
directly to the other branch, I wanted to try applying it in
`ruff_linter` first. This should very slightly reduce the number of
changes in #19415 proper.
It looks like `annotate-snippets` always expands a tab to four spaces,
whereas I think we were aligning to tab stops:
```diff
6 | spam(ham[1], { eggs: 2})
7 | #: E201:1:6
- 8 | spam( ham[1], {eggs: 2})
- | ^^^ E201
+ 8 | spam( ham[1], {eggs: 2})
+ | ^^^^ E201
```
```diff
61 | #: E203:2:15 E702:2:16
62 | if x == 4:
-63 | print(x, y) ; x, y = y, x
- | ^ E203
+63 | print(x, y) ; x, y = y, x
+ | ^^^^ E203
```
```diff
E27.py:15:6: E271 [*] Multiple spaces after keyword
|
-13 | True and False
+13 | True and False
14 | #: E271
15 | a and b
| ^^ E271
```
I don't think this is too bad and has the major benefit of allowing us
to pass the non-tab-expanded range to `annotate-snippets` in #19415,
where it's also displayed in the header. Ruff doesn't have this problem
currently because it uses its own concise diagnostic output as the
header for full diagnostics, where the pre-expansion range is used
directly.
## Test Plan
Existing tests with a few snapshot updates
Summary
--
I looked at other uses of `TextEmitter`, and I think this should be the
only one affected by this. The other integration tests must work
properly since they're run with `assert_cmd_snapshot!`, which I assume
triggers the `SHOULD_COLORIZE` case, and the `cfg!(test)` check will
work for uses in `ruff_linter`.
4a4dc38b5b/crates/ruff_linter/src/message/text.rs (L36-L44)
Alternatively, we could probably move this to a CLI test instead.
Test Plan
--
`cargo test -p ruff`, which was failing on `main` with color codes in
the output before this
## Summary
This PR moves most of the work of rendering concise diagnostics in Ruff
into `ruff_db`, where the code is shared with ty. To accomplish this
without breaking backwards compatibility in Ruff, there are two main
changes on the `ruff_db`/ty side:
- Added the logic from Ruff for remapping notebook line numbers to cells
- Reordered the fields in the diagnostic to match Ruff and rustc
```text
# old
error[invalid-assignment] try.py:3:1: Object of type `Literal[1]` is not
assignable to `str`
# new
try.py:3:1: error[invalid-assignment]: Object of type `Literal[1]` is
not assignable to `str`
```
I don't think the notebook change failed any tests on its own, and only
a handful of snaphots changed in ty after reordering the fields, but
this will obviously affect any other uses of the concise format, outside
of tests, too.
The other big change should only affect Ruff:
- Added three new `DisplayDiagnosticConfig` options
Micha and I hoped that we could get by with one option
(`hide_severity`), but Ruff also toggles `show_fix_status` itself,
independently (there are cases where we want neither severity nor the
fix status), and during the implementation I realized we also needed
access to an `Applicability`. The main goal here is to suppress the
severity (`error` above) because ruff only uses the `error` severity and
to use the secondary/noqa code instead of the line name
(`invalid-assignment` above).
```text
# ty - same as "new" above
try.py:3:1: error[invalid-assignment]: Object of type `Literal[1]` is
not assignable to `str`
# ruff
try.py:3:1: RUF123 [*] Object of type `Literal[1]` is not assignable to
`str`
```
This part of the concise diagnostic is actually shared with the `full`
output format in Ruff, but with the settings above, there are no
snapshot changes to either format.
## Test Plan
Existing tests with the handful of updates mentioned above, as well as
some new tests in the `concise` module.
Also this PR. Swapping the fields might have broken mypy_primer, unless
it occasionally times out on its own.
I also ran this script in the root of my Ruff checkout, which also has
CPython in it:
```shell
flags=(--isolated --no-cache --no-respect-gitignore --output-format concise .)
diff <(target/release/ruff check ${flags[@]} 2> /dev/null) \
<(ruff check ${flags[@]} 2> /dev/null)
```
This yielded an expected diff due to some t-string error changes on main
since 0.12.4:
```diff
33622c33622
< crates/ruff_python_parser/resources/inline/err/f_string_lambda_without_parentheses.py:1:15: SyntaxError: Expected an element of or the end of the f-string
---
> crates/ruff_python_parser/resources/inline/err/f_string_lambda_without_parentheses.py:1:15: SyntaxError: Expected an f-string or t-string element or the end of the f-string or t-string
33742c33742
< crates/ruff_python_parser/resources/inline/err/implicitly_concatenated_unterminated_string_multiline.py:4:1: SyntaxError: Expected an element of or the end of the f-string
---
> crates/ruff_python_parser/resources/inline/err/implicitly_concatenated_unterminated_string_multiline.py:4:1: SyntaxError: Expected an f-string or t-string element or the end of the f-string or t-string
34131c34131
< crates/ruff_python_parser/resources/inline/err/t_string_lambda_without_parentheses.py:2:15: SyntaxError: Expected an element of or the end of the t-string
---
> crates/ruff_python_parser/resources/inline/err/t_string_lambda_without_parentheses.py:2:15: SyntaxError: Expected an f-string or t-string element or the end of the f-string or t-string
```
So modulo color, the results are identical on 38,186 errors in our test
suite and CPython 3.10.
---------
Co-authored-by: David Peter <mail@david-peter.de>
Summary
--
This PR tweaks Ruff's internal usage of the new diagnostic model to more
closely
match the intended use, as I understand it. Specifically, it moves the
fix/help
suggestion from the primary annotation's message to a subdiagnostic. In
turn, it
adds the secondary/noqa code as the new primary annotation message. As
shown in
the new `ruff_db` tests, this more closely mirrors Ruff's current
diagnostic
output.
I also added `Severity::Help` to render the fix suggestion with a
`help:` prefix
instead of `info:`.
These changes don't have any external impact now but should help a bit
with #19415.
Test Plan
--
New full output format tests in `ruff_db`
Rendered Diagnostics
--
Full diagnostic output from `annotate-snippets` in this PR:
```
error[unused-import]: `os` imported but unused
--> fib.py:1:8
|
1 | import os
| ^^
|
help: Remove unused import: `os`
```
Current Ruff output for the same code:
```
fib.py:1:8: F401 [*] `os` imported but unused
|
1 | import os
| ^^ F401
|
= help: Remove unused import: `os`
```
Proposed final output after #19415:
```
F401 [*] `os` imported but unused
--> fib.py:1:8
|
1 | import os
| ^^
|
help: Remove unused import: `os`
```
These are slightly updated from
https://github.com/astral-sh/ruff/pull/19464#issuecomment-3097377634
below to remove the extra noqa codes in the primary annotation messages
for the first and third cases.
Summary
--
This PR moves the JUnit output format to the new rendering
infrastructure. As I
mention in a TODO in the code, there's some code that will be shared
with the
`grouped` output format. Hopefully I'll have that PR up too by the time
this one
is reviewed.
Test Plan
--
Existing tests moved to `ruff_db`
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
Summary
--
This is a very simple output format, the only decision is what to do if
the file
is missing from the diagnostic. For now, I opted to `unwrap_or_default`
both the
path and the `OneIndexed` row number, giving `:1: main diagnostic
message` in
the test without a file.
Another quirk here is that the path is relativized. I just pasted in the
`relativize_path` and `get_cwd` implementations from `ruff_linter::fs`
for now,
but maybe there's a better place for them.
I didn't see any details about why this needs to be relativized in the
original
[issue](https://github.com/astral-sh/ruff/issues/1953),
[PR](https://github.com/astral-sh/ruff/pull/1995), or in the pylint
[docs](https://flake8.pycqa.org/en/latest/internal/formatters.html#pylint-formatter),
but it did change the results of the CLI integration test when I tried
deleting
it. I haven't been able to reproduce that in the CLI, though, so it may
only
happen with `Command::current_dir`.
Test Plan
--
Tests ported from `ruff_linter` and a new test for the case with no file
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
## Summary
Another output format like #19133. This is the
[reviewdog](https://github.com/reviewdog/reviewdog) output format, which
is somewhat similar to regular JSON. Like #19270, in the first commit I
converted from using `json!` to `Serialize` structs, then in the second
commit I moved the module to `ruff_db`.
The reviewdog
[schema](320a8e73a9/proto/rdf/jsonschema/DiagnosticResult.json)
seems a bit more flexible than our JSON schema, so I'm not sure if we
need any preview checks here. I'll flag the places I wasn't sure about
as review comments.
## Test Plan
New tests in `rdjson.rs`, ported from the old `rjdson.rs` module, as
well as the new CLI output tests.
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
## Summary
This was originally stacked on #19129, but some of the changes I made
for JSON also impacted the Azure format, so I went ahead and combined
them. The main changes here are:
- Implementing `FileResolver` for Ruff's `EmitterContext`
- Adding `FileResolver::notebook_index` and `FileResolver::is_notebook`
methods
- Adding a `DisplayDiagnostics` (with an "s") type for rendering a group
of diagnostics at once
- Adding `Azure`, `Json`, and `JsonLines` as new `DiagnosticFormat`s
I tried a couple of alternatives to the `FileResolver::notebook` methods
like passing down the `NotebookIndex` separately and trying to reparse a
`Notebook` from Ruff's `SourceFile`. The latter seemed promising, but
the `SourceFile` only stores the concatenated plain text of the
notebook, not the re-parsable JSON. I guess the current version is just
a variation on passing the `NotebookIndex`, but at least we can reuse
the existing `resolver` argument. I think a lot of this can be cleaned
up once Ruff has its own actual file resolver.
As suggested, I also tried deleting the corresponding `Emitter` files in
`ruff_linter`, but it doesn't look like git was able to follow this as a
rename. It did, however, track that the tests were moved, so the
snapshots should be easy to review.
## Test Plan
Existing Ruff tests ported to tests in `ruff_db`. I think some other
existing ruff tests also cover parts of this refactor.
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
## Summary
See https://github.com/astral-sh/ruff/pull/19133#discussion_r2198413586
for recent discussion. This PR moves to using structs for the types in
our JSON output format instead of the `json!` macro.
I didn't rename any of the `message` references because that should be
handled when rebasing #19133 onto this.
My plan for handling the `preview` behavior with the new diagnostics is
to use a wrapper enum. Something like:
```rust
#[derive(Serialize)]
#[serde(untagged)]
pub(crate) enum JsonDiagnostic<'a> {
Old(OldJsonDiagnostic<'a>),
}
#[derive(Serialize)]
pub(crate) struct OldJsonDiagnostic<'a> {
// ...
}
```
Initially I thought I could use a `&dyn Serialize` for the affected
fields, but I see that `Serialize` isn't dyn-compatible in testing this
now.
## Test Plan
Existing tests. One quirk of the new types is that their fields are in
alphabetical order. I guess `json!` sorts the fields alphabetically? The
tests were failing before I sorted the struct fields.
## Other formats
It looks like the `rdjson`, `sarif`, and `gitlab` formats also use
`json!`, so if we decide to merge this, I can do something similar for
those before moving them to the new diagnostic format.
## Summary
This PR addresses some additional feedback on #19053:
- Renaming the `syntax_error` methods to `invalid_syntax` to match the
lint id
- Moving the standalone `diagnostic_from_violation` function to
`Violation::into_diagnostic`
- Removing the `Ord` and `PartialOrd` implementations from `Diagnostic`
in favor of `Diagnostic::start_ordering`
## Test Plan
Existing tests
## Additional Follow-ups
Besides these, I also put the following comments on my todo list, but
they seemed like they might be big enough to have their own PRs:
- [Use `LintId::IOError` for IO
errors](https://github.com/astral-sh/ruff/pull/19053#discussion_r2189425922)
- [Move `Fix` and
`Edit`](https://github.com/astral-sh/ruff/pull/19053#discussion_r2189448647)
- [Avoid so many
unwraps](https://github.com/astral-sh/ruff/pull/19053#discussion_r2189465980)
## Summary
This PR is a collaboration with @AlexWaygood from our pairing session
last Friday.
The main goal here is removing `ruff_linter::message::OldDiagnostic` in
favor of
using `ruff_db::diagnostic::Diagnostic` directly. This involved a few
major steps:
- Transferring the fields
- Transferring the methods and trait implementations, where possible
- Converting some constructor methods to free functions
- Moving the `SecondaryCode` struct
- Updating the method names
I'm hoping that some of the methods, especially those in the
`expect_ruff_*`
family, won't be necessary long-term, but I avoided trying to replace
them
entirely for now to keep the already-large diff a bit smaller.
### Related refactors
Alex and I noticed a few refactoring opportunities while looking at the
code,
specifically the very similar implementations for
`create_parse_diagnostic`,
`create_unsupported_syntax_diagnostic`, and
`create_semantic_syntax_diagnostic`.
We combined these into a single generic function, which I then copied
into
`ruff_linter::message` with some small changes and a TODO to combine
them in the
future.
I also deleted the `DisplayParseErrorType` and `TruncateAtNewline` types
for
reporting parse errors. These were added in #4124, I believe to work
around the
error messages from LALRPOP. Removing these didn't affect any tests, so
I think
they were unnecessary now that we fully control the error messages from
the
parser.
On a more minor note, I factored out some calls to the
`OldDiagnostic::filename`
(now `Diagnostic::expect_ruff_filename`) function to avoid repeatedly
allocating
`String`s in some places.
### Snapshot changes
The `show_statistics_syntax_errors` integration test changed because the
`OldDiagnostic::name` method used `syntax-error` instead of
`invalid-syntax`
like in ty. I think this (`--statistics`) is one of the only places we
actually
use this name for syntax errors, so I hope this is okay. An alternative
is to
use `syntax-error` in ty too.
The other snapshot changes are from removing this code, as discussed on
[Discord](https://discord.com/channels/1039017663004942429/1228460843033821285/1388252408848847069):
34052a1185/crates/ruff_linter/src/message/mod.rs (L128-L135)
I think both of these are technically breaking changes, but they only
affect
syntax errors and are very narrow in scope, while also pretty
substantially
simplifying the refactor, so I hope they're okay to include in a patch
release.
## Test plan
Existing tests, with the adjustments mentioned above
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
## Summary
I think this should be the last step before combining `OldDiagnostic`
and `ruff_db::Diagnostic`. We can't store a `NoqaCode` on
`ruff_db::Diagnostic`, so I converted the `noqa_code` field to an
`Option<String>` and then propagated this change to all of the callers.
I tried to use `&str` everywhere it was possible, so I think the
remaining `to_string` calls are necessary. I spent some time trying to
convert _everything_ to `&str` but ran into lifetime issues, especially
in the `FixTable`. Maybe we can take another look at that if it causes a
performance regression, but hopefully these paths aren't too hot. We
also avoid some `to_string` calls, so it might even out a bit too.
## Test Plan
Existing tests
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
## Summary
This PR removes the last two places we were using `NoqaCode::rule` in
`linter.rs` (see
https://github.com/astral-sh/ruff/pull/18391#discussion_r2154637329 and
https://github.com/astral-sh/ruff/pull/18391#discussion_r2154649726) by
checking whether fixes are actually desired before adding them to a
`DiagnosticGuard`. I implemented this by storing a `Violation`'s `Rule`
on the `DiagnosticGuard` so that we could check if it was enabled in the
embedded `LinterSettings` when trying to set a fix.
All of the corresponding `set_fix` methods on `OldDiagnostic` were now
unused (except in tests where I just set `.fix` directly), so I moved
these to the guard instead of keeping both sets.
The very last place where we were using `NoqaCode::rule` was in the
cache. I just reverted this to parsing the `Rule` from the name. I had
forgotten to update the comment there anyway. Hopefully this doesn't
cause too much of a perf hit.
In terms of binary size, we're back down almost to where `main` was two
days ago
(https://github.com/astral-sh/ruff/pull/18391#discussion_r2155034320):
```
41,559,344 bytes for main 2 days ago
41,669,840 bytes for #18391
41,653,760 bytes for main now (after #18391 merged)
41,602,224 bytes for this branch
```
Only 43 kb up, but that shouldn't all be me this time :)
## Test Plan
Existing tests and benchmarks on this PR
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](8156992540)
before I started renaming.
Test Plan
--
Existing tests
Future Work
--
I think it's time to start shifting some of these fields to the new
`Diagnostic`
kind. I believe we want `Fix` for sure, but I'm less sure about the
others. We
may want to keep a thin wrapper type here anyway to implement a `rule`
method,
so we could leave some of these fields on that too.
## Summary
As the title says, this PR removes the `Message::to_rule` method by
replacing related uses of `Rule` with `NoqaCode` (or the rule's name in
the case of the cache). Where it seemed a `Rule` was really needed, we
convert back to the `Rule` by parsing either the rule name (with
`str::parse`) or the `NoqaCode` (with `Rule::from_code`).
I thought this was kind of like cheating and that it might not resolve
this part of Micha's
[comment](https://github.com/astral-sh/ruff/pull/18391#issuecomment-2933764275):
> because we can't add Rule to Diagnostic or **have it anywhere in our
shared rendering logic**
but after looking again, the only remaining `Rule` conversion in
rendering code is for the SARIF output format. The other two non-test
`Rule` conversions are for caching and writing a fix summary, which I
don't think fall into the shared rendering logic. That leaves the SARIF
format as the only real problem, but maybe we can delay that for now.
The motivation here is that we won't be able to store a `Rule` on the
new `Diagnostic` type, but we should be able to store a `NoqaCode`,
likely as a string.
## Test Plan
Existing tests
##
[Benchmarks](https://codspeed.io/astral-sh/ruff/branches/brent%2Fremove-to-rule)
Almost no perf regression, only -1% on
`linter/default-rules[large/dataset.py]`.
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
Summary
--
This is the last main difference between the `OldDiagnostic` and
`Message`
types, so attaching a `SourceFile` to `OldDiagnostic` should make
combining the
two types almost trivial.
Initially I updated the remaining rules without access to a `Checker` to
take a
`&SourceFile` directly, but after Micha's suggestion in
https://github.com/astral-sh/ruff/pull/18356#discussion_r2113281552, I
updated all of these calls to take a
`LintContext` instead. This new type is a thin wrapper around a
`RefCell<Vec<OldDiagnostic>>`
and a `SourceFile` and now has the `report_diagnostic` method returning
a `DiagnosticGuard` instead of `Checker`.
This allows the same `Drop`-based implementation to be used in cases
without a `Checker` and also avoids a lot of intermediate allocations of
`Vec<OldDiagnostic>`s.
`Checker` now also contains a `LintContext`, which it defers to for its
`report_diagnostic` methods, which I preserved for convenience.
Test Plan
--
Existing tests
Summary
--
It's a bit late in the refactoring process, but I think there are still
a couple of PRs left before getting rid of this type entirely, so I
thought it would still be worth doing.
This PR is just a quick rename with no other changes.
Test Plan
--
Existing tests
## Summary
This PR unifies the ruff `Message` enum variants for syntax errors and
rule violations into a single `Message` struct consisting of a shared
`db::Diagnostic` and some additional, optional fields used for some rule
violations.
This version of `Message` is nearly a drop-in replacement for
`ruff_diagnostics::Diagnostic`, which is the next step I have in mind
for the refactor.
I think this is also a useful checkpoint because we could possibly add
some of these optional fields to the new `Diagnostic` type. I think
we've previously discussed wanting support for `Fix`es, but the other
fields seem less relevant, so we may just need to preserve the `Message`
wrapper for a bit longer.
## Test plan
Existing tests
---------
Co-authored-by: Micha Reiser <micha@reiser.io>
## Summary
This PR deletes the `DiagnosticKind` type by inlining its three fields
(`name`, `body`, and `suggestion`) into three other diagnostic types:
`Diagnostic`, `DiagnosticMessage`, and `CacheMessage`.
Instead of deferring to an internal `DiagnosticKind`, both `Diagnostic`
and `DiagnosticMessage` now have their own macro-generated `AsRule`
implementations.
This should make both https://github.com/astral-sh/ruff/pull/18051 and
another follow-up PR changing the type of `name` on `CacheMessage`
easier since its type will be able to change separately from
`Diagnostic` and `DiagnosticMessage`.
## Test Plan
Existing tests
## Summary
This PR is a first step toward integration of the new `Diagnostic` type
into ruff. There are two main changes:
- A new `UnifiedFile` enum wrapping `File` for red-knot and a
`SourceFile` for ruff
- ruff's `Message::SyntaxError` variant is now a `Diagnostic` instead of
a `SyntaxErrorMessage`
The second of these changes was mostly just a proof of concept for the
first, and it went pretty smoothly. Converting `DiagnosticMessage`s will
be most of the work in replacing `Message` entirely.
## Test Plan
Existing tests, which show no changes.
---------
Co-authored-by: Carl Meyer <carl@astral.sh>
Co-authored-by: Micha Reiser <micha@reiser.io>
When attempting to determine whether `import foo.bar.baz` is a known
first-party import relative to [user-provided source
paths](https://docs.astral.sh/ruff/settings/#src), when `preview` is
enabled we now check that `SRC/foo/bar/baz` is a directory or
`SRC/foo/bar/baz.py` or `SRC/foo/bar/baz.pyi` exist.
Previously, we just checked the analogous thing for `SRC/foo`, but this
can be misleading in situations with disjoint namespace packages that
share a common base name (e.g. we may be working inside the namespace
package `foo.buzz` and importing `foo.bar` from elsewhere).
Supersedes #12987Closes#12984
add fix safety section to docs for #15584, I'm new to ruff and not sure
if the content of this PR is correct, but I hope it can be helpful.
---------
Co-authored-by: Brent Westbrook <brentrwestbrook@gmail.com>
## Summary
There is a new official URL for the typing documentation:
https://typing.python.org/
Change all https://typing.readthedocs.io/ links to use the new sub
domain, which is slightly shorter and looks more official.
## Test Plan
Tested to see if each and every new URL is accessible. I noticed that
some links go to https://typing.python.org/en/latest/source/stubs.html
which seems to be outdated, but that is a separate issue. The same page
shows up for the old URL.
<!--
Thank you for contributing to Ruff! To help us out with reviewing,
please consider the following:
- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title?
- Does this pull request include references to any relevant issues?
-->
## Summary
I decided to disable the new
[`needless_continue`](https://rust-lang.github.io/rust-clippy/master/index.html#needless_continue)
rule because I often found the explicit `continue` more readable over an
empty block or having to invert the condition of an other branch.
## Test Plan
`cargo test`
---------
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Luca Chiodini
Brent Westbrook
Dylan
Nikolas Hearp
Amethyst Reese
Micha Reiser
cristian64
Hunter Hogan
Hans
David Peter