Skip to content

Commit

Permalink
Fix FBT001 false negative with unions and optional (#7501)
Browse files Browse the repository at this point in the history
## 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.
  • Loading branch information
JonathanPlasse committed Nov 12, 2023
1 parent 5f78580 commit 776eb87
Show file tree
Hide file tree
Showing 4 changed files with 207 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -91,3 +91,18 @@ def foo(self, value: bool) -> None:

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
19 changes: 19 additions & 0 deletions crates/ruff_linter/src/rules/flake8_boolean_trap/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};

Expand All @@ -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(())
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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;

Expand All @@ -96,6 +102,7 @@ impl Violation for BooleanTypeHintPositionalArgument {
}
}

/// FBT001
pub(crate) fn boolean_type_hint_positional_argument(
checker: &mut Checker,
name: &str,
Expand All @@ -122,19 +129,70 @@ 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,
parameter.name.range(),
));
}
}

/// 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,
}
}
Original file line number Diff line number Diff line change
@@ -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
|


0 comments on commit 776eb87

Please sign in to comment.