From a99832088a31b65f79e578e3131756cfdc0afb0d Mon Sep 17 00:00:00 2001 From: Steve C Date: Thu, 29 Aug 2024 01:20:16 -0400 Subject: [PATCH] [`ruff`] - extend comment deletions for unused-noqa (`RUF100`) (#13105) ## Summary Extends deletions for RUF100, deleting trailing text from noqa directives, while preserving upcoming comments on the same line if any. In cases where it deletes a comment up to another comment on the same line, the whitespace between them is now shown to be in the autofix in the diagnostic as well. Leading whitespace before the removed comment is not, though. Fixes #12251 ## Test Plan `cargo test` --- .../resources/test/fixtures/ruff/RUF100_5.py | 10 +++++ crates/ruff_linter/src/checkers/noqa.rs | 24 +++++----- crates/ruff_linter/src/fix/edits.rs | 7 +-- ..._linter__rules__ruff__tests__ruf100_3.snap | 10 ++--- ..._linter__rules__ruff__tests__ruf100_5.snap | 45 +++++++++++++++++++ 5 files changed, 72 insertions(+), 24 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/ruff/RUF100_5.py b/crates/ruff_linter/resources/test/fixtures/ruff/RUF100_5.py index d5d7f47b02cff..e212fd3390067 100644 --- a/crates/ruff_linter/resources/test/fixtures/ruff/RUF100_5.py +++ b/crates/ruff_linter/resources/test/fixtures/ruff/RUF100_5.py @@ -9,3 +9,13 @@ #import os # noqa: E501 + +def f(): + data = 1 + # line below should autofix to `return data # fmt: skip` + return data # noqa: RET504 # fmt: skip + +def f(): + data = 1 + # line below should autofix to `return data` + return data # noqa: RET504 - intentional incorrect noqa, will be removed diff --git a/crates/ruff_linter/src/checkers/noqa.rs b/crates/ruff_linter/src/checkers/noqa.rs index e893551dfcd4b..947c92547aa8a 100644 --- a/crates/ruff_linter/src/checkers/noqa.rs +++ b/crates/ruff_linter/src/checkers/noqa.rs @@ -118,10 +118,10 @@ pub(crate) fn check_noqa( match &line.directive { Directive::All(directive) => { if line.matches.is_empty() { + let edit = delete_comment(directive.range(), locator); let mut diagnostic = Diagnostic::new(UnusedNOQA { codes: None }, directive.range()); - diagnostic - .set_fix(Fix::safe_edit(delete_comment(directive.range(), locator))); + diagnostic.set_fix(Fix::safe_edit(edit)); diagnostics.push(diagnostic); } @@ -172,6 +172,14 @@ pub(crate) fn check_noqa( && unknown_codes.is_empty() && unmatched_codes.is_empty()) { + let edit = if valid_codes.is_empty() { + delete_comment(directive.range(), locator) + } else { + Edit::range_replacement( + format!("# noqa: {}", valid_codes.join(", ")), + directive.range(), + ) + }; let mut diagnostic = Diagnostic::new( UnusedNOQA { codes: Some(UnusedCodes { @@ -195,17 +203,7 @@ pub(crate) fn check_noqa( }, directive.range(), ); - if valid_codes.is_empty() { - diagnostic.set_fix(Fix::safe_edit(delete_comment( - directive.range(), - locator, - ))); - } else { - diagnostic.set_fix(Fix::safe_edit(Edit::range_replacement( - format!("# noqa: {}", valid_codes.join(", ")), - directive.range(), - ))); - } + diagnostic.set_fix(Fix::safe_edit(edit)); diagnostics.push(diagnostic); } } diff --git a/crates/ruff_linter/src/fix/edits.rs b/crates/ruff_linter/src/fix/edits.rs index 9b440285fe9f3..7b1fa7ebb7cfe 100644 --- a/crates/ruff_linter/src/fix/edits.rs +++ b/crates/ruff_linter/src/fix/edits.rs @@ -99,11 +99,8 @@ pub(crate) fn delete_comment(range: TextRange, locator: &Locator) -> Edit { } // Ex) `x = 1 # noqa here` else { - // Replace `# noqa here` with `# here`. - Edit::range_replacement( - "# ".to_string(), - TextRange::new(range.start(), range.end() + trailing_space_len), - ) + // Remove `# noqa here` and whitespace + Edit::deletion(range.start() - leading_space_len, line_range.end()) } } diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__ruf100_3.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__ruf100_3.snap index 5f38b489e2ab9..1ccfc73987d4e 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__ruf100_3.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__ruf100_3.snap @@ -112,7 +112,7 @@ RUF100_3.py:6:10: RUF100 [*] Unused blanket `noqa` directive 4 4 | print() # noqa # comment 5 5 | print() # noqa # comment 6 |-print() # noqa comment - 6 |+print() # comment + 6 |+print() 7 7 | print() # noqa comment 8 8 | print(a) # noqa 9 9 | print(a) # noqa # comment @@ -133,7 +133,7 @@ RUF100_3.py:7:10: RUF100 [*] Unused blanket `noqa` directive 5 5 | print() # noqa # comment 6 6 | print() # noqa comment 7 |-print() # noqa comment - 7 |+print() # comment + 7 |+print() 8 8 | print(a) # noqa 9 9 | print(a) # noqa # comment 10 10 | print(a) # noqa # comment @@ -257,7 +257,7 @@ RUF100_3.py:19:10: RUF100 [*] Unused `noqa` directive (unused: `E501`, `F821`) 17 17 | print() # noqa: E501, F821 # comment 18 18 | print() # noqa: E501, F821 # comment 19 |-print() # noqa: E501, F821 comment - 19 |+print() # comment + 19 |+print() 20 20 | print() # noqa: E501, F821 comment 21 21 | print(a) # noqa: E501, F821 22 22 | print(a) # noqa: E501, F821 # comment @@ -278,7 +278,7 @@ RUF100_3.py:20:10: RUF100 [*] Unused `noqa` directive (unused: `E501`, `F821`) 18 18 | print() # noqa: E501, F821 # comment 19 19 | print() # noqa: E501, F821 comment 20 |-print() # noqa: E501, F821 comment - 20 |+print() # comment + 20 |+print() 21 21 | print(a) # noqa: E501, F821 22 22 | print(a) # noqa: E501, F821 # comment 23 23 | print(a) # noqa: E501, F821 # comment @@ -428,5 +428,3 @@ RUF100_3.py:28:39: RUF100 [*] Unused `noqa` directive (unused: `E501`) 27 27 | print(a) # comment with unicode µ # noqa: E501 28 |-print(a) # comment with unicode µ # noqa: E501, F821 28 |+print(a) # comment with unicode µ # noqa: F821 - - diff --git a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__ruf100_5.snap b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__ruf100_5.snap index a58ef10cb160d..23e5840e5720c 100644 --- a/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__ruf100_5.snap +++ b/crates/ruff_linter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__ruf100_5.snap @@ -24,6 +24,8 @@ RUF100_5.py:11:1: ERA001 Found commented-out code | 11 | #import os # noqa: E501 | ^^^^^^^^^^^^^^^^^^^^^^^^ ERA001 +12 | +13 | def f(): | = help: Remove commented-out code @@ -32,11 +34,16 @@ RUF100_5.py:11:1: ERA001 Found commented-out code 9 9 | 10 10 | 11 |-#import os # noqa: E501 +12 11 | +13 12 | def f(): +14 13 | data = 1 RUF100_5.py:11:13: RUF100 [*] Unused `noqa` directive (unused: `E501`) | 11 | #import os # noqa: E501 | ^^^^^^^^^^^^ RUF100 +12 | +13 | def f(): | = help: Remove unused `noqa` directive @@ -46,5 +53,43 @@ RUF100_5.py:11:13: RUF100 [*] Unused `noqa` directive (unused: `E501`) 10 10 | 11 |-#import os # noqa: E501 11 |+#import os +12 12 | +13 13 | def f(): +14 14 | data = 1 +RUF100_5.py:16:18: RUF100 [*] Unused `noqa` directive (non-enabled: `RET504`) + | +14 | data = 1 +15 | # line below should autofix to `return data # fmt: skip` +16 | return data # noqa: RET504 # fmt: skip + | ^^^^^^^^^^^^^^ RUF100 +17 | +18 | def f(): + | + = help: Remove unused `noqa` directive + +ℹ Safe fix +13 13 | def f(): +14 14 | data = 1 +15 15 | # line below should autofix to `return data # fmt: skip` +16 |- return data # noqa: RET504 # fmt: skip + 16 |+ return data # fmt: skip +17 17 | +18 18 | def f(): +19 19 | data = 1 +RUF100_5.py:21:18: RUF100 [*] Unused `noqa` directive (non-enabled: `RET504`) + | +19 | data = 1 +20 | # line below should autofix to `return data` +21 | return data # noqa: RET504 - intentional incorrect noqa, will be removed + | ^^^^^^^^^^^^^^ RUF100 + | + = help: Remove unused `noqa` directive + +ℹ Safe fix +18 18 | def f(): +19 19 | data = 1 +20 20 | # line below should autofix to `return data` +21 |- return data # noqa: RET504 - intentional incorrect noqa, will be removed + 21 |+ return data