-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Fix false negatives in Truthiness::from_expr for lambdas, generators, and f-strings
#20704
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
Conversation
|
dylwil3
left a comment
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.
Sorry for the delay - I have one question that I didn't notice the first time around.
| InterpolatedStringElement::Interpolation(f_string) => inner(&f_string.expression), | ||
| InterpolatedStringElement::Interpolation(f_string) => { | ||
| f_string.debug_text.is_some() | ||
| || !matches!(f_string.conversion, ast::ConversionFlag::None) |
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.
I'm not sure I understand this line. Which of the deductions in inner is incorrect for string, ascii, or repr conversion flags?
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.
When an f-string interpolation has a conversion flag like !s, !a, or !r, Python's str(), ascii(), and repr() functions always produce a non-empty string representation of the object.
This means that all the complex expressions in the inner function (like BoolOp, BinOp, Call, Attribute, etc.) that normally return false because we can't determine if they're empty become guaranteed to be non-empty when conversion flags are applied.
The conversion flags essentially override the uncertainty by ensuring the result is always a meaningful string representation, which is why the condition correctly returns true whenever any conversion flag is present.
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.
All three conversions can produce empty strings.
$ cat >counterexamples.py <<'# EOF'
from configparser import Error
error = Error()
print(len(f"{error!s}"))
print(len(f"{error!a}"))
print(len(f"{error!r}"))
# EOF
$ python counterexamples.py
0
0
0There 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 for this; I was wrong about that then. In that case, we should have the condition removed.
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.
I don't think those counterexamples are relevant here, as far as I can tell. The function in question returns true when we can be certain that the f-string is nonempty, and false otherwise. It already returns false for Name expressions, so it would already be okay for counterexamples.py with or without worrying about conversion flags.
My claim is that the expressions for which we return true would be unaffected by the presence of these flags (insofar as their boolean value).
…able * dcreager/non-inferable-api: just the api parts [ty] Fix further issues in `super()` inference logic (#20843) [ty] Document when a rule was added (#20859) [ty] Treat `Callable` dunder members as bound method descriptors (#20860) [ty] Handle decorators which return unions of `Callable`s (#20858) Fix false negatives in `Truthiness::from_expr` for lambdas, generators, and f-strings (#20704) [ty] Rename Type unwrapping methods (#20857) Update `lint.flake8-type-checking.quoted-annotations` docs (#20765)
Summary
Fixes #20703