Address review feedback

Author: xFrednet

clippy_lints/src/index_refutable_slice.rs

@@ -1,8 +1,8 @@
 use clippy_utils::consts::{constant, Constant};
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::higher::IfLet;
-use clippy_utils::ty::implements_trait;
-use clippy_utils::{in_macro, is_expn_of, is_lint_allowed, meets_msrv, msrvs, path_to_local};
+use clippy_utils::ty::is_copy;
+use clippy_utils::{is_expn_of, is_lint_allowed, meets_msrv, msrvs, path_to_local};
 use if_chain::if_chain;
 use rustc_data_structures::fx::{FxHashMap, FxHashSet};
 use rustc_errors::Applicability;
@@ -46,21 +46,22 @@ declare_clippy_lint! {
     ///     println!("{}", first);
     /// }
     /// ```
+    #[clippy::version = "1.58.0"]
     pub INDEX_REFUTABLE_SLICE,
     nursery,
     "avoid indexing on slices which could be destructed"
 }
 
 #[derive(Copy, Clone)]
 pub struct IndexRefutableSlice {
-    indexing_limit: u64,
+    max_suggested_slice: u64,
     msrv: Option<RustcVersion>,
 }
 
 impl IndexRefutableSlice {
     pub fn new(max_suggested_slice_pattern_length: u64, msrv: Option<RustcVersion>) -> Self {
         Self {
-            indexing_limit: max_suggested_slice_pattern_length - 1,
+            max_suggested_slice: max_suggested_slice_pattern_length,
             msrv,
         }
     }
@@ -71,14 +72,14 @@ impl_lint_pass!(IndexRefutableSlice => [INDEX_REFUTABLE_SLICE]);
 impl LateLintPass<'_> for IndexRefutableSlice {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
         if_chain! {
-            if !in_macro(expr.span) || is_expn_of(expr.span, "if_chain").is_some();
+            if !expr.span.from_expansion() || is_expn_of(expr.span, "if_chain").is_some();
             if let Some(IfLet {let_pat, if_then, ..}) = IfLet::hir(cx, expr);
             if !is_lint_allowed(cx, INDEX_REFUTABLE_SLICE, expr.hir_id);
             if meets_msrv(self.msrv.as_ref(), &msrvs::SLICE_PATTERNS);
 
             let found_slices = find_slice_values(cx, let_pat);
             if !found_slices.is_empty();
-            let filtered_slices = filter_lintable_slices(cx, found_slices, self.indexing_limit, if_then);
+            let filtered_slices = filter_lintable_slices(cx, found_slices, self.max_suggested_slice, if_then);
             if !filtered_slices.is_empty();
             then {
                 for slice in filtered_slices.values() {
@@ -117,12 +118,7 @@ fn find_slice_values(cx: &LateContext<'_>, pat: &hir::Pat<'_>) -> FxHashMap<hir:
                 // The values need to use the `ref` keyword if they can't be copied.
                 // This will need to be adjusted if the lint want to support multable access in the future
                 let src_is_ref = bound_ty.is_ref() && binding != hir::BindingAnnotation::Ref;
-                let is_copy = cx
-                    .tcx
-                    .lang_items()
-                    .copy_trait()
-                    .map_or(false, |trait_id| implements_trait(cx, inner_ty, trait_id, &[]));
-                let needs_ref = !(src_is_ref || is_copy);
+                let needs_ref = !(src_is_ref || is_copy(cx, inner_ty));
 
                 let slice_info = slices
                     .entry(value_hir_id)
@@ -183,10 +179,9 @@ fn lint_slice(cx: &LateContext<'_>, slice: &SliceLintInformation) {
                     Applicability::MaybeIncorrect,
                 );
 
-                // I thought about adding a note to the lint message to inform the user that these
-                // refactorings will remove the index expression. However, I decided against this,
-                // as `filter_lintable_slices` will only return slices where all access indices are
-                // known at compile time. The removal should therefore not have any side effects.
+                // The lint message doesn't contain a warning about the removed index expression,
+                // since `filter_lintable_slices` will only return slices where all access indices
+                // are known at compile time. Therefore, they can be removed without side effects.
             },
         );
     }
@@ -214,13 +209,13 @@ impl SliceLintInformation {
 fn filter_lintable_slices<'a, 'tcx>(
     cx: &'a LateContext<'tcx>,
     slice_lint_info: FxHashMap<hir::HirId, SliceLintInformation>,
-    index_limit: u64,
+    max_suggested_slice: u64,
     scope: &'tcx hir::Expr<'tcx>,
 ) -> FxHashMap<hir::HirId, SliceLintInformation> {
     let mut visitor = SliceIndexLintingVisitor {
         cx,
         slice_lint_info,
-        index_limit,
+        max_suggested_slice,
     };
 
     intravisit::walk_expr(&mut visitor, scope);
@@ -234,7 +229,7 @@ fn filter_lintable_slices<'a, 'tcx>(
 struct SliceIndexLintingVisitor<'a, 'tcx> {
     cx: &'a LateContext<'tcx>,
     slice_lint_info: FxHashMap<hir::HirId, SliceLintInformation>,
-    index_limit: u64,
+    max_suggested_slice: u64,
 }
 
 impl<'a, 'tcx> Visitor<'tcx> for SliceIndexLintingVisitor<'a, 'tcx> {
@@ -249,7 +244,7 @@ impl<'a, 'tcx> Visitor<'tcx> for SliceIndexLintingVisitor<'a, 'tcx> {
             let Self {
                 cx,
                 ref mut slice_lint_info,
-                index_limit,
+                max_suggested_slice,
             } = *self;
 
             if_chain! {
@@ -264,7 +259,7 @@ impl<'a, 'tcx> Visitor<'tcx> for SliceIndexLintingVisitor<'a, 'tcx> {
                 if let hir::ExprKind::Index(_, index_expr) = parent_expr.kind;
                 if let Some((Constant::Int(index_value), _)) = constant(cx, cx.typeck_results(), index_expr);
                 if let Ok(index_value) = index_value.try_into();
-                if index_value <= index_limit;
+                if index_value < max_suggested_slice;
 
                 // Make sure that this slice index is read only
                 let maybe_addrof_id = map.get_parent_node(parent_id);

clippy_lints/src/lib.rs

@@ -582,7 +582,12 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     store.register_late_pass(|| Box::new(size_of_in_element_count::SizeOfInElementCount));
     store.register_late_pass(|| Box::new(same_name_method::SameNameMethod));
     let max_suggested_slice_pattern_length = conf.max_suggested_slice_pattern_length;
-    store.register_late_pass(move || Box::new(index_refutable_slice::IndexRefutableSlice::new(max_suggested_slice_pattern_length, msrv)));
+    store.register_late_pass(move || {
+        Box::new(index_refutable_slice::IndexRefutableSlice::new(
+            max_suggested_slice_pattern_length,
+            msrv,
+        ))
+    });
     store.register_late_pass(|| Box::new(map_clone::MapClone));
     store.register_late_pass(|| Box::new(map_err_ignore::MapErrIgnore));
     store.register_late_pass(|| Box::new(shadow::Shadow::default()));

clippy_lints/src/utils/conf.rs

@@ -298,9 +298,9 @@ define_Conf! {
     (enable_raw_pointer_heuristic_for_send: bool = true),
     /// Lint: INDEX_REFUTABLE_SLICE.
     ///
-    /// The limit after which slice indexing will not be linted.
-    ///
-    /// As an example, the default limit of `3` would allow `slice[3]` but lint `slice[2]`.
+    /// When Clippy suggests using a slice pattern, this is the maximum number of elements allowed in
+    /// the slice pattern that is suggested. If more elements would be necessary, the lint is suppressed.
+    /// For example, `[_, _, _, e, ..]` is a slice pattern with 4 elements.
     (max_suggested_slice_pattern_length: u64 = 3),
 }
 

clippy_utils/src/ty.rs

@@ -19,6 +19,7 @@ use rustc_trait_selection::traits::query::normalize::AtExt;
 
 use crate::{match_def_path, must_use_attr};
 
+// Checks if the given type implements copy.
 pub fn is_copy<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
     ty.is_copy_modulo_regions(cx.tcx.at(DUMMY_SP), cx.param_env)
 }

tests/ui-toml/max_suggested_slice_pattern_length/avoidable_slice_indexing_limit.rs renamed to tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.rs

@@ -4,7 +4,7 @@ fn below_limit() {
     let slice: Option<&[u32]> = Some(&[1, 2, 3]);
     if let Some(slice) = slice {
         // This would usually not be linted but is included now due to the
-        // index limit in the config gile
+        // index limit in the config file
         println!("{}", slice[7]);
     }
 }

tests/ui-toml/max_suggested_slice_pattern_length/avoidable_slice_indexing_limit.stderr renamed to tests/ui-toml/max_suggested_slice_pattern_length/index_refutable_slice.stderr

@@ -1,11 +1,11 @@
 error: this binding can be a slice pattern to avoid indexing
-  --> $DIR/avoidable_slice_indexing_limit.rs:5:17
+  --> $DIR/index_refutable_slice.rs:5:17
    |
 LL |     if let Some(slice) = slice {
    |                 ^^^^^
    |
 note: the lint level is defined here
-  --> $DIR/avoidable_slice_indexing_limit.rs:1:9
+  --> $DIR/index_refutable_slice.rs:1:9
    |
 LL | #![deny(clippy::index_refutable_slice)]
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

tests/ui-toml/min_rust_version/min_rust_version.rs

@@ -59,8 +59,8 @@ fn manual_strip_msrv() {
     }
 }
 
-fn check_avoidable_slice_indexing() {
-    // This shouldn't trigger `clippy::avoidable_slice_indexing` as the suggestion
+fn check_index_refutable_slice() {
+    // This shouldn't trigger `clippy::index_refutable_slice` as the suggestion
     // would only be valid from 1.42.0 onward
     let slice: Option<&[u32]> = Some(&[1]);
     if let Some(slice) = slice {
@@ -74,5 +74,5 @@ fn main() {
     match_same_arms();
     match_same_arms2();
     manual_strip_msrv();
-    check_avoidable_slice_indexing();
+    check_index_refutable_slice();
 }