[useless_vec]: lint vec! invocations when a slice or an array would do
#10901
Conversation
|
r? @Alexendoo (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
the
S-waiting-on-review
|
@Centri3 may also review this |
| /// that also exists on slices. If this returns true, it means that | ||
| /// this expression does not actually require a `Vec` and could just work with an array. | ||
| fn is_allowed_vec_method(cx: &LateContext<'_>, e: &Expr<'_>) -> bool { | ||
| const ALLOWED_METHOD_NAMES: &[&str] = &["len", "as_ptr", "is_empty"]; |
There was a problem hiding this comment.
I'm a bit curious why len and is_empty are allowed.
There was a problem hiding this comment.
The method names that are checked for here exist on both slices and Vec<T>.
So, given a variable ve of type Vec<i32>, a call expression like ve.is_empty() does not actually call slice::is_empty, but Vec::is_empty, which also means that adjusts_to_slice(ve) would return false.
If we didn't have len and is_empty here, the lint would assume that those are Vec<T> methods that really do require a Vec<T> (like push and pop, for example).
For len and is_empty specifically, that isn't the case. These two methods also exist on slices, and we still want to lint for cases like these:
let ve = vec![1, 2];
dbg!(ve.is_empty()); // Vec::is_empty, but this would also work fine if ve was `[1, 2]`There was a problem hiding this comment.
Ah, I misunderstood then 😅 I thought those were exceptions. Yeah, that sounds fine then.
| // Do lint | ||
| let mut x = vec![1, 2, 3]; | ||
| x.fill(123); | ||
| dbg!(x[0]); |
There was a problem hiding this comment.
One interesting case I just thought of is that changing a vec to an array can sometimes lead to compile errors when indexing with a constant that is out of bounds, because the compiler checks index expressions for arrays but not vecs:
let ve = vec![1, 2];
ve[3]; // ok (no compile error)
let arr = [1, 2];
arr[3]; // compile errorNot really sure what to do here. Accept it as a "known issue"? I doubt this is something that people run into by accident?
There was a problem hiding this comment.
"Known issue" is likely fine IMO, this would likely be helpful anyway
|
@bors r=Manishearth,Centri3 |
|
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
[`useless_vec`]: lint on `vec![_]` invocations that adjust to a slice Fixes #2262 (well, actually my PR over at #10901 did do most of the stuff, but this PR implements the one last other case mentioned in the comments that my PR didn't fix) Before this change, it would lint `(&vec![1]).iter().sum::<i32>()`, but not `vec![1].iter().sum::<i32>()`. This PR handles this case. This also refactors a few things that I wanted to do in my other PR but forgot about. changelog: [`useless_vec`]: lint on `vec![_]` invocations that adjust to a slice
First off, sorry for that large diff in tests. A lot of tests seem to trigger the lint with this new change, so I decided to
#![allow()]the lint in the affected tests to make reviewing this easier, and also split the commits up so that the first commit is the actual logic of the lint and the second commit contains all the test changes. The stuff that changed in the tests is mostly just line numbers now. So, as large as the diff looks, it's not actually that bad. 😅I manually went through all of these to find out about edge cases and decided to put them in
tests/ui/vec.rs.For more context, I wrote about the idea of this PR here: #2262 (comment) (that explains the logic)
Basically, it now also considers the case where a
Vecis put in a local variable and the user only ever does things with it that one could also do with a slice or an array. This should catch a lot more cases, and (at least from looking at the tests) it does.changelog: [
useless_vec]: lintvec!invocations when a slice or an array would do (also considering local variables now)