-
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 5 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,50 @@ | ||||||||||||||||||||
| use clippy_utils::consts::{ConstEvalCtxt, Constant}; | ||||||||||||||||||||
| use clippy_utils::diagnostics::span_lint_and_help; | ||||||||||||||||||||
| use clippy_utils::msrvs::{self, Msrv}; | ||||||||||||||||||||
| use clippy_utils::source::snippet; | ||||||||||||||||||||
| use clippy_utils::sym; | ||||||||||||||||||||
| use rustc_hir::Expr; | ||||||||||||||||||||
| use rustc_lint::LateContext; | ||||||||||||||||||||
| use rustc_span::Symbol; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| use super::CHUNKS_EXACT_WITH_CONST_SIZE; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| pub(super) fn check( | ||||||||||||||||||||
| cx: &LateContext<'_>, | ||||||||||||||||||||
| expr: &Expr<'_>, | ||||||||||||||||||||
| recv: &Expr<'_>, | ||||||||||||||||||||
| arg: &Expr<'_>, | ||||||||||||||||||||
| method_name: Symbol, | ||||||||||||||||||||
| msrv: Msrv, | ||||||||||||||||||||
| ) { | ||||||||||||||||||||
| // Check for Rust version | ||||||||||||||||||||
| if !msrv.meets(cx, msrvs::AS_CHUNKS) { | ||||||||||||||||||||
| return; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // Check receiver is slice or array type | ||||||||||||||||||||
| let recv_ty = cx.typeck_results().expr_ty(recv).peel_refs(); | ||||||||||||||||||||
| if !recv_ty.is_slice() && !recv_ty.is_array() { | ||||||||||||||||||||
| return; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
||||||||||||||||||||
| // Check receiver is slice or array type | |
| let recv_ty = cx.typeck_results().expr_ty(recv).peel_refs(); | |
| if !recv_ty.is_slice() && !recv_ty.is_array() { | |
| return; | |
| } | |
| // Check if receiver is slice-like | |
| if !cx.typeck_results().expr_ty_adjusted(recv).peel_refs().is_slice() { | |
| return; | |
| } |
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 1d99b91
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.
You want is_const_evaluable here.
Outdated
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 could be pretty effortlessly switched over to give suggestions, which are quite nice to have. https://doc.rust-lang.org/clippy/development/emitting_lints.html#suggestions-automatic-fixes should help you in that, but don't hesitate to ask if something's unclear
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 7082eef
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.
Awesome, thank you!
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ mod chars_last_cmp; | |
| mod chars_last_cmp_with_unwrap; | ||
| mod chars_next_cmp; | ||
| mod chars_next_cmp_with_unwrap; | ||
| mod chunks_exact_with_const_size; | ||
| mod clear_with_drain; | ||
| mod clone_on_copy; | ||
| mod clone_on_ref_ptr; | ||
|
|
@@ -2087,6 +2088,32 @@ declare_clippy_lint! { | |
| "replace `.bytes().nth()` with `.as_bytes().get()`" | ||
| } | ||
|
|
||
| declare_clippy_lint! { | ||
| /// ### What it does | ||
| /// Checks for usage of `chunks_exact` or `chunks_exact_mut` with a constant chunk size. | ||
| /// | ||
| /// ### Why is this bad? | ||
| /// `as_chunks` provides better ergonomics and type safety by returning arrays instead of slices. | ||
| /// It was stabilized in Rust 1.88. | ||
| /// | ||
| /// ### Example | ||
| /// ```no_run | ||
| /// let slice = [1, 2, 3, 4, 5, 6]; | ||
| /// let mut it = slice.chunks_exact(2); | ||
| /// for chunk in it {} | ||
| /// ``` | ||
| /// Use instead: | ||
| /// ```no_run | ||
| /// let slice = [1, 2, 3, 4, 5, 6]; | ||
| /// let (chunks, remainder) = slice.as_chunks::<2>(); | ||
| /// for chunk in chunks {} | ||
| /// ``` | ||
| #[clippy::version = "1.93.0"] | ||
| pub CHUNKS_EXACT_WITH_CONST_SIZE, | ||
| style, | ||
| "using `chunks_exact` with constant when `as_chunks` is more ergonomic" | ||
| } | ||
|
|
||
| declare_clippy_lint! { | ||
| /// ### What it does | ||
| /// Checks for the usage of `_.to_owned()`, `vec.to_vec()`, or similar when calling `_.clone()` would be clearer. | ||
|
|
@@ -4787,6 +4814,7 @@ impl_lint_pass!(Methods => [ | |
| ITER_NTH, | ||
| ITER_NTH_ZERO, | ||
| BYTES_NTH, | ||
| CHUNKS_EXACT_WITH_CONST_SIZE, | ||
| ITER_SKIP_NEXT, | ||
| GET_UNWRAP, | ||
| GET_LAST_WITH_LEN, | ||
|
|
@@ -5715,6 +5743,9 @@ impl Methods { | |
| unwrap_expect_used::Variant::Unwrap, | ||
| ); | ||
| }, | ||
| (name @ (sym::chunks_exact | sym::chunks_exact_mut), [arg]) => { | ||
| chunks_exact_with_const_size::check(cx, expr, recv, arg, name, self.msrv); | ||
|
||
| }, | ||
| _ => {}, | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| #![warn(clippy::chunks_exact_with_const_size)] | ||
| #![allow(unused)] | ||
|
|
||
| fn main() { | ||
| let slice = [1, 2, 3, 4, 5, 6, 7, 8]; | ||
|
|
||
| // Should trigger lint - literal constant | ||
| let mut it = slice.chunks_exact(4); | ||
| //~^ ERROR: chunks_exact_with_const_size | ||
| for chunk in it {} | ||
|
|
||
| // Should trigger lint - const value | ||
| const CHUNK_SIZE: usize = 4; | ||
| let mut it = slice.chunks_exact(CHUNK_SIZE); | ||
| //~^ ERROR: chunks_exact_with_const_size | ||
| for chunk in it {} | ||
|
|
||
| // Should NOT trigger - runtime value | ||
| let size = 4; | ||
| let mut it = slice.chunks_exact(size); | ||
| for chunk in it {} | ||
|
|
||
| // Should trigger lint - with remainder | ||
| let mut it = slice.chunks_exact(3); | ||
| //~^ ERROR: chunks_exact_with_const_size | ||
| for chunk in &mut it {} | ||
| for e in it.remainder() {} | ||
|
|
||
| // Should trigger - mutable variant | ||
| let mut arr = [1, 2, 3, 4, 5, 6, 7, 8]; | ||
| let mut it = arr.chunks_exact_mut(4); | ||
| //~^ ERROR: chunks_exact_with_const_size | ||
| for chunk in it {} | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| error: chunks_exact_with_const_size | ||
| --> tests/ui/chunks_exact_with_const_size.rs:8:18 | ||
| | | ||
| LL | let mut it = slice.chunks_exact(4); | ||
| | ^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| = help: consider using `as_chunks::<4>()` for better ergonomics | ||
| = 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: chunks_exact_with_const_size | ||
| --> tests/ui/chunks_exact_with_const_size.rs:14:18 | ||
| | | ||
| LL | let mut it = slice.chunks_exact(CHUNK_SIZE); | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| = help: consider using `as_chunks::<CHUNK_SIZE>()` for better ergonomics | ||
|
|
||
| error: chunks_exact_with_const_size | ||
| --> tests/ui/chunks_exact_with_const_size.rs:24:18 | ||
| | | ||
| LL | let mut it = slice.chunks_exact(3); | ||
| | ^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| = help: consider using `as_chunks::<3>()` for better ergonomics | ||
|
|
||
| error: chunks_exact_with_const_size | ||
| --> tests/ui/chunks_exact_with_const_size.rs:31:18 | ||
| | | ||
| LL | let mut it = arr.chunks_exact_mut(4); | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^ | ||
| | | ||
| = help: consider using `as_chunks_mut::<4>()` for better ergonomics | ||
|
|
||
| error: aborting due to 4 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 check is somewhat expensive, so it's best to perform it towards the end, e.g. after the constant check
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 2731771
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.
Sorry, I meant after the constant check, but before the lint emission -- currently, the check doesn't actually do anything 😅
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