Fix redundant_slicing when the result is used as the reciever in a method call.

Jarcho · Jarcho · commit 1b67c0b621f0 · 2021-11-14T09:15:48.000-05:00
diff --git a/clippy_lints/src/redundant_slicing.rs b/clippy_lints/src/redundant_slicing.rs
@@ -1,11 +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::source::{position_of_expr, snippet_expr, ExprPosition};
 use clippy_utils::ty::is_type_lang_item;
 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::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability};
 use rustc_middle::ty::TyS;
 use rustc_session::{declare_lint_pass, declare_tool_lint};
 
@@ -57,34 +58,47 @@ impl LateLintPass<'_> for RedundantSlicing {
             if TyS::same_type(cx.typeck_results().expr_ty(expr), cx.typeck_results().expr_ty(indexed));
             then {
                 let mut app = Applicability::MachineApplicable;
-                let snip = snippet_with_context(cx, indexed.span, ctxt, "..", &mut app).0;
+                let position = position_of_expr(cx, expr);
 
-                let (reborrow_str, help_str) = if mutability == Mutability::Mut {
+                let (reborrow_str, help_str, snip_position) = if mutability == Mutability::Mut {
                     // The slice was used to reborrow the mutable reference.
-                    ("&mut *", "reborrow the original value instead")
+                    ("&mut *", "reborrow the original value instead", ExprPosition::Prefix)
                 } else if matches!(
                     get_parent_expr(cx, expr),
                     Some(Expr {
                         kind: ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, _),
                         ..
                     })
+                ) || matches!(
+                    cx.typeck_results().expr_adjustments(expr),
+                    [Adjustment {
+                        kind: Adjust::Borrow(AutoBorrow::Ref(_, AutoBorrowMutability::Mut { .. })),
+                        ..
+                    }]
                 ) {
                     // The slice was used to make a temporary reference.
-                    ("&*", "reborrow the original value instead")
+                    ("&*", "reborrow the original value instead", ExprPosition::Prefix)
                 } else {
-                    ("", "use the original value instead")
+                    ("", "use the original value instead", position)
                 };
 
+                let snip = snippet_expr(cx, indexed, snip_position, ctxt, &mut app);
+
                 span_lint_and_sugg(
                     cx,
                     REDUNDANT_SLICING,
                     expr.span,
                     "redundant slicing of the whole range",
                     help_str,
-                    format!("{}{}", reborrow_str, snip),
+                    if reborrow_str != "" && position > ExprPosition::Prefix {
+                        format!("({}{})", reborrow_str, snip)
+                    } else {
+                        format!("{}{}", reborrow_str, snip)
+                    },
                     app,
                 );
             }
+
         }
     }
 }
diff --git a/clippy_utils/src/source.rs b/clippy_utils/src/source.rs
@@ -2,9 +2,9 @@
 
 #![allow(clippy::module_name_repetitions)]
 
-use crate::line_span;
+use crate::{get_parent_expr, line_span};
 use rustc_errors::Applicability;
-use rustc_hir::{Expr, ExprKind};
+use rustc_hir::{BinOpKind, Expr, ExprKind, MatchSource};
 use rustc_lint::{LateContext, LintContext};
 use rustc_span::hygiene;
 use rustc_span::{BytePos, Pos, Span, SyntaxContext};
@@ -306,6 +306,199 @@ pub fn snippet_with_context(
     )
 }
 
+#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+pub enum ExprPosition {
+    // Also includes `return`, `yield`, `break` and closures
+    Paren,
+    AssignmentRhs,
+    AssignmentLhs,
+    RangeLhs,
+    RangeRhs,
+    OrLhs,
+    OrRhs,
+    AndLhs,
+    AndRhs,
+    Let,
+    EqLhs,
+    EqRhs,
+    BitOrLhs,
+    BitOrRhs,
+    BitXorLhs,
+    BitXorRhs,
+    BitAndLhs,
+    BitAndRhs,
+    ShiftLhs,
+    ShiftRhs,
+    AddLhs,
+    AddRhs,
+    MulLhs,
+    MulRhs,
+    // Also includes type ascription
+    Cast,
+    Prefix,
+    Postfix,
+}
+
+/// Extracts a snippet of the given expression taking into account the `SyntaxContext` the snippet
+/// needs to be taken from. Parenthesis will be added if needed to place the snippet in the target
+/// precedence level. Returns a placeholder (`(..)`) if a snippet can't be extracted (e.g. an
+/// invalid span).
+///
+/// The `SyntaxContext` of the expression will be walked up to the given target context (usually
+/// from the parent expression) before extracting a snippet. This allows getting the call to a macro
+/// rather than the expression from expanding the macro. e.g. In the expression `&vec![]` taking a
+/// snippet of the chile of the borrow expression will get a snippet of what `vec![]` expands in to.
+/// With the target context set to the same as the borrow expression, this will get a snippet of the
+/// call to the macro.
+///
+/// The applicability will be modified in two ways:
+/// * If a snippet can't be extracted it will be changed from `MachineApplicable` or
+///   `MaybeIncorrect` to `HasPlaceholders`.
+/// * If the snippet is taken from a macro expansion then it will be changed from
+///   `MachineApplicable` to `MaybeIncorrect`.
+pub fn snippet_expr(
+    cx: &LateContext<'_>,
+    expr: &Expr<'_>,
+    target_position: ExprPosition,
+    ctxt: SyntaxContext,
+    app: &mut Applicability,
+) -> String {
+    let (snip, is_mac_call) = snippet_with_context(cx, expr.span, ctxt, "(..)", app);
+
+    match snip {
+        Cow::Borrowed(snip) => snip.to_owned(),
+        Cow::Owned(snip) if is_mac_call => snip,
+        Cow::Owned(mut snip) => {
+            let ctxt = expr.span.ctxt();
+
+            // Attempt to determine if parenthesis are needed base on the target position. The snippet may have
+            // parenthesis already, so attempt to find those.
+            // TODO: Remove parenthesis if they aren't needed at the target position.
+            let needs_paren = match expr.peel_drop_temps().kind {
+                ExprKind::Binary(_, lhs, rhs)
+                    if (ctxt == lhs.span.ctxt() && expr.span.lo() != lhs.span.lo())
+                        || (ctxt == rhs.span.ctxt() && expr.span.hi() != rhs.span.hi()) =>
+                {
+                    false
+                },
+                ExprKind::Binary(op, ..) => match op.node {
+                    BinOpKind::Add | BinOpKind::Sub => target_position > ExprPosition::AddLhs,
+                    BinOpKind::Mul | BinOpKind::Div | BinOpKind::Rem => target_position > ExprPosition::MulLhs,
+                    BinOpKind::And => target_position > ExprPosition::AndLhs,
+                    BinOpKind::Or => target_position > ExprPosition::OrLhs,
+                    BinOpKind::BitXor => target_position > ExprPosition::BitXorLhs,
+                    BinOpKind::BitAnd => target_position > ExprPosition::BitAndLhs,
+                    BinOpKind::BitOr => target_position > ExprPosition::BitOrLhs,
+                    BinOpKind::Shl | BinOpKind::Shr => target_position > ExprPosition::ShiftLhs,
+                    BinOpKind::Eq | BinOpKind::Lt | BinOpKind::Le | BinOpKind::Ne | BinOpKind::Gt | BinOpKind::Ge => {
+                        target_position > ExprPosition::EqLhs
+                    },
+                },
+                ExprKind::Box(..) | ExprKind::Unary(..) | ExprKind::AddrOf(..) if snip.starts_with('(') => false,
+                ExprKind::Box(..) | ExprKind::Unary(..) | ExprKind::AddrOf(..) => {
+                    target_position > ExprPosition::Prefix
+                },
+                ExprKind::Let(..) if snip.starts_with('(') => false,
+                ExprKind::Let(..) => target_position > ExprPosition::Let,
+                ExprKind::Cast(lhs, rhs)
+                    if (ctxt == lhs.span.ctxt() && expr.span.lo() != lhs.span.lo())
+                        || (ctxt == rhs.span.ctxt() && expr.span.hi() != rhs.span.hi()) =>
+                {
+                    false
+                },
+                ExprKind::Cast(..) | ExprKind::Type(..) => target_position > ExprPosition::Cast,
+
+                ExprKind::Closure(..)
+                | ExprKind::Break(..)
+                | ExprKind::Ret(..)
+                | ExprKind::Yield(..)
+                | ExprKind::Assign(..)
+                | ExprKind::AssignOp(..) => target_position > ExprPosition::AssignmentRhs,
+
+                // Postfix operators, or expression with braces of some form
+                ExprKind::Array(_)
+                | ExprKind::Call(..)
+                | ExprKind::ConstBlock(_)
+                | ExprKind::MethodCall(..)
+                | ExprKind::Tup(..)
+                | ExprKind::Lit(..)
+                | ExprKind::DropTemps(_)
+                | ExprKind::If(..)
+                | ExprKind::Loop(..)
+                | ExprKind::Match(..)
+                | ExprKind::Block(..)
+                | ExprKind::Field(..)
+                | ExprKind::Index(..)
+                | ExprKind::Path(_)
+                | ExprKind::Continue(_)
+                | ExprKind::InlineAsm(_)
+                | ExprKind::LlvmInlineAsm(_)
+                | ExprKind::Struct(..)
+                | ExprKind::Repeat(..)
+                | ExprKind::Err => false,
+            };
+
+            if needs_paren {
+                snip.insert(0, '(');
+                snip.push(')');
+            }
+            snip
+        },
+    }
+}
+
+/// Gets which position the expression is in relative to it's parent. Defaults to `Paren` if the
+/// parent node is not an expression.
+pub fn position_of_expr(cx: &LateContext<'_>, expr: &Expr<'_>) -> ExprPosition {
+    match get_parent_expr(cx, expr) {
+        None => ExprPosition::Paren,
+        Some(parent) => match parent.kind {
+            ExprKind::DropTemps(_) => position_of_expr(cx, parent),
+            ExprKind::Binary(op, lhs, _) => match (op.node, expr.hir_id == lhs.hir_id) {
+                (BinOpKind::Add | BinOpKind::Sub, true) => ExprPosition::AddLhs,
+                (BinOpKind::Add | BinOpKind::Sub, false) => ExprPosition::AddRhs,
+                (BinOpKind::Mul | BinOpKind::Div | BinOpKind::Rem, true) => ExprPosition::MulLhs,
+                (BinOpKind::Mul | BinOpKind::Div | BinOpKind::Rem, false) => ExprPosition::MulRhs,
+                (BinOpKind::And, true) => ExprPosition::AndLhs,
+                (BinOpKind::And, false) => ExprPosition::AndRhs,
+                (BinOpKind::Or, true) => ExprPosition::OrLhs,
+                (BinOpKind::Or, false) => ExprPosition::OrRhs,
+                (BinOpKind::BitXor, true) => ExprPosition::BitXorLhs,
+                (BinOpKind::BitXor, false) => ExprPosition::BitXorRhs,
+                (BinOpKind::BitAnd, true) => ExprPosition::BitAndLhs,
+                (BinOpKind::BitAnd, false) => ExprPosition::BitAndRhs,
+                (BinOpKind::BitOr, true) => ExprPosition::BitOrLhs,
+                (BinOpKind::BitOr, false) => ExprPosition::BitOrRhs,
+                (BinOpKind::Shl | BinOpKind::Shr, true) => ExprPosition::ShiftLhs,
+                (BinOpKind::Shl | BinOpKind::Shr, false) => ExprPosition::ShiftRhs,
+                (
+                    BinOpKind::Eq | BinOpKind::Lt | BinOpKind::Le | BinOpKind::Ne | BinOpKind::Gt | BinOpKind::Ge,
+                    true,
+                ) => ExprPosition::EqLhs,
+                (
+                    BinOpKind::Eq | BinOpKind::Lt | BinOpKind::Le | BinOpKind::Ne | BinOpKind::Gt | BinOpKind::Ge,
+                    false,
+                ) => ExprPosition::EqRhs,
+            },
+            ExprKind::Unary(..) | ExprKind::AddrOf(..) => ExprPosition::Prefix,
+            ExprKind::Cast(..) | ExprKind::Type(..) => ExprPosition::Cast,
+            ExprKind::Assign(lhs, ..) | ExprKind::AssignOp(_, lhs, _) if lhs.hir_id == expr.hir_id => {
+                ExprPosition::AssignmentLhs
+            },
+            ExprKind::Assign(..) | ExprKind::AssignOp(..) => ExprPosition::AssignmentRhs,
+            ExprKind::Call(e, _) | ExprKind::MethodCall(_, _, [e, ..], _) | ExprKind::Index(e, _)
+                if expr.hir_id == e.hir_id =>
+            {
+                ExprPosition::Postfix
+            },
+            ExprKind::Match(_, _, MatchSource::TryDesugar | MatchSource::AwaitDesugar) | ExprKind::Field(..) => {
+                ExprPosition::Postfix
+            },
+            _ => ExprPosition::Paren,
+        },
+    }
+}
+
 /// Walks the span up to the target context, thereby returning the macro call site if the span is
 /// inside a macro expansion, or the original span if it is not. Note this will return `None` in the
 /// case of the span being in a macro expansion, but the target context is from expanding a macro
diff --git a/tests/ui/redundant_slicing.fixed b/tests/ui/redundant_slicing.fixed
@@ -0,0 +1,43 @@
+// run-rustfix
+#![allow(unused)]
+#![warn(clippy::redundant_slicing)]
+
+fn main() {
+    let slice: &[u32] = &[0];
+    let _ = slice;
+
+    let v = vec![0];
+    let _ = &v[..]; // Changes the type
+    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 *mut_slice; // Should reborrow 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
+
+    trait T {
+        fn f(&mut self);
+    }
+    impl T for &'_ [u32] {
+        fn f(&mut self) {}
+    }
+
+    // Reborrow, `f` takes a mutable reference to the slice
+    (&*slice).f();
+}
diff --git a/tests/ui/redundant_slicing.rs b/tests/ui/redundant_slicing.rs
@@ -1,3 +1,4 @@
+// run-rustfix
 #![allow(unused)]
 #![warn(clippy::redundant_slicing)]
 
@@ -29,4 +30,14 @@ fn main() {
         };
     }
     let _ = m2!(slice); // Don't lint in a macro
+
+    trait T {
+        fn f(&mut self);
+    }
+    impl T for &'_ [u32] {
+        fn f(&mut self) {}
+    }
+
+    // Reborrow, `f` takes a mutable reference to the slice
+    (&slice[..]).f();
 }
diff --git a/tests/ui/redundant_slicing.stderr b/tests/ui/redundant_slicing.stderr
@@ -1,34 +1,40 @@
 error: redundant slicing of the whole range
-  --> $DIR/redundant_slicing.rs:6:13
+  --> $DIR/redundant_slicing.rs:7:13
    |
 LL |     let _ = &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[..])`
 
 error: redundant slicing of the whole range
-  --> $DIR/redundant_slicing.rs:13:20
+  --> $DIR/redundant_slicing.rs:14: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: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:25: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:42:5
+   |
+LL |     (&slice[..]).f();
+   |     ^^^^^^^^^^^^ help: reborrow the original value instead: `(&*slice)`
+
+error: aborting due to 6 previous errors
 
