Skip to content
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

Add a lint for when ranges are used inside of a Vec #10932

Closed
orowith2os opened this issue Jun 12, 2023 · 8 comments · Fixed by #10934
Closed

Add a lint for when ranges are used inside of a Vec #10932

orowith2os opened this issue Jun 12, 2023 · 8 comments · Fixed by #10934
Assignees
Labels
A-lint Area: New lints

Comments

@orowith2os
Copy link

orowith2os commented Jun 12, 2023

What it does

When someone goes to put a range inside of a Vec, Clippy should probably check with them to make sure that's what they actually want to do. They may want to go over the range and collect that all into a Vec.

Advantage

  • New programmers to Rust (especially those using Rustlings) can get more information for why using a vec![0..100] doesn't work.
  • Prevents using the wrong type on accident inside of a Vec.

Drawbacks

None AFAIK, aside from needing to disable it for valid use cases.

Example

let a = [0..200];

You may want to make a Vec of integers, with the length specified here, not a Vec of ranges. Here are some potential solutions:

  • Initialize a Vec of length 200, with each value set to 0.
let a = vec![0; 200]
  • Initialize a Vec, containing the values from 1 to 200.
let a = (0..200).collect::<Vec<i32>>()

Alternatively, you may want to make an Array instead, with each value initialized to 0:

let a = [0..200]
@orowith2os orowith2os added the A-lint Area: New lints label Jun 12, 2023
@orowith2os
Copy link
Author

@zkldi @anden3 and @arvsrn helped me figure this out while I came across this issue on Rustlings (primitive_ types3), they may be able to improve this suggestion some.

And yes, I know this isn't how I was supposed to do the exercise, but now I found a lint that can be added to improve the UX :)

@Centri3
Copy link
Member

Centri3 commented Jun 12, 2023

There are many cases where a Vec<Range<T>> may be desired, but this is a very nice idea :) I think it should only lint on vec/arrays that are of size 1 since [0..200; 2] is almost certainly intentional

@rustbot claim

@zkrising
Copy link

similarly, [0..100, 2..40] would be intentional. It's pretty much entirely for the case of vec![0..100], which is misleading and should be replaced with 0..100.collect()

@tustvold
Copy link

I've run into this lint firing a fair amount for code that deals with Vec<Range<usize>>, I have found a workaround in the form of vec![0..2; 1] but I am a little bit confused by this lint. vec![0..2] has a different type from vec![0; 200], etc... so I'm a little confused about what this lint is providing beyond what is provided already by the type system?

@Centri3
Copy link
Member

Centri3 commented Aug 24, 2023

Its main use case is the demonstrative example, where there are no type annotations and no clear indication that it's a range. There are many cases where vec![0; 200] and vec![0..200] would both work. It's one of those lints that serves as a slight nudge in the right direction, rather than something that'd show up at all in production code.

@tustvold
Copy link

Surely this would become obvious once you came to use the Vec? I dunno, it feels odd to me that a lint would be enabled by default that is so susceptible to false positives?

@Centri3
Copy link
Member

Centri3 commented Aug 24, 2023

expr_use_ctxt is all we need to solve most of them, once I get around to it. If anyone wants to they can add a call to it; it should be a one line change (plus tests). The tests can be copied from #11088

The important part of expr_use_ctxt is the is_ty_unified field.

@Centri3
Copy link
Member

Centri3 commented Aug 24, 2023

Ah, this unfortunately got into stable already. For now just disable the lint in larger codebases, hopefully there will either be a coincidental point release or the fix will get into 1.73.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants