diff options
Diffstat (limited to 'src/tools/clippy/clippy_lints/src/dereference.rs')
-rw-r--r-- | src/tools/clippy/clippy_lints/src/dereference.rs | 714 |
1 files changed, 525 insertions, 189 deletions
diff --git a/src/tools/clippy/clippy_lints/src/dereference.rs b/src/tools/clippy/clippy_lints/src/dereference.rs index 514661589..88e28018e 100644 --- a/src/tools/clippy/clippy_lints/src/dereference.rs +++ b/src/tools/clippy/clippy_lints/src/dereference.rs @@ -1,24 +1,34 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then}; use clippy_utils::source::{snippet_with_applicability, snippet_with_context}; use clippy_utils::sugg::has_enclosing_paren; -use clippy_utils::ty::{expr_sig, peel_mid_ty_refs, variant_of_res}; -use clippy_utils::{get_parent_expr, is_lint_allowed, path_to_local, walk_to_expr_usage}; +use clippy_utils::ty::{expr_sig, is_copy, peel_mid_ty_refs, ty_sig, variant_of_res}; +use clippy_utils::{ + fn_def_id, get_parent_expr, get_parent_expr_for_hir, is_lint_allowed, meets_msrv, msrvs, path_to_local, + walk_to_expr_usage, +}; use rustc_ast::util::parser::{PREC_POSTFIX, PREC_PREFIX}; use rustc_data_structures::fx::FxIndexMap; use rustc_errors::Applicability; use rustc_hir::intravisit::{walk_ty, Visitor}; use rustc_hir::{ - self as hir, BindingAnnotation, Body, BodyId, BorrowKind, Closure, Expr, ExprKind, FnRetTy, GenericArg, HirId, - ImplItem, ImplItemKind, Item, ItemKind, Local, MatchSource, Mutability, Node, Pat, PatKind, Path, QPath, TraitItem, - TraitItemKind, TyKind, UnOp, + self as hir, def_id::DefId, BindingAnnotation, Body, BodyId, BorrowKind, Closure, Expr, ExprKind, FnRetTy, + GenericArg, HirId, ImplItem, ImplItemKind, Item, ItemKind, Local, MatchSource, Mutability, Node, Pat, PatKind, + Path, QPath, TraitItem, TraitItemKind, TyKind, UnOp, }; +use rustc_index::bit_set::BitSet; use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability}; -use rustc_middle::ty::{self, Ty, TyCtxt, TypeVisitable, TypeckResults}; +use rustc_middle::ty::{ + self, subst::Subst, Binder, BoundVariableKind, EarlyBinder, FnSig, GenericArgKind, List, ParamTy, PredicateKind, + ProjectionPredicate, Ty, TyCtxt, TypeVisitable, TypeckResults, +}; +use rustc_semver::RustcVersion; use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::{symbol::sym, Span, Symbol}; -use rustc_trait_selection::infer::InferCtxtExt; +use rustc_span::{symbol::sym, Span, Symbol, DUMMY_SP}; +use rustc_trait_selection::infer::InferCtxtExt as _; +use rustc_trait_selection::traits::{query::evaluate_obligation::InferCtxtExt as _, Obligation, ObligationCause}; +use std::collections::VecDeque; declare_clippy_lint! { /// ### What it does @@ -127,7 +137,7 @@ declare_clippy_lint! { /// ``` #[clippy::version = "1.60.0"] pub EXPLICIT_AUTO_DEREF, - nursery, + complexity, "dereferencing when the compiler would automatically dereference" } @@ -151,6 +161,7 @@ pub struct Dereferencing { /// been finished. Note we can't lint at the end of every body as they can be nested within each /// other. current_body: Option<BodyId>, + /// 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 @@ -158,6 +169,19 @@ pub struct Dereferencing { /// /// e.g. `m!(x) | Foo::Bar(ref x)` ref_locals: FxIndexMap<HirId, Option<RefPat>>, + + // `IntoIterator` for arrays requires Rust 1.53. + msrv: Option<RustcVersion>, +} + +impl Dereferencing { + #[must_use] + pub fn new(msrv: Option<RustcVersion>) -> Self { + Self { + msrv, + ..Dereferencing::default() + } + } } struct StateData { @@ -170,6 +194,7 @@ struct StateData { struct DerefedBorrow { count: usize, msg: &'static str, + snip_expr: Option<HirId>, } enum State { @@ -183,24 +208,24 @@ enum State { }, DerefedBorrow(DerefedBorrow), ExplicitDeref { - // Span and id of the top-level deref expression if the parent expression is a borrow. - deref_span_id: Option<(Span, HirId)>, + mutability: Option<Mutability>, }, ExplicitDerefField { name: Symbol, }, Reborrow { - deref_span: Span, - deref_hir_id: HirId, + mutability: Mutability, + }, + Borrow { + mutability: Mutability, }, - Borrow, } // A reference operation considered by this lint pass enum RefOp { Method(Mutability), Deref, - AddrOf, + AddrOf(Mutability), } struct RefPat { @@ -250,7 +275,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { match (self.state.take(), kind) { (None, kind) => { let expr_ty = typeck.expr_ty(expr); - let (position, adjustments) = walk_parents(cx, expr); + let (position, adjustments) = walk_parents(cx, expr, self.msrv); match kind { RefOp::Deref => { @@ -263,7 +288,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { )); } else if position.is_deref_stable() { self.state = Some(( - State::ExplicitDeref { deref_span_id: None }, + State::ExplicitDeref { mutability: None }, StateData { span: expr.span, hir_id: expr.hir_id, position }, )); } @@ -289,7 +314,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { }, )); }, - RefOp::AddrOf => { + RefOp::AddrOf(mutability) => { // Find the number of times the borrow is auto-derefed. let mut iter = adjustments.iter(); let mut deref_count = 0usize; @@ -331,20 +356,23 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { let deref_msg = "this expression creates a reference which is immediately dereferenced by the compiler"; let borrow_msg = "this expression borrows a value the compiler would automatically borrow"; + let impl_msg = "the borrowed expression implements the required traits"; - let (required_refs, msg) = if position.can_auto_borrow() { - (1, if deref_count == 1 { borrow_msg } else { deref_msg }) + let (required_refs, msg, snip_expr) = if position.can_auto_borrow() { + (1, if deref_count == 1 { borrow_msg } else { deref_msg }, None) + } else if let Position::ImplArg(hir_id) = position { + (0, impl_msg, Some(hir_id)) } else if let Some(&Adjust::Borrow(AutoBorrow::Ref(_, mutability))) = next_adjust.map(|a| &a.kind) { if matches!(mutability, AutoBorrowMutability::Mut { .. }) && !position.is_reborrow_stable() { - (3, deref_msg) + (3, deref_msg, None) } else { - (2, deref_msg) + (2, deref_msg, None) } } else { - (2, deref_msg) + (2, deref_msg, None) }; if deref_count >= required_refs { @@ -354,12 +382,17 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { // can't be removed without breaking the code. See earlier comment. count: deref_count - required_refs, msg, + snip_expr, }), StateData { span: expr.span, hir_id: expr.hir_id, position }, )); - } else if position.is_deref_stable() { + } else if position.is_deref_stable() + // Auto-deref doesn't combine with other adjustments + && next_adjust.map_or(true, |a| matches!(a.kind, Adjust::Deref(_) | Adjust::Borrow(_))) + && iter.all(|a| matches!(a.kind, Adjust::Deref(_) | Adjust::Borrow(_))) + { self.state = Some(( - State::Borrow, + State::Borrow { mutability }, StateData { span: expr.span, hir_id: expr.hir_id, @@ -395,7 +428,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { data, )); }, - (Some((State::DerefedBorrow(state), data)), RefOp::AddrOf) if state.count != 0 => { + (Some((State::DerefedBorrow(state), data)), RefOp::AddrOf(_)) if state.count != 0 => { self.state = Some(( State::DerefedBorrow(DerefedBorrow { count: state.count - 1, @@ -404,12 +437,12 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { data, )); }, - (Some((State::DerefedBorrow(state), data)), RefOp::AddrOf) => { + (Some((State::DerefedBorrow(state), data)), RefOp::AddrOf(mutability)) => { let position = data.position; report(cx, expr, State::DerefedBorrow(state), data); if position.is_deref_stable() { self.state = Some(( - State::Borrow, + State::Borrow { mutability }, StateData { span: expr.span, hir_id: expr.hir_id, @@ -430,43 +463,28 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { )); } else if position.is_deref_stable() { self.state = Some(( - State::ExplicitDeref { deref_span_id: None }, + State::ExplicitDeref { mutability: None }, StateData { span: expr.span, hir_id: expr.hir_id, position }, )); } }, - (Some((State::Borrow, data)), RefOp::Deref) => { + (Some((State::Borrow { mutability }, data)), RefOp::Deref) => { if typeck.expr_ty(sub_expr).is_ref() { - self.state = Some(( - State::Reborrow { - deref_span: expr.span, - deref_hir_id: expr.hir_id, - }, - data, - )); + self.state = Some((State::Reborrow { mutability }, data)); } else { self.state = Some(( State::ExplicitDeref { - deref_span_id: Some((expr.span, expr.hir_id)), + mutability: Some(mutability), }, data, )); } }, - ( - Some(( - State::Reborrow { - deref_span, - deref_hir_id, - }, - data, - )), - RefOp::Deref, - ) => { + (Some((State::Reborrow { mutability }, data)), RefOp::Deref) => { self.state = Some(( State::ExplicitDeref { - deref_span_id: Some((deref_span, deref_hir_id)), + mutability: Some(mutability), }, data, )); @@ -485,7 +503,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { } fn check_pat(&mut self, cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>) { - if let PatKind::Binding(BindingAnnotation::Ref, id, name, _) = pat.kind { + 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 { @@ -521,7 +539,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { spans: vec![pat.span], app, replacements: vec![(pat.span, snip.into())], - hir_id: pat.hir_id + hir_id: pat.hir_id, }), ); } @@ -553,6 +571,8 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { self.current_body = None; } } + + extract_msrv_attr!(LateContext); } fn try_parse_ref_op<'tcx>( @@ -561,7 +581,7 @@ fn try_parse_ref_op<'tcx>( expr: &'tcx Expr<'_>, ) -> Option<(RefOp, &'tcx Expr<'tcx>)> { let (def_id, arg) = match expr.kind { - ExprKind::MethodCall(_, [arg], _) => (typeck.type_dependent_def_id(expr.hir_id)?, arg), + ExprKind::MethodCall(_, arg, [], _) => (typeck.type_dependent_def_id(expr.hir_id)?, arg), ExprKind::Call( Expr { kind: ExprKind::Path(path), @@ -573,7 +593,7 @@ fn try_parse_ref_op<'tcx>( ExprKind::Unary(UnOp::Deref, sub_expr) if !typeck.expr_ty(sub_expr).is_unsafe_ptr() => { return Some((RefOp::Deref, sub_expr)); }, - ExprKind::AddrOf(BorrowKind::Ref, _, sub_expr) => return Some((RefOp::AddrOf, sub_expr)), + ExprKind::AddrOf(BorrowKind::Ref, mutability, sub_expr) => return Some((RefOp::AddrOf(mutability), sub_expr)), _ => return None, }; if tcx.is_diagnostic_item(sym::deref_method, def_id) { @@ -605,22 +625,26 @@ enum Position { /// The method is defined on a reference type. e.g. `impl Foo for &T` MethodReceiverRefImpl, Callee, + ImplArg(HirId), FieldAccess(Symbol), Postfix, Deref, /// Any other location which will trigger auto-deref to a specific time. - DerefStable(i8), + /// Contains the precedence of the parent expression and whether the target type is sized. + DerefStable(i8, bool), /// Any other location which will trigger auto-reborrowing. + /// Contains the precedence of the parent expression. ReborrowStable(i8), + /// Contains the precedence of the parent expression. Other(i8), } impl Position { fn is_deref_stable(self) -> bool { - matches!(self, Self::DerefStable(_)) + matches!(self, Self::DerefStable(..)) } fn is_reborrow_stable(self) -> bool { - matches!(self, Self::DerefStable(_) | Self::ReborrowStable(_)) + matches!(self, Self::DerefStable(..) | Self::ReborrowStable(_)) } fn can_auto_borrow(self) -> bool { @@ -628,7 +652,7 @@ impl Position { } fn lint_explicit_deref(self) -> bool { - matches!(self, Self::Other(_) | Self::DerefStable(_) | Self::ReborrowStable(_)) + matches!(self, Self::Other(_) | Self::DerefStable(..) | Self::ReborrowStable(_)) } fn precedence(self) -> i8 { @@ -638,8 +662,8 @@ impl Position { | Self::Callee | Self::FieldAccess(_) | Self::Postfix => PREC_POSTFIX, - Self::Deref => PREC_PREFIX, - Self::DerefStable(p) | Self::ReborrowStable(p) | Self::Other(p) => p, + Self::ImplArg(_) | Self::Deref => PREC_PREFIX, + Self::DerefStable(p, _) | Self::ReborrowStable(p) | Self::Other(p) => p, } } } @@ -647,8 +671,12 @@ impl Position { /// Walks up the parent expressions attempting to determine both how stable the auto-deref result /// is, and which adjustments will be applied to it. Note this will not consider auto-borrow /// locations as those follow different rules. -#[allow(clippy::too_many_lines)] -fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, &'tcx [Adjustment<'tcx>]) { +#[expect(clippy::too_many_lines)] +fn walk_parents<'tcx>( + cx: &LateContext<'tcx>, + e: &'tcx Expr<'_>, + msrv: Option<RustcVersion>, +) -> (Position, &'tcx [Adjustment<'tcx>]) { let mut adjustments = [].as_slice(); let mut precedence = 0i8; let ctxt = e.span.ctxt(); @@ -659,7 +687,7 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & } match parent { Node::Local(Local { ty: Some(ty), span, .. }) if span.ctxt() == ctxt => { - Some(binding_ty_auto_deref_stability(ty, precedence)) + Some(binding_ty_auto_deref_stability(cx, ty, precedence, List::empty())) }, Node::Item(&Item { kind: ItemKind::Static(..) | ItemKind::Const(..), @@ -680,11 +708,7 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & .. }) if span.ctxt() == ctxt => { let ty = cx.tcx.type_of(def_id); - Some(if ty.is_ref() { - Position::DerefStable(precedence) - } else { - Position::Other(precedence) - }) + Some(ty_auto_deref_stability(cx, ty, precedence).position_for_result(cx)) }, Node::Item(&Item { @@ -705,45 +729,51 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & span, .. }) if span.ctxt() == ctxt => { - let output = cx.tcx.fn_sig(def_id.to_def_id()).skip_binder().output(); - Some(if !output.is_ref() { - Position::Other(precedence) - } else if output.has_placeholders() || output.has_opaque_types() { - Position::ReborrowStable(precedence) - } else { - Position::DerefStable(precedence) - }) + let output = cx + .tcx + .erase_late_bound_regions(cx.tcx.fn_sig(def_id.to_def_id()).output()); + Some(ty_auto_deref_stability(cx, output, precedence).position_for_result(cx)) + }, + + Node::ExprField(field) if field.span.ctxt() == ctxt => match get_parent_expr_for_hir(cx, field.hir_id) { + Some(Expr { + hir_id, + kind: ExprKind::Struct(path, ..), + .. + }) => variant_of_res(cx, cx.qpath_res(path, *hir_id)) + .and_then(|variant| variant.fields.iter().find(|f| f.name == field.ident.name)) + .map(|field_def| { + ty_auto_deref_stability(cx, cx.tcx.type_of(field_def.did), precedence).position_for_arg() + }), + _ => None, }, Node::Expr(parent) if parent.span.ctxt() == ctxt => match parent.kind { ExprKind::Ret(_) => { let owner_id = cx.tcx.hir().body_owner(cx.enclosing_body.unwrap()); Some( - if let Node::Expr(Expr { - kind: ExprKind::Closure(&Closure { fn_decl, .. }), - .. - }) = cx.tcx.hir().get(owner_id) + if let Node::Expr( + closure_expr @ Expr { + kind: ExprKind::Closure(closure), + .. + }, + ) = cx.tcx.hir().get(owner_id) { - match fn_decl.output { - FnRetTy::Return(ty) => binding_ty_auto_deref_stability(ty, precedence), - FnRetTy::DefaultReturn(_) => Position::Other(precedence), - } + closure_result_position(cx, closure, cx.typeck_results().expr_ty(closure_expr), precedence) } else { let output = cx .tcx - .fn_sig(cx.tcx.hir().local_def_id(owner_id)) - .skip_binder() - .output(); - if !output.is_ref() { - Position::Other(precedence) - } else if output.has_placeholders() || output.has_opaque_types() { - Position::ReborrowStable(precedence) - } else { - Position::DerefStable(precedence) - } + .erase_late_bound_regions(cx.tcx.fn_sig(cx.tcx.hir().local_def_id(owner_id)).output()); + ty_auto_deref_stability(cx, output, precedence).position_for_result(cx) }, ) }, + ExprKind::Closure(closure) => Some(closure_result_position( + cx, + closure, + cx.typeck_results().expr_ty(parent), + precedence, + )), ExprKind::Call(func, _) if func.hir_id == child_id => { (child_id == e.hir_id).then_some(Position::Callee) }, @@ -751,65 +781,72 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & .iter() .position(|arg| arg.hir_id == child_id) .zip(expr_sig(cx, func)) - .and_then(|(i, sig)| sig.input_with_hir(i)) - .map(|(hir_ty, ty)| match hir_ty { - // Type inference for closures can depend on how they're called. Only go by the explicit - // types here. - Some(ty) => binding_ty_auto_deref_stability(ty, precedence), - None => param_auto_deref_stability(ty.skip_binder(), precedence), + .and_then(|(i, sig)| { + sig.input_with_hir(i).map(|(hir_ty, ty)| match hir_ty { + // Type inference for closures can depend on how they're called. Only go by the explicit + // types here. + Some(hir_ty) => binding_ty_auto_deref_stability(cx, hir_ty, precedence, ty.bound_vars()), + None => { + if let ty::Param(param_ty) = ty.skip_binder().kind() { + needless_borrow_impl_arg_position(cx, parent, i, *param_ty, e, precedence, msrv) + } else { + ty_auto_deref_stability(cx, cx.tcx.erase_late_bound_regions(ty), precedence) + .position_for_arg() + } + }, + }) }), - ExprKind::MethodCall(_, args, _) => { + ExprKind::MethodCall(_, receiver, args, _) => { let id = cx.typeck_results().type_dependent_def_id(parent.hir_id).unwrap(); - args.iter().position(|arg| arg.hir_id == child_id).map(|i| { - if i == 0 { - // Check for calls to trait methods where the trait is implemented on a reference. - // Two cases need to be handled: - // * `self` methods on `&T` will never have auto-borrow - // * `&self` methods on `&T` can have auto-borrow, but `&self` methods on `T` will take - // priority. - if e.hir_id != child_id { - Position::ReborrowStable(precedence) - } else if let Some(trait_id) = cx.tcx.trait_of_item(id) - && let arg_ty = cx.tcx.erase_regions(cx.typeck_results().expr_ty_adjusted(e)) - && let ty::Ref(_, sub_ty, _) = *arg_ty.kind() - && let subs = match cx - .typeck_results() - .node_substs_opt(parent.hir_id) - .and_then(|subs| subs.get(1..)) - { - Some(subs) => cx.tcx.mk_substs(subs.iter().copied()), - None => cx.tcx.mk_substs([].iter()), - } && let impl_ty = if cx.tcx.fn_sig(id).skip_binder().inputs()[0].is_ref() { - // Trait methods taking `&self` - sub_ty - } else { - // Trait methods taking `self` - arg_ty - } && impl_ty.is_ref() - && cx.tcx.infer_ctxt().enter(|infcx| - infcx - .type_implements_trait(trait_id, impl_ty, subs, cx.param_env) - .must_apply_modulo_regions() - ) + if receiver.hir_id == child_id { + // Check for calls to trait methods where the trait is implemented on a reference. + // Two cases need to be handled: + // * `self` methods on `&T` will never have auto-borrow + // * `&self` methods on `&T` can have auto-borrow, but `&self` methods on `T` will take + // priority. + if e.hir_id != child_id { + return Some(Position::ReborrowStable(precedence)) + } else if let Some(trait_id) = cx.tcx.trait_of_item(id) + && let arg_ty = cx.tcx.erase_regions(cx.typeck_results().expr_ty_adjusted(e)) + && let ty::Ref(_, sub_ty, _) = *arg_ty.kind() + && let subs = match cx + .typeck_results() + .node_substs_opt(parent.hir_id) + .and_then(|subs| subs.get(1..)) { - Position::MethodReceiverRefImpl + Some(subs) => cx.tcx.mk_substs(subs.iter().copied()), + None => cx.tcx.mk_substs(std::iter::empty::<ty::subst::GenericArg<'_>>()), + } && let impl_ty = if cx.tcx.fn_sig(id).skip_binder().inputs()[0].is_ref() { + // Trait methods taking `&self` + sub_ty } else { - Position::MethodReceiver - } + // Trait methods taking `self` + arg_ty + } && impl_ty.is_ref() + && cx.tcx.infer_ctxt().enter(|infcx| + infcx + .type_implements_trait(trait_id, impl_ty, subs, cx.param_env) + .must_apply_modulo_regions() + ) + { + return Some(Position::MethodReceiverRefImpl) + } + return Some(Position::MethodReceiver); + } + args.iter().position(|arg| arg.hir_id == child_id).map(|i| { + let ty = cx.tcx.fn_sig(id).skip_binder().inputs()[i + 1]; + if let ty::Param(param_ty) = ty.kind() { + needless_borrow_impl_arg_position(cx, parent, i + 1, *param_ty, e, precedence, msrv) } else { - param_auto_deref_stability(cx.tcx.fn_sig(id).skip_binder().inputs()[i], precedence) + ty_auto_deref_stability( + cx, + cx.tcx.erase_late_bound_regions(cx.tcx.fn_sig(id).input(i + 1)), + precedence, + ) + .position_for_arg() } }) }, - ExprKind::Struct(path, fields, _) => { - let variant = variant_of_res(cx, cx.qpath_res(path, parent.hir_id)); - fields - .iter() - .find(|f| f.expr.hir_id == child_id) - .zip(variant) - .and_then(|(field, variant)| variant.fields.iter().find(|f| f.name == field.ident.name)) - .map(|field| param_auto_deref_stability(cx.tcx.type_of(field.did), precedence)) - }, ExprKind::Field(child, name) if child.hir_id == e.hir_id => Some(Position::FieldAccess(name.name)), ExprKind::Unary(UnOp::Deref, child) if child.hir_id == e.hir_id => Some(Position::Deref), ExprKind::Match(child, _, MatchSource::TryDesugar | MatchSource::AwaitDesugar) @@ -831,6 +868,26 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & (position, adjustments) } +fn closure_result_position<'tcx>( + cx: &LateContext<'tcx>, + closure: &'tcx Closure<'_>, + ty: Ty<'tcx>, + precedence: i8, +) -> Position { + match closure.fn_decl.output { + FnRetTy::Return(hir_ty) => { + if let Some(sig) = ty_sig(cx, ty) + && let Some(output) = sig.output() + { + binding_ty_auto_deref_stability(cx, hir_ty, precedence, output.bound_vars()) + } else { + Position::Other(precedence) + } + }, + FnRetTy::DefaultReturn(_) => Position::Other(precedence), + } +} + // Checks the stability of auto-deref when assigned to a binding with the given explicit type. // // e.g. @@ -840,7 +897,12 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & // // Here `y1` and `y2` would resolve to different types, so the type `&Box<_>` is not stable when // switching to auto-dereferencing. -fn binding_ty_auto_deref_stability(ty: &hir::Ty<'_>, precedence: i8) -> Position { +fn binding_ty_auto_deref_stability<'tcx>( + cx: &LateContext<'tcx>, + ty: &'tcx hir::Ty<'_>, + precedence: i8, + binder_args: &'tcx List<BoundVariableKind>, +) -> Position { let TyKind::Rptr(_, ty) = &ty.kind else { return Position::Other(precedence); }; @@ -870,21 +932,33 @@ fn binding_ty_auto_deref_stability(ty: &hir::Ty<'_>, precedence: i8) -> Position { Position::ReborrowStable(precedence) } else { - Position::DerefStable(precedence) + Position::DerefStable( + precedence, + cx.tcx + .erase_late_bound_regions(Binder::bind_with_vars( + cx.typeck_results().node_type(ty.ty.hir_id), + binder_args, + )) + .is_sized(cx.tcx.at(DUMMY_SP), cx.param_env.without_caller_bounds()), + ) } }, - TyKind::Slice(_) - | TyKind::Array(..) - | TyKind::BareFn(_) - | TyKind::Never + TyKind::Slice(_) => Position::DerefStable(precedence, false), + TyKind::Array(..) | TyKind::Ptr(_) | TyKind::BareFn(_) => Position::DerefStable(precedence, true), + TyKind::Never | TyKind::Tup(_) - | TyKind::Ptr(_) - | TyKind::TraitObject(..) - | TyKind::Path(_) => Position::DerefStable(precedence), - TyKind::OpaqueDef(..) - | TyKind::Infer - | TyKind::Typeof(..) - | TyKind::Err => Position::ReborrowStable(precedence), + | TyKind::Path(_) => Position::DerefStable( + precedence, + cx.tcx + .erase_late_bound_regions(Binder::bind_with_vars( + cx.typeck_results().node_type(ty.ty.hir_id), + binder_args, + )) + .is_sized(cx.tcx.at(DUMMY_SP), cx.param_env.without_caller_bounds()), + ), + TyKind::OpaqueDef(..) | TyKind::Infer | TyKind::Typeof(..) | TyKind::TraitObject(..) | TyKind::Err => { + Position::ReborrowStable(precedence) + }, }; } } @@ -920,10 +994,238 @@ fn ty_contains_infer(ty: &hir::Ty<'_>) -> bool { v.0 } +// Checks whether: +// * child is an expression of the form `&e` in an argument position requiring an `impl Trait` +// * `e`'s type implements `Trait` and is copyable +// If the conditions are met, returns `Some(Position::ImplArg(..))`; otherwise, returns `None`. +// The "is copyable" condition is to avoid the case where removing the `&` means `e` would have to +// be moved, but it cannot be. +fn needless_borrow_impl_arg_position<'tcx>( + cx: &LateContext<'tcx>, + parent: &Expr<'tcx>, + arg_index: usize, + param_ty: ParamTy, + mut expr: &Expr<'tcx>, + precedence: i8, + msrv: Option<RustcVersion>, +) -> Position { + let destruct_trait_def_id = cx.tcx.lang_items().destruct_trait(); + let sized_trait_def_id = cx.tcx.lang_items().sized_trait(); + + let Some(callee_def_id) = fn_def_id(cx, parent) else { return Position::Other(precedence) }; + let fn_sig = cx.tcx.fn_sig(callee_def_id).skip_binder(); + let substs_with_expr_ty = cx + .typeck_results() + .node_substs(if let ExprKind::Call(callee, _) = parent.kind { + callee.hir_id + } else { + parent.hir_id + }); + + let predicates = cx.tcx.param_env(callee_def_id).caller_bounds(); + let projection_predicates = predicates + .iter() + .filter_map(|predicate| { + if let PredicateKind::Projection(projection_predicate) = predicate.kind().skip_binder() { + Some(projection_predicate) + } else { + None + } + }) + .collect::<Vec<_>>(); + + let mut trait_with_ref_mut_self_method = false; + + // If no traits were found, or only the `Destruct`, `Sized`, or `Any` traits were found, return. + if predicates + .iter() + .filter_map(|predicate| { + if let PredicateKind::Trait(trait_predicate) = predicate.kind().skip_binder() + && trait_predicate.trait_ref.self_ty() == param_ty.to_ty(cx.tcx) + { + Some(trait_predicate.trait_ref.def_id) + } else { + None + } + }) + .inspect(|trait_def_id| { + trait_with_ref_mut_self_method |= has_ref_mut_self_method(cx, *trait_def_id); + }) + .all(|trait_def_id| { + Some(trait_def_id) == destruct_trait_def_id + || Some(trait_def_id) == sized_trait_def_id + || cx.tcx.is_diagnostic_item(sym::Any, trait_def_id) + }) + { + return Position::Other(precedence); + } + + // `substs_with_referent_ty` can be constructed outside of `check_referent` because the same + // elements are modified each time `check_referent` is called. + let mut substs_with_referent_ty = substs_with_expr_ty.to_vec(); + + let mut check_referent = |referent| { + let referent_ty = cx.typeck_results().expr_ty(referent); + + if !is_copy(cx, referent_ty) { + return false; + } + + // https://github.com/rust-lang/rust-clippy/pull/9136#pullrequestreview-1037379321 + if trait_with_ref_mut_self_method && !matches!(referent_ty.kind(), ty::Ref(_, _, Mutability::Mut)) { + return false; + } + + if !replace_types( + cx, + param_ty, + referent_ty, + fn_sig, + arg_index, + &projection_predicates, + &mut substs_with_referent_ty, + ) { + return false; + } + + predicates.iter().all(|predicate| { + if let PredicateKind::Trait(trait_predicate) = predicate.kind().skip_binder() + && cx.tcx.is_diagnostic_item(sym::IntoIterator, trait_predicate.trait_ref.def_id) + && let ty::Param(param_ty) = trait_predicate.self_ty().kind() + && let GenericArgKind::Type(ty) = substs_with_referent_ty[param_ty.index as usize].unpack() + && ty.is_array() + && !meets_msrv(msrv, msrvs::ARRAY_INTO_ITERATOR) + { + return false; + } + + let predicate = EarlyBinder(predicate).subst(cx.tcx, &substs_with_referent_ty); + let obligation = Obligation::new(ObligationCause::dummy(), cx.param_env, predicate); + cx.tcx + .infer_ctxt() + .enter(|infcx| infcx.predicate_must_hold_modulo_regions(&obligation)) + }) + }; + + let mut needless_borrow = false; + while let ExprKind::AddrOf(_, _, referent) = expr.kind { + if !check_referent(referent) { + break; + } + expr = referent; + needless_borrow = true; + } + + if needless_borrow { + Position::ImplArg(expr.hir_id) + } else { + Position::Other(precedence) + } +} + +fn has_ref_mut_self_method(cx: &LateContext<'_>, trait_def_id: DefId) -> bool { + cx.tcx + .associated_items(trait_def_id) + .in_definition_order() + .any(|assoc_item| { + if assoc_item.fn_has_self_parameter { + let self_ty = cx.tcx.fn_sig(assoc_item.def_id).skip_binder().inputs()[0]; + matches!(self_ty.kind(), ty::Ref(_, _, Mutability::Mut)) + } else { + false + } + }) +} + +// Iteratively replaces `param_ty` with `new_ty` in `substs`, and similarly for each resulting +// projected type that is a type parameter. Returns `false` if replacing the types would have an +// effect on the function signature beyond substituting `new_ty` for `param_ty`. +// See: https://github.com/rust-lang/rust-clippy/pull/9136#discussion_r927212757 +fn replace_types<'tcx>( + cx: &LateContext<'tcx>, + param_ty: ParamTy, + new_ty: Ty<'tcx>, + fn_sig: FnSig<'tcx>, + arg_index: usize, + projection_predicates: &[ProjectionPredicate<'tcx>], + substs: &mut [ty::GenericArg<'tcx>], +) -> bool { + let mut replaced = BitSet::new_empty(substs.len()); + + let mut deque = VecDeque::with_capacity(substs.len()); + deque.push_back((param_ty, new_ty)); + + while let Some((param_ty, new_ty)) = deque.pop_front() { + // If `replaced.is_empty()`, then `param_ty` and `new_ty` are those initially passed in. + if !fn_sig + .inputs_and_output + .iter() + .enumerate() + .all(|(i, ty)| (replaced.is_empty() && i == arg_index) || !ty.contains(param_ty.to_ty(cx.tcx))) + { + return false; + } + + substs[param_ty.index as usize] = ty::GenericArg::from(new_ty); + + // The `replaced.insert(...)` check provides some protection against infinite loops. + if replaced.insert(param_ty.index) { + for projection_predicate in projection_predicates { + if projection_predicate.projection_ty.self_ty() == param_ty.to_ty(cx.tcx) + && let Some(term_ty) = projection_predicate.term.ty() + && let ty::Param(term_param_ty) = term_ty.kind() + { + let item_def_id = projection_predicate.projection_ty.item_def_id; + let assoc_item = cx.tcx.associated_item(item_def_id); + let projection = cx.tcx + .mk_projection(assoc_item.def_id, cx.tcx.mk_substs_trait(new_ty, &[])); + + if let Ok(projected_ty) = cx.tcx.try_normalize_erasing_regions(cx.param_env, projection) + && substs[term_param_ty.index as usize] != ty::GenericArg::from(projected_ty) + { + deque.push_back((*term_param_ty, projected_ty)); + } + } + } + } + } + + true +} + +struct TyPosition<'tcx> { + position: Position, + ty: Option<Ty<'tcx>>, +} +impl From<Position> for TyPosition<'_> { + fn from(position: Position) -> Self { + Self { position, ty: None } + } +} +impl<'tcx> TyPosition<'tcx> { + fn new_deref_stable_for_result(precedence: i8, ty: Ty<'tcx>) -> Self { + Self { + position: Position::ReborrowStable(precedence), + ty: Some(ty), + } + } + fn position_for_result(self, cx: &LateContext<'tcx>) -> Position { + match (self.position, self.ty) { + (Position::ReborrowStable(precedence), Some(ty)) => { + Position::DerefStable(precedence, ty.is_sized(cx.tcx.at(DUMMY_SP), cx.param_env)) + }, + (position, _) => position, + } + } + fn position_for_arg(self) -> Position { + self.position + } +} + // Checks whether a type is stable when switching to auto dereferencing, -fn param_auto_deref_stability(ty: Ty<'_>, precedence: i8) -> Position { +fn ty_auto_deref_stability<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, precedence: i8) -> TyPosition<'tcx> { let ty::Ref(_, mut ty, _) = *ty.kind() else { - return Position::Other(precedence); + return Position::Other(precedence).into(); }; loop { @@ -932,35 +1234,38 @@ fn param_auto_deref_stability(ty: Ty<'_>, precedence: i8) -> Position { ty = ref_ty; continue; }, - ty::Infer(_) - | ty::Error(_) - | ty::Param(_) - | ty::Bound(..) - | ty::Opaque(..) - | ty::Placeholder(_) - | ty::Dynamic(..) => Position::ReborrowStable(precedence), - ty::Adt(..) if ty.has_placeholders() || ty.has_param_types_or_consts() => { - Position::ReborrowStable(precedence) + ty::Param(_) => TyPosition::new_deref_stable_for_result(precedence, ty), + ty::Infer(_) | ty::Error(_) | ty::Bound(..) | ty::Opaque(..) | ty::Placeholder(_) | ty::Dynamic(..) => { + Position::ReborrowStable(precedence).into() }, - ty::Adt(..) - | ty::Bool + ty::Adt(..) if ty.has_placeholders() || ty.has_opaque_types() => { + Position::ReborrowStable(precedence).into() + }, + ty::Adt(_, substs) if substs.has_param_types_or_consts() => { + TyPosition::new_deref_stable_for_result(precedence, ty) + }, + ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) - | ty::Float(_) - | ty::Foreign(_) - | ty::Str | ty::Array(..) - | ty::Slice(..) + | ty::Float(_) | ty::RawPtr(..) + | ty::FnPtr(_) => Position::DerefStable(precedence, true).into(), + ty::Str | ty::Slice(..) => Position::DerefStable(precedence, false).into(), + ty::Adt(..) + | ty::Foreign(_) | ty::FnDef(..) - | ty::FnPtr(_) - | ty::Closure(..) | ty::Generator(..) | ty::GeneratorWitness(..) + | ty::Closure(..) | ty::Never | ty::Tuple(_) - | ty::Projection(_) => Position::DerefStable(precedence), + | ty::Projection(_) => Position::DerefStable( + precedence, + ty.is_sized(cx.tcx.at(DUMMY_SP), cx.param_env.without_caller_bounds()), + ) + .into(), }; } } @@ -1026,7 +1331,8 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data }, State::DerefedBorrow(state) => { let mut app = Applicability::MachineApplicable; - let (snip, snip_is_macro) = snippet_with_context(cx, expr.span, data.span.ctxt(), "..", &mut app); + let snip_expr = state.snip_expr.map_or(expr, |hir_id| cx.tcx.hir().expect_expr(hir_id)); + let (snip, snip_is_macro) = snippet_with_context(cx, snip_expr.span, data.span.ctxt(), "..", &mut app); span_lint_hir_and_then(cx, NEEDLESS_BORROW, data.hir_id, data.span, state.msg, |diag| { let calls_field = matches!(expr.kind, ExprKind::Field(..)) && matches!(data.position, Position::Callee); let sugg = if !snip_is_macro @@ -1040,34 +1346,64 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data diag.span_suggestion(data.span, "change this to", sugg, app); }); }, - State::ExplicitDeref { deref_span_id } => { - let (span, hir_id, precedence) = if let Some((span, hir_id)) = deref_span_id + State::ExplicitDeref { mutability } => { + if matches!( + expr.kind, + ExprKind::Block(..) + | ExprKind::ConstBlock(_) + | ExprKind::If(..) + | ExprKind::Loop(..) + | ExprKind::Match(..) + ) && matches!(data.position, Position::DerefStable(_, true)) + { + // Rustc bug: auto deref doesn't work on block expression when targeting sized types. + return; + } + + let (prefix, precedence) = if let Some(mutability) = mutability && !cx.typeck_results().expr_ty(expr).is_ref() { - (span, hir_id, PREC_PREFIX) + let prefix = match mutability { + Mutability::Not => "&", + Mutability::Mut => "&mut ", + }; + (prefix, 0) } else { - (data.span, data.hir_id, data.position.precedence()) + ("", data.position.precedence()) }; span_lint_hir_and_then( cx, EXPLICIT_AUTO_DEREF, - hir_id, - span, + data.hir_id, + data.span, "deref which would be done by auto-deref", |diag| { let mut app = Applicability::MachineApplicable; - let (snip, snip_is_macro) = snippet_with_context(cx, expr.span, span.ctxt(), "..", &mut app); + let (snip, snip_is_macro) = snippet_with_context(cx, expr.span, data.span.ctxt(), "..", &mut app); let sugg = if !snip_is_macro && expr.precedence().order() < precedence && !has_enclosing_paren(&snip) { - format!("({})", snip) + format!("{}({})", prefix, snip) } else { - snip.into() + format!("{}{}", prefix, snip) }; - diag.span_suggestion(span, "try this", sugg, app); + diag.span_suggestion(data.span, "try this", sugg, app); }, ); }, State::ExplicitDerefField { .. } => { + if matches!( + expr.kind, + ExprKind::Block(..) + | ExprKind::ConstBlock(_) + | ExprKind::If(..) + | ExprKind::Loop(..) + | ExprKind::Match(..) + ) && matches!(data.position, Position::DerefStable(_, true)) + { + // Rustc bug: auto deref doesn't work on block expression when targeting sized types. + return; + } + span_lint_hir_and_then( cx, EXPLICIT_AUTO_DEREF, @@ -1081,7 +1417,7 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data }, ); }, - State::Borrow | State::Reborrow { .. } => (), + State::Borrow { .. } | State::Reborrow { .. } => (), } } |