diff --git a/crates/ruff/src/cache.rs b/crates/ruff/src/cache.rs index 20a8c9726b..6eaef9382d 100644 --- a/crates/ruff/src/cache.rs +++ b/crates/ruff/src/cache.rs @@ -681,7 +681,7 @@ mod tests { UnsafeFixes::Enabled, ) .unwrap(); - if diagnostics.inner.iter().any(Diagnostic::is_syntax_error) { + if diagnostics.inner.iter().any(Diagnostic::is_invalid_syntax) { parse_errors.push(path.clone()); } paths.push(path); diff --git a/crates/ruff/src/commands/check.rs b/crates/ruff/src/commands/check.rs index 8a7a71fc7b..b33dcfe8cb 100644 --- a/crates/ruff/src/commands/check.rs +++ b/crates/ruff/src/commands/check.rs @@ -9,15 +9,15 @@ use ignore::Error; use log::{debug, error, warn}; #[cfg(not(target_family = "wasm"))] use rayon::prelude::*; -use ruff_linter::message::diagnostic_from_violation; use rustc_hash::FxHashMap; +use ruff_db::diagnostic::Diagnostic; use ruff_db::panic::catch_unwind; use ruff_linter::package::PackageRoot; use ruff_linter::registry::Rule; use ruff_linter::settings::types::UnsafeFixes; use ruff_linter::settings::{LinterSettings, flags}; -use ruff_linter::{IOError, fs, warn_user_once}; +use ruff_linter::{IOError, Violation, fs, warn_user_once}; use ruff_source_file::SourceFileBuilder; use ruff_text_size::TextRange; use ruff_workspace::resolver::{ @@ -129,11 +129,7 @@ pub(crate) fn check( SourceFileBuilder::new(path.to_string_lossy().as_ref(), "").finish(); Diagnostics::new( - vec![diagnostic_from_violation( - IOError { message }, - TextRange::default(), - &dummy, - )], + vec![IOError { message }.into_diagnostic(TextRange::default(), &dummy)], FxHashMap::default(), ) } else { @@ -166,7 +162,9 @@ pub(crate) fn check( |a, b| (a.0 + b.0, a.1 + b.1), ); - all_diagnostics.inner.sort(); + all_diagnostics + .inner + .sort_by(Diagnostic::ruff_start_ordering); // Store the caches. caches.persist()?; diff --git a/crates/ruff/src/commands/check_stdin.rs b/crates/ruff/src/commands/check_stdin.rs index 2af7918480..f76b3ab2df 100644 --- a/crates/ruff/src/commands/check_stdin.rs +++ b/crates/ruff/src/commands/check_stdin.rs @@ -1,6 +1,7 @@ use std::path::Path; use anyhow::Result; +use ruff_db::diagnostic::Diagnostic; use ruff_linter::package::PackageRoot; use ruff_linter::packaging; use ruff_linter::settings::flags; @@ -52,6 +53,8 @@ pub(crate) fn check_stdin( noqa, fix_mode, )?; - diagnostics.inner.sort_unstable(); + diagnostics + .inner + .sort_unstable_by(Diagnostic::ruff_start_ordering); Ok(diagnostics) } diff --git a/crates/ruff/src/diagnostics.rs b/crates/ruff/src/diagnostics.rs index 44c3263487..38bfedbc44 100644 --- a/crates/ruff/src/diagnostics.rs +++ b/crates/ruff/src/diagnostics.rs @@ -13,13 +13,13 @@ use log::{debug, warn}; use ruff_db::diagnostic::Diagnostic; use ruff_linter::codes::Rule; use ruff_linter::linter::{FixTable, FixerResult, LinterResult, ParseSource, lint_fix, lint_only}; -use ruff_linter::message::{create_syntax_error_diagnostic, diagnostic_from_violation}; +use ruff_linter::message::create_syntax_error_diagnostic; use ruff_linter::package::PackageRoot; use ruff_linter::pyproject_toml::lint_pyproject_toml; use ruff_linter::settings::types::UnsafeFixes; use ruff_linter::settings::{LinterSettings, flags}; use ruff_linter::source_kind::{SourceError, SourceKind}; -use ruff_linter::{IOError, fs}; +use ruff_linter::{IOError, Violation, fs}; use ruff_notebook::{Notebook, NotebookError, NotebookIndex}; use ruff_python_ast::{PySourceType, SourceType, TomlSourceType}; use ruff_source_file::SourceFileBuilder; @@ -62,13 +62,12 @@ impl Diagnostics { let name = path.map_or_else(|| "-".into(), Path::to_string_lossy); let source_file = SourceFileBuilder::new(name, "").finish(); Self::new( - vec![diagnostic_from_violation( + vec![ IOError { message: err.to_string(), - }, - TextRange::default(), - &source_file, - )], + } + .into_diagnostic(TextRange::default(), &source_file), + ], FxHashMap::default(), ) } else { diff --git a/crates/ruff_db/src/diagnostic/mod.rs b/crates/ruff_db/src/diagnostic/mod.rs index dadb037fdf..9b449ee6f8 100644 --- a/crates/ruff_db/src/diagnostic/mod.rs +++ b/crates/ruff_db/src/diagnostic/mod.rs @@ -83,7 +83,7 @@ impl Diagnostic { /// /// Note that `message` is stored in the primary annotation, _not_ in the primary diagnostic /// message. - pub fn syntax_error( + pub fn invalid_syntax( span: impl Into, message: impl IntoDiagnosticMessage, range: impl Ranged, @@ -365,7 +365,7 @@ impl Diagnostic { } /// Returns `true` if `self` is a syntax error message. - pub fn is_syntax_error(&self) -> bool { + pub fn is_invalid_syntax(&self) -> bool { self.id().is_invalid_syntax() } @@ -381,7 +381,7 @@ impl Diagnostic { /// Returns the URL for the rule documentation, if it exists. pub fn to_url(&self) -> Option { - if self.is_syntax_error() { + if self.is_invalid_syntax() { None } else { Some(format!( @@ -447,20 +447,16 @@ impl Diagnostic { pub fn expect_range(&self) -> TextRange { self.range().expect("Expected a range for the primary span") } -} -impl Ord for Diagnostic { - fn cmp(&self, other: &Self) -> std::cmp::Ordering { - self.partial_cmp(other).unwrap_or(std::cmp::Ordering::Equal) - } -} - -impl PartialOrd for Diagnostic { - fn partial_cmp(&self, other: &Self) -> Option { - Some( - (self.ruff_source_file()?, self.range()?.start()) - .cmp(&(other.ruff_source_file()?, other.range()?.start())), - ) + /// Returns the ordering of diagnostics based on the start of their ranges, if they have any. + /// + /// Panics if either diagnostic has no primary span, if the span has no range, or if its file is + /// not a `SourceFile`. + pub fn ruff_start_ordering(&self, other: &Self) -> std::cmp::Ordering { + (self.expect_ruff_source_file(), self.expect_range().start()).cmp(&( + other.expect_ruff_source_file(), + other.expect_range().start(), + )) } } diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index e1def841e5..0fc24dc8e2 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -64,7 +64,6 @@ use ruff_text_size::{Ranged, TextRange, TextSize}; use crate::checkers::ast::annotation::AnnotationContext; use crate::docstrings::extraction::ExtractionTarget; use crate::importer::{ImportRequest, Importer, ResolutionError}; -use crate::message::diagnostic_from_violation; use crate::noqa::NoqaMapping; use crate::package::PackageRoot; use crate::preview::is_undefined_export_in_dunder_init_enabled; @@ -3158,7 +3157,7 @@ impl<'a> LintContext<'a> { ) -> DiagnosticGuard<'chk, 'a> { DiagnosticGuard { context: self, - diagnostic: Some(diagnostic_from_violation(kind, range, &self.source_file)), + diagnostic: Some(kind.into_diagnostic(range, &self.source_file)), rule: T::rule(), } } @@ -3177,7 +3176,7 @@ impl<'a> LintContext<'a> { if self.is_rule_enabled(rule) { Some(DiagnosticGuard { context: self, - diagnostic: Some(diagnostic_from_violation(kind, range, &self.source_file)), + diagnostic: Some(kind.into_diagnostic(range, &self.source_file)), rule, }) } else { diff --git a/crates/ruff_linter/src/fix/edits.rs b/crates/ruff_linter/src/fix/edits.rs index f17cbc3290..e4ac1b5d0f 100644 --- a/crates/ruff_linter/src/fix/edits.rs +++ b/crates/ruff_linter/src/fix/edits.rs @@ -618,8 +618,7 @@ mod tests { use crate::fix::edits::{ add_to_dunder_all, make_redundant_alias, next_stmt_break, trailing_semicolon, }; - use crate::message::diagnostic_from_violation; - use crate::{Edit, Fix, Locator}; + use crate::{Edit, Fix, Locator, Violation}; /// Parse the given source using [`Mode::Module`] and return the first statement. fn parse_first_stmt(source: &str) -> Result { @@ -750,8 +749,8 @@ x = 1 \ let diag = { use crate::rules::pycodestyle::rules::MissingNewlineAtEndOfFile; let mut iter = edits.into_iter(); - let mut diagnostic = diagnostic_from_violation( - MissingNewlineAtEndOfFile, // The choice of rule here is arbitrary. + // The choice of rule here is arbitrary. + let mut diagnostic = MissingNewlineAtEndOfFile.into_diagnostic( TextRange::default(), &SourceFileBuilder::new("", "").finish(), ); diff --git a/crates/ruff_linter/src/fix/mod.rs b/crates/ruff_linter/src/fix/mod.rs index e68898a5e6..e2672f2f37 100644 --- a/crates/ruff_linter/src/fix/mod.rs +++ b/crates/ruff_linter/src/fix/mod.rs @@ -172,11 +172,10 @@ mod tests { use ruff_source_file::SourceFileBuilder; use ruff_text_size::{Ranged, TextSize}; - use crate::Locator; use crate::fix::{FixResult, apply_fixes}; - use crate::message::diagnostic_from_violation; use crate::rules::pycodestyle::rules::MissingNewlineAtEndOfFile; use crate::{Edit, Fix}; + use crate::{Locator, Violation}; use ruff_db::diagnostic::Diagnostic; fn create_diagnostics( @@ -187,8 +186,7 @@ mod tests { edit.into_iter() .map(|edit| { // The choice of rule here is arbitrary. - let mut diagnostic = diagnostic_from_violation( - MissingNewlineAtEndOfFile, + let mut diagnostic = MissingNewlineAtEndOfFile.into_diagnostic( edit.range(), &SourceFileBuilder::new(filename, source).finish(), ); diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs index ddb82c7d20..dd8543b983 100644 --- a/crates/ruff_linter/src/linter.rs +++ b/crates/ruff_linter/src/linter.rs @@ -514,7 +514,7 @@ pub fn lint_only( LinterResult { has_valid_syntax: parsed.has_valid_syntax(), - has_no_syntax_errors: !diagnostics.iter().any(Diagnostic::is_syntax_error), + has_no_syntax_errors: !diagnostics.iter().any(Diagnostic::is_invalid_syntax), diagnostics, } } @@ -629,7 +629,7 @@ pub fn lint_fix<'a>( if iterations == 0 { has_valid_syntax = parsed.has_valid_syntax(); - has_no_syntax_errors = !diagnostics.iter().any(Diagnostic::is_syntax_error); + has_no_syntax_errors = !diagnostics.iter().any(Diagnostic::is_invalid_syntax); } else { // If the source code had no syntax errors on the first pass, but // does on a subsequent pass, then we've introduced a diff --git a/crates/ruff_linter/src/message/mod.rs b/crates/ruff_linter/src/message/mod.rs index 85897a8a3d..2dc119e440 100644 --- a/crates/ruff_linter/src/message/mod.rs +++ b/crates/ruff_linter/src/message/mod.rs @@ -24,7 +24,6 @@ pub use sarif::SarifEmitter; pub use text::TextEmitter; use crate::Fix; -use crate::Violation; use crate::registry::Rule; mod azure; @@ -108,28 +107,6 @@ where diagnostic } -// TODO(brent) We temporarily allow this to avoid updating all of the call sites to add -// references. I expect this method to go away or change significantly with the rest of the -// diagnostic refactor, but if it still exists in this form at the end of the refactor, we -// should just update the call sites. -#[expect(clippy::needless_pass_by_value)] -pub fn diagnostic_from_violation( - kind: T, - range: TextRange, - file: &SourceFile, -) -> Diagnostic { - create_lint_diagnostic( - Violation::message(&kind), - Violation::fix_title(&kind), - range, - None, - None, - file.clone(), - None, - T::rule(), - ) -} - struct MessageWithLocation<'a> { message: &'a Diagnostic, start_location: LineColumn, diff --git a/crates/ruff_linter/src/noqa.rs b/crates/ruff_linter/src/noqa.rs index 832aceb6ce..7601b6ccb7 100644 --- a/crates/ruff_linter/src/noqa.rs +++ b/crates/ruff_linter/src/noqa.rs @@ -1225,8 +1225,6 @@ mod tests { use ruff_source_file::{LineEnding, SourceFileBuilder}; use ruff_text_size::{TextLen, TextRange, TextSize}; - use crate::Edit; - use crate::message::diagnostic_from_violation; use crate::noqa::{ Directive, LexicalError, NoqaLexerOutput, NoqaMapping, add_noqa_inner, lex_codes, lex_file_exemption, lex_inline_noqa, @@ -1234,6 +1232,7 @@ mod tests { use crate::rules::pycodestyle::rules::{AmbiguousVariableName, UselessSemicolon}; use crate::rules::pyflakes::rules::UnusedVariable; use crate::rules::pyupgrade::rules::PrintfStringFormatting; + use crate::{Edit, Violation}; use crate::{Locator, generate_noqa_edits}; fn assert_lexed_ranges_match_slices( @@ -2832,10 +2831,10 @@ mod tests { assert_eq!(output, format!("{contents}")); let source_file = SourceFileBuilder::new(path.to_string_lossy(), contents).finish(); - let messages = [diagnostic_from_violation( - UnusedVariable { - name: "x".to_string(), - }, + let messages = [UnusedVariable { + name: "x".to_string(), + } + .into_diagnostic( TextRange::new(TextSize::from(0), TextSize::from(0)), &source_file, )]; @@ -2856,15 +2855,14 @@ mod tests { let source_file = SourceFileBuilder::new(path.to_string_lossy(), contents).finish(); let messages = [ - diagnostic_from_violation( - AmbiguousVariableName("x".to_string()), + AmbiguousVariableName("x".to_string()).into_diagnostic( TextRange::new(TextSize::from(0), TextSize::from(0)), &source_file, ), - diagnostic_from_violation( - UnusedVariable { - name: "x".to_string(), - }, + UnusedVariable { + name: "x".to_string(), + } + .into_diagnostic( TextRange::new(TextSize::from(0), TextSize::from(0)), &source_file, ), @@ -2887,15 +2885,14 @@ mod tests { let source_file = SourceFileBuilder::new(path.to_string_lossy(), contents).finish(); let messages = [ - diagnostic_from_violation( - AmbiguousVariableName("x".to_string()), + AmbiguousVariableName("x".to_string()).into_diagnostic( TextRange::new(TextSize::from(0), TextSize::from(0)), &source_file, ), - diagnostic_from_violation( - UnusedVariable { - name: "x".to_string(), - }, + UnusedVariable { + name: "x".to_string(), + } + .into_diagnostic( TextRange::new(TextSize::from(0), TextSize::from(0)), &source_file, ), @@ -2931,11 +2928,8 @@ print( "#; let noqa_line_for = [TextRange::new(8.into(), 68.into())].into_iter().collect(); let source_file = SourceFileBuilder::new(path.to_string_lossy(), source).finish(); - let messages = [diagnostic_from_violation( - PrintfStringFormatting, - TextRange::new(12.into(), 79.into()), - &source_file, - )]; + let messages = [PrintfStringFormatting + .into_diagnostic(TextRange::new(12.into(), 79.into()), &source_file)]; let comment_ranges = CommentRanges::default(); let edits = generate_noqa_edits( path, @@ -2964,11 +2958,8 @@ foo; bar = "; let source_file = SourceFileBuilder::new(path.to_string_lossy(), source).finish(); - let messages = [diagnostic_from_violation( - UselessSemicolon, - TextRange::new(4.into(), 5.into()), - &source_file, - )]; + let messages = + [UselessSemicolon.into_diagnostic(TextRange::new(4.into(), 5.into()), &source_file)]; let noqa_line_for = NoqaMapping::default(); let comment_ranges = CommentRanges::default(); let edits = generate_noqa_edits( diff --git a/crates/ruff_linter/src/pyproject_toml.rs b/crates/ruff_linter/src/pyproject_toml.rs index a45c0b5036..8b319f595c 100644 --- a/crates/ruff_linter/src/pyproject_toml.rs +++ b/crates/ruff_linter/src/pyproject_toml.rs @@ -6,11 +6,10 @@ use ruff_text_size::{TextRange, TextSize}; use ruff_db::diagnostic::Diagnostic; use ruff_source_file::SourceFile; -use crate::IOError; -use crate::message::diagnostic_from_violation; use crate::registry::Rule; use crate::rules::ruff::rules::InvalidPyprojectToml; use crate::settings::LinterSettings; +use crate::{IOError, Violation}; /// RUF200 pub fn lint_pyproject_toml(source_file: &SourceFile, settings: &LinterSettings) -> Vec { @@ -30,11 +29,8 @@ pub fn lint_pyproject_toml(source_file: &SourceFile, settings: &LinterSettings) source_file.name(), ); if settings.rules.enabled(Rule::IOError) { - let diagnostic = diagnostic_from_violation( - IOError { message }, - TextRange::default(), - source_file, - ); + let diagnostic = + IOError { message }.into_diagnostic(TextRange::default(), source_file); messages.push(diagnostic); } else { warn!( @@ -56,11 +52,8 @@ pub fn lint_pyproject_toml(source_file: &SourceFile, settings: &LinterSettings) if settings.rules.enabled(Rule::InvalidPyprojectToml) { let toml_err = err.message().to_string(); - let diagnostic = diagnostic_from_violation( - InvalidPyprojectToml { message: toml_err }, - range, - source_file, - ); + let diagnostic = + InvalidPyprojectToml { message: toml_err }.into_diagnostic(range, source_file); messages.push(diagnostic); } diff --git a/crates/ruff_linter/src/rules/pyflakes/mod.rs b/crates/ruff_linter/src/rules/pyflakes/mod.rs index d78c8217c6..2aa8104d6b 100644 --- a/crates/ruff_linter/src/rules/pyflakes/mod.rs +++ b/crates/ruff_linter/src/rules/pyflakes/mod.rs @@ -774,7 +774,7 @@ mod tests { messages.sort_by_key(|diagnostic| diagnostic.expect_range().start()); let actual = messages .iter() - .filter(|msg| !msg.is_syntax_error()) + .filter(|msg| !msg.is_invalid_syntax()) .map(Diagnostic::name) .collect::>(); let expected: Vec<_> = expected.iter().map(|rule| rule.name().as_str()).collect(); diff --git a/crates/ruff_linter/src/test.rs b/crates/ruff_linter/src/test.rs index c34faaf413..bab73eb86e 100644 --- a/crates/ruff_linter/src/test.rs +++ b/crates/ruff_linter/src/test.rs @@ -292,7 +292,7 @@ Either ensure you always emit a fix or change `Violation::FIX_AVAILABILITY` to e .chain(parsed.errors().iter().map(|parse_error| { create_syntax_error_diagnostic(source_code.clone(), &parse_error.error, parse_error) })) - .sorted() + .sorted_by(Diagnostic::ruff_start_ordering) .collect(); (messages, transformed) } @@ -317,7 +317,7 @@ fn print_syntax_errors(errors: &[ParseError], path: &Path, source: &SourceKind) /// Print the lint diagnostics in `diagnostics`. fn print_diagnostics(mut diagnostics: Vec, path: &Path, source: &SourceKind) -> String { - diagnostics.retain(|msg| !msg.is_syntax_error()); + diagnostics.retain(|msg| !msg.is_invalid_syntax()); if let Some(notebook) = source.as_ipy_notebook() { print_jupyter_messages(&diagnostics, path, notebook) diff --git a/crates/ruff_linter/src/violation.rs b/crates/ruff_linter/src/violation.rs index 9939efe722..f32fa40e9a 100644 --- a/crates/ruff_linter/src/violation.rs +++ b/crates/ruff_linter/src/violation.rs @@ -1,6 +1,10 @@ use std::fmt::{Debug, Display}; -use crate::codes::Rule; +use ruff_db::diagnostic::Diagnostic; +use ruff_source_file::SourceFile; +use ruff_text_size::TextRange; + +use crate::{codes::Rule, message::create_lint_diagnostic}; #[derive(Debug, Copy, Clone)] pub enum FixAvailability { @@ -28,7 +32,7 @@ pub trait ViolationMetadata { fn explain() -> Option<&'static str>; } -pub trait Violation: ViolationMetadata { +pub trait Violation: ViolationMetadata + Sized { /// `None` in the case a fix is never available or otherwise Some /// [`FixAvailability`] describing the available fix. const FIX_AVAILABILITY: FixAvailability = FixAvailability::None; @@ -48,6 +52,20 @@ pub trait Violation: ViolationMetadata { /// Returns the format strings used by [`message`](Violation::message). fn message_formats() -> &'static [&'static str]; + + /// Convert the violation into a [`Diagnostic`]. + fn into_diagnostic(self, range: TextRange, file: &SourceFile) -> Diagnostic { + create_lint_diagnostic( + self.message(), + self.fix_title(), + range, + None, + None, + file.clone(), + None, + Self::rule(), + ) + } } /// This trait exists just to make implementing the [`Violation`] trait more diff --git a/crates/ruff_server/src/lint.rs b/crates/ruff_server/src/lint.rs index 7ee2dcc4dc..4283cde139 100644 --- a/crates/ruff_server/src/lint.rs +++ b/crates/ruff_server/src/lint.rs @@ -163,7 +163,7 @@ pub(crate) fn check( .into_iter() .zip(noqa_edits) .filter_map(|(message, noqa_edit)| { - if message.is_syntax_error() && !show_syntax_errors { + if message.is_invalid_syntax() && !show_syntax_errors { None } else { Some(to_lsp_diagnostic( diff --git a/crates/ty_project/src/lib.rs b/crates/ty_project/src/lib.rs index f829c6b85a..bcc31937be 100644 --- a/crates/ty_project/src/lib.rs +++ b/crates/ty_project/src/lib.rs @@ -500,11 +500,11 @@ impl Project { parsed_ref .errors() .iter() - .map(|error| Diagnostic::syntax_error(file, &error.error, error)), + .map(|error| Diagnostic::invalid_syntax(file, &error.error, error)), ); diagnostics.extend(parsed_ref.unsupported_syntax_errors().iter().map(|error| { - let mut error = Diagnostic::syntax_error(file, error, error); + let mut error = Diagnostic::invalid_syntax(file, error, error); add_inferred_python_version_hint_to_diagnostic(db, &mut error, "parsing syntax"); error })); diff --git a/crates/ty_python_semantic/src/types.rs b/crates/ty_python_semantic/src/types.rs index 4eb6d9207a..4a834fe38c 100644 --- a/crates/ty_python_semantic/src/types.rs +++ b/crates/ty_python_semantic/src/types.rs @@ -106,7 +106,7 @@ pub fn check_types(db: &dyn Db, file: File) -> TypeCheckDiagnostics { index .semantic_syntax_errors() .iter() - .map(|error| Diagnostic::syntax_error(file, error, error)), + .map(|error| Diagnostic::invalid_syntax(file, error, error)), ); check_suppressions(db, file, &mut diagnostics); diff --git a/crates/ty_test/src/lib.rs b/crates/ty_test/src/lib.rs index 97d36b072f..50670fe1cb 100644 --- a/crates/ty_test/src/lib.rs +++ b/crates/ty_test/src/lib.rs @@ -322,14 +322,14 @@ fn run_test( let mut diagnostics: Vec = parsed .errors() .iter() - .map(|error| Diagnostic::syntax_error(test_file.file, &error.error, error)) + .map(|error| Diagnostic::invalid_syntax(test_file.file, &error.error, error)) .collect(); diagnostics.extend( parsed .unsupported_syntax_errors() .iter() - .map(|error| Diagnostic::syntax_error(test_file.file, error, error)), + .map(|error| Diagnostic::invalid_syntax(test_file.file, error, error)), ); let mdtest_result = attempt_test(db, check_types, test_file, "run mdtest", None);