From ce216c79cc853181694439a5f6175bd25fd0d56d Mon Sep 17 00:00:00 2001 From: Brent Westbrook <36778786+ntBre@users.noreply.github.com> Date: Thu, 5 Jun 2025 12:48:29 -0400 Subject: [PATCH] Remove `Message::to_rule` (#18447) ## Summary As the title says, this PR removes the `Message::to_rule` method by replacing related uses of `Rule` with `NoqaCode` (or the rule's name in the case of the cache). Where it seemed a `Rule` was really needed, we convert back to the `Rule` by parsing either the rule name (with `str::parse`) or the `NoqaCode` (with `Rule::from_code`). I thought this was kind of like cheating and that it might not resolve this part of Micha's [comment](https://github.com/astral-sh/ruff/pull/18391#issuecomment-2933764275): > because we can't add Rule to Diagnostic or **have it anywhere in our shared rendering logic** but after looking again, the only remaining `Rule` conversion in rendering code is for the SARIF output format. The other two non-test `Rule` conversions are for caching and writing a fix summary, which I don't think fall into the shared rendering logic. That leaves the SARIF format as the only real problem, but maybe we can delay that for now. The motivation here is that we won't be able to store a `Rule` on the new `Diagnostic` type, but we should be able to store a `NoqaCode`, likely as a string. ## Test Plan Existing tests ## [Benchmarks](https://codspeed.io/astral-sh/ruff/branches/brent%2Fremove-to-rule) Almost no perf regression, only -1% on `linter/default-rules[large/dataset.py]`. --------- Co-authored-by: Micha Reiser --- crates/ruff/src/cache.rs | 5 +- crates/ruff/src/commands/rule.rs | 4 +- crates/ruff/src/diagnostics.rs | 12 ++-- crates/ruff/src/printer.rs | 18 ++--- crates/ruff_dev/src/generate_docs.rs | 4 +- crates/ruff_dev/src/generate_rules_table.rs | 2 +- crates/ruff_linter/src/codes.rs | 4 +- crates/ruff_linter/src/fix/mod.rs | 62 +++++++++------- crates/ruff_linter/src/linter.rs | 71 +++++++++++++++---- crates/ruff_linter/src/message/azure.rs | 2 +- crates/ruff_linter/src/message/github.rs | 4 +- crates/ruff_linter/src/message/gitlab.rs | 2 +- crates/ruff_linter/src/message/json.rs | 4 +- crates/ruff_linter/src/message/junit.rs | 2 +- crates/ruff_linter/src/message/mod.rs | 28 +++----- crates/ruff_linter/src/message/pylint.rs | 2 +- crates/ruff_linter/src/message/rdjson.rs | 4 +- crates/ruff_linter/src/message/sarif.rs | 28 +++++--- crates/ruff_linter/src/message/text.rs | 6 +- crates/ruff_linter/src/noqa.rs | 32 ++++----- crates/ruff_linter/src/registry.rs | 24 +++++-- crates/ruff_linter/src/registry/rule_set.rs | 3 +- crates/ruff_linter/src/rule_selector.rs | 3 +- crates/ruff_linter/src/rules/fastapi/mod.rs | 5 +- .../ruff_linter/src/rules/flake8_fixme/mod.rs | 2 +- .../src/rules/flake8_gettext/mod.rs | 2 +- .../ruff_linter/src/rules/flake8_raise/mod.rs | 3 +- .../ruff_linter/src/rules/flake8_self/mod.rs | 3 +- .../ruff_linter/src/rules/flake8_todos/mod.rs | 3 +- .../src/rules/flake8_type_checking/mod.rs | 25 +++---- crates/ruff_linter/src/rules/numpy/mod.rs | 3 +- crates/ruff_linter/src/rules/pydoclint/mod.rs | 9 ++- crates/ruff_linter/src/rules/pyflakes/mod.rs | 4 +- .../ruff_linter/src/rules/tryceratops/mod.rs | 3 +- crates/ruff_linter/src/test.rs | 6 +- crates/ruff_macros/src/map_codes.rs | 13 ++-- crates/ruff_server/src/lint.rs | 2 +- .../src/server/api/requests/hover.rs | 2 +- crates/ruff_wasm/src/lib.rs | 2 +- crates/ruff_workspace/src/configuration.rs | 2 +- 40 files changed, 232 insertions(+), 183 deletions(-) diff --git a/crates/ruff/src/cache.rs b/crates/ruff/src/cache.rs index 4c0f11392f..69530ea953 100644 --- a/crates/ruff/src/cache.rs +++ b/crates/ruff/src/cache.rs @@ -439,7 +439,10 @@ impl LintCacheData { let messages = messages .iter() - .filter_map(|msg| msg.to_rule().map(|rule| (rule, msg))) + // Parse the kebab-case rule name into a `Rule`. This will fail for syntax errors, so + // this also serves to filter them out, but we shouldn't be caching files with syntax + // errors anyway. + .filter_map(|msg| Some((msg.name().parse().ok()?, msg))) .map(|(rule, msg)| { // Make sure that all message use the same source file. assert_eq!( diff --git a/crates/ruff/src/commands/rule.rs b/crates/ruff/src/commands/rule.rs index 45b071d2ea..adc761b3e5 100644 --- a/crates/ruff/src/commands/rule.rs +++ b/crates/ruff/src/commands/rule.rs @@ -30,7 +30,7 @@ impl<'a> Explanation<'a> { let (linter, _) = Linter::parse_code(&code).unwrap(); let fix = rule.fixable().to_string(); Self { - name: rule.as_ref(), + name: rule.name().as_str(), code, linter: linter.name(), summary: rule.message_formats()[0], @@ -44,7 +44,7 @@ impl<'a> Explanation<'a> { fn format_rule_text(rule: Rule) -> String { let mut output = String::new(); - let _ = write!(&mut output, "# {} ({})", rule.as_ref(), rule.noqa_code()); + let _ = write!(&mut output, "# {} ({})", rule.name(), rule.noqa_code()); output.push('\n'); output.push('\n'); diff --git a/crates/ruff/src/diagnostics.rs b/crates/ruff/src/diagnostics.rs index d562c009b1..dff416bc55 100644 --- a/crates/ruff/src/diagnostics.rs +++ b/crates/ruff/src/diagnostics.rs @@ -165,9 +165,9 @@ impl AddAssign for FixMap { continue; } let fixed_in_file = self.0.entry(filename).or_default(); - for (rule, count) in fixed { + for (rule, name, count) in fixed.iter() { if count > 0 { - *fixed_in_file.entry(rule).or_default() += count; + *fixed_in_file.entry(rule).or_default(name) += count; } } } @@ -305,7 +305,7 @@ pub(crate) fn lint_path( ParseSource::None, ); let transformed = source_kind; - let fixed = FxHashMap::default(); + let fixed = FixTable::default(); (result, transformed, fixed) } } else { @@ -319,7 +319,7 @@ pub(crate) fn lint_path( ParseSource::None, ); let transformed = source_kind; - let fixed = FxHashMap::default(); + let fixed = FixTable::default(); (result, transformed, fixed) }; @@ -473,7 +473,7 @@ pub(crate) fn lint_stdin( } let transformed = source_kind; - let fixed = FxHashMap::default(); + let fixed = FixTable::default(); (result, transformed, fixed) } } else { @@ -487,7 +487,7 @@ pub(crate) fn lint_stdin( ParseSource::None, ); let transformed = source_kind; - let fixed = FxHashMap::default(); + let fixed = FixTable::default(); (result, transformed, fixed) }; diff --git a/crates/ruff/src/printer.rs b/crates/ruff/src/printer.rs index 2f72e2bcd5..38530d18f4 100644 --- a/crates/ruff/src/printer.rs +++ b/crates/ruff/src/printer.rs @@ -7,6 +7,7 @@ use bitflags::bitflags; use colored::Colorize; use itertools::{Itertools, iterate}; use ruff_linter::codes::NoqaCode; +use ruff_linter::linter::FixTable; use serde::Serialize; use ruff_linter::fs::relativize_path; @@ -80,7 +81,7 @@ impl Printer { let fixed = diagnostics .fixed .values() - .flat_map(std::collections::HashMap::values) + .flat_map(FixTable::counts) .sum::(); if self.flags.intersects(Flags::SHOW_VIOLATIONS) { @@ -302,7 +303,7 @@ impl Printer { let statistics: Vec = diagnostics .messages .iter() - .map(|message| (message.to_noqa_code(), message)) + .map(|message| (message.noqa_code(), message)) .sorted_by_key(|(code, message)| (*code, message.fixable())) .fold( vec![], @@ -472,13 +473,13 @@ fn show_fix_status(fix_mode: flags::FixMode, fixables: Option<&FixableStatistics fn print_fix_summary(writer: &mut dyn Write, fixed: &FixMap) -> Result<()> { let total = fixed .values() - .map(|table| table.values().sum::()) + .map(|table| table.counts().sum::()) .sum::(); assert!(total > 0); let num_digits = num_digits( - *fixed + fixed .values() - .filter_map(|table| table.values().max()) + .filter_map(|table| table.counts().max()) .max() .unwrap(), ); @@ -498,12 +499,11 @@ fn print_fix_summary(writer: &mut dyn Write, fixed: &FixMap) -> Result<()> { relativize_path(filename).bold(), ":".cyan() )?; - for (rule, count) in table.iter().sorted_by_key(|(.., count)| Reverse(*count)) { + for (code, name, count) in table.iter().sorted_by_key(|(.., count)| Reverse(*count)) { writeln!( writer, - " {count:>num_digits$} × {} ({})", - rule.noqa_code().to_string().red().bold(), - rule.as_ref(), + " {count:>num_digits$} × {code} ({name})", + code = code.to_string().red().bold(), )?; } } diff --git a/crates/ruff_dev/src/generate_docs.rs b/crates/ruff_dev/src/generate_docs.rs index d191e82649..5f2309328e 100644 --- a/crates/ruff_dev/src/generate_docs.rs +++ b/crates/ruff_dev/src/generate_docs.rs @@ -29,7 +29,7 @@ pub(crate) fn main(args: &Args) -> Result<()> { if let Some(explanation) = rule.explanation() { let mut output = String::new(); - let _ = writeln!(&mut output, "# {} ({})", rule.as_ref(), rule.noqa_code()); + let _ = writeln!(&mut output, "# {} ({})", rule.name(), rule.noqa_code()); let (linter, _) = Linter::parse_code(&rule.noqa_code().to_string()).unwrap(); if linter.url().is_some() { @@ -101,7 +101,7 @@ pub(crate) fn main(args: &Args) -> Result<()> { let filename = PathBuf::from(ROOT_DIR) .join("docs") .join("rules") - .join(rule.as_ref()) + .join(&*rule.name()) .with_extension("md"); if args.dry_run { diff --git a/crates/ruff_dev/src/generate_rules_table.rs b/crates/ruff_dev/src/generate_rules_table.rs index 48b1cdc2c9..3255f8f42b 100644 --- a/crates/ruff_dev/src/generate_rules_table.rs +++ b/crates/ruff_dev/src/generate_rules_table.rs @@ -55,7 +55,7 @@ fn generate_table(table_out: &mut String, rules: impl IntoIterator, FixAvailability::None => format!(""), }; - let rule_name = rule.as_ref(); + let rule_name = rule.name(); // If the message ends in a bracketed expression (like: "Use {replacement}"), escape the // brackets. Otherwise, it'll be interpreted as an HTML attribute via the `attr_list` diff --git a/crates/ruff_linter/src/codes.rs b/crates/ruff_linter/src/codes.rs index 69ac2201ce..c31e989a46 100644 --- a/crates/ruff_linter/src/codes.rs +++ b/crates/ruff_linter/src/codes.rs @@ -4,13 +4,13 @@ /// `--select`. For pylint this is e.g. C0414 and E0118 but also C and E01. use std::fmt::Formatter; -use strum_macros::{AsRefStr, EnumIter}; +use strum_macros::EnumIter; use crate::registry::Linter; use crate::rule_selector::is_single_rule_selector; use crate::rules; -#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct NoqaCode(&'static str, &'static str); impl NoqaCode { diff --git a/crates/ruff_linter/src/fix/mod.rs b/crates/ruff_linter/src/fix/mod.rs index ae77973ed7..93541a24f6 100644 --- a/crates/ruff_linter/src/fix/mod.rs +++ b/crates/ruff_linter/src/fix/mod.rs @@ -1,7 +1,7 @@ use std::collections::BTreeSet; use itertools::Itertools; -use rustc_hash::{FxHashMap, FxHashSet}; +use rustc_hash::FxHashSet; use ruff_diagnostics::{IsolationLevel, SourceMap}; use ruff_text_size::{Ranged, TextLen, TextRange, TextSize}; @@ -59,13 +59,13 @@ fn apply_fixes<'a>( let mut last_pos: Option = None; let mut applied: BTreeSet<&Edit> = BTreeSet::default(); let mut isolated: FxHashSet = FxHashSet::default(); - let mut fixed = FxHashMap::default(); + let mut fixed = FixTable::default(); let mut source_map = SourceMap::default(); - for (rule, fix) in diagnostics - .filter_map(|msg| msg.to_rule().map(|rule| (rule, msg))) - .filter_map(|(rule, diagnostic)| diagnostic.fix().map(|fix| (rule, fix))) - .sorted_by(|(rule1, fix1), (rule2, fix2)| cmp_fix(*rule1, *rule2, fix1, fix2)) + for (code, name, fix) in diagnostics + .filter_map(|msg| msg.noqa_code().map(|code| (code, msg.name(), msg))) + .filter_map(|(code, name, diagnostic)| diagnostic.fix().map(|fix| (code, name, fix))) + .sorted_by(|(_, name1, fix1), (_, name2, fix2)| cmp_fix(name1, name2, fix1, fix2)) { let mut edits = fix .edits() @@ -110,7 +110,7 @@ fn apply_fixes<'a>( } applied.extend(applied_edits.drain(..)); - *fixed.entry(rule).or_default() += 1; + *fixed.entry(code).or_default(name) += 1; } // Add the remaining content. @@ -125,34 +125,44 @@ fn apply_fixes<'a>( } /// Compare two fixes. -fn cmp_fix(rule1: Rule, rule2: Rule, fix1: &Fix, fix2: &Fix) -> std::cmp::Ordering { +fn cmp_fix(name1: &str, name2: &str, fix1: &Fix, fix2: &Fix) -> std::cmp::Ordering { // Always apply `RedefinedWhileUnused` before `UnusedImport`, as the latter can end up fixing // the former. But we can't apply this just for `RedefinedWhileUnused` and `UnusedImport` because it violates // `< is transitive: a < b and b < c implies a < c. The same must hold for both == and >.` // See https://github.com/astral-sh/ruff/issues/12469#issuecomment-2244392085 - match (rule1, rule2) { - (Rule::RedefinedWhileUnused, Rule::RedefinedWhileUnused) => std::cmp::Ordering::Equal, - (Rule::RedefinedWhileUnused, _) => std::cmp::Ordering::Less, - (_, Rule::RedefinedWhileUnused) => std::cmp::Ordering::Greater, - _ => std::cmp::Ordering::Equal, + let redefined_while_unused = Rule::RedefinedWhileUnused.name().as_str(); + if (name1, name2) == (redefined_while_unused, redefined_while_unused) { + std::cmp::Ordering::Equal + } else if name1 == redefined_while_unused { + std::cmp::Ordering::Less + } else if name2 == redefined_while_unused { + std::cmp::Ordering::Greater + } else { + std::cmp::Ordering::Equal } // Apply fixes in order of their start position. .then_with(|| fix1.min_start().cmp(&fix2.min_start())) // Break ties in the event of overlapping rules, for some specific combinations. - .then_with(|| match (&rule1, &rule2) { + .then_with(|| { + let rules = (name1, name2); // Apply `MissingTrailingPeriod` fixes before `NewLineAfterLastParagraph` fixes. - (Rule::MissingTrailingPeriod, Rule::NewLineAfterLastParagraph) => std::cmp::Ordering::Less, - (Rule::NewLineAfterLastParagraph, Rule::MissingTrailingPeriod) => { + let missing_trailing_period = Rule::MissingTrailingPeriod.name().as_str(); + let newline_after_last_paragraph = Rule::NewLineAfterLastParagraph.name().as_str(); + let if_else_instead_of_dict_get = Rule::IfElseBlockInsteadOfDictGet.name().as_str(); + let if_else_instead_of_if_exp = Rule::IfElseBlockInsteadOfIfExp.name().as_str(); + if rules == (missing_trailing_period, newline_after_last_paragraph) { + std::cmp::Ordering::Less + } else if rules == (newline_after_last_paragraph, missing_trailing_period) { std::cmp::Ordering::Greater } // Apply `IfElseBlockInsteadOfDictGet` fixes before `IfElseBlockInsteadOfIfExp` fixes. - (Rule::IfElseBlockInsteadOfDictGet, Rule::IfElseBlockInsteadOfIfExp) => { + else if rules == (if_else_instead_of_dict_get, if_else_instead_of_if_exp) { std::cmp::Ordering::Less - } - (Rule::IfElseBlockInsteadOfIfExp, Rule::IfElseBlockInsteadOfDictGet) => { + } else if rules == (if_else_instead_of_if_exp, if_else_instead_of_dict_get) { std::cmp::Ordering::Greater + } else { + std::cmp::Ordering::Equal } - _ => std::cmp::Ordering::Equal, }) } @@ -197,7 +207,7 @@ mod tests { source_map, } = apply_fixes(diagnostics.iter(), &locator); assert_eq!(code, ""); - assert_eq!(fixes.values().sum::(), 0); + assert_eq!(fixes.counts().sum::(), 0); assert!(source_map.markers().is_empty()); } @@ -234,7 +244,7 @@ print("hello world") "# .trim() ); - assert_eq!(fixes.values().sum::(), 1); + assert_eq!(fixes.counts().sum::(), 1); assert_eq!( source_map.markers(), &[ @@ -275,7 +285,7 @@ class A(Bar): " .trim(), ); - assert_eq!(fixes.values().sum::(), 1); + assert_eq!(fixes.counts().sum::(), 1); assert_eq!( source_map.markers(), &[ @@ -312,7 +322,7 @@ class A: " .trim() ); - assert_eq!(fixes.values().sum::(), 1); + assert_eq!(fixes.counts().sum::(), 1); assert_eq!( source_map.markers(), &[ @@ -353,7 +363,7 @@ class A(object): " .trim() ); - assert_eq!(fixes.values().sum::(), 2); + assert_eq!(fixes.counts().sum::(), 2); assert_eq!( source_map.markers(), &[ @@ -395,7 +405,7 @@ class A: " .trim(), ); - assert_eq!(fixes.values().sum::(), 1); + assert_eq!(fixes.counts().sum::(), 1); assert_eq!( source_map.markers(), &[ diff --git a/crates/ruff_linter/src/linter.rs b/crates/ruff_linter/src/linter.rs index 99e35681ec..2b31495b2a 100644 --- a/crates/ruff_linter/src/linter.rs +++ b/crates/ruff_linter/src/linter.rs @@ -1,4 +1,5 @@ use std::borrow::Cow; +use std::collections::hash_map::Entry; use std::path::Path; use anyhow::{Result, anyhow}; @@ -22,6 +23,7 @@ use crate::checkers::imports::check_imports; use crate::checkers::noqa::check_noqa; use crate::checkers::physical_lines::check_physical_lines; use crate::checkers::tokens::check_tokens; +use crate::codes::NoqaCode; use crate::directives::Directives; use crate::doc_lines::{doc_lines_from_ast, doc_lines_from_tokens}; use crate::fix::{FixResult, fix_file}; @@ -84,7 +86,53 @@ impl LinterResult { } } -pub type FixTable = FxHashMap; +#[derive(Debug, Default, PartialEq)] +struct FixCount { + rule_name: &'static str, + count: usize, +} + +/// A mapping from a noqa code to the corresponding lint name and a count of applied fixes. +#[derive(Debug, Default, PartialEq)] +pub struct FixTable(FxHashMap); + +impl FixTable { + pub fn counts(&self) -> impl Iterator { + self.0.values().map(|fc| fc.count) + } + + pub fn entry(&mut self, code: NoqaCode) -> FixTableEntry { + FixTableEntry(self.0.entry(code)) + } + + pub fn iter(&self) -> impl Iterator { + self.0 + .iter() + .map(|(code, FixCount { rule_name, count })| (*code, *rule_name, *count)) + } + + pub fn keys(&self) -> impl Iterator { + self.0.keys().copied() + } + + pub fn is_empty(&self) -> bool { + self.0.is_empty() + } +} + +pub struct FixTableEntry<'a>(Entry<'a, NoqaCode, FixCount>); + +impl<'a> FixTableEntry<'a> { + pub fn or_default(self, rule_name: &'static str) -> &'a mut usize { + &mut (self + .0 + .or_insert(FixCount { + rule_name, + count: 0, + }) + .count) + } +} pub struct FixerResult<'a> { /// The result returned by the linter, after applying any fixes. @@ -581,7 +629,7 @@ pub fn lint_fix<'a>( let mut transformed = Cow::Borrowed(source_kind); // Track the number of fixed errors across iterations. - let mut fixed = FxHashMap::default(); + let mut fixed = FixTable::default(); // As an escape hatch, bail after 100 iterations. let mut iterations = 0; @@ -650,12 +698,7 @@ pub fn lint_fix<'a>( // syntax error. Return the original code. if has_valid_syntax && has_no_syntax_errors { if let Some(error) = parsed.errors().first() { - report_fix_syntax_error( - path, - transformed.source_code(), - error, - fixed.keys().copied(), - ); + report_fix_syntax_error(path, transformed.source_code(), error, fixed.keys()); return Err(anyhow!("Fix introduced a syntax error")); } } @@ -670,8 +713,8 @@ pub fn lint_fix<'a>( { if iterations < MAX_ITERATIONS { // Count the number of fixed errors. - for (rule, count) in applied { - *fixed.entry(rule).or_default() += count; + for (rule, name, count) in applied.iter() { + *fixed.entry(rule).or_default(name) += count; } transformed = Cow::Owned(transformed.updated(fixed_contents, &source_map)); @@ -698,10 +741,10 @@ pub fn lint_fix<'a>( } } -fn collect_rule_codes(rules: impl IntoIterator) -> String { +fn collect_rule_codes(rules: impl IntoIterator) -> String { rules .into_iter() - .map(|rule| rule.noqa_code().to_string()) + .map(|rule| rule.to_string()) .sorted_unstable() .dedup() .join(", ") @@ -709,7 +752,7 @@ fn collect_rule_codes(rules: impl IntoIterator) -> String { #[expect(clippy::print_stderr)] fn report_failed_to_converge_error(path: &Path, transformed: &str, messages: &[Message]) { - let codes = collect_rule_codes(messages.iter().filter_map(Message::to_rule)); + let codes = collect_rule_codes(messages.iter().filter_map(Message::noqa_code)); if cfg!(debug_assertions) { eprintln!( "{}{} Failed to converge after {} iterations in `{}` with rule codes {}:---\n{}\n---", @@ -745,7 +788,7 @@ fn report_fix_syntax_error( path: &Path, transformed: &str, error: &ParseError, - rules: impl IntoIterator, + rules: impl IntoIterator, ) { let codes = collect_rule_codes(rules); if cfg!(debug_assertions) { diff --git a/crates/ruff_linter/src/message/azure.rs b/crates/ruff_linter/src/message/azure.rs index 107224d61e..1f0fe2c5b5 100644 --- a/crates/ruff_linter/src/message/azure.rs +++ b/crates/ruff_linter/src/message/azure.rs @@ -33,7 +33,7 @@ impl Emitter for AzureEmitter { line = location.line, col = location.column, code = message - .to_noqa_code() + .noqa_code() .map_or_else(String::new, |code| format!("code={code};")), body = message.body(), )?; diff --git a/crates/ruff_linter/src/message/github.rs b/crates/ruff_linter/src/message/github.rs index 748af78964..5c31dc134c 100644 --- a/crates/ruff_linter/src/message/github.rs +++ b/crates/ruff_linter/src/message/github.rs @@ -33,7 +33,7 @@ impl Emitter for GithubEmitter { writer, "::error title=Ruff{code},file={file},line={row},col={column},endLine={end_row},endColumn={end_column}::", code = message - .to_noqa_code() + .noqa_code() .map_or_else(String::new, |code| format!(" ({code})")), file = message.filename(), row = source_location.line, @@ -50,7 +50,7 @@ impl Emitter for GithubEmitter { column = location.column, )?; - if let Some(code) = message.to_noqa_code() { + if let Some(code) = message.noqa_code() { write!(writer, " {code}")?; } diff --git a/crates/ruff_linter/src/message/gitlab.rs b/crates/ruff_linter/src/message/gitlab.rs index 3d7fd85e27..b04cd86ad4 100644 --- a/crates/ruff_linter/src/message/gitlab.rs +++ b/crates/ruff_linter/src/message/gitlab.rs @@ -90,7 +90,7 @@ impl Serialize for SerializedMessages<'_> { } fingerprints.insert(message_fingerprint); - let (description, check_name) = if let Some(code) = message.to_noqa_code() { + let (description, check_name) = if let Some(code) = message.noqa_code() { (message.body().to_string(), code.to_string()) } else { let description = message.body(); diff --git a/crates/ruff_linter/src/message/json.rs b/crates/ruff_linter/src/message/json.rs index 3492047f44..e116f55e96 100644 --- a/crates/ruff_linter/src/message/json.rs +++ b/crates/ruff_linter/src/message/json.rs @@ -81,8 +81,8 @@ pub(crate) fn message_to_json_value(message: &Message, context: &EmitterContext) } json!({ - "code": message.to_noqa_code().map(|code| code.to_string()), - "url": message.to_rule().and_then(|rule| rule.url()), + "code": message.noqa_code().map(|code| code.to_string()), + "url": message.to_url(), "message": message.body(), "fix": fix, "cell": notebook_cell_index, diff --git a/crates/ruff_linter/src/message/junit.rs b/crates/ruff_linter/src/message/junit.rs index 38575d93d3..d0bbaca515 100644 --- a/crates/ruff_linter/src/message/junit.rs +++ b/crates/ruff_linter/src/message/junit.rs @@ -59,7 +59,7 @@ impl Emitter for JunitEmitter { body = message.body() )); let mut case = TestCase::new( - if let Some(code) = message.to_noqa_code() { + if let Some(code) = message.noqa_code() { format!("org.ruff.{code}") } else { "org.ruff".to_string() diff --git a/crates/ruff_linter/src/message/mod.rs b/crates/ruff_linter/src/message/mod.rs index 49bd23cd1e..ce4f1041a6 100644 --- a/crates/ruff_linter/src/message/mod.rs +++ b/crates/ruff_linter/src/message/mod.rs @@ -224,30 +224,22 @@ impl Message { self.fix().is_some() } - /// Returns the [`Rule`] corresponding to the diagnostic message. - pub fn to_rule(&self) -> Option { - if self.is_syntax_error() { - None - } else { - Some(self.name().parse().expect("Expected a valid rule name")) - } - } - /// Returns the [`NoqaCode`] corresponding to the diagnostic message. - pub fn to_noqa_code(&self) -> Option { + pub fn noqa_code(&self) -> Option { self.noqa_code } /// Returns the URL for the rule documentation, if it exists. pub fn to_url(&self) -> Option { - // TODO(brent) Rule::url calls Rule::explanation, which calls ViolationMetadata::explain, - // which when derived (seems always to be the case?) is always `Some`, so I think it's - // pretty safe to inline the Rule::url implementation here, using `self.name()`: - // - // format!("{}/rules/{}", env!("CARGO_PKG_HOMEPAGE"), self.name()) - // - // at least in the case of diagnostics, I guess syntax errors will return None - self.to_rule().and_then(|rule| rule.url()) + if self.is_syntax_error() { + None + } else { + Some(format!( + "{}/rules/{}", + env!("CARGO_PKG_HOMEPAGE"), + self.name() + )) + } } /// Returns the filename for the message. diff --git a/crates/ruff_linter/src/message/pylint.rs b/crates/ruff_linter/src/message/pylint.rs index 60c7a182dc..a4bf41225f 100644 --- a/crates/ruff_linter/src/message/pylint.rs +++ b/crates/ruff_linter/src/message/pylint.rs @@ -26,7 +26,7 @@ impl Emitter for PylintEmitter { message.compute_start_location().line }; - let body = if let Some(code) = message.to_noqa_code() { + let body = if let Some(code) = message.noqa_code() { format!("[{code}] {body}", body = message.body()) } else { message.body().to_string() diff --git a/crates/ruff_linter/src/message/rdjson.rs b/crates/ruff_linter/src/message/rdjson.rs index 34c89d0bd0..28be271ee0 100644 --- a/crates/ruff_linter/src/message/rdjson.rs +++ b/crates/ruff_linter/src/message/rdjson.rs @@ -71,7 +71,7 @@ fn message_to_rdjson_value(message: &Message) -> Value { "range": rdjson_range(start_location, end_location), }, "code": { - "value": message.to_noqa_code().map(|code| code.to_string()), + "value": message.noqa_code().map(|code| code.to_string()), "url": message.to_url(), }, "suggestions": rdjson_suggestions(fix.edits(), &source_code), @@ -84,7 +84,7 @@ fn message_to_rdjson_value(message: &Message) -> Value { "range": rdjson_range(start_location, end_location), }, "code": { - "value": message.to_noqa_code().map(|code| code.to_string()), + "value": message.noqa_code().map(|code| code.to_string()), "url": message.to_url(), }, }) diff --git a/crates/ruff_linter/src/message/sarif.rs b/crates/ruff_linter/src/message/sarif.rs index 5b5021b531..b14a93497f 100644 --- a/crates/ruff_linter/src/message/sarif.rs +++ b/crates/ruff_linter/src/message/sarif.rs @@ -8,7 +8,7 @@ use serde_json::json; use ruff_source_file::OneIndexed; use crate::VERSION; -use crate::codes::Rule; +use crate::codes::NoqaCode; use crate::fs::normalize_path; use crate::message::{Emitter, EmitterContext, Message}; use crate::registry::{Linter, RuleNamespace}; @@ -27,7 +27,7 @@ impl Emitter for SarifEmitter { .map(SarifResult::from_message) .collect::>>()?; - let unique_rules: HashSet<_> = results.iter().filter_map(|result| result.rule).collect(); + let unique_rules: HashSet<_> = results.iter().filter_map(|result| result.code).collect(); let mut rules: Vec = unique_rules.into_iter().map(SarifRule::from).collect(); rules.sort_by(|a, b| a.code.cmp(&b.code)); @@ -61,13 +61,19 @@ struct SarifRule<'a> { url: Option, } -impl From for SarifRule<'_> { - fn from(rule: Rule) -> Self { - let code = rule.noqa_code().to_string(); - let (linter, _) = Linter::parse_code(&code).unwrap(); +impl From for SarifRule<'_> { + fn from(code: NoqaCode) -> Self { + let code_str = code.to_string(); + // This is a manual re-implementation of Rule::from_code, but we also want the Linter. This + // avoids calling Linter::parse_code twice. + let (linter, suffix) = Linter::parse_code(&code_str).unwrap(); + let rule = linter + .all_rules() + .find(|rule| rule.noqa_code().suffix() == suffix) + .expect("Expected a valid noqa code corresponding to a rule"); Self { name: rule.into(), - code, + code: code_str, linter: linter.name(), summary: rule.message_formats()[0], explanation: rule.explanation(), @@ -106,7 +112,7 @@ impl Serialize for SarifRule<'_> { #[derive(Debug)] struct SarifResult { - rule: Option, + code: Option, level: String, message: String, uri: String, @@ -123,7 +129,7 @@ impl SarifResult { let end_location = message.compute_end_location(); let path = normalize_path(&*message.filename()); Ok(Self { - rule: message.to_rule(), + code: message.noqa_code(), level: "error".to_string(), message: message.body().to_string(), uri: url::Url::from_file_path(&path) @@ -143,7 +149,7 @@ impl SarifResult { let end_location = message.compute_end_location(); let path = normalize_path(&*message.filename()); Ok(Self { - rule: message.to_rule(), + code: message.noqa_code(), level: "error".to_string(), message: message.body().to_string(), uri: path.display().to_string(), @@ -178,7 +184,7 @@ impl Serialize for SarifResult { } } }], - "ruleId": self.rule.map(|rule| rule.noqa_code().to_string()), + "ruleId": self.code.map(|code| code.to_string()), }) .serialize(serializer) } diff --git a/crates/ruff_linter/src/message/text.rs b/crates/ruff_linter/src/message/text.rs index 65b98d9c1b..c8f89fc495 100644 --- a/crates/ruff_linter/src/message/text.rs +++ b/crates/ruff_linter/src/message/text.rs @@ -151,7 +151,7 @@ impl Display for RuleCodeAndBody<'_> { if let Some(fix) = self.message.fix() { // Do not display an indicator for inapplicable fixes if fix.applies(self.unsafe_fixes.required_applicability()) { - if let Some(code) = self.message.to_noqa_code() { + if let Some(code) = self.message.noqa_code() { write!(f, "{} ", code.to_string().red().bold())?; } return write!( @@ -164,7 +164,7 @@ impl Display for RuleCodeAndBody<'_> { } } - if let Some(code) = self.message.to_noqa_code() { + if let Some(code) = self.message.noqa_code() { write!( f, "{code} {body}", @@ -254,7 +254,7 @@ impl Display for MessageCodeFrame<'_> { let label = self .message - .to_noqa_code() + .noqa_code() .map_or_else(String::new, |code| code.to_string()); let line_start = self.notebook_index.map_or_else( diff --git a/crates/ruff_linter/src/noqa.rs b/crates/ruff_linter/src/noqa.rs index a7ff0c0c45..3d7ada3a11 100644 --- a/crates/ruff_linter/src/noqa.rs +++ b/crates/ruff_linter/src/noqa.rs @@ -12,13 +12,14 @@ use log::warn; use ruff_python_trivia::{CommentRanges, Cursor, indentation_at_offset}; use ruff_source_file::{LineEnding, LineRanges}; use ruff_text_size::{Ranged, TextLen, TextRange, TextSize}; +use rustc_hash::FxHashSet; use crate::Edit; use crate::Locator; use crate::codes::NoqaCode; use crate::fs::relativize_path; use crate::message::Message; -use crate::registry::{Rule, RuleSet}; +use crate::registry::Rule; use crate::rule_redirects::get_redirect_target; /// Generates an array of edits that matches the length of `messages`. @@ -780,7 +781,7 @@ fn build_noqa_edits_by_diagnostic( if let Some(noqa_edit) = generate_noqa_edit( comment.directive, comment.line, - RuleSet::from_rule(comment.rule), + FxHashSet::from_iter([comment.code]), locator, line_ending, ) { @@ -816,7 +817,7 @@ fn build_noqa_edits_by_line<'a>( offset, matches .into_iter() - .map(|NoqaComment { rule, .. }| rule) + .map(|NoqaComment { code, .. }| code) .collect(), locator, line_ending, @@ -829,7 +830,7 @@ fn build_noqa_edits_by_line<'a>( struct NoqaComment<'a> { line: TextSize, - rule: Rule, + code: NoqaCode, directive: Option<&'a Directive<'a>>, } @@ -845,13 +846,11 @@ fn find_noqa_comments<'a>( // Mark any non-ignored diagnostics. for message in messages { - let Some(rule) = message.to_rule() else { + let Some(code) = message.noqa_code() else { comments_by_line.push(None); continue; }; - let code = rule.noqa_code(); - match &exemption { FileExemption::All(_) => { // If the file is exempted, don't add any noqa directives. @@ -900,7 +899,7 @@ fn find_noqa_comments<'a>( if !codes.includes(code) { comments_by_line.push(Some(NoqaComment { line: directive_line.start(), - rule, + code, directive: Some(directive), })); } @@ -912,7 +911,7 @@ fn find_noqa_comments<'a>( // There's no existing noqa directive that suppresses the diagnostic. comments_by_line.push(Some(NoqaComment { line: locator.line_start(noqa_offset), - rule, + code, directive: None, })); } @@ -922,7 +921,7 @@ fn find_noqa_comments<'a>( struct NoqaEdit<'a> { edit_range: TextRange, - rules: RuleSet, + noqa_codes: FxHashSet, codes: Option<&'a Codes<'a>>, line_ending: LineEnding, } @@ -941,18 +940,15 @@ impl NoqaEdit<'_> { Some(codes) => { push_codes( writer, - self.rules + self.noqa_codes .iter() - .map(|rule| rule.noqa_code().to_string()) + .map(ToString::to_string) .chain(codes.iter().map(ToString::to_string)) .sorted_unstable(), ); } None => { - push_codes( - writer, - self.rules.iter().map(|rule| rule.noqa_code().to_string()), - ); + push_codes(writer, self.noqa_codes.iter().map(ToString::to_string)); } } write!(writer, "{}", self.line_ending.as_str()).unwrap(); @@ -968,7 +964,7 @@ impl Ranged for NoqaEdit<'_> { fn generate_noqa_edit<'a>( directive: Option<&'a Directive>, offset: TextSize, - rules: RuleSet, + noqa_codes: FxHashSet, locator: &Locator, line_ending: LineEnding, ) -> Option> { @@ -997,7 +993,7 @@ fn generate_noqa_edit<'a>( Some(NoqaEdit { edit_range, - rules, + noqa_codes, codes, line_ending, }) diff --git a/crates/ruff_linter/src/registry.rs b/crates/ruff_linter/src/registry.rs index 9b03a8b7f3..9351cb5de2 100644 --- a/crates/ruff_linter/src/registry.rs +++ b/crates/ruff_linter/src/registry.rs @@ -1,6 +1,7 @@ //! Remnant of the registry of all [`Rule`] implementations, now it's reexporting from codes.rs //! with some helper symbols +use ruff_db::diagnostic::LintName; use strum_macros::EnumIter; pub use codes::Rule; @@ -348,9 +349,18 @@ impl Rule { /// Return the URL for the rule documentation, if it exists. pub fn url(&self) -> Option { - self.explanation() - .is_some() - .then(|| format!("{}/rules/{}", env!("CARGO_PKG_HOMEPAGE"), self.as_ref())) + self.explanation().is_some().then(|| { + format!( + "{}/rules/{name}", + env!("CARGO_PKG_HOMEPAGE"), + name = self.name() + ) + }) + } + + pub fn name(&self) -> LintName { + let name: &'static str = self.into(); + LintName::of(name) } } @@ -421,7 +431,7 @@ pub mod clap_completion { fn possible_values(&self) -> Option + '_>> { Some(Box::new(Rule::iter().map(|rule| { let name = rule.noqa_code().to_string(); - let help = rule.as_ref().to_string(); + let help = rule.name().as_str(); PossibleValue::new(name).help(help) }))) } @@ -443,7 +453,7 @@ mod tests { assert!( rule.explanation().is_some(), "Rule {} is missing documentation", - rule.as_ref() + rule.name() ); } } @@ -460,10 +470,10 @@ mod tests { .collect(); for rule in Rule::iter() { - let rule_name = rule.as_ref(); + let rule_name = rule.name(); for pattern in &patterns { assert!( - !pattern.matches(rule_name), + !pattern.matches(&rule_name), "{rule_name} does not match naming convention, see CONTRIBUTING.md" ); } diff --git a/crates/ruff_linter/src/registry/rule_set.rs b/crates/ruff_linter/src/registry/rule_set.rs index d83ff07712..62601d7229 100644 --- a/crates/ruff_linter/src/registry/rule_set.rs +++ b/crates/ruff_linter/src/registry/rule_set.rs @@ -302,9 +302,8 @@ impl Display for RuleSet { } else { writeln!(f, "[")?; for rule in self { - let name = rule.as_ref(); let code = rule.noqa_code(); - writeln!(f, "\t{name} ({code}),")?; + writeln!(f, "\t{name} ({code}),", name = rule.name())?; } write!(f, "]")?; } diff --git a/crates/ruff_linter/src/rule_selector.rs b/crates/ruff_linter/src/rule_selector.rs index 7588aa9f64..74f069b976 100644 --- a/crates/ruff_linter/src/rule_selector.rs +++ b/crates/ruff_linter/src/rule_selector.rs @@ -485,8 +485,7 @@ pub mod clap_completion { prefix.linter().common_prefix(), prefix.short_code() ); - let name: &'static str = rule.into(); - return Some(PossibleValue::new(code).help(name)); + return Some(PossibleValue::new(code).help(rule.name().as_str())); } None diff --git a/crates/ruff_linter/src/rules/fastapi/mod.rs b/crates/ruff_linter/src/rules/fastapi/mod.rs index 90f52b7d66..60f1809737 100644 --- a/crates/ruff_linter/src/rules/fastapi/mod.rs +++ b/crates/ruff_linter/src/rules/fastapi/mod.rs @@ -3,7 +3,6 @@ pub(crate) mod rules; #[cfg(test)] mod tests { - use std::convert::AsRef; use std::path::Path; use anyhow::Result; @@ -18,7 +17,7 @@ mod tests { #[test_case(Rule::FastApiNonAnnotatedDependency, Path::new("FAST002_1.py"))] #[test_case(Rule::FastApiUnusedPathParameter, Path::new("FAST003.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); + let snapshot = format!("{}_{}", rule_code.name(), path.to_string_lossy()); let diagnostics = test_path( Path::new("fastapi").join(path).as_path(), &settings::LinterSettings::for_rule(rule_code), @@ -32,7 +31,7 @@ mod tests { #[test_case(Rule::FastApiNonAnnotatedDependency, Path::new("FAST002_0.py"))] #[test_case(Rule::FastApiNonAnnotatedDependency, Path::new("FAST002_1.py"))] fn rules_py38(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!("{}_{}_py38", rule_code.as_ref(), path.to_string_lossy()); + let snapshot = format!("{}_{}_py38", rule_code.name(), path.to_string_lossy()); let diagnostics = test_path( Path::new("fastapi").join(path).as_path(), &settings::LinterSettings { diff --git a/crates/ruff_linter/src/rules/flake8_fixme/mod.rs b/crates/ruff_linter/src/rules/flake8_fixme/mod.rs index d2433fc8b0..44071d1b78 100644 --- a/crates/ruff_linter/src/rules/flake8_fixme/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_fixme/mod.rs @@ -16,7 +16,7 @@ mod tests { #[test_case(Rule::LineContainsTodo; "T003")] #[test_case(Rule::LineContainsXxx; "T004")] fn rules(rule_code: Rule) -> Result<()> { - let snapshot = format!("{}_T00.py", rule_code.as_ref()); + let snapshot = format!("{}_T00.py", rule_code.name()); let diagnostics = test_path( Path::new("flake8_fixme/T00.py"), &settings::LinterSettings::for_rule(rule_code), diff --git a/crates/ruff_linter/src/rules/flake8_gettext/mod.rs b/crates/ruff_linter/src/rules/flake8_gettext/mod.rs index 09d440526e..cd385932ac 100644 --- a/crates/ruff_linter/src/rules/flake8_gettext/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_gettext/mod.rs @@ -29,7 +29,7 @@ mod tests { #[test_case(Rule::FormatInGetTextFuncCall, Path::new("INT002.py"))] #[test_case(Rule::PrintfInGetTextFuncCall, Path::new("INT003.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); + let snapshot = format!("{}_{}", rule_code.name(), path.to_string_lossy()); let diagnostics = test_path( Path::new("flake8_gettext").join(path).as_path(), &settings::LinterSettings::for_rule(rule_code), diff --git a/crates/ruff_linter/src/rules/flake8_raise/mod.rs b/crates/ruff_linter/src/rules/flake8_raise/mod.rs index d2d636b1e8..1780454533 100644 --- a/crates/ruff_linter/src/rules/flake8_raise/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_raise/mod.rs @@ -3,7 +3,6 @@ pub(crate) mod rules; #[cfg(test)] mod tests { - use std::convert::AsRef; use std::path::Path; use anyhow::Result; @@ -15,7 +14,7 @@ mod tests { #[test_case(Rule::UnnecessaryParenOnRaiseException, Path::new("RSE102.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); + let snapshot = format!("{}_{}", rule_code.name(), path.to_string_lossy()); let diagnostics = test_path( Path::new("flake8_raise").join(path).as_path(), &settings::LinterSettings::for_rule(rule_code), diff --git a/crates/ruff_linter/src/rules/flake8_self/mod.rs b/crates/ruff_linter/src/rules/flake8_self/mod.rs index a445b40baf..7a3f83ddd3 100644 --- a/crates/ruff_linter/src/rules/flake8_self/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_self/mod.rs @@ -4,7 +4,6 @@ pub mod settings; #[cfg(test)] mod tests { - use std::convert::AsRef; use std::path::Path; use crate::registry::Rule; @@ -18,7 +17,7 @@ mod tests { #[test_case(Rule::PrivateMemberAccess, Path::new("SLF001.py"))] #[test_case(Rule::PrivateMemberAccess, Path::new("SLF001_1.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); + let snapshot = format!("{}_{}", rule_code.name(), path.to_string_lossy()); let diagnostics = test_path( Path::new("flake8_self").join(path).as_path(), &settings::LinterSettings::for_rule(rule_code), diff --git a/crates/ruff_linter/src/rules/flake8_todos/mod.rs b/crates/ruff_linter/src/rules/flake8_todos/mod.rs index 7a4129a78b..486c46af04 100644 --- a/crates/ruff_linter/src/rules/flake8_todos/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_todos/mod.rs @@ -2,7 +2,6 @@ pub(crate) mod rules; #[cfg(test)] mod tests { - use std::convert::AsRef; use std::path::Path; use anyhow::Result; @@ -20,7 +19,7 @@ mod tests { #[test_case(Rule::InvalidTodoCapitalization, Path::new("TD006.py"))] #[test_case(Rule::MissingSpaceAfterTodoColon, Path::new("TD007.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); + let snapshot = format!("{}_{}", rule_code.name(), path.to_string_lossy()); let diagnostics = test_path( Path::new("flake8_todos").join(path).as_path(), &settings::LinterSettings::for_rule(rule_code), diff --git a/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs b/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs index f5b777ec46..acdb788f52 100644 --- a/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_type_checking/mod.rs @@ -6,7 +6,6 @@ pub mod settings; #[cfg(test)] mod tests { - use std::convert::AsRef; use std::path::Path; use anyhow::Result; @@ -55,7 +54,7 @@ mod tests { #[test_case(Rule::TypingOnlyThirdPartyImport, Path::new("typing_modules_1.py"))] #[test_case(Rule::TypingOnlyThirdPartyImport, Path::new("typing_modules_2.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); + let snapshot = format!("{}_{}", rule_code.name(), path.to_string_lossy()); let diagnostics = test_path( Path::new("flake8_type_checking").join(path).as_path(), &settings::LinterSettings::for_rule(rule_code), @@ -70,7 +69,7 @@ mod tests { #[test_case(Rule::QuotedTypeAlias, Path::new("TC008.py"))] #[test_case(Rule::QuotedTypeAlias, Path::new("TC008_typing_execution_context.py"))] fn type_alias_rules(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); + let snapshot = format!("{}_{}", rule_code.name(), path.to_string_lossy()); let diagnostics = test_path( Path::new("flake8_type_checking").join(path).as_path(), &settings::LinterSettings::for_rules(vec![ @@ -84,11 +83,7 @@ mod tests { #[test_case(Rule::QuotedTypeAlias, Path::new("TC008_union_syntax_pre_py310.py"))] fn type_alias_rules_pre_py310(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!( - "pre_py310_{}_{}", - rule_code.as_ref(), - path.to_string_lossy() - ); + let snapshot = format!("pre_py310_{}_{}", rule_code.name(), path.to_string_lossy()); let diagnostics = test_path( Path::new("flake8_type_checking").join(path).as_path(), &settings::LinterSettings { @@ -107,7 +102,7 @@ mod tests { #[test_case(Rule::RuntimeImportInTypeCheckingBlock, Path::new("quote3.py"))] #[test_case(Rule::TypingOnlyThirdPartyImport, Path::new("quote3.py"))] fn quote(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!("quote_{}_{}", rule_code.as_ref(), path.to_string_lossy()); + let snapshot = format!("quote_{}_{}", rule_code.name(), path.to_string_lossy()); let diagnostics = test_path( Path::new("flake8_type_checking").join(path).as_path(), &settings::LinterSettings { @@ -126,7 +121,7 @@ mod tests { #[test_case(Rule::TypingOnlyStandardLibraryImport, Path::new("init_var.py"))] #[test_case(Rule::TypingOnlyStandardLibraryImport, Path::new("kw_only.py"))] fn strict(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!("strict_{}_{}", rule_code.as_ref(), path.to_string_lossy()); + let snapshot = format!("strict_{}_{}", rule_code.name(), path.to_string_lossy()); let diagnostics = test_path( Path::new("flake8_type_checking").join(path).as_path(), &settings::LinterSettings { @@ -170,7 +165,7 @@ mod tests { Path::new("exempt_type_checking_3.py") )] fn exempt_type_checking(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); + let snapshot = format!("{}_{}", rule_code.name(), path.to_string_lossy()); let diagnostics = test_path( Path::new("flake8_type_checking").join(path).as_path(), &settings::LinterSettings { @@ -207,7 +202,7 @@ mod tests { Path::new("runtime_evaluated_base_classes_5.py") )] fn runtime_evaluated_base_classes(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); + let snapshot = format!("{}_{}", rule_code.name(), path.to_string_lossy()); let diagnostics = test_path( Path::new("flake8_type_checking").join(path).as_path(), &settings::LinterSettings { @@ -238,7 +233,7 @@ mod tests { Path::new("runtime_evaluated_decorators_3.py") )] fn runtime_evaluated_decorators(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); + let snapshot = format!("{}_{}", rule_code.name(), path.to_string_lossy()); let diagnostics = test_path( Path::new("flake8_type_checking").join(path).as_path(), &settings::LinterSettings { @@ -264,7 +259,7 @@ mod tests { Path::new("module/undefined.py") )] fn base_class_same_file(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); + let snapshot = format!("{}_{}", rule_code.name(), path.to_string_lossy()); let diagnostics = test_path( Path::new("flake8_type_checking").join(path).as_path(), &settings::LinterSettings { @@ -282,7 +277,7 @@ mod tests { #[test_case(Rule::RuntimeImportInTypeCheckingBlock, Path::new("module/app.py"))] #[test_case(Rule::TypingOnlyStandardLibraryImport, Path::new("module/routes.py"))] fn decorator_same_file(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); + let snapshot = format!("{}_{}", rule_code.name(), path.to_string_lossy()); let diagnostics = test_path( Path::new("flake8_type_checking").join(path).as_path(), &settings::LinterSettings { diff --git a/crates/ruff_linter/src/rules/numpy/mod.rs b/crates/ruff_linter/src/rules/numpy/mod.rs index 7a313eac2e..9403f25016 100644 --- a/crates/ruff_linter/src/rules/numpy/mod.rs +++ b/crates/ruff_linter/src/rules/numpy/mod.rs @@ -4,7 +4,6 @@ pub(crate) mod rules; #[cfg(test)] mod tests { - use std::convert::AsRef; use std::path::Path; use anyhow::Result; @@ -22,7 +21,7 @@ mod tests { #[test_case(Rule::Numpy2Deprecation, Path::new("NPY201_2.py"))] #[test_case(Rule::Numpy2Deprecation, Path::new("NPY201_3.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); + let snapshot = format!("{}_{}", rule_code.name(), path.to_string_lossy()); let diagnostics = test_path( Path::new("numpy").join(path).as_path(), &settings::LinterSettings::for_rule(rule_code), diff --git a/crates/ruff_linter/src/rules/pydoclint/mod.rs b/crates/ruff_linter/src/rules/pydoclint/mod.rs index af4131e427..6e2ff94021 100644 --- a/crates/ruff_linter/src/rules/pydoclint/mod.rs +++ b/crates/ruff_linter/src/rules/pydoclint/mod.rs @@ -4,7 +4,6 @@ pub mod settings; #[cfg(test)] mod tests { - use std::convert::AsRef; use std::path::Path; use anyhow::Result; @@ -20,7 +19,7 @@ mod tests { #[test_case(Rule::DocstringMissingException, Path::new("DOC501.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); + let snapshot = format!("{}_{}", rule_code.name(), path.to_string_lossy()); let diagnostics = test_path( Path::new("pydoclint").join(path).as_path(), &settings::LinterSettings::for_rule(rule_code), @@ -36,7 +35,7 @@ mod tests { #[test_case(Rule::DocstringMissingException, Path::new("DOC501_google.py"))] #[test_case(Rule::DocstringExtraneousException, Path::new("DOC502_google.py"))] fn rules_google_style(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); + let snapshot = format!("{}_{}", rule_code.name(), path.to_string_lossy()); let diagnostics = test_path( Path::new("pydoclint").join(path).as_path(), &settings::LinterSettings { @@ -58,7 +57,7 @@ mod tests { #[test_case(Rule::DocstringMissingException, Path::new("DOC501_numpy.py"))] #[test_case(Rule::DocstringExtraneousException, Path::new("DOC502_numpy.py"))] fn rules_numpy_style(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); + let snapshot = format!("{}_{}", rule_code.name(), path.to_string_lossy()); let diagnostics = test_path( Path::new("pydoclint").join(path).as_path(), &settings::LinterSettings { @@ -79,7 +78,7 @@ mod tests { fn rules_google_style_ignore_one_line(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!( "{}_{}_ignore_one_line", - rule_code.as_ref(), + rule_code.name(), path.to_string_lossy() ); let diagnostics = test_path( diff --git a/crates/ruff_linter/src/rules/pyflakes/mod.rs b/crates/ruff_linter/src/rules/pyflakes/mod.rs index ce2162e73d..51f3255395 100644 --- a/crates/ruff_linter/src/rules/pyflakes/mod.rs +++ b/crates/ruff_linter/src/rules/pyflakes/mod.rs @@ -776,8 +776,10 @@ mod tests { messages.sort_by_key(Ranged::start); let actual = messages .iter() - .filter_map(Message::to_rule) + .filter(|msg| !msg.is_syntax_error()) + .map(Message::name) .collect::>(); + let expected: Vec<_> = expected.iter().map(|rule| rule.name().as_str()).collect(); assert_eq!(actual, expected); } diff --git a/crates/ruff_linter/src/rules/tryceratops/mod.rs b/crates/ruff_linter/src/rules/tryceratops/mod.rs index 21f3a01382..8d431aae67 100644 --- a/crates/ruff_linter/src/rules/tryceratops/mod.rs +++ b/crates/ruff_linter/src/rules/tryceratops/mod.rs @@ -4,7 +4,6 @@ pub(crate) mod rules; #[cfg(test)] mod tests { - use std::convert::AsRef; use std::path::Path; use anyhow::Result; @@ -25,7 +24,7 @@ mod tests { #[test_case(Rule::ErrorInsteadOfException, Path::new("TRY400.py"))] #[test_case(Rule::VerboseLogMessage, Path::new("TRY401.py"))] fn rules(rule_code: Rule, path: &Path) -> Result<()> { - let snapshot = format!("{}_{}", rule_code.as_ref(), path.to_string_lossy()); + let snapshot = format!("{}_{}", rule_code.name(), path.to_string_lossy()); let diagnostics = test_path( Path::new("tryceratops").join(path).as_path(), &settings::LinterSettings::for_rule(rule_code), diff --git a/crates/ruff_linter/src/test.rs b/crates/ruff_linter/src/test.rs index 7148206df8..ed05d62673 100644 --- a/crates/ruff_linter/src/test.rs +++ b/crates/ruff_linter/src/test.rs @@ -20,6 +20,7 @@ use ruff_python_parser::{ParseError, ParseOptions}; use ruff_python_trivia::textwrap::dedent; use ruff_source_file::SourceFileBuilder; +use crate::codes::Rule; use crate::fix::{FixResult, fix_file}; use crate::linter::check_path; use crate::message::{Emitter, EmitterContext, Message, TextEmitter}; @@ -233,8 +234,9 @@ Source with applied fixes: let messages = messages .into_iter() - .filter_map(|msg| Some((msg.to_rule()?, msg))) - .map(|(rule, mut diagnostic)| { + .filter_map(|msg| Some((msg.noqa_code()?, msg))) + .map(|(code, mut diagnostic)| { + let rule = Rule::from_code(&code.to_string()).unwrap(); let fixable = diagnostic.fix().is_some_and(|fix| { matches!( fix.applicability(), diff --git a/crates/ruff_macros/src/map_codes.rs b/crates/ruff_macros/src/map_codes.rs index 7d1ccaf028..39993af6af 100644 --- a/crates/ruff_macros/src/map_codes.rs +++ b/crates/ruff_macros/src/map_codes.rs @@ -174,7 +174,7 @@ pub(crate) fn map_codes(func: &ItemFn) -> syn::Result { output.extend(quote! { impl #linter { - pub fn rules(&self) -> ::std::vec::IntoIter { + pub(crate) fn rules(&self) -> ::std::vec::IntoIter { match self { #prefix_into_iter_match_arms } } } @@ -182,7 +182,7 @@ pub(crate) fn map_codes(func: &ItemFn) -> syn::Result { } output.extend(quote! { impl RuleCodePrefix { - pub fn parse(linter: &Linter, code: &str) -> Result { + pub(crate) fn parse(linter: &Linter, code: &str) -> Result { use std::str::FromStr; Ok(match linter { @@ -190,7 +190,7 @@ pub(crate) fn map_codes(func: &ItemFn) -> syn::Result { }) } - pub fn rules(&self) -> ::std::vec::IntoIter { + pub(crate) fn rules(&self) -> ::std::vec::IntoIter { match self { #(RuleCodePrefix::#linter_idents(prefix) => prefix.clone().rules(),)* } @@ -319,7 +319,7 @@ See also https://github.com/astral-sh/ruff/issues/2186. matches!(self.group(), RuleGroup::Preview) } - pub fn is_stable(&self) -> bool { + pub(crate) fn is_stable(&self) -> bool { matches!(self.group(), RuleGroup::Stable) } @@ -371,7 +371,7 @@ fn generate_iter_impl( quote! { impl Linter { /// Rules not in the preview. - pub fn rules(self: &Linter) -> ::std::vec::IntoIter { + pub(crate) fn rules(self: &Linter) -> ::std::vec::IntoIter { match self { #linter_rules_match_arms } @@ -385,7 +385,7 @@ fn generate_iter_impl( } impl RuleCodePrefix { - pub fn iter() -> impl Iterator { + pub(crate) fn iter() -> impl Iterator { use strum::IntoEnumIterator; let mut prefixes = Vec::new(); @@ -436,7 +436,6 @@ fn register_rules<'a>(input: impl Iterator) -> TokenStream { PartialOrd, Ord, ::ruff_macros::CacheKey, - AsRefStr, ::strum_macros::IntoStaticStr, ::strum_macros::EnumString, ::serde::Serialize, diff --git a/crates/ruff_server/src/lint.rs b/crates/ruff_server/src/lint.rs index bf20172cba..92c11068de 100644 --- a/crates/ruff_server/src/lint.rs +++ b/crates/ruff_server/src/lint.rs @@ -242,7 +242,7 @@ fn to_lsp_diagnostic( let body = diagnostic.body().to_string(); let fix = diagnostic.fix(); let suggestion = diagnostic.suggestion(); - let code = diagnostic.to_noqa_code(); + let code = diagnostic.noqa_code(); let fix = fix.and_then(|fix| fix.applies(Applicability::Unsafe).then_some(fix)); diff --git a/crates/ruff_server/src/server/api/requests/hover.rs b/crates/ruff_server/src/server/api/requests/hover.rs index 6b391e15bc..846f3654c5 100644 --- a/crates/ruff_server/src/server/api/requests/hover.rs +++ b/crates/ruff_server/src/server/api/requests/hover.rs @@ -85,7 +85,7 @@ pub(crate) fn hover( fn format_rule_text(rule: Rule) -> String { let mut output = String::new(); - let _ = write!(&mut output, "# {} ({})", rule.as_ref(), rule.noqa_code()); + let _ = write!(&mut output, "# {} ({})", rule.name(), rule.noqa_code()); output.push('\n'); output.push('\n'); diff --git a/crates/ruff_wasm/src/lib.rs b/crates/ruff_wasm/src/lib.rs index 24e683042b..3548882503 100644 --- a/crates/ruff_wasm/src/lib.rs +++ b/crates/ruff_wasm/src/lib.rs @@ -208,7 +208,7 @@ impl Workspace { let messages: Vec = messages .into_iter() .map(|msg| ExpandedMessage { - code: msg.to_noqa_code().map(|code| code.to_string()), + code: msg.noqa_code().map(|code| code.to_string()), message: msg.body().to_string(), start_location: source_code.line_column(msg.start()).into(), end_location: source_code.line_column(msg.end()).into(), diff --git a/crates/ruff_workspace/src/configuration.rs b/crates/ruff_workspace/src/configuration.rs index 684a2b434d..ae0d231504 100644 --- a/crates/ruff_workspace/src/configuration.rs +++ b/crates/ruff_workspace/src/configuration.rs @@ -1098,7 +1098,7 @@ impl LintConfiguration { // approach to give each pair it's own `warn_user_once`. for (preferred, expendable, message) in INCOMPATIBLE_CODES { if rules.enabled(*preferred) && rules.enabled(*expendable) { - warn_user_once_by_id!(expendable.as_ref(), "{}", message); + warn_user_once_by_id!(expendable.name().as_str(), "{}", message); rules.disable(*expendable); } }