From e8d17d23cb2b1d9dc4c657429124968c009f028a Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Thu, 23 Mar 2023 18:33:58 -0400 Subject: [PATCH] Expand the scope of useless-expression (B018) (#3455) --- .../test/fixtures/flake8_bugbear/B018.py | 6 ++ crates/ruff/src/checkers/ast/mod.rs | 11 +-- .../rules/useless_comparison.rs | 1 + .../rules/useless_expression.rs | 86 +++++++++++++------ ...__flake8_bugbear__tests__B018_B018.py.snap | 39 +++++++++ ...ke8_simplify__tests__SIM401_SIM401.py.snap | 20 ----- ...ules__pyflakes__tests__F841_F841_0.py.snap | 6 +- crates/ruff_python_ast/src/helpers.rs | 33 +++++++ 8 files changed, 146 insertions(+), 56 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_bugbear/B018.py b/crates/ruff/resources/test/fixtures/flake8_bugbear/B018.py index 2d0b2dfc19..37d7145f8d 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 32b33d6064..9b3ca623df 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/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 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 19495c88b0..18ca6cbd4c 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 b36f7d35e0..8d19837ec1 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.") - } -} - -/// 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))); - } - }, - _ => {} + 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, 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 76681653cc..540551f013 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 ed3f19c8bf..a45d242625 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 a25da2b540..8d4deac77e 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 f626d81108..f14a1412b2 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/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,