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

ntBre · 2024-11-27T14:35:03Z

Summary

Fixes #14531, where ruff was not reporting diagnostics on file-level # ruff: noqa comments with redirected rule codes.

The main change here is making ParsedFileExemption closer in structure to Directive, with a sequence of Codes with TextRanges instead of just &str for diagnostic reporting.

Test Plan

The changes were tested against the example code from #14531 to make sure the diagnostic is now emitted.

github-actions · 2024-11-27T14:41:27Z

`ruff-ecosystem` results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

MichaReiser · 2024-11-27T14:50:19Z

crates/ruff_linter/src/rules/ruff/rules/redirected_noqa.rs

+        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);
+            }
+        }
+    }


Hmm, it's annoying that FileNoqaDirectives and NoqaDirectives have such a different structure. But maybe there's an opportunity to reuse some of the code in this loop?

Oh yes, great idea! I factored this out into a build_diagnostics function.

MichaReiser · 2024-11-27T14:51:43Z

crates/ruff_linter/resources/test/fixtures/ruff/RUF101_1.py

+
+RUF101 should trigger here because the TCH rules have been recoded to TC.
+"""
+# ruff: noqa: TCH002


Would you mind adding an example where it's a non-direct-match redirect. E.g. TCH or TCH0

I tried # ruff: noqa: TCH, and it looks like it's not even being detected as a valid code (ParsedFileExemption::try_extract returns Err(MissingCodes)). Is that what you expected, or am I misunderstanding/broke something? I rolled back to before my changes, and I'm still seeing the Err(MissingCodes), so I'm probably misunderstanding.

It's probably me. It's very likely that Ruff doesn't support file-level suppressions for selectors? If that's the case, then we're all good here

Yeah I think it has to be an exact code or a catch-all (so TCH wouldn't be allowed).

MichaReiser

Neat!

MichaReiser · 2024-11-27T14:54:50Z

crates/ruff_linter/src/noqa.rs

 }

 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> {


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).

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

MichaReiser · 2024-11-27T14:55:39Z

crates/ruff_linter/src/noqa.rs

+        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();


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();

MichaReiser · 2024-11-27T14:56:23Z

crates/ruff_linter/src/noqa.rs

+                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),
+                });


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),

});

MichaReiser · 2024-11-27T16:09:32Z

crates/ruff_linter/src/noqa.rs

 }

 impl<'a> ParsedFileExemption<'a> {
    /// Return a [`ParsedFileExemption`] for a given comment line.
-    fn try_extract(line: &'a str) -> Result<Option<Self>, ParseError> {
-        let line = Self::lex_whitespace(line);
+    fn try_extract(comment_range: TextSize, source: &'a str) -> Result<Option<Self>, ParseError> {


Sorry that I haven't been clear about what signature I'd expect. I think this signature still has the same "flaw" in that it's unclear if comment_range is an offset relative to source or the entire source text of the file.

To avoid this, I suggest changing the signature to try_extract(comment_range: TextRange, source; &'a str) and then call it like this: ParsedFileExemption::try_extract(range, locator.contents()); It's then the try_extract's responsible to slice the comment text out of the source text.

Oh sorry about that. I missed that you suggested TextRange instead of TextSize 🤦 This makes much more sense to me now.

MichaReiser · 2024-11-27T16:40:49Z

Perfect. Let me know when it's ready to merge and I hit the button

ntBre · 2024-11-27T16:50:03Z

Awesome, thanks for the reviews! I just noticed that the snapshot metadata is not correct anymore after the signature change (the expression lines should look like expression: "ParsedFileExemption::try_extract(TextRange::up_to(source.text_len()), source,)" now). I ran cargo insta test --force-update-snapshots, but it changed the metadata in a ton of other snapshots too. Should I just update the ones I actually changed here, or ignore this for now?

Otherwise ready to merge!

MichaReiser · 2024-11-27T16:51:43Z

Should I just update the ones I actually changed here, or ignore this for now?

Ideally yes. You can also try to update cargo-insta to a newer version

ntBre · 2024-11-27T17:08:38Z

I think the newest version of insta might be the problem (mitsuhiko/insta#676; adding snapshot_kind: text to the metadata now), but I just added the real changes I caused.

MichaReiser · 2024-11-27T17:25:11Z

I think the newest version of insta might be the problem (mitsuhiko/insta#676; adding snapshot_kind: text to the metadata now), but I just added the real changes I caused.

Oh interesting: I thought I updated all snapshots to use the new format but quiet possible that I missed some.

AlexWaygood · 2024-11-27T17:41:35Z

Oh interesting: I thought I updated all snapshots to use the new format but quiet possible that I missed some.

I think some of them have been changed back (and some new ones with the old format have been added) because not all contributors have the latest version of cargo-insta installed

ntBre added 6 commits November 26, 2024 16:03

add test case from astral-sh#14531

7a6dd87

detect noqa redirections at file level

7953e8f

include comment character in noqa range

7676a23

update code representation

28a1426

call as_str once

24829c5

use len_utf8 like Directive

e0afd4a

MichaReiser reviewed Nov 27, 2024

View reviewed changes

MichaReiser approved these changes Nov 27, 2024

View reviewed changes

MichaReiser added the rule Implementing or modifying a lint rule label Nov 27, 2024

ntBre added 3 commits November 27, 2024 10:20

factor out build_diagnostics

3da4bba

update ParsedFileExemption::try_extract signature

6e55a75

use TextLen throughout

212b9a3

MichaReiser reviewed Nov 27, 2024

View reviewed changes

actually fix try_extract signature

2af107d

update snapshot metadata to match try_extract signature

eb5bb0b

MichaReiser merged commit 6fcbe8e into astral-sh:main Nov 27, 2024
21 checks passed

ntBre deleted the brent/fix-14531 branch November 27, 2024 17:52

BrewTestBot mentioned this pull request Nov 29, 2024

ruff 0.8.1 Homebrew/homebrew-core#199449

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ntBre commented Nov 27, 2024

github-actions bot commented Nov 27, 2024 •

edited

Loading

MichaReiser Nov 27, 2024 •

edited by AlexWaygood

Loading

ntBre Nov 27, 2024

MichaReiser Nov 27, 2024

ntBre Nov 27, 2024

MichaReiser Nov 27, 2024

charliermarsh Nov 27, 2024

MichaReiser left a comment

MichaReiser Nov 27, 2024

ntBre Nov 27, 2024

MichaReiser Nov 27, 2024 •

edited by AlexWaygood

Loading

MichaReiser Nov 27, 2024

MichaReiser Nov 27, 2024

ntBre Nov 27, 2024

MichaReiser commented Nov 27, 2024

ntBre commented Nov 27, 2024

MichaReiser commented Nov 27, 2024

ntBre commented Nov 27, 2024

MichaReiser commented Nov 27, 2024

AlexWaygood commented Nov 27, 2024 •

edited

Loading

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

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

Conversation

ntBre commented Nov 27, 2024

Summary

Test Plan

github-actions bot commented Nov 27, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

MichaReiser Nov 27, 2024 • edited by AlexWaygood Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser Nov 27, 2024 • edited by AlexWaygood Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaReiser commented Nov 27, 2024

ntBre commented Nov 27, 2024

MichaReiser commented Nov 27, 2024

ntBre commented Nov 27, 2024

MichaReiser commented Nov 27, 2024

AlexWaygood commented Nov 27, 2024 • edited Loading

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

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

github-actions bot commented Nov 27, 2024 •

edited

Loading

`ruff-ecosystem` results

MichaReiser Nov 27, 2024 •

edited by AlexWaygood

Loading

MichaReiser Nov 27, 2024 •

edited by AlexWaygood

Loading

AlexWaygood commented Nov 27, 2024 •

edited

Loading