diff --git a/CHANGELOG.md b/CHANGELOG.md index c9adf77c0d63..ef7139d63bf0 100644 --- 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 index a80320a578f0..2451a5aab090 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -420,6 +420,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 index 5a89fdb05a99..f89f35b885c1 100644 --- 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 index cd3aee556553..25a9072ef6e0 100644 --- 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 new file mode 100644 index 000000000000..b26276218b78 --- /dev/null +++ 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 new file mode 100644 index 000000000000..6aa1408ba176 --- /dev/null +++ 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 new file mode 100644 index 000000000000..ffd76de378df --- /dev/null +++ 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 new file mode 100644 index 000000000000..8dd8d3092378 --- /dev/null +++ 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 index 554b6ba36ae0..51c16dd8d65a 100644 --- 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 index bbd10eafbbe7..82367143c07f 100644 --- a/tests/ui/redundant_slicing.stderr +++ b/tests/ui/redundant_slicing.stderr @@ -1,34 +1,22 @@ error: redundant slicing of the whole range - --> $DIR/redundant_slicing.rs:6:13 + --> $DIR/redundant_slicing.rs:10: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 - | -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 - | -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:14:13 | -LL | let _ = &mut mut_slice[..]; // Should reborrow instead of slice - | ^^^^^^^^^^^^^^^^^^ help: reborrow the original value instead: `&mut *mut_slice` +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:24:13 + --> $DIR/redundant_slicing.rs:31:13 | LL | let _ = &m!(slice)[..]; | ^^^^^^^^^^^^^^ help: use the original value instead: `slice` -error: aborting due to 5 previous errors +error: aborting due to 3 previous errors