[pylint] Mark fixes unsafe when deleting comments (PLR5501, PLW0120)

crates/ruff_linter/resources/test/fixtures/pylint/collapsible_else_if.py

@@ -144,3 +144,24 @@ def not_ok1_with_mixed_indented_comments():
             pass
         else:
             pass  # final pass comment
+
+
+def not_ok1_with_else_line_comment(a, b):
+    if a:
+        return 1
+    else:  # explain the fallback
+        if b:
+            return 2
+        else:
+            return 3
+
+
+def not_ok1_with_else_line_and_inner_comments():
+    if 1:
+        pass
+    else:  # else line comment
+        # inner comment
+        if 2:
+            pass
+        else:
+            pass  # final pass comment

crates/ruff_linter/resources/test/fixtures/pylint/useless_else_on_loop.py

@@ -146,3 +146,19 @@ def test_retain_comment():
         print("fat chance")
         for j in range(10):
             break
+
+
+def test_delete_else_line_comment_retain_block_comment():
+    """Delete the comment on the `else` line, but retain the comment within the `else` block"""
+    for j in range(10):
+        pass
+    else:  # comment on the `else` line
+        # comment within the `else` block
+        print("fat chance")
+
+
+def test_retain_inline_else_body_comment():
+    """The comment trailing an inline `else` body belongs to its statement"""
+    for j in range(10):
+        pass
+    else: print("fat chance")  # comment on the statement

crates/ruff_linter/src/rules/pylint/rules/collapsible_else_if.rs

@@ -5,14 +5,15 @@ use ruff_macros::{ViolationMetadata, derive_message_formats};
 use ruff_python_ast::{self as ast, ElifElseClause, Stmt};
 use ruff_python_codegen::Stylist;
 use ruff_python_index::Indexer;
+use ruff_python_trivia::CommentRanges;
 use ruff_source_file::LineRanges;
 use ruff_text_size::{Ranged, TextRange};
 
 use crate::Locator;
 use crate::checkers::ast::Checker;
 use crate::codes::Category;
 use crate::fix::edits::adjust_indentation;
-use crate::{Edit, Fix, FixAvailability, Violation};
+use crate::{Applicability, Edit, Fix, FixAvailability, Violation};
 
 /// ## What it does
 /// Checks for `else` blocks that consist of a single `if` statement.
@@ -44,6 +45,10 @@ use crate::{Edit, Fix, FixAvailability, Violation};
 ///         print("Number is zero.")
 /// ```
 ///
+/// ## Fix safety
+/// This fix is marked unsafe if the `else` line has a trailing comment (e.g., `else:  # comment`),
+/// since the fix deletes the comment.
+///
 /// ## References
 /// - [Python documentation: `if` Statements](https://docs.python.org/3/tutorial/controlflow.html#if-statements)
 #[derive(ViolationMetadata)]
@@ -95,6 +100,7 @@ pub(crate) fn collapsible_else_if(checker: &Checker, stmt: &Stmt) {
             checker.locator(),
             checker.indexer(),
             checker.stylist(),
+            checker.comment_ranges(),
         )
     });
 }
@@ -106,6 +112,7 @@ fn convert_to_elif(
     locator: &Locator,
     indexer: &Indexer,
     stylist: &Stylist,
+    comment_ranges: &CommentRanges,
 ) -> Result<Fix> {
     let inner_if_line_start = locator.line_start(first.start());
     let inner_if_line_end = locator.line_end(first.end());
@@ -146,12 +153,21 @@ fn convert_to_elif(
     };
     let indented = format!("{indentation}el{unindented}");
 
-    Ok(Fix::safe_edits(
+    // The `else` line is replaced, so a trailing comment on it (e.g., `else:  # comment`) is
+    // deleted. Comments between the `else` and the `if` are restored above.
+    let applicability = if comment_ranges.intersects(locator.line_range(else_clause.start())) {
+        Applicability::Unsafe
+    } else {
+        Applicability::Safe
+    };
+
+    Ok(Fix::applicable_edits(
         Edit::replacement(
             indented,
             locator.line_start(else_clause.start()),
             inner_if_line_end,
         ),
         trivia,
+        applicability,
     ))
 }

crates/ruff_linter/src/rules/pylint/rules/useless_else_on_loop.rs

@@ -6,14 +6,15 @@ use ruff_python_ast::identifier;
 use ruff_python_ast::{self as ast, ExceptHandler, MatchCase, Stmt};
 use ruff_python_codegen::Stylist;
 use ruff_python_index::Indexer;
+use ruff_python_trivia::CommentRanges;
 use ruff_source_file::LineRanges;
 use ruff_text_size::{Ranged, TextRange};
 
 use crate::Locator;
 use crate::checkers::ast::Checker;
 use crate::codes::Category;
 use crate::fix::edits::adjust_indentation;
-use crate::{Edit, Fix, FixAvailability, Violation};
+use crate::{Applicability, Edit, Fix, FixAvailability, Violation};
 
 /// ## What it does
 /// Checks for `else` clauses on loops without a `break` statement.
@@ -45,6 +46,10 @@ use crate::{Edit, Fix, FixAvailability, Violation};
 /// print("All items printed")
 /// ```
 ///
+/// ## Fix safety
+/// This fix is marked unsafe if the `else` line has a trailing comment (e.g., `else:  # comment`),
+/// since the fix deletes the comment.
+///
 /// ## References
 /// - [Python documentation: `break` and `continue` Statements, and `else` Clauses on Loops](https://docs.python.org/3/tutorial/controlflow.html#break-and-continue-statements-and-else-clauses-on-loops)
 #[derive(ViolationMetadata)]
@@ -83,6 +88,7 @@ pub(crate) fn useless_else_on_loop(checker: &Checker, stmt: &Stmt, body: &[Stmt]
             checker.locator(),
             checker.indexer(),
             checker.stylist(),
+            checker.comment_ranges(),
         )
     });
 }
@@ -136,6 +142,7 @@ fn remove_else(
     locator: &Locator,
     indexer: &Indexer,
     stylist: &Stylist,
+    comment_ranges: &CommentRanges,
 ) -> Result<Fix> {
     let Some(start) = orelse.first() else {
         return Err(anyhow::anyhow!("Empty `else` clause"));
@@ -169,11 +176,22 @@ fn remove_else(
             stylist,
         )?;
 
+        // The `else` line is replaced, so a trailing comment on it (e.g., `else:  # comment`) is
+        // deleted.
+        let applicability = if comment_ranges.intersects(locator.line_range(else_range.start())) {
+            Applicability::Unsafe
+        } else {
+            Applicability::Safe
+        };
+
         // Replace the content from the start of the `else` to the end of the loop.
-        Ok(Fix::safe_edit(Edit::replacement(
-            indented,
-            locator.line_start(else_range.start()),
-            locator.full_line_end(end.end()),
-        )))
+        Ok(Fix::applicable_edit(
+            Edit::replacement(
+                indented,
+                locator.line_start(else_range.start()),
+                locator.full_line_end(end.end()),
+            ),
+            applicability,
+        ))
     }
 }

crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__collapsible-else-if_collapsible_else_if.py.snap

@@ -286,4 +286,61 @@ help: Convert to `elif`
 143 +         pass
 144 +     else:
 145 +         pass  # final pass comment
+146 |
     |
+
+PLR5501 [*] Use `elif` instead of `else` then `if`, to reduce indentation
+   --> collapsible_else_if.py:152:5
+    |
+150 |       if a:
+151 |           return 1
+152 | /     else:  # explain the fallback
+153 | |         if b:
+    | |________^
+154 |               return 2
+155 |           else:
+    |
+help: Convert to `elif`
+    |
+151 |         return 1
+    -     else:  # explain the fallback
+    -         if b:
+    -             return 2
+    -         else:
+    -             return 3
+152 +     elif b:
+153 +         return 2
+154 +     else:
+155 +         return 3
+156 |
+    |
+note: This is an unsafe fix and may change runtime behavior
+
+PLR5501 [*] Use `elif` instead of `else` then `if`, to reduce indentation
+   --> collapsible_else_if.py:162:5
+    |
+160 |       if 1:
+161 |           pass
+162 | /     else:  # else line comment
+163 | |         # inner comment
+164 | |         if 2:
+    | |________^
+165 |               pass
+166 |           else:
+    |
+help: Convert to `elif`
+    |
+161 |         pass
+    -     else:  # else line comment
+    -         # inner comment
+    -         if 2:
+    -             pass
+    -         else:
+    -             pass  # final pass comment
+162 +     # inner comment
+163 +     elif 2:
+164 +         pass
+165 +     else:
+166 +         pass  # final pass comment
+    |
+note: This is an unsafe fix and may change runtime behavior

crates/ruff_linter/src/rules/pylint/snapshots/ruff_linter__rules__pylint__tests__useless-else-on-loop_useless_else_on_loop.py.snap

@@ -19,6 +19,7 @@ help: Remove `else`
 9  +     print("math is broken")
 10 |     return None
    |
+note: This is an unsafe fix and may change runtime behavior
 
 PLW0120 [*] `else` clause on loop without a `break` statement; remove the `else` and dedent its contents
   --> useless_else_on_loop.py:18:5
@@ -38,6 +39,7 @@ help: Remove `else`
 18 +     print("math is broken")
 19 |     return None
    |
+note: This is an unsafe fix and may change runtime behavior
 
 PLW0120 [*] `else` clause on loop without a `break` statement; remove the `else` and dedent its contents
   --> useless_else_on_loop.py:30:1
@@ -56,6 +58,7 @@ help: Remove `else`
 30 + print("or else!")
 31 |
    |
+note: This is an unsafe fix and may change runtime behavior
 
 PLW0120 [*] `else` clause on loop without a `break` statement; remove the `else` and dedent its contents
   --> useless_else_on_loop.py:37:1
@@ -74,6 +77,7 @@ help: Remove `else`
 37 + print("or else!")
 38 |
    |
+note: This is an unsafe fix and may change runtime behavior
 
 PLW0120 [*] `else` clause on loop without a `break` statement; remove the `else` and dedent its contents
   --> useless_else_on_loop.py:42:1
@@ -97,6 +101,7 @@ help: Remove `else`
 44 +     break
 45 |
    |
+note: This is an unsafe fix and may change runtime behavior
 
 PLW0120 [*] `else` clause on loop without a `break` statement; remove the `else` and dedent its contents
   --> useless_else_on_loop.py:88:5
@@ -116,6 +121,7 @@ help: Remove `else`
 88 +     return True
 89 |     return False
    |
+note: This is an unsafe fix and may change runtime behavior
 
 PLW0120 [*] `else` clause on loop without a `break` statement; remove the `else` and dedent its contents
    --> useless_else_on_loop.py:98:9
@@ -160,4 +166,41 @@ help: Remove `else`
 145 +     print("fat chance")
 146 +     for j in range(10):
 147 +         break
+148 |
+    |
+
+PLW0120 [*] `else` clause on loop without a `break` statement; remove the `else` and dedent its contents
+   --> useless_else_on_loop.py:155:5
+    |
+153 |     for j in range(10):
+154 |         pass
+155 |     else:  # comment on the `else` line
+    |     ^^^^
+156 |         # comment within the `else` block
+157 |         print("fat chance")
+    |
+help: Remove `else`
+    |
+154 |         pass
+    -     else:  # comment on the `else` line
+    -         # comment within the `else` block
+    -         print("fat chance")
+155 +     # comment within the `else` block
+156 +     print("fat chance")
+157 |
+    |
+note: This is an unsafe fix and may change runtime behavior
+
+PLW0120 [*] `else` clause on loop without a `break` statement; remove the `else` and dedent its contents
+   --> useless_else_on_loop.py:164:5
+    |
+162 |     for j in range(10):
+163 |         pass
+164 |     else: print("fat chance")  # comment on the statement
+    |     ^^^^
+help: Remove `else`
+    |
+163 |         pass
+    -     else: print("fat chance")  # comment on the statement
+164 +     print("fat chance")  # comment on the statement
     |