From 1571cc7449735dd40f42c328326c83693210d68e Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sat, 13 Nov 2021 22:38:28 -0500 Subject: [PATCH] Fix `redundant_slicing` when the result is used as the reciever in a method call. --- clippy_lints/src/redundant_slicing.rs | 28 +++- clippy_utils/src/source.rs | 197 +++++++++++++++++++++++++- tests/ui/redundant_slicing.fixed | 43 ++++++ tests/ui/redundant_slicing.rs | 11 ++ tests/ui/redundant_slicing.stderr | 18 ++- 5 files changed, 282 insertions(+), 15 deletions(-) create mode 100644 tests/ui/redundant_slicing.fixed diff --git a/clippy_lints/src/redundant_slicing.rs b/clippy_lints/src/redundant_slicing.rs index b2bd0103d111..2e9e2c238cd7 100644 --- 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.is_empty() && 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 index d928317259da..5af99472f683 100644 --- 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 new file mode 100644 index 000000000000..d2508089bfbc --- /dev/null +++ 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 index 554b6ba36ae0..f9a4ac3cfc9f 100644 --- 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 index bbd10eafbbe7..80c72b0957aa 100644 --- a/tests/ui/redundant_slicing.stderr +++ b/tests/ui/redundant_slicing.stderr @@ -1,5 +1,5 @@ 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` @@ -7,28 +7,34 @@ LL | let _ = &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