-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add lint to suggest as_chunks over chunks_exact with constant #16002
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
base: master
Are you sure you want to change the base?
Changes from 15 commits
3d3d2be
4a4c955
d332ec5
610acda
632e37b
bc1d010
1d99b91
2731771
7082eef
dc9fa9d
afaf92b
dfd8065
3cfa99a
94181ab
370d0ae
24798af
5e05b1d
ae2cb12
43a7a72
64aae82
83e0039
1e276d7
c50ccf8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,58 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use clippy_utils::consts::{ConstEvalCtxt, Constant}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use clippy_utils::diagnostics::span_lint_and_sugg; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use clippy_utils::msrvs::{self, Msrv}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use clippy_utils::source::snippet_with_applicability; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use clippy_utils::sym; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use rustc_errors::Applicability; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use rustc_hir::Expr; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use rustc_lint::LateContext; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use rustc_span::{Span, Symbol}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use super::CHUNKS_EXACT_WITH_CONST_SIZE; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub(super) fn check( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cx: &LateContext<'_>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| recv: &Expr<'_>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| arg: &Expr<'_>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| call_span: Span, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| method_name: Symbol, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| msrv: Msrv, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Check if receiver is slice-like | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if !cx.typeck_results().expr_ty_adjusted(recv).peel_refs().is_slice() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Check if argument is a constant | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let constant_eval = ConstEvalCtxt::new(cx); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if let Some(Constant::Int(_)) = constant_eval.eval(arg) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+27
to
+28
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You want |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Check for Rust version - only check after we know we would emit a lint | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if !msrv.meets(cx, msrvs::AS_CHUNKS) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Determine the suggested method name | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let suggestion_method = if method_name == sym::chunks_exact_mut { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "as_chunks_mut" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "as_chunks" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Build the suggestion with proper applicability tracking | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut applicability = Applicability::MachineApplicable; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let arg_str = snippet_with_applicability(cx, arg.span, "_", &mut applicability); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Suggestion replaces just "chunks_exact(N)" with "as_chunks::<N>().0.iter()" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let suggestion = format!("{suggestion_method}::<{arg_str}>().0.iter()"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| span_lint_and_sugg( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| cx, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| CHUNKS_EXACT_WITH_CONST_SIZE, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| call_span, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| format!("using `{method_name}` with a constant chunk size"), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "consider using `as_chunks` instead", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| suggestion, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| applicability, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Suggestion replaces just "chunks_exact(N)" with "as_chunks::<N>().0.iter()" | |
| let suggestion = format!("{suggestion_method}::<{arg_str}>().0.iter()"); | |
| span_lint_and_sugg( | |
| cx, | |
| CHUNKS_EXACT_WITH_CONST_SIZE, | |
| call_span, | |
| format!("using `{method_name}` with a constant chunk size"), | |
| "consider using `as_chunks` instead", | |
| suggestion, | |
| applicability, | |
| ); | |
| } | |
| let as_chunks = format_args!("{suggestion_method}::<{arg_str}>()"); | |
| span_lint_and_then( | |
| cx, | |
| CHUNKS_EXACT_WITH_CONST_SIZE, | |
| call_span, | |
| format!("using `{method_name}` with a constant chunk size"), | |
| |diag| { | |
| if let Node::LetStmt(_) = cx.tcx.parent_hir_node(expr.hir_id) { | |
| // The `ChunksExact(Mut)` struct is stored for later -- this likely means that the user intends to not only use it as an iterator, | |
| // but also access the remainder using `(into_)remainder`. For now, just give a help message in this case. | |
| // TODO: give a suggestion that replaces this: | |
| // ``` | |
| // let chunk_iter = bytes.chunks_exact(CHUNK_SIZE); | |
| // let remainder_chunk = chunk_iter.remainder(); | |
| // for chunk in chunk_iter { | |
| // /* ... */ | |
| // } | |
| // ``` | |
| // with this: | |
| // ``` | |
| // let chunk_iter = bytes.as_chunks::<CHUNK_SIZE>(); | |
| // let remainder_chunk = chunk_iter.1; | |
| // for chunk in chunk_iter.0.iter() { | |
| // /* ... */ | |
| // } | |
| // ``` | |
| diag.span_help( | |
| call_span, | |
| format!("consider using `{as_chunks}` instead"), | |
| ); | |
| } else { | |
| diag.span_suggestion( | |
| call_span, | |
| "consider using `as_chunks` instead", | |
| // Suggestion replaces just "chunks_exact(N)" with "as_chunks::<N>().0.iter()" | |
| format!("{as_chunks}.0.iter()"), | |
| applicability, | |
| ); | |
| } | |
| } | |
| ); | |
| } |
Footnotes
-
foreshadowing :P ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might get that totally wrong but isn't that in opposition to your comment above? Because I think it is useful to have the suggestion the possibility to access to the tuple.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I wouldn't want to suggest that in here is that, again, I think that if the user stores the ChunksExact in a variable, then they probably want to access the remainder -- and suggesting .0.iter() would go against that. If they do want to use the variable just as an iterator, they can of course add .0.iter() themselves, but that scenario would be rather rare I think.
In general, the return type of as_chunks is pretty self-explanatory imo -- it's a tuple, and a quick glance at the API docs tells you what each half stands for, so I imagine the user will be able to figure that out...
But if we do want to be extra helpful, we could emit an additional note: given the example code from above, it could look like this:
note: you can access the chunks using `chunk_iter.0.iter()`, and the remainder using `chunk_iter.1`
To do that, you'll need to:
- extract the
patfromLetStmt patis an arbitrary pattern, since the left-hand side of a let-statement could in theory be an arbitrary pattern (e.g. destructuring), but we can assume it's justPatKind::Binding. Therefore, match on that and extractident-- the identifier (=name) of the variable- construct the note message, and emit it using
diag.span_note(after emitting the help message)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done!:)
Oh, but there's a caveat I forgot about and you no doubt noticed – since in one of the test cases we now only give a help message and not a suggestion, ui_test (the UI test library we're using) will fail, as even after applying all the suggestions, the test file still raises warnings, precisely because help-only cases haven't actually changed.
Because of this, such test cases are placed into a separate file, usually called <lint_name>_unfixable.rs (there's also this //@no-rustfix thing you'll often see in such files, but you don't need to worry about that for now)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering why the tests were failing. Luckily, I do not get frustrated that easily might have taken me a couple of attempts before I would have noticed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 64aae82
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, could you please add the comment from the suggestion (of some version of it, as you please) to your code? Having a TODO will let an interested contributor pick it up in the future
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am going to use your comment as it is more helpful and provides transparency next possible steps.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| #![warn(clippy::chunks_exact_with_const_size)] | ||
| #![allow(unused)] | ||
| #![allow(clippy::iter_cloned_collect)] | ||
|
|
||
| fn main() { | ||
| let slice = [1, 2, 3, 4, 5, 6, 7, 8]; | ||
|
|
||
| // Should trigger lint - literal constant | ||
| let result = slice.as_chunks::<4>().0.iter(); | ||
| //~^ chunks_exact_with_const_size | ||
|
|
||
| // Should trigger lint - const value | ||
| const CHUNK_SIZE: usize = 4; | ||
| let result = slice.as_chunks::<CHUNK_SIZE>().0.iter(); | ||
| //~^ chunks_exact_with_const_size | ||
|
|
||
| // Should NOT trigger - runtime value | ||
| let size = 4; | ||
| let mut it = slice.chunks_exact(size); | ||
| for chunk in it {} | ||
|
|
||
| // Should trigger lint - simple iteration | ||
| let result = slice.as_chunks::<3>().0.iter(); | ||
| //~^ chunks_exact_with_const_size | ||
|
|
||
| // Should trigger - mutable variant | ||
| let mut arr = [1, 2, 3, 4, 5, 6, 7, 8]; | ||
| let result = arr.as_chunks_mut::<4>().0.iter(); | ||
| //~^ chunks_exact_with_const_size | ||
|
|
||
| // Should trigger - multiline expression | ||
| let result = slice.iter().copied().collect::<Vec<_>>().as_chunks::<2>().0.iter(); | ||
| //~^ chunks_exact_with_const_size | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| #![warn(clippy::chunks_exact_with_const_size)] | ||
| #![allow(unused)] | ||
| #![allow(clippy::iter_cloned_collect)] | ||
|
|
||
| fn main() { | ||
| let slice = [1, 2, 3, 4, 5, 6, 7, 8]; | ||
|
|
||
| // Should trigger lint - literal constant | ||
| let result = slice.chunks_exact(4); | ||
| //~^ chunks_exact_with_const_size | ||
|
|
||
| // Should trigger lint - const value | ||
| const CHUNK_SIZE: usize = 4; | ||
| let result = slice.chunks_exact(CHUNK_SIZE); | ||
| //~^ chunks_exact_with_const_size | ||
|
|
||
| // Should NOT trigger - runtime value | ||
| let size = 4; | ||
| let mut it = slice.chunks_exact(size); | ||
| for chunk in it {} | ||
|
|
||
| // Should trigger lint - simple iteration | ||
| let result = slice.chunks_exact(3); | ||
| //~^ chunks_exact_with_const_size | ||
|
|
||
| // Should trigger - mutable variant | ||
| let mut arr = [1, 2, 3, 4, 5, 6, 7, 8]; | ||
| let result = arr.chunks_exact_mut(4); | ||
| //~^ chunks_exact_with_const_size | ||
|
|
||
| // Should trigger - multiline expression | ||
| let result = slice.iter().copied().collect::<Vec<_>>().chunks_exact(2); | ||
| //~^ chunks_exact_with_const_size | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| error: using `chunks_exact` with a constant chunk size | ||
| --> tests/ui/chunks_exact_with_const_size.rs:9:24 | ||
| | | ||
| LL | let result = slice.chunks_exact(4); | ||
| | ^^^^^^^^^^^^^^^ help: consider using `as_chunks` instead: `as_chunks::<4>().0.iter()` | ||
| | | ||
| = note: `-D clippy::chunks-exact-with-const-size` implied by `-D warnings` | ||
| = help: to override `-D warnings` add `#[allow(clippy::chunks_exact_with_const_size)]` | ||
|
|
||
| error: using `chunks_exact` with a constant chunk size | ||
| --> tests/ui/chunks_exact_with_const_size.rs:14:24 | ||
| | | ||
| LL | let result = slice.chunks_exact(CHUNK_SIZE); | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using `as_chunks` instead: `as_chunks::<CHUNK_SIZE>().0.iter()` | ||
|
|
||
| error: using `chunks_exact` with a constant chunk size | ||
| --> tests/ui/chunks_exact_with_const_size.rs:23:24 | ||
| | | ||
| LL | let result = slice.chunks_exact(3); | ||
| | ^^^^^^^^^^^^^^^ help: consider using `as_chunks` instead: `as_chunks::<3>().0.iter()` | ||
|
|
||
| error: using `chunks_exact_mut` with a constant chunk size | ||
| --> tests/ui/chunks_exact_with_const_size.rs:28:22 | ||
| | | ||
| LL | let result = arr.chunks_exact_mut(4); | ||
| | ^^^^^^^^^^^^^^^^^^^ help: consider using `as_chunks` instead: `as_chunks_mut::<4>().0.iter()` | ||
|
|
||
| error: using `chunks_exact` with a constant chunk size | ||
| --> tests/ui/chunks_exact_with_const_size.rs:32:60 | ||
| | | ||
| LL | let result = slice.iter().copied().collect::<Vec<_>>().chunks_exact(2); | ||
| | ^^^^^^^^^^^^^^^ help: consider using `as_chunks` instead: `as_chunks::<2>().0.iter()` | ||
|
|
||
| error: aborting due to 5 previous errors | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't actually constrain the method correctly. You want to for a single reference to a slice.