-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[flake8-bandit] Fix truthiness: dict-only ** displays not truthy for shell (S602, S604, S609)
#20177
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
|
|
With this change, |
Good call, I had nested dict displays as truthy. I just pushed a change to have them checked recursively. |
| DictItem { | ||
| key: None, | ||
| value: Expr::Name(..) | ||
| // If the dict consists only of double-starred items (e.g., {**x, **y}), |
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 for diving deep here, but I think I'd kind of lean towards just considering any starred mapping to be of unknown truthiness (so just removing the Expr::Name requirement from matches!). I don't think nested, unpacked, empty dict literals are very common, so it seems like a nicer trade-off between implementation complexity and accuracy. That's also what we do for lists, sets, and tuples just above this. Expr::is_starred_expr for those elements is analogous to matches!(item, DictItem { key: None, ... }) for dicts.
|
|
||
| # Check dict display with only double-starred expressions can be falsey. | ||
| Popen("true", shell={**{}}) | ||
| Popen("true", shell={**{**{}}}) |
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.
It might also be nice to add a test case for a more common version of this problem like {**self.shell_defaults, **self.fetch_shell_config(self.username)} mentioned here. But the code fix should be the same anyway.
|
The CI failures seem to be unrelated to my changes, wondering if the one for |
|
Yeah no worries about the fuzz build. There was a breaking libcst release that we need to handle. |
ntBre
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.
Thank you! I'll probably close and reopen to re-trigger CI, but this looks ready to go once it passes.
Summary
Fixes #19927