Skip to content

Lint uses of Vec where arrays would do fine #2262

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

Closed
clarfonthey opened this issue Dec 6, 2017 · 3 comments · Fixed by #10933
Closed

Lint uses of Vec where arrays would do fine #2262

clarfonthey opened this issue Dec 6, 2017 · 3 comments · Fixed by #10933
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages E-medium Call for participation: Medium difficulty level problem and requires some initial experience. L-unnecessary Lint: Warn about unnecessary code T-middle Type: Probably requires verifiying types

Comments

@clarfonthey
Copy link

For example:

fn something_with_slice<T>(s: &[T]) { ... }

let vals = vec![1, 2];
something_with_slice(&vals);

Bonus points: suggest using ArrayVec when the size is statically guaranteed to be less than a certain amount.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 7, 2017

Don't we already have this? https://rust-lang-nursery.github.io/rust-clippy/master/index.html#useless_vec

Doesn't trigger in your case. So just an improvement to the existing lint required

@oli-obk oli-obk added L-unnecessary Lint: Warn about unnecessary code E-medium Call for participation: Medium difficulty level problem and requires some initial experience. C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages T-middle Type: Probably requires verifiying types labels Dec 7, 2017
@matthiaskrgr
Copy link
Member

It should also catch cases like vec![.....].iter() where we could use a simple array:

let x: i32 = vec![1,2,3].iter().sum();

@y21
Copy link
Member

y21 commented Jun 6, 2023

I've been experimenting with the idea of improving the already existing useless_vec lint a little bit and came up with this idea:

  • If a vec![_] macro invocation is found and the parent node is a local (i.e. let ve = vec![_]), then for each use of this local:
  • Check if:
    • (changing it to an array would not be too big for the stack, though this check is already covered by the lint)
    • the use expression adjusts to a slice (e.g. calling ve.chunks(16), ve adjusts to &[_])
    • the parent expression is an index expression (e.g. ve[5])
    • the parent expression is a method call and the method name is one of ["len", "as_ptr", "as_mut_ptr", "as_slice", "is_empty"]
      • as_slice would need some special handling, we would need to also suggest removing that call
  • If every use of the local passes at least one of these checks, the vec can (hopefully) be safely replaced with an array or a slice

This already catches a lot of cases in the tests alone (61) and it's going to take a while to go through all of them.

(Also, interestingly, the case @matthiaskrgr mentioned does not get caught, but changing it very slightly to immediately borrow the vec makes clippy catch it: playground)

bors added a commit that referenced this issue Jun 8, 2023
[`useless_vec`]: lint `vec!` invocations when a slice or an array would do

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 `Vec` is 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`]: lint `vec!` invocations when a slice or an array would do (also considering local variables now)
@bors bors closed this as completed in da56c35 Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Enhancement of lints, like adding more cases or adding help messages E-medium Call for participation: Medium difficulty level problem and requires some initial experience. L-unnecessary Lint: Warn about unnecessary code T-middle Type: Probably requires verifiying types
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants