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 @@
# ruff: noqa
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
# flake8: noqa
14 changes: 10 additions & 4 deletions crates/ruff_linter/src/checkers/noqa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ pub(crate) fn check_noqa(
}

match &exemption {
Some(FileExemption::All) => {
Some(FileExemption::All(_)) => {
// If the file is exempted, ignore all diagnostics.
ignored_diagnostics.push(index);
continue;
Expand Down Expand Up @@ -109,8 +109,8 @@ pub(crate) fn check_noqa(
// suppressed.
if settings.rules.enabled(Rule::UnusedNOQA)
&& analyze_directives
&& !exemption.is_some_and(|exemption| match exemption {
FileExemption::All => true,
&& !exemption.as_ref().is_some_and(|exemption| match exemption {
FileExemption::All(_) => true,
FileExemption::Codes(codes) => codes.contains(&Rule::UnusedNOQA.noqa_code()),
})
&& !per_file_ignores.contains(Rule::UnusedNOQA)
Expand Down Expand Up @@ -215,7 +215,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,
&exemption,
settings.preview,
);
}

if settings.rules.enabled(Rule::RedirectedNOQA) {
Expand Down
6 changes: 3 additions & 3 deletions crates/ruff_linter/src/noqa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -279,7 +279,7 @@ pub(crate) fn rule_is_ignored(
#[derive(Debug)]
pub(crate) enum FileExemption {
/// The file is exempt from all rules.
All,
All(All),
/// The file is exempt from the given rules.
Codes(Vec<NoqaCode>),
}
Expand Down Expand Up @@ -315,7 +315,7 @@ impl FileExemption {

match exemption {
ParsedFileExemption::All => {
return Some(Self::All);
return Some(Self::All(All { range: *range }));
}
ParsedFileExemption::Codes(codes) => {
exempt_codes.extend(codes.into_iter().filter_map(|code| {
Expand Down Expand Up @@ -656,7 +656,7 @@ fn find_noqa_comments<'a>(
// Mark any non-ignored diagnostics.
for diagnostic in diagnostics {
match &exemption {
Some(FileExemption::All) => {
Some(FileExemption::All(_)) => {
// If the file is exempted, don't add any noqa directives.
comments_by_line.push(None);
continue;
Expand Down
22 changes: 22 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,16 @@ 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::BlanketNOQA, Path::new("PGH004_3.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 +30,23 @@ mod tests {
assert_messages!(snapshot, diagnostics);
Ok(())
}

#[test_case(Rule::BlanketNOQA, Path::new("PGH004_2.py"))]
#[test_case(Rule::BlanketNOQA, Path::new("PGH004_3.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(())
}
}
15 changes: 14 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::{Directive, FileExemption, NoqaDirectives};
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,19 @@ pub(crate) fn blanket_noqa(
diagnostics: &mut Vec<Diagnostic>,
noqa_directives: &NoqaDirectives,
locator: &Locator,
exemption: &Option<FileExemption>,
Copy link
Member

Choose a reason for hiding this comment

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

One downside here is that this is only going to flag the first # ruff: noqa in the file. I assume if you add multiple such comments, only the first is flagged as a diagnostic?

Copy link
Member

Choose a reason for hiding this comment

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

We might need to do something like NoqaDirectives, whereby we parse all of these upfront, then use the parsed exemptions in crates/ruff_linter/src/checkers/noqa.rs (and here).

Copy link
Member

Choose a reason for hiding this comment

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

@ajesipow - Do you want to give that a try?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the ping @charliermarsh.
I haven't found the time yet after work this week, but today is a public holiday over here so I'll look into it today :)

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 just pushed the changes and adapted the tests again accordingly.

Looking forward to your feedback again.

preview: PreviewMode,
) {
if let (Some(FileExemption::All(all)), PreviewMode::Enabled) = (exemption, preview) {
diagnostics.push(Diagnostic::new(
BlanketNOQA {
missing_colon: false,
space_before_colon: false,
},
all.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,4 @@
---
source: crates/ruff_linter/src/rules/pygrep_hooks/mod.rs
---

Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
source: crates/ruff_linter/src/rules/pygrep_hooks/mod.rs
---

Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
source: crates/ruff_linter/src/rules/pygrep_hooks/mod.rs
---
PGH004_2.py:1:1: PGH004 Use specific rule codes when using `noqa`
|
1 | # ruff: noqa
| ^^^^^^^^^^^^ PGH004
|
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
source: crates/ruff_linter/src/rules/pygrep_hooks/mod.rs
---
PGH004_3.py:1:1: PGH004 Use specific rule codes when using `noqa`
|
1 | # flake8: noqa
| ^^^^^^^^^^^^^^ PGH004
|
Loading