Skip to content

Commit

Permalink
[ruff] Detect redirected-noqa in file-level comments (RUF101) (#1…
Browse files Browse the repository at this point in the history
  • Loading branch information
ntBre authored Nov 27, 2024
1 parent c40b37a commit 6fcbe8e
Show file tree
Hide file tree
Showing 16 changed files with 173 additions and 58 deletions.
13 changes: 13 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF101_1.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
"""Regression test for #14531.
RUF101 should trigger here because the TCH rules have been recoded to TC.
"""
# ruff: noqa: TCH002

from __future__ import annotations

import local_module


def func(sized: local_module.Container) -> int:
return len(sized)
1 change: 1 addition & 0 deletions crates/ruff_linter/src/checkers/noqa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ pub(crate) fn check_noqa(
&& !exemption.includes(Rule::RedirectedNOQA)
{
ruff::rules::redirected_noqa(diagnostics, &noqa_directives);
ruff::rules::redirected_file_noqa(diagnostics, &file_noqa_directives);
}

if settings.rules.enabled(Rule::BlanketNOQA)
Expand Down
73 changes: 56 additions & 17 deletions crates/ruff_linter/src/noqa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ impl<'a> From<&'a FileNoqaDirectives<'a>> for FileExemption<'a> {
if directives
.lines()
.iter()
.any(|line| ParsedFileExemption::All == line.parsed_file_exemption)
.any(|line| matches!(line.parsed_file_exemption, ParsedFileExemption::All))
{
FileExemption::All(codes)
} else {
Expand Down Expand Up @@ -362,7 +362,7 @@ impl<'a> FileNoqaDirectives<'a> {
let mut lines = vec![];

for range in comment_ranges {
match ParsedFileExemption::try_extract(&locator.contents()[range]) {
match ParsedFileExemption::try_extract(range, locator.contents()) {
Err(err) => {
#[allow(deprecated)]
let line = locator.compute_line_index(range.start());
Expand All @@ -384,6 +384,7 @@ impl<'a> FileNoqaDirectives<'a> {
}
ParsedFileExemption::Codes(codes) => {
codes.iter().filter_map(|code| {
let code = code.as_str();
// Ignore externally-defined rules.
if external.iter().any(|external| code.starts_with(external)) {
return None;
Expand Down Expand Up @@ -424,21 +425,26 @@ impl<'a> FileNoqaDirectives<'a> {
/// An individual file-level exemption (e.g., `# ruff: noqa` or `# ruff: noqa: F401, F841`). Like
/// [`FileNoqaDirectives`], but only for a single line, as opposed to an aggregated set of exemptions
/// across a source file.
#[derive(Debug, PartialEq, Eq)]
#[derive(Debug)]
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`).
Codes(Vec<&'a str>),
Codes(Codes<'a>),
}

impl<'a> ParsedFileExemption<'a> {
/// Return a [`ParsedFileExemption`] for a given comment line.
fn try_extract(line: &'a str) -> Result<Option<Self>, ParseError> {
/// Return a [`ParsedFileExemption`] for a given `comment_range` in `source`.
fn try_extract(comment_range: TextRange, source: &'a str) -> Result<Option<Self>, ParseError> {
let line = &source[comment_range];
let offset = comment_range.start();
let init_line_len = line.text_len();

let line = Self::lex_whitespace(line);
let Some(line) = Self::lex_char(line, '#') else {
return Ok(None);
};
let comment_start = init_line_len - line.text_len() - '#'.text_len();
let line = Self::lex_whitespace(line);

let Some(line) = Self::lex_flake8(line).or_else(|| Self::lex_ruff(line)) else {
Expand Down Expand Up @@ -469,7 +475,11 @@ impl<'a> ParsedFileExemption<'a> {
let mut codes = vec![];
let mut line = line;
while let Some(code) = Self::lex_code(line) {
codes.push(code);
let codes_end = init_line_len - line.text_len();
codes.push(Code {
code,
range: TextRange::at(codes_end, code.text_len()).add(offset),
});
line = &line[code.len()..];

// Codes can be comma- or whitespace-delimited.
Expand All @@ -485,7 +495,12 @@ impl<'a> ParsedFileExemption<'a> {
return Err(ParseError::MissingCodes);
}

Self::Codes(codes)
let codes_end = init_line_len - line.text_len();
let range = TextRange::new(comment_start, codes_end);
Self::Codes(Codes {
range: range.add(offset),
codes,
})
}))
}

Expand Down Expand Up @@ -1059,7 +1074,7 @@ mod tests {
use ruff_diagnostics::{Diagnostic, Edit};
use ruff_python_trivia::CommentRanges;
use ruff_source_file::LineEnding;
use ruff_text_size::{TextRange, TextSize};
use ruff_text_size::{TextLen, TextRange, TextSize};

use crate::noqa::{add_noqa_inner, Directive, NoqaMapping, ParsedFileExemption};
use crate::rules::pycodestyle::rules::{AmbiguousVariableName, UselessSemicolon};
Expand Down Expand Up @@ -1226,50 +1241,74 @@ mod tests {
#[test]
fn flake8_exemption_all() {
let source = "# flake8: noqa";
assert_debug_snapshot!(ParsedFileExemption::try_extract(source));
assert_debug_snapshot!(ParsedFileExemption::try_extract(
TextRange::up_to(source.text_len()),
source,
));
}

#[test]
fn ruff_exemption_all() {
let source = "# ruff: noqa";
assert_debug_snapshot!(ParsedFileExemption::try_extract(source));
assert_debug_snapshot!(ParsedFileExemption::try_extract(
TextRange::up_to(source.text_len()),
source,
));
}

#[test]
fn flake8_exemption_all_no_space() {
let source = "#flake8:noqa";
assert_debug_snapshot!(ParsedFileExemption::try_extract(source));
assert_debug_snapshot!(ParsedFileExemption::try_extract(
TextRange::up_to(source.text_len()),
source,
));
}

#[test]
fn ruff_exemption_all_no_space() {
let source = "#ruff:noqa";
assert_debug_snapshot!(ParsedFileExemption::try_extract(source));
assert_debug_snapshot!(ParsedFileExemption::try_extract(
TextRange::up_to(source.text_len()),
source,
));
}

#[test]
fn flake8_exemption_codes() {
// Note: Flake8 doesn't support this; it's treated as a blanket exemption.
let source = "# flake8: noqa: F401, F841";
assert_debug_snapshot!(ParsedFileExemption::try_extract(source));
assert_debug_snapshot!(ParsedFileExemption::try_extract(
TextRange::up_to(source.text_len()),
source,
));
}

#[test]
fn ruff_exemption_codes() {
let source = "# ruff: noqa: F401, F841";
assert_debug_snapshot!(ParsedFileExemption::try_extract(source));
assert_debug_snapshot!(ParsedFileExemption::try_extract(
TextRange::up_to(source.text_len()),
source,
));
}

#[test]
fn flake8_exemption_all_case_insensitive() {
let source = "# flake8: NoQa";
assert_debug_snapshot!(ParsedFileExemption::try_extract(source));
assert_debug_snapshot!(ParsedFileExemption::try_extract(
TextRange::up_to(source.text_len()),
source,
));
}

#[test]
fn ruff_exemption_all_case_insensitive() {
let source = "# ruff: NoQa";
assert_debug_snapshot!(ParsedFileExemption::try_extract(source));
assert_debug_snapshot!(ParsedFileExemption::try_extract(
TextRange::up_to(source.text_len()),
source,
));
}

#[test]
Expand Down
3 changes: 2 additions & 1 deletion crates/ruff_linter/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ mod tests {
#[test_case(Rule::IncorrectlyParenthesizedTupleInSubscript, Path::new("RUF031.py"))]
#[test_case(Rule::DecimalFromFloatLiteral, Path::new("RUF032.py"))]
#[test_case(Rule::UselessIfElse, Path::new("RUF034.py"))]
#[test_case(Rule::RedirectedNOQA, Path::new("RUF101.py"))]
#[test_case(Rule::RedirectedNOQA, Path::new("RUF101_0.py"))]
#[test_case(Rule::RedirectedNOQA, Path::new("RUF101_1.py"))]
#[test_case(Rule::PostInitDefault, Path::new("RUF033.py"))]
#[test_case(Rule::NoneNotAtEndOfUnion, Path::new("RUF036.py"))]
#[test_case(Rule::NoneNotAtEndOfUnion, Path::new("RUF036.pyi"))]
Expand Down
53 changes: 36 additions & 17 deletions crates/ruff_linter/src/rules/ruff/rules/redirected_noqa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use ruff_diagnostics::{AlwaysFixableViolation, Diagnostic, Edit, Fix};
use ruff_macros::{derive_message_formats, ViolationMetadata};
use ruff_text_size::Ranged;

use crate::noqa::{Directive, NoqaDirectives};
use crate::noqa::{Codes, Directive, FileNoqaDirectives, NoqaDirectives, ParsedFileExemption};
use crate::rule_redirects::get_redirect_target;

/// ## What it does
Expand Down Expand Up @@ -42,28 +42,47 @@ impl AlwaysFixableViolation for RedirectedNOQA {
}
}

/// RUF101
/// RUF101 for in-line noqa directives
pub(crate) fn redirected_noqa(diagnostics: &mut Vec<Diagnostic>, noqa_directives: &NoqaDirectives) {
for line in noqa_directives.lines() {
let Directive::Codes(directive) = &line.directive else {
continue;
};

for code in directive.iter() {
if let Some(redirected) = get_redirect_target(code.as_str()) {
let mut diagnostic = Diagnostic::new(
RedirectedNOQA {
original: code.to_string(),
target: redirected.to_string(),
},
code.range(),
);
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
redirected.to_string(),
code.range(),
)));
diagnostics.push(diagnostic);
}
build_diagnostics(diagnostics, directive);
}
}

/// RUF101 for file noqa directives
pub(crate) fn redirected_file_noqa(
diagnostics: &mut Vec<Diagnostic>,
noqa_directives: &FileNoqaDirectives,
) {
for line in noqa_directives.lines() {
let ParsedFileExemption::Codes(codes) = &line.parsed_file_exemption else {
continue;
};

build_diagnostics(diagnostics, codes);
}
}

/// Convert a sequence of [Codes] into [Diagnostic]s and append them to `diagnostics`.
fn build_diagnostics(diagnostics: &mut Vec<Diagnostic>, codes: &Codes<'_>) {
for code in codes.iter() {
if let Some(redirected) = get_redirect_target(code.as_str()) {
let mut diagnostic = Diagnostic::new(
RedirectedNOQA {
original: code.to_string(),
target: redirected.to_string(),
},
code.range(),
);
diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement(
redirected.to_string(),
code.range(),
)));
diagnostics.push(diagnostic);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
source: crates/ruff_linter/src/rules/ruff/mod.rs
snapshot_kind: text
---
RUF101.py:1:15: RUF101 [*] `RUF940` is a redirect to `RUF950`
RUF101_0.py:1:15: RUF101 [*] `RUF940` is a redirect to `RUF950`
|
1 | x = 2 # noqa: RUF940
| ^^^^^^ RUF101
Expand All @@ -19,7 +19,7 @@ RUF101.py:1:15: RUF101 [*] `RUF940` is a redirect to `RUF950`
4 4 | x = 2 # noqa: RUF950, RUF940, RUF950, RUF950, RUF950
5 5 | x = 2 # noqa: RUF940, RUF950, RUF940

RUF101.py:3:15: RUF101 [*] `RUF940` is a redirect to `RUF950`
RUF101_0.py:3:15: RUF101 [*] `RUF940` is a redirect to `RUF950`
|
1 | x = 2 # noqa: RUF940
2 | x = 2 # noqa: RUF950
Expand All @@ -39,7 +39,7 @@ RUF101.py:3:15: RUF101 [*] `RUF940` is a redirect to `RUF950`
5 5 | x = 2 # noqa: RUF940, RUF950, RUF940
6 6 | x = 2 # noqa: RUF940, RUF950, RUF940, RUF950

RUF101.py:4:23: RUF101 [*] `RUF940` is a redirect to `RUF950`
RUF101_0.py:4:23: RUF101 [*] `RUF940` is a redirect to `RUF950`
|
2 | x = 2 # noqa: RUF950
3 | x = 2 # noqa: RUF940, RUF950
Expand All @@ -59,7 +59,7 @@ RUF101.py:4:23: RUF101 [*] `RUF940` is a redirect to `RUF950`
5 5 | x = 2 # noqa: RUF940, RUF950, RUF940
6 6 | x = 2 # noqa: RUF940, RUF950, RUF940, RUF950

RUF101.py:5:15: RUF101 [*] `RUF940` is a redirect to `RUF950`
RUF101_0.py:5:15: RUF101 [*] `RUF940` is a redirect to `RUF950`
|
3 | x = 2 # noqa: RUF940, RUF950
4 | x = 2 # noqa: RUF950, RUF940, RUF950, RUF950, RUF950
Expand All @@ -77,7 +77,7 @@ RUF101.py:5:15: RUF101 [*] `RUF940` is a redirect to `RUF950`
5 |+x = 2 # noqa: RUF950, RUF950, RUF940
6 6 | x = 2 # noqa: RUF940, RUF950, RUF940, RUF950

RUF101.py:5:31: RUF101 [*] `RUF940` is a redirect to `RUF950`
RUF101_0.py:5:31: RUF101 [*] `RUF940` is a redirect to `RUF950`
|
3 | x = 2 # noqa: RUF940, RUF950
4 | x = 2 # noqa: RUF950, RUF940, RUF950, RUF950, RUF950
Expand All @@ -95,7 +95,7 @@ RUF101.py:5:31: RUF101 [*] `RUF940` is a redirect to `RUF950`
5 |+x = 2 # noqa: RUF940, RUF950, RUF950
6 6 | x = 2 # noqa: RUF940, RUF950, RUF940, RUF950

RUF101.py:6:15: RUF101 [*] `RUF940` is a redirect to `RUF950`
RUF101_0.py:6:15: RUF101 [*] `RUF940` is a redirect to `RUF950`
|
4 | x = 2 # noqa: RUF950, RUF940, RUF950, RUF950, RUF950
5 | x = 2 # noqa: RUF940, RUF950, RUF940
Expand All @@ -111,7 +111,7 @@ RUF101.py:6:15: RUF101 [*] `RUF940` is a redirect to `RUF950`
6 |-x = 2 # noqa: RUF940, RUF950, RUF940, RUF950
6 |+x = 2 # noqa: RUF950, RUF950, RUF940, RUF950

RUF101.py:6:31: RUF101 [*] `RUF940` is a redirect to `RUF950`
RUF101_0.py:6:31: RUF101 [*] `RUF940` is a redirect to `RUF950`
|
4 | x = 2 # noqa: RUF950, RUF940, RUF950, RUF950, RUF950
5 | x = 2 # noqa: RUF940, RUF950, RUF940
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
---
source: crates/ruff_linter/src/rules/ruff/mod.rs
snapshot_kind: text
---
RUF101_1.py:5:15: RUF101 [*] `TCH002` is a redirect to `TC002`
|
3 | RUF101 should trigger here because the TCH rules have been recoded to TC.
4 | """
5 | # ruff: noqa: TCH002
| ^^^^^^ RUF101
6 |
7 | from __future__ import annotations
|
= help: Replace with `TC002`

Safe fix
2 2 |
3 3 | RUF101 should trigger here because the TCH rules have been recoded to TC.
4 4 | """
5 |-# ruff: noqa: TCH002
5 |+# ruff: noqa: TC002
6 6 |
7 7 | from __future__ import annotations
8 8 |
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
---
source: crates/ruff_linter/src/noqa.rs
expression: "ParsedFileExemption::try_extract(source)"
expression: "ParsedFileExemption::try_extract(TextRange::up_to(source.text_len()), source,)"
snapshot_kind: text
---
Ok(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
---
source: crates/ruff_linter/src/noqa.rs
expression: "ParsedFileExemption::try_extract(source)"
expression: "ParsedFileExemption::try_extract(TextRange::up_to(source.text_len()), source,)"
snapshot_kind: text
---
Ok(
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
---
source: crates/ruff_linter/src/noqa.rs
expression: "ParsedFileExemption::try_extract(source)"
expression: "ParsedFileExemption::try_extract(TextRange::up_to(source.text_len()), source,)"
snapshot_kind: text
---
Ok(
Expand Down
Loading

0 comments on commit 6fcbe8e

Please sign in to comment.