From 219f97094c6139831dfcd5aa657758dcb6521df4 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 11 Mar 2023 23:58:50 -0500 Subject: [PATCH] Expand the scope of useless-expression (B018) --- .../test/fixtures/flake8_bugbear/B018.py | 5 ++ crates/ruff/src/checkers/ast/mod.rs | 11 +-- .../rules/useless_comparison.rs | 1 + .../rules/useless_expression.rs | 82 ++++++++++++++----- ...__flake8_bugbear__tests__B018_B018.py.snap | 26 ++++++ ...ke8_simplify__tests__SIM401_SIM401.py.snap | 20 ----- ...ules__pyflakes__tests__F841_F841_0.py.snap | 6 +- crates/ruff_python_ast/src/helpers.rs | 17 ++++ 8 files changed, 115 insertions(+), 53 deletions(-) diff --git a/crates/ruff/resources/test/fixtures/flake8_bugbear/B018.py b/crates/ruff/resources/test/fixtures/flake8_bugbear/B018.py index 2d0b2dfc19cec..2bd24e993382a 100644 --- a/crates/ruff/resources/test/fixtures/flake8_bugbear/B018.py +++ b/crates/ruff/resources/test/fixtures/flake8_bugbear/B018.py @@ -57,3 +57,8 @@ def foo3(): def foo4(): ... + + +def foo5(): + 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 47b8068dda0e8..0a1152a610b18 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -420,10 +420,6 @@ where pyupgrade::rules::functools_cache(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 @@ -1886,6 +1878,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..59fe8938944a0 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,79 @@ -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; + } + + // 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; } + + // Ignore statements that have side effects. + if contains_effect(&checker.ctx, 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..564e3d89ed0a8 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,30 @@ expression: diagnostics column: 5 fix: ~ parent: ~ +- kind: + name: UselessExpression + body: Found useless attribute access. Either assign it to a variable or remove it. + suggestion: ~ + fixable: false + location: + row: 63 + column: 4 + end_location: + row: 63 + 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: 64 + column: 4 + end_location: + row: 64 + 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 7f34a32bab0bc..78e28c77cf77f 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: DictGetWithDefault - 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: DictGetWithDefault 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 047e03c59b030..ec16256bce4b2 100644 --- a/crates/ruff_python_ast/src/helpers.rs +++ b/crates/ruff_python_ast/src/helpers.rs @@ -130,6 +130,23 @@ 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 { .. } + ) { + return true; + } + if !matches!( + right.node, + ExprKind::Constant { .. } | ExprKind::JoinedStr { .. } + ) { + return true; + } + return false; + } + // Otherwise, avoid all complex expressions. matches!( expr.node,