[flake8-simplify] Fix truthiness assumption for non-iterable arguments in tuple/list/set calls (SIM222, SIM223)
#21479
Conversation
|
|
Thanks, @danparizher, for this PR. It's great to see how you jump on every Ruff issue. We appreciate your contributions greatly, and you're very productive. You're that productive that it is challenging for us to keep up with the number of PRs you submit; it can sometimes even feel a bit overwhelming to see 5 new PRs. That's why I'd like to ask you to give us some time to catch up. I think an ideal contribution experience for us (you as a contributor and we as a reviewer) would be to aim for a low one-digit number of open PRs that we aim to land before starting any new ones. That gives us the necessary time to properly review your PRs, as well as those from other contributors. I hope you won't receive this as discouraging because we really value your contributions and we would love to see more of them, maybe just have them a little more spaces out so that we can keep up :) |
|
I'm not sure why this Thanks so much for the kind words @MichaReiser! No worries at all, I completely get it—sorry for the PR flood! "Overzealous contributor" is probably a fitting title; I just get excited about Ruff 😄 I'll throttle back and stick to just a couple of open PRs at a time. I totally respect the need to keep the review pipeline manageable for you all and appreciate the heads-up! |
| } 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 |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
I wonder if it might make more sense to list the types we explicitly want to recurse for instead of filtering out the cases where we don't want to recurse. Maybe just even a big match on argument would be easiest to reason about. What do you think?
Basically I think we need to return Self::Unknown here and avoid recursing for any type that has a definite Self::Truthy or Self::Falsey (or Self::None) type in one of the match arms above this, if the type might result in an empty iterable or will raise a type error.
As it stands, I don't think this PR actually fixes the t-string case from the issue.
|
|
||
|
|
||
| # https://github.com/astral-sh/ruff/issues/21473 | ||
| tuple("") or True # OK |
There was a problem hiding this comment.
We should add a t-string case:
| tuple("") or True # OK | |
| tuple(t"") or True # OK |
3 participants
Summary
Fixes false positives in SIM222 and SIM223 where truthiness was incorrectly assumed for
tuple(x),list(x),set(x)whenxis not iterable.Fixes #21473.
Problem
Truthiness::from_exprrecursively called itself on arguments to iterable initializers (tuple,list,set) without checking if the argument is iterable, causing false positives for cases liketuple(0) or Trueandtuple("") or True.Approach
Added
is_definitely_not_iterablehelper and updatedTruthiness::from_exprto returnUnknownfor non-iterable arguments (numbers, booleans, None) and string literals when called with iterable initializers, preventing incorrect truthiness assumptions.Test Plan
Added test cases to
SIM222.pyandSIM223.pyfortuple(""),tuple(0),tuple(1),tuple(False), andtuple(None)withor Trueandand Falsepatterns.