Skip to content

Commit

Permalink
add manual_slice_fill lint (rust-lang#14082)
Browse files Browse the repository at this point in the history
close rust-lang#13856

changelog: [`manual_slice_fill`]: new lint
  • Loading branch information
llogiq authored Feb 3, 2025
2 parents 6d1482c + 07ede9c commit 2c51951
Show file tree
Hide file tree
Showing 14 changed files with 433 additions and 35 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5773,6 +5773,7 @@ Released 2018-09-13
[`manual_retain`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_retain
[`manual_rotate`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_rotate
[`manual_saturating_arithmetic`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_saturating_arithmetic
[`manual_slice_fill`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_slice_fill
[`manual_slice_size_calculation`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_slice_size_calculation
[`manual_split_once`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_split_once
[`manual_str_repeat`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_str_repeat
Expand Down
1 change: 1 addition & 0 deletions book/src/lint_configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -752,6 +752,7 @@ The minimum rust version that the project supports. Defaults to the `rust-versio
* [`manual_rem_euclid`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_rem_euclid)
* [`manual_repeat_n`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_repeat_n)
* [`manual_retain`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_retain)
* [`manual_slice_fill`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_slice_fill)
* [`manual_split_once`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_split_once)
* [`manual_str_repeat`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_str_repeat)
* [`manual_strip`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_strip)
Expand Down
1 change: 1 addition & 0 deletions clippy_config/src/conf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -621,6 +621,7 @@ define_Conf! {
manual_rem_euclid,
manual_repeat_n,
manual_retain,
manual_slice_fill,
manual_split_once,
manual_str_repeat,
manual_strip,
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -292,6 +292,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
crate::loops::MANUAL_FIND_INFO,
crate::loops::MANUAL_FLATTEN_INFO,
crate::loops::MANUAL_MEMCPY_INFO,
crate::loops::MANUAL_SLICE_FILL_INFO,
crate::loops::MANUAL_WHILE_LET_SOME_INFO,
crate::loops::MISSING_SPIN_LOOP_INFO,
crate::loops::MUT_RANGE_BOUND_INFO,
Expand Down
111 changes: 111 additions & 0 deletions clippy_lints/src/loops/manual_slice_fill.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
use clippy_utils::diagnostics::span_lint_and_sugg;
use clippy_utils::eager_or_lazy::switch_to_eager_eval;
use clippy_utils::macros::span_is_local;
use clippy_utils::msrvs::{self, Msrv};
use clippy_utils::source::{HasSession, snippet_with_applicability};
use clippy_utils::ty::implements_trait;
use clippy_utils::{higher, peel_blocks_with_stmt, span_contains_comment};
use rustc_ast::ast::LitKind;
use rustc_ast::{RangeLimits, UnOp};
use rustc_data_structures::packed::Pu128;
use rustc_errors::Applicability;
use rustc_hir::QPath::Resolved;
use rustc_hir::def::Res;
use rustc_hir::{Expr, ExprKind, Pat};
use rustc_lint::LateContext;
use rustc_span::source_map::Spanned;
use rustc_span::sym;

use super::MANUAL_SLICE_FILL;

pub(super) fn check<'tcx>(
cx: &LateContext<'tcx>,
pat: &'tcx Pat<'_>,
arg: &'tcx Expr<'_>,
body: &'tcx Expr<'_>,
expr: &'tcx Expr<'_>,
msrv: &Msrv,
) {
if !msrv.meets(msrvs::SLICE_FILL) {
return;
}

// `for _ in 0..slice.len() { slice[_] = value; }`
if let Some(higher::Range {
start: Some(start),
end: Some(end),
limits: RangeLimits::HalfOpen,
}) = higher::Range::hir(arg)
&& let ExprKind::Lit(Spanned {
node: LitKind::Int(Pu128(0), _),
..
}) = start.kind
&& let ExprKind::Block(..) = body.kind
// Check if the body is an assignment to a slice element.
&& let ExprKind::Assign(assignee, assignval, _) = peel_blocks_with_stmt(body).kind
&& let ExprKind::Index(slice, _, _) = assignee.kind
// Check if `len()` is used for the range end.
&& let ExprKind::MethodCall(path, recv,..) = end.kind
&& path.ident.name == sym::len
// Check if the slice which is being assigned to is the same as the one being iterated over.
&& let ExprKind::Path(Resolved(_, recv_path)) = recv.kind
&& let ExprKind::Path(Resolved(_, slice_path)) = slice.kind
&& recv_path.res == slice_path.res
&& !assignval.span.from_expansion()
// It is generally not equivalent to use the `fill` method if `assignval` can have side effects
&& switch_to_eager_eval(cx, assignval)
&& span_is_local(assignval.span)
// The `fill` method requires that the slice's element type implements the `Clone` trait.
&& let Some(clone_trait) = cx.tcx.lang_items().clone_trait()
&& implements_trait(cx, cx.typeck_results().expr_ty(slice), clone_trait, &[])
{
sugg(cx, body, expr, slice.span, assignval.span);
}
// `for _ in &mut slice { *_ = value; }`
else if let ExprKind::AddrOf(_, _, recv) = arg.kind
// Check if the body is an assignment to a slice element.
&& let ExprKind::Assign(assignee, assignval, _) = peel_blocks_with_stmt(body).kind
&& let ExprKind::Unary(UnOp::Deref, slice_iter) = assignee.kind
&& let ExprKind::Path(Resolved(_, recv_path)) = recv.kind
// Check if the slice which is being assigned to is the same as the one being iterated over.
&& let ExprKind::Path(Resolved(_, slice_path)) = slice_iter.kind
&& let Res::Local(local) = slice_path.res
&& local == pat.hir_id
&& !assignval.span.from_expansion()
&& switch_to_eager_eval(cx, assignval)
&& span_is_local(assignval.span)
// The `fill` method cannot be used if the slice's element type does not implement the `Clone` trait.
&& let Some(clone_trait) = cx.tcx.lang_items().clone_trait()
&& implements_trait(cx, cx.typeck_results().expr_ty(recv), clone_trait, &[])
{
sugg(cx, body, expr, recv_path.span, assignval.span);
}
}

fn sugg<'tcx>(
cx: &LateContext<'tcx>,
body: &'tcx Expr<'_>,
expr: &'tcx Expr<'_>,
slice_span: rustc_span::Span,
assignval_span: rustc_span::Span,
) {
let mut app = if span_contains_comment(cx.sess().source_map(), body.span) {
Applicability::MaybeIncorrect // Comments may be informational.
} else {
Applicability::MachineApplicable
};

span_lint_and_sugg(
cx,
MANUAL_SLICE_FILL,
expr.span,
"manually filling a slice",
"try",
format!(
"{}.fill({});",
snippet_with_applicability(cx, slice_span, "..", &mut app),
snippet_with_applicability(cx, assignval_span, "..", &mut app),
),
app,
);
}
28 changes: 28 additions & 0 deletions clippy_lints/src/loops/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ mod iter_next_loop;
mod manual_find;
mod manual_flatten;
mod manual_memcpy;
mod manual_slice_fill;
mod manual_while_let_some;
mod missing_spin_loop;
mod mut_range_bound;
Expand Down Expand Up @@ -714,6 +715,31 @@ declare_clippy_lint! {
"possibly unintended infinite loop"
}

declare_clippy_lint! {
/// ### What it does
/// Checks for manually filling a slice with a value.
///
/// ### Why is this bad?
/// Using the `fill` method is more idiomatic and concise.
///
/// ### Example
/// ```no_run
/// let mut some_slice = [1, 2, 3, 4, 5];
/// for i in 0..some_slice.len() {
/// some_slice[i] = 0;
/// }
/// ```
/// Use instead:
/// ```no_run
/// let mut some_slice = [1, 2, 3, 4, 5];
/// some_slice.fill(0);
/// ```
#[clippy::version = "1.86.0"]
pub MANUAL_SLICE_FILL,
style,
"manually filling a slice with a value"
}

pub struct Loops {
msrv: Msrv,
enforce_iter_loop_reborrow: bool,
Expand Down Expand Up @@ -750,6 +776,7 @@ impl_lint_pass!(Loops => [
MANUAL_WHILE_LET_SOME,
UNUSED_ENUMERATE_INDEX,
INFINITE_LOOP,
MANUAL_SLICE_FILL,
]);

impl<'tcx> LateLintPass<'tcx> for Loops {
Expand Down Expand Up @@ -823,6 +850,7 @@ impl Loops {
) {
let is_manual_memcpy_triggered = manual_memcpy::check(cx, pat, arg, body, expr);
if !is_manual_memcpy_triggered {
manual_slice_fill::check(cx, pat, arg, body, expr, &self.msrv);
needless_range_loop::check(cx, pat, arg, body, expr);
explicit_counter_loop::check(cx, pat, arg, body, expr, label);
}
Expand Down
2 changes: 1 addition & 1 deletion clippy_utils/src/msrvs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ msrv_aliases! {
1,53,0 { OR_PATTERNS, MANUAL_BITS, BTREE_MAP_RETAIN, BTREE_SET_RETAIN, ARRAY_INTO_ITERATOR }
1,52,0 { STR_SPLIT_ONCE, REM_EUCLID_CONST }
1,51,0 { BORROW_AS_PTR, SEEK_FROM_CURRENT, UNSIGNED_ABS }
1,50,0 { BOOL_THEN, CLAMP }
1,50,0 { BOOL_THEN, CLAMP, SLICE_FILL }
1,47,0 { TAU, IS_ASCII_DIGIT_CONST, ARRAY_IMPL_ANY_LEN, SATURATING_SUB_CONST }
1,46,0 { CONST_IF_MATCH }
1,45,0 { STR_STRIP_PREFIX }
Expand Down
7 changes: 6 additions & 1 deletion tests/ui/manual_memcpy/without_loop_counters.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
#![warn(clippy::manual_memcpy)]
#![allow(clippy::assigning_clones, clippy::useless_vec, clippy::needless_range_loop)]
#![allow(
clippy::assigning_clones,
clippy::useless_vec,
clippy::needless_range_loop,
clippy::manual_slice_fill
)]

//@no-rustfix
const LOOP_OFFSET: usize = 5000;
Expand Down
36 changes: 18 additions & 18 deletions tests/ui/manual_memcpy/without_loop_counters.stderr
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: it looks like you're manually copying between slices
--> tests/ui/manual_memcpy/without_loop_counters.rs:9:5
--> tests/ui/manual_memcpy/without_loop_counters.rs:14:5
|
LL | / for i in 0..src.len() {
LL | |
Expand All @@ -12,7 +12,7 @@ LL | | }
= help: to override `-D warnings` add `#[allow(clippy::manual_memcpy)]`

error: it looks like you're manually copying between slices
--> tests/ui/manual_memcpy/without_loop_counters.rs:16:5
--> tests/ui/manual_memcpy/without_loop_counters.rs:21:5
|
LL | / for i in 0..src.len() {
LL | |
Expand All @@ -21,7 +21,7 @@ LL | | }
| |_____^ help: try replacing the loop by: `dst[10..(src.len() + 10)].copy_from_slice(&src[..]);`

error: it looks like you're manually copying between slices
--> tests/ui/manual_memcpy/without_loop_counters.rs:22:5
--> tests/ui/manual_memcpy/without_loop_counters.rs:27:5
|
LL | / for i in 0..src.len() {
LL | |
Expand All @@ -30,7 +30,7 @@ LL | | }
| |_____^ help: try replacing the loop by: `dst[..src.len()].copy_from_slice(&src[10..(src.len() + 10)]);`

error: it looks like you're manually copying between slices
--> tests/ui/manual_memcpy/without_loop_counters.rs:28:5
--> tests/ui/manual_memcpy/without_loop_counters.rs:33:5
|
LL | / for i in 11..src.len() {
LL | |
Expand All @@ -39,7 +39,7 @@ LL | | }
| |_____^ help: try replacing the loop by: `dst[11..src.len()].copy_from_slice(&src[(11 - 10)..(src.len() - 10)]);`

error: it looks like you're manually copying between slices
--> tests/ui/manual_memcpy/without_loop_counters.rs:34:5
--> tests/ui/manual_memcpy/without_loop_counters.rs:39:5
|
LL | / for i in 0..dst.len() {
LL | |
Expand All @@ -48,7 +48,7 @@ LL | | }
| |_____^ help: try replacing the loop by: `dst.copy_from_slice(&src[..dst.len()]);`

error: it looks like you're manually copying between slices
--> tests/ui/manual_memcpy/without_loop_counters.rs:48:5
--> tests/ui/manual_memcpy/without_loop_counters.rs:53:5
|
LL | / for i in 10..256 {
LL | |
Expand All @@ -64,7 +64,7 @@ LL + dst2[(10 + 500)..(256 + 500)].copy_from_slice(&src[10..256]);
|

error: it looks like you're manually copying between slices
--> tests/ui/manual_memcpy/without_loop_counters.rs:61:5
--> tests/ui/manual_memcpy/without_loop_counters.rs:66:5
|
LL | / for i in 10..LOOP_OFFSET {
LL | |
Expand All @@ -73,7 +73,7 @@ LL | | }
| |_____^ help: try replacing the loop by: `dst[(10 + LOOP_OFFSET)..(LOOP_OFFSET + LOOP_OFFSET)].copy_from_slice(&src[(10 - some_var)..(LOOP_OFFSET - some_var)]);`

error: it looks like you're manually copying between slices
--> tests/ui/manual_memcpy/without_loop_counters.rs:75:5
--> tests/ui/manual_memcpy/without_loop_counters.rs:80:5
|
LL | / for i in 0..src_vec.len() {
LL | |
Expand All @@ -82,7 +82,7 @@ LL | | }
| |_____^ help: try replacing the loop by: `dst_vec[..src_vec.len()].copy_from_slice(&src_vec[..]);`

error: it looks like you're manually copying between slices
--> tests/ui/manual_memcpy/without_loop_counters.rs:105:5
--> tests/ui/manual_memcpy/without_loop_counters.rs:110:5
|
LL | / for i in from..from + src.len() {
LL | |
Expand All @@ -91,7 +91,7 @@ LL | | }
| |_____^ help: try replacing the loop by: `dst[from..(from + src.len())].copy_from_slice(&src[..(from + src.len() - from)]);`

error: it looks like you're manually copying between slices
--> tests/ui/manual_memcpy/without_loop_counters.rs:110:5
--> tests/ui/manual_memcpy/without_loop_counters.rs:115:5
|
LL | / for i in from..from + 3 {
LL | |
Expand All @@ -100,7 +100,7 @@ LL | | }
| |_____^ help: try replacing the loop by: `dst[from..(from + 3)].copy_from_slice(&src[..(from + 3 - from)]);`

error: it looks like you're manually copying between slices
--> tests/ui/manual_memcpy/without_loop_counters.rs:116:5
--> tests/ui/manual_memcpy/without_loop_counters.rs:121:5
|
LL | / for i in 0..5 {
LL | |
Expand All @@ -109,7 +109,7 @@ LL | | }
| |_____^ help: try replacing the loop by: `dst[..5].copy_from_slice(&src);`

error: it looks like you're manually copying between slices
--> tests/ui/manual_memcpy/without_loop_counters.rs:122:5
--> tests/ui/manual_memcpy/without_loop_counters.rs:127:5
|
LL | / for i in 0..0 {
LL | |
Expand All @@ -118,7 +118,7 @@ LL | | }
| |_____^ help: try replacing the loop by: `dst[..0].copy_from_slice(&src[..0]);`

error: it looks like you're manually copying between slices
--> tests/ui/manual_memcpy/without_loop_counters.rs:146:5
--> tests/ui/manual_memcpy/without_loop_counters.rs:151:5
|
LL | / for i in 0..4 {
LL | |
Expand All @@ -127,7 +127,7 @@ LL | | }
| |_____^ help: try replacing the loop by: `dst.copy_from_slice(&src[..4]);`

error: it looks like you're manually copying between slices
--> tests/ui/manual_memcpy/without_loop_counters.rs:152:5
--> tests/ui/manual_memcpy/without_loop_counters.rs:157:5
|
LL | / for i in 0..5 {
LL | |
Expand All @@ -136,7 +136,7 @@ LL | | }
| |_____^ help: try replacing the loop by: `dst[..5].copy_from_slice(&src);`

error: it looks like you're manually copying between slices
--> tests/ui/manual_memcpy/without_loop_counters.rs:158:5
--> tests/ui/manual_memcpy/without_loop_counters.rs:163:5
|
LL | / for i in 0..5 {
LL | |
Expand All @@ -145,7 +145,7 @@ LL | | }
| |_____^ help: try replacing the loop by: `dst.copy_from_slice(&src);`

error: it looks like you're manually copying between slices
--> tests/ui/manual_memcpy/without_loop_counters.rs:205:5
--> tests/ui/manual_memcpy/without_loop_counters.rs:210:5
|
LL | / for i in 0..5 {
LL | |
Expand All @@ -154,7 +154,7 @@ LL | | }
| |_____^ help: try replacing the loop by: `dst.copy_from_slice(&src[0]);`

error: it looks like you're manually copying between slices
--> tests/ui/manual_memcpy/without_loop_counters.rs:211:5
--> tests/ui/manual_memcpy/without_loop_counters.rs:216:5
|
LL | / for i in 0..5 {
LL | |
Expand All @@ -163,7 +163,7 @@ LL | | }
| |_____^ help: try replacing the loop by: `dst.copy_from_slice(&src[0][1]);`

error: it looks like you're manually copying between slices
--> tests/ui/manual_memcpy/without_loop_counters.rs:219:5
--> tests/ui/manual_memcpy/without_loop_counters.rs:224:5
|
LL | / for i in 0..src.len() {
LL | |
Expand Down
Loading

0 comments on commit 2c51951

Please sign in to comment.