Github Add `needless_bitwise_bool` lint by arya-k · Pull Request #7133 · rust-la...
source link: https://github.com/rust-lang/rust-clippy/pull/7133
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
changelog: Add [`needless_bitwise_bool`] lint
Creates a new bitwise_bool lint to convert x & y to x && y when both x and y are booleans. I also had to adjust thh needless_bool lint slightly, and fix a couple failing dogfood tests. I made it a correctness lint as per flip1995's comment here, from a previous WIP attempt at this lint.
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) soon.
If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.
Please see the contribution instructions for more information.
Not a review. Just some comments.
& is only 'bitwise and' when applied to integers. Applied to booleans, & is 'logical and' and && is 'lazy and'. This is the terminology used by the reference. I think this lint needs a better name.
This can't be a correction lint. x & y and x && y do exactly the same thing (hopefully!). Maybe this is a performance or style lint.
It looks like there is no checking for side-effects when y is a function. I suspect this lint will have a lot of false positives since most people use && habitually and only & when they specifically want side-effects.
Yeah, I would make it a pedantic lint or perhaps a style lint. But: Performance is actually tricky in that regard. Let's take an example: while x & some_expensive_computation(..) would likely benefit, the other way 'round would very likely not; it is not clear whether introducing branches serves any useful purpose here. On the other hand, those are the exact cases where the side effects could make a difference.
To check for side effects, we could reuse some of the must_use_candidate machinery, which is not perfect, but the best we currently have. So I'd propose changing the lint so that it only lints in cases where the right hand side does some computation (is not only an ExprPath/ExprLit nor in const context) that is free of side effects (as per !(must_use::has_mutable_arg(..) || must_use::mutates_static(..)).
So, to summarize:
- Small naming nit in the function.
- Lint only where
- the right hand side is a
Call,MethodCall,BinOporUnOpand - the called method (or trait method) does not match``must_use::has_mutable_args(..)
normust_use::mutates_static(..)`. You may move the functions to `clippy_utils` for that.
- the right hand side is a
- Make it a pedantic lint (I would like to make it perf, but the checks don't capture all side effects)
- Change the docs to that effect. Write that it is likely to improve perf, but that the lint does not detect all possible side effects, so apply caution before applying the suggestion.
The latest upstream changes (presumably #7137) made this pull request unmergeable. Please resolve the merge conflicts.
Now that I'm aware of the short-circuiting difference, I'm starting to question the value of this lint. If side-effects are possible, Clippy has no way of knowing the user's intent and can't lint. If side-effects are not possible (like OR'ing two variables), then maybe logical operators are more readable but it seems quite subjective in those cases. But my original idea with this lint was to strictly prefer logical operators for readability because the meaning is not "overloaded" with bitwise operations.
For the name, I'd suggest needless_bitwise_bool (bool operations that are needlessly bitwise). And maybe complement it with a restriction lint bitwise_bool that bans the operators altogether.
@flip1995 Do you still think this should be correctness? It is not "outright wrong or useless". Just questionable style, and subjectively so.
I ran this lint on a bunch of crates. Here are some things I found.
-
Large number (16) of warnings in ascii . This is not necessary a problem but it means proceed with caution.
-
The problem where the right operand contains a side effect in bitvec. Applying the suggestion breaks this code. We already discussed this but now we have a real-world example.
warning: use of bitwise operator instead of logical operator between booleans
--> src/slice/ops.rs:38:26
|
38 | self.for_each(|_, bit| bit & iter.next().unwrap_or(false));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `bit && iter.next().unwrap_or(false)`
|
= note: requested on the command line with `-W clippy::bitwise-bool`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bitwise_bool
- A place in ndarray where replacing
|with||is probably a big mistake performance wise.
warning: use of bitwise operator instead of logical operator between booleans
--> src/numeric_util.rs:126:12
|
126 | if (xs[0] != ys[0])
| ____________^
127 | | | (xs[1] != ys[1])
128 | | | (xs[2] != ys[2])
129 | | | (xs[3] != ys[3])
... |
132 | | | (xs[6] != ys[6])
133 | | | (xs[7] != ys[7])
| |______________________________^
|
= note: requested on the command line with `-W clippy::bitwise-bool`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bitwise_bool
help: try
|
126 | if (xs[0] != ys[0])
127 | | (xs[1] != ys[1])
128 | | (xs[2] != ys[2])
129 | | (xs[3] != ys[3])
130 | | (xs[4] != ys[4])
131 | | (xs[5] != ys[5])
...
- The lint also needs an external macro check. RustCrypto for example.
warning: use of bitwise operator instead of logical operator between booleans
--> src/backend/autodetect.rs:7:1
|
7 | cpuid_bool::new!(clmul_cpuid, "pclmulqdq", "sse4.1");
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: requested on the command line with `-W clippy::bitwise-bool`
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bitwise_bool
= note: this warning originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)
126 | if (xs[0] != ys[0]) | ____________^ 127 | | | (xs[1] != ys[1]) 128 | | | (xs[2] != ys[2]) 129 | | | (xs[3] != ys[3]) ... | 132 | | | (xs[6] != ys[6]) 133 | | | (xs[7] != ys[7])
Tangential, but this looks like another lint opportunity. xs[..8] == ys[..8] amiright?
@camsteffen no, that will likely short circuit if I'm not mistaken. The bitwise op here is by design so that the equality gets autovectorized.
@flip1995 Do you still think this should be
correctness? It is not "outright wrong or useless". Just questionable style, and subjectively so.
I can't remember when and where I thought this should be correctness. No, this isn't wrong. Maybe perf? Reading through @mikerite findings pedantic would maybe also be reasonable.
I can't remember when and where I thought this should be
correctness.
See link in OP. Or just forget it. It was over two years ago. 
I would think it compiles the same if LLVM can see there are no side-effects? Sounds like we can all agree on pedantic for now.
Okay thank you everyone for the feedback- I didn't expect this lint to garner so much discussion, but it's been illuminating to see the impacts of what at first appears to be a relatively benign change. I'll try and update the lint with the following changes:
- Rename to
needless_bitwise_bool - Make the lint
pedantic - Lint only where the right hand side is a Call, MethodCall, BinOp or UnOp and the called method (or trait method) does not match``must_use::has_mutable_args(..)normust_use::mutates_static(..)` and the lint is not in a macro
- Adjust the docs (in particular, fixing the notation of logical (&) and lazy (&&) operators).
I'll then go ahead and run the lints on the same crates that @mikerite did, and report back. Does this seem like a reasonable plan to everyone?
I think you can just use Expr::can_have_side_effects for the right side check.
Hello everyone! Sorry for the delay- I believe I've implemented all the changes I outlined above. I took @camsteffen's suggestion and made the lint use Expr::can_have_side_effects, since that will improve over time.
For now the lint is very conservative; I've added some tests that I hope we could one day lint in the future, depending on how can_have_side_effects improves. At this time I think I'm ready for a fresh review if @llogiq is able to!
Thank you for seeing this through! And yes, the side effect check is overly pessimistic. As I wrote above, I had written my own one for the must_use_candidate lint, but that may lead to false positives. In any event, it's good to start out conservatively.
@bors r+
Commit 5ba236f has been approved by llogiq
Testing commit 
Test failed - checks-action_test
@bors r=llogiq
Commit 0dcde71 has been approved by llogiq
Test failed - checks-action_test
@bors retry
Recommend
About Joyk
Aggregate valuable and interesting links.
Joyk means Joy of geeK