Skip to content

Commit

Permalink
[pygrep_hooks] Check blanket ignores via file-level pragmas (`PGH00…
Browse files Browse the repository at this point in the history
…4`) (#11540)

## Summary

Should resolve #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.
  • Loading branch information
ajesipow authored Jun 4, 2024
1 parent e1133a2 commit b56a577
Show file tree
Hide file tree
Showing 7 changed files with 204 additions and 46 deletions.
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
27 changes: 16 additions & 11 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::{
Code, Directive, FileExemption, FileNoqaDirectives, 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,14 @@ 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 file_noqa_directives = FileNoqaDirectives::extract(
locator.contents(),
comment_ranges,
&settings.external,
path,
locator,
);
let exemption = FileExemption::from(&file_noqa_directives);

// Extract all `noqa` directives.
let mut noqa_directives = NoqaDirectives::from_commented_ranges(comment_ranges, path, locator);
Expand All @@ -52,19 +55,18 @@ pub(crate) fn check_noqa(
}

match &exemption {
Some(FileExemption::All) => {
FileExemption::All => {
// If the file is exempted, ignore all diagnostics.
ignored_diagnostics.push(index);
continue;
}
Some(FileExemption::Codes(codes)) => {
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;
}
}
None => {}
}

let noqa_offsets = diagnostic
Expand Down Expand Up @@ -109,10 +111,7 @@ pub(crate) fn check_noqa(
// suppressed.
if settings.rules.enabled(Rule::UnusedNOQA)
&& analyze_directives
&& !exemption.is_some_and(|exemption| match exemption {
FileExemption::All => true,
FileExemption::Codes(codes) => codes.contains(&Rule::UnusedNOQA.noqa_code()),
})
&& !exemption.includes(Rule::UnusedNOQA)
&& !per_file_ignores.contains(Rule::UnusedNOQA)
{
for line in noqa_directives.lines() {
Expand Down Expand Up @@ -215,7 +214,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,
&file_noqa_directives,
settings.preview,
);
}

if settings.rules.enabled(Rule::RedirectedNOQA) {
Expand Down
126 changes: 93 additions & 33 deletions crates/ruff_linter/src/noqa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,9 @@ 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 file_directives =
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 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 +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)]
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>),
}

impl FileExemption {
/// Extract the [`FileExemption`] for a given Python source file, enumerating any rules that are
/// globally ignored within the file.
pub(crate) fn try_extract(
contents: &str,
impl<'a> FileExemption<'a> {
/// Returns `true` if the file is exempt from the given rule.
pub(crate) fn includes(&self, needle: Rule) -> bool {
let needle = needle.noqa_code();
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,
external: &[String],
path: &Path,
locator: &Locator,
) -> Option<Self> {
let mut exempt_codes: Vec<NoqaCode> = vec![];
) -> Self {
let mut lines = vec![];

for range in comment_ranges {
match ParsedFileExemption::try_extract(&contents[*range]) {
Expand All @@ -313,12 +367,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 +388,33 @@ impl FileExemption {
warn!("Invalid rule code provided to `# ruff: noqa` at {path_display}:{line}: {code}");
None
}
}));
}).collect()
}
}
};

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

if exempt_codes.is_empty() {
None
} else {
Some(Self::Codes(exempt_codes))
}
Self(lines)
}

pub(crate) fn lines(&self) -> &[FileNoqaDirectiveLine] {
&self.0
}
}

/// 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.
#[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 @@ -549,9 +609,10 @@ fn add_noqa_inner(
let mut count = 0;

// 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 directives =
FileNoqaDirectives::extract(locator.contents(), comment_ranges, external, path, locator);
let exemption = FileExemption::from(&directives);

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 @@ -646,7 +707,7 @@ struct NoqaComment<'a> {
fn find_noqa_comments<'a>(
diagnostics: &'a [Diagnostic],
locator: &'a Locator,
exemption: &Option<FileExemption>,
exemption: &'a FileExemption,
directives: &'a NoqaDirectives,
noqa_line_for: &NoqaMapping,
) -> Vec<Option<NoqaComment<'a>>> {
Expand All @@ -656,19 +717,18 @@ fn find_noqa_comments<'a>(
// Mark any non-ignored diagnostics.
for diagnostic in diagnostics {
match &exemption {
Some(FileExemption::All) => {
FileExemption::All => {
// If the file is exempted, don't add any noqa directives.
comments_by_line.push(None);
continue;
}
Some(FileExemption::Codes(codes)) => {
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;
}
}
None => {}
}

// Is the violation ignored by a `noqa` directive on the parent line?
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(())
}
}
Loading

0 comments on commit b56a577

Please sign in to comment.