diff --git a/clippy_lints/src/bytecount.rs b/clippy_lints/src/bytecount.rs index a938ada9d2aa..afb317421d07 100644 --- a/clippy_lints/src/bytecount.rs +++ b/clippy_lints/src/bytecount.rs @@ -74,10 +74,10 @@ impl<'tcx> LateLintPass<'tcx> for ByteCount { if (p == sym::iter || p == sym!(iter_mut)) && args.len() == 1 { &args[0] } else { - &filter_recv + filter_recv } } else { - &filter_recv + filter_recv }; let mut applicability = Applicability::MaybeIncorrect; span_lint_and_sugg( diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index bbaae94d3ddb..fa2b348591be 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -1,14 +1,20 @@ -use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::source::snippet_with_context; +use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then}; +use clippy_utils::source::{snippet_with_applicability, snippet_with_context}; use clippy_utils::ty::peel_mid_ty_refs; -use clippy_utils::{get_parent_node, is_lint_allowed}; -use rustc_ast::util::parser::PREC_PREFIX; +use clippy_utils::{get_parent_expr, get_parent_node, is_lint_allowed, path_to_local}; +use rustc_ast::util::parser::{PREC_POSTFIX, PREC_PREFIX}; +use rustc_data_structures::fx::FxIndexMap; use rustc_errors::Applicability; -use rustc_hir::{BorrowKind, Expr, ExprKind, HirId, MatchSource, Mutability, Node, UnOp}; +use rustc_hir::{ + BindingAnnotation, Body, BodyId, BorrowKind, Destination, Expr, ExprKind, HirId, MatchSource, Mutability, Node, + Pat, PatKind, UnOp, +}; use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow}; use rustc_middle::ty::{self, Ty, TyCtxt, TyS, TypeckResults}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::{symbol::sym, Span}; +use std::iter; declare_clippy_lint! { /// ### What it does @@ -40,8 +46,64 @@ declare_clippy_lint! { "Explicit use of deref or deref_mut method while not in a method chain." } +declare_clippy_lint! { + /// ### What it does + /// Checks for address of operations (`&`) that are going to + /// be dereferenced immediately by the compiler. + /// + /// ### Why is this bad? + /// Suggests that the receiver of the expression borrows + /// the expression. + /// + /// ### Example + /// ```rust + /// fn fun(_a: &i32) {} + /// + /// // Bad + /// let x: &i32 = &&&&&&5; + /// fun(&x); + /// + /// // Good + /// let x: &i32 = &5; + /// fun(x); + /// ``` + #[clippy::version = "pre 1.29.0"] + pub NEEDLESS_BORROW, + style, + "taking a reference that is going to be automatically dereferenced" +} + +declare_clippy_lint! { + /// ### What it does + /// Checks for `ref` bindings which create a reference to a reference. + /// + /// ### Why is this bad? + /// The address-of operator at the use site is clearer about the need for a reference. + /// + /// ### Example + /// ```rust + /// // Bad + /// let x = Some(""); + /// if let Some(ref x) = x { + /// // use `x` here + /// } + /// + /// // Good + /// let x = Some(""); + /// if let Some(x) = x { + /// // use `&x` here + /// } + /// ``` + #[clippy::version = "1.54.0"] + pub REF_BINDING_TO_REFERENCE, + pedantic, + "`ref` binding to a reference" +} + impl_lint_pass!(Dereferencing => [ EXPLICIT_DEREF_METHODS, + NEEDLESS_BORROW, + REF_BINDING_TO_REFERENCE, ]); #[derive(Default)] @@ -52,6 +114,18 @@ pub struct Dereferencing { // expression. This is to store the id of that expression so it can be skipped when // `check_expr` is called for it. skip_expr: Option, + + /// The body the first local was found in. Used to emit lints when the traversal of the body has + /// been finished. Note we can't lint at the end of every body as they can be nested within each + /// other. + current_body: Option, + /// The list of locals currently being checked by the lint. + /// If the value is `None`, then the binding has been seen as a ref pattern, but is not linted. + /// This is needed for or patterns where one of the branches can be linted, but another can not + /// be. + /// + /// e.g. `m!(x) | Foo::Bar(ref x)` + ref_locals: FxIndexMap>, } struct StateData { @@ -68,6 +142,9 @@ enum State { ty_changed_count: usize, is_final_ufcs: bool, }, + DerefedBorrow { + count: u32, + }, } // A reference operation considered by this lint pass @@ -77,13 +154,29 @@ enum RefOp { AddrOf, } +struct RefPat { + /// Whether every usage of the binding is dereferenced. + always_deref: bool, + /// The spans of all the ref bindings for this local. + spans: Vec, + /// The applicability of this suggestion. + app: Applicability, + /// All the replacements which need to be made. + replacements: Vec<(Span, String)>, +} + impl<'tcx> LateLintPass<'tcx> for Dereferencing { + #[allow(clippy::too_many_lines)] fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { // Skip path expressions from deref calls. e.g. `Deref::deref(e)` if Some(expr.hir_id) == self.skip_expr.take() { return; } + if let Some(local) = path_to_local(expr) { + self.check_local_usage(cx, expr, local); + } + // Stop processing sub expressions when a macro call is seen if expr.span.from_expansion() { if let Some((state, data)) = self.state.take() { @@ -128,6 +221,48 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { }, )); }, + RefOp::AddrOf => { + // Find the number of times the borrow is auto-derefed. + let mut iter = find_adjustments(cx.tcx, typeck, expr).iter(); + if let Some((i, adjust)) = iter.by_ref().enumerate().find_map(|(i, adjust)| { + if !matches!(adjust.kind, Adjust::Deref(_)) { + Some((i, adjust)) + } else if !adjust.target.is_ref() { + // Add one to the number of references found. + Some((i + 1, adjust)) + } else { + None + } + }) { + // Found two consecutive derefs. At least one can be removed. + if i > 1 { + let target_mut = iter::once(adjust) + .chain(iter) + .find_map(|adjust| match adjust.kind { + Adjust::Borrow(AutoBorrow::Ref(_, m)) => Some(m.into()), + _ => None, + }) + // This default should never happen. Auto-deref always reborrows. + .unwrap_or(Mutability::Not); + self.state = Some(( + // Subtract one for the current borrow expression, and one to cover the last + // reference which can't be removed (it's either reborrowed, or needed for + // auto-deref to happen). + State::DerefedBorrow { + count: + // Truncation here would require more than a `u32::MAX` level reference. The compiler + // does not support this. + #[allow(clippy::cast_possible_truncation)] + { i as u32 - 2 } + }, + StateData { + span: expr.span, + target_mut, + }, + )); + } + } + }, _ => (), } }, @@ -144,10 +279,80 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { data, )); }, + (Some((State::DerefedBorrow { count }, data)), RefOp::AddrOf) if count != 0 => { + self.state = Some((State::DerefedBorrow { count: count - 1 }, data)); + }, (Some((state, data)), _) => report(cx, expr, state, data), } } + + fn check_pat(&mut self, cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>) { + if let PatKind::Binding(BindingAnnotation::Ref, id, name, _) = pat.kind { + if let Some(opt_prev_pat) = self.ref_locals.get_mut(&id) { + // This binding id has been seen before. Add this pattern to the list of changes. + if let Some(prev_pat) = opt_prev_pat { + if pat.span.from_expansion() { + // Doesn't match the context of the previous pattern. Can't lint here. + *opt_prev_pat = None; + } else { + prev_pat.spans.push(pat.span); + prev_pat.replacements.push(( + pat.span, + snippet_with_context(cx, name.span, pat.span.ctxt(), "..", &mut prev_pat.app) + .0 + .into(), + )); + } + } + return; + } + + if_chain! { + if !pat.span.from_expansion(); + if let ty::Ref(_, tam, _) = *cx.typeck_results().pat_ty(pat).kind(); + // only lint immutable refs, because borrowed `&mut T` cannot be moved out + if let ty::Ref(_, _, Mutability::Not) = *tam.kind(); + then { + let mut app = Applicability::MachineApplicable; + let snip = snippet_with_context(cx, name.span, pat.span.ctxt(), "..", &mut app).0; + self.current_body = self.current_body.or(cx.enclosing_body); + self.ref_locals.insert( + id, + Some(RefPat { + always_deref: true, + spans: vec![pat.span], + app, + replacements: vec![(pat.span, snip.into())], + }), + ); + } + } + } + } + + fn check_body_post(&mut self, cx: &LateContext<'tcx>, body: &'tcx Body<'_>) { + if Some(body.id()) == self.current_body { + for pat in self.ref_locals.drain(..).filter_map(|(_, x)| x) { + let replacements = pat.replacements; + let app = pat.app; + span_lint_and_then( + cx, + if pat.always_deref { + NEEDLESS_BORROW + } else { + REF_BINDING_TO_REFERENCE + }, + pat.spans, + "this pattern creates a reference to a reference", + |diag| { + diag.multipart_suggestion("try this", replacements, app); + }, + ); + } + self.current_body = None; + } + } } fn try_parse_ref_op( @@ -251,6 +456,48 @@ fn is_linted_explicit_deref_position(parent: Option>, child_id: HirId, } } +/// Adjustments are sometimes made in the parent block rather than the expression itself. +fn find_adjustments( + tcx: TyCtxt<'tcx>, + typeck: &'tcx TypeckResults<'_>, + expr: &'tcx Expr<'_>, +) -> &'tcx [Adjustment<'tcx>] { + let map = tcx.hir(); + let mut iter = map.parent_iter(expr.hir_id); + let mut prev = expr; + + loop { + match typeck.expr_adjustments(prev) { + [] => (), + a => break a, + }; + + match iter.next().map(|(_, x)| x) { + Some(Node::Block(_)) => { + if let Some((_, Node::Expr(e))) = iter.next() { + prev = e; + } else { + // This shouldn't happen. Blocks are always contained in an expression. + break &[]; + } + }, + Some(Node::Expr(&Expr { + kind: ExprKind::Break(Destination { target_id: Ok(id), .. }, _), + .. + })) => { + if let Some(Node::Expr(e)) = map.find(id) { + prev = e; + iter = map.parent_iter(id); + } else { + // This shouldn't happen. The destination should exist. + break &[]; + } + }, + _ => break &[], + } + } +} + #[allow(clippy::needless_pass_by_value)] fn report(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data: StateData) { match state { @@ -301,5 +548,83 @@ fn report(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data: Stat app, ); }, + State::DerefedBorrow { .. } => { + let mut app = Applicability::MachineApplicable; + let snip = snippet_with_context(cx, expr.span, data.span.ctxt(), "..", &mut app).0; + span_lint_and_sugg( + cx, + NEEDLESS_BORROW, + data.span, + &format!( + "this expression borrows a reference (`{}`) that is immediately dereferenced by the compiler", + cx.typeck_results().expr_ty(expr), + ), + "change this to", + snip.into(), + app, + ); + }, + } +} + +impl Dereferencing { + fn check_local_usage(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>, local: HirId) { + if let Some(outer_pat) = self.ref_locals.get_mut(&local) { + if let Some(pat) = outer_pat { + // Check for auto-deref + if !matches!( + cx.typeck_results().expr_adjustments(e), + [ + Adjustment { + kind: Adjust::Deref(_), + .. + }, + Adjustment { + kind: Adjust::Deref(_), + .. + }, + .. + ] + ) { + match get_parent_expr(cx, e) { + // Field accesses are the same no matter the number of references. + Some(Expr { + kind: ExprKind::Field(..), + .. + }) => (), + Some(&Expr { + span, + kind: ExprKind::Unary(UnOp::Deref, _), + .. + }) if !span.from_expansion() => { + // Remove explicit deref. + let snip = snippet_with_context(cx, e.span, span.ctxt(), "..", &mut pat.app).0; + pat.replacements.push((span, snip.into())); + }, + Some(parent) if !parent.span.from_expansion() => { + // Double reference might be needed at this point. + if parent.precedence().order() == PREC_POSTFIX { + // Parentheses would be needed here, don't lint. + *outer_pat = None; + } else { + pat.always_deref = false; + let snip = snippet_with_context(cx, e.span, parent.span.ctxt(), "..", &mut pat.app).0; + pat.replacements.push((e.span, format!("&{}", snip))); + } + }, + _ if !e.span.from_expansion() => { + // Double reference might be needed at this point. + pat.always_deref = false; + let snip = snippet_with_applicability(cx, e.span, "..", &mut pat.app); + pat.replacements.push((e.span, format!("&{}", snip))); + }, + // Edge case for macros. The span of the identifier will usually match the context of the + // binding, but not if the identifier was created in a macro. e.g. `concat_idents` and proc + // macros + _ => *outer_pat = None, + } + } + } + } } } diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 15edb79d36c2..e24ed9905b6a 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -33,6 +33,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(copies::IFS_SAME_COND), LintId::of(copies::IF_SAME_THEN_ELSE), LintId::of(default::FIELD_REASSIGN_WITH_DEFAULT), + LintId::of(dereference::NEEDLESS_BORROW), LintId::of(derivable_impls::DERIVABLE_IMPLS), LintId::of(derive::DERIVE_HASH_XOR_EQ), LintId::of(derive::DERIVE_ORD_XOR_PARTIAL_ORD), @@ -203,7 +204,6 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(needless_arbitrary_self_type::NEEDLESS_ARBITRARY_SELF_TYPE), LintId::of(needless_bool::BOOL_COMPARISON), LintId::of(needless_bool::NEEDLESS_BOOL), - LintId::of(needless_borrow::NEEDLESS_BORROW), LintId::of(needless_borrowed_ref::NEEDLESS_BORROWED_REFERENCE), LintId::of(needless_option_as_deref::NEEDLESS_OPTION_AS_DEREF), LintId::of(needless_question_mark::NEEDLESS_QUESTION_MARK), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 6d1d45f89000..1458d6eab1a1 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -92,6 +92,8 @@ store.register_lints(&[ default::FIELD_REASSIGN_WITH_DEFAULT, default_numeric_fallback::DEFAULT_NUMERIC_FALLBACK, dereference::EXPLICIT_DEREF_METHODS, + dereference::NEEDLESS_BORROW, + dereference::REF_BINDING_TO_REFERENCE, derivable_impls::DERIVABLE_IMPLS, derive::DERIVE_HASH_XOR_EQ, derive::DERIVE_ORD_XOR_PARTIAL_ORD, @@ -356,8 +358,6 @@ store.register_lints(&[ needless_bitwise_bool::NEEDLESS_BITWISE_BOOL, needless_bool::BOOL_COMPARISON, needless_bool::NEEDLESS_BOOL, - needless_borrow::NEEDLESS_BORROW, - needless_borrow::REF_BINDING_TO_REFERENCE, needless_borrowed_ref::NEEDLESS_BORROWED_REFERENCE, needless_continue::NEEDLESS_CONTINUE, needless_for_each::NEEDLESS_FOR_EACH, diff --git a/clippy_lints/src/lib.register_pedantic.rs b/clippy_lints/src/lib.register_pedantic.rs index 404ca20b5abc..5ee7799f9c51 100644 --- a/clippy_lints/src/lib.register_pedantic.rs +++ b/clippy_lints/src/lib.register_pedantic.rs @@ -21,6 +21,7 @@ store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![ LintId::of(copy_iterator::COPY_ITERATOR), LintId::of(default::DEFAULT_TRAIT_ACCESS), LintId::of(dereference::EXPLICIT_DEREF_METHODS), + LintId::of(dereference::REF_BINDING_TO_REFERENCE), LintId::of(derive::EXPL_IMPL_CLONE_ON_COPY), LintId::of(derive::UNSAFE_DERIVE_DESERIALIZE), LintId::of(doc::DOC_MARKDOWN), @@ -68,7 +69,6 @@ store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![ LintId::of(misc::USED_UNDERSCORE_BINDING), LintId::of(mut_mut::MUT_MUT), LintId::of(needless_bitwise_bool::NEEDLESS_BITWISE_BOOL), - LintId::of(needless_borrow::REF_BINDING_TO_REFERENCE), LintId::of(needless_continue::NEEDLESS_CONTINUE), LintId::of(needless_for_each::NEEDLESS_FOR_EACH), LintId::of(needless_pass_by_value::NEEDLESS_PASS_BY_VALUE), diff --git a/clippy_lints/src/lib.register_style.rs b/clippy_lints/src/lib.register_style.rs index 744880bda3e6..f336441ea842 100644 --- a/clippy_lints/src/lib.register_style.rs +++ b/clippy_lints/src/lib.register_style.rs @@ -15,6 +15,7 @@ store.register_group(true, "clippy::style", Some("clippy_style"), vec![ LintId::of(collapsible_match::COLLAPSIBLE_MATCH), LintId::of(comparison_chain::COMPARISON_CHAIN), LintId::of(default::FIELD_REASSIGN_WITH_DEFAULT), + LintId::of(dereference::NEEDLESS_BORROW), LintId::of(doc::MISSING_SAFETY_DOC), LintId::of(doc::NEEDLESS_DOCTEST_MAIN), LintId::of(enum_variants::ENUM_VARIANT_NAMES), @@ -81,7 +82,6 @@ store.register_group(true, "clippy::style", Some("clippy_style"), vec![ LintId::of(misc_early::REDUNDANT_PATTERN), LintId::of(mut_mutex_lock::MUT_MUTEX_LOCK), LintId::of(mut_reference::UNNECESSARY_MUT_PASSED), - LintId::of(needless_borrow::NEEDLESS_BORROW), LintId::of(neg_multiply::NEG_MULTIPLY), LintId::of(new_without_default::NEW_WITHOUT_DEFAULT), LintId::of(non_copy_const::BORROW_INTERIOR_MUTABLE_CONST), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 22f344919627..60109e8aebcf 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -297,7 +297,6 @@ mod mutex_atomic; mod needless_arbitrary_self_type; mod needless_bitwise_bool; mod needless_bool; -mod needless_borrow; mod needless_borrowed_ref; mod needless_continue; mod needless_for_each; @@ -602,7 +601,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(zero_div_zero::ZeroDiv)); store.register_late_pass(|| Box::new(mutex_atomic::Mutex)); store.register_late_pass(|| Box::new(needless_update::NeedlessUpdate)); - store.register_late_pass(|| Box::new(needless_borrow::NeedlessBorrow::default())); store.register_late_pass(|| Box::new(needless_borrowed_ref::NeedlessBorrowedRef)); store.register_late_pass(|| Box::new(no_effect::NoEffect)); store.register_late_pass(|| Box::new(temporary_assignment::TemporaryAssignment)); diff --git a/clippy_lints/src/needless_borrow.rs b/clippy_lints/src/needless_borrow.rs deleted file mode 100644 index 6709fed91bd1..000000000000 --- a/clippy_lints/src/needless_borrow.rs +++ /dev/null @@ -1,284 +0,0 @@ -//! Checks for needless address of operations (`&`) -//! -//! This lint is **warn** by default - -use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::source::{snippet_opt, snippet_with_applicability, snippet_with_context}; -use clippy_utils::{get_parent_expr, path_to_local}; -use if_chain::if_chain; -use rustc_ast::util::parser::PREC_POSTFIX; -use rustc_data_structures::fx::FxIndexMap; -use rustc_errors::Applicability; -use rustc_hir::{BindingAnnotation, Body, BodyId, BorrowKind, Expr, ExprKind, HirId, Mutability, Pat, PatKind, UnOp}; -use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::ty; -use rustc_middle::ty::adjustment::{Adjust, Adjustment}; -use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::Span; - -declare_clippy_lint! { - /// ### What it does - /// Checks for address of operations (`&`) that are going to - /// be dereferenced immediately by the compiler. - /// - /// ### Why is this bad? - /// Suggests that the receiver of the expression borrows - /// the expression. - /// - /// ### Example - /// ```rust - /// fn fun(_a: &i32) {} - /// - /// // Bad - /// let x: &i32 = &&&&&&5; - /// fun(&x); - /// - /// // Good - /// let x: &i32 = &5; - /// fun(x); - /// ``` - #[clippy::version = "pre 1.29.0"] - pub NEEDLESS_BORROW, - style, - "taking a reference that is going to be automatically dereferenced" -} - -declare_clippy_lint! { - /// ### What it does - /// Checks for `ref` bindings which create a reference to a reference. - /// - /// ### Why is this bad? - /// The address-of operator at the use site is clearer about the need for a reference. - /// - /// ### Example - /// ```rust - /// // Bad - /// let x = Some(""); - /// if let Some(ref x) = x { - /// // use `x` here - /// } - /// - /// // Good - /// let x = Some(""); - /// if let Some(x) = x { - /// // use `&x` here - /// } - /// ``` - #[clippy::version = "1.54.0"] - pub REF_BINDING_TO_REFERENCE, - pedantic, - "`ref` binding to a reference" -} - -impl_lint_pass!(NeedlessBorrow => [NEEDLESS_BORROW, REF_BINDING_TO_REFERENCE]); -#[derive(Default)] -pub struct NeedlessBorrow { - /// The body the first local was found in. Used to emit lints when the traversal of the body has - /// been finished. Note we can't lint at the end of every body as they can be nested within each - /// other. - current_body: Option, - /// The list of locals currently being checked by the lint. - /// If the value is `None`, then the binding has been seen as a ref pattern, but is not linted. - /// This is needed for or patterns where one of the branches can be linted, but another can not - /// be. - /// - /// e.g. `m!(x) | Foo::Bar(ref x)` - ref_locals: FxIndexMap>, -} - -struct RefPat { - /// Whether every usage of the binding is dereferenced. - always_deref: bool, - /// The spans of all the ref bindings for this local. - spans: Vec, - /// The applicability of this suggestion. - app: Applicability, - /// All the replacements which need to be made. - replacements: Vec<(Span, String)>, -} - -impl<'tcx> LateLintPass<'tcx> for NeedlessBorrow { - fn check_expr(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) { - if let Some(local) = path_to_local(e) { - self.check_local_usage(cx, e, local); - } - - if e.span.from_expansion() { - return; - } - if let ExprKind::AddrOf(BorrowKind::Ref, mutability, inner) = e.kind { - if let ty::Ref(_, ty, _) = cx.typeck_results().expr_ty(inner).kind() { - for adj3 in cx.typeck_results().expr_adjustments(e).windows(3) { - if let [ - Adjustment { - kind: Adjust::Deref(_), .. - }, - Adjustment { - kind: Adjust::Deref(_), .. - }, - Adjustment { - kind: Adjust::Borrow(_), - .. - }, - ] = *adj3 - { - let help_msg_ty = if matches!(mutability, Mutability::Not) { - format!("&{}", ty) - } else { - format!("&mut {}", ty) - }; - - span_lint_and_then( - cx, - NEEDLESS_BORROW, - e.span, - &format!( - "this expression borrows a reference (`{}`) that is immediately dereferenced \ - by the compiler", - help_msg_ty - ), - |diag| { - if let Some(snippet) = snippet_opt(cx, inner.span) { - diag.span_suggestion( - e.span, - "change this to", - snippet, - Applicability::MachineApplicable, - ); - } - }, - ); - } - } - } - } - } - - fn check_pat(&mut self, cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>) { - if let PatKind::Binding(BindingAnnotation::Ref, id, name, _) = pat.kind { - if let Some(opt_prev_pat) = self.ref_locals.get_mut(&id) { - // This binding id has been seen before. Add this pattern to the list of changes. - if let Some(prev_pat) = opt_prev_pat { - if pat.span.from_expansion() { - // Doesn't match the context of the previous pattern. Can't lint here. - *opt_prev_pat = None; - } else { - prev_pat.spans.push(pat.span); - prev_pat.replacements.push(( - pat.span, - snippet_with_context(cx, name.span, pat.span.ctxt(), "..", &mut prev_pat.app) - .0 - .into(), - )); - } - } - return; - } - - if_chain! { - if !pat.span.from_expansion(); - if let ty::Ref(_, tam, _) = *cx.typeck_results().pat_ty(pat).kind(); - // only lint immutable refs, because borrowed `&mut T` cannot be moved out - if let ty::Ref(_, _, Mutability::Not) = *tam.kind(); - then { - let mut app = Applicability::MachineApplicable; - let snip = snippet_with_context(cx, name.span, pat.span.ctxt(), "..", &mut app).0; - self.current_body = self.current_body.or(cx.enclosing_body); - self.ref_locals.insert( - id, - Some(RefPat { - always_deref: true, - spans: vec![pat.span], - app, - replacements: vec![(pat.span, snip.into())], - }), - ); - } - } - } - } - - fn check_body_post(&mut self, cx: &LateContext<'tcx>, body: &'tcx Body<'_>) { - if Some(body.id()) == self.current_body { - for pat in self.ref_locals.drain(..).filter_map(|(_, x)| x) { - let replacements = pat.replacements; - let app = pat.app; - span_lint_and_then( - cx, - if pat.always_deref { - NEEDLESS_BORROW - } else { - REF_BINDING_TO_REFERENCE - }, - pat.spans, - "this pattern creates a reference to a reference", - |diag| { - diag.multipart_suggestion("try this", replacements, app); - }, - ); - } - self.current_body = None; - } - } -} -impl NeedlessBorrow { - fn check_local_usage(&mut self, cx: &LateContext<'tcx>, e: &'tcx Expr<'_>, local: HirId) { - if let Some(outer_pat) = self.ref_locals.get_mut(&local) { - if let Some(pat) = outer_pat { - // Check for auto-deref - if !matches!( - cx.typeck_results().expr_adjustments(e), - [ - Adjustment { - kind: Adjust::Deref(_), - .. - }, - Adjustment { - kind: Adjust::Deref(_), - .. - }, - .. - ] - ) { - match get_parent_expr(cx, e) { - // Field accesses are the same no matter the number of references. - Some(Expr { - kind: ExprKind::Field(..), - .. - }) => (), - Some(&Expr { - span, - kind: ExprKind::Unary(UnOp::Deref, _), - .. - }) if !span.from_expansion() => { - // Remove explicit deref. - let snip = snippet_with_context(cx, e.span, span.ctxt(), "..", &mut pat.app).0; - pat.replacements.push((span, snip.into())); - }, - Some(parent) if !parent.span.from_expansion() => { - // Double reference might be needed at this point. - if parent.precedence().order() == PREC_POSTFIX { - // Parentheses would be needed here, don't lint. - *outer_pat = None; - } else { - pat.always_deref = false; - let snip = snippet_with_context(cx, e.span, parent.span.ctxt(), "..", &mut pat.app).0; - pat.replacements.push((e.span, format!("&{}", snip))); - } - }, - _ if !e.span.from_expansion() => { - // Double reference might be needed at this point. - pat.always_deref = false; - let snip = snippet_with_applicability(cx, e.span, "..", &mut pat.app); - pat.replacements.push((e.span, format!("&{}", snip))); - }, - // Edge case for macros. The span of the identifier will usually match the context of the - // binding, but not if the identifier was created in a macro. e.g. `concat_idents` and proc - // macros - _ => *outer_pat = None, - } - } - } - } - } -} diff --git a/tests/ui/needless_borrow.fixed b/tests/ui/needless_borrow.fixed index 42c2bb9f4149..9e37fb925598 100644 --- a/tests/ui/needless_borrow.fixed +++ b/tests/ui/needless_borrow.fixed @@ -1,9 +1,10 @@ // run-rustfix #[warn(clippy::all, clippy::needless_borrow)] -#[allow(unused_variables)] +#[allow(unused_variables, clippy::unnecessary_mut_passed)] fn main() { let a = 5; + let ref_a = &a; let _ = x(&a); // no warning let _ = x(&a); // warn @@ -21,11 +22,29 @@ fn main() { 44 => &a, 45 => { println!("foo"); - &&a // FIXME: this should lint, too + &a }, 46 => &a, + 47 => { + println!("foo"); + loop { + println!("{}", a); + if a == 25 { + break ref_a; + } + } + }, _ => panic!(), }; + + let _ = x(&a); + let _ = x(&a); + let _ = x(&mut b); + let _ = x(ref_a); + { + let b = &mut b; + x(b); + } } #[allow(clippy::needless_borrowed_reference)] diff --git a/tests/ui/needless_borrow.rs b/tests/ui/needless_borrow.rs index 31977416bc70..093277784beb 100644 --- a/tests/ui/needless_borrow.rs +++ b/tests/ui/needless_borrow.rs @@ -1,9 +1,10 @@ // run-rustfix #[warn(clippy::all, clippy::needless_borrow)] -#[allow(unused_variables)] +#[allow(unused_variables, clippy::unnecessary_mut_passed)] fn main() { let a = 5; + let ref_a = &a; let _ = x(&a); // no warning let _ = x(&&a); // warn @@ -21,11 +22,29 @@ fn main() { 44 => &a, 45 => { println!("foo"); - &&a // FIXME: this should lint, too + &&a }, 46 => &&a, + 47 => { + println!("foo"); + loop { + println!("{}", a); + if a == 25 { + break &ref_a; + } + } + }, _ => panic!(), }; + + let _ = x(&&&a); + let _ = x(&mut &&a); + let _ = x(&&&mut b); + let _ = x(&&ref_a); + { + let b = &mut b; + x(&b); + } } #[allow(clippy::needless_borrowed_reference)] diff --git a/tests/ui/needless_borrow.stderr b/tests/ui/needless_borrow.stderr index 012d62e28715..03a5b3d260e6 100644 --- a/tests/ui/needless_borrow.stderr +++ b/tests/ui/needless_borrow.stderr @@ -1,5 +1,5 @@ error: this expression borrows a reference (`&i32`) that is immediately dereferenced by the compiler - --> $DIR/needless_borrow.rs:8:15 + --> $DIR/needless_borrow.rs:9:15 | LL | let _ = x(&&a); // warn | ^^^ help: change this to: `&a` @@ -7,16 +7,58 @@ LL | let _ = x(&&a); // warn = note: `-D clippy::needless-borrow` implied by `-D warnings` error: this expression borrows a reference (`&mut i32`) that is immediately dereferenced by the compiler - --> $DIR/needless_borrow.rs:12:13 + --> $DIR/needless_borrow.rs:13:13 | LL | mut_ref(&mut &mut b); // warn | ^^^^^^^^^^^ help: change this to: `&mut b` error: this expression borrows a reference (`&i32`) that is immediately dereferenced by the compiler - --> $DIR/needless_borrow.rs:26:15 + --> $DIR/needless_borrow.rs:25:13 + | +LL | &&a + | ^^^ help: change this to: `&a` + +error: this expression borrows a reference (`&i32`) that is immediately dereferenced by the compiler + --> $DIR/needless_borrow.rs:27:15 | LL | 46 => &&a, | ^^^ help: change this to: `&a` -error: aborting due to 3 previous errors +error: this expression borrows a reference (`&i32`) that is immediately dereferenced by the compiler + --> $DIR/needless_borrow.rs:33:27 + | +LL | break &ref_a; + | ^^^^^^ help: change this to: `ref_a` + +error: this expression borrows a reference (`&i32`) that is immediately dereferenced by the compiler + --> $DIR/needless_borrow.rs:40:15 + | +LL | let _ = x(&&&a); + | ^^^^ help: change this to: `&a` + +error: this expression borrows a reference (`&i32`) that is immediately dereferenced by the compiler + --> $DIR/needless_borrow.rs:41:15 + | +LL | let _ = x(&mut &&a); + | ^^^^^^^^ help: change this to: `&a` + +error: this expression borrows a reference (`&mut i32`) that is immediately dereferenced by the compiler + --> $DIR/needless_borrow.rs:42:15 + | +LL | let _ = x(&&&mut b); + | ^^^^^^^^ help: change this to: `&mut b` + +error: this expression borrows a reference (`&i32`) that is immediately dereferenced by the compiler + --> $DIR/needless_borrow.rs:43:15 + | +LL | let _ = x(&&ref_a); + | ^^^^^^^ help: change this to: `ref_a` + +error: this expression borrows a reference (`&mut i32`) that is immediately dereferenced by the compiler + --> $DIR/needless_borrow.rs:46:11 + | +LL | x(&b); + | ^^ help: change this to: `b` + +error: aborting due to 10 previous errors