Skip to content

Commit

Permalink
[ruff] Implement redundant-bool-literal (RUF038) (astral-sh#14319)
Browse files Browse the repository at this point in the history
## Summary

Implements `redundant-bool-literal`

## Test Plan

<!-- How was it tested? -->

`cargo test`

The ecosystem results are all correct, but for `Airflow` the rule is not
relevant due to the use of overloading (and is marked as unsafe
correctly).

---------

Co-authored-by: Charlie Marsh <[email protected]>
  • Loading branch information
2 people authored and dylwil3 committed Nov 17, 2024
1 parent 0cff871 commit 20d6106
Show file tree
Hide file tree
Showing 11 changed files with 421 additions and 1 deletion.
1 change: 1 addition & 0 deletions crates/red_knot_workspace/tests/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -317,5 +317,6 @@ const KNOWN_FAILURES: &[(&str, bool, bool)] = &[
("crates/ruff_linter/resources/test/fixtures/ruff/RUF013_0.py", true, true),
("crates/ruff_linter/resources/test/fixtures/ruff/RUF013_3.py", true, true),
("crates/ruff_linter/resources/test/fixtures/ruff/RUF022.py", true, true),
("crates/ruff_linter/resources/test/fixtures/ruff/RUF038.py", true, true),
("crates/ruff_python_parser/resources/valid/expressions/f_string.py", true, true),
];
33 changes: 33 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF038.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
from typing import Literal


def func1(arg1: Literal[True, False]):
...


def func2(arg1: Literal[True, False, True]):
...


def func3() -> Literal[True, False]:
...


def func4(arg1: Literal[True, False] | bool):
...


def func5(arg1: Literal[False, True]):
...


def func6(arg1: Literal[True, False, "hello", "world"]):
...

# ok
def good_func1(arg1: bool):
...


def good_func2(arg1: Literal[True]):
...
20 changes: 20 additions & 0 deletions crates/ruff_linter/resources/test/fixtures/ruff/RUF038.pyi
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
from typing import Literal


def func1(arg1: Literal[True, False]): ...

def func2(arg1: Literal[True, False, True]): ...

def func3() -> Literal[True, False]: ...

def func4(arg1: Literal[True, False] | bool): ...

def func5(arg1: Literal[False, True]): ...

def func6(arg1: Literal[True, False, "hello", "world"]): ...

# ok
def good_func1(arg1: bool): ...

def good_func2(arg1: Literal[True]): ...

9 changes: 8 additions & 1 deletion crates/ruff_linter/src/checkers/ast/analyze/expression.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,11 +104,18 @@ pub(crate) fn expression(expr: &Expr, checker: &mut Checker) {
}

// Ex) Literal[...]
if checker.any_enabled(&[Rule::DuplicateLiteralMember, Rule::RedundantNoneLiteral]) {
if checker.any_enabled(&[
Rule::DuplicateLiteralMember,
Rule::RedundantBoolLiteral,
Rule::RedundantNoneLiteral,
]) {
if !checker.semantic.in_nested_literal() {
if checker.enabled(Rule::DuplicateLiteralMember) {
flake8_pyi::rules::duplicate_literal_member(checker, expr);
}
if checker.enabled(Rule::RedundantBoolLiteral) {
ruff::rules::redundant_bool_literal(checker, expr);
}
if checker.enabled(Rule::RedundantNoneLiteral) {
flake8_pyi::rules::redundant_none_literal(checker, expr);
}
Expand Down
1 change: 1 addition & 0 deletions crates/ruff_linter/src/codes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -969,6 +969,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> {
(Ruff, "034") => (RuleGroup::Preview, rules::ruff::rules::UselessIfElse),
(Ruff, "035") => (RuleGroup::Preview, rules::ruff::rules::UnsafeMarkupUse),
(Ruff, "036") => (RuleGroup::Preview, rules::ruff::rules::NoneNotAtEndOfUnion),
(Ruff, "038") => (RuleGroup::Preview, rules::ruff::rules::RedundantBoolLiteral),
(Ruff, "100") => (RuleGroup::Stable, rules::ruff::rules::UnusedNOQA),
(Ruff, "101") => (RuleGroup::Stable, rules::ruff::rules::RedirectedNOQA),

Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/ruff/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,8 @@ mod tests {
#[test_case(Rule::PostInitDefault, Path::new("RUF033.py"))]
#[test_case(Rule::NoneNotAtEndOfUnion, Path::new("RUF036.py"))]
#[test_case(Rule::NoneNotAtEndOfUnion, Path::new("RUF036.pyi"))]
#[test_case(Rule::RedundantBoolLiteral, Path::new("RUF038.py"))]
#[test_case(Rule::RedundantBoolLiteral, Path::new("RUF038.pyi"))]
fn rules(rule_code: Rule, path: &Path) -> Result<()> {
let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy());
let diagnostics = test_path(
Expand Down
2 changes: 2 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ pub(crate) use parenthesize_logical_operators::*;
pub(crate) use post_init_default::*;
pub(crate) use quadratic_list_summation::*;
pub(crate) use redirected_noqa::*;
pub(crate) use redundant_bool_literal::*;
pub(crate) use sort_dunder_all::*;
pub(crate) use sort_dunder_slots::*;
pub(crate) use static_key_dict_comprehension::*;
Expand Down Expand Up @@ -61,6 +62,7 @@ mod parenthesize_logical_operators;
mod post_init_default;
mod quadratic_list_summation;
mod redirected_noqa;
mod redundant_bool_literal;
mod sequence_sorting;
mod sort_dunder_all;
mod sort_dunder_slots;
Expand Down
137 changes: 137 additions & 0 deletions crates/ruff_linter/src/rules/ruff/rules/redundant_bool_literal.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
use ruff_diagnostics::{Diagnostic, Edit, Fix, FixAvailability, Violation};
use ruff_macros::{derive_message_formats, violation};
use ruff_python_ast::Expr;
use ruff_python_semantic::analyze::typing::traverse_literal;
use ruff_text_size::Ranged;

use bitflags::bitflags;

use crate::checkers::ast::Checker;

/// ## What it does
/// Checks for `Literal[True, False]` type annotations.
///
/// ## Why is this bad?
/// `Literal[True, False]` can be replaced with `bool` in type annotations,
/// which has the same semantic meaning but is more concise and readable.
///
/// `bool` type has exactly two constant instances: `True` and `False`. Static
/// type checkers such as [mypy] treat `Literal[True, False]` as equivalent to
/// `bool` in a type annotation.
///
/// ## Example
/// ```python
/// from typing import Literal
///
/// x: Literal[True, False]
/// y: Literal[True, False, "hello", "world"]
/// ```
///
/// Use instead:
/// ```python
/// from typing import Literal
///
/// x: bool
/// y: Literal["hello", "world"] | bool
/// ```
///
/// ## Fix safety
/// The fix for this rule is marked as unsafe, as it may change the semantics of the code.
/// Specifically:
///
/// - Type checkers may not treat `bool` as equivalent when overloading boolean arguments
/// with `Literal[True]` and `Literal[False]` (see, e.g., [#14764] and [#5421]).
/// - `bool` is not strictly equivalent to `Literal[True, False]`, as `bool` is
/// a subclass of `int`, and this rule may not apply if the type annotations are used
/// in a numeric context.
///
/// Further, the `Literal` slice may contain trailing-line comments which the fix would remove.
///
/// ## References
/// - [Typing documentation: Legal parameters for `Literal` at type check time](https://typing.readthedocs.io/en/latest/spec/literal.html#legal-parameters-for-literal-at-type-check-time)
/// - [Python documentation: Boolean type - `bool`](https://docs.python.org/3/library/stdtypes.html#boolean-type-bool)
///
/// [mypy]: https://github.com/python/mypy/blob/master/mypy/typeops.py#L985
/// [#14764]: https://github.com/python/mypy/issues/14764
/// [#5421]: https://github.com/microsoft/pyright/issues/5421
#[violation]
pub struct RedundantBoolLiteral {
seen_others: bool,
}

impl Violation for RedundantBoolLiteral {
const FIX_AVAILABILITY: FixAvailability = FixAvailability::Sometimes;

#[derive_message_formats]
fn message(&self) -> String {
if self.seen_others {
"`Literal[True, False, ...]` can be replaced with `Literal[...] | bool`".to_string()
} else {
"`Literal[True, False]` can be replaced with `bool`".to_string()
}
}

fn fix_title(&self) -> Option<String> {
Some(if self.seen_others {
"Replace with `Literal[...] | bool`".to_string()
} else {
"Replace with `bool`".to_string()
})
}
}

/// RUF038
pub(crate) fn redundant_bool_literal<'a>(checker: &mut Checker, literal_expr: &'a Expr) {
if !checker.semantic().seen_typing() {
return;
}

let mut seen_expr = BooleanLiteral::empty();

let mut find_bools = |expr: &'a Expr, _parent: &'a Expr| {
let expr_type = match expr {
Expr::BooleanLiteral(boolean_expr) => {
if boolean_expr.value {
BooleanLiteral::TRUE
} else {
BooleanLiteral::FALSE
}
}
_ => BooleanLiteral::OTHER,
};
seen_expr.insert(expr_type);
};

traverse_literal(&mut find_bools, checker.semantic(), literal_expr);

if !seen_expr.contains(BooleanLiteral::TRUE | BooleanLiteral::FALSE) {
return;
}

let seen_others = seen_expr.contains(BooleanLiteral::OTHER);

let mut diagnostic =
Diagnostic::new(RedundantBoolLiteral { seen_others }, literal_expr.range());

// Provide a [`Fix`] when the complete `Literal` can be replaced. Applying the fix
// can leave an unused import to be fixed by the `unused-import` rule.
if !seen_others {
if checker.semantic().has_builtin_binding("bool") {
diagnostic.set_fix(Fix::unsafe_edit(Edit::range_replacement(
"bool".to_string(),
literal_expr.range(),
)));
}
}

checker.diagnostics.push(diagnostic);
}

bitflags! {
#[derive(Default, Debug)]
struct BooleanLiteral: u8 {
const TRUE = 1 << 0;
const FALSE = 1 << 1;
const OTHER = 1 << 2;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
---
source: crates/ruff_linter/src/rules/ruff/mod.rs
---
RUF038.py:4:17: RUF038 [*] `Literal[True, False]` can be replaced with `bool`
|
4 | def func1(arg1: Literal[True, False]):
| ^^^^^^^^^^^^^^^^^^^^ RUF038
5 | ...
|
= help: Replace with `bool`

Unsafe fix
1 1 | from typing import Literal
2 2 |
3 3 |
4 |-def func1(arg1: Literal[True, False]):
4 |+def func1(arg1: bool):
5 5 | ...
6 6 |
7 7 |

RUF038.py:8:17: RUF038 [*] `Literal[True, False]` can be replaced with `bool`
|
8 | def func2(arg1: Literal[True, False, True]):
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF038
9 | ...
|
= help: Replace with `bool`

Unsafe fix
5 5 | ...
6 6 |
7 7 |
8 |-def func2(arg1: Literal[True, False, True]):
8 |+def func2(arg1: bool):
9 9 | ...
10 10 |
11 11 |

RUF038.py:12:16: RUF038 [*] `Literal[True, False]` can be replaced with `bool`
|
12 | def func3() -> Literal[True, False]:
| ^^^^^^^^^^^^^^^^^^^^ RUF038
13 | ...
|
= help: Replace with `bool`

Unsafe fix
9 9 | ...
10 10 |
11 11 |
12 |-def func3() -> Literal[True, False]:
12 |+def func3() -> bool:
13 13 | ...
14 14 |
15 15 |

RUF038.py:16:17: RUF038 [*] `Literal[True, False]` can be replaced with `bool`
|
16 | def func4(arg1: Literal[True, False] | bool):
| ^^^^^^^^^^^^^^^^^^^^ RUF038
17 | ...
|
= help: Replace with `bool`

Unsafe fix
13 13 | ...
14 14 |
15 15 |
16 |-def func4(arg1: Literal[True, False] | bool):
16 |+def func4(arg1: bool | bool):
17 17 | ...
18 18 |
19 19 |

RUF038.py:20:17: RUF038 [*] `Literal[True, False]` can be replaced with `bool`
|
20 | def func5(arg1: Literal[False, True]):
| ^^^^^^^^^^^^^^^^^^^^ RUF038
21 | ...
|
= help: Replace with `bool`

Unsafe fix
17 17 | ...
18 18 |
19 19 |
20 |-def func5(arg1: Literal[False, True]):
20 |+def func5(arg1: bool):
21 21 | ...
22 22 |
23 23 |

RUF038.py:24:17: RUF038 `Literal[True, False, ...]` can be replaced with `Literal[...] | bool`
|
24 | def func6(arg1: Literal[True, False, "hello", "world"]):
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ RUF038
25 | ...
|
= help: Replace with `Literal[...] | bool`
Loading

0 comments on commit 20d6106

Please sign in to comment.