[`pygrep_hooks`] Check blanket ignores via file-level pragmas (`PGH004`) (#11540)

## Summary

Should resolve https://github.com/astral-sh/ruff/issues/11454.

This is my first PR to `ruff`, so I may have missed something.

If I understood the suggestion in the issue correctly, rule `PGH004`
should be set to `Preview` again.

## Test Plan

Created two fixtures derived from the issue.
This commit is contained in:
Alex 2024-06-04 05:42:58 +02:00 committed by GitHub
parent e1133a24ed
commit b56a577f25
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 204 additions and 46 deletions

View File

@ -0,0 +1,7 @@
# noqa
# ruff : noqa
# ruff: noqa: F401
# flake8: noqa
import math as m

View File

@ -11,7 +11,9 @@ use ruff_source_file::Locator;
use ruff_text_size::Ranged; use ruff_text_size::Ranged;
use crate::fix::edits::delete_comment; use crate::fix::edits::delete_comment;
use crate::noqa::{Code, Directive, FileExemption, NoqaDirectives, NoqaMapping}; use crate::noqa::{
Code, Directive, FileExemption, FileNoqaDirectives, NoqaDirectives, NoqaMapping,
};
use crate::registry::{AsRule, Rule, RuleSet}; use crate::registry::{AsRule, Rule, RuleSet};
use crate::rule_redirects::get_redirect_target; use crate::rule_redirects::get_redirect_target;
use crate::rules::pygrep_hooks; use crate::rules::pygrep_hooks;
@ -31,13 +33,14 @@ pub(crate) fn check_noqa(
settings: &LinterSettings, settings: &LinterSettings,
) -> Vec<usize> { ) -> Vec<usize> {
// Identify any codes that are globally exempted (within the current file). // Identify any codes that are globally exempted (within the current file).
let exemption = FileExemption::try_extract( let file_noqa_directives = FileNoqaDirectives::extract(
locator.contents(), locator.contents(),
comment_ranges, comment_ranges,
&settings.external, &settings.external,
path, path,
locator, locator,
); );
let exemption = FileExemption::from(&file_noqa_directives);
// Extract all `noqa` directives. // Extract all `noqa` directives.
let mut noqa_directives = NoqaDirectives::from_commented_ranges(comment_ranges, path, locator); let mut noqa_directives = NoqaDirectives::from_commented_ranges(comment_ranges, path, locator);
@ -52,19 +55,18 @@ pub(crate) fn check_noqa(
} }
match &exemption { match &exemption {
Some(FileExemption::All) => { FileExemption::All => {
// If the file is exempted, ignore all diagnostics. // If the file is exempted, ignore all diagnostics.
ignored_diagnostics.push(index); ignored_diagnostics.push(index);
continue; continue;
} }
Some(FileExemption::Codes(codes)) => { FileExemption::Codes(codes) => {
// If the diagnostic is ignored by a global exemption, ignore it. // If the diagnostic is ignored by a global exemption, ignore it.
if codes.contains(&diagnostic.kind.rule().noqa_code()) { if codes.contains(&&diagnostic.kind.rule().noqa_code()) {
ignored_diagnostics.push(index); ignored_diagnostics.push(index);
continue; continue;
} }
} }
None => {}
} }
let noqa_offsets = diagnostic let noqa_offsets = diagnostic
@ -109,10 +111,7 @@ pub(crate) fn check_noqa(
// suppressed. // suppressed.
if settings.rules.enabled(Rule::UnusedNOQA) if settings.rules.enabled(Rule::UnusedNOQA)
&& analyze_directives && analyze_directives
&& !exemption.is_some_and(|exemption| match exemption { && !exemption.includes(Rule::UnusedNOQA)
FileExemption::All => true,
FileExemption::Codes(codes) => codes.contains(&Rule::UnusedNOQA.noqa_code()),
})
&& !per_file_ignores.contains(Rule::UnusedNOQA) && !per_file_ignores.contains(Rule::UnusedNOQA)
{ {
for line in noqa_directives.lines() { for line in noqa_directives.lines() {
@ -215,7 +214,13 @@ pub(crate) fn check_noqa(
} }
if settings.rules.enabled(Rule::BlanketNOQA) { if settings.rules.enabled(Rule::BlanketNOQA) {
pygrep_hooks::rules::blanket_noqa(diagnostics, &noqa_directives, locator); pygrep_hooks::rules::blanket_noqa(
diagnostics,
&noqa_directives,
locator,
&file_noqa_directives,
settings.preview,
);
} }
if settings.rules.enabled(Rule::RedirectedNOQA) { if settings.rules.enabled(Rule::RedirectedNOQA) {

View File

@ -33,8 +33,9 @@ pub fn generate_noqa_edits(
noqa_line_for: &NoqaMapping, noqa_line_for: &NoqaMapping,
line_ending: LineEnding, line_ending: LineEnding,
) -> Vec<Option<Edit>> { ) -> Vec<Option<Edit>> {
let exemption = let file_directives =
FileExemption::try_extract(locator.contents(), comment_ranges, external, path, locator); FileNoqaDirectives::extract(locator.contents(), comment_ranges, external, path, locator);
let exemption = FileExemption::from(&file_directives);
let directives = NoqaDirectives::from_commented_ranges(comment_ranges, path, locator); let directives = NoqaDirectives::from_commented_ranges(comment_ranges, path, locator);
let comments = find_noqa_comments(diagnostics, locator, &exemption, &directives, noqa_line_for); let comments = find_noqa_comments(diagnostics, locator, &exemption, &directives, noqa_line_for);
build_noqa_edits_by_diagnostic(comments, locator, line_ending) build_noqa_edits_by_diagnostic(comments, locator, line_ending)
@ -275,26 +276,79 @@ pub(crate) fn rule_is_ignored(
} }
} }
/// The file-level exemptions extracted from a given Python file. /// A summary of the file-level exemption as extracted from [`FileNoqaDirectives`].
#[derive(Debug)] #[derive(Debug)]
pub(crate) enum FileExemption { pub(crate) enum FileExemption<'a> {
/// The file is exempt from all rules. /// The file is exempt from all rules.
All, All,
/// The file is exempt from the given rules. /// The file is exempt from the given rules.
Codes(Vec<NoqaCode>), Codes(Vec<&'a NoqaCode>),
} }
impl FileExemption { impl<'a> FileExemption<'a> {
/// Extract the [`FileExemption`] for a given Python source file, enumerating any rules that are /// Returns `true` if the file is exempt from the given rule.
/// globally ignored within the file. pub(crate) fn includes(&self, needle: Rule) -> bool {
pub(crate) fn try_extract( let needle = needle.noqa_code();
contents: &str, match self {
FileExemption::All => true,
FileExemption::Codes(codes) => codes.iter().any(|code| needle == **code),
}
}
}
impl<'a> From<&'a FileNoqaDirectives<'a>> for FileExemption<'a> {
fn from(directives: &'a FileNoqaDirectives) -> Self {
if directives
.lines()
.iter()
.any(|line| ParsedFileExemption::All == line.parsed_file_exemption)
{
FileExemption::All
} else {
FileExemption::Codes(
directives
.lines()
.iter()
.flat_map(|line| &line.matches)
.collect(),
)
}
}
}
/// The directive for a file-level exemption (e.g., `# ruff: noqa`) from an individual line.
#[derive(Debug)]
pub(crate) struct FileNoqaDirectiveLine<'a> {
/// The range of the text line for which the noqa directive applies.
pub(crate) range: TextRange,
/// The blanket noqa directive.
pub(crate) parsed_file_exemption: ParsedFileExemption<'a>,
/// The codes that are ignored by the parsed exemptions.
pub(crate) matches: Vec<NoqaCode>,
}
impl Ranged for FileNoqaDirectiveLine<'_> {
/// The range of the `noqa` directive.
fn range(&self) -> TextRange {
self.range
}
}
/// All file-level exemptions (e.g., `# ruff: noqa`) from a given Python file.
#[derive(Debug)]
pub(crate) struct FileNoqaDirectives<'a>(Vec<FileNoqaDirectiveLine<'a>>);
impl<'a> FileNoqaDirectives<'a> {
/// Extract the [`FileNoqaDirectives`] for a given Python source file, enumerating any rules
/// that are globally ignored within the file.
pub(crate) fn extract(
contents: &'a str,
comment_ranges: &CommentRanges, comment_ranges: &CommentRanges,
external: &[String], external: &[String],
path: &Path, path: &Path,
locator: &Locator, locator: &Locator,
) -> Option<Self> { ) -> Self {
let mut exempt_codes: Vec<NoqaCode> = vec![]; let mut lines = vec![];
for range in comment_ranges { for range in comment_ranges {
match ParsedFileExemption::try_extract(&contents[*range]) { match ParsedFileExemption::try_extract(&contents[*range]) {
@ -313,12 +367,12 @@ impl FileExemption {
continue; continue;
} }
match exemption { let matches = match &exemption {
ParsedFileExemption::All => { ParsedFileExemption::All => {
return Some(Self::All); vec![]
} }
ParsedFileExemption::Codes(codes) => { ParsedFileExemption::Codes(codes) => {
exempt_codes.extend(codes.into_iter().filter_map(|code| { codes.iter().filter_map(|code| {
// Ignore externally-defined rules. // Ignore externally-defined rules.
if external.iter().any(|external| code.starts_with(external)) { if external.iter().any(|external| code.starts_with(external)) {
return None; return None;
@ -334,27 +388,33 @@ impl FileExemption {
warn!("Invalid rule code provided to `# ruff: noqa` at {path_display}:{line}: {code}"); warn!("Invalid rule code provided to `# ruff: noqa` at {path_display}:{line}: {code}");
None None
} }
})); }).collect()
} }
} };
lines.push(FileNoqaDirectiveLine {
range: *range,
parsed_file_exemption: exemption,
matches,
});
} }
Ok(None) => {} Ok(None) => {}
} }
} }
if exempt_codes.is_empty() { Self(lines)
None }
} else {
Some(Self::Codes(exempt_codes)) pub(crate) fn lines(&self) -> &[FileNoqaDirectiveLine] {
} &self.0
} }
} }
/// An individual file-level exemption (e.g., `# ruff: noqa` or `# ruff: noqa: F401, F841`). Like /// An individual file-level exemption (e.g., `# ruff: noqa` or `# ruff: noqa: F401, F841`). Like
/// [`FileExemption`], but only for a single line, as opposed to an aggregated set of exemptions /// [`FileNoqaDirectives`], but only for a single line, as opposed to an aggregated set of exemptions
/// across a source file. /// across a source file.
#[derive(Debug)] #[derive(Debug, PartialEq, Eq)]
enum ParsedFileExemption<'a> { pub(crate) enum ParsedFileExemption<'a> {
/// The file-level exemption ignores all rules (e.g., `# ruff: noqa`). /// The file-level exemption ignores all rules (e.g., `# ruff: noqa`).
All, All,
/// The file-level exemption ignores specific rules (e.g., `# ruff: noqa: F401, F841`). /// The file-level exemption ignores specific rules (e.g., `# ruff: noqa: F401, F841`).
@ -549,9 +609,10 @@ fn add_noqa_inner(
let mut count = 0; let mut count = 0;
// Whether the file is exempted from all checks. // Whether the file is exempted from all checks.
// Codes that are globally exempted (within the current file). let directives =
let exemption = FileNoqaDirectives::extract(locator.contents(), comment_ranges, external, path, locator);
FileExemption::try_extract(locator.contents(), comment_ranges, external, path, locator); let exemption = FileExemption::from(&directives);
let directives = NoqaDirectives::from_commented_ranges(comment_ranges, path, locator); let directives = NoqaDirectives::from_commented_ranges(comment_ranges, path, locator);
let comments = find_noqa_comments(diagnostics, locator, &exemption, &directives, noqa_line_for); let comments = find_noqa_comments(diagnostics, locator, &exemption, &directives, noqa_line_for);
@ -646,7 +707,7 @@ struct NoqaComment<'a> {
fn find_noqa_comments<'a>( fn find_noqa_comments<'a>(
diagnostics: &'a [Diagnostic], diagnostics: &'a [Diagnostic],
locator: &'a Locator, locator: &'a Locator,
exemption: &Option<FileExemption>, exemption: &'a FileExemption,
directives: &'a NoqaDirectives, directives: &'a NoqaDirectives,
noqa_line_for: &NoqaMapping, noqa_line_for: &NoqaMapping,
) -> Vec<Option<NoqaComment<'a>>> { ) -> Vec<Option<NoqaComment<'a>>> {
@ -656,19 +717,18 @@ fn find_noqa_comments<'a>(
// Mark any non-ignored diagnostics. // Mark any non-ignored diagnostics.
for diagnostic in diagnostics { for diagnostic in diagnostics {
match &exemption { match &exemption {
Some(FileExemption::All) => { FileExemption::All => {
// If the file is exempted, don't add any noqa directives. // If the file is exempted, don't add any noqa directives.
comments_by_line.push(None); comments_by_line.push(None);
continue; continue;
} }
Some(FileExemption::Codes(codes)) => { FileExemption::Codes(codes) => {
// If the diagnostic is ignored by a global exemption, don't add a noqa directive. // If the diagnostic is ignored by a global exemption, don't add a noqa directive.
if codes.contains(&diagnostic.kind.rule().noqa_code()) { if codes.contains(&&diagnostic.kind.rule().noqa_code()) {
comments_by_line.push(None); comments_by_line.push(None);
continue; continue;
} }
} }
None => {}
} }
// Is the violation ignored by a `noqa` directive on the parent line? // Is the violation ignored by a `noqa` directive on the parent line?

View File

@ -10,6 +10,7 @@ mod tests {
use crate::registry::Rule; use crate::registry::Rule;
use crate::settings::types::PreviewMode;
use crate::test::test_path; use crate::test::test_path;
use crate::{assert_messages, settings}; use crate::{assert_messages, settings};
@ -17,6 +18,7 @@ mod tests {
#[test_case(Rule::BlanketTypeIgnore, Path::new("PGH003_1.py"))] #[test_case(Rule::BlanketTypeIgnore, Path::new("PGH003_1.py"))]
#[test_case(Rule::BlanketNOQA, Path::new("PGH004_0.py"))] #[test_case(Rule::BlanketNOQA, Path::new("PGH004_0.py"))]
#[test_case(Rule::BlanketNOQA, Path::new("PGH004_1.py"))] #[test_case(Rule::BlanketNOQA, Path::new("PGH004_1.py"))]
#[test_case(Rule::BlanketNOQA, Path::new("PGH004_2.py"))]
#[test_case(Rule::InvalidMockAccess, Path::new("PGH005_0.py"))] #[test_case(Rule::InvalidMockAccess, Path::new("PGH005_0.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> { fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
@ -27,4 +29,22 @@ mod tests {
assert_messages!(snapshot, diagnostics); assert_messages!(snapshot, diagnostics);
Ok(()) Ok(())
} }
#[test_case(Rule::BlanketNOQA, Path::new("PGH004_2.py"))]
fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!(
"preview__{}_{}",
rule_code.noqa_code(),
path.to_string_lossy()
);
let diagnostics = test_path(
Path::new("pygrep_hooks").join(path).as_path(),
&settings::LinterSettings {
preview: PreviewMode::Enabled,
..settings::LinterSettings::for_rule(rule_code)
},
)?;
assert_messages!(snapshot, diagnostics);
Ok(())
}
} }

View File

@ -4,7 +4,8 @@ use ruff_python_trivia::Cursor;
use ruff_source_file::Locator; use ruff_source_file::Locator;
use ruff_text_size::{Ranged, TextRange}; use ruff_text_size::{Ranged, TextRange};
use crate::noqa::{Directive, NoqaDirectives}; use crate::noqa::{Directive, FileNoqaDirectives, NoqaDirectives, ParsedFileExemption};
use crate::settings::types::PreviewMode;
/// ## What it does /// ## What it does
/// Check for `noqa` annotations that suppress all diagnostics, as opposed to /// Check for `noqa` annotations that suppress all diagnostics, as opposed to
@ -17,6 +18,9 @@ use crate::noqa::{Directive, NoqaDirectives};
/// maintain, as the annotation does not clarify which diagnostics are intended /// maintain, as the annotation does not clarify which diagnostics are intended
/// to be suppressed. /// to be suppressed.
/// ///
/// In [preview], this rule also checks for blanket file-level annotations (e.g.,
/// `# ruff: noqa`, as opposed to `# ruff: noqa: F401`).
///
/// ## Example /// ## Example
/// ```python /// ```python
/// from .base import * # noqa /// from .base import * # noqa
@ -37,10 +41,13 @@ use crate::noqa::{Directive, NoqaDirectives};
/// ///
/// ## References /// ## References
/// - [Ruff documentation](https://docs.astral.sh/ruff/configuration/#error-suppression) /// - [Ruff documentation](https://docs.astral.sh/ruff/configuration/#error-suppression)
///
/// [preview]: https://docs.astral.sh/ruff/preview/
#[violation] #[violation]
pub struct BlanketNOQA { pub struct BlanketNOQA {
missing_colon: bool, missing_colon: bool,
space_before_colon: bool, space_before_colon: bool,
file_exemption: bool,
} }
impl Violation for BlanketNOQA { impl Violation for BlanketNOQA {
@ -51,12 +58,15 @@ impl Violation for BlanketNOQA {
let BlanketNOQA { let BlanketNOQA {
missing_colon, missing_colon,
space_before_colon, space_before_colon,
file_exemption,
} = self; } = self;
// This awkward branching is necessary to ensure that the generic message is picked up by // This awkward branching is necessary to ensure that the generic message is picked up by
// `derive_message_formats`. // `derive_message_formats`.
if !missing_colon && !space_before_colon { if !missing_colon && !space_before_colon && !file_exemption {
format!("Use specific rule codes when using `noqa`") format!("Use specific rule codes when using `noqa`")
} else if *file_exemption {
format!("Use specific rule codes when using `ruff: noqa`")
} else if *missing_colon { } else if *missing_colon {
format!("Use a colon when specifying `noqa` rule codes") format!("Use a colon when specifying `noqa` rule codes")
} else { } else {
@ -68,6 +78,7 @@ impl Violation for BlanketNOQA {
let BlanketNOQA { let BlanketNOQA {
missing_colon, missing_colon,
space_before_colon, space_before_colon,
..
} = self; } = self;
if *missing_colon { if *missing_colon {
@ -85,7 +96,24 @@ pub(crate) fn blanket_noqa(
diagnostics: &mut Vec<Diagnostic>, diagnostics: &mut Vec<Diagnostic>,
noqa_directives: &NoqaDirectives, noqa_directives: &NoqaDirectives,
locator: &Locator, locator: &Locator,
file_noqa_directives: &FileNoqaDirectives,
preview: PreviewMode,
) { ) {
if preview.is_enabled() {
for line in file_noqa_directives.lines() {
if let ParsedFileExemption::All = line.parsed_file_exemption {
diagnostics.push(Diagnostic::new(
BlanketNOQA {
missing_colon: false,
space_before_colon: false,
file_exemption: true,
},
line.range(),
));
}
}
}
for directive_line in noqa_directives.lines() { for directive_line in noqa_directives.lines() {
if let Directive::All(all) = &directive_line.directive { if let Directive::All(all) = &directive_line.directive {
let line = locator.slice(directive_line); let line = locator.slice(directive_line);
@ -104,6 +132,7 @@ pub(crate) fn blanket_noqa(
BlanketNOQA { BlanketNOQA {
missing_colon: false, missing_colon: false,
space_before_colon: true, space_before_colon: true,
file_exemption: false,
}, },
TextRange::new(all.start(), end), TextRange::new(all.start(), end),
); );
@ -118,6 +147,7 @@ pub(crate) fn blanket_noqa(
BlanketNOQA { BlanketNOQA {
missing_colon: true, missing_colon: true,
space_before_colon: false, space_before_colon: false,
file_exemption: false,
}, },
TextRange::new(all.start(), end), TextRange::new(all.start(), end),
); );
@ -129,6 +159,7 @@ pub(crate) fn blanket_noqa(
BlanketNOQA { BlanketNOQA {
missing_colon: false, missing_colon: false,
space_before_colon: false, space_before_colon: false,
file_exemption: false,
}, },
all.range(), all.range(),
)); ));

View File

@ -0,0 +1,10 @@
---
source: crates/ruff_linter/src/rules/pygrep_hooks/mod.rs
---
PGH004_2.py:1:1: PGH004 Use specific rule codes when using `noqa`
|
1 | # noqa
| ^^^^^^ PGH004
2 | # ruff : noqa
3 | # ruff: noqa: F401
|

View File

@ -0,0 +1,25 @@
---
source: crates/ruff_linter/src/rules/pygrep_hooks/mod.rs
---
PGH004_2.py:1:1: PGH004 Use specific rule codes when using `noqa`
|
1 | # noqa
| ^^^^^^ PGH004
2 | # ruff : noqa
3 | # ruff: noqa: F401
|
PGH004_2.py:2:1: PGH004 Use specific rule codes when using `ruff: noqa`
|
1 | # noqa
2 | # ruff : noqa
| ^^^^^^^^^^^^^ PGH004
3 | # ruff: noqa: F401
|
PGH004_2.py:6:1: PGH004 Use specific rule codes when using `ruff: noqa`
|
6 | # flake8: noqa
| ^^^^^^^^^^^^^^ PGH004
7 | import math as m
|