From 5aec7890a0414307ad241f67d674049de96efcc2 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sun, 7 Jan 2024 23:24:15 -0500 Subject: [PATCH] Allow Boolean positionals in setters --- .../test/fixtures/flake8_boolean_trap/FBT.py | 5 ++++ .../src/checkers/ast/analyze/expression.rs | 2 +- .../src/rules/flake8_boolean_trap/helpers.rs | 26 +++++++++++++++---- .../rules/boolean_positional_value_in_call.rs | 13 +++++++--- ...e8_boolean_trap__tests__FBT001_FBT.py.snap | 8 +++--- ...e8_boolean_trap__tests__FBT003_FBT.py.snap | 14 ++++------ ...n_trap__tests__preview__FBT001_FBT.py.snap | 20 +++++++------- 7 files changed, 55 insertions(+), 33 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_boolean_trap/FBT.py b/crates/ruff_linter/resources/test/fixtures/flake8_boolean_trap/FBT.py index 2c48a7d629fdaf..cdb8a97db94d71 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_boolean_trap/FBT.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_boolean_trap/FBT.py @@ -71,6 +71,8 @@ def used(do): bar.is_not(False) next(iter([]), False) sa.func.coalesce(tbl.c.valid, False) +setVisible(True) +set_visible(True) class Registry: @@ -114,3 +116,6 @@ def func(x: int | str): @override def func(x: bool): pass + + +settings(True) diff --git a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs index 3b4286fd8a2906..1a45bedc14a1e6 100644 --- a/crates/ruff_linter/src/checkers/ast/analyze/expression.rs +++ b/crates/ruff_linter/src/checkers/ast/analyze/expression.rs @@ -724,7 +724,7 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) { ); } if checker.enabled(Rule::BooleanPositionalValueInCall) { - flake8_boolean_trap::rules::boolean_positional_value_in_call(checker, args, func); + flake8_boolean_trap::rules::boolean_positional_value_in_call(checker, call); } if checker.enabled(Rule::Debugger) { flake8_debugger::rules::debugger_call(checker, expr, func); diff --git a/crates/ruff_linter/src/rules/flake8_boolean_trap/helpers.rs b/crates/ruff_linter/src/rules/flake8_boolean_trap/helpers.rs index f1782582611679..5f8f7a5dd9cc7f 100644 --- a/crates/ruff_linter/src/rules/flake8_boolean_trap/helpers.rs +++ b/crates/ruff_linter/src/rules/flake8_boolean_trap/helpers.rs @@ -51,13 +51,29 @@ pub(super) fn is_allowed_func_def(name: &str) -> bool { /// Returns `true` if an argument is allowed to use a boolean trap. To return /// `true`, the function name must be explicitly allowed, and the argument must /// be either the first or second argument in the call. -pub(super) fn allow_boolean_trap(func: &Expr) -> bool { - if let Expr::Attribute(ast::ExprAttribute { attr, .. }) = func { - return is_allowed_func_call(attr); +pub(super) fn allow_boolean_trap(call: &ast::ExprCall) -> bool { + let func_name = match call.func.as_ref() { + Expr::Attribute(ast::ExprAttribute { attr, .. }) => attr.as_str(), + Expr::Name(ast::ExprName { id, .. }) => id.as_str(), + _ => return false, + }; + + // If the function name is explicitly allowed, then the boolean trap is + // allowed. + if is_allowed_func_call(func_name) { + return true; } - if let Expr::Name(ast::ExprName { id, .. }) = func { - return is_allowed_func_call(id); + // If the function appears to be a setter (e.g., `set_visible` or `setVisible`), then the + // boolean trap is allowed. We want to avoid raising a violation for cases in which the argument + // is positional-only and third-party, and this tends to be the case for setters. + if call.arguments.args.len() == 1 { + if func_name + .strip_prefix("set") + .is_some_and(|suffix| suffix.starts_with(|c: char| c == '_' || c.is_ascii_uppercase())) + { + return true; + } } false diff --git a/crates/ruff_linter/src/rules/flake8_boolean_trap/rules/boolean_positional_value_in_call.rs b/crates/ruff_linter/src/rules/flake8_boolean_trap/rules/boolean_positional_value_in_call.rs index df95dc0ef5bc97..9d5c9697e1b260 100644 --- a/crates/ruff_linter/src/rules/flake8_boolean_trap/rules/boolean_positional_value_in_call.rs +++ b/crates/ruff_linter/src/rules/flake8_boolean_trap/rules/boolean_positional_value_in_call.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 as ast; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -45,11 +45,16 @@ impl Violation for BooleanPositionalValueInCall { } } -pub(crate) fn boolean_positional_value_in_call(checker: &mut Checker, args: &[Expr], func: &Expr) { - if allow_boolean_trap(func) { +pub(crate) fn boolean_positional_value_in_call(checker: &mut Checker, call: &ast::ExprCall) { + if allow_boolean_trap(call) { return; } - for arg in args.iter().filter(|arg| arg.is_boolean_literal_expr()) { + for arg in call + .arguments + .args + .iter() + .filter(|arg| arg.is_boolean_literal_expr()) + { checker .diagnostics .push(Diagnostic::new(BooleanPositionalValueInCall, arg.range())); diff --git a/crates/ruff_linter/src/rules/flake8_boolean_trap/snapshots/ruff_linter__rules__flake8_boolean_trap__tests__FBT001_FBT.py.snap b/crates/ruff_linter/src/rules/flake8_boolean_trap/snapshots/ruff_linter__rules__flake8_boolean_trap__tests__FBT001_FBT.py.snap index fe9adc83fc21f2..fe22f4c481a216 100644 --- a/crates/ruff_linter/src/rules/flake8_boolean_trap/snapshots/ruff_linter__rules__flake8_boolean_trap__tests__FBT001_FBT.py.snap +++ b/crates/ruff_linter/src/rules/flake8_boolean_trap/snapshots/ruff_linter__rules__flake8_boolean_trap__tests__FBT001_FBT.py.snap @@ -81,12 +81,12 @@ FBT.py:19:5: FBT001 Boolean-typed positional argument in function definition 21 | kwonly_nonvalued_nohint, | -FBT.py:89:19: FBT001 Boolean-typed positional argument in function definition +FBT.py:91:19: FBT001 Boolean-typed positional argument in function definition | -88 | # FBT001: Boolean positional arg in function definition -89 | def foo(self, value: bool) -> None: +90 | # FBT001: Boolean positional arg in function definition +91 | def foo(self, value: bool) -> None: | ^^^^^ FBT001 -90 | pass +92 | pass | diff --git a/crates/ruff_linter/src/rules/flake8_boolean_trap/snapshots/ruff_linter__rules__flake8_boolean_trap__tests__FBT003_FBT.py.snap b/crates/ruff_linter/src/rules/flake8_boolean_trap/snapshots/ruff_linter__rules__flake8_boolean_trap__tests__FBT003_FBT.py.snap index d2b902114550b5..af7c5002a07adf 100644 --- a/crates/ruff_linter/src/rules/flake8_boolean_trap/snapshots/ruff_linter__rules__flake8_boolean_trap__tests__FBT003_FBT.py.snap +++ b/crates/ruff_linter/src/rules/flake8_boolean_trap/snapshots/ruff_linter__rules__flake8_boolean_trap__tests__FBT003_FBT.py.snap @@ -28,14 +28,10 @@ FBT.py:57:17: FBT003 Boolean positional value in function call 59 | mylist.index(True) | -FBT.py:69:38: FBT003 Boolean positional value in function call - | -67 | os.set_blocking(0, False) -68 | g_action.set_enabled(True) -69 | settings.set_enable_developer_extras(True) - | ^^^^ FBT003 -70 | foo.is_(True) -71 | bar.is_not(False) - | +FBT.py:121:10: FBT003 Boolean positional value in function call + | +121 | settings(True) + | ^^^^ FBT003 + | diff --git a/crates/ruff_linter/src/rules/flake8_boolean_trap/snapshots/ruff_linter__rules__flake8_boolean_trap__tests__preview__FBT001_FBT.py.snap b/crates/ruff_linter/src/rules/flake8_boolean_trap/snapshots/ruff_linter__rules__flake8_boolean_trap__tests__preview__FBT001_FBT.py.snap index 575615ae710074..1f892a4d165330 100644 --- a/crates/ruff_linter/src/rules/flake8_boolean_trap/snapshots/ruff_linter__rules__flake8_boolean_trap__tests__preview__FBT001_FBT.py.snap +++ b/crates/ruff_linter/src/rules/flake8_boolean_trap/snapshots/ruff_linter__rules__flake8_boolean_trap__tests__preview__FBT001_FBT.py.snap @@ -81,26 +81,26 @@ FBT.py:19:5: FBT001 Boolean-typed positional argument in function definition 21 | kwonly_nonvalued_nohint, | -FBT.py:89:19: FBT001 Boolean-typed positional argument in function definition +FBT.py:91:19: FBT001 Boolean-typed positional argument in function definition | -88 | # FBT001: Boolean positional arg in function definition -89 | def foo(self, value: bool) -> None: +90 | # FBT001: Boolean positional arg in function definition +91 | def foo(self, value: bool) -> None: | ^^^^^ FBT001 -90 | pass +92 | pass | -FBT.py:99:10: FBT001 Boolean-typed positional argument in function definition +FBT.py:101:10: FBT001 Boolean-typed positional argument in function definition | - 99 | def func(x: Union[list, Optional[int | str | float | bool]]): +101 | def func(x: Union[list, Optional[int | str | float | bool]]): | ^ FBT001 -100 | pass +102 | pass | -FBT.py:103:10: FBT001 Boolean-typed positional argument in function definition +FBT.py:105:10: FBT001 Boolean-typed positional argument in function definition | -103 | def func(x: bool | str): +105 | def func(x: bool | str): | ^ FBT001 -104 | pass +106 | pass |