Implement RUF028 to detect useless formatter suppression comments (#9899

)

<!--
Thank you for contributing to Ruff! To help us out with reviewing,
please consider the following:

- Does this pull request include a summary of the change? (See below.)
- Does this pull request include a descriptive title?
- Does this pull request include references to any relevant issues?
-->
Fixes #6611

## Summary

This lint rule spots comments that are _intended_ to suppress or enable
the formatter, but will be ignored by the Ruff formatter.

We borrow some functions the formatter uses for determining comment
placement / putting them in context within an AST.

The analysis function uses an AST visitor to visit each comment and
attach it to the AST. It then uses that context to check:
1. Is this comment in an expression?
2. Does this comment have bad placement? (e.g. a `# fmt: skip` above a
function instead of at the end of a line)
3. Is this comment redundant?
4. Does this comment actually suppress any code?
5. Does this comment have ambiguous placement? (e.g. a `# fmt: off`
above an `else:` block)

If any of these are true, a violation is thrown. The reported reason
depends on the order of the above check-list: in other words, a `# fmt:
skip` comment on its own line within a list expression will be reported
as being in an expression, since that reason takes priority.

The lint suggests removing the comment as an unsafe fix, regardless of
the reason.

## Test Plan

A snapshot test has been created.

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

@@ -0,0 +1,63 @@
+def fmt_off_between_lists():
+    test_list = [
+        # fmt: off
+        1,
+        2,
+        3,
+    ]
+
+
+# note: the second `fmt: skip`` should be OK
+def fmt_skip_on_own_line():
+    # fmt: skip
+    pass  # fmt: skip
+
+
+@fmt_skip_on_own_line
+# fmt: off
+@fmt_off_between_lists
+def fmt_off_between_decorators():
+    pass
+
+
+@fmt_off_between_decorators
+# fmt: off
+class FmtOffBetweenClassDecorators:
+    ...
+
+
+def fmt_off_in_else():
+    x = [1, 2, 3]
+    for val in x:
+        print(x)
+    # fmt: off
+    else:
+        print("done")
+    while False:
+        print("while")
+        # fmt: off
+    # fmt: off
+    else:
+        print("done")
+    if len(x) > 3:
+        print("huh?")
+    # fmt: on
+    # fmt: off
+    else:
+        print("expected")
+
+
+class Test:
+    @classmethod
+    # fmt: off
+    def cls_method_a(
+        # fmt: off
+        cls,
+    ) -> None: # noqa: test # fmt: skip
+        pass
+
+
+def fmt_on_trailing():
+    # fmt: off
+    val = 5 # fmt: on
+    pass # fmt: on

crates/ruff_linter/src/checkers/ast/analyze/module.rs

@@ -2,11 +2,14 @@ use ruff_python_ast::Suite;
 
 use crate::checkers::ast::Checker;
 use crate::codes::Rule;
-use crate::rules::flake8_bugbear;
+use crate::rules::{flake8_bugbear, ruff};
 
 /// Run lint rules over a module.
 pub(crate) fn module(suite: &Suite, checker: &mut Checker) {
     if checker.enabled(Rule::FStringDocstring) {
         flake8_bugbear::rules::f_string_docstring(checker, suite);
     }
+    if checker.enabled(Rule::InvalidFormatterSuppressionComment) {
+        ruff::rules::ignored_formatter_suppression_comment(checker, suite);
+    }
 }

crates/ruff_linter/src/checkers/noqa.rs

@@ -3,12 +3,13 @@
 use std::path::Path;
 
 use itertools::Itertools;
-use ruff_text_size::{Ranged, TextLen, TextRange};
+use ruff_text_size::Ranged;
 
 use ruff_diagnostics::{Diagnostic, Edit, Fix};
-use ruff_python_trivia::{CommentRanges, PythonWhitespace};
+use ruff_python_trivia::CommentRanges;
 use ruff_source_file::Locator;
 
+use crate::fix::edits::delete_comment;
 use crate::noqa;
 use crate::noqa::{Directive, FileExemption, NoqaDirectives, NoqaMapping};
 use crate::registry::{AsRule, Rule, RuleSet};
@@ -111,7 +112,8 @@ pub(crate) fn check_noqa(
                     if line.matches.is_empty() {
                         let mut diagnostic =
                             Diagnostic::new(UnusedNOQA { codes: None }, directive.range());
-                        diagnostic.set_fix(Fix::safe_edit(delete_noqa(directive.range(), locator)));
+                        diagnostic
+                            .set_fix(Fix::safe_edit(delete_comment(directive.range(), locator)));
 
                         diagnostics.push(diagnostic);
                     }
@@ -177,8 +179,10 @@ pub(crate) fn check_noqa(
                             directive.range(),
                         );
                         if valid_codes.is_empty() {
-                            diagnostic
-                                .set_fix(Fix::safe_edit(delete_noqa(directive.range(), locator)));
+                            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(", ")),
@@ -195,48 +199,3 @@ pub(crate) fn check_noqa(
     ignored_diagnostics.sort_unstable();
     ignored_diagnostics
 }
-
-/// Generate a [`Edit`] to delete a `noqa` directive.
-fn delete_noqa(range: TextRange, locator: &Locator) -> Edit {
-    let line_range = locator.line_range(range.start());
-
-    // Compute the leading space.
-    let prefix = locator.slice(TextRange::new(line_range.start(), range.start()));
-    let leading_space_len = prefix.text_len() - prefix.trim_whitespace_end().text_len();
-
-    // Compute the trailing space.
-    let suffix = locator.slice(TextRange::new(range.end(), line_range.end()));
-    let trailing_space_len = suffix.text_len() - suffix.trim_whitespace_start().text_len();
-
-    // Ex) `# noqa`
-    if line_range
-        == TextRange::new(
-            range.start() - leading_space_len,
-            range.end() + trailing_space_len,
-        )
-    {
-        let full_line_end = locator.full_line_end(line_range.end());
-        Edit::deletion(line_range.start(), full_line_end)
-    }
-    // Ex) `x = 1  # noqa`
-    else if range.end() + trailing_space_len == line_range.end() {
-        Edit::deletion(range.start() - leading_space_len, line_range.end())
-    }
-    // Ex) `x = 1  # noqa  # type: ignore`
-    else if locator
-        .slice(TextRange::new(
-            range.end() + trailing_space_len,
-            line_range.end(),
-        ))
-        .starts_with('#')
-    {
-        Edit::deletion(range.start(), range.end() + trailing_space_len)
-    }
-    // Ex) `x = 1  # noqa here`
-    else {
-        Edit::deletion(
-            range.start() + "# ".text_len(),
-            range.end() + trailing_space_len,
-        )
-    }
-}

crates/ruff_linter/src/codes.rs

@@ -945,6 +945,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
         (Ruff, "025") => (RuleGroup::Preview, rules::ruff::rules::UnnecessaryDictComprehensionForIterable),
         (Ruff, "026") => (RuleGroup::Preview, rules::ruff::rules::DefaultFactoryKwarg),
         (Ruff, "027") => (RuleGroup::Preview, rules::ruff::rules::MissingFStringSyntax),
+        (Ruff, "028") => (RuleGroup::Preview, rules::ruff::rules::InvalidFormatterSuppressionComment),
         (Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA),
         (Ruff, "200") => (RuleGroup::Stable, rules::ruff::rules::InvalidPyprojectToml),
         #[cfg(feature = "test-rules")]

crates/ruff_linter/src/fix/edits.rs

@@ -62,6 +62,51 @@ pub(crate) fn delete_stmt(
     }
 }
 
+/// Generate a [`Edit`] to delete a comment (for example: a `noqa` directive).
+pub(crate) fn delete_comment(range: TextRange, locator: &Locator) -> Edit {
+    let line_range = locator.line_range(range.start());
+
+    // Compute the leading space.
+    let prefix = locator.slice(TextRange::new(line_range.start(), range.start()));
+    let leading_space_len = prefix.text_len() - prefix.trim_whitespace_end().text_len();
+
+    // Compute the trailing space.
+    let suffix = locator.slice(TextRange::new(range.end(), line_range.end()));
+    let trailing_space_len = suffix.text_len() - suffix.trim_whitespace_start().text_len();
+
+    // Ex) `# noqa`
+    if line_range
+        == TextRange::new(
+            range.start() - leading_space_len,
+            range.end() + trailing_space_len,
+        )
+    {
+        let full_line_end = locator.full_line_end(line_range.end());
+        Edit::deletion(line_range.start(), full_line_end)
+    }
+    // Ex) `x = 1  # noqa`
+    else if range.end() + trailing_space_len == line_range.end() {
+        Edit::deletion(range.start() - leading_space_len, line_range.end())
+    }
+    // Ex) `x = 1  # noqa  # type: ignore`
+    else if locator
+        .slice(TextRange::new(
+            range.end() + trailing_space_len,
+            line_range.end(),
+        ))
+        .starts_with('#')
+    {
+        Edit::deletion(range.start(), range.end() + trailing_space_len)
+    }
+    // Ex) `x = 1  # noqa here`
+    else {
+        Edit::deletion(
+            range.start() + "# ".text_len(),
+            range.end() + trailing_space_len,
+        )
+    }
+}
+
 /// Generate a `Fix` to remove the specified imports from an `import` statement.
 pub(crate) fn remove_unused_imports<'a>(
     member_names: impl Iterator<Item = &'a str>,

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

@@ -49,6 +49,7 @@ mod tests {
     #[test_case(Rule::MissingFStringSyntax, Path::new("RUF027_0.py"))]
     #[test_case(Rule::MissingFStringSyntax, Path::new("RUF027_1.py"))]
     #[test_case(Rule::MissingFStringSyntax, Path::new("RUF027_2.py"))]
+    #[test_case(Rule::InvalidFormatterSuppressionComment, Path::new("RUF028.py"))]
     fn rules(rule_code: Rule, path: &Path) -> Result<()> {
         let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
         let diagnostics = test_path(