add diagnostic `Span` (couples `File` and `TextRange`) by BurntSushi · Pull Request #16101 · astral-sh/ruff

BurntSushi · 2025-02-11T18:02:31Z

This essentially makes it impossible to construct a Diagnostic
that has a TextRange but no File.

This is meant to be a precursor to multi-span support.

(Note that I consider this more of a prototyping-change and not
necessarily what this is going to look like longer term.)

Reviewers can probably review this PR as one big diff instead of
commit-by-commit.

github-actions · 2025-02-11T18:09:05Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

MichaReiser

Nice: The only thing that's unclear to me is whether we want to store Spans directly on the concrete Diagnostic types or if we want to construct the span ad-hoc in the span method. I think I'm fine with either.

crates/red_knot_project/src/metadata/options.rs

MichaReiser · 2025-02-11T18:12:39Z

crates/red_knot_python_semantic/src/types/diagnostic.rs

-
-    fn range(&self) -> Option<TextRange> {
-        Some(Ranged::range(self))
+    fn span(&self) -> Option<Span> {


Same: I'm sort of inclined to simply store the Span directly on the Diagnostic struct. But I guess this way is fine too

This one is a little trickier because a TypedDiagnostic always has a File and a Range, where as a Span always has a File and an optional Range. So I'm going to leave this as-is for now.

This is a small change to facilitate migrating to a trait without separate `file` and `range` methods.

... in favor of the implementations providing this routine themselves.

... and update all of the consumers to use `Diagnostic::span`.

AlexWaygood

Since Span is Copy, you could make the range and file methods take self rather than &self, which would allow you to get rid of a few closures:

Patch

diff --git a/crates/red_knot_project/src/lib.rs b/crates/red_knot_project/src/lib.rs
index 2126f36a8..bd3dfe60a 100644
--- a/crates/red_knot_project/src/lib.rs
+++ b/crates/red_knot_project/src/lib.rs
@@ -347,7 +347,7 @@ fn check_file_impl(db: &dyn Db, file: File) -> Vec<Box<dyn Diagnostic>> {
     diagnostics.sort_unstable_by_key(|diagnostic| {
         diagnostic
             .span()
-            .and_then(|span| span.range())
+            .and_then(Span::range)
             .unwrap_or_default()
             .start()
     });
diff --git a/crates/red_knot_test/src/diagnostic.rs b/crates/red_knot_test/src/diagnostic.rs
index 56ee51223..7cf258f11 100644
--- a/crates/red_knot_test/src/diagnostic.rs
+++ b/crates/red_knot_test/src/diagnostic.rs
@@ -2,7 +2,7 @@
 //!
 //! We don't assume that we will get the diagnostics in source order.
 
-use ruff_db::diagnostic::Diagnostic;
+use ruff_db::diagnostic::{Diagnostic, Span};
 use ruff_source_file::{LineIndex, OneIndexed};
 use std::ops::{Deref, Range};
 
@@ -27,7 +27,7 @@ where
             .map(|diagnostic| DiagnosticWithLine {
                 line_number: diagnostic
                     .span()
-                    .and_then(|span| span.range())
+                    .and_then(Span::range)
                     .map_or(OneIndexed::from_zero_indexed(0), |range| {
                         line_index.line_index(range.start())
                     }),
diff --git a/crates/red_knot_test/src/matcher.rs b/crates/red_knot_test/src/matcher.rs
index d350ec7c6..d90bcef03 100644
--- a/crates/red_knot_test/src/matcher.rs
+++ b/crates/red_knot_test/src/matcher.rs
@@ -4,7 +4,7 @@ use crate::assertion::{Assertion, ErrorAssertion, InlineFileAssertions};
 use crate::db::Db;
 use crate::diagnostic::SortedDiagnostics;
 use colored::Colorize;
-use ruff_db::diagnostic::{Diagnostic, DiagnosticAsStrError, DiagnosticId};
+use ruff_db::diagnostic::{Diagnostic, DiagnosticAsStrError, DiagnosticId, Span};
 use ruff_db::files::File;
 use ruff_db::source::{line_index, source_text, SourceText};
 use ruff_source_file::{LineIndex, OneIndexed};
@@ -258,7 +258,7 @@ impl Matcher {
     fn column<T: Diagnostic>(&self, diagnostic: &T) -> OneIndexed {
         diagnostic
             .span()
-            .and_then(|span| span.range())
+            .and_then(Span::range)
             .map(|range| {
                 self.line_index
                     .source_location(range.start(), &self.source)
diff --git a/crates/ruff_benchmark/benches/red_knot.rs b/crates/ruff_benchmark/benches/red_knot.rs
index 87366b04d..423446dde 100644
--- a/crates/ruff_benchmark/benches/red_knot.rs
+++ b/crates/ruff_benchmark/benches/red_knot.rs
@@ -11,7 +11,7 @@ use red_knot_project::{Db, ProjectDatabase, ProjectMetadata};
 use red_knot_python_semantic::PythonVersion;
 use ruff_benchmark::criterion::{criterion_group, criterion_main, BatchSize, Criterion};
 use ruff_benchmark::TestFile;
-use ruff_db::diagnostic::{Diagnostic, DiagnosticId, Severity};
+use ruff_db::diagnostic::{Diagnostic, DiagnosticId, Severity, Span};
 use ruff_db::files::{system_path_to_file, File};
 use ruff_db::source::source_text;
 use ruff_db::system::{MemoryFileSystem, SystemPath, SystemPathBuf, TestSystem};
@@ -231,11 +231,11 @@ fn assert_diagnostics(db: &dyn Db, diagnostics: &[Box<dyn Diagnostic>]) {
                 diagnostic.id(),
                 diagnostic
                     .span()
-                    .map(|span| span.file())
+                    .map(Span::file)
                     .map(|file| file.path(db).as_str()),
                 diagnostic
                     .span()
-                    .and_then(|span| span.range())
+                    .and_then(Span::range)
                     .map(Range::<usize>::from),
                 diagnostic.message(),
                 diagnostic.severity(),
diff --git a/crates/ruff_db/src/diagnostic.rs b/crates/ruff_db/src/diagnostic.rs
index 02c701267..5bcec79c5 100644
--- a/crates/ruff_db/src/diagnostic.rs
+++ b/crates/ruff_db/src/diagnostic.rs
@@ -196,7 +196,7 @@ pub struct Span {
 
 impl Span {
     /// Returns the `File` attached to this `Span`.
-    pub fn file(&self) -> File {
+    pub fn file(self) -> File {
         self.file
     }
 
@@ -205,7 +205,7 @@ impl Span {
     /// When there is no range, it is convention to assume that this `Span`
     /// refers to the corresponding `File` as a whole. In some cases, consumers
     /// of this API may use the range `0..0` to represent this case.
-    pub fn range(&self) -> Option<TextRange> {
+    pub fn range(self) -> Option<TextRange> {
         self.range
     }

(No strong opinion on whether that's worth it, though)

MichaReiser · 2025-02-11T18:51:55Z

It's unclear if Span can remain Copy if we integrate it into Ruff where we don't have a Copy identifier for a file.

BurntSushi · 2025-02-11T19:41:54Z

Yeah Span being Copy is interesting. I wasn't thinking too carefully about that yet, but I can see it being annoying if it can't be Copy. But I wouldn't be surprised if the refactor in Ruff to permit it was brutal.

I'm going to remove the Copy impl, but retain APIs as if it were Copy for now. (Which essentially means writing span.clone() in some cases.) If we really commit to non-Copy, then we'll want to use &Span in more places, which is pretty annoying.

I think ideally this type would be `Copy`, but it sounds like that might prove difficult for Ruff since it doesn't have a `Copy` file handle.

MichaReiser · 2025-02-11T19:44:15Z

I could see us interning files in Ruff when we create the diagnostics, but yeah, it's somewhat unclear if that's hard. But definitely annoying if Span can't be Copy.

* main: add diagnostic `Span` (couples `File` and `TextRange`) (#16101) Remove `Hash` and `Eq` from `AstNodeRef` for types not implementing `Eq` or `Hash` (#16100) Fix release build warning about unused todo type message (#16102) [`pydocstyle`] Handle arguments with the same names as sections (`D417`) (#16011) [red-knot] Reduce usage of `From<Type>` implementations when working with `Symbol`s (#16076) Transition to salsa coarse-grained tracked structs (#15763) [`pyupgrade`] Handle micro version numbers correctly (`UP036`) (#16091) [red-knot] `T | object == object` (#16088) [`ruff`] Skip singleton starred expressions for `incorrectly-parenthesized-tuple-in-subscript` (`RUF031`) (#16083) Delete left-over `verbosity.rs (#16081) [red-knot] User-level configuration (#16021) Add `user_configuration_directory` to `System` (#16020)

BurntSushi requested review from AlexWaygood, MichaReiser, carljm and sharkdp as code owners February 11, 2025 18:02

MichaReiser added the ty Multi-file analysis & type inference label Feb 11, 2025

MichaReiser approved these changes Feb 11, 2025

View reviewed changes

BurntSushi added 4 commits February 11, 2025 13:36

ruff_db: add Diagnostic::span routine

b993468

This is a small change to facilitate migrating to a trait without separate `file` and `range` methods.

ruff_db: remove default implementation of Diagnostic::span

8b86a5f

... in favor of the implementations providing this routine themselves.

ruff_db: remove Diagnostic::file and Diagnostic::range

868e3b5

... and update all of the consumers to use `Diagnostic::span`.

red_knot_project: simplify Span construction

1266c26

BurntSushi force-pushed the ag/couple-file-with-range branch from 95ec2f4 to 1266c26 Compare February 11, 2025 18:38

AlexWaygood approved these changes Feb 11, 2025

View reviewed changes

ruff_db: remove Copy impl for Span

ace0455

I think ideally this type would be `Copy`, but it sounds like that might prove difficult for Ruff since it doesn't have a `Copy` file handle.

BurntSushi merged commit 6e34f74 into main Feb 11, 2025
21 checks passed

BurntSushi deleted the ag/couple-file-with-range branch February 11, 2025 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

add diagnostic `Span` (couples `File` and `TextRange`)#16101

add diagnostic `Span` (couples `File` and `TextRange`)#16101
BurntSushi merged 5 commits intomainfrom
ag/couple-file-with-range

BurntSushi commented Feb 11, 2025

Uh oh!

github-actions bot commented Feb 11, 2025 •

edited

Loading

Uh oh!

MichaReiser left a comment

Uh oh!

Uh oh!

MichaReiser Feb 11, 2025

Uh oh!

BurntSushi Feb 11, 2025

Uh oh!

AlexWaygood left a comment

Uh oh!

MichaReiser commented Feb 11, 2025

Uh oh!

BurntSushi commented Feb 11, 2025

Uh oh!

MichaReiser commented Feb 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

Conversation

BurntSushi commented Feb 11, 2025

Uh oh!

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

Uh oh!

ruff-ecosystem results

Linter (stable)

Linter (preview)

Uh oh!

MichaReiser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MichaReiser Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

BurntSushi Feb 11, 2025

Choose a reason for hiding this comment

Uh oh!

AlexWaygood left a comment

Choose a reason for hiding this comment

Uh oh!

MichaReiser commented Feb 11, 2025

Uh oh!

BurntSushi commented Feb 11, 2025

Uh oh!

MichaReiser commented Feb 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Feb 11, 2025 •

edited

Loading

`ruff-ecosystem` results