language-servers/ruff
1
0
Fork 0
mirror of https://github.com/astral-sh/ruff.git synced 2025-08-04 10:49:50 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 3

Rename Diagnostic::syntax_error methods, separate Ord implementation (#19179)

Browse source 
## Summary

This PR addresses some additional feedback on #19053:

- Renaming the `syntax_error` methods to `invalid_syntax` to match the
lint id
- Moving the standalone `diagnostic_from_violation` function to
`Violation::into_diagnostic`
- Removing the `Ord` and `PartialOrd` implementations from `Diagnostic`
in favor of `Diagnostic::start_ordering`

## Test Plan

Existing tests

## Additional Follow-ups

Besides these, I also put the following comments on my todo list, but
they seemed like they might be big enough to have their own PRs:

- [Use `LintId::IOError` for IO
errors](https://github.com/astral-sh/ruff/pull/19053#discussion_r2189425922)
- [Move `Fix` and
`Edit`](https://github.com/astral-sh/ruff/pull/19053#discussion_r2189448647)
- [Avoid so many
unwraps](https://github.com/astral-sh/ruff/pull/19053#discussion_r2189465980)
This commit is contained in:
Brent Westbrook 2025-07-08 09:54:19 -04:00 • committed by GitHub
parent 738692baff
commit 2643dc5b7a
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
19 changed files with 91 additions and 120 deletions
Download patch file Download diff file Expand all files Collapse all files

2
crates/ruff/src/cache.rs
View file

@ -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);

14
crates/ruff/src/commands/check.rs
View file

@ -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()?;

5
crates/ruff/src/commands/check_stdin.rs
View file

@ -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)
}

13
crates/ruff/src/diagnostics.rs
View file

@ -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 {

28
crates/ruff_db/src/diagnostic/mod.rs
View file

@ -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<Span>,
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<String> {
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<std::cmp::Ordering> {
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(),
))
}
}

5
crates/ruff_linter/src/checkers/ast/mod.rs
View file

@ -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 {

7
crates/ruff_linter/src/fix/edits.rs
View file

@ -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<Stmt> {
@ -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("<filename>", "<code>").finish(),
);

6
crates/ruff_linter/src/fix/mod.rs
View file

@ -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(),
);

4
crates/ruff_linter/src/linter.rs
View file

@ -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

23
crates/ruff_linter/src/message/mod.rs
View file

@ -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<T: 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,

47
crates/ruff_linter/src/noqa.rs
View file

@ -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(

17
crates/ruff_linter/src/pyproject_toml.rs
View file

@ -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<Diagnostic> {
@ -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);
}

2
crates/ruff_linter/src/rules/pyflakes/mod.rs
View file

@ -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::<Vec<_>>();
let expected: Vec<_> = expected.iter().map(|rule| rule.name().as_str()).collect();

4
crates/ruff_linter/src/test.rs
View file

@ -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<Diagnostic>, 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)

22
crates/ruff_linter/src/violation.rs
View file

@ -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

2
crates/ruff_server/src/lint.rs
View file

@ -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(

4
crates/ty_project/src/lib.rs
View file

@ -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
}));

2
crates/ty_python_semantic/src/types.rs
View file

@ -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);

4
crates/ty_test/src/lib.rs
View file

@ -322,14 +322,14 @@ fn run_test(
let mut diagnostics: Vec<Diagnostic> = 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);