Clear with drain #10528
Clear with drain #10528
Conversation
Rename method to `is_range_full` because the type is actually `RangeFull`. Method moved to `clippy_utils` for reuse in `clear_with_drain`.
|
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. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
rustbot
added
the
S-waiting-on-review
| v.drain(1..v.len() - 1); // Yay | ||
| } | ||
|
|
||
| fn main() {} |
There was a problem hiding this comment.
I'd like to see some test cases where the drain iterator is actually used.
There was a problem hiding this comment.
Would you like me to add some code that uses the iter variables in the existing test cases? Or did you mean that I should write some additional tests, like for instance:
let mut v = vec![1, 2, 3];
for x in v.drain(..) {
// do something with x
}There was a problem hiding this comment.
We should rule out both cases. Even if it's v.drain(..).count(), the lint should not trigger.
There was a problem hiding this comment.
I added some tests where the drain iterator is used, in both the 'should trigger' and 'should not trigger' cases
clippy_utils/src/lib.rs
Outdated
| /// Checks whether the given `Range` is equivalent to a `RangeFull`. | ||
| /// Inclusive ranges are not considered because they already constitute a lint. | ||
| pub fn is_range_full(cx: &LateContext<'_>, container: &Expr<'_>, range: Range<'_>) -> bool { | ||
| range.start.map_or(true, |e| is_integer_const(cx, e, 0)) |
There was a problem hiding this comment.
Since you made this a clippy_utils function, we should take care to either document any possible shortcomings or rule them out. For example, I think the method will fail on a Range<i8>.
There was a problem hiding this comment.
Thx for pointing that out! It didn't cross my mind...
Unfortunately I don't see a way to make it work for signed integers because right now the test on the lower bound is performed with is_integer_const, which takes a u128 as a value to check for, so I can't check for i8::MIN.
Do you have some way that that could be accomplished in mind? Otherwise, I will document the function more precisely.
I have to admit that the applicability of this function is a bit awkward, for example I don't think it works on ..usize::MAX either. It was made specifically to detect RangeFulls or equivalents used to index Vecs and VecDeques, but the way it's written it also works in a few other cases. Maybe I can tweak the implementation to only check for RangeTo, RangeFrom and Range if the range is the argument of a method call of a Vec or VecDeque, what do you think?
There was a problem hiding this comment.
I guess the easiest way is to rename to is_usize_range_full and document the restricted applicability of the method. Otherwise you can get the type (via expr_ty), then the generic, then match on that to figure out the bounds.
There was a problem hiding this comment.
Thx for the pointer, I will try to get the generic case working and if I really can't I will fall back to the usize solution with added documentation.
There was a problem hiding this comment.
Cool! Feel free to ping us here or on zulip if you need further assistance.
There was a problem hiding this comment.
It took a fair amount of work but I think I got it! At least it works as expected in my use case, and I added a couple of tests where I use usize::MIN as a lower bound to make sure that works as well (it already worked before with the hard-coded 0 but I thought it would be nice to make sure that it works).
I haven't added tests for the case where signed integers are used because this lint only applies in a method call to drain which only takes usize ranges. Do you need me to add unit tests for the new is_range_full function before accepting this PR? If so, where are such tests placed in the repo?
Note that I had to significantly alter the function signature of is_range_full to make it more generic. For instance, I made the container optional so that this function can be called on any kind of range.
|
I only found a few nits, with the additional tests and the wrong lower bound in the |
Make this function work with signed integer types by extracting the underlying type and finding the min and max values. Change the signature to make it more consistent: - The range is now given as an `Expr` in order to extract the type - The container's path is now passed, and only as an `Option` so that the function can be called in the general case without a container
tests/ui/clear_with_drain.rs
Outdated
| let iter = v.drain(0..); // Yay | ||
|
|
||
| let mut v = vec![1, 2, 3]; | ||
| let next = v.drain(0..).next(); // Yay |
There was a problem hiding this comment.
Could we have an example where the iter is only used later?
|
Thank you! @bors r+ |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
changelog: [
clear_with_drain]: Add new lintFixes #9339