diff --git a/crates/ruff/resources/test/fixtures/flake8_bugbear/B018.py b/crates/ruff/resources/test/fixtures/flake8_bugbear/B018.py index 2d0b2dfc19cec..37d7145f8d260 100644 --- a/crates/ruff/resources/test/fixtures/flake8_bugbear/B018.py +++ b/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) diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index b8b3317baa818..7bd40519533ca 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -416,10 +416,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); } @@ -783,10 +779,6 @@ where } } - if self.settings.rules.enabled(Rule::UselessExpression) { - flake8_bugbear::rules::useless_expression(self, body); - } - if !self.is_stub { if self .settings @@ -1872,6 +1864,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 diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/useless_comparison.rs b/crates/ruff/src/rules/flake8_bugbear/rules/useless_comparison.rs index 19495c88b0542..18ca6cbd4c186 100644 --- a/crates/ruff/src/rules/flake8_bugbear/rules/useless_comparison.rs +++ b/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 diff --git a/crates/ruff/src/rules/flake8_bugbear/rules/useless_expression.rs b/crates/ruff/src/rules/flake8_bugbear/rules/useless_expression.rs index b36f7d35e025a..8d19837ec1fd7 100644 --- a/crates/ruff/src/rules/flake8_bugbear/rules/useless_expression.rs +++ b/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 { - format!("Found useless expression. Either assign it to a variable or remove it.") + 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))); - } - ExprKind::Constant { value: val, .. } => match &val { - Constant::Str { .. } | Constant::Ellipsis => {} - _ => { - 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; + } + + // 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), + )); } diff --git a/crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B018_B018.py.snap b/crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B018_B018.py.snap index 76681653cc43c..540551f01328c 100644 --- a/crates/ruff/src/rules/flake8_bugbear/snapshots/ruff__rules__flake8_bugbear__tests__B018_B018.py.snap +++ b/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: ~ diff --git a/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM401_SIM401.py.snap b/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM401_SIM401.py.snap index ed3f19c8bfa46..a45d24262534e 100644 --- a/crates/ruff/src/rules/flake8_simplify/snapshots/ruff__rules__flake8_simplify__tests__SIM401_SIM401.py.snap +++ b/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" diff --git a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F841_F841_0.py.snap b/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F841_F841_0.py.snap index a25da2b54009c..8d4deac77ee9b 100644 --- a/crates/ruff/src/rules/pyflakes/snapshots/ruff__rules__pyflakes__tests__F841_F841_0.py.snap +++ b/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 diff --git a/crates/ruff_python_ast/src/helpers.rs b/crates/ruff_python_ast/src/helpers.rs index 0c399fe6a72f0..737abef7b5146 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -132,6 +132,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,