Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -216,3 +216,11 @@ def get_items_list():

def get_items_set():
return tuple({item for item in items}) or None # OK


# https://github.com/astral-sh/ruff/issues/21473
tuple("") or True # OK
tuple(0) or True # OK
tuple(1) or True # OK
tuple(False) or True # OK
tuple(None) or True # OK
Original file line number Diff line number Diff line change
Expand Up @@ -157,3 +157,11 @@

# https://github.com/astral-sh/ruff/issues/7127
def f(a: "'' and 'b'"): ...


# https://github.com/astral-sh/ruff/issues/21473
tuple("") and False # OK
tuple(0) and False # OK
tuple(1) and False # OK
tuple(False) and False # OK
tuple(None) and False # OK
Original file line number Diff line number Diff line change
Expand Up @@ -174,14 +174,3 @@ PT015 Assertion always fails, replace with `pytest.fail()`
| ^^^^^^^^^^^^^^^^^
25 | assert tuple("")
|

PT015 Assertion always fails, replace with `pytest.fail()`
--> PT015.py:25:5
|
23 | assert list([])
24 | assert set(set())
25 | assert tuple("")
| ^^^^^^^^^^^^^^^^
26 |
27 | # https://github.com/astral-sh/ruff/issues/19935
|
26 changes: 26 additions & 0 deletions crates/ruff_python_ast/src/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1212,6 +1212,19 @@ pub enum Truthiness {
Unknown,
}

/// Return `true` if the expression is definitely not iterable (e.g., numbers, booleans, None).
/// This is used to avoid incorrectly assuming truthiness for `tuple(x)`, `list(x)`, etc.
/// when `x` is not iterable.
fn is_definitely_not_iterable(expr: &Expr) -> bool {
matches!(
expr,
Expr::NumberLiteral(_)
| Expr::BooleanLiteral(_)
| Expr::NoneLiteral(_)
| Expr::EllipsisLiteral(_)
)
}

impl Truthiness {
/// Return the truthiness of an expression.
pub fn from_expr<F>(expr: &Expr, is_builtin: F) -> Self
Expand Down Expand Up @@ -1328,6 +1341,19 @@ impl Truthiness {
// recursing into Self::from_expr below, which returns Unknown for them.
if argument.is_generator_expr() {
Self::Unknown
} else if is_definitely_not_iterable(argument) {
// For non-iterable arguments like numbers, booleans, None, etc.,
// we can't assume truthiness. For example, tuple(0) raises TypeError.
Self::Unknown
} else if matches!(argument, Expr::StringLiteral(_)) {
// For strings, tuple("") creates an empty tuple which is falsy,
// but tuple("a") creates a non-empty tuple which is truthy.
// However, we can't assume truthiness matches because:
// - tuple("") is falsy (empty tuple), but "" is also falsy
// - The issue is that we conflate truthiness with non-emptiness
// - For SIM222/SIM223, we should be conservative and return Unknown
// to avoid false positives
Self::Unknown
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can omit this branch. Self::from_expr should already work correctly for string literals. The problem in the issue is for t-strings, among other types, which behave differently.

Removing this also fixes the pytest snapshot, which I think was a true positive.

} else {
Self::from_expr(argument, is_builtin)
}
Expand Down
Loading