add manual_slice_fill lint

Author: lapla-cogito

CHANGELOG.md

@@ -5772,6 +5772,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

book/src/lint_configuration.md

@@ -751,6 +751,7 @@ The minimum rust version that the project supports. Defaults to the `rust-versio
 * [`manual_range_contains`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_range_contains)
 * [`manual_rem_euclid`](https://rust-lang.github.io/rust-clippy/master/index.html#manual_rem_euclid)
 * [`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)

clippy_config/src/conf.rs

@@ -620,6 +620,7 @@ define_Conf! {
         manual_range_contains,
         manual_rem_euclid,
         manual_retain,
+        manual_slice_fill,
         manual_split_once,
         manual_str_repeat,
         manual_strip,

clippy_lints/src/declared_lints.rs

@@ -291,6 +291,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,

clippy_lints/src/loops/manual_slice_fill.rs

@@ -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,
+    );
+}

clippy_lints/src/loops/mod.rs

@@ -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;
@@ -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,
@@ -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 {
@@ -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);
         }

tests/ui/manual_slice_fill.fixed

@@ -0,0 +1,101 @@
+#![warn(clippy::manual_slice_fill)]
+#![allow(clippy::needless_range_loop)]
+
+macro_rules! assign_element {
+    ($slice:ident, $index:expr) => {
+        $slice[$index] = 0;
+    };
+}
+
+macro_rules! assign_element_2 {
+    ($i:expr) => {
+        $i = 0;
+    };
+}
+
+struct NoClone;
+
+fn num() -> usize {
+    5
+}
+
+fn should_lint() {
+    let mut some_slice = [1, 2, 3, 4, 5];
+
+    some_slice.fill(0);
+
+    let x = 5;
+    some_slice.fill(x);
+
+    some_slice.fill(0);
+
+    // This should trigger `manual_slice_fill`, but the applicability is `MaybeIncorrect` since comments
+    // within the loop might be purely informational.
+    some_slice.fill(0);
+}
+
+fn should_not_lint() {
+    let mut some_slice = [1, 2, 3, 4, 5];
+
+    // Should not lint because we can't determine if the scope of the loop is intended to access all the
+    // elements of the slice.
+    for i in 0..5 {
+        some_slice[i] = 0;
+    }
+
+    // Should not lint, as using a function to assign values to elements might be
+    // intentional. For example, the contents of `num()` could be temporary and subject to change
+    // later.
+    for i in 0..some_slice.len() {
+        some_slice[i] = num();
+    }
+
+    // Should not lint because this loop isn't equivalent to `fill`.
+    for i in 0..some_slice.len() {
+        some_slice[i] = 0;
+        println!("foo");
+    }
+
+    // Should not lint because it may be intentional to use a macro to perform an operation equivalent
+    // to `fill`.
+    for i in 0..some_slice.len() {
+        assign_element!(some_slice, i);
+    }
+
+    let another_slice = [1, 2, 3];
+    // Should not lint because the range is not for `some_slice`.
+    for i in 0..another_slice.len() {
+        some_slice[i] = 0;
+    }
+
+    let mut vec: Vec<Option<NoClone>> = Vec::with_capacity(5);
+    // Should not lint because `NoClone` does not have `Clone` trait.
+    for i in 0..vec.len() {
+        vec[i] = None;
+    }
+
+    // Should not lint, as using a function to assign values to elements might be
+    // intentional. For example, the contents of `num()` could be temporary and subject to change
+    // later.
+    for i in &mut some_slice {
+        *i = num();
+    }
+
+    // Should not lint because this loop isn't equivalent to `fill`.
+    for i in &mut some_slice {
+        *i = 0;
+        println!("foo");
+    }
+
+    // Should not lint because it may be intentional to use a macro to perform an operation equivalent
+    // to `fill`.
+    for i in &mut some_slice {
+        assign_element_2!(*i);
+    }
+
+    let mut vec: Vec<Option<NoClone>> = Vec::with_capacity(5);
+    // Should not lint because `NoClone` does not have `Clone` trait.
+    for i in &mut vec {
+        *i = None;
+    }
+}