mirror of
https://github.com/astral-sh/ruff.git
synced 2025-09-28 12:55:05 +00:00
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.
This commit is contained in:
parent
5f78580775
commit
776eb8724f
4 changed files with 207 additions and 9 deletions
|
@ -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
|
||||
|
|
|
@ -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(())
|
||||
}
|
||||
}
|
||||
|
|
|
@ -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,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") {
|
||||
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,
|
||||
}
|
||||
}
|
||||
|
|
|
@ -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
|
||||
|
|
||||
|
||||
|
Loading…
Add table
Add a link
Reference in a new issue