Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion crates/ruff/src/cache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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!(
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff/src/commands/rule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand All @@ -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');

Expand Down
12 changes: 6 additions & 6 deletions crates/ruff/src/diagnostics.rs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup in this commit!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
};

Expand Down Expand Up @@ -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 {
Expand All @@ -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)
};

Expand Down
18 changes: 9 additions & 9 deletions crates/ruff/src/printer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -80,7 +81,7 @@ impl Printer {
let fixed = diagnostics
.fixed
.values()
.flat_map(std::collections::HashMap::values)
.flat_map(FixTable::counts)
.sum::<usize>();

if self.flags.intersects(Flags::SHOW_VIOLATIONS) {
Expand Down Expand Up @@ -302,7 +303,7 @@ impl Printer {
let statistics: Vec<ExpandedStatistics> = 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![],
Expand Down Expand Up @@ -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::<usize>())
.map(|table| table.counts().sum::<usize>())
.sum::<usize>();
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(),
);
Expand All @@ -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(),
)?;
}
}
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_dev/src/generate_docs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_dev/src/generate_rules_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ fn generate_table(table_out: &mut String, rules: impl IntoIterator<Item = Rule>,
FixAvailability::None => format!("<span {SYMBOL_STYLE}></span>"),
};

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`
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
62 changes: 36 additions & 26 deletions crates/ruff_linter/src/fix/mod.rs
Original file line number Diff line number Diff line change
@@ -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};
Expand Down Expand Up @@ -59,13 +59,13 @@ fn apply_fixes<'a>(
let mut last_pos: Option<TextSize> = None;
let mut applied: BTreeSet<&Edit> = BTreeSet::default();
let mut isolated: FxHashSet<u32> = 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()
Expand Down Expand Up @@ -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.
Expand All @@ -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,
})
}

Expand Down Expand Up @@ -197,7 +207,7 @@ mod tests {
source_map,
} = apply_fixes(diagnostics.iter(), &locator);
assert_eq!(code, "");
assert_eq!(fixes.values().sum::<usize>(), 0);
assert_eq!(fixes.counts().sum::<usize>(), 0);
assert!(source_map.markers().is_empty());
}

Expand Down Expand Up @@ -234,7 +244,7 @@ print("hello world")
"#
.trim()
);
assert_eq!(fixes.values().sum::<usize>(), 1);
assert_eq!(fixes.counts().sum::<usize>(), 1);
assert_eq!(
source_map.markers(),
&[
Expand Down Expand Up @@ -275,7 +285,7 @@ class A(Bar):
"
.trim(),
);
assert_eq!(fixes.values().sum::<usize>(), 1);
assert_eq!(fixes.counts().sum::<usize>(), 1);
assert_eq!(
source_map.markers(),
&[
Expand Down Expand Up @@ -312,7 +322,7 @@ class A:
"
.trim()
);
assert_eq!(fixes.values().sum::<usize>(), 1);
assert_eq!(fixes.counts().sum::<usize>(), 1);
assert_eq!(
source_map.markers(),
&[
Expand Down Expand Up @@ -353,7 +363,7 @@ class A(object):
"
.trim()
);
assert_eq!(fixes.values().sum::<usize>(), 2);
assert_eq!(fixes.counts().sum::<usize>(), 2);
assert_eq!(
source_map.markers(),
&[
Expand Down Expand Up @@ -395,7 +405,7 @@ class A:
"
.trim(),
);
assert_eq!(fixes.values().sum::<usize>(), 1);
assert_eq!(fixes.counts().sum::<usize>(), 1);
assert_eq!(
source_map.markers(),
&[
Expand Down
Loading
Loading