From 0bae7e613d94b2e95be9e5cdfc6e4937d96a77eb Mon Sep 17 00:00:00 2001 From: Dan Parizher <105245560+danparizher@users.noreply.github.com> Date: Fri, 26 Sep 2025 03:06:26 -0400 Subject: [PATCH] Use `Annotation::tags` instead of hardcoded rule matching in ruff server (#20565) Co-authored-by: Micha Reiser --- crates/ruff_linter/src/checkers/ast/mod.rs | 52 ++++++++++++++++++- .../pyflakes/rules/redefined_while_unused.rs | 4 +- .../src/rules/pyflakes/rules/unused_import.rs | 4 ++ .../rules/pyflakes/rules/unused_variable.rs | 2 + .../pyupgrade/rules/deprecated_import.rs | 1 + .../ruff/rules/unused_unpacked_variable.rs | 2 + crates/ruff_server/src/lint.rs | 23 ++++---- 7 files changed, 75 insertions(+), 13 deletions(-) diff --git a/crates/ruff_linter/src/checkers/ast/mod.rs b/crates/ruff_linter/src/checkers/ast/mod.rs index 95014cbd1f..961052eebb 100644 --- a/crates/ruff_linter/src/checkers/ast/mod.rs +++ b/crates/ruff_linter/src/checkers/ast/mod.rs @@ -28,7 +28,7 @@ use itertools::Itertools; use log::debug; use rustc_hash::{FxHashMap, FxHashSet}; -use ruff_db::diagnostic::{Annotation, Diagnostic, IntoDiagnosticMessage, Span}; +use ruff_db::diagnostic::{Annotation, Diagnostic, DiagnosticTag, IntoDiagnosticMessage, Span}; use ruff_diagnostics::{Applicability, Fix, IsolationLevel}; use ruff_notebook::{CellOffsets, NotebookIndex}; use ruff_python_ast::helpers::{collect_import_from_member, is_docstring_stmt, to_module_path}; @@ -3326,6 +3326,56 @@ impl DiagnosticGuard<'_, '_> { pub(crate) fn defuse(mut self) { self.diagnostic = None; } + + /// Set the message on the primary annotation for this diagnostic. + /// + /// If a message already exists on the primary annotation, then this + /// overwrites the existing message. + /// + /// This message is associated with the primary annotation created + /// for every `Diagnostic` that uses the `DiagnosticGuard` API. + /// Specifically, the annotation is derived from the `TextRange` given to + /// the `LintContext::report_diagnostic` API. + /// + /// Callers can add additional primary or secondary annotations via the + /// `DerefMut` trait implementation to a `Diagnostic`. + pub(crate) fn set_primary_message(&mut self, message: impl IntoDiagnosticMessage) { + // N.B. It is normally bad juju to define `self` methods + // on types that implement `Deref`. Instead, it's idiomatic + // to do `fn foo(this: &mut LintDiagnosticGuard)`, which in + // turn forces callers to use + // `LintDiagnosticGuard(&mut guard, message)`. But this is + // supremely annoying for what is expected to be a common + // case. + // + // Moreover, most of the downside that comes from these sorts + // of methods is a semver hazard. Because the deref target type + // could also define a method by the same name, and that leads + // to confusion. But we own all the code involved here and + // there is no semver boundary. So... ¯\_(ツ)_/¯ ---AG + + // OK because we know the diagnostic was constructed with a single + // primary annotation that will always come before any other annotation + // in the diagnostic. (This relies on the `Diagnostic` API not exposing + // any methods for removing annotations or re-ordering them, which is + // true as of 2025-04-11.) + let ann = self.primary_annotation_mut().unwrap(); + ann.set_message(message); + } + + /// Adds a tag on the primary annotation for this diagnostic. + /// + /// This tag is associated with the primary annotation created + /// for every `Diagnostic` that uses the `DiagnosticGuard` API. + /// Specifically, the annotation is derived from the `TextRange` given to + /// the `LintContext::report_diagnostic` API. + /// + /// Callers can add additional primary or secondary annotations via the + /// `DerefMut` trait implementation to a `Diagnostic`. + pub(crate) fn add_primary_tag(&mut self, tag: DiagnosticTag) { + let ann = self.primary_annotation_mut().unwrap(); + ann.push_tag(tag); + } } impl DiagnosticGuard<'_, '_> { diff --git a/crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs b/crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs index 65019a997b..184a88f408 100644 --- a/crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs +++ b/crates/ruff_linter/src/rules/pyflakes/rules/redefined_while_unused.rs @@ -197,9 +197,7 @@ pub(crate) fn redefined_while_unused(checker: &Checker, scope_id: ScopeId, scope shadowed, ); - if let Some(ann) = diagnostic.primary_annotation_mut() { - ann.set_message(format_args!("`{name}` redefined here")); - } + diagnostic.set_primary_message(format_args!("`{name}` redefined here")); if let Some(range) = binding.parent_range(checker.semantic()) { diagnostic.set_parent(range.start()); diff --git a/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs b/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs index 9f73bb3b97..49adff0133 100644 --- a/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs +++ b/crates/ruff_linter/src/rules/pyflakes/rules/unused_import.rs @@ -435,6 +435,8 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope) { diagnostic.set_fix(fix.clone()); } } + + diagnostic.add_primary_tag(ruff_db::diagnostic::DiagnosticTag::Unnecessary); } } @@ -455,6 +457,8 @@ pub(crate) fn unused_import(checker: &Checker, scope: &Scope) { if let Some(range) = binding.parent_range { diagnostic.set_parent(range.start()); } + + diagnostic.add_primary_tag(ruff_db::diagnostic::DiagnosticTag::Unnecessary); } } diff --git a/crates/ruff_linter/src/rules/pyflakes/rules/unused_variable.rs b/crates/ruff_linter/src/rules/pyflakes/rules/unused_variable.rs index 2474bed8a9..b52e01142b 100644 --- a/crates/ruff_linter/src/rules/pyflakes/rules/unused_variable.rs +++ b/crates/ruff_linter/src/rules/pyflakes/rules/unused_variable.rs @@ -272,4 +272,6 @@ pub(crate) fn unused_variable(checker: &Checker, name: &str, binding: &Binding) if let Some(fix) = remove_unused_variable(binding, checker) { diagnostic.set_fix(fix); } + // Add Unnecessary tag for unused variables + diagnostic.add_primary_tag(ruff_db::diagnostic::DiagnosticTag::Unnecessary); } diff --git a/crates/ruff_linter/src/rules/pyupgrade/rules/deprecated_import.rs b/crates/ruff_linter/src/rules/pyupgrade/rules/deprecated_import.rs index c3428ef945..46d317d292 100644 --- a/crates/ruff_linter/src/rules/pyupgrade/rules/deprecated_import.rs +++ b/crates/ruff_linter/src/rules/pyupgrade/rules/deprecated_import.rs @@ -755,6 +755,7 @@ pub(crate) fn deprecated_import(checker: &Checker, import_from_stmt: &StmtImport }, import_from_stmt.range(), ); + diagnostic.add_primary_tag(ruff_db::diagnostic::DiagnosticTag::Deprecated); if let Some(content) = fix { diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( content, diff --git a/crates/ruff_linter/src/rules/ruff/rules/unused_unpacked_variable.rs b/crates/ruff_linter/src/rules/ruff/rules/unused_unpacked_variable.rs index a00c860ba0..f17d3de60d 100644 --- a/crates/ruff_linter/src/rules/ruff/rules/unused_unpacked_variable.rs +++ b/crates/ruff_linter/src/rules/ruff/rules/unused_unpacked_variable.rs @@ -100,4 +100,6 @@ pub(crate) fn unused_unpacked_variable(checker: &Checker, name: &str, binding: & if let Some(fix) = remove_unused_variable(binding, checker) { diagnostic.set_fix(fix); } + // Add Unnecessary tag for unused unpacked variables + diagnostic.add_primary_tag(ruff_db::diagnostic::DiagnosticTag::Unnecessary); } diff --git a/crates/ruff_server/src/lint.rs b/crates/ruff_server/src/lint.rs index 0d3d62c8b0..073587a386 100644 --- a/crates/ruff_server/src/lint.rs +++ b/crates/ruff_server/src/lint.rs @@ -287,7 +287,7 @@ fn to_lsp_diagnostic( let code = code.to_string(); ( Some(severity(&code)), - tags(&code), + tags(diagnostic), Some(lsp_types::NumberOrString::String(code)), ) } else { @@ -338,12 +338,17 @@ fn severity(code: &str) -> lsp_types::DiagnosticSeverity { } } -fn tags(code: &str) -> Option> { - match code { - // F401: imported but unused - // F841: local variable is assigned to but never used - // RUF059: Unused unpacked variable - "F401" | "F841" | "RUF059" => Some(vec![lsp_types::DiagnosticTag::UNNECESSARY]), - _ => None, - } +fn tags(diagnostic: &Diagnostic) -> Option> { + diagnostic.primary_tags().map(|tags| { + tags.iter() + .map(|tag| match tag { + ruff_db::diagnostic::DiagnosticTag::Unnecessary => { + lsp_types::DiagnosticTag::UNNECESSARY + } + ruff_db::diagnostic::DiagnosticTag::Deprecated => { + lsp_types::DiagnosticTag::DEPRECATED + } + }) + .collect() + }) }