as shown in the snapshot changes, this allows us to manually align what
annotate-snippets calls the header sigil (the `-->` arrow in the diagnostic
header) with our diff line number separators.
these were aligned automatically in the linter because we were also emitting
snippets with annotate-snippets with the same line numbers, but since the
formatter diagnostics only have diffs and not snippets, the default line number
width was zero.
note that this still isn't perfect because we align with the highest line number
in the entire file, not necessarily the highest _rendered_ line number, as shown
in the updated notebook snapshot. still, it should get us closer to aligned than
not having an offset at all and also end up being correct in most(?) cases.
## 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
Fixes the remaining range reporting differences between the `ruff_db`
diagnostic rendering and Ruff's existing rendering, as noted in
https://github.com/astral-sh/ruff/pull/19415#issuecomment-3160525595.
This PR is structured as a series of three pairs. The first commit in
each pair adds a test showing the previous behavior, followed by a fix
and the updated snapshot. It's quite a small PR, but that might be
helpful just for the contrast.
You can also look at [this
range](052e656c6c..c3ea51030d)
of commits from #19415 to see the impact on real Ruff diagnostics. I
spun these commits out of that PR.
## Test Plan
New `ruff_db` tests
## Summary
This PR remaps ranges in Jupyter notebooks from simple `row:column`
indices in the concatenated source code to `cell:row:col` to match
Ruff's output. This is probably not a likely change to land upstream in
`annotate-snippets`, but I didn't see a good way around it.
The remapping logic is taken nearly verbatim from here:
cd6bf1457d/crates/ruff_linter/src/message/text.rs (L212-L222)
## Test Plan
New `full` rendering test for a notebook
I was mainly focused on Ruff, but in local tests this also works for ty:
```
error[invalid-assignment]: Object of type `Literal[1]` is not assignable to `str`
--> Untitled.ipynb:cell 1:3:1
|
1 | import math
2 |
3 | x: str = 1
| ^
|
info: rule `invalid-assignment` is enabled by default
error[invalid-assignment]: Object of type `Literal[1]` is not assignable to `str`
--> Untitled.ipynb:cell 2:3:1
|
1 | import math
2 |
3 | x: str = 1
| ^
|
info: rule `invalid-assignment` is enabled by default
```
This isn't a duplicate diagnostic, just an unimaginative example:
```py
# cell 1
import math
x: str = 1
# cell 2
import math
x: str = 1
```
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`).
It turns out that astral-sh/ty#18692 also fixedastral-sh/ty#203. This
PR adds a regression test for it. (Locally, I "unfixed" the bug and
confirmed that this is actually a regression test.)
Fixesastral-sh/ty#203
It turns out that `annotate-snippets` doesn't do a great job of
consistently handling tabs. The intent of the implementation is clearly
to expand tabs into 4 ASCII whitespace characters. But there are a few
places where the column computation wasn't taking this expansion into
account. In particular, the `unicode-width` crate returns `None` for a
`\t` input, and `annotate-snippets` would in turn treat this as either
zero columns or one column. Both are wrong.
In patching this, it caused one of the existing `annotate-snippets`
tests to fail. I spent a fair bit of time on it trying to fix it before
coming to the conclusion that the test itself was wrong. In particular,
the annotation ranges are 4 bytes off. However, when the range was
wrong, the buggy code was rendering the example as intended since `\t`
characters were treated as taking up zero columns of space. Now that
they are correctly computed as taking up 4 columns of space, the offsets
of the test needed to be adjusted.
Fixes#670
<!--
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>
<!--
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
This is a cleanup PR. I am fixing various English language spelling
errors. This is mostly in docs and docstrings.
## Test Plan
The usual CI tests were run. I tried to build the docs (though I had
some troubles there). The testing needs here are, I trust, very low
impact. (Though I would happily test more.)
## Summary
Found a comment that looks to be intended as docstring but accidentally
is just a normal comment.
Didn't create an issue as the readme said it's not neccessary for
trivial changes.
## Test Plan
<!-- How was it tested? -->
Can be tested by regenerating the docs.
Co-authored-by: Marcus Näslund <vidaochmarcus@gmail.com>
We do this because `...` is valid Python, which makes it pretty likely
that some line trimming will lead to ambiguous output. So we add support
for overriding the cut indicator. This also requires changing some of
the alignment math, which was previously tightly coupled to `...`.
For Ruff, we go with `…` (`U+2026 HORIZONTAL ELLIPSIS`) for our cut
indicator.
For more details, see the patch sent to upstream:
https://github.com/rust-lang/annotate-snippets-rs/pull/172
This fix was sent upstream and the PR description includes more details:
https://github.com/rust-lang/annotate-snippets-rs/pull/170
Without this fix, there was an errant snapshot diff that looked like
this:
|
1 | version = "0.1.0"
2 | # Ensure that the spans from toml handle utf-8 correctly
3 | authors = [
| ___________^
4 | | { name = "Z͑ͫ̓ͪ̂ͫ̽͏̴̙...A̴̵̜̰͔ͫ͗͢L̠ͨͧͩ͘G̴̻͈͍̑͗̎̅͛́Ǫ̵̹̻̝̳͂̌̌͘", email = 1 }
5 | | ]
| |_^ RUF200
|
That ellipsis should _not_ be inserted since the line is not actually
truncated. The handling of line length (in bytes versus actual rendered
length) wasn't quite being handled correctly in all cases.
With this fix, there's (correctly) no snapshot diff.
The change to the rendering code is elaborated on in more detail here,
where I attempted to upstream it:
https://github.com/rust-lang/annotate-snippets-rs/pull/169
Otherwise, the snapshot diff also shows a bug fix: a `^` is now rendered
where as it previously was not.
This is a tiny change that, perhaps slightly shady, permits us to use
the `annotate-snippets` renderer without its mandatory header (which
wasn't there in `annotate-snippets 0.9`). Specifically, we can now do
this:
Level::None.title("")
The combination of a "none" level and an empty label results in the
`annotate-snippets` header being skipped entirely. (Not even an empty
line is written.)
This is maybe not the right API for upstream `annotate-snippets`, but
it's very easy for us to do and unblocks the upgrade (albeit relying on
a vendored copy).
Ref https://github.com/rust-lang/annotate-snippets-rs/issues/167
This merely adds the crate to our repository. Some cosmetic changes are
made to make it work in our repo and follow our conventions, such as
changing the name to `ruff_annotate_snippets`. We retain the original
license information. We do drop some things, such as benchmarks, but
keep tests and examples.
