Avoid removing comment hash for noqa's with trailing content (#3589)

crates/ruff/resources/test/fixtures/ruff/RUF100_3.py

@@ -2,12 +2,24 @@
 # noqa # comment
 print()  # noqa
 print()  # noqa # comment
+print()  # noqa  # comment
+print()  # noqa comment
+print()  # noqa  comment
 print(a)  # noqa
 print(a)  # noqa # comment
+print(a)  # noqa  # comment
+print(a)  # noqa comment
+print(a)  # noqa  comment
 
 # noqa: E501, F821
 # noqa: E501, F821 # comment
 print()  # noqa: E501, F821
 print()  # noqa: E501, F821 # comment
+print()  # noqa: E501, F821  # comment
+print()  # noqa: E501, F821 comment
+print()  # noqa: E501, F821  comment
 print(a)  # noqa: E501, F821
 print(a)  # noqa: E501, F821 # comment
+print(a)  # noqa: E501, F821  # comment
+print(a)  # noqa: E501, F821 comment
+print(a)  # noqa: E501, F821  comment

crates/ruff/src/checkers/noqa.rs

@@ -143,30 +143,26 @@ pub fn check_noqa(
             match directive {
                 Directive::All(leading_spaces, start_byte, end_byte, trailing_spaces) => {
                     if matches.is_empty() {
-                        let start = lines[row][..start_byte].chars().count();
+                        let start_char = lines[row][..start_byte].chars().count();
-                        let end = start + lines[row][start_byte..end_byte].chars().count();
+                        let end_char =
+                            start_char + lines[row][start_byte..end_byte].chars().count();
 
                         let mut diagnostic = Diagnostic::new(
                             UnusedNOQA { codes: None },
-                            Range::new(Location::new(row + 1, start), Location::new(row + 1, end)),
+                            Range::new(
+                                Location::new(row + 1, start_char),
+                                Location::new(row + 1, end_char),
+                            ),
                         );
                         if autofix.into() && settings.rules.should_fix(diagnostic.kind.rule()) {
-                            if start - leading_spaces == 0 && end == lines[row].chars().count() {
+                            diagnostic.amend(delete_noqa(
-                                diagnostic.amend(Fix::deletion(
+                                row,
-                                    Location::new(row + 1, 0),
+                                lines[row],
-                                    Location::new(row + 2, 0),
+                                leading_spaces,
+                                start_byte,
+                                end_byte,
+                                trailing_spaces,
                             ));
-                            } else if end == lines[row].chars().count() {
-                                diagnostic.amend(Fix::deletion(
-                                    Location::new(row + 1, start - leading_spaces),
-                                    Location::new(row + 1, end + trailing_spaces),
-                                ));
-                            } else {
-                                diagnostic.amend(Fix::deletion(
-                                    Location::new(row + 1, start),
-                                    Location::new(row + 1, end + trailing_spaces),
-                                ));
-                            }
                         }
                         diagnostics.push(diagnostic);
                     }
@@ -207,8 +203,9 @@ pub fn check_noqa(
                         && unknown_codes.is_empty()
                         && unmatched_codes.is_empty())
                     {
-                        let start = lines[row][..start_byte].chars().count();
+                        let start_char = lines[row][..start_byte].chars().count();
-                        let end = start + lines[row][start_byte..end_byte].chars().count();
+                        let end_char =
+                            start_char + lines[row][start_byte..end_byte].chars().count();
 
                         let mut diagnostic = Diagnostic::new(
                             UnusedNOQA {
@@ -227,32 +224,26 @@ pub fn check_noqa(
                                         .collect(),
                                 }),
                             },
-                            Range::new(Location::new(row + 1, start), Location::new(row + 1, end)),
+                            Range::new(
+                                Location::new(row + 1, start_char),
+                                Location::new(row + 1, end_char),
+                            ),
                         );
                         if autofix.into() && settings.rules.should_fix(diagnostic.kind.rule()) {
                             if valid_codes.is_empty() {
-                                if start - leading_spaces == 0 && end == lines[row].chars().count()
+                                diagnostic.amend(delete_noqa(
-                                {
+                                    row,
-                                    diagnostic.amend(Fix::deletion(
+                                    lines[row],
-                                        Location::new(row + 1, 0),
+                                    leading_spaces,
-                                        Location::new(row + 2, 0),
+                                    start_byte,
+                                    end_byte,
+                                    trailing_spaces,
                                 ));
-                                } else if end == lines[row].chars().count() {
-                                    diagnostic.amend(Fix::deletion(
-                                        Location::new(row + 1, start - leading_spaces),
-                                        Location::new(row + 1, end + trailing_spaces),
-                                    ));
-                                } else {
-                                    diagnostic.amend(Fix::deletion(
-                                        Location::new(row + 1, start),
-                                        Location::new(row + 1, end + trailing_spaces),
-                                    ));
-                                }
                             } else {
                                 diagnostic.amend(Fix::replacement(
                                     format!("# noqa: {}", valid_codes.join(", ")),
-                                    Location::new(row + 1, start),
+                                    Location::new(row + 1, start_char),
-                                    Location::new(row + 1, end),
+                                    Location::new(row + 1, end_char),
                                 ));
                             }
                         }
@@ -267,3 +258,42 @@ pub fn check_noqa(
     ignored_diagnostics.sort_unstable();
     ignored_diagnostics
 }
+
+/// Generate a [`Fix`] to delete a `noqa` directive.
+fn delete_noqa(
+    row: usize,
+    line: &str,
+    leading_spaces: usize,
+    start_byte: usize,
+    end_byte: usize,
+    trailing_spaces: usize,
+) -> Fix {
+    if start_byte - leading_spaces == 0 && end_byte == line.len() {
+        // Ex) `# noqa`
+        Fix::deletion(Location::new(row + 1, 0), Location::new(row + 2, 0))
+    } else if end_byte == line.len() {
+        // Ex) `x = 1  # noqa`
+        let start_char = line[..start_byte].chars().count();
+        let end_char = start_char + line[start_byte..end_byte].chars().count();
+        Fix::deletion(
+            Location::new(row + 1, start_char - leading_spaces),
+            Location::new(row + 1, end_char + trailing_spaces),
+        )
+    } else if line[end_byte..].trim_start().starts_with('#') {
+        // Ex) `x = 1  # noqa  # type: ignore`
+        let start_char = line[..start_byte].chars().count();
+        let end_char = start_char + line[start_byte..end_byte].chars().count();
+        Fix::deletion(
+            Location::new(row + 1, start_char),
+            Location::new(row + 1, end_char + trailing_spaces),
+        )
+    } else {
+        // Ex) `x = 1  # noqa here`
+        let start_char = line[..start_byte].chars().count();
+        let end_char = start_char + line[start_byte..end_byte].chars().count();
+        Fix::deletion(
+            Location::new(row + 1, start_char + 1 + 1),
+            Location::new(row + 1, end_char + trailing_spaces),
+        )
+    }
+}

crates/ruff/src/rules/ruff/snapshots/ruff__rules__ruff__tests__ruf100_3.snap

@@ -82,24 +82,84 @@ expression: diagnostics
       row: 4
       column: 16
   parent: ~
+- kind:
+    name: UnusedNOQA
+    body: "Unused blanket `noqa` directive"
+    suggestion: "Remove unused `noqa` directive"
+    fixable: true
+  location:
+    row: 5
+    column: 9
+  end_location:
+    row: 5
+    column: 15
+  fix:
+    content: ""
+    location:
+      row: 5
+      column: 9
+    end_location:
+      row: 5
+      column: 17
+  parent: ~
+- kind:
+    name: UnusedNOQA
+    body: "Unused blanket `noqa` directive"
+    suggestion: "Remove unused `noqa` directive"
+    fixable: true
+  location:
+    row: 6
+    column: 9
+  end_location:
+    row: 6
+    column: 15
+  fix:
+    content: ""
+    location:
+      row: 6
+      column: 11
+    end_location:
+      row: 6
+      column: 16
+  parent: ~
+- kind:
+    name: UnusedNOQA
+    body: "Unused blanket `noqa` directive"
+    suggestion: "Remove unused `noqa` directive"
+    fixable: true
+  location:
+    row: 7
+    column: 9
+  end_location:
+    row: 7
+    column: 15
+  fix:
+    content: ""
+    location:
+      row: 7
+      column: 11
+    end_location:
+      row: 7
+      column: 17
+  parent: ~
 - kind:
     name: UnusedNOQA
     body: "Unused `noqa` directive (unused: `E501`, `F821`)"
     suggestion: "Remove unused `noqa` directive"
     fixable: true
   location:
-    row: 8
+    row: 14
     column: 0
   end_location:
-    row: 8
+    row: 14
     column: 18
   fix:
     content: ""
     location:
-      row: 8
+      row: 14
       column: 0
     end_location:
-      row: 9
+      row: 15
       column: 0
   parent: ~
 - kind:
@@ -108,18 +168,18 @@ expression: diagnostics
     suggestion: "Remove unused `noqa` directive"
     fixable: true
   location:
-    row: 9
+    row: 15
     column: 0
   end_location:
-    row: 9
+    row: 15
     column: 18
   fix:
     content: ""
     location:
-      row: 9
+      row: 15
       column: 0
     end_location:
-      row: 9
+      row: 15
       column: 19
   parent: ~
 - kind:
@@ -128,18 +188,18 @@ expression: diagnostics
     suggestion: "Remove unused `noqa` directive"
     fixable: true
   location:
-    row: 10
+    row: 16
     column: 9
   end_location:
-    row: 10
+    row: 16
     column: 27
   fix:
     content: ""
     location:
-      row: 10
+      row: 16
       column: 7
     end_location:
-      row: 10
+      row: 16
       column: 27
   parent: ~
 - kind:
@@ -148,18 +208,98 @@ expression: diagnostics
     suggestion: "Remove unused `noqa` directive"
     fixable: true
   location:
-    row: 11
+    row: 17
     column: 9
   end_location:
-    row: 11
+    row: 17
     column: 27
   fix:
     content: ""
     location:
-      row: 11
+      row: 17
       column: 9
     end_location:
-      row: 11
+      row: 17
+      column: 28
+  parent: ~
+- kind:
+    name: UnusedNOQA
+    body: "Unused `noqa` directive (unused: `E501`, `F821`)"
+    suggestion: "Remove unused `noqa` directive"
+    fixable: true
+  location:
+    row: 18
+    column: 9
+  end_location:
+    row: 18
+    column: 27
+  fix:
+    content: ""
+    location:
+      row: 18
+      column: 9
+    end_location:
+      row: 18
+      column: 29
+  parent: ~
+- kind:
+    name: UnusedNOQA
+    body: "Unused `noqa` directive (unused: `E501`, `F821`)"
+    suggestion: "Remove unused `noqa` directive"
+    fixable: true
+  location:
+    row: 19
+    column: 9
+  end_location:
+    row: 19
+    column: 27
+  fix:
+    content: ""
+    location:
+      row: 19
+      column: 11
+    end_location:
+      row: 19
+      column: 28
+  parent: ~
+- kind:
+    name: UnusedNOQA
+    body: "Unused `noqa` directive (unused: `E501`, `F821`)"
+    suggestion: "Remove unused `noqa` directive"
+    fixable: true
+  location:
+    row: 20
+    column: 9
+  end_location:
+    row: 20
+    column: 27
+  fix:
+    content: ""
+    location:
+      row: 20
+      column: 11
+    end_location:
+      row: 20
+      column: 29
+  parent: ~
+- kind:
+    name: UnusedNOQA
+    body: "Unused `noqa` directive (unused: `E501`)"
+    suggestion: "Remove unused `noqa` directive"
+    fixable: true
+  location:
+    row: 21
+    column: 10
+  end_location:
+    row: 21
+    column: 28
+  fix:
+    content: "# noqa: F821"
+    location:
+      row: 21
+      column: 10
+    end_location:
+      row: 21
       column: 28
   parent: ~
 - kind:
@@ -168,18 +308,18 @@ expression: diagnostics
     suggestion: "Remove unused `noqa` directive"
     fixable: true
   location:
-    row: 12
+    row: 22
     column: 10
   end_location:
-    row: 12
+    row: 22
     column: 28
   fix:
     content: "# noqa: F821"
     location:
-      row: 12
+      row: 22
       column: 10
     end_location:
-      row: 12
+      row: 22
       column: 28
   parent: ~
 - kind:
@@ -188,18 +328,58 @@ expression: diagnostics
     suggestion: "Remove unused `noqa` directive"
     fixable: true
   location:
-    row: 13
+    row: 23
     column: 10
   end_location:
-    row: 13
+    row: 23
     column: 28
   fix:
     content: "# noqa: F821"
     location:
-      row: 13
+      row: 23
       column: 10
     end_location:
-      row: 13
+      row: 23
+      column: 28
+  parent: ~
+- kind:
+    name: UnusedNOQA
+    body: "Unused `noqa` directive (unused: `E501`)"
+    suggestion: "Remove unused `noqa` directive"
+    fixable: true
+  location:
+    row: 24
+    column: 10
+  end_location:
+    row: 24
+    column: 28
+  fix:
+    content: "# noqa: F821"
+    location:
+      row: 24
+      column: 10
+    end_location:
+      row: 24
+      column: 28
+  parent: ~
+- kind:
+    name: UnusedNOQA
+    body: "Unused `noqa` directive (unused: `E501`)"
+    suggestion: "Remove unused `noqa` directive"
+    fixable: true
+  location:
+    row: 25
+    column: 10
+  end_location:
+    row: 25
+    column: 28
+  fix:
+    content: "# noqa: F821"
+    location:
+      row: 25
+      column: 10
+    end_location:
+      row: 25
       column: 28
   parent: ~