Convert `Message::SyntaxError` to use `Diagnostic` internally by ntBre · Pull Request #17784 · astral-sh/ruff

ntBre · 2025-05-01T23:51:16Z

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 DiagnosticMessages will be most of the work in replacing Message entirely.

Test Plan

Existing tests, which show no changes.

github-actions · 2025-05-01T23:53:50Z

`mypy_primer` results

No ecosystem changes detected ✅

github-actions · 2025-05-02T00:06:18Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

mesonbuild/meson-python (error)

warning: Detected debug build without --no-cache.
error: Failed to read tests/packages/symlinks/baz.py: No such file or directory (os error 2)
error: Failed to read tests/packages/symlinks/qux.py: No such file or directory (os error 2)

Formatter (preview)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

mesonbuild/meson-python (error)

ruff format --preview

warning: Detected debug build without --no-cache.
error: Failed to read tests/packages/symlinks/baz.py: No such file or directory (os error 2)
error: Failed to read tests/packages/symlinks/qux.py: No such file or directory (os error 2)

BurntSushi · 2025-05-02T13:27:27Z

crates/ruff_db/src/diagnostic/mod.rs

+            UnifiedFile::RedKnot(file) => resolver.path(*file),
+            UnifiedFile::Ruff(file) => file.name(),
+        }
+    }


FWIW, I think the main intent with FileResolver is that it would become a trait, and this would be how we'd handle Ruff using its own interning for files. i.e., Red Knot would pass its own Salsa-backed resolver, but Ruff would use its non-Salsa thing.

I think if you aren't doing the interning, then yeah, this API is probably fine and perhaps even slightly over-engineered. If you do still plan to do interning---and I think that will let you definitively make UnifiedFile and therefore Span implement Copy---then I think this is a good stepping stone toward that.

I find having both path and file_path on UnifiedFile confusing because the only difference is whether they take a db or FileResolver, but that's not apparent from the method signature.

One option is to make FileResolver a trait as @BurntSushi suggests and implement it for Db (and Upcast). We would then need to implement a similar struct for Ruff.

Or we remove file_path (and all other methods that take db) and callers should instead call expect_ty and the path on the returned file. The renderer could call resolver.path(file) directly and we move this match into resolver.path. I think I sort of prefer this.

I don't feel too strongly here primarily because path() is an internal API, so I think the blast radius of confusion is somewhat more limited.

I'm happy with either of @MichaReiser's suggestions though. I kinda like just jumping straight to the trait approach, which was the original intent.

Is this close to what y'all had in mind? 1faa5e0

I feel like I may have gone astray here in the details, but FileResolver is now a trait, the code compiles, and I got rid of file_path, at least.

The docs on FileResolver are probably slightly outdated now, but they still sounded relevant enough to me to leave alone since we're not really fully decoupled from salsa yet.

Yeah that looks about right to me! Thanks!

I'd probably go for dyn Trait here though since I don't think the virtual call is going to matter perf-wise, and it will hopefully save from duplicating the rendering code.

crates/ruff_linter/src/message/mod.rs

add RuffFile with a cached SourceText Revert "add RuffFile with a cached SourceText" This reverts commit 315e834eea6f34a2e1909bdc75e5f505c4597f85.

ntBre · 2025-05-06T17:57:18Z

Okay, I think this is ready for review. The two main changes since y'all last had a look are:

Removing the macros
I added a somewhat overly-specific Diagnostic::expect_ruff_span method that just unwraps a primary_span call but with a ruff-centric message. I think this should eventually go away when we actually use Diagnostic for output, so hopefully it's okay temporarily. Otherwise I just inlined the macros. They were mostly just to consolidate expect messages.
Removing the repeated Notebook parsing
I added a comment to this effect in the code, but Ruff already does its own notebook handling when Messages are emitted. Thanks @MichaReiser for the suggestion there!

Co-authored-by: Carl Meyer <carl@astral.sh>

Co-authored-by: Micha Reiser <micha@reiser.io>

ntBre · 2025-05-07T13:57:32Z

I went ahead and pushed the minor changes, working on the SourceText and FileResolver stuff next.

BurntSushi

Looking pretty good!

BurntSushi · 2025-05-07T14:04:15Z

crates/ruff_linter/src/message/mod.rs

 }

 impl Message {
+    pub fn syntax_error(message: String, range: TextRange, file: SourceFile) -> Message {


It might be a little nicer to accept a impl std::fmt::Display here (which is what most of the Diagnostic APIs accept, at least, in spirit). Then you could avoid the various err.to_string() calls and just pass err.

BurntSushi · 2025-05-07T14:06:23Z

crates/ruff_db/src/diagnostic/mod.rs

        self.primary_annotation().map(|ann| ann.tags.as_slice())
    }
+
+    pub fn expect_span(&self) -> Span {


Can you add a comment documenting this? In particular, it would be good to state the panic conditions and the higher level motivation for why you might want to use this. (e.g., "When working with diagnostics in Ruff, the context requires that every diagnostic have a primary span.")

I think I'd also rename this to expect_primary_span() so that it's consistent with primary_span().

BurntSushi · 2025-05-07T14:07:31Z

crates/ruff_db/src/diagnostic/mod.rs

+pub enum UnifiedFile {
+    Ty(File),
+    Ruff(SourceFile),
+}


Can you add docs to this type? This might be a nice place to put some prose about why this exists and the high level strategy for how we're trying to share the diagnostics system between both ty and ruff.

Nice, much better!

BurntSushi · 2025-05-07T14:15:37Z

crates/ruff_db/src/diagnostic/mod.rs

+            UnifiedFile::RedKnot(file) => resolver.path(*file),
+            UnifiedFile::Ruff(file) => file.name(),
+        }
+    }


I don't feel too strongly here primarily because path() is an internal API, so I think the blast radius of confusion is somewhat more limited.

I'm happy with either of @MichaReiser's suggestions though. I kinda like just jumping straight to the trait approach, which was the original intent.

BurntSushi · 2025-05-07T14:34:31Z

crates/ruff_db/src/source.rs

 #[derive(Clone, Eq, PartialEq)]
 pub struct SourceText {
-    inner: Arc<SourceTextInner>,
+    pub(crate) inner: Arc<SourceTextInner>,


I think using just SourceCode might be a little tricky here, because you need somewhere to anchor its lifetimes to.

I do agree that reusing SourceText here is a little wonky.

Maybe the way to go here is another enum:

enum DiagnosticSource { Ty(SourceText), Ruff(SourceFile), )

And then you should be able to define a method to return a SourceCode I think.

MichaReiser · 2025-05-08T07:53:58Z

@ntBre can you ping me when this is ready for re-review (it is already)?

ntBre · 2025-05-08T12:43:46Z

It should be ready for another review!

BurntSushi

LGTM!

BurntSushi · 2025-05-08T13:19:07Z

crates/ruff_db/src/diagnostic/mod.rs

+pub enum UnifiedFile {
+    Ty(File),
+    Ruff(SourceFile),
+}


Nice, much better!

BurntSushi · 2025-05-08T13:19:33Z

crates/ruff_db/src/diagnostic/mod.rs

+enum DiagnosticSource {
+    Ty(Input),
+    Ruff(SourceFile),
+}


Nice docs. And I like this type. Much clearer.

MichaReiser

This is excellent. Thank you

ntBre · 2025-05-08T16:45:03Z

Thank you both!

* main: [ty] Respect the gradual guarantee when reporting errors in resolving MROs (#17962) Convert `Message::SyntaxError` to use `Diagnostic` internally (#17784) [ty] Support extending `__all__` with a literal tuple or set as well as a literal list (#17948) [ty] Make `unused-ignore-comment` disabled by default for now (#17955) [ty] Change default severity for `unbound-reference` to `error` (#17936) [ty] Ignore `possibly-unresolved-reference` by default (#17934) [ty] Default to latest supported python version (#17938) [ty] Generate and add rules table (#17953) Update the schemastore script to match changes in ty (#17952) [ty] Implement `global` handling and `load-before-global-declaration` syntax error (#17637)

* dcreager/default-typevars: clean up the diff remove trait track in type again clippy Better expansion of default typevars specialize_partial enum for TypeMapping [ty] Respect the gradual guarantee when reporting errors in resolving MROs (#17962) Specialize trait Convert `Message::SyntaxError` to use `Diagnostic` internally (#17784) [ty] Support extending `__all__` with a literal tuple or set as well as a literal list (#17948) [ty] Make `unused-ignore-comment` disabled by default for now (#17955) [ty] Change default severity for `unbound-reference` to `error` (#17936) [ty] Ignore `possibly-unresolved-reference` by default (#17934) [ty] Default to latest supported python version (#17938) [ty] Generate and add rules table (#17953) Update the schemastore script to match changes in ty (#17952) [ty] Implement `global` handling and `load-before-global-declaration` syntax error (#17637)

ntBre force-pushed the brent/diagnostics-file-enum branch from 4c2da7c to 4fce96a Compare May 1, 2025 23:56

BurntSushi reviewed May 2, 2025

View reviewed changes

AlexWaygood added the diagnostics Related to reporting of diagnostics. label May 3, 2025

ntBre added 17 commits May 6, 2025 11:23

rough draft of FileWrapper enum that might work for ruff

5048108

use SourceFile and names from the proposal

2f01da5

add NewDiagnostic wrapper

3a04278

add unused NewDiagnostic::Diagnostic variant

3c5065d

convert syntax errors to new diagnostics

0e1a827

fix wasm errors

122e787

handle notebook todos

1559c29

delete Message::SyntaxError variant

9b50816

move constructors to NewDiagnostic

f717a6b

delete SyntaxErrorMessage

70f47bf

absorb Message into NewDiagnostic

26c2009

rename NewDiagnostic back to Message to clarify diff

97bf7f3

fix ordering to minimize diff

17753cb

RedKnot -> Ty, expect_file -> expect_ty

007aa71

add Diagnostic::expect_ruff_span, inline macros

7f59117

add RuffFile with a cached SourceText Revert "add RuffFile with a cached SourceText" This reverts commit 315e834eea6f34a2e1909bdc75e5f505c4597f85.

delete outdated/obvious todos

056d369

remove notebook parsing

f11b040

ntBre force-pushed the brent/diagnostics-file-enum branch from 4fce96a to f11b040 Compare May 6, 2025 17:45

ntBre marked this pull request as ready for review May 6, 2025 17:57

ntBre requested review from AlexWaygood, MichaReiser, carljm, dcreager and sharkdp as code owners May 6, 2025 17:57

ntBre and others added 5 commits May 7, 2025 09:20

red-knot -> ty

e9f2c36

Co-authored-by: Carl Meyer <carl@astral.sh>

UnifiedFile::expect_ty -> Span::expect_ty_file

6b6a45b

restore ty.rs with expect_ty_file

eb1e332

rename expect_ruff_span, add expect_ruff_file

88ae57e

clean up unnecessary lifetimes

57581f2

Co-authored-by: Micha Reiser <micha@reiser.io>

clean up extra blank line

53eb330

BurntSushi reviewed May 7, 2025

View reviewed changes

ntBre added 11 commits May 7, 2025 10:39

use expect_ty_file to revert test-specific methods

f22a1ad

pass impl Display to syntax_error

a77d9fb

expect_span -> expect_primary_span and add docs

3f8ccd9

add some docs to UnifiedFile

e76d2ec

add DiagnosticSource instead of reusing SourceText

60e18ad

convert FileResolver to a trait

1faa5e0

Merge branch 'main' into brent/diagnostics-file-enum

561f2ac

clean up remaining input -> diagnostic_source renames

e29eb48

fix docs link

33b75e6

switch to dyn FileResolver

8614e56

Clean up remaining pub(crate)s on SourceTextInner

3471834

ntBre requested review from BurntSushi and MichaReiser May 8, 2025 12:43

BurntSushi approved these changes May 8, 2025

View reviewed changes

MichaReiser approved these changes May 8, 2025

View reviewed changes

ntBre merged commit 981bd70 into main May 8, 2025
35 checks passed

ntBre deleted the brent/diagnostics-file-enum branch May 8, 2025 16:45

Conversation

ntBre commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

github-actions bot commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

mypy_primer results

Uh oh!

github-actions bot commented May 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ntBre commented May 6, 2025

Uh oh!

ntBre commented May 7, 2025

Uh oh!

BurntSushi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaReiser commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ntBre commented May 8, 2025

Uh oh!

BurntSushi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

ntBre commented May 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ntBre commented May 1, 2025 •

edited

Loading

github-actions bot commented May 1, 2025 •

edited

Loading

`mypy_primer` results

github-actions bot commented May 2, 2025 •

edited

Loading

`ruff-ecosystem` results

MichaReiser commented May 8, 2025 •

edited

Loading