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
70 changes: 55 additions & 15 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(&locator.contents()[range], range.start()) {
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,23 @@ 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> {
fn try_extract(line: &'a str, offset: TextSize) -> Result<Option<Self>, ParseError> {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: The current signature is ambigious in the sense that it's unclear whether offset is an offset relative to line (that's what I would assume) or if it's an absolute index into the source text.

I suggest changingt the signature to try_extract(comment_range: TextRange, source: &'a str) to make this clear (we commonly use source to refer to the entire source text).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah that makes sense. I was just trying to copy the signature of Directive::try_extract, but this makes more sense here.

let init_line_len = line.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.len() - '#'.len_utf8();
Copy link
Member

@MichaReiser MichaReiser Nov 27, 2024

Choose a reason for hiding this comment

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

Nit: I suggest using TextLen methods here to avoid the conversion further down

Suggested change
let init_line_len = line.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.len() - '#'.len_utf8();
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 +472,12 @@ 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.len();
codes.push(Code {
code,
range: TextRange::at(TextSize::try_from(codes_end).unwrap(), code.text_len())
.add(offset),
});
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let codes_end = init_line_len - line.len();
codes.push(Code {
code,
range: TextRange::at(TextSize::try_from(codes_end).unwrap(), code.text_len())
.add(offset),
});
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 +493,15 @@ impl<'a> ParsedFileExemption<'a> {
return Err(ParseError::MissingCodes);
}

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

Expand Down Expand Up @@ -1226,50 +1242,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(
source,
TextSize::default()
));
}

#[test]
fn ruff_exemption_all() {
let source = "# ruff: noqa";
assert_debug_snapshot!(ParsedFileExemption::try_extract(source));
assert_debug_snapshot!(ParsedFileExemption::try_extract(
source,
TextSize::default()
));
}

#[test]
fn flake8_exemption_all_no_space() {
let source = "#flake8:noqa";
assert_debug_snapshot!(ParsedFileExemption::try_extract(source));
assert_debug_snapshot!(ParsedFileExemption::try_extract(
source,
TextSize::default()
));
}

#[test]
fn ruff_exemption_all_no_space() {
let source = "#ruff:noqa";
assert_debug_snapshot!(ParsedFileExemption::try_extract(source));
assert_debug_snapshot!(ParsedFileExemption::try_extract(
source,
TextSize::default()
));
}

#[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(
source,
TextSize::default()
));
}

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

#[test]
fn flake8_exemption_all_case_insensitive() {
let source = "# flake8: NoQa";
assert_debug_snapshot!(ParsedFileExemption::try_extract(source));
assert_debug_snapshot!(ParsedFileExemption::try_extract(
source,
TextSize::default()
));
}

#[test]
fn ruff_exemption_all_case_insensitive() {
let source = "# ruff: NoQa";
assert_debug_snapshot!(ParsedFileExemption::try_extract(source));
assert_debug_snapshot!(ParsedFileExemption::try_extract(
source,
TextSize::default()
));
}

#[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
33 changes: 31 additions & 2 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::{Directive, FileNoqaDirectives, NoqaDirectives, ParsedFileExemption};
use crate::rule_redirects::get_redirect_target;

/// ## What it does
Expand Down Expand Up @@ -42,7 +42,7 @@ 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 {
Expand All @@ -67,3 +67,32 @@ pub(crate) fn redirected_noqa(diagnostics: &mut Vec<Diagnostic>, noqa_directives
}
}
}

/// 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;
};

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);
}
}
}
MichaReiser marked this conversation as resolved.
Show resolved Hide resolved
}
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,15 +1,24 @@
---
source: crates/ruff_linter/src/noqa.rs
expression: "ParsedFileExemption::try_extract(source)"
expression: "ParsedFileExemption::try_extract(source, TextSize::default())"
snapshot_kind: text
---
Ok(
Some(
Codes(
[
"F401",
"F841",
],
Codes {
range: 0..26,
codes: [
Code {
code: "F401",
range: 16..20,
},
Code {
code: "F841",
range: 22..26,
},
],
},
),
),
)
Original file line number Diff line number Diff line change
@@ -1,15 +1,24 @@
---
source: crates/ruff_linter/src/noqa.rs
expression: "ParsedFileExemption::try_extract(source)"
expression: "ParsedFileExemption::try_extract(source, TextSize::default())"
snapshot_kind: text
---
Ok(
Some(
Codes(
[
"F401",
"F841",
],
Codes {
range: 0..24,
codes: [
Code {
code: "F401",
range: 14..18,
},
Code {
code: "F841",
range: 20..24,
},
],
},
),
),
)