Mark flake8-simplify rules as unfixable in non-fixable cases (#2676)

README.md

@@ -1211,7 +1211,7 @@ For more, see [flake8-simplify](https://pypi.org/project/flake8-simplify/) on Py
 | ---- | ---- | ------- | --- |
 | SIM101 | duplicate-isinstance-call | Multiple `isinstance` calls for `{name}`, merge into a single call | 🛠 |
 | SIM102 | nested-if-statements | Use a single `if` statement instead of nested `if` statements | 🛠 |
-| SIM103 | return-bool-condition-directly | Return the condition `{cond}` directly | 🛠 |
+| SIM103 | return-bool-condition-directly | Return the condition `{condition}` directly | 🛠 |
 | SIM105 | use-contextlib-suppress | Use `contextlib.suppress({exception})` instead of try-except-pass |  |
 | SIM107 | return-in-try-except-finally | Don't use `return` in `try`/`except` and `finally` |  |
 | SIM108 | use-ternary-operator | Use ternary operator `{contents}` instead of if-else-block | 🛠 |

crates/ruff/src/rules/flake8_simplify/rules/ast_if.rs

@@ -1,7 +1,8 @@
 use log::error;
-use ruff_macros::{define_violation, derive_message_formats};
 use rustpython_parser::ast::{Cmpop, Constant, Expr, ExprContext, ExprKind, Stmt, StmtKind};
 
+use ruff_macros::{define_violation, derive_message_formats};
+
 use crate::ast::comparable::ComparableExpr;
 use crate::ast::helpers::{
     contains_call_path, contains_effect, create_expr, create_stmt, first_colon_range, has_comments,
@@ -12,44 +13,63 @@ use crate::checkers::ast::Checker;
 use crate::fix::Fix;
 use crate::registry::Diagnostic;
 use crate::rules::flake8_simplify::rules::fix_if;
-use crate::violation::{AlwaysAutofixableViolation, Availability, Violation};
+use crate::violation::{Availability, Violation};
 use crate::AutofixKind;
 
 define_violation!(
-    pub struct NestedIfStatements;
+    pub struct NestedIfStatements {
+        pub fixable: bool,
+    }
 );
-impl AlwaysAutofixableViolation for NestedIfStatements {
+impl Violation for NestedIfStatements {
+    const AUTOFIX: Option<AutofixKind> = Some(AutofixKind::new(Availability::Sometimes));
+
     #[derive_message_formats]
     fn message(&self) -> String {
         format!("Use a single `if` statement instead of nested `if` statements")
     }
 
-    fn autofix_title(&self) -> String {
-        "Combine `if` statements using `and`".to_string()
+    fn autofix_title_formatter(&self) -> Option<fn(&Self) -> String> {
+        let NestedIfStatements { fixable, .. } = self;
+        if *fixable {
+            Some(|_| format!("Combine `if` statements using `and`"))
+        } else {
+            None
+        }
     }
 }
 
 define_violation!(
     pub struct ReturnBoolConditionDirectly {
-        pub cond: String,
+        pub condition: String,
+        pub fixable: bool,
     }
 );
-impl AlwaysAutofixableViolation for ReturnBoolConditionDirectly {
+impl Violation for ReturnBoolConditionDirectly {
+    const AUTOFIX: Option<AutofixKind> = Some(AutofixKind::new(Availability::Sometimes));
+
     #[derive_message_formats]
     fn message(&self) -> String {
-        let ReturnBoolConditionDirectly { cond } = self;
-        format!("Return the condition `{cond}` directly")
+        let ReturnBoolConditionDirectly { condition, .. } = self;
+        format!("Return the condition `{condition}` directly")
     }
 
-    fn autofix_title(&self) -> String {
-        let ReturnBoolConditionDirectly { cond } = self;
-        format!("Replace with `return {cond}`")
+    fn autofix_title_formatter(&self) -> Option<fn(&Self) -> String> {
+        let ReturnBoolConditionDirectly { fixable, .. } = self;
+        if *fixable {
+            Some(|ReturnBoolConditionDirectly { condition, .. }| {
+                format!("Replace with `return {condition}`")
+            })
+        } else {
+            None
+        }
     }
 }
 
 define_violation!(
     pub struct UseTernaryOperator {
         pub contents: String,
+        pub fixable: bool,
     }
 );
 impl Violation for UseTernaryOperator {
@@ -57,30 +77,44 @@ impl Violation for UseTernaryOperator {
 
     #[derive_message_formats]
     fn message(&self) -> String {
-        let UseTernaryOperator { contents } = self;
+        let UseTernaryOperator { contents, .. } = self;
         format!("Use ternary operator `{contents}` instead of if-else-block")
     }
 
     fn autofix_title_formatter(&self) -> Option<fn(&Self) -> String> {
-        Some(|UseTernaryOperator { contents }| format!("Replace if-else-block with `{contents}`"))
+        let UseTernaryOperator { fixable, .. } = self;
+        if *fixable {
+            Some(|UseTernaryOperator { contents, .. }| {
+                format!("Replace if-else-block with `{contents}`")
+            })
+        } else {
+            None
+        }
     }
 }
 
 define_violation!(
     pub struct DictGetWithDefault {
         pub contents: String,
+        pub fixable: bool,
     }
 );
-impl AlwaysAutofixableViolation for DictGetWithDefault {
+impl Violation for DictGetWithDefault {
+    const AUTOFIX: Option<AutofixKind> = Some(AutofixKind::new(Availability::Sometimes));
+
     #[derive_message_formats]
     fn message(&self) -> String {
-        let DictGetWithDefault { contents } = self;
+        let DictGetWithDefault { contents, .. } = self;
         format!("Use `{contents}` instead of an `if` block")
     }
 
-    fn autofix_title(&self) -> String {
-        let DictGetWithDefault { contents } = self;
-        format!("Replace with `{contents}`")
+    fn autofix_title_formatter(&self) -> Option<fn(&Self) -> String> {
+        let DictGetWithDefault { fixable, .. } = self;
+        if *fixable {
+            Some(|DictGetWithDefault { contents, .. }| format!("Replace with `{contents}`"))
+        } else {
+            None
+        }
     }
 }
 
@@ -163,37 +197,39 @@ pub fn nested_if_statements(
     let Some((test, first_stmt)) = find_last_nested_if(body) else {
         return;
     };
+
     let colon = first_colon_range(
         Range::new(test.end_location.unwrap(), first_stmt.location),
         checker.locator,
     );
+
+    // The fixer preserves comments in the nested body, but removes comments between
+    // the outer and inner if statements.
+    let nested_if = &body[0];
+    let fixable = !has_comments_in(
+        Range::new(stmt.location, nested_if.location),
+        checker.locator,
+    );
+
     let mut diagnostic = Diagnostic::new(
-        NestedIfStatements,
+        NestedIfStatements { fixable },
         colon.map_or_else(
             || Range::from_located(stmt),
             |colon| Range::new(stmt.location, colon.end_location),
         ),
     );
-    if checker.patch(diagnostic.kind.rule()) {
-        // The fixer preserves comments in the nested body, but removes comments between
-        // the outer and inner if statements.
-        let nested_if = &body[0];
-        if !has_comments_in(
-            Range::new(stmt.location, nested_if.location),
-            checker.locator,
-        ) {
-            match fix_if::fix_nested_if_statements(checker.locator, checker.stylist, stmt) {
-                Ok(fix) => {
-                    if fix
-                        .content
-                        .lines()
-                        .all(|line| line.len() <= checker.settings.line_length)
-                    {
-                        diagnostic.amend(fix);
-                    }
+    if fixable && checker.patch(diagnostic.kind.rule()) {
+        match fix_if::fix_nested_if_statements(checker.locator, checker.stylist, stmt) {
+            Ok(fix) => {
+                if fix
+                    .content
+                    .lines()
+                    .all(|line| line.len() <= checker.settings.line_length)
+                {
+                    diagnostic.amend(fix);
                 }
-                Err(err) => error!("Failed to fix nested if: {err}"),
             }
+            Err(err) => error!("Failed to fix nested if: {err}"),
         }
     }
     checker.diagnostics.push(diagnostic);
@@ -247,16 +283,18 @@ pub fn return_bool_condition_directly(checker: &mut Checker, stmt: &Stmt) {
     }
 
     let condition = unparse_expr(test, checker.stylist);
-    let mut diagnostic = Diagnostic::new(
-        ReturnBoolConditionDirectly { cond: condition },
-        Range::from_located(stmt),
-    );
-    if checker.patch(diagnostic.kind.rule())
-        && matches!(if_return, Bool::True)
+    let fixable = matches!(if_return, Bool::True)
         && matches!(else_return, Bool::False)
         && !has_comments(stmt, checker.locator)
-    {
+        && (matches!(test.node, ExprKind::Compare { .. }) || checker.is_builtin("bool"));
+
+    let mut diagnostic = Diagnostic::new(
+        ReturnBoolConditionDirectly { condition, fixable },
+        Range::from_located(stmt),
+    );
+    if fixable && checker.patch(diagnostic.kind.rule()) {
         if matches!(test.node, ExprKind::Compare { .. }) {
+            // If the condition is a comparison, we can replace it with the condition.
             diagnostic.amend(Fix::replacement(
                 unparse_stmt(
                     &create_stmt(StmtKind::Return {
@@ -267,7 +305,9 @@ pub fn return_bool_condition_directly(checker: &mut Checker, stmt: &Stmt) {
                 stmt.location,
                 stmt.end_location.unwrap(),
             ));
-        } else if checker.is_builtin("bool") {
+        } else {
+            // Otherwise, we need to wrap the condition in a call to `bool`. (We've already
+            // verified, above, that `bool` is a builtin.)
             diagnostic.amend(Fix::replacement(
                 unparse_stmt(
                     &create_stmt(StmtKind::Return {
@@ -394,13 +434,15 @@ pub fn use_ternary_operator(checker: &mut Checker, stmt: &Stmt, parent: Option<&
         return;
     }
 
+    let fixable = !has_comments(stmt, checker.locator);
     let mut diagnostic = Diagnostic::new(
         UseTernaryOperator {
             contents: contents.clone(),
+            fixable,
         },
         Range::from_located(stmt),
     );
-    if checker.patch(diagnostic.kind.rule()) && !has_comments(stmt, checker.locator) {
+    if fixable && checker.patch(diagnostic.kind.rule()) {
         diagnostic.amend(Fix::replacement(
             contents,
             stmt.location,
@@ -525,13 +567,15 @@ pub fn use_dict_get_with_default(
         return;
     }
 
+    let fixable = !has_comments(stmt, checker.locator);
     let mut diagnostic = Diagnostic::new(
         DictGetWithDefault {
             contents: contents.clone(),
+            fixable,
         },
         Range::from_located(stmt),
     );
-    if checker.patch(diagnostic.kind.rule()) && !has_comments(stmt, checker.locator) {
+    if fixable && checker.patch(diagnostic.kind.rule()) {
         diagnostic.amend(Fix::replacement(
             contents,
             stmt.location,

crates/ruff/src/rules/flake8_simplify/rules/ast_with.rs

@@ -1,18 +1,25 @@
 use log::error;
-use ruff_macros::{define_violation, derive_message_formats};
 use rustpython_parser::ast::{Located, Stmt, StmtKind, Withitem};
 
-use super::fix_with;
+use ruff_macros::{define_violation, derive_message_formats};
+
 use crate::ast::helpers::{first_colon_range, has_comments_in};
 use crate::ast::types::Range;
 use crate::checkers::ast::Checker;
 use crate::registry::Diagnostic;
-use crate::violation::AlwaysAutofixableViolation;
+use crate::violation::{Availability, Violation};
+use crate::AutofixKind;
+
+use super::fix_with;
 
 define_violation!(
-    pub struct MultipleWithStatements;
+    pub struct MultipleWithStatements {
+        pub fixable: bool,
+    }
 );
-impl AlwaysAutofixableViolation for MultipleWithStatements {
+impl Violation for MultipleWithStatements {
+    const AUTOFIX: Option<AutofixKind> = Some(AutofixKind::new(Availability::Sometimes));
+
     #[derive_message_formats]
     fn message(&self) -> String {
         format!(
@@ -21,8 +28,13 @@ impl AlwaysAutofixableViolation for MultipleWithStatements {
         )
     }
 
-    fn autofix_title(&self) -> String {
-        "Combine `with` statements".to_string()
+    fn autofix_title_formatter(&self) -> Option<fn(&Self) -> String> {
+        let MultipleWithStatements { fixable, .. } = self;
+        if *fixable {
+            Some(|_| format!("Combine `with` statements"))
+        } else {
+            None
+        }
     }
 }
 
@@ -60,35 +72,33 @@ pub fn multiple_with_statements(
             ),
             checker.locator,
         );
+        let fixable = !has_comments_in(
+            Range::new(with_stmt.location, with_body[0].location),
+            checker.locator,
+        );
         let mut diagnostic = Diagnostic::new(
-            MultipleWithStatements,
+            MultipleWithStatements { fixable },
             colon.map_or_else(
                 || Range::from_located(with_stmt),
                 |colon| Range::new(with_stmt.location, colon.end_location),
             ),
         );
-        if checker.patch(diagnostic.kind.rule()) {
-            let nested_with = &with_body[0];
-            if !has_comments_in(
-                Range::new(with_stmt.location, nested_with.location),
+        if fixable && checker.patch(diagnostic.kind.rule()) {
+            match fix_with::fix_multiple_with_statements(
                 checker.locator,
+                checker.stylist,
+                with_stmt,
             ) {
-                match fix_with::fix_multiple_with_statements(
-                    checker.locator,
-                    checker.stylist,
-                    with_stmt,
-                ) {
-                    Ok(fix) => {
-                        if fix
-                            .content
-                            .lines()
-                            .all(|line| line.len() <= checker.settings.line_length)
-                        {
-                            diagnostic.amend(fix);
-                        }
+                Ok(fix) => {
+                    if fix
+                        .content
+                        .lines()
+                        .all(|line| line.len() <= checker.settings.line_length)
+                    {
+                        diagnostic.amend(fix);
                     }
-                    Err(err) => error!("Failed to fix nested with: {err}"),
                 }
+                Err(err) => error!("Failed to fix nested with: {err}"),
             }
         }
         checker.diagnostics.push(diagnostic);

crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM102_SIM102.py.snap

@@ -1,9 +1,10 @@
 ---
-source: src/rules/flake8_simplify/mod.rs
+source: crates/ruff/src/rules/flake8_simplify/mod.rs
 expression: diagnostics
 ---
 - kind:
-    NestedIfStatements: ~
+    NestedIfStatements:
+      fixable: true
   location:
     row: 2
     column: 0
@@ -23,7 +24,8 @@ expression: diagnostics
       column: 0
   parent: ~
 - kind:
-    NestedIfStatements: ~
+    NestedIfStatements:
+      fixable: true
   location:
     row: 7
     column: 0
@@ -44,7 +46,8 @@ expression: diagnostics
       column: 0
   parent: ~
 - kind:
-    NestedIfStatements: ~
+    NestedIfStatements:
+      fixable: true
   location:
     row: 15
     column: 0
@@ -64,7 +67,8 @@ expression: diagnostics
       column: 0
   parent: ~
 - kind:
-    NestedIfStatements: ~
+    NestedIfStatements:
+      fixable: false
   location:
     row: 20
     column: 0
@@ -74,7 +78,8 @@ expression: diagnostics
   fix: ~
   parent: ~
 - kind:
-    NestedIfStatements: ~
+    NestedIfStatements:
+      fixable: true
   location:
     row: 26
     column: 0
@@ -95,7 +100,8 @@ expression: diagnostics
       column: 0
   parent: ~
 - kind:
-    NestedIfStatements: ~
+    NestedIfStatements:
+      fixable: true
   location:
     row: 51
     column: 4
@@ -125,7 +131,8 @@ expression: diagnostics
       column: 0
   parent: ~
 - kind:
-    NestedIfStatements: ~
+    NestedIfStatements:
+      fixable: true
   location:
     row: 67
     column: 0
@@ -155,7 +162,8 @@ expression: diagnostics
       column: 0
   parent: ~
 - kind:
-    NestedIfStatements: ~
+    NestedIfStatements:
+      fixable: true
   location:
     row: 83
     column: 4
@@ -177,7 +185,8 @@ expression: diagnostics
       column: 0
   parent: ~
 - kind:
-    NestedIfStatements: ~
+    NestedIfStatements:
+      fixable: true
   location:
     row: 90
     column: 0
@@ -199,7 +208,8 @@ expression: diagnostics
       column: 0
   parent: ~
 - kind:
-    NestedIfStatements: ~
+    NestedIfStatements:
+      fixable: true
   location:
     row: 117
     column: 4

crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM103_SIM103.py.snap

@@ -1,10 +1,11 @@
 ---
-source: src/rules/flake8_simplify/mod.rs
+source: crates/ruff/src/rules/flake8_simplify/mod.rs
 expression: diagnostics
 ---
 - kind:
     ReturnBoolConditionDirectly:
-      cond: a
+      condition: a
+      fixable: true
   location:
     row: 3
     column: 4
@@ -23,7 +24,8 @@ expression: diagnostics
   parent: ~
 - kind:
     ReturnBoolConditionDirectly:
-      cond: a == b
+      condition: a == b
+      fixable: true
   location:
     row: 11
     column: 4
@@ -42,7 +44,8 @@ expression: diagnostics
   parent: ~
 - kind:
     ReturnBoolConditionDirectly:
-      cond: b
+      condition: b
+      fixable: true
   location:
     row: 21
     column: 4
@@ -61,7 +64,8 @@ expression: diagnostics
   parent: ~
 - kind:
     ReturnBoolConditionDirectly:
-      cond: b
+      condition: b
+      fixable: true
   location:
     row: 32
     column: 8
@@ -80,7 +84,8 @@ expression: diagnostics
   parent: ~
 - kind:
     ReturnBoolConditionDirectly:
-      cond: a
+      condition: a
+      fixable: false
   location:
     row: 57
     column: 4
@@ -91,7 +96,8 @@ expression: diagnostics
   parent: ~
 - kind:
     ReturnBoolConditionDirectly:
-      cond: a
+      condition: a
+      fixable: false
   location:
     row: 83
     column: 4

crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM108_SIM108.py.snap

@@ -1,10 +1,11 @@
 ---
-source: src/rules/flake8_simplify/mod.rs
+source: crates/ruff/src/rules/flake8_simplify/mod.rs
 expression: diagnostics
 ---
 - kind:
     UseTernaryOperator:
       contents: b = c if a else d
+      fixable: true
   location:
     row: 2
     column: 0
@@ -24,6 +25,7 @@ expression: diagnostics
 - kind:
     UseTernaryOperator:
       contents: abc = x if x > 0 else -x
+      fixable: false
   location:
     row: 58
     column: 0
@@ -35,6 +37,7 @@ expression: diagnostics
 - kind:
     UseTernaryOperator:
       contents: b = cccccccccccccccccccccccccccccccccccc if a else ddddddddddddddddddddddddddddddddddddd
+      fixable: true
   location:
     row: 82
     column: 0
@@ -54,6 +57,7 @@ expression: diagnostics
 - kind:
     UseTernaryOperator:
       contents: exitcode = 0 if True else 1
+      fixable: false
   location:
     row: 97
     column: 0
@@ -65,6 +69,7 @@ expression: diagnostics
 - kind:
     UseTernaryOperator:
       contents: x = 3 if True else 5
+      fixable: false
   location:
     row: 104
     column: 0
@@ -76,6 +81,7 @@ expression: diagnostics
 - kind:
     UseTernaryOperator:
       contents: x = 3 if True else 5
+      fixable: false
   location:
     row: 109
     column: 0

crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM117_SIM117.py.snap

@@ -1,9 +1,10 @@
 ---
-source: src/rules/flake8_simplify/mod.rs
+source: crates/ruff/src/rules/flake8_simplify/mod.rs
 expression: diagnostics
 ---
 - kind:
-    MultipleWithStatements: ~
+    MultipleWithStatements:
+      fixable: true
   location:
     row: 2
     column: 0
@@ -23,7 +24,8 @@ expression: diagnostics
       column: 0
   parent: ~
 - kind:
-    MultipleWithStatements: ~
+    MultipleWithStatements:
+      fixable: true
   location:
     row: 7
     column: 0
@@ -44,7 +46,8 @@ expression: diagnostics
       column: 0
   parent: ~
 - kind:
-    MultipleWithStatements: ~
+    MultipleWithStatements:
+      fixable: false
   location:
     row: 13
     column: 0
@@ -54,7 +57,8 @@ expression: diagnostics
   fix: ~
   parent: ~
 - kind:
-    MultipleWithStatements: ~
+    MultipleWithStatements:
+      fixable: true
   location:
     row: 19
     column: 0
@@ -75,7 +79,8 @@ expression: diagnostics
       column: 0
   parent: ~
 - kind:
-    MultipleWithStatements: ~
+    MultipleWithStatements:
+      fixable: true
   location:
     row: 53
     column: 4
@@ -105,7 +110,8 @@ expression: diagnostics
       column: 0
   parent: ~
 - kind:
-    MultipleWithStatements: ~
+    MultipleWithStatements:
+      fixable: true
   location:
     row: 68
     column: 0
@@ -128,7 +134,8 @@ expression: diagnostics
       column: 0
   parent: ~
 - kind:
-    MultipleWithStatements: ~
+    MultipleWithStatements:
+      fixable: true
   location:
     row: 76
     column: 0
@@ -151,7 +158,8 @@ expression: diagnostics
       column: 0
   parent: ~
 - kind:
-    MultipleWithStatements: ~
+    MultipleWithStatements:
+      fixable: true
   location:
     row: 84
     column: 0

crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM401_SIM401.py.snap

@@ -1,10 +1,11 @@
 ---
-source: src/rules/flake8_simplify/mod.rs
+source: crates/ruff/src/rules/flake8_simplify/mod.rs
 expression: diagnostics
 ---
 - kind:
     DictGetWithDefault:
       contents: "var = a_dict.get(key, \"default1\")"
+      fixable: true
   location:
     row: 6
     column: 0
@@ -24,6 +25,7 @@ expression: diagnostics
 - kind:
     DictGetWithDefault:
       contents: "var = a_dict.get(key, \"default2\")"
+      fixable: true
   location:
     row: 12
     column: 0
@@ -43,6 +45,7 @@ expression: diagnostics
 - kind:
     DictGetWithDefault:
       contents: "var = a_dict.get(key, val1 + val2)"
+      fixable: true
   location:
     row: 18
     column: 0
@@ -62,6 +65,7 @@ expression: diagnostics
 - kind:
     DictGetWithDefault:
       contents: "var = a_dict.get(keys[idx], \"default\")"
+      fixable: true
   location:
     row: 24
     column: 0
@@ -81,6 +85,7 @@ expression: diagnostics
 - kind:
     DictGetWithDefault:
       contents: "var = dicts[idx].get(key, \"default\")"
+      fixable: true
   location:
     row: 30
     column: 0
@@ -100,6 +105,7 @@ expression: diagnostics
 - kind:
     DictGetWithDefault:
       contents: "vars[idx] = a_dict.get(key, \"default\")"
+      fixable: true
   location:
     row: 36
     column: 0