Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 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
2 changes: 1 addition & 1 deletion crates/ruff/src/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ impl AddAssign for FixMap {
let fixed_in_file = self.0.entry(filename).or_default();
for (rule, name, count) in fixed.iter() {
if count > 0 {
*fixed_in_file.entry(rule).or_default(name) += count;
*fixed_in_file.entry(rule.to_string()).or_default(name) += count;
}
}
}
Expand Down
22 changes: 6 additions & 16 deletions crates/ruff/src/printer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ use anyhow::Result;
use bitflags::bitflags;
use colored::Colorize;
use itertools::{Itertools, iterate};
use ruff_linter::codes::NoqaCode;
use ruff_linter::linter::FixTable;
use serde::Serialize;

Expand Down Expand Up @@ -36,8 +35,8 @@ bitflags! {
}

#[derive(Serialize)]
struct ExpandedStatistics {
code: Option<NoqaCode>,
struct ExpandedStatistics<'a> {
code: Option<&'a str>,
name: &'static str,
count: usize,
fixable: bool,
Expand Down Expand Up @@ -303,11 +302,11 @@ impl Printer {
let statistics: Vec<ExpandedStatistics> = diagnostics
.inner
.iter()
.map(|message| (message.noqa_code(), message))
.map(|message| (message.secondary_code(), message))
.sorted_by_key(|(code, message)| (*code, message.fixable()))
.fold(
vec![],
|mut acc: Vec<((Option<NoqaCode>, &OldDiagnostic), usize)>, (code, message)| {
|mut acc: Vec<((Option<&str>, &OldDiagnostic), usize)>, (code, message)| {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should add a SecondaryCode struct which wraps a String I do find the &str makes the code harder to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I think if it wrapped a String we'd have to clone in OldDiagnostic::secondary_code right? I'll try it with a &str and see if we can use that everywhere. I definitely agree that it would be more readable, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I was responding to these too early in the morning 🤦 I can store the SecondaryCode(String) on the OldDiagnostic and then use &SecondaryCode where I'm currently using &str. I'm working on that, and then I'll try the Rule changes on a separate branch!

if let Some(((prev_code, _prev_message), count)) = acc.last_mut() {
if *prev_code == code {
*count += 1;
Expand Down Expand Up @@ -349,12 +348,7 @@ impl Printer {
);
let code_width = statistics
.iter()
.map(|statistic| {
statistic
.code
.map_or_else(String::new, |rule| rule.to_string())
.len()
})
.map(|statistic| statistic.code.map_or(0, str::len))
.max()
.unwrap();
let any_fixable = statistics.iter().any(|statistic| statistic.fixable);
Expand All @@ -368,11 +362,7 @@ impl Printer {
writer,
"{:>count_width$}\t{:<code_width$}\t{}{}",
statistic.count.to_string().bold(),
statistic
.code
.map_or_else(String::new, |rule| rule.to_string())
.red()
.bold(),
statistic.code.as_ref().unwrap_or(&"").red().bold(),
if any_fixable {
if statistic.fixable {
&fixable
Expand Down
32 changes: 11 additions & 21 deletions crates/ruff_linter/src/checkers/noqa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,27 +47,17 @@ pub(crate) fn check_noqa(
// Remove any ignored diagnostics.
'outer: for (index, diagnostic) in context.iter().enumerate() {
// Can't ignore syntax errors.
let Some(code) = diagnostic.noqa_code() else {
let Some(code) = diagnostic.secondary_code() else {
continue;
};

if code == Rule::BlanketNOQA.noqa_code() {
if Rule::BlanketNOQA.noqa_code() == code {
continue;
}

match &exemption {
FileExemption::All(_) => {
// If the file is exempted, ignore all diagnostics.
ignored_diagnostics.push(index);
continue;
}
FileExemption::Codes(codes) => {
// If the diagnostic is ignored by a global exemption, ignore it.
if codes.contains(&&code) {
ignored_diagnostics.push(index);
continue;
}
}
if exemption.contains(&code) {
ignored_diagnostics.push(index);
continue;
}

let noqa_offsets = diagnostic
Expand All @@ -82,13 +72,13 @@ pub(crate) fn check_noqa(
{
let suppressed = match &directive_line.directive {
Directive::All(_) => {
directive_line.matches.push(code);
directive_line.matches.push(code.to_string());
ignored_diagnostics.push(index);
true
}
Directive::Codes(directive) => {
if directive.includes(code) {
directive_line.matches.push(code);
if directive.includes(&code) {
directive_line.matches.push(code.to_string());
ignored_diagnostics.push(index);
true
} else {
Expand Down Expand Up @@ -147,9 +137,9 @@ pub(crate) fn check_noqa(

if seen_codes.insert(original_code) {
let is_code_used = if is_file_level {
context
.iter()
.any(|diag| diag.noqa_code().is_some_and(|noqa| noqa == code))
context.iter().any(|diag| {
diag.secondary_code().is_some_and(|noqa| noqa == code)
})
} else {
matches.iter().any(|match_| *match_ == code)
} || settings
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/fix/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ fn apply_fixes<'a>(
let mut source_map = SourceMap::default();

for (code, name, fix) in diagnostics
.filter_map(|msg| msg.noqa_code().map(|code| (code, msg.name(), msg)))
.filter_map(|msg| msg.secondary_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))
{
Expand Down Expand Up @@ -110,7 +110,7 @@ fn apply_fixes<'a>(
}

applied.extend(applied_edits.drain(..));
*fixed.entry(code).or_default(name) += 1;
*fixed.entry(code.to_string()).or_default(name) += 1;
}

// Add the remaining content.
Expand Down
32 changes: 13 additions & 19 deletions crates/ruff_linter/src/linter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ 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};
Expand Down Expand Up @@ -95,33 +94,33 @@ struct FixCount {

/// 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<NoqaCode, FixCount>);
pub struct FixTable(FxHashMap<String, FixCount>);

impl FixTable {
pub fn counts(&self) -> impl Iterator<Item = usize> {
self.0.values().map(|fc| fc.count)
}

pub fn entry(&mut self, code: NoqaCode) -> FixTableEntry {
pub fn entry(&mut self, code: String) -> FixTableEntry {
FixTableEntry(self.0.entry(code))
}

pub fn iter(&self) -> impl Iterator<Item = (NoqaCode, &'static str, usize)> {
pub fn iter(&self) -> impl Iterator<Item = (&str, &'static str, usize)> {
self.0
.iter()
.map(|(code, FixCount { rule_name, count })| (*code, *rule_name, *count))
.map(|(code, FixCount { rule_name, count })| (code.as_str(), *rule_name, *count))
}

pub fn keys(&self) -> impl Iterator<Item = NoqaCode> {
self.0.keys().copied()
pub fn keys(&self) -> impl Iterator<Item = &str> {
self.0.keys().map(String::as_str)
}

pub fn is_empty(&self) -> bool {
self.0.is_empty()
}
}

pub struct FixTableEntry<'a>(Entry<'a, NoqaCode, FixCount>);
pub struct FixTableEntry<'a>(Entry<'a, String, FixCount>);

impl<'a> FixTableEntry<'a> {
pub fn or_default(self, rule_name: &'static str) -> &'a mut usize {
Expand Down Expand Up @@ -651,7 +650,7 @@ pub fn lint_fix<'a>(
if iterations < MAX_ITERATIONS {
// Count the number of fixed errors.
for (rule, name, count) in applied.iter() {
*fixed.entry(rule).or_default(name) += count;
*fixed.entry(rule.to_string()).or_default(name) += count;
}

transformed = Cow::Owned(transformed.updated(fixed_contents, &source_map));
Expand All @@ -678,18 +677,13 @@ pub fn lint_fix<'a>(
}
}

fn collect_rule_codes(rules: impl IntoIterator<Item = NoqaCode>) -> String {
rules
.into_iter()
.map(|rule| rule.to_string())
.sorted_unstable()
.dedup()
.join(", ")
fn collect_rule_codes<'a>(rules: impl IntoIterator<Item = &'a str>) -> String {
rules.into_iter().sorted_unstable().dedup().join(", ")
}

#[expect(clippy::print_stderr)]
fn report_failed_to_converge_error(path: &Path, transformed: &str, diagnostics: &[OldDiagnostic]) {
let codes = collect_rule_codes(diagnostics.iter().filter_map(OldDiagnostic::noqa_code));
let codes = collect_rule_codes(diagnostics.iter().filter_map(OldDiagnostic::secondary_code));
if cfg!(debug_assertions) {
eprintln!(
"{}{} Failed to converge after {} iterations in `{}` with rule codes {}:---\n{}\n---",
Expand Down Expand Up @@ -721,11 +715,11 @@ This indicates a bug in Ruff. If you could open an issue at:
}

#[expect(clippy::print_stderr)]
fn report_fix_syntax_error(
fn report_fix_syntax_error<'a>(
path: &Path,
transformed: &str,
error: &ParseError,
rules: impl IntoIterator<Item = NoqaCode>,
rules: impl IntoIterator<Item = &'a str>,
) {
let codes = collect_rule_codes(rules);
if cfg!(debug_assertions) {
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/message/azure.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ impl Emitter for AzureEmitter {
line = location.line,
col = location.column,
code = diagnostic
.noqa_code()
.secondary_code()
.map_or_else(String::new, |code| format!("code={code};")),
body = diagnostic.body(),
)?;
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/message/github.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 = diagnostic
.noqa_code()
.secondary_code()
.map_or_else(String::new, |code| format!(" ({code})")),
file = diagnostic.filename(),
row = source_location.line,
Expand All @@ -50,7 +50,7 @@ impl Emitter for GithubEmitter {
column = location.column,
)?;

if let Some(code) = diagnostic.noqa_code() {
if let Some(code) = diagnostic.secondary_code() {
write!(writer, " {code}")?;
}

Expand Down
9 changes: 3 additions & 6 deletions crates/ruff_linter/src/message/gitlab.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,18 +90,15 @@ impl Serialize for SerializedMessages<'_> {
}
fingerprints.insert(message_fingerprint);

let (description, check_name) = if let Some(code) = diagnostic.noqa_code() {
(diagnostic.body().to_string(), code.to_string())
let (description, check_name) = if let Some(code) = diagnostic.secondary_code() {
(diagnostic.body().to_string(), code)
} else {
let description = diagnostic.body();
let description_without_prefix = description
.strip_prefix("SyntaxError: ")
.unwrap_or(description);

(
description_without_prefix.to_string(),
"syntax-error".to_string(),
)
(description_without_prefix.to_string(), "syntax-error")
};

let value = json!({
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/message/json.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ pub(crate) fn message_to_json_value(message: &OldDiagnostic, context: &EmitterCo
}

json!({
"code": message.noqa_code().map(|code| code.to_string()),
"code": message.secondary_code(),
"url": message.to_url(),
"message": message.body(),
"fix": fix,
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/message/junit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ impl Emitter for JunitEmitter {
body = message.body()
));
let mut case = TestCase::new(
if let Some(code) = message.noqa_code() {
if let Some(code) = message.secondary_code() {
format!("org.ruff.{code}")
} else {
"org.ruff".to_string()
Expand Down
11 changes: 5 additions & 6 deletions crates/ruff_linter/src/message/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ pub use sarif::SarifEmitter;
pub use text::TextEmitter;

use crate::Fix;
use crate::codes::NoqaCode;
use crate::logging::DisplayParseErrorType;
use crate::registry::Rule;
use crate::{Locator, Violation};
Expand Down Expand Up @@ -62,7 +61,7 @@ pub struct OldDiagnostic {
pub fix: Option<Fix>,
pub parent: Option<TextSize>,
pub(crate) noqa_offset: Option<TextSize>,
pub(crate) noqa_code: Option<NoqaCode>,
pub(crate) noqa_code: Option<String>,
}

impl OldDiagnostic {
Expand Down Expand Up @@ -115,7 +114,7 @@ impl OldDiagnostic {
fix,
parent,
noqa_offset,
noqa_code: Some(rule.noqa_code()),
noqa_code: Some(rule.noqa_code().to_string()),
}
}

Expand Down Expand Up @@ -247,9 +246,9 @@ impl OldDiagnostic {
self.fix().is_some()
}

/// Returns the [`NoqaCode`] corresponding to the diagnostic message.
pub fn noqa_code(&self) -> Option<NoqaCode> {
self.noqa_code
/// Returns the noqa code for the diagnostic message as a string.
pub fn secondary_code(&self) -> Option<&str> {
self.noqa_code.as_deref()
}

/// Returns the URL for the rule documentation, if it exists.
Expand Down
2 changes: 1 addition & 1 deletion crates/ruff_linter/src/message/pylint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ impl Emitter for PylintEmitter {
diagnostic.compute_start_location().line
};

let body = if let Some(code) = diagnostic.noqa_code() {
let body = if let Some(code) = diagnostic.secondary_code() {
format!("[{code}] {body}", body = diagnostic.body())
} else {
diagnostic.body().to_string()
Expand Down
4 changes: 2 additions & 2 deletions crates/ruff_linter/src/message/rdjson.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ fn message_to_rdjson_value(message: &OldDiagnostic) -> Value {
"range": rdjson_range(start_location, end_location),
},
"code": {
"value": message.noqa_code().map(|code| code.to_string()),
"value": message.secondary_code(),
"url": message.to_url(),
},
"suggestions": rdjson_suggestions(fix.edits(), &source_code),
Expand All @@ -84,7 +84,7 @@ fn message_to_rdjson_value(message: &OldDiagnostic) -> Value {
"range": rdjson_range(start_location, end_location),
},
"code": {
"value": message.noqa_code().map(|code| code.to_string()),
"value": message.secondary_code(),
"url": message.to_url(),
},
})
Expand Down
Loading
Loading