From 4bdc97c4a66eccfc3925cd691121965b08017908 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Mon, 3 Jan 2022 20:35:32 -0500 Subject: [PATCH] Account for auto-borrows and precedence in `redundant_slicing` lint --- clippy_lints/src/redundant_slicing.rs | 38 +++++++++++++++++++++------ tests/ui/redundant_slicing.fixed | 6 +++++ tests/ui/redundant_slicing.rs | 6 +++++ tests/ui/redundant_slicing.stderr | 26 +++++++++++------- 4 files changed, 58 insertions(+), 18 deletions(-) diff --git a/clippy_lints/src/redundant_slicing.rs b/clippy_lints/src/redundant_slicing.rs index b492f944dc32..1cc0990b283d 100644 --- a/clippy_lints/src/redundant_slicing.rs +++ b/clippy_lints/src/redundant_slicing.rs @@ -3,9 +3,11 @@ use clippy_utils::get_parent_expr; use clippy_utils::source::snippet_with_context; 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_middle::ty::adjustment::{Adjust, AutoBorrow, AutoBorrowMutability}; use rustc_middle::ty::subst::GenericArg; use rustc_session::{declare_lint_pass, declare_tool_lint}; @@ -57,33 +59,51 @@ impl<'tcx> LateLintPass<'tcx> for RedundantSlicing { 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 (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 (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), + 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. ("&*", "reborrow the original value instead") - } else if expr_ref_count != indexed_ref_count { + } else if deref_count != 0 { ("", "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)) + 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) + }; + + (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, @@ -91,10 +111,12 @@ impl<'tcx> LateLintPass<'tcx> for RedundantSlicing { ) { if 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), - ) + 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) + }; + ("dereference the original value instead", sugg) } else { return; } diff --git a/tests/ui/redundant_slicing.fixed b/tests/ui/redundant_slicing.fixed index 9716a48be94b..98b2adec4404 100644 --- a/tests/ui/redundant_slicing.fixed +++ b/tests/ui/redundant_slicing.fixed @@ -3,6 +3,8 @@ #![allow(unused)] #![warn(clippy::redundant_slicing)] +use std::io::Read; + fn main() { let slice: &[u32] = &[0]; let _ = slice; // Redundant slice @@ -37,4 +39,8 @@ fn main() { let slice_ref = &slice; let _ = *slice_ref; // Deref instead of slice + + // Issue #7972 + let bytes: &[u8] = &[]; + let _ = (&*bytes).read_to_end(&mut vec![]).unwrap(); } diff --git a/tests/ui/redundant_slicing.rs b/tests/ui/redundant_slicing.rs index a6902f7619bd..b84a304ec674 100644 --- a/tests/ui/redundant_slicing.rs +++ b/tests/ui/redundant_slicing.rs @@ -3,6 +3,8 @@ #![allow(unused)] #![warn(clippy::redundant_slicing)] +use std::io::Read; + fn main() { let slice: &[u32] = &[0]; let _ = &slice[..]; // Redundant slice @@ -37,4 +39,8 @@ fn main() { let slice_ref = &slice; let _ = &slice_ref[..]; // Deref instead of slice + + // Issue #7972 + let bytes: &[u8] = &[]; + let _ = (&bytes[..]).read_to_end(&mut vec![]).unwrap(); } diff --git a/tests/ui/redundant_slicing.stderr b/tests/ui/redundant_slicing.stderr index c2a5e9e19140..bce351117907 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:8:13 + --> $DIR/redundant_slicing.rs:10:13 | LL | let _ = &slice[..]; // Redundant slice | ^^^^^^^^^^ help: use the original value instead: `slice` @@ -7,52 +7,58 @@ LL | let _ = &slice[..]; // Redundant slice = note: `-D clippy::redundant-slicing` implied by `-D warnings` error: redundant slicing of the whole range - --> $DIR/redundant_slicing.rs:11:13 + --> $DIR/redundant_slicing.rs:13:13 | 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:12:13 + --> $DIR/redundant_slicing.rs:14: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 + --> $DIR/redundant_slicing.rs:17: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:18:21 + --> $DIR/redundant_slicing.rs:20: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 + --> $DIR/redundant_slicing.rs:21: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:22:13 + --> $DIR/redundant_slicing.rs:24: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 + --> $DIR/redundant_slicing.rs:31:13 | LL | let _ = &m!(slice)[..]; | ^^^^^^^^^^^^^^ help: use the original value instead: `slice` error: redundant slicing of the whole range - --> $DIR/redundant_slicing.rs:39:13 + --> $DIR/redundant_slicing.rs:41:13 | LL | let _ = &slice_ref[..]; // Deref instead of slice | ^^^^^^^^^^^^^^ help: dereference the original value instead: `*slice_ref` -error: aborting due to 9 previous errors +error: redundant slicing of the whole range + --> $DIR/redundant_slicing.rs:45:13 + | +LL | let _ = (&bytes[..]).read_to_end(&mut vec![]).unwrap(); + | ^^^^^^^^^^^^ help: reborrow the original value instead: `(&*bytes)` + +error: aborting due to 10 previous errors