From 776eb8724f2348752ea354d749dfac4bf177c97c Mon Sep 17 00:00:00 2001 From: Jonathan Plasse <13716151+JonathanPlasse@users.noreply.github.com> Date: Sun, 12 Nov 2023 21:09:23 +0100 Subject: [PATCH] Fix FBT001 false negative with unions and optional (#7501) ## Summary - Close #7487 In the spirit of `flake8-boolean-trap`, any positional argument that can accept a boolean should raise `FBT001`. Raise `FBT001` for all annotations that accept booleans (e.g. `Optional[bool]`, `Union[int, bool]`). ## Test Plan Add a fixture, with an annotation using `|`, `Optional`, and `Union`, and containing a boolean. --- .../test/fixtures/flake8_boolean_trap/FBT.py | 15 +++ .../src/rules/flake8_boolean_trap/mod.rs | 19 ++++ .../boolean_type_hint_positional_argument.rs | 76 +++++++++++-- ...n_trap__tests__preview__FBT001_FBT.py.snap | 106 ++++++++++++++++++ 4 files changed, 207 insertions(+), 9 deletions(-) create mode 100644 crates/ruff_linter/src/rules/flake8_boolean_trap/snapshots/ruff_linter__rules__flake8_boolean_trap__tests__preview__FBT001_FBT.py.snap 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 88e5468692..58dcb4b516 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 @@ -91,3 +91,18 @@ class Registry: def foo(self) -> None: object.__setattr__(self, "flag", True) + + +from typing import Optional, Union + + +def func(x: Union[list, Optional[int | str | float | bool]]): + pass + + +def func(x: bool | str): + pass + + +def func(x: int | str): + pass diff --git a/crates/ruff_linter/src/rules/flake8_boolean_trap/mod.rs b/crates/ruff_linter/src/rules/flake8_boolean_trap/mod.rs index 41459c4c5e..ef132129d4 100644 --- a/crates/ruff_linter/src/rules/flake8_boolean_trap/mod.rs +++ b/crates/ruff_linter/src/rules/flake8_boolean_trap/mod.rs @@ -10,6 +10,7 @@ mod tests { use test_case::test_case; use crate::registry::Rule; + use crate::settings::types::PreviewMode; use crate::test::test_path; use crate::{assert_messages, settings}; @@ -25,4 +26,22 @@ mod tests { assert_messages!(snapshot, diagnostics); Ok(()) } + + #[test_case(Rule::BooleanTypeHintPositionalArgument, Path::new("FBT.py"))] + fn preview_rules(rule_code: Rule, path: &Path) -> Result<()> { + let snapshot = format!( + "preview__{}_{}", + rule_code.noqa_code(), + path.to_string_lossy() + ); + let diagnostics = test_path( + Path::new("flake8_boolean_trap").join(path).as_path(), + &settings::LinterSettings { + preview: PreviewMode::Enabled, + ..settings::LinterSettings::for_rule(rule_code) + }, + )?; + assert_messages!(snapshot, diagnostics); + Ok(()) + } } diff --git a/crates/ruff_linter/src/rules/flake8_boolean_trap/rules/boolean_type_hint_positional_argument.rs b/crates/ruff_linter/src/rules/flake8_boolean_trap/rules/boolean_type_hint_positional_argument.rs index 7c8fa6d360..1ba9a680d2 100644 --- a/crates/ruff_linter/src/rules/flake8_boolean_trap/rules/boolean_type_hint_positional_argument.rs +++ b/crates/ruff_linter/src/rules/flake8_boolean_trap/rules/boolean_type_hint_positional_argument.rs @@ -4,6 +4,7 @@ use ruff_diagnostics::Diagnostic; use ruff_diagnostics::Violation; use ruff_macros::{derive_message_formats, violation}; use ruff_python_ast::call_path::collect_call_path; +use ruff_python_semantic::SemanticModel; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; @@ -26,6 +27,9 @@ use crate::rules::flake8_boolean_trap::helpers::is_allowed_func_def; /// keyword-only argument, to force callers to be explicit when providing /// the argument. /// +/// In [preview], this rule will also flag annotations that include boolean +/// variants, like `bool | int`. +/// /// ## Example /// ```python /// from math import ceil, floor @@ -86,6 +90,8 @@ use crate::rules::flake8_boolean_trap::helpers::is_allowed_func_def; /// ## References /// - [Python documentation: Calls](https://docs.python.org/3/reference/expressions.html#calls) /// - [_How to Avoid “The Boolean Trap”_ by Adam Johnson](https://adamj.eu/tech/2021/07/10/python-type-hints-how-to-avoid-the-boolean-trap/) +/// +/// [preview]: https://docs.astral.sh/ruff/preview/ #[violation] pub struct BooleanTypeHintPositionalArgument; @@ -96,6 +102,7 @@ impl Violation for BooleanTypeHintPositionalArgument { } } +/// FBT001 pub(crate) fn boolean_type_hint_positional_argument( checker: &mut Checker, name: &str, @@ -122,15 +129,17 @@ pub(crate) fn boolean_type_hint_positional_argument( let Some(annotation) = parameter.annotation.as_ref() else { continue; }; - - // check for both bool (python class) and 'bool' (string annotation) - let hint = match annotation.as_ref() { - Expr::Name(name) => &name.id == "bool", - Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => value == "bool", - _ => false, - }; - if !hint || !checker.semantic().is_builtin("bool") { - continue; + if checker.settings.preview.is_enabled() { + if !match_annotation_to_complex_bool(annotation, checker.semantic()) { + continue; + } + } else { + if !match_annotation_to_literal_bool(annotation) { + continue; + } + } + if !checker.semantic().is_builtin("bool") { + return; } checker.diagnostics.push(Diagnostic::new( BooleanTypeHintPositionalArgument, @@ -138,3 +147,52 @@ pub(crate) fn boolean_type_hint_positional_argument( )); } } + +/// Returns `true` if the annotation is a boolean type hint (e.g., `bool`). +fn match_annotation_to_literal_bool(annotation: &Expr) -> bool { + match annotation { + // Ex) `True` + Expr::Name(name) => &name.id == "bool", + // Ex) `"True"` + Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => value == "bool", + _ => false, + } +} + +/// Returns `true` if the annotation is a boolean type hint (e.g., `bool`), or a type hint that +/// includes boolean as a variant (e.g., `bool | int`). +fn match_annotation_to_complex_bool(annotation: &Expr, semantic: &SemanticModel) -> bool { + match annotation { + // Ex) `bool` + Expr::Name(name) => &name.id == "bool", + // Ex) `"bool"` + Expr::StringLiteral(ast::ExprStringLiteral { value, .. }) => value == "bool", + // Ex) `bool | int` + Expr::BinOp(ast::ExprBinOp { + left, + op: ast::Operator::BitOr, + right, + .. + }) => { + match_annotation_to_complex_bool(left, semantic) + || match_annotation_to_complex_bool(right, semantic) + } + // Ex) `typing.Union[bool, int]` + Expr::Subscript(ast::ExprSubscript { value, slice, .. }) => { + if semantic.match_typing_expr(value, "Union") { + if let Expr::Tuple(ast::ExprTuple { elts, .. }) = slice.as_ref() { + elts.iter() + .any(|elt| match_annotation_to_complex_bool(elt, semantic)) + } else { + // Union with a single type is an invalid type annotation + false + } + } else if semantic.match_typing_expr(value, "Optional") { + match_annotation_to_complex_bool(slice, semantic) + } else { + false + } + } + _ => false, + } +} 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 new file mode 100644 index 0000000000..575615ae71 --- /dev/null +++ b/crates/ruff_linter/src/rules/flake8_boolean_trap/snapshots/ruff_linter__rules__flake8_boolean_trap__tests__preview__FBT001_FBT.py.snap @@ -0,0 +1,106 @@ +--- +source: crates/ruff_linter/src/rules/flake8_boolean_trap/mod.rs +--- +FBT.py:4:5: FBT001 Boolean-typed positional argument in function definition + | +2 | posonly_nohint, +3 | posonly_nonboolhint: int, +4 | posonly_boolhint: bool, + | ^^^^^^^^^^^^^^^^ FBT001 +5 | posonly_boolstrhint: "bool", +6 | /, + | + +FBT.py:5:5: FBT001 Boolean-typed positional argument in function definition + | +3 | posonly_nonboolhint: int, +4 | posonly_boolhint: bool, +5 | posonly_boolstrhint: "bool", + | ^^^^^^^^^^^^^^^^^^^ FBT001 +6 | /, +7 | offset, + | + +FBT.py:10:5: FBT001 Boolean-typed positional argument in function definition + | + 8 | posorkw_nonvalued_nohint, + 9 | posorkw_nonvalued_nonboolhint: int, +10 | posorkw_nonvalued_boolhint: bool, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ FBT001 +11 | posorkw_nonvalued_boolstrhint: "bool", +12 | posorkw_boolvalued_nohint=True, + | + +FBT.py:11:5: FBT001 Boolean-typed positional argument in function definition + | + 9 | posorkw_nonvalued_nonboolhint: int, +10 | posorkw_nonvalued_boolhint: bool, +11 | posorkw_nonvalued_boolstrhint: "bool", + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FBT001 +12 | posorkw_boolvalued_nohint=True, +13 | posorkw_boolvalued_nonboolhint: int = True, + | + +FBT.py:14:5: FBT001 Boolean-typed positional argument in function definition + | +12 | posorkw_boolvalued_nohint=True, +13 | posorkw_boolvalued_nonboolhint: int = True, +14 | posorkw_boolvalued_boolhint: bool = True, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ FBT001 +15 | posorkw_boolvalued_boolstrhint: "bool" = True, +16 | posorkw_nonboolvalued_nohint=1, + | + +FBT.py:15:5: FBT001 Boolean-typed positional argument in function definition + | +13 | posorkw_boolvalued_nonboolhint: int = True, +14 | posorkw_boolvalued_boolhint: bool = True, +15 | posorkw_boolvalued_boolstrhint: "bool" = True, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FBT001 +16 | posorkw_nonboolvalued_nohint=1, +17 | posorkw_nonboolvalued_nonboolhint: int = 2, + | + +FBT.py:18:5: FBT001 Boolean-typed positional argument in function definition + | +16 | posorkw_nonboolvalued_nohint=1, +17 | posorkw_nonboolvalued_nonboolhint: int = 2, +18 | posorkw_nonboolvalued_boolhint: bool = 3, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FBT001 +19 | posorkw_nonboolvalued_boolstrhint: "bool" = 4, +20 | *, + | + +FBT.py:19:5: FBT001 Boolean-typed positional argument in function definition + | +17 | posorkw_nonboolvalued_nonboolhint: int = 2, +18 | posorkw_nonboolvalued_boolhint: bool = 3, +19 | posorkw_nonboolvalued_boolstrhint: "bool" = 4, + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ FBT001 +20 | *, +21 | kwonly_nonvalued_nohint, + | + +FBT.py:89:19: FBT001 Boolean-typed positional argument in function definition + | +88 | # FBT001: Boolean positional arg in function definition +89 | def foo(self, value: bool) -> None: + | ^^^^^ FBT001 +90 | pass + | + +FBT.py:99:10: FBT001 Boolean-typed positional argument in function definition + | + 99 | def func(x: Union[list, Optional[int | str | float | bool]]): + | ^ FBT001 +100 | pass + | + +FBT.py:103:10: FBT001 Boolean-typed positional argument in function definition + | +103 | def func(x: bool | str): + | ^ FBT001 +104 | pass + | + +