Auto merge of #8218 - Jarcho:redundant_slicing_deref, r=camsteffen

bors · bors · commit 668b3e47f90c · 2022-02-17T16:08:45.000Z
Improve `redundant_slicing` lint fixes #7972 fixes #7257 This can supersede #7976 changelog: Fix suggestion for `redundant_slicing` when re-borrowing for a method call changelog: New lint `deref_as_slicing`
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3105,6 +3105,7 @@ Released 2018-09-13
 [`deprecated_cfg_attr`]: https://rust-lang.github.io/rust-clippy/master/index.html#deprecated_cfg_attr
 [`deprecated_semver`]: https://rust-lang.github.io/rust-clippy/master/index.html#deprecated_semver
 [`deref_addrof`]: https://rust-lang.github.io/rust-clippy/master/index.html#deref_addrof
+[`deref_by_slicing`]: https://rust-lang.github.io/rust-clippy/master/index.html#deref_by_slicing
 [`derivable_impls`]: https://rust-lang.github.io/rust-clippy/master/index.html#derivable_impls
 [`derive_hash_xor_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#derive_hash_xor_eq
 [`derive_ord_xor_partial_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#derive_ord_xor_partial_ord
diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs
@@ -422,6 +422,7 @@ store.register_lints(&[
     redundant_else::REDUNDANT_ELSE,
     redundant_field_names::REDUNDANT_FIELD_NAMES,
     redundant_pub_crate::REDUNDANT_PUB_CRATE,
+    redundant_slicing::DEREF_BY_SLICING,
     redundant_slicing::REDUNDANT_SLICING,
     redundant_static_lifetimes::REDUNDANT_STATIC_LIFETIMES,
     ref_option_ref::REF_OPTION_REF,
diff --git a/clippy_lints/src/lib.register_restriction.rs b/clippy_lints/src/lib.register_restriction.rs
@@ -51,6 +51,7 @@ store.register_group(true, "clippy::restriction", Some("clippy_restriction"), ve
     LintId::of(panic_unimplemented::UNIMPLEMENTED),
     LintId::of(panic_unimplemented::UNREACHABLE),
     LintId::of(pattern_type_mismatch::PATTERN_TYPE_MISMATCH),
+    LintId::of(redundant_slicing::DEREF_BY_SLICING),
     LintId::of(same_name_method::SAME_NAME_METHOD),
     LintId::of(shadow::SHADOW_REUSE),
     LintId::of(shadow::SHADOW_SAME),
diff --git a/clippy_lints/src/redundant_slicing.rs b/clippy_lints/src/redundant_slicing.rs
@@ -1,11 +1,14 @@
 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_ast::util::parser::PREC_PREFIX;
 use rustc_errors::Applicability;
 use rustc_hir::{BorrowKind, Expr, ExprKind, LangItem, Mutability};
-use rustc_lint::{LateContext, LateLintPass};
+use rustc_lint::{LateContext, LateLintPass, Lint};
+use rustc_middle::ty::adjustment::{Adjust, AutoBorrow, AutoBorrowMutability};
+use rustc_middle::ty::subst::GenericArg;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 
 declare_clippy_lint! {
@@ -39,7 +42,34 @@ declare_clippy_lint! {
     "redundant slicing of the whole range of a type"
 }
 
-declare_lint_pass!(RedundantSlicing => [REDUNDANT_SLICING]);
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for slicing expressions which are equivalent to dereferencing the
+    /// value.
+    ///
+    /// ### Why is this bad?
+    /// Some people may prefer to dereference rather than slice.
+    ///
+    /// ### Example
+    /// ```rust
+    /// let vec = vec![1, 2, 3];
+    /// let slice = &vec[..];
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// let vec = vec![1, 2, 3];
+    /// let slice = &*vec;
+    /// ```
+    #[clippy::version = "1.60.0"]
+    pub DEREF_BY_SLICING,
+    restriction,
+    "slicing instead of dereferencing"
+}
+
+declare_lint_pass!(RedundantSlicing => [REDUNDANT_SLICING, DEREF_BY_SLICING]);
+
+static REDUNDANT_SLICING_LINT: (&Lint, &str) = (REDUNDANT_SLICING, "redundant slicing of the whole range");
+static DEREF_BY_SLICING_LINT: (&Lint, &str) = (DEREF_BY_SLICING, "slicing when dereferencing would work");
 
 impl<'tcx> LateLintPass<'tcx> for RedundantSlicing {
     fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
@@ -53,34 +83,84 @@ impl<'tcx> LateLintPass<'tcx> 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 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 parent_expr = get_parent_expr(cx, expr);
+                let needs_parens_for_prefix = parent_expr.map_or(false, |parent| {
+                    parent.precedence().order() > PREC_PREFIX
+                });
                 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 ((lint, msg), help, sugg) = if expr_ty == indexed_ty {
+                    if expr_ref_count > indexed_ref_count {
+                        // Indexing takes self by reference and can't return a reference to that
+                        // reference as it's a local variable. The only way this could happen is if
+                        // `self` contains a reference to the `Self` type. If this occurs then the
+                        // lint no longer applies as it's essentially a field access, which is not
+                        // redundant.
+                        return;
+                    }
+                    let deref_count = indexed_ref_count - expr_ref_count;
+
+                    let (lint, reborrow_str, help_str) = if mutability == Mutability::Mut {
+                        // The slice was used to reborrow the mutable reference.
+                        (DEREF_BY_SLICING_LINT, "&mut *", "reborrow the original value instead")
+                    } else if matches!(
+                        parent_expr,
+                        Some(Expr {
+                            kind: ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, _),
+                            ..
+                        })
+                    ) || cx.typeck_results().expr_adjustments(expr).first().map_or(false, |a| {
+                        matches!(a.kind, Adjust::Borrow(AutoBorrow::Ref(_, AutoBorrowMutability::Mut { .. })))
+                    }) {
+                        // The slice was used to make a temporary reference.
+                        (DEREF_BY_SLICING_LINT, "&*", "reborrow the original value instead")
+                    } else if deref_count != 0 {
+                        (DEREF_BY_SLICING_LINT, "", "dereference the original value instead")
+                    } else {
+                        (REDUNDANT_SLICING_LINT, "", "use the original value instead")
+                    };
+
+                    let snip = snippet_with_context(cx, indexed.span, ctxt, "..", &mut app).0;
+                    let sugg = if (deref_count != 0 || !reborrow_str.is_empty()) && needs_parens_for_prefix {
+                        format!("({}{}{})", reborrow_str, "*".repeat(deref_count), snip)
+                    } else {
+                        format!("{}{}{}", reborrow_str, "*".repeat(deref_count), snip)
+                    };
+
+                    (lint, help_str, sugg)
+                } 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 deref_ty == expr_ty {
+                            let snip = snippet_with_context(cx, indexed.span, ctxt, "..", &mut app).0;
+                            let sugg = if needs_parens_for_prefix {
+                                format!("(&{}{}*{})", mutability.prefix_str(), "*".repeat(indexed_ref_count), snip)
+                            } else {
+                                format!("&{}{}*{}", mutability.prefix_str(), "*".repeat(indexed_ref_count), snip)
+                            };
+                            (DEREF_BY_SLICING_LINT, "dereference the original value instead", sugg)
+                        } else {
+                            return;
+                        }
+                    } else {
+                        return;
+                    }
                 } else {
-                    ("", "use the original value instead")
+                    return;
                 };
 
                 span_lint_and_sugg(
                     cx,
-                    REDUNDANT_SLICING,
+                    lint,
                     expr.span,
-                    "redundant slicing of the whole range",
-                    help_str,
-                    format!("{}{}", reborrow_str, snip),
+                    msg,
+                    help,
+                    sugg,
                     app,
                 );
             }
diff --git a/tests/ui/deref_by_slicing.fixed b/tests/ui/deref_by_slicing.fixed
@@ -0,0 +1,29 @@
+// run-rustfix
+
+#![warn(clippy::deref_by_slicing)]
+
+use std::io::Read;
+
+fn main() {
+    let mut vec = vec![0];
+    let _ = &*vec;
+    let _ = &mut *vec;
+
+    let ref_vec = &mut vec;
+    let _ = &**ref_vec;
+    let mut_slice = &mut **ref_vec;
+    let _ = &mut *mut_slice; // Err, re-borrows slice
+
+    let s = String::new();
+    let _ = &*s;
+
+    static S: &[u8] = &[0, 1, 2];
+    let _ = &mut &*S; // Err, re-borrows slice
+
+    let slice: &[u32] = &[0u32, 1u32];
+    let slice_ref = &slice;
+    let _ = *slice_ref; // Err, derefs slice
+
+    let bytes: &[u8] = &[];
+    let _ = (&*bytes).read_to_end(&mut vec![]).unwrap(); // Err, re-borrows slice
+}
diff --git a/tests/ui/deref_by_slicing.rs b/tests/ui/deref_by_slicing.rs
@@ -0,0 +1,29 @@
+// run-rustfix
+
+#![warn(clippy::deref_by_slicing)]
+
+use std::io::Read;
+
+fn main() {
+    let mut vec = vec![0];
+    let _ = &vec[..];
+    let _ = &mut vec[..];
+
+    let ref_vec = &mut vec;
+    let _ = &ref_vec[..];
+    let mut_slice = &mut ref_vec[..];
+    let _ = &mut mut_slice[..]; // Err, re-borrows slice
+
+    let s = String::new();
+    let _ = &s[..];
+
+    static S: &[u8] = &[0, 1, 2];
+    let _ = &mut &S[..]; // Err, re-borrows slice
+
+    let slice: &[u32] = &[0u32, 1u32];
+    let slice_ref = &slice;
+    let _ = &slice_ref[..]; // Err, derefs slice
+
+    let bytes: &[u8] = &[];
+    let _ = (&bytes[..]).read_to_end(&mut vec![]).unwrap(); // Err, re-borrows slice
+}
diff --git a/tests/ui/deref_by_slicing.stderr b/tests/ui/deref_by_slicing.stderr
@@ -0,0 +1,58 @@
+error: slicing when dereferencing would work
+  --> $DIR/deref_by_slicing.rs:9:13
+   |
+LL |     let _ = &vec[..];
+   |             ^^^^^^^^ help: dereference the original value instead: `&*vec`
+   |
+   = note: `-D clippy::deref-by-slicing` implied by `-D warnings`
+
+error: slicing when dereferencing would work
+  --> $DIR/deref_by_slicing.rs:10:13
+   |
+LL |     let _ = &mut vec[..];
+   |             ^^^^^^^^^^^^ help: dereference the original value instead: `&mut *vec`
+
+error: slicing when dereferencing would work
+  --> $DIR/deref_by_slicing.rs:13:13
+   |
+LL |     let _ = &ref_vec[..];
+   |             ^^^^^^^^^^^^ help: dereference the original value instead: `&**ref_vec`
+
+error: slicing when dereferencing would work
+  --> $DIR/deref_by_slicing.rs:14:21
+   |
+LL |     let mut_slice = &mut ref_vec[..];
+   |                     ^^^^^^^^^^^^^^^^ help: dereference the original value instead: `&mut **ref_vec`
+
+error: slicing when dereferencing would work
+  --> $DIR/deref_by_slicing.rs:15:13
+   |
+LL |     let _ = &mut mut_slice[..]; // Err, re-borrows slice
+   |             ^^^^^^^^^^^^^^^^^^ help: reborrow the original value instead: `&mut *mut_slice`
+
+error: slicing when dereferencing would work
+  --> $DIR/deref_by_slicing.rs:18:13
+   |
+LL |     let _ = &s[..];
+   |             ^^^^^^ help: dereference the original value instead: `&*s`
+
+error: slicing when dereferencing would work
+  --> $DIR/deref_by_slicing.rs:21:18
+   |
+LL |     let _ = &mut &S[..]; // Err, re-borrows slice
+   |                  ^^^^^^ help: reborrow the original value instead: `&*S`
+
+error: slicing when dereferencing would work
+  --> $DIR/deref_by_slicing.rs:25:13
+   |
+LL |     let _ = &slice_ref[..]; // Err, derefs slice
+   |             ^^^^^^^^^^^^^^ help: dereference the original value instead: `*slice_ref`
+
+error: slicing when dereferencing would work
+  --> $DIR/deref_by_slicing.rs:28:13
+   |
+LL |     let _ = (&bytes[..]).read_to_end(&mut vec![]).unwrap(); // Err, re-borrows slice
+   |             ^^^^^^^^^^^^ help: reborrow the original value instead: `(&*bytes)`
+
+error: aborting due to 9 previous errors
+
diff --git a/tests/ui/redundant_slicing.fixed b/tests/ui/redundant_slicing.fixed
@@ -0,0 +1,46 @@
+// run-rustfix
+
+#![allow(unused, clippy::deref_by_slicing)]
+#![warn(clippy::redundant_slicing)]
+
+use std::io::Read;
+
+fn main() {
+    let slice: &[u32] = &[0];
+    let _ = slice; // Redundant slice
+
+    let v = vec![0];
+    let _ = &v[..]; // Ok, results in `&[_]`
+    let _ = (&*v); // Outer borrow is redundant
+
+    static S: &[u8] = &[0, 1, 2];
+    let _ = &mut &S[..]; // Ok, re-borrows slice
+
+    let mut vec = vec![0];
+    let mut_slice = &mut vec[..]; // Ok, results in `&mut [_]`
+    let _ = &mut mut_slice[..]; // Ok, re-borrows slice
+
+    let ref_vec = &vec;
+    let _ = &ref_vec[..]; // Ok, results in `&[_]`
+
+    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[..]; // Ok, derefs slice
+
+    // Issue #7972
+    let bytes: &[u8] = &[];
+    let _ = (&bytes[..]).read_to_end(&mut vec![]).unwrap(); // Ok, re-borrows slice
+}
diff --git a/tests/ui/redundant_slicing.rs b/tests/ui/redundant_slicing.rs
@@ -1,20 +1,27 @@
-#![allow(unused)]
+// run-rustfix
+
+#![allow(unused, clippy::deref_by_slicing)]
 #![warn(clippy::redundant_slicing)]
 
+use std::io::Read;
+
 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[..]; // Ok, results in `&[_]`
+    let _ = &(&*v)[..]; // Outer borrow is redundant
 
     static S: &[u8] = &[0, 1, 2];
-    let err = &mut &S[..]; // Should reborrow instead of slice
+    let _ = &mut &S[..]; // Ok, re-borrows slice
 
     let mut vec = vec![0];
-    let mut_slice = &mut *vec;
-    let _ = &mut mut_slice[..]; // Should reborrow instead of slice
+    let mut_slice = &mut vec[..]; // Ok, results in `&mut [_]`
+    let _ = &mut mut_slice[..]; // Ok, re-borrows slice
+
+    let ref_vec = &vec;
+    let _ = &ref_vec[..]; // Ok, results in `&[_]`
 
     macro_rules! m {
         ($e:expr) => {
@@ -29,4 +36,11 @@ fn main() {
         };
     }
     let _ = m2!(slice); // Don't lint in a macro
+
+    let slice_ref = &slice;
+    let _ = &slice_ref[..]; // Ok, derefs slice
+
+    // Issue #7972
+    let bytes: &[u8] = &[];
+    let _ = (&bytes[..]).read_to_end(&mut vec![]).unwrap(); // Ok, re-borrows slice
 }
diff --git a/tests/ui/redundant_slicing.stderr b/tests/ui/redundant_slicing.stderr
