From 5e45c3839c70806322341f05f84804bcc0bfc9a1 Mon Sep 17 00:00:00 2001 From: daniel sonbolian Date: Sat, 17 Aug 2024 01:07:17 +0300 Subject: [PATCH 1/3] b015 different message based on pointless comparison location --- .../test/fixtures/flake8_bugbear/B015.py | 5 ++ .../rules/useless_comparison.rs | 49 +++++++++++++++---- 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B015.py b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B015.py index 9453e3cddfb24..2794f1512b54c 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B015.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bugbear/B015.py @@ -17,6 +17,11 @@ def test(): 1 in (1, 2) +def test2(): + 1 in (1, 2) + return + + data = [x for x in [1, 2, 3] if x in (1, 2)] diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/useless_comparison.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/useless_comparison.rs index d9d217799f748..b8284a376b4c2 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/useless_comparison.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/useless_comparison.rs @@ -1,6 +1,7 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::Expr; +use ruff_python_semantic::ScopeKind; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -33,24 +34,34 @@ use super::super::helpers::at_last_top_level_expression_in_cell; /// ## References /// - [Python documentation: `assert` statement](https://docs.python.org/3/reference/simple_stmts.html#the-assert-statement) #[violation] -pub struct UselessComparison; +pub struct UselessComparison { + at: ComparisonLocationAt, +} impl Violation for UselessComparison { #[derive_message_formats] fn message(&self) -> String { - format!( - "Pointless comparison. Did you mean to assign a value? \ - Otherwise, prepend `assert` or remove it." - ) + match self.at { + ComparisonLocationAt::MiddleBody => format!( + "Pointless comparison. Did you mean to assign a value? \ + Otherwise, prepend `assert` or remove it." + ), + ComparisonLocationAt::EndOfFunction => format!( + "Pointless comparison at end of function scope. Did you mean \ + to return the expression result?" + ), + } } } /// B015 pub(crate) fn useless_comparison(checker: &mut Checker, expr: &Expr) { if expr.is_compare_expr() { + let semantic = checker.semantic(); + if checker.source_type.is_ipynb() && at_last_top_level_expression_in_cell( - checker.semantic(), + semantic, checker.locator(), checker.cell_offsets(), ) @@ -58,8 +69,28 @@ pub(crate) fn useless_comparison(checker: &mut Checker, expr: &Expr) { return; } - checker - .diagnostics - .push(Diagnostic::new(UselessComparison, expr.range())); + if let ScopeKind::Function(func_def) = semantic.current_scope().kind { + if func_def.range.end() == expr.range().end() { + return checker.diagnostics.push(Diagnostic::new( + UselessComparison { + at: ComparisonLocationAt::EndOfFunction, + }, + expr.range(), + )); + } + } + + checker.diagnostics.push(Diagnostic::new( + UselessComparison { + at: ComparisonLocationAt::MiddleBody, + }, + expr.range(), + )) } } + +#[derive(Debug, PartialEq, Eq, Copy, Clone)] +enum ComparisonLocationAt { + MiddleBody, + EndOfFunction, +} From 1493b381a83f6862b5454b7d4504d8df8bb029cd Mon Sep 17 00:00:00 2001 From: daniel sonbolian Date: Sat, 17 Aug 2024 01:37:03 +0300 Subject: [PATCH 2/3] b015 fix test snapshot and inconsistent formatting --- .../rules/useless_comparison.rs | 5 +++-- ...__flake8_bugbear__tests__B015_B015.py.snap | 19 +++++++++++++------ 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/useless_comparison.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/useless_comparison.rs index b8284a376b4c2..a1d93d342d53d 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/useless_comparison.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/useless_comparison.rs @@ -71,12 +71,13 @@ pub(crate) fn useless_comparison(checker: &mut Checker, expr: &Expr) { if let ScopeKind::Function(func_def) = semantic.current_scope().kind { if func_def.range.end() == expr.range().end() { - return checker.diagnostics.push(Diagnostic::new( + checker.diagnostics.push(Diagnostic::new( UselessComparison { at: ComparisonLocationAt::EndOfFunction, }, expr.range(), )); + return; } } @@ -85,7 +86,7 @@ pub(crate) fn useless_comparison(checker: &mut Checker, expr: &Expr) { at: ComparisonLocationAt::MiddleBody, }, expr.range(), - )) + )); } } diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B015_B015.py.snap b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B015_B015.py.snap index a59d23cb43b92..73529071e6797 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B015_B015.py.snap +++ b/crates/ruff_linter/src/rules/flake8_bugbear/snapshots/ruff_linter__rules__flake8_bugbear__tests__B015_B015.py.snap @@ -1,5 +1,6 @@ --- source: crates/ruff_linter/src/rules/flake8_bugbear/mod.rs +assertion_line: 74 --- B015.py:3:1: B015 Pointless comparison. Did you mean to assign a value? Otherwise, prepend `assert` or remove it. | @@ -19,7 +20,7 @@ B015.py:7:1: B015 Pointless comparison. Did you mean to assign a value? Otherwis | ^^^^^^^^^^^ B015 | -B015.py:17:5: B015 Pointless comparison. Did you mean to assign a value? Otherwise, prepend `assert` or remove it. +B015.py:17:5: B015 Pointless comparison at end of function scope. Did you mean to return the expression result? | 15 | assert 1 in (1, 2) 16 | @@ -27,11 +28,17 @@ B015.py:17:5: B015 Pointless comparison. Did you mean to assign a value? Otherwi | ^^^^^^^^^^^ B015 | -B015.py:24:5: B015 Pointless comparison. Did you mean to assign a value? Otherwise, prepend `assert` or remove it. +B015.py:21:5: B015 Pointless comparison. Did you mean to assign a value? Otherwise, prepend `assert` or remove it. | -23 | class TestClass: -24 | 1 == 1 - | ^^^^^^ B015 +20 | def test2(): +21 | 1 in (1, 2) + | ^^^^^^^^^^^ B015 +22 | return | - +B015.py:29:5: B015 Pointless comparison. Did you mean to assign a value? Otherwise, prepend `assert` or remove it. + | +28 | class TestClass: +29 | 1 == 1 + | ^^^^^^ B015 + | From 3d52f2dfb6bb5bfc10191ae7a6504c527a377ec9 Mon Sep 17 00:00:00 2001 From: daniel sonbolian Date: Sat, 17 Aug 2024 14:31:11 +0300 Subject: [PATCH 3/3] b015 more resilient check if expression is at the end of function scope --- .../src/rules/flake8_bugbear/rules/useless_comparison.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/ruff_linter/src/rules/flake8_bugbear/rules/useless_comparison.rs b/crates/ruff_linter/src/rules/flake8_bugbear/rules/useless_comparison.rs index a1d93d342d53d..83e295a3381ae 100644 --- a/crates/ruff_linter/src/rules/flake8_bugbear/rules/useless_comparison.rs +++ b/crates/ruff_linter/src/rules/flake8_bugbear/rules/useless_comparison.rs @@ -1,6 +1,6 @@ use ruff_diagnostics::{Diagnostic, Violation}; use ruff_macros::{derive_message_formats, violation}; -use ruff_python_ast::Expr; +use ruff_python_ast::{Expr, Stmt}; use ruff_python_semantic::ScopeKind; use ruff_text_size::Ranged; @@ -70,7 +70,12 @@ pub(crate) fn useless_comparison(checker: &mut Checker, expr: &Expr) { } if let ScopeKind::Function(func_def) = semantic.current_scope().kind { - if func_def.range.end() == expr.range().end() { + if func_def + .body + .last() + .and_then(Stmt::as_expr_stmt) + .is_some_and(|last_stmt| &*last_stmt.value == expr) + { checker.diagnostics.push(Diagnostic::new( UselessComparison { at: ComparisonLocationAt::EndOfFunction,