Improve redundant_slicing lint

Jarcho · Jarcho · commit c242ef2a79ca · 2022-01-03T15:57:25.000-05:00
* Lint when slicing triggers auto-deref
* Lint when slicing returns the same type as dereferencing
diff --git a/clippy_lints/src/redundant_slicing.rs b/clippy_lints/src/redundant_slicing.rs
@@ -1,12 +1,12 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::get_parent_expr;
 use clippy_utils::source::snippet_with_context;
-use clippy_utils::ty::is_type_lang_item;
+use clippy_utils::ty::{is_type_lang_item, peel_mid_ty_refs};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir::{BorrowKind, Expr, ExprKind, LangItem, Mutability};
 use rustc_lint::{LateContext, LateLintPass};
-use rustc_middle::ty::TyS;
+use rustc_middle::ty::{subst::GenericArg, TyS};
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 
 declare_clippy_lint! {
@@ -54,34 +54,64 @@ impl LateLintPass<'_> for RedundantSlicing {
             if addressee.span.ctxt() == ctxt;
             if let ExprKind::Index(indexed, range) = addressee.kind;
             if is_type_lang_item(cx, cx.typeck_results().expr_ty_adjusted(range), LangItem::RangeFull);
-            if TyS::same_type(cx.typeck_results().expr_ty(expr), cx.typeck_results().expr_ty(indexed));
             then {
+                let (expr_ty, expr_ref_count) = peel_mid_ty_refs(cx.typeck_results().expr_ty(expr));
+                let (indexed_ty, indexed_ref_count) = peel_mid_ty_refs(cx.typeck_results().expr_ty(indexed));
                 let mut app = Applicability::MachineApplicable;
-                let snip = snippet_with_context(cx, indexed.span, ctxt, "..", &mut app).0;
 
-                let (reborrow_str, help_str) = if mutability == Mutability::Mut {
-                    // The slice was used to reborrow the mutable reference.
-                    ("&mut *", "reborrow the original value instead")
-                } else if matches!(
-                    get_parent_expr(cx, expr),
-                    Some(Expr {
-                        kind: ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, _),
-                        ..
-                    })
-                ) {
-                    // The slice was used to make a temporary reference.
-                    ("&*", "reborrow the original value instead")
+                let (help, sugg) = if TyS::same_type(expr_ty, indexed_ty) {
+                    if expr_ref_count > indexed_ref_count {
+                        return;
+                    }
+
+                    let (reborrow_str, help_str) = if mutability == Mutability::Mut {
+                        // The slice was used to reborrow the mutable reference.
+                        ("&mut *", "reborrow the original value instead")
+                    } else if matches!(
+                        get_parent_expr(cx, expr),
+                        Some(Expr {
+                            kind: ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, _),
+                            ..
+                        })
+                    ) {
+                        // The slice was used to make a temporary reference.
+                        ("&*", "reborrow the original value instead")
+                    } else if expr_ref_count != indexed_ref_count {
+                        ("", "dereference the original value instead")
+                    } else {
+                        ("", "use the original value instead")
+                    };
+
+                    let snip = snippet_with_context(cx, indexed.span, ctxt, "..", &mut app).0;
+                    (help_str, format!("{}{}{}", reborrow_str, "*".repeat(indexed_ref_count - expr_ref_count), snip))
+                } else if let Some(target_id) = cx.tcx.lang_items().deref_target() {
+                    if let Ok(deref_ty) = cx.tcx.try_normalize_erasing_regions(
+                        cx.param_env,
+                        cx.tcx.mk_projection(target_id, cx.tcx.mk_substs([GenericArg::from(indexed_ty)].into_iter())),
+                    ) {
+                        if TyS::same_type(deref_ty, expr_ty) {
+                            let snip = snippet_with_context(cx, indexed.span, ctxt, "..", &mut app).0;
+                            (
+                                "dereference the original value instead",
+                                format!("&{}{}*{}", mutability.prefix_str(), "*".repeat(indexed_ref_count), snip),
+                            )
+                        } else {
+                            return;
+                        }
+                    } else {
+                        return;
+                    }
                 } else {
-                    ("", "use the original value instead")
+                    return;
                 };
 
                 span_lint_and_sugg(
                     cx,
                     REDUNDANT_SLICING,
                     expr.span,
                     "redundant slicing of the whole range",
-                    help_str,
-                    format!("{}{}", reborrow_str, snip),
+                    help,
+                    sugg,
                     app,
                 );
             }
diff --git a/tests/ui/bytecount.rs b/tests/ui/bytecount.rs
@@ -4,7 +4,7 @@ fn main() {
 
     let _ = x.iter().filter(|&&a| a == 0).count(); // naive byte count
 
-    let _ = (&x[..]).iter().filter(|&a| *a == 0).count(); // naive byte count
+    let _ = (&*x).iter().filter(|&a| *a == 0).count(); // naive byte count
 
     let _ = x.iter().filter(|a| **a > 0).count(); // not an equality count, OK.
 
diff --git a/tests/ui/bytecount.stderr b/tests/ui/bytecount.stderr
@@ -13,8 +13,8 @@ LL | #[deny(clippy::naive_bytecount)]
 error: you appear to be counting bytes the naive way
   --> $DIR/bytecount.rs:7:13
    |
-LL |     let _ = (&x[..]).iter().filter(|&a| *a == 0).count(); // naive byte count
-   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using the bytecount crate: `bytecount::count((&x[..]), 0)`
+LL |     let _ = (&*x).iter().filter(|&a| *a == 0).count(); // naive byte count
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using the bytecount crate: `bytecount::count((&*x), 0)`
 
 error: you appear to be counting bytes the naive way
   --> $DIR/bytecount.rs:19:13
diff --git a/tests/ui/indexing_slicing_slice.rs b/tests/ui/indexing_slicing_slice.rs
@@ -2,7 +2,7 @@
 // We also check the out_of_bounds_indexing lint here, because it lints similar things and
 // we want to avoid false positives.
 #![warn(clippy::out_of_bounds_indexing)]
-#![allow(clippy::no_effect, clippy::unnecessary_operation)]
+#![allow(clippy::no_effect, clippy::unnecessary_operation, clippy::redundant_slicing)]
 
 fn main() {
     let x = [1, 2, 3, 4];
diff --git a/tests/ui/redundant_slicing.fixed b/tests/ui/redundant_slicing.fixed
@@ -0,0 +1,40 @@
+// run-rustfix
+
+#![allow(unused)]
+#![warn(clippy::redundant_slicing)]
+
+fn main() {
+    let slice: &[u32] = &[0];
+    let _ = slice; // Redundant slice
+
+    let v = vec![0];
+    let _ = &*v; // Deref instead of slice
+    let _ = (&*v); // Outer borrow is redundant
+
+    static S: &[u8] = &[0, 1, 2];
+    let err = &mut &*S; // Should reborrow instead of slice
+
+    let mut vec = vec![0];
+    let mut_slice = &mut *vec; // Deref instead of slice
+    let _ = &mut *mut_slice; // Should reborrow instead of slice
+
+    let ref_vec = &vec;
+    let _ = &**ref_vec; // Deref instead of slice
+
+    macro_rules! m {
+        ($e:expr) => {
+            $e
+        };
+    }
+    let _ = slice;
+
+    macro_rules! m2 {
+        ($e:expr) => {
+            &$e[..]
+        };
+    }
+    let _ = m2!(slice); // Don't lint in a macro
+
+    let slice_ref = &slice;
+    let _ = *slice_ref; // Deref instead of slice
+}
diff --git a/tests/ui/redundant_slicing.rs b/tests/ui/redundant_slicing.rs
@@ -1,21 +1,26 @@
+// run-rustfix
+
 #![allow(unused)]
 #![warn(clippy::redundant_slicing)]
 
 fn main() {
     let slice: &[u32] = &[0];
-    let _ = &slice[..];
+    let _ = &slice[..]; // Redundant slice
 
     let v = vec![0];
-    let _ = &v[..]; // Changes the type
-    let _ = &(&v[..])[..]; // Outer borrow is redundant
+    let _ = &v[..]; // Deref instead of slice
+    let _ = &(&*v)[..]; // Outer borrow is redundant
 
     static S: &[u8] = &[0, 1, 2];
     let err = &mut &S[..]; // Should reborrow instead of slice
 
     let mut vec = vec![0];
-    let mut_slice = &mut *vec;
+    let mut_slice = &mut vec[..]; // Deref instead of slice
     let _ = &mut mut_slice[..]; // Should reborrow instead of slice
 
+    let ref_vec = &vec;
+    let _ = &ref_vec[..]; // Deref instead of slice
+
     macro_rules! m {
         ($e:expr) => {
             $e
@@ -29,4 +34,7 @@ fn main() {
         };
     }
     let _ = m2!(slice); // Don't lint in a macro
+
+    let slice_ref = &slice;
+    let _ = &slice_ref[..]; // Deref instead of slice
 }
diff --git a/tests/ui/redundant_slicing.stderr b/tests/ui/redundant_slicing.stderr
@@ -1,34 +1,58 @@
 error: redundant slicing of the whole range
-  --> $DIR/redundant_slicing.rs:6:13
+  --> $DIR/redundant_slicing.rs:8:13
    |
-LL |     let _ = &slice[..];
+LL |     let _ = &slice[..]; // Redundant slice
    |             ^^^^^^^^^^ help: use the original value instead: `slice`
    |
    = note: `-D clippy::redundant-slicing` implied by `-D warnings`
 
 error: redundant slicing of the whole range
-  --> $DIR/redundant_slicing.rs:10:13
+  --> $DIR/redundant_slicing.rs:11:13
    |
-LL |     let _ = &(&v[..])[..]; // Outer borrow is redundant
-   |             ^^^^^^^^^^^^^ help: use the original value instead: `(&v[..])`
+LL |     let _ = &v[..]; // Deref instead of slice
+   |             ^^^^^^ help: dereference the original value instead: `&*v`
 
 error: redundant slicing of the whole range
-  --> $DIR/redundant_slicing.rs:13:20
+  --> $DIR/redundant_slicing.rs:12:13
+   |
+LL |     let _ = &(&*v)[..]; // Outer borrow is redundant
+   |             ^^^^^^^^^^ help: use the original value instead: `(&*v)`
+
+error: redundant slicing of the whole range
+  --> $DIR/redundant_slicing.rs:15:20
    |
 LL |     let err = &mut &S[..]; // Should reborrow instead of slice
    |                    ^^^^^^ help: reborrow the original value instead: `&*S`
 
 error: redundant slicing of the whole range
-  --> $DIR/redundant_slicing.rs:17:13
+  --> $DIR/redundant_slicing.rs:18:21
+   |
+LL |     let mut_slice = &mut vec[..]; // Deref instead of slice
+   |                     ^^^^^^^^^^^^ help: dereference the original value instead: `&mut *vec`
+
+error: redundant slicing of the whole range
+  --> $DIR/redundant_slicing.rs:19:13
    |
 LL |     let _ = &mut mut_slice[..]; // Should reborrow instead of slice
    |             ^^^^^^^^^^^^^^^^^^ help: reborrow the original value instead: `&mut *mut_slice`
 
 error: redundant slicing of the whole range
-  --> $DIR/redundant_slicing.rs:24:13
+  --> $DIR/redundant_slicing.rs:22:13
+   |
+LL |     let _ = &ref_vec[..]; // Deref instead of slice
+   |             ^^^^^^^^^^^^ help: dereference the original value instead: `&**ref_vec`
+
+error: redundant slicing of the whole range
+  --> $DIR/redundant_slicing.rs:29:13
    |
 LL |     let _ = &m!(slice)[..];
    |             ^^^^^^^^^^^^^^ help: use the original value instead: `slice`
 
-error: aborting due to 5 previous errors
+error: redundant slicing of the whole range
+  --> $DIR/redundant_slicing.rs:39:13
+   |
+LL |     let _ = &slice_ref[..]; // Deref instead of slice
+   |             ^^^^^^^^^^^^^^ help: dereference the original value instead: `*slice_ref`
+
+error: aborting due to 9 previous errors
 
diff --git a/tests/ui/str_to_string.rs b/tests/ui/str_to_string.rs
@@ -2,6 +2,6 @@
 
 fn main() {
     let hello = "hello world".to_string();
-    let msg = &hello[..];
+    let msg = &*hello;
     msg.to_string();
 }

Original file line number	Diff line number	Diff line change
`@@ -2,6 +2,6 @@`
`2`	`2`
`3`	`3`	`fn main() {`
`4`	`4`	`let hello = "hello world".to_string();`
`5`		`- let msg = &hello[..];`
	`5`	`+ let msg = &*hello;`
`6`	`6`	`msg.to_string();`
`7`	`7`	`}`