[red-knot] ruff_db: make diagnostic rendering prettier #15856

BurntSushi · 2025-01-31T16:49:57Z

This change does a simple swap of the existing renderer for one that
uses our vendored copy of annotate-snippets. We don't change anything
about the diagnostic data model, but this alone already makes
diagnostics look a lot nicer!

Here's an example of what it looks like now:

github-actions · 2025-01-31T16:59:00Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

AlexWaygood

This looks fantastic to me!

carljm

So good!

carljm · 2025-01-31T18:06:48Z

crates/red_knot/tests/cli.rs

+      |
+    1 |
+    2 | y = 4 / 0
+      |     ----- Cannot divide object of type `Literal[4]` by zero


Ok really dumb question, but why does annotate-snippets choose ^^^^ in some cases but ---- here?

Good question actually! It chooses a different mark based on the severity level:

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

Lines 716 to 723 in b0b8b06

let mark = match annotation.annotation_type {

DisplayAnnotationType::Error => '^',

DisplayAnnotationType::Warning => '-',

DisplayAnnotationType::Info => '-',

DisplayAnnotationType::Note => '-',

DisplayAnnotationType::Help => '-',

DisplayAnnotationType::None => ' ',

};

Honestly kinda meh on this myself since it's somewhat non-obvious to me.

I think it makes more sense once you have e.g. (multiple) "Info" and "Error" annotations in a single diagnostic. Example from my own project (thinking about it, the function parameter annotation on top here should probably also be "info", but anyway):

I kind of like how the informational parts are just underlined with --- and the actual error is underlined with ^^^ as if to say: look, this is what you need to change.

Ah yeah that's a good point to consider too. I'll leave it as-is for now. This was just meant to get some decent rendering in. We can absolutely iterate on the exact format and what not.

carljm · 2025-01-31T18:32:52Z

Oh, you're gonna need to update the output-validation in our benchmark somehow...

BurntSushi · 2025-01-31T18:35:53Z

Yeah working on that hah. Trying insta.

codspeed-hq · 2025-01-31T19:48:09Z

CodSpeed Performance Report

Merging #15856 will not alter performance

_{Comparing ag/red-knot-prototype-diagnostics (07c80e8) with main (fab86de)}

Summary

✅ 32 untouched benchmarks

carljm · 2025-01-31T20:07:45Z

crates/ruff_benchmark/benches/red_knot.rs

+    // We disable colors here so that our diagnostics don't include
+    // ANSI escape codes.
+    //
+    // NOTE: I kinda feel like this is a sub-optimal way of going about
+    // this. It just feels bad to be reaching in and doing global
+    // state. I feel like it would be better to have a more explicit
+    // configuration step before emitting diagnostics, but I think that
+    // requires more design.
+    colored::control::set_override(false);

-    assert_eq!(&normalized, EXPECTED_DIAGNOSTICS);
+    for (diagnostic, &expected) in diagnostics.into_iter().zip(EXPECTED_DIAGNOSTICS) {
+        let got = diagnostic
+            .display(db.upcast())
+            .to_string()
+            .replace('\\', "/");
+        assert_eq!(got, expected);


Given that we have the diagnostic data structures here, why should we render them using display at all, now that that representation is so inconvienent for comparison and update? (Not that it was ever convenient.) Couldn't we just compare against, like, a tuple of the key fields instead?

Yeah I'll look into this. I was mostly just looking to do the most minimal thing possible, but yeah, if you're cool with switching this out to just doing a comparison of key fields (range and diagnostic id I guess) then that seems fine.

Yeah that seems definitely better to me -- the full diagnostic strings look like they'd be an absolute nightmare to update without help from insta.

carljm · 2025-01-31T20:17:15Z

crates/ruff_benchmark/benches/red_knot.rs

+    // we just check that the number of diagnostics reported is what we
+    // expect, to avoid doing most costly validation.
+    let result: Vec<_> = case.db.check().unwrap();
+    assert_diagnostics(&case.db, result);


On second thought, I think the bogus codspeed numbers are telling us why we can't make this change. It turns the "cold" benchmark into a "no-op incremental" benchmark, which, not surprisingly, is very fast indeed. That's probably why the full diagnostics assertion was included in the benchmark. I think we either need to put it back the way it was, or do it as a totally separate prior step, not part of the per-iteration setup_case at all.

Aye. I'll switch it back. I guess it should be fine now since I'm not using insta any more. Although I guess the diagnostic strings are now longer, so comparisons will be longer.

If we go with key fields instead of the full string, it should be pretty similar to before?

Though doing it as a separate step (not in setup_case) also seems pretty easy and fine.

Yeah sorry I made that comment before the other one.

carljm · 2025-01-31T21:14:36Z

crates/ruff_benchmark/benches/red_knot.rs

+/// the actual diagnostic output, but using `insta` inside benchmarks is
+/// problematic, and updating the strings otherwise when diagnostic rendering
+/// changes is a PITA.
+type KeyDiagnosticFields = (


I think we may want .message() string in here too, but I'm not sure how much difference it will make going forward, and we can always add it back in as a separate change.

Done!

Can you say more about why .message() is desirable here? Do you mean it's desirable to test and validate, or do you mean it's desirable from a measurement perspective?

It's desirable from a usability perspective of validating in PR review that the diagnostics removed from (or added to) this assertion list are sensibly related to the red-knot changes.

Ah I think I understand now. Thank you for explaining!

This change does a simple swap of the existing renderer for one that uses our vendored copy of `annotate-snippets`. We don't change anything about the diagnostic data model, but this alone already makes diagnostics look a lot nicer!

MichaReiser · 2025-02-01T08:52:51Z

crates/red_knot/tests/cli.rs

+     --> <temp_dir>/child/test.py:2:1
+      |
+    2 | from utils import add
+      | ^^^^^^^^^^^^^^^^^^^^^ Cannot resolve import `utils`


This demonstrates that we should start capturing at least some full diagnostics in mdtests because highlighting the entire range here seems incorrect (compared to the import utils case below where it only highlights the name. I'll create an issue

Yeah I agree. We need a fair bit of test coverage beyond what we have. I think a nice short-term target would be at least one test for every type of diagnostic.

I agree that we need a lot more tests that capture the full diagnostic output, but I'm not convinced this should necessarily be an mdtest feature (#15867 (comment))

BurntSushi requested review from carljm, MichaReiser, AlexWaygood and sharkdp as code owners January 31, 2025 16:49

BurntSushi changed the title ~~ruff_db: make diagnostic rendering prettier~~ [red-knot] ruff_db: make diagnostic rendering prettier Jan 31, 2025

AlexWaygood added the red-knot Multi-file analysis & type inference label Jan 31, 2025

AlexWaygood approved these changes Jan 31, 2025

View reviewed changes

BurntSushi force-pushed the ag/red-knot-prototype-diagnostics branch from 27869d0 to f292f01 Compare January 31, 2025 18:04

carljm approved these changes Jan 31, 2025

View reviewed changes

BurntSushi force-pushed the ag/red-knot-prototype-diagnostics branch 4 times, most recently from 2fc4a27 to 5da4fb6 Compare January 31, 2025 19:42

BurntSushi force-pushed the ag/red-knot-prototype-diagnostics branch from 5da4fb6 to 1080155 Compare January 31, 2025 19:50

carljm reviewed Jan 31, 2025

View reviewed changes

BurntSushi force-pushed the ag/red-knot-prototype-diagnostics branch from 1080155 to 88a0707 Compare January 31, 2025 21:03

BurntSushi requested a review from carljm January 31, 2025 21:10

carljm approved these changes Jan 31, 2025

View reviewed changes

ruff_db: make diagnostic rendering prettier

07c80e8

This change does a simple swap of the existing renderer for one that uses our vendored copy of `annotate-snippets`. We don't change anything about the diagnostic data model, but this alone already makes diagnostics look a lot nicer!

BurntSushi force-pushed the ag/red-knot-prototype-diagnostics branch from 88a0707 to 07c80e8 Compare January 31, 2025 21:21

BurntSushi merged commit b58f2c3 into main Jan 31, 2025
21 checks passed

BurntSushi deleted the ag/red-knot-prototype-diagnostics branch January 31, 2025 21:37

MichaReiser reviewed Feb 1, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[red-knot] ruff_db: make diagnostic rendering prettier #15856

[red-knot] ruff_db: make diagnostic rendering prettier #15856

BurntSushi commented Jan 31, 2025 •

edited

Loading

github-actions bot commented Jan 31, 2025 •

edited

Loading

AlexWaygood left a comment

carljm left a comment

carljm Jan 31, 2025

BurntSushi Jan 31, 2025

sharkdp Jan 31, 2025 •

edited

Loading

BurntSushi Jan 31, 2025

carljm commented Jan 31, 2025

BurntSushi commented Jan 31, 2025

codspeed-hq bot commented Jan 31, 2025 •

edited

Loading

carljm Jan 31, 2025

BurntSushi Jan 31, 2025

carljm Jan 31, 2025

carljm Jan 31, 2025 •

edited

Loading

BurntSushi Jan 31, 2025

carljm Jan 31, 2025

carljm Jan 31, 2025

BurntSushi Jan 31, 2025

carljm Jan 31, 2025

BurntSushi Jan 31, 2025

carljm Jan 31, 2025

BurntSushi Feb 1, 2025

MichaReiser Feb 1, 2025

BurntSushi Feb 1, 2025

AlexWaygood Feb 1, 2025

	let mark = match annotation.annotation_type {
	DisplayAnnotationType::Error => '^',
	DisplayAnnotationType::Warning => '-',
	DisplayAnnotationType::Info => '-',
	DisplayAnnotationType::Note => '-',
	DisplayAnnotationType::Help => '-',
	DisplayAnnotationType::None => ' ',
	};

[red-knot] ruff_db: make diagnostic rendering prettier #15856

[red-knot] ruff_db: make diagnostic rendering prettier #15856

Conversation

BurntSushi commented Jan 31, 2025 • edited Loading

github-actions bot commented Jan 31, 2025 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

AlexWaygood left a comment

Choose a reason for hiding this comment

carljm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharkdp Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carljm commented Jan 31, 2025

BurntSushi commented Jan 31, 2025

codspeed-hq bot commented Jan 31, 2025 • edited Loading

Merging #15856 will not alter performance

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carljm Jan 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BurntSushi commented Jan 31, 2025 •

edited

Loading

github-actions bot commented Jan 31, 2025 •

edited

Loading

`ruff-ecosystem` results

sharkdp Jan 31, 2025 •

edited

Loading

codspeed-hq bot commented Jan 31, 2025 •

edited

Loading

carljm Jan 31, 2025 •

edited

Loading