Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
5048108
rough draft of FileWrapper enum that might work for ruff
ntBre Apr 29, 2025
2f01da5
use SourceFile and names from the proposal
ntBre Apr 29, 2025
3a04278
add NewDiagnostic wrapper
ntBre Apr 29, 2025
3c5065d
add unused NewDiagnostic::Diagnostic variant
ntBre Apr 29, 2025
0e1a827
convert syntax errors to new diagnostics
ntBre Apr 29, 2025
122e787
fix wasm errors
ntBre May 1, 2025
1559c29
handle notebook todos
ntBre May 1, 2025
9b50816
delete Message::SyntaxError variant
ntBre May 1, 2025
f717a6b
move constructors to NewDiagnostic
ntBre May 1, 2025
70f47bf
delete SyntaxErrorMessage
ntBre May 1, 2025
26c2009
absorb Message into NewDiagnostic
ntBre May 1, 2025
97bf7f3
rename NewDiagnostic back to Message to clarify diff
ntBre May 1, 2025
17753cb
fix ordering to minimize diff
ntBre May 1, 2025
007aa71
RedKnot -> Ty, expect_file -> expect_ty
ntBre May 6, 2025
7f59117
add Diagnostic::expect_ruff_span, inline macros
ntBre May 6, 2025
056d369
delete outdated/obvious todos
ntBre May 6, 2025
f11b040
remove notebook parsing
ntBre May 6, 2025
e9f2c36
red-knot -> ty
ntBre May 7, 2025
6b6a45b
UnifiedFile::expect_ty -> Span::expect_ty_file
ntBre May 7, 2025
eb1e332
restore ty.rs with expect_ty_file
ntBre May 7, 2025
88ae57e
rename expect_ruff_span, add expect_ruff_file
ntBre May 7, 2025
57581f2
clean up unnecessary lifetimes
ntBre May 7, 2025
53eb330
clean up extra blank line
ntBre May 7, 2025
f22a1ad
use expect_ty_file to revert test-specific methods
ntBre May 7, 2025
a77d9fb
pass `impl Display` to `syntax_error`
ntBre May 7, 2025
3f8ccd9
expect_span -> expect_primary_span and add docs
ntBre May 7, 2025
e76d2ec
add some docs to UnifiedFile
ntBre May 7, 2025
60e18ad
add DiagnosticSource instead of reusing SourceText
ntBre May 7, 2025
1faa5e0
convert FileResolver to a trait
ntBre May 7, 2025
561f2ac
Merge branch 'main' into brent/diagnostics-file-enum
ntBre May 7, 2025
e29eb48
clean up remaining input -> diagnostic_source renames
ntBre May 7, 2025
33b75e6
fix docs link
ntBre May 7, 2025
8614e56
switch to dyn FileResolver
ntBre May 7, 2025
3471834
Clean up remaining pub(crate)s on SourceTextInner
ntBre May 7, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion crates/ruff/src/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,7 @@ impl LintCacheData {
.map(|msg| {
// Make sure that all message use the same source file.
assert_eq!(
&msg.file,
msg.file,
messages.first().unwrap().source_file(),
"message uses a different source file"
);
Expand Down
8 changes: 2 additions & 6 deletions crates/ruff/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use rustc_hash::FxHashMap;
use ruff_diagnostics::Diagnostic;
use ruff_linter::codes::Rule;
use ruff_linter::linter::{lint_fix, lint_only, FixTable, FixerResult, LinterResult, ParseSource};
use ruff_linter::message::{Message, SyntaxErrorMessage};
use ruff_linter::message::Message;
use ruff_linter::package::PackageRoot;
use ruff_linter::pyproject_toml::lint_pyproject_toml;
use ruff_linter::settings::types::UnsafeFixes;
Expand Down Expand Up @@ -102,11 +102,7 @@ impl Diagnostics {
let name = path.map_or_else(|| "-".into(), Path::to_string_lossy);
let dummy = SourceFileBuilder::new(name, "").finish();
Self::new(
vec![Message::SyntaxError(SyntaxErrorMessage {
message: err.to_string(),
range: TextRange::default(),
file: dummy,
})],
vec![Message::syntax_error(err, TextRange::default(), dummy)],
FxHashMap::default(),
)
}
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_benchmark/benches/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ fn assert_diagnostics(db: &dyn Db, diagnostics: &[Diagnostic], expected: &[KeyDi
diagnostic.id(),
diagnostic
.primary_span()
.map(|span| span.file())
.map(|span| span.expect_ty_file())
.map(|file| file.path(db).as_str()),
diagnostic
.primary_span()
Expand Down
121 changes: 107 additions & 14 deletions crates/ruff_db/src/diagnostic/mod.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
use std::{fmt::Formatter, sync::Arc};

use render::{FileResolver, Input};
use ruff_source_file::{SourceCode, SourceFile};
use thiserror::Error;

use ruff_annotate_snippets::Level as AnnotateLevel;
use ruff_text_size::{Ranged, TextRange};

pub use self::render::DisplayDiagnostic;
use crate::files::File;
use crate::Db;
use crate::{files::File, Db};

use self::render::FileResolver;
mod render;
mod stylesheet;

Expand Down Expand Up @@ -115,10 +115,9 @@ impl Diagnostic {
/// callers should prefer using this with `write!` instead of `writeln!`.
pub fn display<'a>(
&'a self,
db: &'a dyn Db,
resolver: &'a dyn FileResolver,
config: &'a DisplayDiagnosticConfig,
) -> DisplayDiagnostic<'a> {
let resolver = FileResolver::new(db);
DisplayDiagnostic::new(resolver, config, self)
}

Expand Down Expand Up @@ -233,6 +232,16 @@ impl Diagnostic {
self.primary_annotation().map(|ann| ann.tags.as_slice())
}

/// Returns the "primary" span of this diagnostic, panicking if it does not exist.
///
/// This should typically only be used when working with diagnostics in Ruff, where diagnostics
/// are currently required to have a primary span.
///
/// See [`Diagnostic::primary_span`] for more details.
pub fn expect_primary_span(&self) -> Span {
self.primary_span().expect("Expected a primary span")
}

/// Returns a key that can be used to sort two diagnostics into the canonical order
/// in which they should appear when rendered.
pub fn rendering_sort_key<'a>(&'a self, db: &'a dyn Db) -> impl Ord + 'a {
Expand Down Expand Up @@ -267,11 +276,7 @@ impl Ord for RenderingSortKey<'_> {
self.diagnostic.primary_span(),
other.diagnostic.primary_span(),
) {
let order = span1
.file()
.path(self.db)
.as_str()
.cmp(span2.file().path(self.db).as_str());
let order = span1.file().path(&self.db).cmp(span2.file().path(&self.db));
if order.is_ne() {
return order;
}
Expand Down Expand Up @@ -643,6 +648,10 @@ impl DiagnosticId {
DiagnosticId::UnknownRule => "unknown-rule",
})
}

pub fn is_invalid_syntax(&self) -> bool {
matches!(self, Self::InvalidSyntax)
}
}

#[derive(Copy, Clone, Debug, Eq, PartialEq, Error)]
Expand All @@ -668,21 +677,77 @@ impl std::fmt::Display for DiagnosticId {
}
}

/// A unified file representation for both ruff and ty.
///
/// Such a representation is needed for rendering [`Diagnostic`]s that can optionally contain
/// [`Annotation`]s with [`Span`]s that need to refer to the text of a file. However, ty and ruff
/// use very different file types: a `Copy`-able salsa-interned [`File`], and a heavier-weight
/// [`SourceFile`], respectively.
///
/// This enum presents a unified interface to these two types for the sake of creating [`Span`]s and
/// emitting diagnostics from both ty and ruff.
#[derive(Debug, Clone, PartialEq, Eq)]
pub enum UnifiedFile {
Ty(File),
Ruff(SourceFile),
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, much better!


impl UnifiedFile {
pub fn path<'a>(&'a self, resolver: &'a dyn FileResolver) -> &'a str {
match self {
UnifiedFile::Ty(file) => resolver.path(*file),
UnifiedFile::Ruff(file) => file.name(),
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


fn diagnostic_source(&self, resolver: &dyn FileResolver) -> DiagnosticSource {
match self {
UnifiedFile::Ty(file) => DiagnosticSource::Ty(resolver.input(*file)),
UnifiedFile::Ruff(file) => DiagnosticSource::Ruff(file.clone()),
}
}
}

/// A unified wrapper for types that can be converted to a [`SourceCode`].
///
/// As with [`UnifiedFile`], ruff and ty use slightly different representations for source code.
/// [`DiagnosticSource`] wraps both of these and provides the single
/// [`DiagnosticSource::as_source_code`] method to produce a [`SourceCode`] with the appropriate
/// lifetimes.
///
/// See [`UnifiedFile::diagnostic_source`] for a way to obtain a [`DiagnosticSource`] from a file
/// and [`FileResolver`].
#[derive(Clone, Debug)]
enum DiagnosticSource {
Ty(Input),
Ruff(SourceFile),
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


impl DiagnosticSource {
/// Returns this input as a `SourceCode` for convenient querying.
fn as_source_code(&self) -> SourceCode {
match self {
DiagnosticSource::Ty(input) => SourceCode::new(input.text.as_str(), &input.line_index),
DiagnosticSource::Ruff(source) => SourceCode::new(source.source_text(), source.index()),
}
}
}

/// A span represents the source of a diagnostic.
///
/// It consists of a `File` and an optional range into that file. When the
/// range isn't present, it semantically implies that the diagnostic refers to
/// the entire file. For example, when the file should be executable but isn't.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct Span {
file: File,
file: UnifiedFile,
range: Option<TextRange>,
}

impl Span {
/// Returns the `File` attached to this `Span`.
pub fn file(&self) -> File {
self.file
/// Returns the `UnifiedFile` attached to this `Span`.
pub fn file(&self) -> &UnifiedFile {
&self.file
}

/// Returns the range, if available, attached to this `Span`.
Expand All @@ -703,10 +768,38 @@ impl Span {
pub fn with_optional_range(self, range: Option<TextRange>) -> Span {
Span { range, ..self }
}

/// Returns the [`File`] attached to this [`Span`].
///
/// Panics if the file is a [`UnifiedFile::Ruff`] instead of a [`UnifiedFile::Ty`].
pub fn expect_ty_file(&self) -> File {
match self.file {
UnifiedFile::Ty(file) => file,
UnifiedFile::Ruff(_) => panic!("Expected a ty `File`, found a ruff `SourceFile`"),
}
}

/// Returns the [`SourceFile`] attached to this [`Span`].
///
/// Panics if the file is a [`UnifiedFile::Ty`] instead of a [`UnifiedFile::Ruff`].
pub fn expect_ruff_file(&self) -> &SourceFile {
match &self.file {
UnifiedFile::Ty(_) => panic!("Expected a ruff `SourceFile`, found a ty `File`"),
UnifiedFile::Ruff(file) => file,
}
}
}

impl From<File> for Span {
fn from(file: File) -> Span {
let file = UnifiedFile::Ty(file);
Span { file, range: None }
}
}

impl From<SourceFile> for Span {
fn from(file: SourceFile) -> Self {
let file = UnifiedFile::Ruff(file);
Span { file, range: None }
}
}
Expand Down
Loading
Loading