From 4ea16b7dd6a0b0f9c2983b27f9cb68fc98e03bc0 Mon Sep 17 00:00:00 2001 From: Brent Westbrook Date: Wed, 24 Sep 2025 11:59:31 -0400 Subject: [PATCH] convert errors to diagnostics too --- crates/ruff/src/commands/format.rs | 92 ++++++++++++++++++++++++++-- crates/ruff_db/src/diagnostic/mod.rs | 14 ++++- 2 files changed, 99 insertions(+), 7 deletions(-) diff --git a/crates/ruff/src/commands/format.rs b/crates/ruff/src/commands/format.rs index 94c37c621e..756e8c3584 100644 --- a/crates/ruff/src/commands/format.rs +++ b/crates/ruff/src/commands/format.rs @@ -194,10 +194,15 @@ pub(crate) fn format( caches.persist()?; // Report on any errors. - errors.sort_unstable_by(|a, b| a.path().cmp(&b.path())); + // + // We only convert errors to `Diagnostic`s in `Check` mode with preview enabled, otherwise we + // fall back on printing simple messages. + if !(preview.is_enabled() && mode.is_check()) { + errors.sort_unstable_by(|a, b| a.path().cmp(&b.path())); - for error in &errors { - error!("{error}"); + for error in &errors { + error!("{error}"); + } } let results = FormatResults::new(results.as_slice(), mode); @@ -205,7 +210,7 @@ pub(crate) fn format( FormatMode::Write => {} FormatMode::Check => { if preview.is_enabled() { - results.write_changed_preview(&mut stdout().lock(), output_format)?; + results.write_changed_preview(&mut stdout().lock(), output_format, &errors)?; } else { results.write_changed(&mut stdout().lock())?; } @@ -605,14 +610,22 @@ impl<'a> FormatResults<'a> { Ok(()) } - /// Write a list of the files that would be changed to the given writer. + /// Write a list of the files that would be changed and any errors to the given writer. + /// + /// Errors are reported first in the order they are provided, followed by the remaining + /// diagnostics sorted by file name. fn write_changed_preview( &self, f: &mut impl Write, output_format: OutputFormat, + errors: &[FormatCommandError], ) -> io::Result<()> { let mut notebook_index = FxHashMap::default(); - let diagnostics: Vec<_> = self.to_diagnostics(&mut notebook_index).collect(); + let diagnostics: Vec<_> = errors + .iter() + .map(Diagnostic::from) + .chain(self.to_diagnostics(&mut notebook_index)) + .collect(); let context = EmitterContext::new(¬ebook_index); let config = DisplayDiagnosticConfig::default(); @@ -852,6 +865,73 @@ impl FormatCommandError { } } +impl From<&FormatCommandError> for Diagnostic { + fn from(error: &FormatCommandError) -> Self { + let annotation = error.path().map(|path| { + let file = SourceFileBuilder::new(path.to_string_lossy(), "").finish(); + let span = Span::from(file); + Annotation::primary(span) + }); + + let mut diagnostic = match error { + // TODO(brent) also not sure about this one. Most of the ignore::Error variants + // do fit pretty well under Io errors, but we could match and be even more + // specific if we wanted. We already have a `DiagnosticId::InvalidGlob`, which + // could map to Error::Glob, for example. + FormatCommandError::Ignore(error) => { + Diagnostic::new(DiagnosticId::Io, Severity::Error, error) + } + // TODO(brent) not sure if this is correct, DisplayParseError includes some + // color and other formatting. I think we'd rather have access to the `path` and + // underlying ParseError directly, like in other variants here. + FormatCommandError::Parse(display_parse_error) => Diagnostic::new( + DiagnosticId::InvalidSyntax, + Severity::Error, + display_parse_error, + ), + FormatCommandError::Panic(_, panic_error) => { + Diagnostic::new(DiagnosticId::Panic, Severity::Fatal, panic_error) + } + + // I/O errors + FormatCommandError::Read(_, source_error) + | FormatCommandError::Write(_, source_error) => { + Diagnostic::new(DiagnosticId::Io, Severity::Error, source_error) + } + FormatCommandError::Diff(_, error) => { + Diagnostic::new(DiagnosticId::Io, Severity::Error, error) + } + + FormatCommandError::Format(_, format_module_error) => match format_module_error { + FormatModuleError::ParseError(parse_error) => Diagnostic::new( + DiagnosticId::InvalidSyntax, + Severity::Error, + &parse_error.error, + ), + FormatModuleError::FormatError(format_error) => { + Diagnostic::new(DiagnosticId::FormatError, Severity::Error, format_error) + } + // TODO(brent) this might need a new DiagnosticId, or we could reuse + // `FormatError`, which also has an InvalidDocument variant, like PrintError + FormatModuleError::PrintError(print_error) => { + Diagnostic::new(DiagnosticId::InvalidSyntax, Severity::Error, print_error) + } + }, + FormatCommandError::RangeFormatNotebook(_) => Diagnostic::new( + DiagnosticId::RangeFormatNotebook, + Severity::Error, + "Range formatting isn't supported for notebooks.", + ), + }; + + if let Some(annotation) = annotation { + diagnostic.annotate(annotation); + } + + diagnostic + } +} + impl Display for FormatCommandError { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { match self { diff --git a/crates/ruff_db/src/diagnostic/mod.rs b/crates/ruff_db/src/diagnostic/mod.rs index b2100403a2..1648456489 100644 --- a/crates/ruff_db/src/diagnostic/mod.rs +++ b/crates/ruff_db/src/diagnostic/mod.rs @@ -443,7 +443,9 @@ impl Diagnostic { | DiagnosticId::UnnecessaryOverridesSection | DiagnosticId::UselessOverridesSection | DiagnosticId::DeprecatedSetting - | DiagnosticId::Unformatted => None, + | DiagnosticId::Unformatted + | DiagnosticId::RangeFormatNotebook + | DiagnosticId::FormatError => None, DiagnosticId::Lint(lint_name) => { Some(format!("{}/rules/{lint_name}", env!("CARGO_PKG_HOMEPAGE"))) } @@ -1026,6 +1028,14 @@ pub enum DiagnosticId { /// The code needs to be formatted. Unformatted, + + /// Range formatting isn't supported for notebooks. + RangeFormatNotebook, + + /// Something went wrong while formatting. + /// + /// This often indicates a bug in the formatter. + FormatError, } impl DiagnosticId { @@ -1066,6 +1076,8 @@ impl DiagnosticId { DiagnosticId::UselessOverridesSection => "useless-overrides-section", DiagnosticId::DeprecatedSetting => "deprecated-setting", DiagnosticId::Unformatted => "unformatted", + DiagnosticId::RangeFormatNotebook => "range-format-notebook", + DiagnosticId::FormatError => "format-error", } }