Apply fix availability and applicability when adding to DiagnosticGuard and remove NoqaCode::rule (#18834)

## Summary

This PR removes the last two places we were using `NoqaCode::rule` in
`linter.rs` (see
https://github.com/astral-sh/ruff/pull/18391#discussion_r2154637329 and
https://github.com/astral-sh/ruff/pull/18391#discussion_r2154649726) by
checking whether fixes are actually desired before adding them to a
`DiagnosticGuard`. I implemented this by storing a `Violation`'s `Rule`
on the `DiagnosticGuard` so that we could check if it was enabled in the
embedded `LinterSettings` when trying to set a fix.

All of the corresponding `set_fix` methods on `OldDiagnostic` were now
unused (except in tests where I just set `.fix` directly), so I moved
these to the guard instead of keeping both sets.

The very last place where we were using `NoqaCode::rule` was in the
cache. I just reverted this to parsing the `Rule` from the name. I had
forgotten to update the comment there anyway. Hopefully this doesn't
cause too much of a perf hit.

In terms of binary size, we're back down almost to where `main` was two
days ago
(https://github.com/astral-sh/ruff/pull/18391#discussion_r2155034320):

```
41,559,344 bytes for main 2 days ago
41,669,840 bytes for #18391
41,653,760 bytes for main now (after #18391 merged)
41,602,224 bytes for this branch
```

Only 43 kb up, but that shouldn't all be me this time :)

## Test Plan

Existing tests and benchmarks on this PR
This commit is contained in:
Brent Westbrook 2025-06-24 10:08:36 -04:00 committed by GitHub
parent 0edbd6c390
commit 02ae8e1210
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
11 changed files with 73 additions and 102 deletions

View file

@ -28,7 +28,7 @@ use itertools::Itertools;
use log::debug;
use rustc_hash::{FxHashMap, FxHashSet};
use ruff_diagnostics::IsolationLevel;
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};
use ruff_python_ast::identifier::Identifier;
@ -3154,6 +3154,7 @@ impl<'a> LintContext<'a> {
DiagnosticGuard {
context: self,
diagnostic: Some(OldDiagnostic::new(kind, range, &self.source_file)),
rule: T::rule(),
}
}
@ -3167,10 +3168,12 @@ impl<'a> LintContext<'a> {
kind: T,
range: TextRange,
) -> Option<DiagnosticGuard<'chk, 'a>> {
if self.is_rule_enabled(T::rule()) {
let rule = T::rule();
if self.is_rule_enabled(rule) {
Some(DiagnosticGuard {
context: self,
diagnostic: Some(OldDiagnostic::new(kind, range, &self.source_file)),
rule,
})
} else {
None
@ -3222,6 +3225,7 @@ pub(crate) struct DiagnosticGuard<'a, 'b> {
///
/// This is always `Some` until the `Drop` (or `defuse`) call.
diagnostic: Option<OldDiagnostic>,
rule: Rule,
}
impl DiagnosticGuard<'_, '_> {
@ -3234,6 +3238,50 @@ impl DiagnosticGuard<'_, '_> {
}
}
impl DiagnosticGuard<'_, '_> {
fn resolve_applicability(&self, fix: &Fix) -> Applicability {
self.context
.settings
.fix_safety
.resolve_applicability(self.rule, fix.applicability())
}
/// Set the [`Fix`] used to fix the diagnostic.
#[inline]
pub(crate) fn set_fix(&mut self, fix: Fix) {
if !self.context.rules.should_fix(self.rule) {
self.fix = None;
return;
}
let applicability = self.resolve_applicability(&fix);
self.fix = Some(fix.with_applicability(applicability));
}
/// Set the [`Fix`] used to fix the diagnostic, if the provided function returns `Ok`.
/// Otherwise, log the error.
#[inline]
pub(crate) fn try_set_fix(&mut self, func: impl FnOnce() -> anyhow::Result<Fix>) {
match func() {
Ok(fix) => self.set_fix(fix),
Err(err) => log::debug!("Failed to create fix for {}: {}", self.name(), err),
}
}
/// Set the [`Fix`] used to fix the diagnostic, if the provided function returns `Ok`.
/// Otherwise, log the error.
#[inline]
pub(crate) fn try_set_optional_fix(
&mut self,
func: impl FnOnce() -> anyhow::Result<Option<Fix>>,
) {
match func() {
Ok(None) => {}
Ok(Some(fix)) => self.set_fix(fix),
Err(err) => log::debug!("Failed to create fix for {}: {}", self.name(), err),
}
}
}
impl std::ops::Deref for DiagnosticGuard<'_, '_> {
type Target = OldDiagnostic;

View file

@ -749,15 +749,16 @@ x = 1 \
let diag = {
use crate::rules::pycodestyle::rules::MissingNewlineAtEndOfFile;
let mut iter = edits.into_iter();
OldDiagnostic::new(
let mut diagnostic = OldDiagnostic::new(
MissingNewlineAtEndOfFile, // The choice of rule here is arbitrary.
TextRange::default(),
&SourceFileBuilder::new("<filename>", "<code>").finish(),
)
.with_fix(Fix::safe_edits(
);
diagnostic.fix = Some(Fix::safe_edits(
iter.next().ok_or(anyhow!("expected edits nonempty"))?,
iter,
))
));
diagnostic
};
assert_eq!(apply_fixes([diag].iter(), &locator).code, expect);
Ok(())

View file

@ -186,12 +186,13 @@ mod tests {
edit.into_iter()
.map(|edit| {
// The choice of rule here is arbitrary.
let diagnostic = OldDiagnostic::new(
let mut diagnostic = OldDiagnostic::new(
MissingNewlineAtEndOfFile,
edit.range(),
&SourceFileBuilder::new(filename, source).finish(),
);
diagnostic.with_fix(Fix::safe_edit(edit))
diagnostic.fix = Some(Fix::safe_edit(edit));
diagnostic
})
.collect()
}

View file

@ -378,32 +378,7 @@ pub fn check_path(
let (mut diagnostics, source_file) = context.into_parts();
if parsed.has_valid_syntax() {
// Remove fixes for any rules marked as unfixable.
for diagnostic in &mut diagnostics {
if diagnostic
.noqa_code()
.and_then(|code| code.rule())
.is_none_or(|rule| !settings.rules.should_fix(rule))
{
diagnostic.fix = None;
}
}
// Update fix applicability to account for overrides
if !settings.fix_safety.is_empty() {
for diagnostic in &mut diagnostics {
if let Some(fix) = diagnostic.fix.take() {
if let Some(rule) = diagnostic.noqa_code().and_then(|code| code.rule()) {
let fixed_applicability = settings
.fix_safety
.resolve_applicability(rule, fix.applicability());
diagnostic.set_fix(fix.with_applicability(fixed_applicability));
}
}
}
}
} else {
if !parsed.has_valid_syntax() {
// Avoid fixing in case the source code contains syntax errors.
for diagnostic in &mut diagnostics {
diagnostic.fix = None;

View file

@ -186,41 +186,6 @@ impl OldDiagnostic {
)
}
/// Consumes `self` and returns a new `Diagnostic` with the given `fix`.
#[inline]
#[must_use]
pub fn with_fix(mut self, fix: Fix) -> Self {
self.set_fix(fix);
self
}
/// Set the [`Fix`] used to fix the diagnostic.
#[inline]
pub fn set_fix(&mut self, fix: Fix) {
self.fix = Some(fix);
}
/// Set the [`Fix`] used to fix the diagnostic, if the provided function returns `Ok`.
/// Otherwise, log the error.
#[inline]
pub fn try_set_fix(&mut self, func: impl FnOnce() -> anyhow::Result<Fix>) {
match func() {
Ok(fix) => self.fix = Some(fix),
Err(err) => log::debug!("Failed to create fix for {}: {}", self.name(), err),
}
}
/// Set the [`Fix`] used to fix the diagnostic, if the provided function returns `Ok`.
/// Otherwise, log the error.
#[inline]
pub fn try_set_optional_fix(&mut self, func: impl FnOnce() -> anyhow::Result<Option<Fix>>) {
match func() {
Ok(None) => {}
Ok(Some(fix)) => self.fix = Some(fix),
Err(err) => log::debug!("Failed to create fix for {}: {}", self.name(), err),
}
}
/// Consumes `self` and returns a new `Diagnostic` with the given parent node.
#[inline]
#[must_use]

View file

@ -215,6 +215,12 @@ pub enum Linter {
}
pub trait RuleNamespace: Sized {
/// Returns the prefix that every single code that ruff uses to identify
/// rules from this linter starts with. In the case that multiple
/// `#[prefix]`es are configured for the variant in the `Linter` enum
/// definition this is the empty string.
fn common_prefix(&self) -> &'static str;
/// Attempts to parse the given rule code. If the prefix is recognized
/// returns the respective variant along with the code with the common
/// prefix stripped.

View file

@ -265,6 +265,7 @@ mod schema {
use strum::IntoEnumIterator;
use crate::RuleSelector;
use crate::registry::RuleNamespace;
use crate::rule_selector::{Linter, RuleCodePrefix};
impl JsonSchema for RuleSelector {