Expand the scope of useless-expression (B018) (#3455)

crates/ruff/resources/test/fixtures/flake8_bugbear/B018.py

@@ -57,3 +57,9 @@ def foo3():
 
 def foo4():
     ...
+
+
+def foo5():
+    foo.bar  # Attribute (raise)
+    object().__class__  # Attribute (raise)
+    "foo" + "bar"  # BinOp (raise)

crates/ruff/src/checkers/ast/mod.rs

@@ -420,10 +420,6 @@ where
                     pyupgrade::rules::lru_cache_with_maxsize_none(self, decorator_list);
                 }
 
-                if self.settings.rules.enabled(Rule::UselessExpression) {
-                    flake8_bugbear::rules::useless_expression(self, body);
-                }
-
                 if self.settings.rules.enabled(Rule::CachedInstanceMethod) {
                     flake8_bugbear::rules::cached_instance_method(self, decorator_list);
                 }
@@ -756,10 +752,6 @@ where
                     }
                 }
 
-                if self.settings.rules.enabled(Rule::UselessExpression) {
-                    flake8_bugbear::rules::useless_expression(self, body);
-                }
-
                 if !self.is_stub {
                     if self.settings.rules.any_enabled(&[
                         Rule::AbstractBaseClassWithoutAbstractMethod,
@@ -1830,6 +1822,9 @@ where
                 if self.settings.rules.enabled(Rule::UselessComparison) {
                     flake8_bugbear::rules::useless_comparison(self, value);
                 }
+                if self.settings.rules.enabled(Rule::UselessExpression) {
+                    flake8_bugbear::rules::useless_expression(self, value);
+                }
                 if self
                     .settings
                     .rules

crates/ruff/src/rules/flake8_bugbear/rules/useless_comparison.rs

@@ -19,6 +19,7 @@ impl Violation for UselessComparison {
     }
 }
 
+/// B015
 pub fn useless_comparison(checker: &mut Checker, expr: &Expr) {
     if matches!(expr.node, ExprKind::Compare { .. }) {
         checker

crates/ruff/src/rules/flake8_bugbear/rules/useless_expression.rs

@@ -1,41 +1,77 @@
-use rustpython_parser::ast::{Constant, ExprKind, Stmt, StmtKind};
+use rustpython_parser::ast::{Constant, Expr, ExprKind};
 
 use ruff_diagnostics::{Diagnostic, Violation};
 use ruff_macros::{derive_message_formats, violation};
+use ruff_python_ast::helpers::contains_effect;
 use ruff_python_ast::types::Range;
 
 use crate::checkers::ast::Checker;
 
+#[derive(Debug, PartialEq, Eq)]
+pub enum Kind {
+    Expression,
+    Attribute,
+}
+
 #[violation]
-pub struct UselessExpression;
+pub struct UselessExpression {
+    kind: Kind,
+}
 
 impl Violation for UselessExpression {
     #[derive_message_formats]
     fn message(&self) -> String {
+        match self.kind {
+            Kind::Expression => {
                 format!("Found useless expression. Either assign it to a variable or remove it.")
             }
+            Kind::Attribute => {
+                format!(
+                    "Found useless attribute access. Either assign it to a variable or remove it."
+                )
+            }
+        }
+    }
 }
 
 /// B018
-pub fn useless_expression(checker: &mut Checker, body: &[Stmt]) {
-    for stmt in body {
-        if let StmtKind::Expr { value } = &stmt.node {
-            match &value.node {
-                ExprKind::List { .. } | ExprKind::Dict { .. } | ExprKind::Set { .. } => {
-                    checker
-                        .diagnostics
-                        .push(Diagnostic::new(UselessExpression, Range::from(value)));
+pub fn useless_expression(checker: &mut Checker, value: &Expr) {
+    // Ignore comparisons, as they're handled by `useless_comparison`.
+    if matches!(value.node, ExprKind::Compare { .. }) {
+        return;
     }
-                ExprKind::Constant { value: val, .. } => match &val {
-                    Constant::Str { .. } | Constant::Ellipsis => {}
-                    _ => {
-                        checker
-                            .diagnostics
-                            .push(Diagnostic::new(UselessExpression, Range::from(value)));
+
+    // Ignore strings, to avoid false positives with docstrings.
+    if matches!(
+        value.node,
+        ExprKind::JoinedStr { .. }
+            | ExprKind::Constant {
+                value: Constant::Str(..) | Constant::Ellipsis,
+                ..
             }
+    ) {
+        return;
+    }
+
+    // Ignore statements that have side effects.
+    if contains_effect(&checker.ctx, value) {
+        // Flag attributes as useless expressions, even if they're attached to calls or other
+        // expressions.
+        if matches!(value.node, ExprKind::Attribute { .. }) {
+            checker.diagnostics.push(Diagnostic::new(
+                UselessExpression {
+                    kind: Kind::Attribute,
                 },
-                _ => {}
-            }
+                Range::from(value),
+            ));
         }
+        return;
     }
+
+    checker.diagnostics.push(Diagnostic::new(
+        UselessExpression {
+            kind: Kind::Expression,
+        },
+        Range::from(value),
+    ));
 }

crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B018_B018.py.snap

@@ -314,4 +314,43 @@ expression: diagnostics
     column: 5
   fix: ~
   parent: ~
+- kind:
+    name: UselessExpression
+    body: Found useless expression. Either assign it to a variable or remove it.
+    suggestion: ~
+    fixable: false
+  location:
+    row: 63
+    column: 4
+  end_location:
+    row: 63
+    column: 11
+  fix: ~
+  parent: ~
+- kind:
+    name: UselessExpression
+    body: Found useless attribute access. Either assign it to a variable or remove it.
+    suggestion: ~
+    fixable: false
+  location:
+    row: 64
+    column: 4
+  end_location:
+    row: 64
+    column: 22
+  fix: ~
+  parent: ~
+- kind:
+    name: UselessExpression
+    body: Found useless expression. Either assign it to a variable or remove it.
+    suggestion: ~
+    fixable: false
+  location:
+    row: 65
+    column: 4
+  end_location:
+    row: 65
+    column: 17
+  fix: ~
+  parent: ~
 

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

@@ -42,26 +42,6 @@ expression: diagnostics
       row: 15
       column: 21
   parent: ~
-- kind:
-    name: IfElseBlockInsteadOfDictGet
-    body: "Use `var = a_dict.get(key, val1 + val2)` instead of an `if` block"
-    suggestion: "Replace with `var = a_dict.get(key, val1 + val2)`"
-    fixable: true
-  location:
-    row: 18
-    column: 0
-  end_location:
-    row: 21
-    column: 21
-  fix:
-    content: "var = a_dict.get(key, val1 + val2)"
-    location:
-      row: 18
-      column: 0
-    end_location:
-      row: 21
-      column: 21
-  parent: ~
 - kind:
     name: IfElseBlockInsteadOfDictGet
     body: "Use `var = a_dict.get(keys[idx], \"default\")` instead of an `if` block"

crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F841_F841_0.py.snap

@@ -37,10 +37,10 @@ expression: diagnostics
     content: ""
     location:
       row: 16
-      column: 0
+      column: 4
     end_location:
-      row: 17
-      column: 0
+      row: 16
+      column: 8
   parent: ~
 - kind:
     name: UnusedVariable

crates/ruff_python_ast/src/helpers.rs

@@ -131,6 +131,39 @@ pub fn contains_effect(ctx: &Context, expr: &Expr) -> bool {
             }
         }
 
+        // Avoid false positive for overloaded operators.
+        if let ExprKind::BinOp { left, right, .. } = &expr.node {
+            if !matches!(
+                left.node,
+                ExprKind::Constant { .. }
+                    | ExprKind::JoinedStr { .. }
+                    | ExprKind::List { .. }
+                    | ExprKind::Tuple { .. }
+                    | ExprKind::Set { .. }
+                    | ExprKind::Dict { .. }
+                    | ExprKind::ListComp { .. }
+                    | ExprKind::SetComp { .. }
+                    | ExprKind::DictComp { .. }
+            ) {
+                return true;
+            }
+            if !matches!(
+                right.node,
+                ExprKind::Constant { .. }
+                    | ExprKind::JoinedStr { .. }
+                    | ExprKind::List { .. }
+                    | ExprKind::Tuple { .. }
+                    | ExprKind::Set { .. }
+                    | ExprKind::Dict { .. }
+                    | ExprKind::ListComp { .. }
+                    | ExprKind::SetComp { .. }
+                    | ExprKind::DictComp { .. }
+            ) {
+                return true;
+            }
+            return false;
+        }
+
         // Otherwise, avoid all complex expressions.
         matches!(
             expr.node,