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

[ruff] Detect redirected-noqa in file-level comments (RUF101) #14635

Merged
merged 11 commits into from
Nov 27, 2024
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
MichaReiser marked this conversation as resolved.
Show resolved Hide resolved

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, violation};
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