Skip to content

Commit

Permalink
Fix FBT001 false negative with unions and optional
Browse files Browse the repository at this point in the history
  • Loading branch information
JonathanPlasse committed Oct 17, 2023
1 parent dc6b4ad commit 47fc41d
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -91,3 +91,10 @@ def foo(self, value: bool) -> None:

def foo(self) -> None:
object.__setattr__(self, "flag", True)


from typing import Optional, Union


def passes(x: Union[list, Optional[int | str | float | bool]]):
pass
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 Down Expand Up @@ -96,6 +97,40 @@ impl Violation for BooleanTypeHintPositionalArgument {
}
}

fn match_annotation_to_bool(annotation: &Expr, semantic: &SemanticModel) -> bool {
// check for both bool (python class) and 'bool' (string annotation)
let hint = match annotation {
Expr::Name(name) => &name.id == "bool",
Expr::Constant(ast::ExprConstant {
value: Constant::Str(ast::StringConstant { value, .. }),
..
}) => value == "bool",
Expr::BinOp(ast::ExprBinOp {
left,
op: ast::Operator::BitOr,
right,
..
}) => match_annotation_to_bool(left, semantic) || match_annotation_to_bool(right, semantic),
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_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_bool(slice, semantic)
} else {
false
}
}
_ => false,
};
hint && semantic.is_builtin("bool")
}

pub(crate) fn boolean_type_hint_positional_argument(
checker: &mut Checker,
name: &str,
Expand Down Expand Up @@ -123,16 +158,7 @@ pub(crate) fn boolean_type_hint_positional_argument(
continue;
};

// check for both bool (python class) and 'bool' (string annotation)
let hint = match annotation.as_ref() {
Expr::Name(name) => &name.id == "bool",
Expr::Constant(ast::ExprConstant {
value: Constant::Str(ast::StringConstant { value, .. }),
..
}) => value == "bool",
_ => false,
};
if !hint || !checker.semantic().is_builtin("bool") {
if !match_annotation_to_bool(annotation, checker.semantic()) {
continue;
}
checker.diagnostics.push(Diagnostic::new(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,4 +89,11 @@ FBT.py:89:19: FBT001 Boolean-typed positional argument in function definition
90 | pass
|

FBT.py:99:12: FBT001 Boolean-typed positional argument in function definition
|
99 | def passes(x: Union[list, Optional[int | str | float | bool]]):
| ^ FBT001
100 | pass
|


0 comments on commit 47fc41d

Please sign in to comment.