-
Couldn't load subscription status.
- Fork 1.6k
Fix some false positive for is_non_empty_f_string
#21102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This looks good, I just had a couple of small ideas/suggestions.
| } | ||
| }) | ||
| !f_string.elements.is_empty() | ||
| && f_string.elements.iter().all(|element| match element { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this just be any instead? I could certainly be wrong, but it seems like we should check if any part is non-empty rather than all parts are non-empty. That would also give the correct behavior for the empty iterator and match the outer any on line 1401.
I tried this locally, and I think it fixed an existing bug in a SIM222 test too.
| // Compare can return any value, even empty string | ||
| // https://docs.python.org/3/reference/datamodel.html#object.__ge__ | ||
| Expr::Compare(_) => false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move this down to the other complex expressions below? It seems slightly out of place in this block now that the other variants are all true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be more precise, this could still be true if all the operators in the Compare are the ones guaranteed to return bools (in, is, is not, and not in).
Summary
Closes #21048
Fix some false positive edge cases when determining string truthiness. The following cases are now treated as unknown truthiness:
Additionally, fixed a bug that caused
f"{f""!s}"to be treated as non-empty. Since.iter().all()returns true if the iterator is empty, f-strings with no elements (like nestedf"{f""!s}") were incorrectly treated as non-empty.Test Plan
Truthinesshas no tests of its own