Show negated condition in needless-bool diagnostics

astral-sh · Apr 10, 2024 · e984752 · e984752
1 parent 42d52eb
commit e984752
Show file tree

Hide file tree

Showing 4 changed files with 58 additions and 51 deletions.
diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM103.py b/crates/ruff_linter/resources/test/fixtures/flake8_simplify/SIM103.py
@@ -53,7 +53,7 @@ def f():
 
 
 def f():
-    # SIM103 (but not fixable)
+    # SIM103
     if a:
         return False
     else:
@@ -77,7 +77,7 @@ def f():
 
 
 def f():
-    # OK
+    # SIM103 (but not fixable)
     def bool():
         return False
     if a:

diff --git a/crates/ruff_linter/src/rules/flake8_simplify/rules/needless_bool.rs b/crates/ruff_linter/src/rules/flake8_simplify/rules/needless_bool.rs
@@ -41,30 +41,33 @@ use crate::fix::snippet::SourceCodeSnippet;
 /// [preview]: https://docs.astral.sh/ruff/preview/
 #[violation]
 pub struct NeedlessBool {
-    condition: SourceCodeSnippet,
-    replacement: Option<SourceCodeSnippet>,
+    condition: Option<SourceCodeSnippet>,
+    negate: bool,
 }
 
 impl Violation for NeedlessBool {
     const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;
 
     #[derive_message_formats]
     fn message(&self) -> String {
-        let NeedlessBool { condition, .. } = self;
-        if let Some(condition) = condition.full_display() {
+        let NeedlessBool {
+            condition, negate, ..
+        } = self;
+
+        if let Some(condition) = condition.as_ref().and_then(SourceCodeSnippet::full_display) {
             format!("Return the condition `{condition}` directly")
+        } else if *negate {
+            format!("Return the negated condition directly")
         } else {
             format!("Return the condition directly")
         }
     }
 
     fn fix_title(&self) -> Option<String> {
-        let NeedlessBool { replacement, .. } = self;
-        if let Some(replacement) = replacement
-            .as_ref()
-            .and_then(SourceCodeSnippet::full_display)
-        {
-            Some(format!("Replace with `{replacement}`"))
+        let NeedlessBool { condition, .. } = self;
+
+        if let Some(condition) = condition.as_ref().and_then(SourceCodeSnippet::full_display) {
+            Some(format!("Replace with `return {condition}`"))
         } else {
             Some(format!("Inline condition"))
         }
@@ -191,37 +194,29 @@ pub(crate) fn needless_bool(checker: &mut Checker, stmt: &Stmt) {
         return;
     }
 
-    let condition = checker.locator().slice(if_test);
-    let replacement = if checker.indexer().has_comments(&range, checker.locator()) {
+    // Generate the replacement condition.
+    let condition = if checker.indexer().has_comments(&range, checker.locator()) {
         None
     } else {
         // If the return values are inverted, wrap the condition in a `not`.
         if inverted {
-            let node = ast::StmtReturn {
-                value: Some(Box::new(Expr::UnaryOp(ast::ExprUnaryOp {
-                    op: ast::UnaryOp::Not,
-                    operand: Box::new(if_test.clone()),
-                    range: TextRange::default(),
-                }))),
+            Some(Expr::UnaryOp(ast::ExprUnaryOp {
+                op: ast::UnaryOp::Not,
+                operand: Box::new(if_test.clone()),
                 range: TextRange::default(),
-            };
-            Some(checker.generator().stmt(&node.into()))
+            }))
         } else if if_test.is_compare_expr() {
             // If the condition is a comparison, we can replace it with the condition, since we
             // know it's a boolean.
-            let node = ast::StmtReturn {
-                value: Some(Box::new(if_test.clone())),
-                range: TextRange::default(),
-            };
-            Some(checker.generator().stmt(&node.into()))
+            Some(if_test.clone())
         } else if checker.semantic().is_builtin("bool") {
             // Otherwise, we need to wrap the condition in a call to `bool`.
             let func_node = ast::ExprName {
                 id: "bool".into(),
                 ctx: ExprContext::Load,
                 range: TextRange::default(),
             };
-            let value_node = ast::ExprCall {
+            let call_node = ast::ExprCall {
                 func: Box::new(func_node.into()),
                 arguments: Arguments {
                     args: Box::from([if_test.clone()]),
@@ -230,20 +225,32 @@ pub(crate) fn needless_bool(checker: &mut Checker, stmt: &Stmt) {
                 },
                 range: TextRange::default(),
             };
-            let return_node = ast::StmtReturn {
-                value: Some(Box::new(value_node.into())),
-                range: TextRange::default(),
-            };
-            Some(checker.generator().stmt(&return_node.into()))
+            Some(Expr::Call(call_node))
         } else {
             None
         }
     };
 
+    // Generate the replacement `return` statement.
+    let replacement = condition.as_ref().map(|expr| {
+        Stmt::Return(ast::StmtReturn {
+            value: Some(Box::new(expr.clone())),
+            range: TextRange::default(),
+        })
+    });
+
+    // Generate source code.
+    let replacement = replacement
+        .as_ref()
+        .map(|stmt| checker.generator().stmt(stmt));
+    let condition = condition
+        .as_ref()
+        .map(|expr| checker.generator().expr(expr));
+
     let mut diagnostic = Diagnostic::new(
         NeedlessBool {
-            condition: SourceCodeSnippet::from_str(condition),
-            replacement: replacement.clone().map(SourceCodeSnippet::new),
+            condition: condition.map(SourceCodeSnippet::new),
+            negate: inverted,
         },
         range,
     );

diff --git a/...ake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM103_SIM103.py.snap b/...ake8_simplify/snapshots/ruff_linter__rules__flake8_simplify__tests__SIM103_SIM103.py.snap
@@ -1,7 +1,7 @@
 ---
 source: crates/ruff_linter/src/rules/flake8_simplify/mod.rs
 ---
-SIM103.py:3:5: SIM103 [*] Return the condition `a` directly
+SIM103.py:3:5: SIM103 [*] Return the condition `bool(a)` directly
   |
 1 |   def f():
 2 |       # SIM103
@@ -52,7 +52,7 @@ SIM103.py:11:5: SIM103 [*] Return the condition `a == b` directly
 16 13 | 
 17 14 | def f():
 
-SIM103.py:21:5: SIM103 [*] Return the condition `b` directly
+SIM103.py:21:5: SIM103 [*] Return the condition `bool(b)` directly
    |
 19 |       if a:
 20 |           return 1
@@ -78,7 +78,7 @@ SIM103.py:21:5: SIM103 [*] Return the condition `b` directly
 26 23 | 
 27 24 | def f():
 
-SIM103.py:32:9: SIM103 [*] Return the condition `b` directly
+SIM103.py:32:9: SIM103 [*] Return the condition `bool(b)` directly
    |
 30 |           return 1
 31 |       else:
@@ -104,10 +104,10 @@ SIM103.py:32:9: SIM103 [*] Return the condition `b` directly
 37 34 | 
 38 35 | def f():
 
-SIM103.py:57:5: SIM103 [*] Return the condition `a` directly
+SIM103.py:57:5: SIM103 [*] Return the condition `not a` directly
    |
 55 |   def f():
-56 |       # SIM103 (but not fixable)
+56 |       # SIM103
 57 |       if a:
    |  _____^
 58 | |         return False
@@ -120,7 +120,7 @@ SIM103.py:57:5: SIM103 [*] Return the condition `a` directly
 ℹ Unsafe fix
 54 54 | 
 55 55 | def f():
-56 56 |     # SIM103 (but not fixable)
+56 56 |     # SIM103
 57    |-    if a:
 58    |-        return False
 59    |-    else:
@@ -130,7 +130,7 @@ SIM103.py:57:5: SIM103 [*] Return the condition `a` directly
 62 59 | 
 63 60 | def f():
 
-SIM103.py:83:5: SIM103 Return the condition `a` directly
+SIM103.py:83:5: SIM103 Return the condition directly
    |
 81 |       def bool():
 82 |           return False

diff --git a/...lify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM103_SIM103.py.snap b/...lify/snapshots/ruff_linter__rules__flake8_simplify__tests__preview__SIM103_SIM103.py.snap
@@ -1,7 +1,7 @@
 ---
 source: crates/ruff_linter/src/rules/flake8_simplify/mod.rs
 ---
-SIM103.py:3:5: SIM103 [*] Return the condition `a` directly
+SIM103.py:3:5: SIM103 [*] Return the condition `bool(a)` directly
   |
 1 |   def f():
 2 |       # SIM103
@@ -52,7 +52,7 @@ SIM103.py:11:5: SIM103 [*] Return the condition `a == b` directly
 16 13 | 
 17 14 | def f():
 
-SIM103.py:21:5: SIM103 [*] Return the condition `b` directly
+SIM103.py:21:5: SIM103 [*] Return the condition `bool(b)` directly
    |
 19 |       if a:
 20 |           return 1
@@ -78,7 +78,7 @@ SIM103.py:21:5: SIM103 [*] Return the condition `b` directly
 26 23 | 
 27 24 | def f():
 
-SIM103.py:32:9: SIM103 [*] Return the condition `b` directly
+SIM103.py:32:9: SIM103 [*] Return the condition `bool(b)` directly
    |
 30 |           return 1
 31 |       else:
@@ -104,10 +104,10 @@ SIM103.py:32:9: SIM103 [*] Return the condition `b` directly
 37 34 | 
 38 35 | def f():
 
-SIM103.py:57:5: SIM103 [*] Return the condition `a` directly
+SIM103.py:57:5: SIM103 [*] Return the condition `not a` directly
    |
 55 |   def f():
-56 |       # SIM103 (but not fixable)
+56 |       # SIM103
 57 |       if a:
    |  _____^
 58 | |         return False
@@ -120,7 +120,7 @@ SIM103.py:57:5: SIM103 [*] Return the condition `a` directly
 ℹ Unsafe fix
 54 54 | 
 55 55 | def f():
-56 56 |     # SIM103 (but not fixable)
+56 56 |     # SIM103
 57    |-    if a:
 58    |-        return False
 59    |-    else:
@@ -130,7 +130,7 @@ SIM103.py:57:5: SIM103 [*] Return the condition `a` directly
 62 59 | 
 63 60 | def f():
 
-SIM103.py:83:5: SIM103 Return the condition `a` directly
+SIM103.py:83:5: SIM103 Return the condition directly
    |
 81 |       def bool():
 82 |           return False
@@ -143,7 +143,7 @@ SIM103.py:83:5: SIM103 Return the condition `a` directly
    |
    = help: Inline condition
 
-SIM103.py:96:5: SIM103 [*] Return the condition `a` directly
+SIM103.py:96:5: SIM103 [*] Return the condition `bool(a)` directly
    |
 94 |   def f():
 95 |       # SIM103
@@ -167,7 +167,7 @@ SIM103.py:96:5: SIM103 [*] Return the condition `a` directly
 100 98 | 
 101 99 | def f():
 
-SIM103.py:103:5: SIM103 [*] Return the condition `a` directly
+SIM103.py:103:5: SIM103 [*] Return the condition `not a` directly
     |
 101 |   def f():
 102 |       # SIM103