Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Jun 4, 2024
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# noqa
# ruff : noqa
# ruff: noqa: F401


# flake8: noqa
import math as m
23 changes: 17 additions & 6 deletions crates/ruff_linter/src/checkers/noqa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ use ruff_source_file::Locator;
use ruff_text_size::Ranged;

use crate::fix::edits::delete_comment;
use crate::noqa::{Code, Directive, FileExemption, NoqaDirectives, NoqaMapping};
use crate::noqa::{
BlanketNoqaDirectives, Code, Directive, FileExemption, NoqaDirectives, NoqaMapping,
};
use crate::registry::{AsRule, Rule, RuleSet};
use crate::rule_redirects::get_redirect_target;
use crate::rules::pygrep_hooks;
Expand All @@ -31,13 +33,16 @@ pub(crate) fn check_noqa(
settings: &LinterSettings,
) -> Vec<usize> {
// Identify any codes that are globally exempted (within the current file).
let exemption = FileExemption::try_extract(
let maybe_blanket_noqa_directives = BlanketNoqaDirectives::try_extract(
locator.contents(),
comment_ranges,
&settings.external,
path,
locator,
);
let exemption = maybe_blanket_noqa_directives
.as_ref()
.map(|directive| directive.exemption());

// Extract all `noqa` directives.
let mut noqa_directives = NoqaDirectives::from_commented_ranges(comment_ranges, path, locator);
Expand All @@ -59,7 +64,7 @@ pub(crate) fn check_noqa(
}
Some(FileExemption::Codes(codes)) => {
// 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);
continue;
}
Expand Down Expand Up @@ -109,9 +114,9 @@ pub(crate) fn check_noqa(
// suppressed.
if settings.rules.enabled(Rule::UnusedNOQA)
&& analyze_directives
&& !exemption.is_some_and(|exemption| match exemption {
&& !exemption.as_ref().is_some_and(|exemption| match exemption {
FileExemption::All => true,
FileExemption::Codes(codes) => codes.contains(&Rule::UnusedNOQA.noqa_code()),
FileExemption::Codes(codes) => codes.contains(&&Rule::UnusedNOQA.noqa_code()),
})
&& !per_file_ignores.contains(Rule::UnusedNOQA)
{
Expand Down Expand Up @@ -215,7 +220,13 @@ pub(crate) fn check_noqa(
}

if settings.rules.enabled(Rule::BlanketNOQA) {
pygrep_hooks::rules::blanket_noqa(diagnostics, &noqa_directives, locator);
pygrep_hooks::rules::blanket_noqa(
diagnostics,
&noqa_directives,
locator,
&maybe_blanket_noqa_directives,
settings.preview,
);
}

if settings.rules.enabled(Rule::RedirectedNOQA) {
Expand Down
103 changes: 80 additions & 23 deletions crates/ruff_linter/src/noqa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,16 @@ pub fn generate_noqa_edits(
noqa_line_for: &NoqaMapping,
line_ending: LineEnding,
) -> Vec<Option<Edit>> {
let exemption =
FileExemption::try_extract(locator.contents(), comment_ranges, external, path, locator);
let maybe_blanket_noqa_directives = BlanketNoqaDirectives::try_extract(
locator.contents(),
comment_ranges,
external,
path,
locator,
);
let exemption = maybe_blanket_noqa_directives
.as_ref()
.map(|blanket_directives| blanket_directives.exemption());
let directives = NoqaDirectives::from_commented_ranges(comment_ranges, path, locator);
let comments = find_noqa_comments(diagnostics, locator, &exemption, &directives, noqa_line_for);
build_noqa_edits_by_diagnostic(comments, locator, line_ending)
Expand Down Expand Up @@ -275,26 +283,43 @@ 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 [`BlanketNoqaDirectives`].
#[derive(Debug)]
pub(crate) enum FileExemption {
pub(crate) enum FileExemption<'a> {
/// The file is exempt from all rules.
All,
/// The file is exempt from the given rules.
Codes(Vec<NoqaCode>),
Codes(Vec<&'a NoqaCode>),
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 left this as a Vec, but we could consider using a HashSet instead as we're checking if certain NoqaCodes are contained in a few places (one being the loop on line 54 in checkers/noqa.rs).

It's probably fine though as I'd expect there will usually only be very few elements here and checking the small Vec is likely faster compared to the overhead of hashing every time (and building the HashSet in the first place).
We'd also have to derive Hash on NoqaCode.

}

/// The directive for a file-level exemption from an individual line.
#[derive(Debug)]
pub(crate) struct BlanketNoqaDirectiveLine<'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>,
}

/// All blanket, i.e file-level, exemptions from a given Python file.
#[derive(Debug)]
pub(crate) struct BlanketNoqaDirectives<'a> {
inner: Vec<BlanketNoqaDirectiveLine<'a>>,
}

impl FileExemption {
/// Extract the [`FileExemption`] for a given Python source file, enumerating any rules that are
/// globally ignored within the file.
impl<'a> BlanketNoqaDirectives<'a> {
/// Extract the [`BlanketNoqaDirectives`] for a given Python source file, enumerating any rules
/// that are globally ignored within the file.
pub(crate) fn try_extract(
contents: &str,
contents: &'a str,
comment_ranges: &CommentRanges,
external: &[String],
path: &Path,
locator: &Locator,
) -> Option<Self> {
let mut exempt_codes: Vec<NoqaCode> = vec![];
let mut blanket_noqa_lines = vec![];

for range in comment_ranges {
match ParsedFileExemption::try_extract(&contents[*range]) {
Expand All @@ -313,12 +338,12 @@ impl FileExemption {
continue;
}

match exemption {
let matches = match &exemption {
ParsedFileExemption::All => {
return Some(Self::All);
vec![]
}
ParsedFileExemption::Codes(codes) => {
exempt_codes.extend(codes.into_iter().filter_map(|code| {
codes.iter().filter_map(|code| {
// Ignore externally-defined rules.
if external.iter().any(|external| code.starts_with(external)) {
return None;
Expand All @@ -334,27 +359,51 @@ impl FileExemption {
warn!("Invalid rule code provided to `# ruff: noqa` at {path_display}:{line}: {code}");
None
}
}));
}).collect()
}
}
};

blanket_noqa_lines.push(BlanketNoqaDirectiveLine {
range: *range,
parsed_file_exemption: exemption,
matches,
});
}
Ok(None) => {}
}
}

if exempt_codes.is_empty() {
if blanket_noqa_lines.is_empty() {
None
} else {
Some(Self::Codes(exempt_codes))
Some(Self {
inner: blanket_noqa_lines,
})
}
}

pub(crate) fn lines(&self) -> &[BlanketNoqaDirectiveLine] {
&self.inner
}

pub(crate) fn exemption(&self) -> FileExemption {
if self
.inner
.iter()
.any(|line| ParsedFileExemption::All == line.parsed_file_exemption)
{
FileExemption::All
} else {
FileExemption::Codes(self.inner.iter().flat_map(|line| &line.matches).collect())
}
}
}

/// 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
/// [`BlanketNoqaDirectives`], but only for a single line, as opposed to an aggregated set of exemptions
/// across a source file.
#[derive(Debug)]
enum ParsedFileExemption<'a> {
#[derive(Debug, PartialEq, Eq)]
pub(crate) enum ParsedFileExemption<'a> {
/// The file-level exemption ignores all rules (e.g., `# ruff: noqa`).
All,
/// The file-level exemption ignores specific rules (e.g., `# ruff: noqa: F401, F841`).
Expand Down Expand Up @@ -550,8 +599,16 @@ fn add_noqa_inner(

// Whether the file is exempted from all checks.
// Codes that are globally exempted (within the current file).
let exemption =
FileExemption::try_extract(locator.contents(), comment_ranges, external, path, locator);
let maybe_blanket_noqa_directives = BlanketNoqaDirectives::try_extract(
locator.contents(),
comment_ranges,
external,
path,
locator,
);
let exemption = maybe_blanket_noqa_directives
.as_ref()
.map(|blanket_directives| blanket_directives.exemption());
let directives = NoqaDirectives::from_commented_ranges(comment_ranges, path, locator);

let comments = find_noqa_comments(diagnostics, locator, &exemption, &directives, noqa_line_for);
Expand Down Expand Up @@ -663,7 +720,7 @@ fn find_noqa_comments<'a>(
}
Some(FileExemption::Codes(codes)) => {
// 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);
continue;
}
Expand Down
20 changes: 20 additions & 0 deletions crates/ruff_linter/src/rules/pygrep_hooks/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,15 @@ mod tests {

use crate::registry::Rule;

use crate::settings::types::PreviewMode;
use crate::test::test_path;
use crate::{assert_messages, settings};

#[test_case(Rule::BlanketTypeIgnore, Path::new("PGH003_0.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_1.py"))]
#[test_case(Rule::BlanketNOQA, Path::new("PGH004_2.py"))]
#[test_case(Rule::InvalidMockAccess, Path::new("PGH005_0.py"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
Expand All @@ -27,4 +29,22 @@ mod tests {
assert_messages!(snapshot, diagnostics);
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(())
}
}
19 changes: 18 additions & 1 deletion crates/ruff_linter/src/rules/pygrep_hooks/rules/blanket_noqa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ use ruff_python_trivia::Cursor;
use ruff_source_file::Locator;
use ruff_text_size::{Ranged, TextRange};

use crate::noqa::{Directive, NoqaDirectives};
use crate::noqa::{BlanketNoqaDirectives, Directive, NoqaDirectives, ParsedFileExemption};
use crate::settings::types::PreviewMode;

/// ## What it does
/// Check for `noqa` annotations that suppress all diagnostics, as opposed to
Expand Down Expand Up @@ -85,7 +86,23 @@ pub(crate) fn blanket_noqa(
diagnostics: &mut Vec<Diagnostic>,
noqa_directives: &NoqaDirectives,
locator: &Locator,
maybe_blanket_noqa_directives: &Option<BlanketNoqaDirectives>,
preview: PreviewMode,
) {
if let (Some(blankets), PreviewMode::Enabled) = (maybe_blanket_noqa_directives, preview) {
for line in blankets.lines() {
if let ParsedFileExemption::All = line.parsed_file_exemption {
diagnostics.push(Diagnostic::new(
BlanketNOQA {
missing_colon: false,
space_before_colon: false,
},
line.range,
));
}
}
}

for directive_line in noqa_directives.lines() {
if let Directive::All(all) = &directive_line.directive {
let line = locator.slice(directive_line);
Expand Down
Original file line number Diff line number Diff line change
@@ -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
|
Original file line number Diff line number Diff line change
@@ -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 `noqa`
|
1 | # noqa
2 | # ruff : noqa
| ^^^^^^^^^^^^^ PGH004
3 | # ruff: noqa: F401
|

PGH004_2.py:6:1: PGH004 Use specific rule codes when using `noqa`
|
6 | # flake8: noqa
| ^^^^^^^^^^^^^^ PGH004
7 | import math as m
|
Loading