Move full diagnostic rendering to ruff_db

[ntBre]

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 #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.

ruff/crates/ruff_annotate_snippets/src/renderer/display_list.rs

Lines 1204 to 1208 in 529d81d

	assert!(
	snippet.source.is_empty(),
	"Non-empty file-level snippet that won't be rendered: {:?}",
	snippet.source
	);

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, 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

-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.

-    -: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:

-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:

-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

-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:

-__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.

-    Jupyter.ipynb:cell 3:1:7: F821 Undefined name `x`
+    F821 Undefined name `x`
+     --> Jupyter.ipynb:cell 3:1:7
       |
     1 | print(x)
-      |       ^ F821
+      |       ^
       |

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

[MichaReiser]

Thank you for looking into this.

I'm not sure I understand what the diff shown for Diagnostics with FixAvailability::Never or Sometimes has to do with the fix availability.

I think it would be more helpful to list the separate differences:

• The annotated message now shows the help text instead of the rule code (makes sense to me, but I haven't reviewed all snapshot changes)
• The line/column and filename moved: This makes sense to me and is a necessity to support multifile spans
• Full file diagnostics: @BurntSushi didn't you add some special casing for it to ruff's diagnostic rendering?

[MichaReiser]

These are the only ones I noticed that I wasn't very happy with. I tried to
address these by making the Range on Ruff's diagnostics None

We can't change the range or that would be breaking where suppression comments are allowed. We also can't remove the range or the diagnostic can no longer be suppressed.

What I don't understand in your example. Is the rendered code frame just empty? Does it collapse lines or does it render the full file?

[MichaReiser]

What I don't understand in your example. Is the rendered code frame just empty? Does it collapse lines or does it render the full file?

Okay, I think I understand now what's happening. The issue is that ruff uses empty ranges for file level diagnostics. I think the ideal output would be to only render the file name if both the range and message are empty.

We could decide to make Range optional but I think we should keep defaulting to an empty range and special case that in the rendering code. My reasoning is that the range is important to know where to place suppressions AND diagnostics like IO errors have truely no range and we don't want to render line column numbers at all.

but it still left a single blank line.

@BurntSushi might be able to help but you might have to bite the bullet and try to find the source of the empty line yourself.

It's also worth double checking if the new renderer handles these tricky cases

• ruff/crates/ruff_linter/src/message/text.rs

  Lines 374 to 407 in 77a5c5a

  	impl<'a> SourceCode<'a> {
  	/// This attempts to "fix up" the span on `SourceCode` in the case where
  	/// it's an empty span immediately following a line terminator.
  	///
  	/// At present, `annotate-snippets` (both upstream and our vendored copy)
  	/// will render annotations of such spans to point to the space immediately
  	/// following the previous line. But ideally, this should point to the space
  	/// immediately preceding the next line.
  	///
  	/// After attempting to fix `annotate-snippets` and giving up after a couple
  	/// hours, this routine takes a different tact: it adjusts the span to be
  	/// non-empty and it will cover the first codepoint of the following line.
  	/// This forces `annotate-snippets` to point to the right place.
  	///
  	/// See also: <https://github.com/astral-sh/ruff/issues/15509>
  	fn fix_up_empty_spans_after_line_terminator(self) -> SourceCode<'a> {
  	if !self.annotation_range.is_empty()
  	|| self.annotation_range.start() == TextSize::from(0)
  	|| self.annotation_range.start() >= self.text.text_len()
  	{
  	return self;
  	}
  	if self.text.as_bytes()[self.annotation_range.start().to_usize() - 1] != b'\n' {
  	return self;
  	}
  	let locator = Locator::new(&self.text);
  	let start = self.annotation_range.start();
  	let end = locator.ceil_char_boundary(start + TextSize::from(1));
  	SourceCode {
  	annotation_range: TextRange::new(start, end),
  	..self
  	}
  	}
  	}

• ruff/crates/ruff_linter/src/message/text.rs

  Line 287 in 9aaed4f

  .cut_indicator("…");

• ruff/crates/ruff_linter/src/message/text.rs

  Line 299 in 77a5c5a

  fn replace_whitespace_and_unprintable(source: &str, annotation_range: TextRange) -> SourceCode {

[ntBre]

I'm not sure I understand what the diff shown for Diagnostics with FixAvailability::Never or Sometimes has to do with the fix availability.

Oh sorry, I was using that as a proxy for having a fix_title method and thus help text. I'll update those sections with your suggestions.

I tried to address these by making the Range on Ruff's diagnostics None

We can't change the range or that would be breaking where suppression comments are allowed. We also can't remove the range or the diagnostic can no longer be suppressed.

Oops, this was really poorly phrased, but I think you still figured out what I meant in your last comment. Yes, we use TextRange::default as a marker for full-file diagnostics:

ruff/crates/ruff_linter/src/message/text.rs

Lines 119 to 121 in 6403eba

	if self.flags.intersects(EmitterFlags::SHOW_SOURCE) {
	// The `0..0` range is used to highlight file-level diagnostics.
	if message.expect_range() != TextRange::default() {

I tried making those None, not all Ruff ranges.

I think the ideal output would be to only render the file name if both the range and message are empty.

I totally agree. I got close to that, modulo the blank line I need to track down in annotate-snippets, at least I think it's in there. Concretely, I got output like this:

-    -:1:1: RUF901 [*] Hey this is a stable test rule with a safe fix.
-    -:1:1: RUF902 Hey this is a stable test rule with an unsafe fix.
+    [RUF901]: [*] Hey this is a stable test rule with a safe fix.
+
+    [RUF902]: Hey this is a stable test rule with an unsafe fix.
+

I think there was a filename, but maybe not. I'd need to figure that out too.

I don't think we're even passing anything on stdin in these tests, so the three-line code frame isn't even empty lines in the file, it's entirely synthetic.

Thanks for the tricky cases. They look similar to some of Andrew's other code I've run across in ruff_db so I kind of assume they're handled, but I'll double check!

[BurntSushi]

I looked through my commit history here, but couldn't find anything directly related to file-level diagnostics. There were some changes related to tweaking ranges and even one phantom line terminator (although this came from our code and not annotate-snippets). The closest I could find were the following commits:

• 5021f32
• 2ff2a54
• c9b99e4
• 602a27c
• 2ca2f73

[ntBre]

I think I'm getting pretty close to having the output we want, albeit with a potentially hacky implementation. For this input:

import math

x: str = 1

match 42:
    case _: ...

and this shell command:

git diff <(ruff check --no-cache tr{i,y}.py --target-version py39) \
         <(myruff check --no-cache tr{i,y}.py --target-version py39)

I'm currently getting this diff:

-tri.py:1:1: E902 No such file or directory (os error 2)
-try.py:1:8: F401 [*] `math` imported but unused
+E902 No such file or directory (os error 2)
+
+F401 [*] `math` imported but unused
+ --> try.py:1:8
   |
 1 | import math
-  |        ^^^^ F401
+  |        ^^^^
 2 |
 3 | x: str = 1
   |
-  = help: Remove unused import: `math`
+help: Remove unused import: `math`
 
-try.py:5:1: SyntaxError: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10)
+SyntaxError: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10)
+ --> try.py:5:1
   |
 3 | x: str = 1
 4 |

This all looks good to me except the missing filename for the full-file diagnostic E902. I also tried a bit to get rid of the extra newline without much luck. I think the last two commits Andrew linked are in exactly the right place, but reverting to the version in 602a27c removes all the lines between diagnostics. I think I've convinced myself that it's actually nice to have the "extra" line for E902 since that's consistent with other diagnostics, but maybe I'm just giving up too easily.

Another issue with the check I added to remove the empty annotations is that some TextRange::default ranges are intentional, I think. For example, this snapshot changed:

ruff/crates/ty_ide/src/goto_declaration.rs

Lines 289 to 294 in dc10ab8

	info[goto-declaration]: Declaration
	--> mymodule.py:1:1
	|
	1 |
	| ^
	2 | def function():

when I think it might want to point to the beginning of the file, with a real annotation. I002 is another example of a diagnostic that might actually want a default range and changed in my recent commits:

ruff/crates/ruff_linter/src/rules/isort/snapshots/ruff_linter__rules__isort__tests__required_import_with_alias_off.py.snap

Lines 5 to 8 in dc10ab8

	off.py:1:1: I002 [*] Missing required import: `from __future__ import annotations as _annotations`
	ℹ Safe fix
	1 1 | # isort: off
	2 |+from __future__ import annotations as _annotations

I'll keep looking at these remaining issues tomorrow, but at least I made some progress on the brackets and blank lines (maybe) today.

[MichaReiser]

This all looks good to me except the missing filename for the full-file diagnostic E902.

I think that actually looks fine. Have you tried how full renders diagnostics without an annotation? Does it add an empty line between them too (which I think would make sense)? Or is the extra line something that isn't visible in that diff?

Regarding the full range diagnostics. This is obviously something I'd like @BurntSushi's input on before changing but I wonder if the problem here really is that the primary annotation range does:

1. Set the span for the line/column rendering and where the diagnostic can be suppressed
2. Enable code span rendering, even if the message is empty (very common)

Ideally, the diagnostic model would allow a rule to set a span for 1 but without enabling 2.

The first idea that comes to mind is to add a span to Diagnostic itself. I believe the issue is that the file name rendering is tied to the annotation. Which makes sense, because different annotations could point to different files. That means we would only render the file name if no annotations are present. However, this opens us up to the risk that the primary_annotation and the diagnostic.range could point to different files, in which case omitting the diagnostic range's file name could be confusing. But that's an invariant that we could enforce with assertions.

An alternative is to mark a primary annotation to say, nope, only render the file name but no message or annotation. This does seem a bit weird given that that's exactly what an annotation is for but again, something we could enforce with an assertion somewhere.

I think the biggest challenge here might is probably that annotation snippet doesn't allow rendering the file name without a "snippet". We could probably work around this by simply copy pasting the file name rendering.

What's unclear to me from a UX perspective if the following is confusing:

E902 No such file or directory (os error 2)
--> try.py:1:8

F401 [*] `math` imported but unused
--> try.py:1:8
  |
1 | import math
  |        ^^^^
2 |
3 | x: str = 1
  |
help: Remove unused import: `math`

[ntBre]

Have you tried how full renders diagnostics without an annotation? Does it add an empty line between them too (which I think would make sense)? Or is the extra line something that isn't visible in that diff?

What do you mean without an annotation? I think in a literal, but maybe unhelpful, sense all Ruff diagnostics have annotations. Here's the full output of the ruff command above with a couple of extra non-existent files, if that helps:

E902 No such file or directory (os error 2)

E902 No such file or directory (os error 2)

E902 No such file or directory (os error 2)

F401 [*] `math` imported but unused
 --> /tmp/tmp.gqRJ0jMRZ9/try.py:1:8
  |
1 | import math
  |        ^^^^
2 |
3 | x: str = 1
  |
help: Remove unused import: `math`

SyntaxError: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10)
 --> /tmp/tmp.gqRJ0jMRZ9/try.py:5:1
  |
3 | x: str = 1
4 |
5 | match 42:
  | ^^^^^
6 |     case _: ...
  |

Found 5 errors.
[*] 1 fixable with the `--fix` option.

Regarding the full range diagnostics. This is obviously something I'd like @BurntSushi's input on before changing but I wonder if the problem here really is that the primary annotation range does:

1. Set the span for the line/column rendering and where the diagnostic can be suppressed
2. Enable code span rendering, even if the message is empty (very common)

Ideally, the diagnostic model would allow a rule to set a span for 1 but without enabling 2.

I think that's a very good summary of the problem. And thanks for the other ideas, those seem helpful to explore.

What's unclear to me from a UX perspective if the following is confusing:

E902 No such file or directory (os error 2)
--> try.py:1:8

F401 [*] `math` imported but unused
--> try.py:1:8
  |
1 | import math
  |        ^^^^
2 |
3 | x: str = 1
  |
help: Remove unused import: `math`

Yeah, I do like Ruff's current rendering here, basically collapsing back to concise rendering for E902:

tri.py:1:1: E902 No such file or directory (os error 2)
try.py:1:8: F401 [*] `math` imported but unused
  |
1 | import math
  |        ^^^^ F401
2 |
3 | x: str = 1
  |
  = help: Remove unused import: `math`

But that's also consistent with its full output. I'm not sure if mixing the two is confusing too:

tri.py:1:1: E902 No such file or directory (os error 2)
F401 [*] `math` imported but unused
 --> try.py:1:8
  |
1 | import math
  |        ^^^^
2 |
3 | x: str = 1
  |
help: Remove unused import: `math`

Ah, it looks like rustc just embeds the filename in the error message when there's no annotation:

$ rustc fake.rs
error: couldn't read fake.rs: No such file or directory (os error 2)

error: aborting due to 1 previous error

But then we'd have to modify the message on E902 and any other rule like it.

[BurntSushi]

I think adding a knob on to Annotation that lets you suppress code snippets is probably the better choice. This "feels" better to me than trying to put a range on the diagnostic itself, which I think could be quite confusing. (Micha noted this.) While true we could enforce this with assertions, it feels like a more complicated change to the diagnostic model than just letting an annotation be "light" in the sense that there are no code snippets.

I do agree that this makes the name "annotation" somewhat of a misnomer in certain cases. Although, you could recast as, "it's still an annotation, but it applies to contents that are too large to usefully display in an error message."

[ntBre]

We've got filenames!

-tri.py:1:1: E902 No such file or directory (os error 2)
-try.py:1:8: F401 [*] `math` imported but unused
+E902 No such file or directory (os error 2)
+--> tri.py:1:1
+
+F401 [*] `math` imported but unused
+ --> try.py:1:8
   |
 1 | import math
-  |        ^^^^ F401
+  |        ^^^^
 2 |
 3 | x: str = 1
   |
-  = help: Remove unused import: `math`
+help: Remove unused import: `math`

-try.py:5:1: SyntaxError: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10)
+SyntaxError: Cannot use `match` statement on Python 3.9 (syntax was added in Python 3.10)
+ --> try.py:5:1
   |
 3 | x: str = 1
 4 |

Plumbing this setting through the Annotations felt like a nice solution, thank you both for that suggestion! Now we can also set this only in Ruff and avoid the ty snapshot changes I was worried about yesterday. I didn't do this now, but we could also use this on a per-diagnostic basis in Ruff for those rules that happen to point to the start of the file but still want a snippet rendered, unlike E902.

I think the diagnostic output is where I want it now. I'll look over the implementation again and hopefully open it for review later today!

[ntBre]

I think the implementation looks okay. It's mostly just passing flags (that might need better names!) into annotate-snippets and some touchy formatting changes based on them.

I also looked closely at the tricky cases Micha mentioned above. The cut indicator ellipsis was easy thanks to #19420, I just had to copy that over to the new file. That also reverted the remaining snapshot changes in ty crates.

For the SourceCode case, I was able to copy over the code from ruff_linter and integrate it pretty smoothly. We can almost delete the version I copied from, getting back down to one copy, but that code is still used by the grouped output format. I guess we could at least export ceil_char_boundary from ruff_db if we want to avoid the duplication. Hopefully it will get stabilized one day and we can delete both copies, though. This duplication was kind of forcefully resolved by it needing to be pub for the doctests.

I'm a bit stuck on where to inject the replace_whitespace_and_unprintable call. We definitely need it because the snapshot range fixed in e6e610c has regressed. The DiagnosticSource makes it trickier to access and modify the source text directly, at least in ResolvedAnnotation::new where I added the ceil_char_boundary stuff. It might make more sense to add both checks in RenderableSnippet::new, at least that's the other candidate that jumped out to me. I'll keep thinking about this tonight.

All of the code changes except the line terminator fix in the last commit are in this range. I'll update the summary before marking this ready for review.

[MichaReiser]

Would it make sense to split this pr into 1) extending the rendering and b) migrating ruff?

Or can you organize the commits in a way that makes reviewing easier

[MichaReiser]

I didn't review all snapshot changes but this looks great. Thank you for patiently rebasing this PR and splitting out your fixes into smaller PRs!

[MichaReiser]

If you haven't done so already. Can you double check if the diff rendering still looks good?

[ntBre]

This is the diff rendering that only works in the snapshots (#7352). I didn't notice any changes in those sections of the snapshots, as expected, but is there something else you want me to check here?

[MichaReiser]

Oh, I thought it's the CLI's --diff mode. Also, is there any existing code that we can delete, now that we moved the diagnostic rendering (e.g. MessageCodeFrame)?

[ntBre]

No the --diff flag is separate and looks good:

> ruff check try.py --diff
--- try.py
+++ try.py
@@ -1 +0,0 @@
-import math

Would fix 1 error.
> myruff check try.py --diff
--- try.py
+++ try.py
@@ -1 +0,0 @@
-import math

Would fix 1 error.

We can't quite delete the TextEmitter and the MessageCodeFrame rendering because it's still used by the grouped format. Once we move that over, we can delete a big chunk of code.

[MichaReiser]

It seems that GroupEmitter::with_show_source is only called in tests. So we could just delete it? But I also don't mind if you do this in a separate PR

[ntBre]

Oh you're right, I forgot about that one. I'll check again for unused code and follow up!

[MichaReiser]

Sounds good. I'm fine merging this PR and follow up in a separate PR.

[ntBre]

This should be ready for review. I left the commits towards the end named as if they resolve the carriage return, BOM, and EOF issues, but after #19806 they just accept the snapshots from the actual code changes in the other PR.

I also wrote a much shorter AWK version of Claude's script for checking the offsets in the diff. It has three false positives, but I verified that they are just due to git displaying a larger diff in those areas, not real differences in row:column reporting.

Script and output

#!/usr/bin/awk -f

{
    # Old line like:
    # -    -:1:8: F401 [*] `os` imported but unused
    if (match($0, /-.*:([0-9]+):([0-9]+):/, m)) {
        row = m[1]
        col = m[2]
        line = $0
    }
    # New line like:
    # +    F401 [*] `os` imported but unused
    else if (match($0, /+.*-->.*:([0-9]+):([0-9]+)/, m)) {
        new_row = m[1]
        new_col = m[2]
        if (new_row != row || new_col != col) {
            print "MISMATCH:"
            printf "\tOld line: %s\n", line
            printf "\tNew line: %s\n", $0
        }
    }
}

These are false positives due to how git reports the diff for these files.

$ git diff main | ./check_offsets.awk
MISMATCH:
        Old line: -E501_1.py:5:89: E501 Line too long (150 > 88)
        New line: + --> E501_1.py:4:89
MISMATCH:
        Old line: -E501_1.py:6:89: E501 Line too long (149 > 88)
        New line: + --> E501_1.py:5:89
MISMATCH:
        Old line: -all.py:1:5: D103 Missing docstring in public function
        New line: +--> all.py:1:1

For example:

-all.py:1:1: D100 Missing docstring in public module
-all.py:1:5: D103 Missing docstring in public function
+D100 Missing docstring in public module
+--> all.py:1:1
+
+D103 Missing docstring in public function
+ --> all.py:1:5

The E501 case is similar. Basically all of the E501 diagnostics for the file are jumbled together in the diff because the long-line truncation changes too, but all of the individual diagnostics are still there and reported with the correct line and column numbers.

[BurntSushi]

LGTM!