From c107c97e69850a4da3ed3008591281fff7d611d3 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Tue, 25 Jan 2022 16:26:30 -0500 Subject: [PATCH 01/14] Better support projection types when finding the signature for an expression --- clippy_utils/src/ty.rs | 174 ++++++++++++++++++++++++++--------------- 1 file changed, 109 insertions(+), 65 deletions(-) diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index 4c079151f24d..12831ea43554 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -14,8 +14,8 @@ use rustc_lint::LateContext; use rustc_middle::mir::interpret::{ConstValue, Scalar}; use rustc_middle::ty::subst::{GenericArg, GenericArgKind, Subst}; use rustc_middle::ty::{ - self, AdtDef, Binder, BoundRegion, FnSig, IntTy, ParamEnv, Predicate, PredicateKind, Region, RegionKind, Ty, - TyCtxt, TypeFoldable, TypeSuperFoldable, TypeVisitor, UintTy, VariantDiscr, + self, AdtDef, Binder, BoundRegion, FnSig, IntTy, ParamEnv, Predicate, PredicateKind, ProjectionTy, Region, + RegionKind, Ty, TyCtxt, TypeFoldable, TypeSuperFoldable, TypeVisitor, UintTy, VariantDiscr, }; use rustc_span::symbol::Ident; use rustc_span::{sym, Span, Symbol, DUMMY_SP}; @@ -530,74 +530,118 @@ pub fn expr_sig<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) -> Option Some(ExprFnSig::Closure(subs.as_closure().sig())), - ty::FnDef(id, subs) => Some(ExprFnSig::Sig(cx.tcx.bound_fn_sig(id).subst(cx.tcx, subs))), - ty::FnPtr(sig) => Some(ExprFnSig::Sig(sig)), - ty::Dynamic(bounds, _) => { - let lang_items = cx.tcx.lang_items(); - match bounds.principal() { - Some(bound) - if Some(bound.def_id()) == lang_items.fn_trait() - || Some(bound.def_id()) == lang_items.fn_once_trait() - || Some(bound.def_id()) == lang_items.fn_mut_trait() => - { - let output = bounds - .projection_bounds() - .find(|p| lang_items.fn_once_output().map_or(false, |id| id == p.item_def_id())) - .map(|p| p.map_bound(|p| p.term.ty().expect("return type was a const"))); - Some(ExprFnSig::Trait(bound.map_bound(|b| b.substs.type_at(0)), output)) - }, - _ => None, - } - }, - ty::Param(_) | ty::Projection(..) => { - let mut inputs = None; - let mut output = None; - let lang_items = cx.tcx.lang_items(); + ty_sig(cx, cx.typeck_results().expr_ty_adjusted(expr).peel_refs()) + } +} - for (pred, _) in all_predicates_of(cx.tcx, cx.typeck_results().hir_owner.to_def_id()) { - let mut is_input = false; - if let Some(ty) = pred - .kind() - .map_bound(|pred| match pred { - PredicateKind::Trait(p) - if (lang_items.fn_trait() == Some(p.def_id()) - || lang_items.fn_mut_trait() == Some(p.def_id()) - || lang_items.fn_once_trait() == Some(p.def_id())) - && p.self_ty() == ty => - { - is_input = true; - Some(p.trait_ref.substs.type_at(1)) - }, - PredicateKind::Projection(p) - if Some(p.projection_ty.item_def_id) == lang_items.fn_once_output() - && p.projection_ty.self_ty() == ty => - { - is_input = false; - p.term.ty() - }, - _ => None, - }) - .transpose() - { - if is_input && inputs.is_none() { - inputs = Some(ty); - } else if !is_input && output.is_none() { - output = Some(ty); - } else { - // Multiple different fn trait impls. Is this even allowed? - return None; - } - } - } +fn ty_sig<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { + match *ty.kind() { + ty::Closure(_, subs) => Some(ExprFnSig::Closure(subs.as_closure().sig())), + ty::FnDef(id, subs) => Some(ExprFnSig::Sig(cx.tcx.bound_fn_sig(id).subst(cx.tcx, subs))), + ty::FnPtr(sig) => Some(ExprFnSig::Sig(sig)), + ty::Dynamic(bounds, _) => { + let lang_items = cx.tcx.lang_items(); + match bounds.principal() { + Some(bound) + if Some(bound.def_id()) == lang_items.fn_trait() + || Some(bound.def_id()) == lang_items.fn_once_trait() + || Some(bound.def_id()) == lang_items.fn_mut_trait() => + { + let output = bounds + .projection_bounds() + .find(|p| lang_items.fn_once_output().map_or(false, |id| id == p.item_def_id())) + .map(|p| p.map_bound(|p| p.term.ty().unwrap())); + Some(ExprFnSig::Trait(bound.map_bound(|b| b.substs.type_at(0)), output)) + }, + _ => None, + } + }, + ty::Projection(proj) => match cx.tcx.try_normalize_erasing_regions(cx.param_env, ty) { + Ok(normalized_ty) if normalized_ty != ty => ty_sig(cx, normalized_ty), + _ => sig_for_projection(cx, proj).or_else(|| sig_from_bounds(cx, ty)), + }, + ty::Param(_) => sig_from_bounds(cx, ty), + _ => None, + } +} - inputs.map(|ty| ExprFnSig::Trait(ty, output)) +fn sig_from_bounds<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { + let mut inputs = None; + let mut output = None; + let lang_items = cx.tcx.lang_items(); + + for (pred, _) in all_predicates_of(cx.tcx, cx.typeck_results().hir_owner.to_def_id()) { + match pred.kind().skip_binder() { + PredicateKind::Trait(p) + if (lang_items.fn_trait() == Some(p.def_id()) + || lang_items.fn_mut_trait() == Some(p.def_id()) + || lang_items.fn_once_trait() == Some(p.def_id())) + && p.self_ty() == ty => + { + if inputs.is_some() { + // Multiple different fn trait impls. Is this even allowed? + return None; + } + inputs = Some(pred.kind().rebind(p.trait_ref.substs.type_at(1))); }, - _ => None, + PredicateKind::Projection(p) + if Some(p.projection_ty.item_def_id) == lang_items.fn_once_output() + && p.projection_ty.self_ty() == ty => + { + if output.is_some() { + // Multiple different fn trait impls. Is this even allowed? + return None; + } + output = Some(pred.kind().rebind(p.term.ty().unwrap())); + }, + _ => (), } } + + inputs.map(|ty| ExprFnSig::Trait(ty, output)) +} + +fn sig_for_projection<'tcx>(cx: &LateContext<'tcx>, ty: ProjectionTy<'tcx>) -> Option> { + let mut inputs = None; + let mut output = None; + let lang_items = cx.tcx.lang_items(); + + for pred in cx + .tcx + .bound_explicit_item_bounds(ty.item_def_id) + .transpose_iter() + .map(|x| x.map_bound(|(p, _)| p)) + { + match pred.0.kind().skip_binder() { + PredicateKind::Trait(p) + if (lang_items.fn_trait() == Some(p.def_id()) + || lang_items.fn_mut_trait() == Some(p.def_id()) + || lang_items.fn_once_trait() == Some(p.def_id())) => + { + if inputs.is_some() { + // Multiple different fn trait impls. Is this even allowed? + return None; + } + inputs = Some( + pred.map_bound(|pred| pred.kind().rebind(p.trait_ref.substs.type_at(1))) + .subst(cx.tcx, ty.substs), + ); + }, + PredicateKind::Projection(p) if Some(p.projection_ty.item_def_id) == lang_items.fn_once_output() => { + if output.is_some() { + // Multiple different fn trait impls. Is this even allowed? + return None; + } + output = Some( + pred.map_bound(|pred| pred.kind().rebind(p.term.ty().unwrap())) + .subst(cx.tcx, ty.substs), + ); + }, + _ => (), + } + } + + inputs.map(|ty| ExprFnSig::Trait(ty, output)) } #[derive(Clone, Copy)] From 8a74d3357079e5edf2454f6523a9c93081e7348e Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Wed, 26 Jan 2022 22:47:09 -0500 Subject: [PATCH 02/14] Add `explicit_auto_deref` lint --- CHANGELOG.md | 1 + clippy_lints/src/dereference.rs | 309 +++++++++++++++++- clippy_lints/src/lib.register_all.rs | 1 + clippy_lints/src/lib.register_complexity.rs | 1 + clippy_lints/src/lib.register_lints.rs | 1 + .../src/matches/match_str_case_mismatch.rs | 2 +- clippy_lints/src/non_expressive_names.rs | 2 +- clippy_lints/src/ptr.rs | 9 +- .../src/redundant_static_lifetimes.rs | 2 +- clippy_utils/src/lib.rs | 68 +++- clippy_utils/src/ty.rs | 73 ++++- tests/ui/borrow_deref_ref_unfixable.rs | 2 +- tests/ui/explicit_auto_deref.fixed | 161 +++++++++ tests/ui/explicit_auto_deref.rs | 161 +++++++++ tests/ui/explicit_auto_deref.stderr | 160 +++++++++ tests/ui/explicit_deref_methods.fixed | 3 +- tests/ui/explicit_deref_methods.rs | 3 +- tests/ui/explicit_deref_methods.stderr | 24 +- tests/ui/useless_asref.fixed | 1 + tests/ui/useless_asref.rs | 1 + tests/ui/useless_asref.stderr | 22 +- 21 files changed, 954 insertions(+), 53 deletions(-) create mode 100644 tests/ui/explicit_auto_deref.fixed create mode 100644 tests/ui/explicit_auto_deref.rs create mode 100644 tests/ui/explicit_auto_deref.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index bc50ff3a44a8..9bc93c1cb42c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3400,6 +3400,7 @@ Released 2018-09-13 [`expect_fun_call`]: https://rust-lang.github.io/rust-clippy/master/index.html#expect_fun_call [`expect_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#expect_used [`expl_impl_clone_on_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#expl_impl_clone_on_copy +[`explicit_auto_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_auto_deref [`explicit_counter_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_counter_loop [`explicit_deref_methods`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_deref_methods [`explicit_into_iter_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#explicit_into_iter_loop diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index b47441eff374..77518933a230 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -1,18 +1,21 @@ 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::peel_mid_ty_refs; -use clippy_utils::{get_parent_expr, get_parent_node, is_lint_allowed, path_to_local}; +use clippy_utils::ty::{expr_sig, peel_mid_ty_refs, variant_of_res}; +use clippy_utils::{ + get_parent_expr, get_parent_node, is_lint_allowed, path_to_local, peel_hir_ty_refs, 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::{ - BindingAnnotation, Body, BodyId, BorrowKind, Destination, Expr, ExprKind, HirId, MatchSource, Mutability, Node, - Pat, PatKind, UnOp, + self as hir, BindingAnnotation, Body, BodyId, BorrowKind, Destination, Expr, ExprKind, FnRetTy, GenericArg, HirId, + ImplItem, ImplItemKind, Item, ItemKind, Local, MatchSource, Mutability, Node, Pat, PatKind, Path, QPath, TraitItem, + TraitItemKind, TyKind, UnOp, }; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability}; -use rustc_middle::ty::{self, Ty, TyCtxt, TypeckResults}; +use rustc_middle::ty::{self, Ty, TyCtxt, TypeFoldable, TypeckResults}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::{symbol::sym, Span}; @@ -104,10 +107,34 @@ declare_clippy_lint! { "`ref` binding to a reference" } +declare_clippy_lint! { + /// ### What it does + /// Checks for dereferencing expressions which would be covered by auto-deref. + /// + /// ### Why is this bad? + /// This unnecessarily complicates the code. + /// + /// ### Example + /// ```rust + /// let x = String::new(); + /// let y: &str = &*x; + /// ``` + /// Use instead: + /// ```rust + /// let x = String::new(); + /// let y: &str = &x; + /// ``` + #[clippy::version = "1.60.0"] + pub EXPLICIT_AUTO_DEREF, + complexity, + "dereferencing when the compiler would automatically dereference" +} + impl_lint_pass!(Dereferencing => [ EXPLICIT_DEREF_METHODS, NEEDLESS_BORROW, REF_BINDING_TO_REFERENCE, + EXPLICIT_AUTO_DEREF, ]); #[derive(Default)] @@ -152,6 +179,11 @@ enum State { required_precedence: i8, msg: &'static str, }, + ExplicitDeref { + deref_span: Span, + deref_hir_id: HirId, + }, + Borrow, } // A reference operation considered by this lint pass @@ -305,6 +337,14 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { hir_id: expr.hir_id, }, )); + } else if is_stable_auto_deref_position(cx, expr) { + self.state = Some(( + State::Borrow, + StateData { + span: expr.span, + hir_id: expr.hir_id, + }, + )); } }, _ => (), @@ -354,6 +394,18 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { data, )); }, + (Some((State::Borrow, data)), RefOp::Deref) => { + self.state = Some(( + State::ExplicitDeref { + deref_span: expr.span, + deref_hir_id: expr.hir_id, + }, + data, + )); + }, + (state @ Some((State::ExplicitDeref { .. }, _)), RefOp::Deref) => { + self.state = state; + }, (Some((state, data)), _) => report(cx, expr, state, data), } @@ -596,8 +648,230 @@ fn find_adjustments<'tcx>( } } +// Checks if the expression for the given id occurs in a position which auto dereferencing applies. +// Note that the target type must not be inferred in a way that may cause auto-deref to select a +// different type, nor may the position be the result of a macro expansion. +// +// e.g. the following should not linted +// macro_rules! foo { ($e:expr) => { let x: &str = $e; }} +// foo!(&*String::new()); +// fn foo(_: &T) {} +// foo(&*String::new()) +fn is_stable_auto_deref_position<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> bool { + walk_to_expr_usage(cx, e, |node, child_id| match node { + Node::Local(&Local { ty: Some(ty), .. }) => Some(is_binding_ty_auto_deref_stable(ty)), + Node::Item(&Item { + kind: ItemKind::Static(..) | ItemKind::Const(..), + .. + }) + | Node::TraitItem(&TraitItem { + kind: TraitItemKind::Const(..), + .. + }) + | Node::ImplItem(&ImplItem { + kind: ImplItemKind::Const(..), + .. + }) => Some(true), + + Node::Item(&Item { + kind: ItemKind::Fn(..), + def_id, + .. + }) + | Node::TraitItem(&TraitItem { + kind: TraitItemKind::Fn(..), + def_id, + .. + }) + | Node::ImplItem(&ImplItem { + kind: ImplItemKind::Fn(..), + def_id, + .. + }) => { + let output = cx.tcx.fn_sig(def_id.to_def_id()).skip_binder().output(); + Some(!(output.has_placeholders() || output.has_opaque_types())) + }, + + Node::Expr(e) => match e.kind { + ExprKind::Ret(_) => { + let output = cx + .tcx + .fn_sig(cx.tcx.hir().body_owner_def_id(cx.enclosing_body.unwrap())) + .skip_binder() + .output(); + Some(!(output.has_placeholders() || output.has_opaque_types())) + }, + ExprKind::Call(func, args) => Some( + args.iter() + .position(|arg| arg.hir_id == child_id) + .zip(expr_sig(cx, func)) + .and_then(|(i, sig)| sig.input_with_hir(i)) + .map_or(false, |(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) => is_binding_ty_auto_deref_stable(ty), + None => is_param_auto_deref_stable(ty.skip_binder()), + }), + ), + ExprKind::MethodCall(_, [_, args @ ..], _) => { + let id = cx.typeck_results().type_dependent_def_id(e.hir_id).unwrap(); + Some(args.iter().position(|arg| arg.hir_id == child_id).map_or(false, |i| { + let arg = cx.tcx.fn_sig(id).skip_binder().inputs()[i + 1]; + is_param_auto_deref_stable(arg) + })) + }, + ExprKind::Struct(path, fields, _) => { + let variant = variant_of_res(cx, cx.qpath_res(path, e.hir_id)); + Some( + 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_or(false, |field| is_param_auto_deref_stable(cx.tcx.type_of(field.did))), + ) + }, + _ => None, + }, + _ => None, + }) + .unwrap_or(false) +} + +// Checks whether auto-dereferencing any type into a binding of the given type will definitely +// produce the same result. +// +// e.g. +// let x = Box::new(Box::new(0u32)); +// let y1: &Box<_> = x.deref(); +// let y2: &Box<_> = &x; +// +// Here `y1` and `y2` would resolve to different types, so the type `&Box<_>` is not stable when +// switching to auto-dereferencing. +fn is_binding_ty_auto_deref_stable(ty: &hir::Ty<'_>) -> bool { + let (ty, count) = peel_hir_ty_refs(ty); + if count != 1 { + return false; + } + + match &ty.kind { + TyKind::Rptr(_, ty) => is_binding_ty_auto_deref_stable(ty.ty), + &TyKind::Path( + QPath::TypeRelative(_, path) + | QPath::Resolved( + _, + Path { + segments: [.., path], .. + }, + ), + ) => { + if let Some(args) = path.args { + args.args.iter().all(|arg| { + if let GenericArg::Type(ty) = arg { + !ty_contains_infer(ty) + } else { + true + } + }) + } else { + true + } + }, + TyKind::Slice(_) + | TyKind::Array(..) + | TyKind::BareFn(_) + | TyKind::Never + | TyKind::Tup(_) + | TyKind::Ptr(_) + | TyKind::TraitObject(..) + | TyKind::Path(_) => true, + TyKind::OpaqueDef(..) | TyKind::Infer | TyKind::Typeof(..) | TyKind::Err => false, + } +} + +// Checks whether a type is inferred at some point. +// e.g. `_`, `Box<_>`, `[_]` +fn ty_contains_infer(ty: &hir::Ty<'_>) -> bool { + match &ty.kind { + TyKind::Slice(ty) | TyKind::Array(ty, _) => ty_contains_infer(ty), + TyKind::Ptr(ty) | TyKind::Rptr(_, ty) => ty_contains_infer(ty.ty), + TyKind::Tup(tys) => tys.iter().any(ty_contains_infer), + TyKind::BareFn(ty) => { + if ty.decl.inputs.iter().any(ty_contains_infer) { + return true; + } + if let FnRetTy::Return(ty) = &ty.decl.output { + ty_contains_infer(ty) + } else { + false + } + }, + &TyKind::Path( + QPath::TypeRelative(_, path) + | QPath::Resolved( + _, + Path { + segments: [.., path], .. + }, + ), + ) => { + if let Some(args) = path.args { + args.args.iter().any(|arg| { + if let GenericArg::Type(ty) = arg { + ty_contains_infer(ty) + } else { + false + } + }) + } else { + false + } + }, + TyKind::Path(_) | TyKind::OpaqueDef(..) | TyKind::Infer | TyKind::Typeof(_) | TyKind::Err => true, + TyKind::Never | TyKind::TraitObject(..) => false, + } +} + +// Checks whether a type is stable when switching to auto dereferencing, +fn is_param_auto_deref_stable(ty: Ty<'_>) -> bool { + let (ty, count) = peel_mid_ty_refs(ty); + if count != 1 { + return false; + } + + match ty.kind() { + ty::Bool + | ty::Char + | ty::Int(_) + | ty::Uint(_) + | ty::Float(_) + | ty::Foreign(_) + | ty::Str + | ty::Array(..) + | ty::Slice(..) + | ty::RawPtr(..) + | ty::FnDef(..) + | ty::FnPtr(_) + | ty::Closure(..) + | ty::Generator(..) + | ty::GeneratorWitness(..) + | ty::Never + | ty::Tuple(_) + | ty::Ref(..) + | ty::Projection(_) => true, + ty::Infer(_) + | ty::Error(_) + | ty::Param(_) + | ty::Bound(..) + | ty::Opaque(..) + | ty::Placeholder(_) + | ty::Dynamic(..) => false, + ty::Adt(..) => !(ty.has_placeholders() || ty.has_param_types_or_consts()), + } +} + #[expect(clippy::needless_pass_by_value)] -fn report<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>, state: State, data: StateData) { +fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data: StateData) { match state { State::DerefMethod { ty_changed_count, @@ -663,6 +937,29 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>, state: State, data: S diag.span_suggestion(data.span, "change this to", sugg, app); }); }, + State::ExplicitDeref { + deref_span, + deref_hir_id, + } => { + let (span, hir_id) = if cx.typeck_results().expr_ty(expr).is_ref() { + (data.span, data.hir_id) + } else { + (deref_span, deref_hir_id) + }; + span_lint_hir_and_then( + cx, + EXPLICIT_AUTO_DEREF, + hir_id, + span, + "deref which would be done by auto-deref", + |diag| { + let mut app = Applicability::MachineApplicable; + let snip = snippet_with_context(cx, expr.span, span.ctxt(), "..", &mut app).0; + diag.span_suggestion(span, "try this", snip.into_owned(), app); + }, + ); + }, + State::Borrow => (), } } diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index b4edb8ea90ed..56262c8f96e1 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -44,6 +44,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(crate_in_macro_def::CRATE_IN_MACRO_DEF), LintId::of(default::FIELD_REASSIGN_WITH_DEFAULT), LintId::of(default_instead_of_iter_empty::DEFAULT_INSTEAD_OF_ITER_EMPTY), + LintId::of(dereference::EXPLICIT_AUTO_DEREF), LintId::of(dereference::NEEDLESS_BORROW), LintId::of(derivable_impls::DERIVABLE_IMPLS), LintId::of(derive::DERIVE_HASH_XOR_EQ), diff --git a/clippy_lints/src/lib.register_complexity.rs b/clippy_lints/src/lib.register_complexity.rs index 27de11bf1ae9..d7f6140ca4b7 100644 --- a/clippy_lints/src/lib.register_complexity.rs +++ b/clippy_lints/src/lib.register_complexity.rs @@ -9,6 +9,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec! LintId::of(bytes_count_to_len::BYTES_COUNT_TO_LEN), LintId::of(casts::CHAR_LIT_AS_U8), LintId::of(casts::UNNECESSARY_CAST), + LintId::of(dereference::EXPLICIT_AUTO_DEREF), LintId::of(derivable_impls::DERIVABLE_IMPLS), LintId::of(double_comparison::DOUBLE_COMPARISONS), LintId::of(double_parens::DOUBLE_PARENS), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 29a5f368f23b..75993fa168f5 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -110,6 +110,7 @@ store.register_lints(&[ default_instead_of_iter_empty::DEFAULT_INSTEAD_OF_ITER_EMPTY, default_numeric_fallback::DEFAULT_NUMERIC_FALLBACK, default_union_representation::DEFAULT_UNION_REPRESENTATION, + dereference::EXPLICIT_AUTO_DEREF, dereference::EXPLICIT_DEREF_METHODS, dereference::NEEDLESS_BORROW, dereference::REF_BINDING_TO_REFERENCE, diff --git a/clippy_lints/src/matches/match_str_case_mismatch.rs b/clippy_lints/src/matches/match_str_case_mismatch.rs index 8302ce426e57..fa3b8d1fceaa 100644 --- a/clippy_lints/src/matches/match_str_case_mismatch.rs +++ b/clippy_lints/src/matches/match_str_case_mismatch.rs @@ -118,7 +118,7 @@ fn lint(cx: &LateContext<'_>, case_method: &CaseMethod, bad_case_span: Span, bad MATCH_STR_CASE_MISMATCH, bad_case_span, "this `match` arm has a differing case than its expression", - &*format!("consider changing the case of this arm to respect `{}`", method_str), + &format!("consider changing the case of this arm to respect `{}`", method_str), format!("\"{}\"", suggestion), Applicability::MachineApplicable, ); diff --git a/clippy_lints/src/non_expressive_names.rs b/clippy_lints/src/non_expressive_names.rs index c93b4b2f205f..b96af06b8d7c 100644 --- a/clippy_lints/src/non_expressive_names.rs +++ b/clippy_lints/src/non_expressive_names.rs @@ -326,7 +326,7 @@ impl<'a, 'tcx> Visitor<'tcx> for SimilarNamesLocalVisitor<'a, 'tcx> { // add the pattern after the expression because the bindings aren't available // yet in the init // expression - SimilarNamesNameVisitor(self).visit_pat(&*local.pat); + SimilarNamesNameVisitor(self).visit_pat(&local.pat); } fn visit_block(&mut self, blk: &'tcx Block) { self.single_char_names.push(vec![]); diff --git a/clippy_lints/src/ptr.rs b/clippy_lints/src/ptr.rs index b06eba13d2fd..5678b8f6ca68 100644 --- a/clippy_lints/src/ptr.rs +++ b/clippy_lints/src/ptr.rs @@ -574,14 +574,13 @@ fn check_ptr_arg_usage<'tcx>(cx: &LateContext<'tcx>, body: &'tcx Body<'_>, args: Some((Node::Expr(e), child_id)) => match e.kind { ExprKind::Call(f, expr_args) => { let i = expr_args.iter().position(|arg| arg.hir_id == child_id).unwrap_or(0); - if expr_sig(self.cx, f) - .map(|sig| sig.input(i).skip_binder().peel_refs()) - .map_or(true, |ty| match *ty.kind() { + if expr_sig(self.cx, f).and_then(|sig| sig.input(i)).map_or(true, |ty| { + match *ty.skip_binder().peel_refs().kind() { ty::Param(_) => true, ty::Adt(def, _) => def.did() == args.ty_did, _ => false, - }) - { + } + }) { // Passed to a function taking the non-dereferenced type. set_skip_flag(); } diff --git a/clippy_lints/src/redundant_static_lifetimes.rs b/clippy_lints/src/redundant_static_lifetimes.rs index 0825f00f421c..f8801f769e83 100644 --- a/clippy_lints/src/redundant_static_lifetimes.rs +++ b/clippy_lints/src/redundant_static_lifetimes.rs @@ -87,7 +87,7 @@ impl RedundantStaticLifetimes { _ => {}, } } - self.visit_type(&*borrow_type.ty, cx, reason); + self.visit_type(&borrow_type.ty, cx, reason); }, _ => {}, } diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 73c1bdd0e3f4..ca056e7c1a0d 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -2058,6 +2058,21 @@ pub fn peel_hir_expr_refs<'a>(expr: &'a Expr<'a>) -> (&'a Expr<'a>, usize) { (e, count) } +/// Peels off all references on the type. Returns the underlying type and the number of references +/// removed. +pub fn peel_hir_ty_refs<'a>(mut ty: &'a hir::Ty<'a>) -> (&'a hir::Ty<'a>, usize) { + let mut count = 0; + loop { + match &ty.kind { + TyKind::Rptr(_, ref_ty) => { + ty = ref_ty.ty; + count += 1; + }, + _ => break (ty, count), + } + } +} + /// Removes `AddrOf` operators (`&`) or deref operators (`*`), but only if a reference type is /// dereferenced. An overloaded deref such as `Vec` to slice would not be removed. pub fn peel_ref_operators<'hir>(cx: &LateContext<'_>, mut expr: &'hir Expr<'hir>) -> &'hir Expr<'hir> { @@ -2110,7 +2125,7 @@ fn with_test_item_names<'tcx>(tcx: TyCtxt<'tcx>, module: LocalDefId, f: impl Fn( } } names.sort_unstable(); - f(&*entry.insert(names)) + f(entry.insert(names)) }, } } @@ -2168,6 +2183,57 @@ pub fn is_test_module_or_function(tcx: TyCtxt<'_>, item: &Item<'_>) -> bool { && item.ident.name.as_str().split('_').any(|a| a == "test" || a == "tests") } +/// Walks the HIR tree from the given expression, up to the node where the value produced by the +/// expression is consumed. Calls the function for every node encountered this way until it returns +/// `Some`. +/// +/// This allows walking through `if`, `match`, `break`, block expressions to find where the value +/// produced by the expression is consumed. +pub fn walk_to_expr_usage<'tcx, T>( + cx: &LateContext<'tcx>, + e: &Expr<'tcx>, + mut f: impl FnMut(Node<'tcx>, HirId) -> Option, +) -> Option { + let map = cx.tcx.hir(); + let mut iter = map.parent_iter(e.hir_id); + let mut child_id = e.hir_id; + + while let Some((parent_id, parent)) = iter.next() { + if let Some(x) = f(parent, child_id) { + return Some(x); + } + let parent = match parent { + Node::Expr(e) => e, + Node::Block(Block { expr: Some(body), .. }) | Node::Arm(Arm { body, .. }) if body.hir_id == child_id => { + child_id = parent_id; + continue; + }, + Node::Arm(a) if a.body.hir_id == child_id => { + child_id = parent_id; + continue; + }, + _ => return None, + }; + match parent.kind { + ExprKind::If(child, ..) | ExprKind::Match(child, ..) if child.hir_id != child_id => { + child_id = parent_id; + continue; + }, + ExprKind::Break(Destination { target_id: Ok(id), .. }, _) => { + child_id = id; + iter = map.parent_iter(id); + continue; + }, + ExprKind::Block(..) => { + child_id = parent_id; + continue; + }, + _ => return None, + } + } + None +} + macro_rules! op_utils { ($($name:ident $assign:ident)*) => { /// Binary operation traits like `LangItem::Add` diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index 12831ea43554..2c19e8a7adb2 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -6,16 +6,16 @@ use core::ops::ControlFlow; use rustc_ast::ast::Mutability; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_hir as hir; -use rustc_hir::def::{CtorKind, DefKind, Res}; +use rustc_hir::def::{CtorKind, CtorOf, DefKind, Res}; use rustc_hir::def_id::DefId; -use rustc_hir::{Expr, LangItem, TyKind, Unsafety}; +use rustc_hir::{Expr, FnDecl, LangItem, TyKind, Unsafety}; use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::LateContext; use rustc_middle::mir::interpret::{ConstValue, Scalar}; use rustc_middle::ty::subst::{GenericArg, GenericArgKind, Subst}; use rustc_middle::ty::{ - self, AdtDef, Binder, BoundRegion, FnSig, IntTy, ParamEnv, Predicate, PredicateKind, ProjectionTy, Region, - RegionKind, Ty, TyCtxt, TypeFoldable, TypeSuperFoldable, TypeVisitor, UintTy, VariantDiscr, + self, AdtDef, Binder, BoundRegion, DefIdTree, FnSig, IntTy, ParamEnv, Predicate, PredicateKind, ProjectionTy, + Region, RegionKind, Ty, TyCtxt, TypeFoldable, TypeSuperFoldable, TypeVisitor, UintTy, VariantDef, VariantDiscr, }; use rustc_span::symbol::Ident; use rustc_span::{sym, Span, Symbol, DUMMY_SP}; @@ -502,16 +502,46 @@ pub fn all_predicates_of(tcx: TyCtxt<'_>, id: DefId) -> impl Iterator { Sig(Binder<'tcx, FnSig<'tcx>>), - Closure(Binder<'tcx, FnSig<'tcx>>), + Closure(Option<&'tcx FnDecl<'tcx>>, Binder<'tcx, FnSig<'tcx>>), Trait(Binder<'tcx, Ty<'tcx>>, Option>>), } impl<'tcx> ExprFnSig<'tcx> { - /// Gets the argument type at the given offset. - pub fn input(self, i: usize) -> Binder<'tcx, Ty<'tcx>> { + /// Gets the argument type at the given offset. This will return `None` when the index is out of + /// bounds only for variadic functions, otherwise this will panic. + pub fn input(self, i: usize) -> Option>> { match self { - Self::Sig(sig) => sig.input(i), - Self::Closure(sig) => sig.input(0).map_bound(|ty| ty.tuple_fields()[i]), - Self::Trait(inputs, _) => inputs.map_bound(|ty| ty.tuple_fields()[i]), + Self::Sig(sig) => { + if sig.c_variadic() { + sig.inputs().map_bound(|inputs| inputs.get(i).copied()).transpose() + } else { + Some(sig.input(i)) + } + }, + Self::Closure(_, sig) => Some(sig.input(0).map_bound(|ty| ty.tuple_fields()[i])), + Self::Trait(inputs, _) => Some(inputs.map_bound(|ty| ty.tuple_fields()[i])), + } + } + + /// Gets the argument type at the given offset. For closures this will also get the type as + /// written. This will return `None` when the index is out of bounds only for variadic + /// functions, otherwise this will panic. + pub fn input_with_hir(self, i: usize) -> Option<(Option<&'tcx hir::Ty<'tcx>>, Binder<'tcx, Ty<'tcx>>)> { + match self { + Self::Sig(sig) => { + if sig.c_variadic() { + sig.inputs() + .map_bound(|inputs| inputs.get(i).copied()) + .transpose() + .map(|arg| (None, arg)) + } else { + Some((None, sig.input(i))) + } + }, + Self::Closure(decl, sig) => Some(( + decl.and_then(|decl| decl.inputs.get(i)), + sig.input(0).map_bound(|ty| ty.tuple_fields()[i]), + )), + Self::Trait(inputs, _) => Some((None, inputs.map_bound(|ty| ty.tuple_fields()[i]))), } } @@ -519,7 +549,7 @@ impl<'tcx> ExprFnSig<'tcx> { /// specified. pub fn output(self) -> Option>> { match self { - Self::Sig(sig) | Self::Closure(sig) => Some(sig.output()), + Self::Sig(sig) | Self::Closure(_, sig) => Some(sig.output()), Self::Trait(_, output) => output, } } @@ -536,7 +566,12 @@ pub fn expr_sig<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) -> Option(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> Option> { match *ty.kind() { - ty::Closure(_, subs) => Some(ExprFnSig::Closure(subs.as_closure().sig())), + ty::Closure(id, subs) => { + let decl = id + .as_local() + .and_then(|id| cx.tcx.hir().fn_decl_by_hir_id(cx.tcx.hir().local_def_id_to_hir_id(id))); + Some(ExprFnSig::Closure(decl, subs.as_closure().sig())) + }, ty::FnDef(id, subs) => Some(ExprFnSig::Sig(cx.tcx.bound_fn_sig(id).subst(cx.tcx, subs))), ty::FnPtr(sig) => Some(ExprFnSig::Sig(sig)), ty::Dynamic(bounds, _) => { @@ -739,3 +774,17 @@ pub fn for_each_top_level_late_bound_region( } ty.visit_with(&mut V { index: 0, f }) } + +pub fn variant_of_res<'tcx>(cx: &LateContext<'tcx>, res: Res) -> Option<&'tcx VariantDef> { + match res { + Res::Def(DefKind::Struct, id) => Some(cx.tcx.adt_def(id).non_enum_variant()), + Res::Def(DefKind::Variant, id) => Some(cx.tcx.adt_def(cx.tcx.parent(id)).variant_with_id(id)), + Res::Def(DefKind::Ctor(CtorOf::Struct, _), id) => Some(cx.tcx.adt_def(cx.tcx.parent(id)).non_enum_variant()), + Res::Def(DefKind::Ctor(CtorOf::Variant, _), id) => { + let var_id = cx.tcx.parent(id); + Some(cx.tcx.adt_def(cx.tcx.parent(var_id)).variant_with_id(var_id)) + }, + Res::SelfCtor(id) => Some(cx.tcx.type_of(id).ty_adt_def().unwrap().non_enum_variant()), + _ => None, + } +} diff --git a/tests/ui/borrow_deref_ref_unfixable.rs b/tests/ui/borrow_deref_ref_unfixable.rs index a8e2bbfef0f5..b4e93774436a 100644 --- a/tests/ui/borrow_deref_ref_unfixable.rs +++ b/tests/ui/borrow_deref_ref_unfixable.rs @@ -1,4 +1,4 @@ -#![allow(dead_code, unused_variables)] +#![allow(dead_code, unused_variables, clippy::explicit_auto_deref)] fn main() {} diff --git a/tests/ui/explicit_auto_deref.fixed b/tests/ui/explicit_auto_deref.fixed new file mode 100644 index 000000000000..04039ca31541 --- /dev/null +++ b/tests/ui/explicit_auto_deref.fixed @@ -0,0 +1,161 @@ +// run-rustfix + +#![warn(clippy::explicit_auto_deref)] +#![allow( + dead_code, + unused_braces, + clippy::borrowed_box, + clippy::needless_borrow, + clippy::ptr_arg, + clippy::redundant_field_names, + clippy::too_many_arguments +)] + +trait CallableStr { + type T: Fn(&str); + fn callable_str(&self) -> Self::T; +} +impl CallableStr for () { + type T = fn(&str); + fn callable_str(&self) -> Self::T { + fn f(_: &str) {} + f + } +} +impl CallableStr for i32 { + type T = <() as CallableStr>::T; + fn callable_str(&self) -> Self::T { + ().callable_str() + } +} + +trait CallableT { + type T: Fn(&U); + fn callable_t(&self) -> Self::T; +} +impl CallableT for () { + type T = fn(&U); + fn callable_t(&self) -> Self::T { + fn f(_: &U) {} + f:: + } +} +impl CallableT for i32 { + type T = <() as CallableT>::T; + fn callable_t(&self) -> Self::T { + ().callable_t() + } +} + +fn f_str(_: &str) {} +fn f_t(_: T) {} +fn f_ref_t(_: &T) {} + +fn f_str_t(_: &str, _: T) {} + +fn f_box_t(_: &Box) {} + +fn main() { + let s = String::new(); + + let _: &str = &s; + let _ = &*s; // Don't lint. Inferred type would change. + let _: &_ = &*s; // Don't lint. Inferred type would change. + + f_str(&s); + f_t(&*s); // Don't lint. Inferred type would change. + f_ref_t(&*s); // Don't lint. Inferred type would change. + + f_str_t(&s, &*s); // Don't lint second param. + + let b = Box::new(Box::new(Box::new(5))); + let _: &Box = &b; + let _: &Box<_> = &**b; // Don't lint. Inferred type would change. + + f_box_t(&**b); // Don't lint. Inferred type would change. + + let c = |_x: &str| (); + c(&s); + + let c = |_x| (); + c(&*s); // Don't lint. Inferred type would change. + + fn _f(x: &String) -> &str { + x + } + + fn _f1(x: &String) -> &str { + { x } + } + + fn _f2(x: &String) -> &str { + { x } + } + + fn _f3(x: &Box>>) -> &Box { + x + } + + fn _f4( + x: String, + f1: impl Fn(&str), + f2: &dyn Fn(&str), + f3: fn(&str), + f4: impl CallableStr, + f5: <() as CallableStr>::T, + f6: ::T, + f7: &dyn CallableStr, + f8: impl CallableT, + f9: <() as CallableT>::T, + f10: >::T, + f11: &dyn CallableT, + ) { + f1(&x); + f2(&x); + f3(&x); + f4.callable_str()(&x); + f5(&x); + f6(&x); + f7.callable_str()(&x); + f8.callable_t()(&x); + f9(&x); + f10(&x); + f11.callable_t()(&x); + } + + struct S1<'a>(&'a str); + let _ = S1(&s); + + struct S2<'a> { + s: &'a str, + } + let _ = S2 { s: &s }; + + struct S3<'a, T: ?Sized>(&'a T); + let _ = S3(&*s); // Don't lint. Inferred type would change. + + struct S4<'a, T: ?Sized> { + s: &'a T, + } + let _ = S4 { s: &*s }; // Don't lint. Inferred type would change. + + enum E1<'a> { + S1(&'a str), + S2 { s: &'a str }, + } + impl<'a> E1<'a> { + fn m1(s: &'a String) { + let _ = Self::S1(s); + let _ = Self::S2 { s: s }; + } + } + let _ = E1::S1(&s); + let _ = E1::S2 { s: &s }; + + enum E2<'a, T: ?Sized> { + S1(&'a T), + S2 { s: &'a T }, + } + let _ = E2::S1(&*s); // Don't lint. Inferred type would change. + let _ = E2::S2 { s: &*s }; // Don't lint. Inferred type would change. +} diff --git a/tests/ui/explicit_auto_deref.rs b/tests/ui/explicit_auto_deref.rs new file mode 100644 index 000000000000..1cc77c1b08f9 --- /dev/null +++ b/tests/ui/explicit_auto_deref.rs @@ -0,0 +1,161 @@ +// run-rustfix + +#![warn(clippy::explicit_auto_deref)] +#![allow( + dead_code, + unused_braces, + clippy::borrowed_box, + clippy::needless_borrow, + clippy::ptr_arg, + clippy::redundant_field_names, + clippy::too_many_arguments +)] + +trait CallableStr { + type T: Fn(&str); + fn callable_str(&self) -> Self::T; +} +impl CallableStr for () { + type T = fn(&str); + fn callable_str(&self) -> Self::T { + fn f(_: &str) {} + f + } +} +impl CallableStr for i32 { + type T = <() as CallableStr>::T; + fn callable_str(&self) -> Self::T { + ().callable_str() + } +} + +trait CallableT { + type T: Fn(&U); + fn callable_t(&self) -> Self::T; +} +impl CallableT for () { + type T = fn(&U); + fn callable_t(&self) -> Self::T { + fn f(_: &U) {} + f:: + } +} +impl CallableT for i32 { + type T = <() as CallableT>::T; + fn callable_t(&self) -> Self::T { + ().callable_t() + } +} + +fn f_str(_: &str) {} +fn f_t(_: T) {} +fn f_ref_t(_: &T) {} + +fn f_str_t(_: &str, _: T) {} + +fn f_box_t(_: &Box) {} + +fn main() { + let s = String::new(); + + let _: &str = &*s; + let _ = &*s; // Don't lint. Inferred type would change. + let _: &_ = &*s; // Don't lint. Inferred type would change. + + f_str(&*s); + f_t(&*s); // Don't lint. Inferred type would change. + f_ref_t(&*s); // Don't lint. Inferred type would change. + + f_str_t(&*s, &*s); // Don't lint second param. + + let b = Box::new(Box::new(Box::new(5))); + let _: &Box = &**b; + let _: &Box<_> = &**b; // Don't lint. Inferred type would change. + + f_box_t(&**b); // Don't lint. Inferred type would change. + + let c = |_x: &str| (); + c(&*s); + + let c = |_x| (); + c(&*s); // Don't lint. Inferred type would change. + + fn _f(x: &String) -> &str { + &**x + } + + fn _f1(x: &String) -> &str { + { &**x } + } + + fn _f2(x: &String) -> &str { + &**{ x } + } + + fn _f3(x: &Box>>) -> &Box { + &***x + } + + fn _f4( + x: String, + f1: impl Fn(&str), + f2: &dyn Fn(&str), + f3: fn(&str), + f4: impl CallableStr, + f5: <() as CallableStr>::T, + f6: ::T, + f7: &dyn CallableStr, + f8: impl CallableT, + f9: <() as CallableT>::T, + f10: >::T, + f11: &dyn CallableT, + ) { + f1(&*x); + f2(&*x); + f3(&*x); + f4.callable_str()(&*x); + f5(&*x); + f6(&*x); + f7.callable_str()(&*x); + f8.callable_t()(&*x); + f9(&*x); + f10(&*x); + f11.callable_t()(&*x); + } + + struct S1<'a>(&'a str); + let _ = S1(&*s); + + struct S2<'a> { + s: &'a str, + } + let _ = S2 { s: &*s }; + + struct S3<'a, T: ?Sized>(&'a T); + let _ = S3(&*s); // Don't lint. Inferred type would change. + + struct S4<'a, T: ?Sized> { + s: &'a T, + } + let _ = S4 { s: &*s }; // Don't lint. Inferred type would change. + + enum E1<'a> { + S1(&'a str), + S2 { s: &'a str }, + } + impl<'a> E1<'a> { + fn m1(s: &'a String) { + let _ = Self::S1(&**s); + let _ = Self::S2 { s: &**s }; + } + } + let _ = E1::S1(&*s); + let _ = E1::S2 { s: &*s }; + + enum E2<'a, T: ?Sized> { + S1(&'a T), + S2 { s: &'a T }, + } + let _ = E2::S1(&*s); // Don't lint. Inferred type would change. + let _ = E2::S2 { s: &*s }; // Don't lint. Inferred type would change. +} diff --git a/tests/ui/explicit_auto_deref.stderr b/tests/ui/explicit_auto_deref.stderr new file mode 100644 index 000000000000..f3cceb6b889d --- /dev/null +++ b/tests/ui/explicit_auto_deref.stderr @@ -0,0 +1,160 @@ +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:61:20 + | +LL | let _: &str = &*s; + | ^^ help: try this: `s` + | + = note: `-D clippy::explicit-auto-deref` implied by `-D warnings` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:65:12 + | +LL | f_str(&*s); + | ^^ help: try this: `s` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:69:14 + | +LL | f_str_t(&*s, &*s); // Don't lint second param. + | ^^ help: try this: `s` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:72:25 + | +LL | let _: &Box = &**b; + | ^^^ help: try this: `b` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:78:8 + | +LL | c(&*s); + | ^^ help: try this: `s` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:84:9 + | +LL | &**x + | ^^^^ help: try this: `x` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:88:11 + | +LL | { &**x } + | ^^^^ help: try this: `x` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:92:9 + | +LL | &**{ x } + | ^^^^^^^^ help: try this: `{ x }` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:96:9 + | +LL | &***x + | ^^^^^ help: try this: `x` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:113:13 + | +LL | f1(&*x); + | ^^ help: try this: `x` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:114:13 + | +LL | f2(&*x); + | ^^ help: try this: `x` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:115:13 + | +LL | f3(&*x); + | ^^ help: try this: `x` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:116:28 + | +LL | f4.callable_str()(&*x); + | ^^ help: try this: `x` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:117:13 + | +LL | f5(&*x); + | ^^ help: try this: `x` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:118:13 + | +LL | f6(&*x); + | ^^ help: try this: `x` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:119:28 + | +LL | f7.callable_str()(&*x); + | ^^ help: try this: `x` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:120:26 + | +LL | f8.callable_t()(&*x); + | ^^ help: try this: `x` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:121:13 + | +LL | f9(&*x); + | ^^ help: try this: `x` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:122:14 + | +LL | f10(&*x); + | ^^ help: try this: `x` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:123:27 + | +LL | f11.callable_t()(&*x); + | ^^ help: try this: `x` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:127:17 + | +LL | let _ = S1(&*s); + | ^^ help: try this: `s` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:132:22 + | +LL | let _ = S2 { s: &*s }; + | ^^ help: try this: `s` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:148:30 + | +LL | let _ = Self::S1(&**s); + | ^^^^ help: try this: `s` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:149:35 + | +LL | let _ = Self::S2 { s: &**s }; + | ^^^^ help: try this: `s` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:152:21 + | +LL | let _ = E1::S1(&*s); + | ^^ help: try this: `s` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:153:26 + | +LL | let _ = E1::S2 { s: &*s }; + | ^^ help: try this: `s` + +error: aborting due to 26 previous errors + diff --git a/tests/ui/explicit_deref_methods.fixed b/tests/ui/explicit_deref_methods.fixed index 92f27e68549a..523cae183ee6 100644 --- a/tests/ui/explicit_deref_methods.fixed +++ b/tests/ui/explicit_deref_methods.fixed @@ -4,7 +4,8 @@ unused_variables, clippy::clone_double_ref, clippy::needless_borrow, - clippy::borrow_deref_ref + clippy::borrow_deref_ref, + clippy::explicit_auto_deref )] #![warn(clippy::explicit_deref_methods)] diff --git a/tests/ui/explicit_deref_methods.rs b/tests/ui/explicit_deref_methods.rs index d118607f992b..0bbc1ae57cdf 100644 --- a/tests/ui/explicit_deref_methods.rs +++ b/tests/ui/explicit_deref_methods.rs @@ -4,7 +4,8 @@ unused_variables, clippy::clone_double_ref, clippy::needless_borrow, - clippy::borrow_deref_ref + clippy::borrow_deref_ref, + clippy::explicit_auto_deref )] #![warn(clippy::explicit_deref_methods)] diff --git a/tests/ui/explicit_deref_methods.stderr b/tests/ui/explicit_deref_methods.stderr index 8e8b358972be..4b10ed1377b0 100644 --- a/tests/ui/explicit_deref_methods.stderr +++ b/tests/ui/explicit_deref_methods.stderr @@ -1,5 +1,5 @@ error: explicit `deref` method call - --> $DIR/explicit_deref_methods.rs:35:19 + --> $DIR/explicit_deref_methods.rs:36:19 | LL | let b: &str = a.deref(); | ^^^^^^^^^ help: try this: `&*a` @@ -7,67 +7,67 @@ LL | let b: &str = a.deref(); = note: `-D clippy::explicit-deref-methods` implied by `-D warnings` error: explicit `deref_mut` method call - --> $DIR/explicit_deref_methods.rs:37:23 + --> $DIR/explicit_deref_methods.rs:38:23 | LL | let b: &mut str = a.deref_mut(); | ^^^^^^^^^^^^^ help: try this: `&mut **a` error: explicit `deref` method call - --> $DIR/explicit_deref_methods.rs:40:39 + --> $DIR/explicit_deref_methods.rs:41:39 | LL | let b: String = format!("{}, {}", a.deref(), a.deref()); | ^^^^^^^^^ help: try this: `&*a` error: explicit `deref` method call - --> $DIR/explicit_deref_methods.rs:40:50 + --> $DIR/explicit_deref_methods.rs:41:50 | LL | let b: String = format!("{}, {}", a.deref(), a.deref()); | ^^^^^^^^^ help: try this: `&*a` error: explicit `deref` method call - --> $DIR/explicit_deref_methods.rs:42:20 + --> $DIR/explicit_deref_methods.rs:43:20 | LL | println!("{}", a.deref()); | ^^^^^^^^^ help: try this: `&*a` error: explicit `deref` method call - --> $DIR/explicit_deref_methods.rs:45:11 + --> $DIR/explicit_deref_methods.rs:46:11 | LL | match a.deref() { | ^^^^^^^^^ help: try this: `&*a` error: explicit `deref` method call - --> $DIR/explicit_deref_methods.rs:49:28 + --> $DIR/explicit_deref_methods.rs:50:28 | LL | let b: String = concat(a.deref()); | ^^^^^^^^^ help: try this: `&*a` error: explicit `deref` method call - --> $DIR/explicit_deref_methods.rs:51:13 + --> $DIR/explicit_deref_methods.rs:52:13 | LL | let b = just_return(a).deref(); | ^^^^^^^^^^^^^^^^^^^^^^ help: try this: `just_return(a)` error: explicit `deref` method call - --> $DIR/explicit_deref_methods.rs:53:28 + --> $DIR/explicit_deref_methods.rs:54:28 | LL | let b: String = concat(just_return(a).deref()); | ^^^^^^^^^^^^^^^^^^^^^^ help: try this: `just_return(a)` error: explicit `deref` method call - --> $DIR/explicit_deref_methods.rs:55:19 + --> $DIR/explicit_deref_methods.rs:56:19 | LL | let b: &str = a.deref().deref(); | ^^^^^^^^^^^^^^^^^ help: try this: `&**a` error: explicit `deref` method call - --> $DIR/explicit_deref_methods.rs:58:13 + --> $DIR/explicit_deref_methods.rs:59:13 | LL | let b = opt_a.unwrap().deref(); | ^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&*opt_a.unwrap()` error: explicit `deref` method call - --> $DIR/explicit_deref_methods.rs:84:31 + --> $DIR/explicit_deref_methods.rs:85:31 | LL | let b: &str = expr_deref!(a.deref()); | ^^^^^^^^^ help: try this: `&*a` diff --git a/tests/ui/useless_asref.fixed b/tests/ui/useless_asref.fixed index e431661d180d..90cb8945e77f 100644 --- a/tests/ui/useless_asref.fixed +++ b/tests/ui/useless_asref.fixed @@ -1,6 +1,7 @@ // run-rustfix #![deny(clippy::useless_asref)] +#![allow(clippy::explicit_auto_deref)] use std::fmt::Debug; diff --git a/tests/ui/useless_asref.rs b/tests/ui/useless_asref.rs index 6ae931d7aa48..cb9f8ae5909a 100644 --- a/tests/ui/useless_asref.rs +++ b/tests/ui/useless_asref.rs @@ -1,6 +1,7 @@ // run-rustfix #![deny(clippy::useless_asref)] +#![allow(clippy::explicit_auto_deref)] use std::fmt::Debug; diff --git a/tests/ui/useless_asref.stderr b/tests/ui/useless_asref.stderr index 5876b54aca8f..b21c67bb3645 100644 --- a/tests/ui/useless_asref.stderr +++ b/tests/ui/useless_asref.stderr @@ -1,5 +1,5 @@ error: this call to `as_ref` does nothing - --> $DIR/useless_asref.rs:43:18 + --> $DIR/useless_asref.rs:44:18 | LL | foo_rstr(rstr.as_ref()); | ^^^^^^^^^^^^^ help: try this: `rstr` @@ -11,61 +11,61 @@ LL | #![deny(clippy::useless_asref)] | ^^^^^^^^^^^^^^^^^^^^^ error: this call to `as_ref` does nothing - --> $DIR/useless_asref.rs:45:20 + --> $DIR/useless_asref.rs:46:20 | LL | foo_rslice(rslice.as_ref()); | ^^^^^^^^^^^^^^^ help: try this: `rslice` error: this call to `as_mut` does nothing - --> $DIR/useless_asref.rs:49:21 + --> $DIR/useless_asref.rs:50:21 | LL | foo_mrslice(mrslice.as_mut()); | ^^^^^^^^^^^^^^^^ help: try this: `mrslice` error: this call to `as_ref` does nothing - --> $DIR/useless_asref.rs:51:20 + --> $DIR/useless_asref.rs:52:20 | LL | foo_rslice(mrslice.as_ref()); | ^^^^^^^^^^^^^^^^ help: try this: `mrslice` error: this call to `as_ref` does nothing - --> $DIR/useless_asref.rs:58:20 + --> $DIR/useless_asref.rs:59:20 | LL | foo_rslice(rrrrrslice.as_ref()); | ^^^^^^^^^^^^^^^^^^^ help: try this: `rrrrrslice` error: this call to `as_ref` does nothing - --> $DIR/useless_asref.rs:60:18 + --> $DIR/useless_asref.rs:61:18 | LL | foo_rstr(rrrrrstr.as_ref()); | ^^^^^^^^^^^^^^^^^ help: try this: `rrrrrstr` error: this call to `as_mut` does nothing - --> $DIR/useless_asref.rs:65:21 + --> $DIR/useless_asref.rs:66:21 | LL | foo_mrslice(mrrrrrslice.as_mut()); | ^^^^^^^^^^^^^^^^^^^^ help: try this: `mrrrrrslice` error: this call to `as_ref` does nothing - --> $DIR/useless_asref.rs:67:20 + --> $DIR/useless_asref.rs:68:20 | LL | foo_rslice(mrrrrrslice.as_ref()); | ^^^^^^^^^^^^^^^^^^^^ help: try this: `mrrrrrslice` error: this call to `as_ref` does nothing - --> $DIR/useless_asref.rs:71:16 + --> $DIR/useless_asref.rs:72:16 | LL | foo_rrrrmr((&&&&MoreRef).as_ref()); | ^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&&&&MoreRef)` error: this call to `as_mut` does nothing - --> $DIR/useless_asref.rs:121:13 + --> $DIR/useless_asref.rs:122:13 | LL | foo_mrt(mrt.as_mut()); | ^^^^^^^^^^^^ help: try this: `mrt` error: this call to `as_ref` does nothing - --> $DIR/useless_asref.rs:123:12 + --> $DIR/useless_asref.rs:124:12 | LL | foo_rt(mrt.as_ref()); | ^^^^^^^^^^^^ help: try this: `mrt` From ee532c02223059a5f6105b9ce423556032f1b487 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Thu, 27 Jan 2022 10:17:14 -0500 Subject: [PATCH 03/14] Don't lint `explicit_auto_deref` on reborrows --- clippy_lints/src/dereference.rs | 38 +++++++++++++++++-- tests/ui/borrow_deref_ref_unfixable.rs | 2 +- tests/ui/explicit_auto_deref.fixed | 8 +++- tests/ui/explicit_auto_deref.rs | 8 +++- tests/ui/explicit_auto_deref.stderr | 52 +++++++++++++------------- 5 files changed, 76 insertions(+), 32 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 77518933a230..dc51da262697 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -183,6 +183,10 @@ enum State { deref_span: Span, deref_hir_id: HirId, }, + Reborrow { + deref_span: Span, + deref_hir_id: HirId, + }, Borrow, } @@ -395,10 +399,38 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { )); }, (Some((State::Borrow, 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, + )); + } else { + self.state = Some(( + State::ExplicitDeref { + deref_span: expr.span, + deref_hir_id: expr.hir_id, + }, + data, + )); + } + }, + ( + Some(( + State::Reborrow { + deref_span, + deref_hir_id, + }, + data, + )), + RefOp::Deref, + ) => { self.state = Some(( State::ExplicitDeref { - deref_span: expr.span, - deref_hir_id: expr.hir_id, + deref_span, + deref_hir_id, }, data, )); @@ -959,7 +991,7 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data }, ); }, - State::Borrow => (), + State::Borrow | State::Reborrow { .. } => (), } } diff --git a/tests/ui/borrow_deref_ref_unfixable.rs b/tests/ui/borrow_deref_ref_unfixable.rs index b4e93774436a..a8e2bbfef0f5 100644 --- a/tests/ui/borrow_deref_ref_unfixable.rs +++ b/tests/ui/borrow_deref_ref_unfixable.rs @@ -1,4 +1,4 @@ -#![allow(dead_code, unused_variables, clippy::explicit_auto_deref)] +#![allow(dead_code, unused_variables)] fn main() {} diff --git a/tests/ui/explicit_auto_deref.fixed b/tests/ui/explicit_auto_deref.fixed index 04039ca31541..0d3f8b61afc2 100644 --- a/tests/ui/explicit_auto_deref.fixed +++ b/tests/ui/explicit_auto_deref.fixed @@ -8,7 +8,8 @@ clippy::needless_borrow, clippy::ptr_arg, clippy::redundant_field_names, - clippy::too_many_arguments + clippy::too_many_arguments, + clippy::borrow_deref_ref )] trait CallableStr { @@ -48,6 +49,7 @@ impl CallableT for i32 { } fn f_str(_: &str) {} +fn f_string(_: &String) {} fn f_t(_: T) {} fn f_ref_t(_: &T) {} @@ -158,4 +160,8 @@ fn main() { } let _ = E2::S1(&*s); // Don't lint. Inferred type would change. let _ = E2::S2 { s: &*s }; // Don't lint. Inferred type would change. + + let ref_s = &s; + let _: &String = &*ref_s; // Don't lint reborrow. + f_string(&*ref_s); // Don't lint reborrow. } diff --git a/tests/ui/explicit_auto_deref.rs b/tests/ui/explicit_auto_deref.rs index 1cc77c1b08f9..31c268bcdece 100644 --- a/tests/ui/explicit_auto_deref.rs +++ b/tests/ui/explicit_auto_deref.rs @@ -8,7 +8,8 @@ clippy::needless_borrow, clippy::ptr_arg, clippy::redundant_field_names, - clippy::too_many_arguments + clippy::too_many_arguments, + clippy::borrow_deref_ref )] trait CallableStr { @@ -48,6 +49,7 @@ impl CallableT for i32 { } fn f_str(_: &str) {} +fn f_string(_: &String) {} fn f_t(_: T) {} fn f_ref_t(_: &T) {} @@ -158,4 +160,8 @@ fn main() { } let _ = E2::S1(&*s); // Don't lint. Inferred type would change. let _ = E2::S2 { s: &*s }; // Don't lint. Inferred type would change. + + let ref_s = &s; + let _: &String = &*ref_s; // Don't lint reborrow. + f_string(&*ref_s); // Don't lint reborrow. } diff --git a/tests/ui/explicit_auto_deref.stderr b/tests/ui/explicit_auto_deref.stderr index f3cceb6b889d..b53b4b4a200c 100644 --- a/tests/ui/explicit_auto_deref.stderr +++ b/tests/ui/explicit_auto_deref.stderr @@ -1,5 +1,5 @@ error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:61:20 + --> $DIR/explicit_auto_deref.rs:63:20 | LL | let _: &str = &*s; | ^^ help: try this: `s` @@ -7,151 +7,151 @@ LL | let _: &str = &*s; = note: `-D clippy::explicit-auto-deref` implied by `-D warnings` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:65:12 + --> $DIR/explicit_auto_deref.rs:67:12 | LL | f_str(&*s); | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:69:14 + --> $DIR/explicit_auto_deref.rs:71:14 | LL | f_str_t(&*s, &*s); // Don't lint second param. | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:72:25 + --> $DIR/explicit_auto_deref.rs:74:25 | LL | let _: &Box = &**b; | ^^^ help: try this: `b` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:78:8 + --> $DIR/explicit_auto_deref.rs:80:8 | LL | c(&*s); | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:84:9 + --> $DIR/explicit_auto_deref.rs:86:9 | LL | &**x | ^^^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:88:11 + --> $DIR/explicit_auto_deref.rs:90:11 | LL | { &**x } | ^^^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:92:9 + --> $DIR/explicit_auto_deref.rs:94:9 | LL | &**{ x } | ^^^^^^^^ help: try this: `{ x }` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:96:9 + --> $DIR/explicit_auto_deref.rs:98:9 | LL | &***x | ^^^^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:113:13 + --> $DIR/explicit_auto_deref.rs:115:13 | LL | f1(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:114:13 + --> $DIR/explicit_auto_deref.rs:116:13 | LL | f2(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:115:13 + --> $DIR/explicit_auto_deref.rs:117:13 | LL | f3(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:116:28 + --> $DIR/explicit_auto_deref.rs:118:28 | LL | f4.callable_str()(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:117:13 + --> $DIR/explicit_auto_deref.rs:119:13 | LL | f5(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:118:13 + --> $DIR/explicit_auto_deref.rs:120:13 | LL | f6(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:119:28 + --> $DIR/explicit_auto_deref.rs:121:28 | LL | f7.callable_str()(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:120:26 + --> $DIR/explicit_auto_deref.rs:122:26 | LL | f8.callable_t()(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:121:13 + --> $DIR/explicit_auto_deref.rs:123:13 | LL | f9(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:122:14 + --> $DIR/explicit_auto_deref.rs:124:14 | LL | f10(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:123:27 + --> $DIR/explicit_auto_deref.rs:125:27 | LL | f11.callable_t()(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:127:17 + --> $DIR/explicit_auto_deref.rs:129:17 | LL | let _ = S1(&*s); | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:132:22 + --> $DIR/explicit_auto_deref.rs:134:22 | LL | let _ = S2 { s: &*s }; | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:148:30 + --> $DIR/explicit_auto_deref.rs:150:30 | LL | let _ = Self::S1(&**s); | ^^^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:149:35 + --> $DIR/explicit_auto_deref.rs:151:35 | LL | let _ = Self::S2 { s: &**s }; | ^^^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:152:21 + --> $DIR/explicit_auto_deref.rs:154:21 | LL | let _ = E1::S1(&*s); | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:153:26 + --> $DIR/explicit_auto_deref.rs:155:26 | LL | let _ = E1::S2 { s: &*s }; | ^^ help: try this: `s` From a187d6412bbcf52f2e0cdcb271b3e453520c2840 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Thu, 27 Jan 2022 20:03:50 -0500 Subject: [PATCH 04/14] Merge different parent walking loops in `dereference.rs` `needless_borrow` will now walk further to find the target type. --- clippy_lints/src/dereference.rs | 443 +++++++++++++++----------------- tests/ui/needless_borrow.fixed | 13 +- tests/ui/needless_borrow.rs | 13 +- tests/ui/needless_borrow.stderr | 18 +- 4 files changed, 249 insertions(+), 238 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index dc51da262697..bb0b04b6366d 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -2,15 +2,13 @@ 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, get_parent_node, is_lint_allowed, path_to_local, peel_hir_ty_refs, walk_to_expr_usage, -}; +use clippy_utils::{get_parent_expr, get_parent_node, is_lint_allowed, 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::{ - self as hir, BindingAnnotation, Body, BodyId, BorrowKind, Destination, Expr, ExprKind, FnRetTy, GenericArg, HirId, - ImplItem, ImplItemKind, Item, ItemKind, Local, MatchSource, Mutability, Node, Pat, PatKind, Path, QPath, TraitItem, + self as hir, BindingAnnotation, Body, BodyId, BorrowKind, Expr, ExprKind, FnRetTy, GenericArg, HirId, ImplItem, + ImplItemKind, Item, ItemKind, Local, MatchSource, Mutability, Node, Pat, PatKind, Path, QPath, TraitItem, TraitItemKind, TyKind, UnOp, }; use rustc_lint::{LateContext, LateLintPass}; @@ -268,8 +266,9 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { )); }, RefOp::AddrOf => { + let (stability, adjustments) = walk_parents(cx, expr); // Find the number of times the borrow is auto-derefed. - let mut iter = find_adjustments(cx.tcx, typeck, expr).iter(); + let mut iter = adjustments.iter(); let mut deref_count = 0usize; let next_adjust = loop { match iter.next() { @@ -316,8 +315,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { } else if let Some(&Adjust::Borrow(AutoBorrow::Ref(_, mutability))) = next_adjust.map(|a| &a.kind) { - if matches!(mutability, AutoBorrowMutability::Mut { .. }) - && !is_auto_reborrow_position(parent) + if matches!(mutability, AutoBorrowMutability::Mut { .. }) && !stability.is_reborrow_stable() { (3, 0, deref_msg) } else { @@ -341,7 +339,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { hir_id: expr.hir_id, }, )); - } else if is_stable_auto_deref_position(cx, expr) { + } else if stability.is_deref_stable() { self.state = Some(( State::Borrow, StateData { @@ -614,14 +612,122 @@ fn is_linted_explicit_deref_position(parent: Option>, child_id: HirId, } } -/// Checks if the given expression is in a position which can be auto-reborrowed. -/// Note: This is only correct assuming auto-deref is already occurring. -fn is_auto_reborrow_position(parent: Option>) -> bool { - match parent { - Some(Node::Expr(parent)) => matches!(parent.kind, ExprKind::MethodCall(..) | ExprKind::Call(..)), - Some(Node::Local(_)) => true, - _ => false, +/// How stable the result of auto-deref is. +#[derive(Clone, Copy)] +enum AutoDerefStability { + /// Auto-deref will always choose the same type. + Deref, + /// Auto-deref will always reborrow a reference. + Reborrow, + /// Auto-deref will not occur, or it may select a different type. + None, +} +impl AutoDerefStability { + fn is_deref_stable(self) -> bool { + matches!(self, Self::Deref) } + + fn is_reborrow_stable(self) -> bool { + matches!(self, Self::Deref | Self::Reborrow) + } +} + +/// 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. +fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (AutoDerefStability, &'tcx [Adjustment<'tcx>]) { + let mut adjustments = [].as_slice(); + let stability = walk_to_expr_usage(cx, e, &mut |node, child_id| { + // LocalTableInContext returns the wrong lifetime, so go use `expr_adjustments` instead. + if adjustments.is_empty() && let Node::Expr(e) = cx.tcx.hir().get(child_id) { + adjustments = cx.typeck_results().expr_adjustments(e); + } + match node { + Node::Local(Local { ty: Some(ty), .. }) => Some(binding_ty_auto_deref_stability(ty)), + Node::Item(&Item { + kind: ItemKind::Static(..) | ItemKind::Const(..), + .. + }) + | Node::TraitItem(&TraitItem { + kind: TraitItemKind::Const(..), + .. + }) + | Node::ImplItem(&ImplItem { + kind: ImplItemKind::Const(..), + .. + }) => Some(AutoDerefStability::Deref), + + Node::Item(&Item { + kind: ItemKind::Fn(..), + def_id, + .. + }) + | Node::TraitItem(&TraitItem { + kind: TraitItemKind::Fn(..), + def_id, + .. + }) + | Node::ImplItem(&ImplItem { + kind: ImplItemKind::Fn(..), + def_id, + .. + }) => { + let output = cx.tcx.fn_sig(def_id.to_def_id()).skip_binder().output(); + Some(if output.has_placeholders() || output.has_opaque_types() { + AutoDerefStability::Reborrow + } else { + AutoDerefStability::Deref + }) + }, + + Node::Expr(e) => match e.kind { + ExprKind::Ret(_) => { + let output = cx + .tcx + .fn_sig(cx.tcx.hir().body_owner_def_id(cx.enclosing_body.unwrap())) + .skip_binder() + .output(); + Some(if output.has_placeholders() || output.has_opaque_types() { + AutoDerefStability::Reborrow + } else { + AutoDerefStability::Deref + }) + }, + ExprKind::Call(func, args) => args + .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), + None => param_auto_deref_stability(ty.skip_binder()), + }), + ExprKind::MethodCall(_, [_, args @ ..], _) => { + let id = cx.typeck_results().type_dependent_def_id(e.hir_id).unwrap(); + args.iter().position(|arg| arg.hir_id == child_id).map(|i| { + let arg = cx.tcx.fn_sig(id).skip_binder().inputs()[i + 1]; + param_auto_deref_stability(arg) + }) + }, + ExprKind::MethodCall(..) => Some(AutoDerefStability::Reborrow), + ExprKind::Struct(path, fields, _) => { + let variant = variant_of_res(cx, cx.qpath_res(path, e.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))) + }, + _ => None, + }, + _ => None, + } + }) + .unwrap_or(AutoDerefStability::None); + (stability, adjustments) } /// Checks if the given expression is a position which can auto-borrow. @@ -638,140 +744,7 @@ fn is_auto_borrow_position(parent: Option>, child_id: HirId) -> bool { } } -/// Adjustments are sometimes made in the parent block rather than the expression itself. -fn find_adjustments<'tcx>( - tcx: TyCtxt<'tcx>, - typeck: &'tcx TypeckResults<'tcx>, - expr: &'tcx Expr<'tcx>, -) -> &'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 &[], - } - } -} - -// Checks if the expression for the given id occurs in a position which auto dereferencing applies. -// Note that the target type must not be inferred in a way that may cause auto-deref to select a -// different type, nor may the position be the result of a macro expansion. -// -// e.g. the following should not linted -// macro_rules! foo { ($e:expr) => { let x: &str = $e; }} -// foo!(&*String::new()); -// fn foo(_: &T) {} -// foo(&*String::new()) -fn is_stable_auto_deref_position<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> bool { - walk_to_expr_usage(cx, e, |node, child_id| match node { - Node::Local(&Local { ty: Some(ty), .. }) => Some(is_binding_ty_auto_deref_stable(ty)), - Node::Item(&Item { - kind: ItemKind::Static(..) | ItemKind::Const(..), - .. - }) - | Node::TraitItem(&TraitItem { - kind: TraitItemKind::Const(..), - .. - }) - | Node::ImplItem(&ImplItem { - kind: ImplItemKind::Const(..), - .. - }) => Some(true), - - Node::Item(&Item { - kind: ItemKind::Fn(..), - def_id, - .. - }) - | Node::TraitItem(&TraitItem { - kind: TraitItemKind::Fn(..), - def_id, - .. - }) - | Node::ImplItem(&ImplItem { - kind: ImplItemKind::Fn(..), - def_id, - .. - }) => { - let output = cx.tcx.fn_sig(def_id.to_def_id()).skip_binder().output(); - Some(!(output.has_placeholders() || output.has_opaque_types())) - }, - - Node::Expr(e) => match e.kind { - ExprKind::Ret(_) => { - let output = cx - .tcx - .fn_sig(cx.tcx.hir().body_owner_def_id(cx.enclosing_body.unwrap())) - .skip_binder() - .output(); - Some(!(output.has_placeholders() || output.has_opaque_types())) - }, - ExprKind::Call(func, args) => Some( - args.iter() - .position(|arg| arg.hir_id == child_id) - .zip(expr_sig(cx, func)) - .and_then(|(i, sig)| sig.input_with_hir(i)) - .map_or(false, |(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) => is_binding_ty_auto_deref_stable(ty), - None => is_param_auto_deref_stable(ty.skip_binder()), - }), - ), - ExprKind::MethodCall(_, [_, args @ ..], _) => { - let id = cx.typeck_results().type_dependent_def_id(e.hir_id).unwrap(); - Some(args.iter().position(|arg| arg.hir_id == child_id).map_or(false, |i| { - let arg = cx.tcx.fn_sig(id).skip_binder().inputs()[i + 1]; - is_param_auto_deref_stable(arg) - })) - }, - ExprKind::Struct(path, fields, _) => { - let variant = variant_of_res(cx, cx.qpath_res(path, e.hir_id)); - Some( - 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_or(false, |field| is_param_auto_deref_stable(cx.tcx.type_of(field.did))), - ) - }, - _ => None, - }, - _ => None, - }) - .unwrap_or(false) -} - -// Checks whether auto-dereferencing any type into a binding of the given type will definitely -// produce the same result. +// Checks the stability of auto-deref when assigned to a binding with the given explicit type. // // e.g. // let x = Box::new(Box::new(0u32)); @@ -780,44 +753,49 @@ fn is_stable_auto_deref_position<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_> // // Here `y1` and `y2` would resolve to different types, so the type `&Box<_>` is not stable when // switching to auto-dereferencing. -fn is_binding_ty_auto_deref_stable(ty: &hir::Ty<'_>) -> bool { - let (ty, count) = peel_hir_ty_refs(ty); - if count != 1 { - return false; - } +fn binding_ty_auto_deref_stability(ty: &hir::Ty<'_>) -> AutoDerefStability { + let TyKind::Rptr(_, ty) = &ty.kind else { + return AutoDerefStability::None; + }; + let mut ty = ty; - match &ty.kind { - TyKind::Rptr(_, ty) => is_binding_ty_auto_deref_stable(ty.ty), - &TyKind::Path( - QPath::TypeRelative(_, path) - | QPath::Resolved( - _, - Path { - segments: [.., path], .. - }, - ), - ) => { - if let Some(args) = path.args { - args.args.iter().all(|arg| { - if let GenericArg::Type(ty) = arg { - !ty_contains_infer(ty) - } else { - true - } - }) - } else { - true - } - }, - TyKind::Slice(_) - | TyKind::Array(..) - | TyKind::BareFn(_) - | TyKind::Never - | TyKind::Tup(_) - | TyKind::Ptr(_) - | TyKind::TraitObject(..) - | TyKind::Path(_) => true, - TyKind::OpaqueDef(..) | TyKind::Infer | TyKind::Typeof(..) | TyKind::Err => false, + loop { + break match ty.ty.kind { + TyKind::Rptr(_, ref ref_ty) => { + ty = ref_ty; + continue; + }, + TyKind::Path( + QPath::TypeRelative(_, path) + | QPath::Resolved( + _, + Path { + segments: [.., path], .. + }, + ), + ) => { + if let Some(args) = path.args + && args.args.iter().any(|arg| match arg { + GenericArg::Infer(_) => true, + GenericArg::Type(ty) => ty_contains_infer(ty), + _ => false, + }) + { + AutoDerefStability::Reborrow + } else { + AutoDerefStability::Deref + } + }, + TyKind::Slice(_) + | TyKind::Array(..) + | TyKind::BareFn(_) + | TyKind::Never + | TyKind::Tup(_) + | TyKind::Ptr(_) + | TyKind::TraitObject(..) + | TyKind::Path(_) => AutoDerefStability::Deref, + TyKind::OpaqueDef(..) | TyKind::Infer | TyKind::Typeof(..) | TyKind::Err => AutoDerefStability::Reborrow, + }; } } @@ -846,59 +824,58 @@ fn ty_contains_infer(ty: &hir::Ty<'_>) -> bool { segments: [.., path], .. }, ), - ) => { - if let Some(args) = path.args { - args.args.iter().any(|arg| { - if let GenericArg::Type(ty) = arg { - ty_contains_infer(ty) - } else { - false - } - }) - } else { - false - } - }, + ) => path.args.map_or(false, |args| { + args.args.iter().any(|arg| match arg { + GenericArg::Infer(_) => true, + GenericArg::Type(ty) => ty_contains_infer(ty), + _ => false, + }) + }), TyKind::Path(_) | TyKind::OpaqueDef(..) | TyKind::Infer | TyKind::Typeof(_) | TyKind::Err => true, TyKind::Never | TyKind::TraitObject(..) => false, } } // Checks whether a type is stable when switching to auto dereferencing, -fn is_param_auto_deref_stable(ty: Ty<'_>) -> bool { - let (ty, count) = peel_mid_ty_refs(ty); - if count != 1 { - return false; - } +fn param_auto_deref_stability(ty: Ty<'_>) -> AutoDerefStability { + let ty::Ref(_, mut ty, _) = *ty.kind() else { + return AutoDerefStability::None; + }; - match ty.kind() { - ty::Bool - | ty::Char - | ty::Int(_) - | ty::Uint(_) - | ty::Float(_) - | ty::Foreign(_) - | ty::Str - | ty::Array(..) - | ty::Slice(..) - | ty::RawPtr(..) - | ty::FnDef(..) - | ty::FnPtr(_) - | ty::Closure(..) - | ty::Generator(..) - | ty::GeneratorWitness(..) - | ty::Never - | ty::Tuple(_) - | ty::Ref(..) - | ty::Projection(_) => true, - ty::Infer(_) - | ty::Error(_) - | ty::Param(_) - | ty::Bound(..) - | ty::Opaque(..) - | ty::Placeholder(_) - | ty::Dynamic(..) => false, - ty::Adt(..) => !(ty.has_placeholders() || ty.has_param_types_or_consts()), + loop { + break match *ty.kind() { + ty::Ref(_, ref_ty, _) => { + ty = ref_ty; + continue; + }, + ty::Bool + | ty::Char + | ty::Int(_) + | ty::Uint(_) + | ty::Float(_) + | ty::Foreign(_) + | ty::Str + | ty::Array(..) + | ty::Slice(..) + | ty::RawPtr(..) + | ty::FnDef(..) + | ty::FnPtr(_) + | ty::Closure(..) + | ty::Generator(..) + | ty::GeneratorWitness(..) + | ty::Never + | ty::Tuple(_) + | ty::Projection(_) => AutoDerefStability::Deref, + ty::Infer(_) + | ty::Error(_) + | ty::Param(_) + | ty::Bound(..) + | ty::Opaque(..) + | ty::Placeholder(_) + | ty::Dynamic(..) => AutoDerefStability::Reborrow, + ty::Adt(..) if ty.has_placeholders() || ty.has_param_types_or_consts() => AutoDerefStability::Reborrow, + ty::Adt(..) => AutoDerefStability::Deref, + }; } } diff --git a/tests/ui/needless_borrow.fixed b/tests/ui/needless_borrow.fixed index e7a483c05829..f48f2ae58dc1 100644 --- a/tests/ui/needless_borrow.fixed +++ b/tests/ui/needless_borrow.fixed @@ -62,7 +62,18 @@ fn main() { 0 => &mut x, _ => &mut *x, }; - + let y: &mut i32 = match 0 { + // Lint here. The type given above triggers auto-borrow. + 0 => x, + _ => &mut *x, + }; + fn ref_mut_i32(_: &mut i32) {} + ref_mut_i32(match 0 { + // Lint here. The type given above triggers auto-borrow. + 0 => x, + _ => &mut *x, + }); + // use 'x' after to make sure it's still usable in the fixed code. *x = 5; let s = String::new(); diff --git a/tests/ui/needless_borrow.rs b/tests/ui/needless_borrow.rs index 1d6bf46405a2..63515a821589 100644 --- a/tests/ui/needless_borrow.rs +++ b/tests/ui/needless_borrow.rs @@ -62,7 +62,18 @@ fn main() { 0 => &mut x, _ => &mut *x, }; - + let y: &mut i32 = match 0 { + // Lint here. The type given above triggers auto-borrow. + 0 => &mut x, + _ => &mut *x, + }; + fn ref_mut_i32(_: &mut i32) {} + ref_mut_i32(match 0 { + // Lint here. The type given above triggers auto-borrow. + 0 => &mut x, + _ => &mut *x, + }); + // use 'x' after to make sure it's still usable in the fixed code. *x = 5; let s = String::new(); diff --git a/tests/ui/needless_borrow.stderr b/tests/ui/needless_borrow.stderr index be59d8f546d2..cd23d9fd072a 100644 --- a/tests/ui/needless_borrow.stderr +++ b/tests/ui/needless_borrow.stderr @@ -84,17 +84,29 @@ error: this expression creates a reference which is immediately dereferenced by LL | let y: &mut i32 = &mut &mut x; | ^^^^^^^^^^^ help: change this to: `x` +error: this expression creates a reference which is immediately dereferenced by the compiler + --> $DIR/needless_borrow.rs:67:14 + | +LL | 0 => &mut x, + | ^^^^^^ help: change this to: `x` + +error: this expression creates a reference which is immediately dereferenced by the compiler + --> $DIR/needless_borrow.rs:73:14 + | +LL | 0 => &mut x, + | ^^^^^^ help: change this to: `x` + error: this expression borrows a value the compiler would automatically borrow - --> $DIR/needless_borrow.rs:74:13 + --> $DIR/needless_borrow.rs:85:13 | LL | let _ = (&x).0; | ^^^^ help: change this to: `x` error: this expression borrows a value the compiler would automatically borrow - --> $DIR/needless_borrow.rs:76:22 + --> $DIR/needless_borrow.rs:87:22 | LL | let _ = unsafe { (&*x).0 }; | ^^^^^ help: change this to: `(*x)` -error: aborting due to 16 previous errors +error: aborting due to 18 previous errors From 20ea26234a99eb94727b901c00a1f31de91e22b8 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 28 Jan 2022 09:12:34 -0500 Subject: [PATCH 05/14] Lint field accesses in `explicit_auto_deref` --- clippy_lints/src/dereference.rs | 46 +++++++++++++++++-- .../src/matches/redundant_pattern_match.rs | 4 +- tests/ui/explicit_auto_deref.fixed | 20 ++++++++ tests/ui/explicit_auto_deref.rs | 20 ++++++++ tests/ui/explicit_auto_deref.stderr | 14 +++++- 5 files changed, 98 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index bb0b04b6366d..f1915f16339e 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -15,7 +15,7 @@ use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability}; use rustc_middle::ty::{self, Ty, TyCtxt, TypeFoldable, TypeckResults}; use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::{symbol::sym, Span}; +use rustc_span::{symbol::sym, Span, Symbol}; declare_clippy_lint! { /// ### What it does @@ -181,6 +181,9 @@ enum State { deref_span: Span, deref_hir_id: HirId, }, + ExplicitDerefField { + name: Symbol, + }, Reborrow { deref_span: Span, deref_hir_id: HirId, @@ -243,8 +246,18 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { (None, kind) => { let parent = get_parent_node(cx.tcx, expr.hir_id); let expr_ty = typeck.expr_ty(expr); - match kind { + RefOp::Deref => { + if let Some(Node::Expr(e)) = parent + && let ExprKind::Field(_, name) = e.kind + && !ty_contains_field(typeck.expr_ty(sub_expr), name.name) + { + self.state = Some(( + State::ExplicitDerefField { name: name.name }, + StateData { span: expr.span, hir_id: expr.hir_id }, + )); + } + } RefOp::Method(target_mut) if !is_lint_allowed(cx, EXPLICIT_DEREF_METHODS, expr.hir_id) && is_linted_explicit_deref_position(parent, expr.hir_id, expr.span) => @@ -349,7 +362,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { )); } }, - _ => (), + RefOp::Method(..) => (), } }, ( @@ -436,6 +449,11 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { (state @ Some((State::ExplicitDeref { .. }, _)), RefOp::Deref) => { self.state = state; }, + (Some((State::ExplicitDerefField { name }, data)), RefOp::Deref) + if !ty_contains_field(typeck.expr_ty(sub_expr), name) => + { + self.state = Some((State::ExplicitDerefField { name }, data)); + }, (Some((state, data)), _) => report(cx, expr, state, data), } @@ -879,6 +897,14 @@ fn param_auto_deref_stability(ty: Ty<'_>) -> AutoDerefStability { } } +fn ty_contains_field(ty: Ty<'_>, name: Symbol) -> bool { + if let ty::Adt(adt, _) = *ty.kind() { + adt.is_struct() && adt.non_enum_variant().fields.iter().any(|f| f.name == name) + } else { + false + } +} + #[expect(clippy::needless_pass_by_value)] fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data: StateData) { match state { @@ -968,6 +994,20 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data }, ); }, + State::ExplicitDerefField { .. } => { + span_lint_hir_and_then( + cx, + EXPLICIT_AUTO_DEREF, + data.hir_id, + data.span, + "deref which would be done by auto-deref", + |diag| { + let mut app = Applicability::MachineApplicable; + let snip = snippet_with_context(cx, expr.span, data.span.ctxt(), "..", &mut app).0; + diag.span_suggestion(data.span, "try this", snip.into_owned(), app); + }, + ); + }, State::Borrow | State::Reborrow { .. } => (), } } diff --git a/clippy_lints/src/matches/redundant_pattern_match.rs b/clippy_lints/src/matches/redundant_pattern_match.rs index 0ea3f3b673b7..65cecd333f1c 100644 --- a/clippy_lints/src/matches/redundant_pattern_match.rs +++ b/clippy_lints/src/matches/redundant_pattern_match.rs @@ -362,9 +362,9 @@ fn find_good_method_for_match<'a>( .qpath_res(path_right, arms[1].pat.hir_id) .opt_def_id()?; let body_node_pair = if match_def_path(cx, left_id, expected_left) && match_def_path(cx, right_id, expected_right) { - (&(*arms[0].body).kind, &(*arms[1].body).kind) + (&arms[0].body.kind, &arms[1].body.kind) } else if match_def_path(cx, right_id, expected_left) && match_def_path(cx, right_id, expected_right) { - (&(*arms[1].body).kind, &(*arms[0].body).kind) + (&arms[1].body.kind, &arms[0].body.kind) } else { return None; }; diff --git a/tests/ui/explicit_auto_deref.fixed b/tests/ui/explicit_auto_deref.fixed index 0d3f8b61afc2..91d0379b70e7 100644 --- a/tests/ui/explicit_auto_deref.fixed +++ b/tests/ui/explicit_auto_deref.fixed @@ -164,4 +164,24 @@ fn main() { let ref_s = &s; let _: &String = &*ref_s; // Don't lint reborrow. f_string(&*ref_s); // Don't lint reborrow. + + struct S5 { + foo: u32, + } + let b = Box::new(Box::new(S5 { foo: 5 })); + let _ = b.foo; + let _ = b.foo; + let _ = b.foo; + + struct S6 { + foo: S5, + } + impl core::ops::Deref for S6 { + type Target = S5; + fn deref(&self) -> &Self::Target { + &self.foo + } + } + let s6 = S6 { foo: S5 { foo: 5 } }; + let _ = (*s6).foo; // Don't lint. `S6` also has a field named `foo` } diff --git a/tests/ui/explicit_auto_deref.rs b/tests/ui/explicit_auto_deref.rs index 31c268bcdece..e57553b2a992 100644 --- a/tests/ui/explicit_auto_deref.rs +++ b/tests/ui/explicit_auto_deref.rs @@ -164,4 +164,24 @@ fn main() { let ref_s = &s; let _: &String = &*ref_s; // Don't lint reborrow. f_string(&*ref_s); // Don't lint reborrow. + + struct S5 { + foo: u32, + } + let b = Box::new(Box::new(S5 { foo: 5 })); + let _ = b.foo; + let _ = (*b).foo; + let _ = (**b).foo; + + struct S6 { + foo: S5, + } + impl core::ops::Deref for S6 { + type Target = S5; + fn deref(&self) -> &Self::Target { + &self.foo + } + } + let s6 = S6 { foo: S5 { foo: 5 } }; + let _ = (*s6).foo; // Don't lint. `S6` also has a field named `foo` } diff --git a/tests/ui/explicit_auto_deref.stderr b/tests/ui/explicit_auto_deref.stderr index b53b4b4a200c..54f1a2cd8868 100644 --- a/tests/ui/explicit_auto_deref.stderr +++ b/tests/ui/explicit_auto_deref.stderr @@ -156,5 +156,17 @@ error: deref which would be done by auto-deref LL | let _ = E1::S2 { s: &*s }; | ^^ help: try this: `s` -error: aborting due to 26 previous errors +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:173:13 + | +LL | let _ = (*b).foo; + | ^^^^ help: try this: `b` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:174:13 + | +LL | let _ = (**b).foo; + | ^^^^^ help: try this: `b` + +error: aborting due to 28 previous errors From 442a68c64b177480da34fd7ee5349987ffd7f3d7 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 28 Jan 2022 10:56:20 -0500 Subject: [PATCH 06/14] Only check parent node once in `dereference.rs` --- clippy_lints/src/dereference.rs | 115 +++++++++++--------------------- 1 file changed, 40 insertions(+), 75 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index f1915f16339e..12c796bd1003 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -15,7 +15,7 @@ use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability}; use rustc_middle::ty::{self, Ty, TyCtxt, TypeFoldable, TypeckResults}; use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::{symbol::sym, Span, Symbol}; +use rustc_span::{symbol::sym, Span, Symbol, SyntaxContext}; declare_clippy_lint! { /// ### What it does @@ -244,23 +244,22 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { match (self.state.take(), kind) { (None, kind) => { - let parent = get_parent_node(cx.tcx, expr.hir_id); let expr_ty = typeck.expr_ty(expr); + let (position, parent_ctxt) = get_expr_position(cx, expr); match kind { RefOp::Deref => { - if let Some(Node::Expr(e)) = parent - && let ExprKind::Field(_, name) = e.kind - && !ty_contains_field(typeck.expr_ty(sub_expr), name.name) + if let Position::FieldAccess(name) = position + && !ty_contains_field(typeck.expr_ty(sub_expr), name) { self.state = Some(( - State::ExplicitDerefField { name: name.name }, + State::ExplicitDerefField { name }, StateData { span: expr.span, hir_id: expr.hir_id }, )); } } RefOp::Method(target_mut) if !is_lint_allowed(cx, EXPLICIT_DEREF_METHODS, expr.hir_id) - && is_linted_explicit_deref_position(parent, expr.hir_id, expr.span) => + && (position.lint_explicit_deref() || parent_ctxt != expr.span.ctxt()) => { self.state = Some(( State::DerefMethod { @@ -322,8 +321,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { "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 (required_refs, required_precedence, msg) = if is_auto_borrow_position(parent, expr.hir_id) - { + let (required_refs, required_precedence, msg) = if position.can_auto_borrow() { (1, PREC_POSTFIX, if deref_count == 1 { borrow_msg } else { deref_msg }) } else if let Some(&Adjust::Borrow(AutoBorrow::Ref(_, mutability))) = next_adjust.map(|a| &a.kind) @@ -573,60 +571,41 @@ fn deref_method_same_type<'tcx>(result_ty: Ty<'tcx>, arg_ty: Ty<'tcx>) -> bool { } } -// Checks whether the parent node is a suitable context for switching from a deref method to the -// deref operator. -fn is_linted_explicit_deref_position(parent: Option>, child_id: HirId, child_span: Span) -> bool { - let parent = match parent { - Some(Node::Expr(e)) if e.span.ctxt() == child_span.ctxt() => e, - _ => return true, - }; - match parent.kind { - // Leave deref calls in the middle of a method chain. - // e.g. x.deref().foo() - ExprKind::MethodCall(_, [self_arg, ..], _) if self_arg.hir_id == child_id => false, +#[derive(Clone, Copy)] +enum Position { + MethodReceiver, + FieldAccess(Symbol), + Callee, + Postfix, + Deref, + Other, +} +impl Position { + fn can_auto_borrow(self) -> bool { + matches!(self, Self::MethodReceiver | Self::FieldAccess(_) | Self::Callee) + } - // Leave deref calls resulting in a called function - // e.g. (x.deref())() - ExprKind::Call(func_expr, _) if func_expr.hir_id == child_id => false, + fn lint_explicit_deref(self) -> bool { + matches!(self, Self::Other) + } +} - // Makes an ugly suggestion - // e.g. *x.deref() => *&*x - ExprKind::Unary(UnOp::Deref, _) - // Postfix expressions would require parens - | ExprKind::Match(_, _, MatchSource::TryDesugar | MatchSource::AwaitDesugar) - | ExprKind::Field(..) - | ExprKind::Index(..) - | ExprKind::Err => false, - - ExprKind::Box(..) - | ExprKind::ConstBlock(..) - | ExprKind::Array(_) - | ExprKind::Call(..) - | ExprKind::MethodCall(..) - | ExprKind::Tup(..) - | ExprKind::Binary(..) - | ExprKind::Unary(..) - | ExprKind::Lit(..) - | ExprKind::Cast(..) - | ExprKind::Type(..) - | ExprKind::DropTemps(..) - | ExprKind::If(..) - | ExprKind::Loop(..) - | ExprKind::Match(..) - | ExprKind::Let(..) - | ExprKind::Closure{..} - | ExprKind::Block(..) - | ExprKind::Assign(..) - | ExprKind::AssignOp(..) - | ExprKind::Path(..) - | ExprKind::AddrOf(..) - | ExprKind::Break(..) - | ExprKind::Continue(..) - | ExprKind::Ret(..) - | ExprKind::InlineAsm(..) - | ExprKind::Struct(..) - | ExprKind::Repeat(..) - | ExprKind::Yield(..) => true, +/// Get which position an expression is in relative to it's parent. +fn get_expr_position(cx: &LateContext<'_>, e: &Expr<'_>) -> (Position, SyntaxContext) { + if let Some(Node::Expr(parent)) = get_parent_node(cx.tcx, e.hir_id) { + let pos = match parent.kind { + ExprKind::MethodCall(_, [self_arg, ..], _) if self_arg.hir_id == e.hir_id => Position::MethodReceiver, + ExprKind::Field(_, name) => Position::FieldAccess(name.name), + ExprKind::Call(f, _) if f.hir_id == e.hir_id => Position::Callee, + ExprKind::Unary(UnOp::Deref, _) => Position::Deref, + ExprKind::Match(_, _, MatchSource::TryDesugar | MatchSource::AwaitDesugar) | ExprKind::Index(..) => { + Position::Postfix + }, + _ => Position::Other, + }; + (pos, parent.span.ctxt()) + } else { + (Position::Other, SyntaxContext::root()) } } @@ -748,20 +727,6 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (AutoDerefSt (stability, adjustments) } -/// Checks if the given expression is a position which can auto-borrow. -fn is_auto_borrow_position(parent: Option>, child_id: HirId) -> bool { - if let Some(Node::Expr(parent)) = parent { - match parent.kind { - // ExprKind::MethodCall(_, [self_arg, ..], _) => self_arg.hir_id == child_id, - ExprKind::Field(..) => true, - ExprKind::Call(f, _) => f.hir_id == child_id, - _ => false, - } - } else { - false - } -} - // Checks the stability of auto-deref when assigned to a binding with the given explicit type. // // e.g. From 65bc6cb8bf1f1005ae02a0044f0f3027c702a807 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Fri, 28 Jan 2022 14:54:28 -0500 Subject: [PATCH 07/14] Lint `explicit_auto_deref` without a leading borrow --- clippy_lints/src/dereference.rs | 29 +++++++++++++++++++--- tests/ui/explicit_auto_deref.fixed | 14 +++++++++++ tests/ui/explicit_auto_deref.rs | 14 +++++++++++ tests/ui/explicit_auto_deref.stderr | 14 ++++++++++- tests/ui/needless_borrow_pat.rs | 2 +- tests/ui/ref_binding_to_reference.rs | 2 +- tests/ui/search_is_some_fixable_none.fixed | 2 +- tests/ui/search_is_some_fixable_none.rs | 2 +- tests/ui/search_is_some_fixable_some.fixed | 2 +- tests/ui/search_is_some_fixable_some.rs | 2 +- 10 files changed, 72 insertions(+), 11 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 12c796bd1003..32d1e26b74cf 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -246,6 +246,8 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { (None, kind) => { let expr_ty = typeck.expr_ty(expr); let (position, parent_ctxt) = get_expr_position(cx, expr); + let (stability, adjustments) = walk_parents(cx, expr); + match kind { RefOp::Deref => { if let Position::FieldAccess(name) = position @@ -255,6 +257,11 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { State::ExplicitDerefField { name }, StateData { span: expr.span, hir_id: expr.hir_id }, )); + } else if stability.is_deref_stable() { + self.state = Some(( + State::ExplicitDeref { deref_span: expr.span, deref_hir_id: expr.hir_id }, + StateData { span: expr.span, hir_id: expr.hir_id }, + )); } } RefOp::Method(target_mut) @@ -278,7 +285,6 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { )); }, RefOp::AddrOf => { - let (stability, adjustments) = walk_parents(cx, expr); // Find the number of times the borrow is auto-derefed. let mut iter = adjustments.iter(); let mut deref_count = 0usize; @@ -632,6 +638,7 @@ impl AutoDerefStability { /// 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<'_>) -> (AutoDerefStability, &'tcx [Adjustment<'tcx>]) { let mut adjustments = [].as_slice(); let stability = walk_to_expr_usage(cx, e, &mut |node, child_id| { @@ -643,16 +650,26 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (AutoDerefSt Node::Local(Local { ty: Some(ty), .. }) => Some(binding_ty_auto_deref_stability(ty)), Node::Item(&Item { kind: ItemKind::Static(..) | ItemKind::Const(..), + def_id, .. }) | Node::TraitItem(&TraitItem { kind: TraitItemKind::Const(..), + def_id, .. }) | Node::ImplItem(&ImplItem { kind: ImplItemKind::Const(..), + def_id, .. - }) => Some(AutoDerefStability::Deref), + }) => { + let ty = cx.tcx.type_of(def_id); + Some(if ty.is_ref() { + AutoDerefStability::None + } else { + AutoDerefStability::Deref + }) + }, Node::Item(&Item { kind: ItemKind::Fn(..), @@ -670,7 +687,9 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (AutoDerefSt .. }) => { let output = cx.tcx.fn_sig(def_id.to_def_id()).skip_binder().output(); - Some(if output.has_placeholders() || output.has_opaque_types() { + Some(if !output.is_ref() { + AutoDerefStability::None + } else if output.has_placeholders() || output.has_opaque_types() { AutoDerefStability::Reborrow } else { AutoDerefStability::Deref @@ -684,7 +703,9 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (AutoDerefSt .fn_sig(cx.tcx.hir().body_owner_def_id(cx.enclosing_body.unwrap())) .skip_binder() .output(); - Some(if output.has_placeholders() || output.has_opaque_types() { + Some(if !output.is_ref() { + AutoDerefStability::None + } else if output.has_placeholders() || output.has_opaque_types() { AutoDerefStability::Reborrow } else { AutoDerefStability::Deref diff --git a/tests/ui/explicit_auto_deref.fixed b/tests/ui/explicit_auto_deref.fixed index 91d0379b70e7..d20a58e53bd1 100644 --- a/tests/ui/explicit_auto_deref.fixed +++ b/tests/ui/explicit_auto_deref.fixed @@ -6,6 +6,7 @@ unused_braces, clippy::borrowed_box, clippy::needless_borrow, + clippy::needless_return, clippy::ptr_arg, clippy::redundant_field_names, clippy::too_many_arguments, @@ -184,4 +185,17 @@ fn main() { } let s6 = S6 { foo: S5 { foo: 5 } }; let _ = (*s6).foo; // Don't lint. `S6` also has a field named `foo` + + let ref_str = &"foo"; + let _ = f_str(ref_str); + let ref_ref_str = &ref_str; + let _ = f_str(ref_ref_str); + + fn _f5(x: &u32) -> u32 { + if true { + *x + } else { + return *x; + } + } } diff --git a/tests/ui/explicit_auto_deref.rs b/tests/ui/explicit_auto_deref.rs index e57553b2a992..f49bc8bb9f01 100644 --- a/tests/ui/explicit_auto_deref.rs +++ b/tests/ui/explicit_auto_deref.rs @@ -6,6 +6,7 @@ unused_braces, clippy::borrowed_box, clippy::needless_borrow, + clippy::needless_return, clippy::ptr_arg, clippy::redundant_field_names, clippy::too_many_arguments, @@ -184,4 +185,17 @@ fn main() { } let s6 = S6 { foo: S5 { foo: 5 } }; let _ = (*s6).foo; // Don't lint. `S6` also has a field named `foo` + + let ref_str = &"foo"; + let _ = f_str(*ref_str); + let ref_ref_str = &ref_str; + let _ = f_str(**ref_ref_str); + + fn _f5(x: &u32) -> u32 { + if true { + *x + } else { + return *x; + } + } } diff --git a/tests/ui/explicit_auto_deref.stderr b/tests/ui/explicit_auto_deref.stderr index 54f1a2cd8868..de8c40ce5be1 100644 --- a/tests/ui/explicit_auto_deref.stderr +++ b/tests/ui/explicit_auto_deref.stderr @@ -168,5 +168,17 @@ error: deref which would be done by auto-deref LL | let _ = (**b).foo; | ^^^^^ help: try this: `b` -error: aborting due to 28 previous errors +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:189:19 + | +LL | let _ = f_str(*ref_str); + | ^^^^^^^^ help: try this: `ref_str` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:191:19 + | +LL | let _ = f_str(**ref_ref_str); + | ^^^^^^^^^^^^^ help: try this: `ref_ref_str` + +error: aborting due to 30 previous errors diff --git a/tests/ui/needless_borrow_pat.rs b/tests/ui/needless_borrow_pat.rs index 04b6283da3c3..222e8e617995 100644 --- a/tests/ui/needless_borrow_pat.rs +++ b/tests/ui/needless_borrow_pat.rs @@ -1,7 +1,7 @@ // FIXME: run-rustfix waiting on multi-span suggestions #![warn(clippy::needless_borrow)] -#![allow(clippy::needless_borrowed_reference)] +#![allow(clippy::needless_borrowed_reference, clippy::explicit_auto_deref)] fn f1(_: &str) {} macro_rules! m1 { diff --git a/tests/ui/ref_binding_to_reference.rs b/tests/ui/ref_binding_to_reference.rs index 570ef406e4a9..c8d0e56b197f 100644 --- a/tests/ui/ref_binding_to_reference.rs +++ b/tests/ui/ref_binding_to_reference.rs @@ -2,7 +2,7 @@ #![feature(lint_reasons)] #![warn(clippy::ref_binding_to_reference)] -#![allow(clippy::needless_borrowed_reference)] +#![allow(clippy::needless_borrowed_reference, clippy::explicit_auto_deref)] fn f1(_: &str) {} macro_rules! m2 { diff --git a/tests/ui/search_is_some_fixable_none.fixed b/tests/ui/search_is_some_fixable_none.fixed index 6831fb2cf59e..5190c5304c75 100644 --- a/tests/ui/search_is_some_fixable_none.fixed +++ b/tests/ui/search_is_some_fixable_none.fixed @@ -1,5 +1,5 @@ // run-rustfix -#![allow(dead_code)] +#![allow(dead_code, clippy::explicit_auto_deref)] #![warn(clippy::search_is_some)] fn main() { diff --git a/tests/ui/search_is_some_fixable_none.rs b/tests/ui/search_is_some_fixable_none.rs index 767518ab0c0f..310d87333a93 100644 --- a/tests/ui/search_is_some_fixable_none.rs +++ b/tests/ui/search_is_some_fixable_none.rs @@ -1,5 +1,5 @@ // run-rustfix -#![allow(dead_code)] +#![allow(dead_code, clippy::explicit_auto_deref)] #![warn(clippy::search_is_some)] fn main() { diff --git a/tests/ui/search_is_some_fixable_some.fixed b/tests/ui/search_is_some_fixable_some.fixed index 7c940a2b069e..5a2aee465d1b 100644 --- a/tests/ui/search_is_some_fixable_some.fixed +++ b/tests/ui/search_is_some_fixable_some.fixed @@ -1,5 +1,5 @@ // run-rustfix -#![allow(dead_code)] +#![allow(dead_code, clippy::explicit_auto_deref)] #![warn(clippy::search_is_some)] fn main() { diff --git a/tests/ui/search_is_some_fixable_some.rs b/tests/ui/search_is_some_fixable_some.rs index 77fd52e4ce76..0e98ae18a217 100644 --- a/tests/ui/search_is_some_fixable_some.rs +++ b/tests/ui/search_is_some_fixable_some.rs @@ -1,5 +1,5 @@ // run-rustfix -#![allow(dead_code)] +#![allow(dead_code, clippy::explicit_auto_deref)] #![warn(clippy::search_is_some)] fn main() { From 0204b9535731cbaba1689b6d6ef44a93aef2dae3 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sat, 29 Jan 2022 09:36:46 -0500 Subject: [PATCH 08/14] Lint `explicit_auto_deref` immediately after `needless_borrow` --- clippy_lints/src/dereference.rs | 88 ++++++++++++++++++----------- tests/ui/explicit_auto_deref.fixed | 3 + tests/ui/explicit_auto_deref.rs | 3 + tests/ui/explicit_auto_deref.stderr | 14 ++++- 4 files changed, 73 insertions(+), 35 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 32d1e26b74cf..cac99c21e9e4 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -163,6 +163,14 @@ struct StateData { hir_id: HirId, } +struct DerefedBorrow { + count: usize, + required_precedence: i8, + msg: &'static str, + stability: AutoDerefStability, + position: Position, +} + enum State { // Any number of deref method calls. DerefMethod { @@ -172,11 +180,7 @@ enum State { /// The required mutability target_mut: Mutability, }, - DerefedBorrow { - count: usize, - required_precedence: i8, - msg: &'static str, - }, + DerefedBorrow(DerefedBorrow), ExplicitDeref { deref_span: Span, deref_hir_id: HirId, @@ -344,17 +348,16 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { if deref_count >= required_refs { self.state = Some(( - State::DerefedBorrow { + State::DerefedBorrow(DerefedBorrow { // One of the required refs is for the current borrow expression, the remaining ones // can't be removed without breaking the code. See earlier comment. count: deref_count - required_refs, required_precedence, msg, - }, - StateData { - span: expr.span, - hir_id: expr.hir_id, - }, + stability, + position, + }), + StateData { span: expr.span, hir_id: expr.hir_id }, )); } else if stability.is_deref_stable() { self.state = Some(( @@ -393,26 +396,47 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { data, )); }, - ( - Some(( - State::DerefedBorrow { - count, - required_precedence, - msg, - }, - data, - )), - RefOp::AddrOf, - ) if count != 0 => { + (Some((State::DerefedBorrow(state), data)), RefOp::AddrOf) if state.count != 0 => { self.state = Some(( - State::DerefedBorrow { - count: count - 1, - required_precedence, - msg, - }, + State::DerefedBorrow(DerefedBorrow { + count: state.count - 1, + ..state + }), data, )); }, + (Some((State::DerefedBorrow(state), data)), RefOp::AddrOf) => { + let stability = state.stability; + report(cx, expr, State::DerefedBorrow(state), data); + if stability.is_deref_stable() { + self.state = Some(( + State::Borrow, + StateData { + span: expr.span, + hir_id: expr.hir_id, + }, + )); + } + }, + (Some((State::DerefedBorrow(state), data)), RefOp::Deref) => { + let stability = state.stability; + let position = state.position; + report(cx, expr, State::DerefedBorrow(state), data); + if let Position::FieldAccess(name) = position + && !ty_contains_field(typeck.expr_ty(sub_expr), name) + { + self.state = Some(( + State::ExplicitDerefField { name }, + StateData { span: expr.span, hir_id: expr.hir_id }, + )); + } else if stability.is_deref_stable() { + self.state = Some(( + State::ExplicitDeref { deref_span: expr.span, deref_hir_id: expr.hir_id }, + StateData { span: expr.span, hir_id: expr.hir_id }, + )); + } + }, + (Some((State::Borrow, data)), RefOp::Deref) => { if typeck.expr_ty(sub_expr).is_ref() { self.state = Some(( @@ -942,15 +966,11 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data app, ); }, - State::DerefedBorrow { - required_precedence, - msg, - .. - } => { + State::DerefedBorrow(state) => { let mut app = Applicability::MachineApplicable; let snip = snippet_with_context(cx, expr.span, data.span.ctxt(), "..", &mut app).0; - span_lint_hir_and_then(cx, NEEDLESS_BORROW, data.hir_id, data.span, msg, |diag| { - let sugg = if required_precedence > expr.precedence().order() && !has_enclosing_paren(&snip) { + span_lint_hir_and_then(cx, NEEDLESS_BORROW, data.hir_id, data.span, state.msg, |diag| { + let sugg = if state.required_precedence > expr.precedence().order() && !has_enclosing_paren(&snip) { format!("({})", snip) } else { snip.into() diff --git a/tests/ui/explicit_auto_deref.fixed b/tests/ui/explicit_auto_deref.fixed index d20a58e53bd1..f534714bc659 100644 --- a/tests/ui/explicit_auto_deref.fixed +++ b/tests/ui/explicit_auto_deref.fixed @@ -198,4 +198,7 @@ fn main() { return *x; } } + + f_str(&&ref_str); // `needless_borrow` will suggest removing both references + f_str(&ref_str); // `needless_borrow` will suggest removing only one reference } diff --git a/tests/ui/explicit_auto_deref.rs b/tests/ui/explicit_auto_deref.rs index f49bc8bb9f01..4781962882bc 100644 --- a/tests/ui/explicit_auto_deref.rs +++ b/tests/ui/explicit_auto_deref.rs @@ -198,4 +198,7 @@ fn main() { return *x; } } + + f_str(&&*ref_str); // `needless_borrow` will suggest removing both references + f_str(&&**ref_str); // `needless_borrow` will suggest removing only one reference } diff --git a/tests/ui/explicit_auto_deref.stderr b/tests/ui/explicit_auto_deref.stderr index de8c40ce5be1..26357ffd96a0 100644 --- a/tests/ui/explicit_auto_deref.stderr +++ b/tests/ui/explicit_auto_deref.stderr @@ -180,5 +180,17 @@ error: deref which would be done by auto-deref LL | let _ = f_str(**ref_ref_str); | ^^^^^^^^^^^^^ help: try this: `ref_ref_str` -error: aborting due to 30 previous errors +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:201:13 + | +LL | f_str(&&*ref_str); // `needless_borrow` will suggest removing both references + | ^^^^^^^^ help: try this: `ref_str` + +error: deref which would be done by auto-deref + --> $DIR/explicit_auto_deref.rs:202:12 + | +LL | f_str(&&**ref_str); // `needless_borrow` will suggest removing only one reference + | ^^^^^^^^^^ help: try this: `ref_str` + +error: aborting due to 32 previous errors From 0b4ba734cbd6758948cb18210437b4768e139fa9 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sat, 29 Jan 2022 18:06:14 -0500 Subject: [PATCH 09/14] Refactor `dereference.rs` Merge `Position` and `AutoDerefStability` --- clippy_lints/src/dereference.rs | 174 ++++++++++++++-------------- tests/ui/explicit_auto_deref.fixed | 3 +- tests/ui/explicit_auto_deref.rs | 3 +- tests/ui/explicit_auto_deref.stderr | 64 +++++----- 4 files changed, 120 insertions(+), 124 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index cac99c21e9e4..3821beaea53b 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -2,7 +2,7 @@ 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, get_parent_node, is_lint_allowed, path_to_local, walk_to_expr_usage}; +use clippy_utils::{get_parent_expr, is_lint_allowed, 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; @@ -15,7 +15,7 @@ use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability}; use rustc_middle::ty::{self, Ty, TyCtxt, TypeFoldable, TypeckResults}; use rustc_session::{declare_tool_lint, impl_lint_pass}; -use rustc_span::{symbol::sym, Span, Symbol, SyntaxContext}; +use rustc_span::{symbol::sym, Span, Symbol}; declare_clippy_lint! { /// ### What it does @@ -167,7 +167,6 @@ struct DerefedBorrow { count: usize, required_precedence: i8, msg: &'static str, - stability: AutoDerefStability, position: Position, } @@ -249,8 +248,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { match (self.state.take(), kind) { (None, kind) => { let expr_ty = typeck.expr_ty(expr); - let (position, parent_ctxt) = get_expr_position(cx, expr); - let (stability, adjustments) = walk_parents(cx, expr); + let (position, adjustments) = walk_parents(cx, expr); match kind { RefOp::Deref => { @@ -261,7 +259,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { State::ExplicitDerefField { name }, StateData { span: expr.span, hir_id: expr.hir_id }, )); - } else if stability.is_deref_stable() { + } else if position.is_deref_stable() { self.state = Some(( State::ExplicitDeref { deref_span: expr.span, deref_hir_id: expr.hir_id }, StateData { span: expr.span, hir_id: expr.hir_id }, @@ -270,7 +268,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { } RefOp::Method(target_mut) if !is_lint_allowed(cx, EXPLICIT_DEREF_METHODS, expr.hir_id) - && (position.lint_explicit_deref() || parent_ctxt != expr.span.ctxt()) => + && position.lint_explicit_deref() => { self.state = Some(( State::DerefMethod { @@ -336,7 +334,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { } else if let Some(&Adjust::Borrow(AutoBorrow::Ref(_, mutability))) = next_adjust.map(|a| &a.kind) { - if matches!(mutability, AutoBorrowMutability::Mut { .. }) && !stability.is_reborrow_stable() + if matches!(mutability, AutoBorrowMutability::Mut { .. }) && !position.is_reborrow_stable() { (3, 0, deref_msg) } else { @@ -354,12 +352,11 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { count: deref_count - required_refs, required_precedence, msg, - stability, position, }), StateData { span: expr.span, hir_id: expr.hir_id }, )); - } else if stability.is_deref_stable() { + } else if position.is_deref_stable() { self.state = Some(( State::Borrow, StateData { @@ -406,9 +403,9 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { )); }, (Some((State::DerefedBorrow(state), data)), RefOp::AddrOf) => { - let stability = state.stability; + let position = state.position; report(cx, expr, State::DerefedBorrow(state), data); - if stability.is_deref_stable() { + if position.is_deref_stable() { self.state = Some(( State::Borrow, StateData { @@ -419,7 +416,6 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { } }, (Some((State::DerefedBorrow(state), data)), RefOp::Deref) => { - let stability = state.stability; let position = state.position; report(cx, expr, State::DerefedBorrow(state), data); if let Position::FieldAccess(name) = position @@ -429,7 +425,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { State::ExplicitDerefField { name }, StateData { span: expr.span, hir_id: expr.hir_id }, )); - } else if stability.is_deref_stable() { + } else if position.is_deref_stable() { self.state = Some(( State::ExplicitDeref { deref_span: expr.span, deref_hir_id: expr.hir_id }, StateData { span: expr.span, hir_id: expr.hir_id }, @@ -601,61 +597,35 @@ fn deref_method_same_type<'tcx>(result_ty: Ty<'tcx>, arg_ty: Ty<'tcx>) -> bool { } } +/// The position of an expression relative to it's parent. #[derive(Clone, Copy)] enum Position { MethodReceiver, - FieldAccess(Symbol), Callee, + FieldAccess(Symbol), Postfix, Deref, + /// Any other location which will trigger auto-deref to a specific time. + DerefStable, + /// Any other location which will trigger auto-reborrowing. + ReborrowStable, Other, } impl Position { + fn is_deref_stable(self) -> bool { + matches!(self, Self::DerefStable) + } + + fn is_reborrow_stable(self) -> bool { + matches!(self, Self::DerefStable | Self::ReborrowStable) + } + fn can_auto_borrow(self) -> bool { matches!(self, Self::MethodReceiver | Self::FieldAccess(_) | Self::Callee) } fn lint_explicit_deref(self) -> bool { - matches!(self, Self::Other) - } -} - -/// Get which position an expression is in relative to it's parent. -fn get_expr_position(cx: &LateContext<'_>, e: &Expr<'_>) -> (Position, SyntaxContext) { - if let Some(Node::Expr(parent)) = get_parent_node(cx.tcx, e.hir_id) { - let pos = match parent.kind { - ExprKind::MethodCall(_, [self_arg, ..], _) if self_arg.hir_id == e.hir_id => Position::MethodReceiver, - ExprKind::Field(_, name) => Position::FieldAccess(name.name), - ExprKind::Call(f, _) if f.hir_id == e.hir_id => Position::Callee, - ExprKind::Unary(UnOp::Deref, _) => Position::Deref, - ExprKind::Match(_, _, MatchSource::TryDesugar | MatchSource::AwaitDesugar) | ExprKind::Index(..) => { - Position::Postfix - }, - _ => Position::Other, - }; - (pos, parent.span.ctxt()) - } else { - (Position::Other, SyntaxContext::root()) - } -} - -/// How stable the result of auto-deref is. -#[derive(Clone, Copy)] -enum AutoDerefStability { - /// Auto-deref will always choose the same type. - Deref, - /// Auto-deref will always reborrow a reference. - Reborrow, - /// Auto-deref will not occur, or it may select a different type. - None, -} -impl AutoDerefStability { - fn is_deref_stable(self) -> bool { - matches!(self, Self::Deref) - } - - fn is_reborrow_stable(self) -> bool { - matches!(self, Self::Deref | Self::Reborrow) + matches!(self, Self::Other | Self::DerefStable | Self::ReborrowStable) } } @@ -663,64 +633,73 @@ impl AutoDerefStability { /// 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<'_>) -> (AutoDerefStability, &'tcx [Adjustment<'tcx>]) { +fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, &'tcx [Adjustment<'tcx>]) { let mut adjustments = [].as_slice(); - let stability = walk_to_expr_usage(cx, e, &mut |node, child_id| { + let ctxt = e.span.ctxt(); + let position = walk_to_expr_usage(cx, e, &mut |parent, child_id| { // LocalTableInContext returns the wrong lifetime, so go use `expr_adjustments` instead. if adjustments.is_empty() && let Node::Expr(e) = cx.tcx.hir().get(child_id) { adjustments = cx.typeck_results().expr_adjustments(e); } - match node { - Node::Local(Local { ty: Some(ty), .. }) => Some(binding_ty_auto_deref_stability(ty)), + match parent { + Node::Local(Local { ty: Some(ty), span, .. }) if span.ctxt() == ctxt => { + Some(binding_ty_auto_deref_stability(ty)) + }, Node::Item(&Item { kind: ItemKind::Static(..) | ItemKind::Const(..), def_id, + span, .. }) | Node::TraitItem(&TraitItem { kind: TraitItemKind::Const(..), def_id, + span, .. }) | Node::ImplItem(&ImplItem { kind: ImplItemKind::Const(..), def_id, + span, .. - }) => { + }) if span.ctxt() == ctxt => { let ty = cx.tcx.type_of(def_id); Some(if ty.is_ref() { - AutoDerefStability::None + Position::Other } else { - AutoDerefStability::Deref + Position::DerefStable }) }, Node::Item(&Item { kind: ItemKind::Fn(..), def_id, + span, .. }) | Node::TraitItem(&TraitItem { kind: TraitItemKind::Fn(..), def_id, + span, .. }) | Node::ImplItem(&ImplItem { kind: ImplItemKind::Fn(..), def_id, + span, .. - }) => { + }) if span.ctxt() == ctxt => { let output = cx.tcx.fn_sig(def_id.to_def_id()).skip_binder().output(); Some(if !output.is_ref() { - AutoDerefStability::None + Position::Other } else if output.has_placeholders() || output.has_opaque_types() { - AutoDerefStability::Reborrow + Position::ReborrowStable } else { - AutoDerefStability::Deref + Position::DerefStable }) }, - Node::Expr(e) => match e.kind { + Node::Expr(parent) if parent.span.ctxt() == ctxt => match parent.kind { ExprKind::Ret(_) => { let output = cx .tcx @@ -728,13 +707,14 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (AutoDerefSt .skip_binder() .output(); Some(if !output.is_ref() { - AutoDerefStability::None + Position::Other } else if output.has_placeholders() || output.has_opaque_types() { - AutoDerefStability::Reborrow + Position::ReborrowStable } else { - AutoDerefStability::Deref + Position::DerefStable }) }, + ExprKind::Call(func, _) if func.hir_id == child_id => (child_id == e.hir_id).then(|| Position::Callee), ExprKind::Call(func, args) => args .iter() .position(|arg| arg.hir_id == child_id) @@ -746,16 +726,22 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (AutoDerefSt Some(ty) => binding_ty_auto_deref_stability(ty), None => param_auto_deref_stability(ty.skip_binder()), }), - ExprKind::MethodCall(_, [_, args @ ..], _) => { - let id = cx.typeck_results().type_dependent_def_id(e.hir_id).unwrap(); + ExprKind::MethodCall(_, 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| { - let arg = cx.tcx.fn_sig(id).skip_binder().inputs()[i + 1]; - param_auto_deref_stability(arg) + if i == 0 { + if e.hir_id == child_id { + Position::MethodReceiver + } else { + Position::ReborrowStable + } + } else { + param_auto_deref_stability(cx.tcx.fn_sig(id).skip_binder().inputs()[i]) + } }) }, - ExprKind::MethodCall(..) => Some(AutoDerefStability::Reborrow), ExprKind::Struct(path, fields, _) => { - let variant = variant_of_res(cx, cx.qpath_res(path, e.hir_id)); + let variant = variant_of_res(cx, cx.qpath_res(path, parent.hir_id)); fields .iter() .find(|f| f.expr.hir_id == child_id) @@ -763,13 +749,21 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (AutoDerefSt .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))) }, + 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) + | ExprKind::Index(child, _) + if child.hir_id == e.hir_id => + { + Some(Position::Postfix) + }, _ => None, }, _ => None, } }) - .unwrap_or(AutoDerefStability::None); - (stability, adjustments) + .unwrap_or(Position::Other); + (position, adjustments) } // Checks the stability of auto-deref when assigned to a binding with the given explicit type. @@ -781,9 +775,9 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (AutoDerefSt // // 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<'_>) -> AutoDerefStability { +fn binding_ty_auto_deref_stability(ty: &hir::Ty<'_>) -> Position { let TyKind::Rptr(_, ty) = &ty.kind else { - return AutoDerefStability::None; + return Position::Other; }; let mut ty = ty; @@ -809,9 +803,9 @@ fn binding_ty_auto_deref_stability(ty: &hir::Ty<'_>) -> AutoDerefStability { _ => false, }) { - AutoDerefStability::Reborrow + Position::ReborrowStable } else { - AutoDerefStability::Deref + Position::DerefStable } }, TyKind::Slice(_) @@ -821,8 +815,8 @@ fn binding_ty_auto_deref_stability(ty: &hir::Ty<'_>) -> AutoDerefStability { | TyKind::Tup(_) | TyKind::Ptr(_) | TyKind::TraitObject(..) - | TyKind::Path(_) => AutoDerefStability::Deref, - TyKind::OpaqueDef(..) | TyKind::Infer | TyKind::Typeof(..) | TyKind::Err => AutoDerefStability::Reborrow, + | TyKind::Path(_) => Position::DerefStable, + TyKind::OpaqueDef(..) | TyKind::Infer | TyKind::Typeof(..) | TyKind::Err => Position::ReborrowStable, }; } } @@ -865,9 +859,9 @@ fn ty_contains_infer(ty: &hir::Ty<'_>) -> bool { } // Checks whether a type is stable when switching to auto dereferencing, -fn param_auto_deref_stability(ty: Ty<'_>) -> AutoDerefStability { +fn param_auto_deref_stability(ty: Ty<'_>) -> Position { let ty::Ref(_, mut ty, _) = *ty.kind() else { - return AutoDerefStability::None; + return Position::Other; }; loop { @@ -893,16 +887,16 @@ fn param_auto_deref_stability(ty: Ty<'_>) -> AutoDerefStability { | ty::GeneratorWitness(..) | ty::Never | ty::Tuple(_) - | ty::Projection(_) => AutoDerefStability::Deref, + | ty::Projection(_) => Position::DerefStable, ty::Infer(_) | ty::Error(_) | ty::Param(_) | ty::Bound(..) | ty::Opaque(..) | ty::Placeholder(_) - | ty::Dynamic(..) => AutoDerefStability::Reborrow, - ty::Adt(..) if ty.has_placeholders() || ty.has_param_types_or_consts() => AutoDerefStability::Reborrow, - ty::Adt(..) => AutoDerefStability::Deref, + | ty::Dynamic(..) => Position::ReborrowStable, + ty::Adt(..) if ty.has_placeholders() || ty.has_param_types_or_consts() => Position::ReborrowStable, + ty::Adt(..) => Position::DerefStable, }; } } diff --git a/tests/ui/explicit_auto_deref.fixed b/tests/ui/explicit_auto_deref.fixed index f534714bc659..d4a9aa7e46c7 100644 --- a/tests/ui/explicit_auto_deref.fixed +++ b/tests/ui/explicit_auto_deref.fixed @@ -10,7 +10,8 @@ clippy::ptr_arg, clippy::redundant_field_names, clippy::too_many_arguments, - clippy::borrow_deref_ref + clippy::borrow_deref_ref, + clippy::let_unit_value )] trait CallableStr { diff --git a/tests/ui/explicit_auto_deref.rs b/tests/ui/explicit_auto_deref.rs index 4781962882bc..243b8e919c6a 100644 --- a/tests/ui/explicit_auto_deref.rs +++ b/tests/ui/explicit_auto_deref.rs @@ -10,7 +10,8 @@ clippy::ptr_arg, clippy::redundant_field_names, clippy::too_many_arguments, - clippy::borrow_deref_ref + clippy::borrow_deref_ref, + clippy::let_unit_value )] trait CallableStr { diff --git a/tests/ui/explicit_auto_deref.stderr b/tests/ui/explicit_auto_deref.stderr index 26357ffd96a0..19435eab96b7 100644 --- a/tests/ui/explicit_auto_deref.stderr +++ b/tests/ui/explicit_auto_deref.stderr @@ -1,5 +1,5 @@ error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:63:20 + --> $DIR/explicit_auto_deref.rs:65:20 | LL | let _: &str = &*s; | ^^ help: try this: `s` @@ -7,187 +7,187 @@ LL | let _: &str = &*s; = note: `-D clippy::explicit-auto-deref` implied by `-D warnings` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:67:12 + --> $DIR/explicit_auto_deref.rs:69:12 | LL | f_str(&*s); | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:71:14 + --> $DIR/explicit_auto_deref.rs:73:14 | LL | f_str_t(&*s, &*s); // Don't lint second param. | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:74:25 + --> $DIR/explicit_auto_deref.rs:76:25 | LL | let _: &Box = &**b; | ^^^ help: try this: `b` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:80:8 + --> $DIR/explicit_auto_deref.rs:82:8 | LL | c(&*s); | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:86:9 + --> $DIR/explicit_auto_deref.rs:88:9 | LL | &**x | ^^^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:90:11 + --> $DIR/explicit_auto_deref.rs:92:11 | LL | { &**x } | ^^^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:94:9 + --> $DIR/explicit_auto_deref.rs:96:9 | LL | &**{ x } | ^^^^^^^^ help: try this: `{ x }` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:98:9 + --> $DIR/explicit_auto_deref.rs:100:9 | LL | &***x | ^^^^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:115:13 + --> $DIR/explicit_auto_deref.rs:117:13 | LL | f1(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:116:13 + --> $DIR/explicit_auto_deref.rs:118:13 | LL | f2(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:117:13 + --> $DIR/explicit_auto_deref.rs:119:13 | LL | f3(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:118:28 + --> $DIR/explicit_auto_deref.rs:120:28 | LL | f4.callable_str()(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:119:13 + --> $DIR/explicit_auto_deref.rs:121:13 | LL | f5(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:120:13 + --> $DIR/explicit_auto_deref.rs:122:13 | LL | f6(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:121:28 + --> $DIR/explicit_auto_deref.rs:123:28 | LL | f7.callable_str()(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:122:26 + --> $DIR/explicit_auto_deref.rs:124:26 | LL | f8.callable_t()(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:123:13 + --> $DIR/explicit_auto_deref.rs:125:13 | LL | f9(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:124:14 + --> $DIR/explicit_auto_deref.rs:126:14 | LL | f10(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:125:27 + --> $DIR/explicit_auto_deref.rs:127:27 | LL | f11.callable_t()(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:129:17 + --> $DIR/explicit_auto_deref.rs:131:17 | LL | let _ = S1(&*s); | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:134:22 + --> $DIR/explicit_auto_deref.rs:136:22 | LL | let _ = S2 { s: &*s }; | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:150:30 + --> $DIR/explicit_auto_deref.rs:152:30 | LL | let _ = Self::S1(&**s); | ^^^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:151:35 + --> $DIR/explicit_auto_deref.rs:153:35 | LL | let _ = Self::S2 { s: &**s }; | ^^^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:154:21 + --> $DIR/explicit_auto_deref.rs:156:21 | LL | let _ = E1::S1(&*s); | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:155:26 + --> $DIR/explicit_auto_deref.rs:157:26 | LL | let _ = E1::S2 { s: &*s }; | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:173:13 + --> $DIR/explicit_auto_deref.rs:175:13 | LL | let _ = (*b).foo; | ^^^^ help: try this: `b` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:174:13 + --> $DIR/explicit_auto_deref.rs:176:13 | LL | let _ = (**b).foo; | ^^^^^ help: try this: `b` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:189:19 + --> $DIR/explicit_auto_deref.rs:191:19 | LL | let _ = f_str(*ref_str); | ^^^^^^^^ help: try this: `ref_str` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:191:19 + --> $DIR/explicit_auto_deref.rs:193:19 | LL | let _ = f_str(**ref_ref_str); | ^^^^^^^^^^^^^ help: try this: `ref_ref_str` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:201:13 + --> $DIR/explicit_auto_deref.rs:203:13 | LL | f_str(&&*ref_str); // `needless_borrow` will suggest removing both references | ^^^^^^^^ help: try this: `ref_str` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:202:12 + --> $DIR/explicit_auto_deref.rs:204:12 | LL | f_str(&&**ref_str); // `needless_borrow` will suggest removing only one reference | ^^^^^^^^^^ help: try this: `ref_str` From 6d21b79be90553ef45f58b682b40432cac840b10 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Sat, 29 Jan 2022 20:29:43 -0500 Subject: [PATCH 10/14] Fix `needless_borrow` suggestion when calling a trait method taking `self` --- clippy_lints/src/dereference.rs | 55 ++++++++++++++++++++++++++------- tests/ui/needless_borrow.fixed | 30 ++++++++++++++++++ tests/ui/needless_borrow.rs | 30 ++++++++++++++++++ tests/ui/needless_borrow.stderr | 14 ++++++++- 4 files changed, 117 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 3821beaea53b..9418c4a66b60 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -11,11 +11,13 @@ use rustc_hir::{ ImplItemKind, Item, ItemKind, Local, MatchSource, Mutability, Node, Pat, PatKind, Path, QPath, TraitItem, TraitItemKind, TyKind, UnOp, }; +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, TypeFoldable, TypeckResults}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::{symbol::sym, Span, Symbol}; +use rustc_trait_selection::infer::InferCtxtExt; declare_clippy_lint! { /// ### What it does @@ -165,7 +167,6 @@ struct StateData { struct DerefedBorrow { count: usize, - required_precedence: i8, msg: &'static str, position: Position, } @@ -329,19 +330,19 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { "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 (required_refs, required_precedence, msg) = if position.can_auto_borrow() { - (1, PREC_POSTFIX, if deref_count == 1 { borrow_msg } else { deref_msg }) + let (required_refs, msg) = if position.can_auto_borrow() { + (1, if deref_count == 1 { borrow_msg } else { deref_msg }) } 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, 0, deref_msg) + (3, deref_msg) } else { - (2, 0, deref_msg) + (2, deref_msg) } } else { - (2, 0, deref_msg) + (2, deref_msg) }; if deref_count >= required_refs { @@ -350,7 +351,6 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { // One of the required refs is for the current borrow expression, the remaining ones // can't be removed without breaking the code. See earlier comment. count: deref_count - required_refs, - required_precedence, msg, position, }), @@ -601,6 +601,8 @@ fn deref_method_same_type<'tcx>(result_ty: Ty<'tcx>, arg_ty: Ty<'tcx>) -> bool { #[derive(Clone, Copy)] enum Position { MethodReceiver, + /// The method is defined on a reference type. e.g. `impl Foo for &T` + MethodReceiverRefImpl, Callee, FieldAccess(Symbol), Postfix, @@ -627,6 +629,13 @@ impl Position { fn lint_explicit_deref(self) -> bool { matches!(self, Self::Other | Self::DerefStable | Self::ReborrowStable) } + + fn needs_parens(self, precedence: i8) -> bool { + matches!( + self, + Self::MethodReceiver | Self::MethodReceiverRefImpl | Self::Callee | Self::FieldAccess(_) | Self::Postfix + ) && precedence < PREC_POSTFIX + } } /// Walks up the parent expressions attempting to determine both how stable the auto-deref result @@ -730,10 +739,34 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & 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 { - if e.hir_id == child_id { - Position::MethodReceiver - } else { + // 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 + } 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 = cx.typeck_results().node_substs_opt(child_id).unwrap_or_else( + || 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() + ) + { + Position::MethodReceiverRefImpl + } else { + Position::MethodReceiver } } else { param_auto_deref_stability(cx.tcx.fn_sig(id).skip_binder().inputs()[i]) @@ -964,7 +997,7 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data let mut app = Applicability::MachineApplicable; let snip = snippet_with_context(cx, expr.span, data.span.ctxt(), "..", &mut app).0; span_lint_hir_and_then(cx, NEEDLESS_BORROW, data.hir_id, data.span, state.msg, |diag| { - let sugg = if state.required_precedence > expr.precedence().order() && !has_enclosing_paren(&snip) { + let sugg = if state.position.needs_parens(expr.precedence().order()) && !has_enclosing_paren(&snip) { format!("({})", snip) } else { snip.into() diff --git a/tests/ui/needless_borrow.fixed b/tests/ui/needless_borrow.fixed index f48f2ae58dc1..cb0051224360 100644 --- a/tests/ui/needless_borrow.fixed +++ b/tests/ui/needless_borrow.fixed @@ -85,6 +85,36 @@ fn main() { let _ = x.0; let x = &x as *const (i32, i32); let _ = unsafe { (*x).0 }; + + // Issue #8367 + trait Foo { + fn foo(self); + } + impl Foo for &'_ () { + fn foo(self) {} + } + (&()).foo(); // Don't lint. `()` doesn't implement `Foo` + (&()).foo(); + + impl Foo for i32 { + fn foo(self) {} + } + impl Foo for &'_ i32 { + fn foo(self) {} + } + (&5).foo(); // Don't lint. `5` will call `::foo` + (&5).foo(); + + trait FooRef { + fn foo_ref(&self); + } + impl FooRef for () { + fn foo_ref(&self) {} + } + impl FooRef for &'_ () { + fn foo_ref(&self) {} + } + (&&()).foo_ref(); // Don't lint. `&()` will call `<() as FooRef>::foo_ref` } #[allow(clippy::needless_borrowed_reference)] diff --git a/tests/ui/needless_borrow.rs b/tests/ui/needless_borrow.rs index 63515a821589..d636a4010037 100644 --- a/tests/ui/needless_borrow.rs +++ b/tests/ui/needless_borrow.rs @@ -85,6 +85,36 @@ fn main() { let _ = (&x).0; let x = &x as *const (i32, i32); let _ = unsafe { (&*x).0 }; + + // Issue #8367 + trait Foo { + fn foo(self); + } + impl Foo for &'_ () { + fn foo(self) {} + } + (&()).foo(); // Don't lint. `()` doesn't implement `Foo` + (&&()).foo(); + + impl Foo for i32 { + fn foo(self) {} + } + impl Foo for &'_ i32 { + fn foo(self) {} + } + (&5).foo(); // Don't lint. `5` will call `::foo` + (&&5).foo(); + + trait FooRef { + fn foo_ref(&self); + } + impl FooRef for () { + fn foo_ref(&self) {} + } + impl FooRef for &'_ () { + fn foo_ref(&self) {} + } + (&&()).foo_ref(); // Don't lint. `&()` will call `<() as FooRef>::foo_ref` } #[allow(clippy::needless_borrowed_reference)] diff --git a/tests/ui/needless_borrow.stderr b/tests/ui/needless_borrow.stderr index cd23d9fd072a..8a2e2b989590 100644 --- a/tests/ui/needless_borrow.stderr +++ b/tests/ui/needless_borrow.stderr @@ -108,5 +108,17 @@ error: this expression borrows a value the compiler would automatically borrow LL | let _ = unsafe { (&*x).0 }; | ^^^^^ help: change this to: `(*x)` -error: aborting due to 18 previous errors +error: this expression creates a reference which is immediately dereferenced by the compiler + --> $DIR/needless_borrow.rs:97:5 + | +LL | (&&()).foo(); + | ^^^^^^ help: change this to: `(&())` + +error: this expression creates a reference which is immediately dereferenced by the compiler + --> $DIR/needless_borrow.rs:106:5 + | +LL | (&&5).foo(); + | ^^^^^ help: change this to: `(&5)` + +error: aborting due to 20 previous errors From 9788107931ed6d86fc14cebca3d15b7da0d082c4 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Mon, 31 Jan 2022 02:06:18 -0500 Subject: [PATCH 11/14] Handle future precedence issues in `explicit_auto_deref` + cleanup --- clippy_lints/src/dereference.rs | 155 ++++++++++++++++++-------------- 1 file changed, 89 insertions(+), 66 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 9418c4a66b60..e3529d1c9240 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -163,12 +163,12 @@ struct StateData { /// Span of the top level expression span: Span, hir_id: HirId, + position: Position, } struct DerefedBorrow { count: usize, msg: &'static str, - position: Position, } enum State { @@ -182,8 +182,8 @@ enum State { }, DerefedBorrow(DerefedBorrow), ExplicitDeref { - deref_span: Span, - deref_hir_id: HirId, + // Span and id of the top-level deref expression if the parent expression is a borrow. + deref_span_id: Option<(Span, HirId)>, }, ExplicitDerefField { name: Symbol, @@ -258,12 +258,12 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { { self.state = Some(( State::ExplicitDerefField { name }, - StateData { span: expr.span, hir_id: expr.hir_id }, + StateData { span: expr.span, hir_id: expr.hir_id, position }, )); } else if position.is_deref_stable() { self.state = Some(( - State::ExplicitDeref { deref_span: expr.span, deref_hir_id: expr.hir_id }, - StateData { span: expr.span, hir_id: expr.hir_id }, + State::ExplicitDeref { deref_span_id: None }, + StateData { span: expr.span, hir_id: expr.hir_id, position }, )); } } @@ -284,6 +284,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { StateData { span: expr.span, hir_id: expr.hir_id, + position }, )); }, @@ -352,9 +353,8 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { // can't be removed without breaking the code. See earlier comment. count: deref_count - required_refs, msg, - position, }), - StateData { span: expr.span, hir_id: expr.hir_id }, + StateData { span: expr.span, hir_id: expr.hir_id, position }, )); } else if position.is_deref_stable() { self.state = Some(( @@ -362,6 +362,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { StateData { span: expr.span, hir_id: expr.hir_id, + position }, )); } @@ -403,7 +404,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { )); }, (Some((State::DerefedBorrow(state), data)), RefOp::AddrOf) => { - let position = state.position; + let position = data.position; report(cx, expr, State::DerefedBorrow(state), data); if position.is_deref_stable() { self.state = Some(( @@ -411,24 +412,25 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { StateData { span: expr.span, hir_id: expr.hir_id, + position, }, )); } }, (Some((State::DerefedBorrow(state), data)), RefOp::Deref) => { - let position = state.position; + let position = data.position; report(cx, expr, State::DerefedBorrow(state), data); if let Position::FieldAccess(name) = position && !ty_contains_field(typeck.expr_ty(sub_expr), name) { self.state = Some(( State::ExplicitDerefField { name }, - StateData { span: expr.span, hir_id: expr.hir_id }, + StateData { span: expr.span, hir_id: expr.hir_id, position }, )); } else if position.is_deref_stable() { self.state = Some(( - State::ExplicitDeref { deref_span: expr.span, deref_hir_id: expr.hir_id }, - StateData { span: expr.span, hir_id: expr.hir_id }, + State::ExplicitDeref { deref_span_id: None }, + StateData { span: expr.span, hir_id: expr.hir_id, position }, )); } }, @@ -445,8 +447,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { } else { self.state = Some(( State::ExplicitDeref { - deref_span: expr.span, - deref_hir_id: expr.hir_id, + deref_span_id: Some((expr.span, expr.hir_id)), }, data, )); @@ -464,8 +465,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing { ) => { self.state = Some(( State::ExplicitDeref { - deref_span, - deref_hir_id, + deref_span_id: Some((deref_span, deref_hir_id)), }, data, )); @@ -608,18 +608,18 @@ enum Position { Postfix, Deref, /// Any other location which will trigger auto-deref to a specific time. - DerefStable, + DerefStable(i8), /// Any other location which will trigger auto-reborrowing. - ReborrowStable, - Other, + ReborrowStable(i8), + 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 { @@ -627,14 +627,19 @@ 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 needs_parens(self, precedence: i8) -> bool { - matches!( - self, - Self::MethodReceiver | Self::MethodReceiverRefImpl | Self::Callee | Self::FieldAccess(_) | Self::Postfix - ) && precedence < PREC_POSTFIX + fn precedence(self) -> i8 { + match self { + Self::MethodReceiver + | Self::MethodReceiverRefImpl + | Self::Callee + | Self::FieldAccess(_) + | Self::Postfix => PREC_POSTFIX, + Self::Deref => PREC_PREFIX, + Self::DerefStable(p) | Self::ReborrowStable(p) | Self::Other(p) => p, + } } } @@ -644,6 +649,7 @@ impl Position { #[allow(clippy::too_many_lines)] fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, &'tcx [Adjustment<'tcx>]) { let mut adjustments = [].as_slice(); + let mut precedence = 0i8; let ctxt = e.span.ctxt(); let position = walk_to_expr_usage(cx, e, &mut |parent, child_id| { // LocalTableInContext returns the wrong lifetime, so go use `expr_adjustments` instead. @@ -652,7 +658,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)) + Some(binding_ty_auto_deref_stability(ty, precedence)) }, Node::Item(&Item { kind: ItemKind::Static(..) | ItemKind::Const(..), @@ -674,9 +680,9 @@ 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::Other + Position::DerefStable(precedence) } else { - Position::DerefStable + Position::Other(precedence) }) }, @@ -700,11 +706,11 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & }) 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 + Position::Other(precedence) } else if output.has_placeholders() || output.has_opaque_types() { - Position::ReborrowStable + Position::ReborrowStable(precedence) } else { - Position::DerefStable + Position::DerefStable(precedence) }) }, @@ -716,11 +722,11 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & .skip_binder() .output(); Some(if !output.is_ref() { - Position::Other + Position::Other(precedence) } else if output.has_placeholders() || output.has_opaque_types() { - Position::ReborrowStable + Position::ReborrowStable(precedence) } else { - Position::DerefStable + Position::DerefStable(precedence) }) }, ExprKind::Call(func, _) if func.hir_id == child_id => (child_id == e.hir_id).then(|| Position::Callee), @@ -732,8 +738,8 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & .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), - None => param_auto_deref_stability(ty.skip_binder()), + Some(ty) => binding_ty_auto_deref_stability(ty, precedence), + None => param_auto_deref_stability(ty.skip_binder(), precedence), }), ExprKind::MethodCall(_, args, _) => { let id = cx.typeck_results().type_dependent_def_id(parent.hir_id).unwrap(); @@ -745,7 +751,7 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & // * `&self` methods on `&T` can have auto-borrow, but `&self` methods on `T` will take // priority. if e.hir_id != child_id { - Position::ReborrowStable + 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() @@ -769,7 +775,7 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & Position::MethodReceiver } } else { - param_auto_deref_stability(cx.tcx.fn_sig(id).skip_binder().inputs()[i]) + param_auto_deref_stability(cx.tcx.fn_sig(id).skip_binder().inputs()[i], precedence) } }) }, @@ -780,7 +786,7 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & .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))) + .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), @@ -790,12 +796,16 @@ fn walk_parents<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (Position, & { Some(Position::Postfix) }, + _ if child_id == e.hir_id => { + precedence = parent.precedence().order(); + None + }, _ => None, }, _ => None, } }) - .unwrap_or(Position::Other); + .unwrap_or(Position::Other(precedence)); (position, adjustments) } @@ -808,9 +818,9 @@ 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<'_>) -> Position { +fn binding_ty_auto_deref_stability(ty: &hir::Ty<'_>, precedence: i8) -> Position { let TyKind::Rptr(_, ty) = &ty.kind else { - return Position::Other; + return Position::Other(precedence); }; let mut ty = ty; @@ -836,9 +846,9 @@ fn binding_ty_auto_deref_stability(ty: &hir::Ty<'_>) -> Position { _ => false, }) { - Position::ReborrowStable + Position::ReborrowStable(precedence) } else { - Position::DerefStable + Position::DerefStable(precedence) } }, TyKind::Slice(_) @@ -848,8 +858,11 @@ fn binding_ty_auto_deref_stability(ty: &hir::Ty<'_>) -> Position { | TyKind::Tup(_) | TyKind::Ptr(_) | TyKind::TraitObject(..) - | TyKind::Path(_) => Position::DerefStable, - TyKind::OpaqueDef(..) | TyKind::Infer | TyKind::Typeof(..) | TyKind::Err => Position::ReborrowStable, + | TyKind::Path(_) => Position::DerefStable(precedence), + TyKind::OpaqueDef(..) + | TyKind::Infer + | TyKind::Typeof(..) + | TyKind::Err => Position::ReborrowStable(precedence), }; } } @@ -892,9 +905,9 @@ fn ty_contains_infer(ty: &hir::Ty<'_>) -> bool { } // Checks whether a type is stable when switching to auto dereferencing, -fn param_auto_deref_stability(ty: Ty<'_>) -> Position { +fn param_auto_deref_stability(ty: Ty<'_>, precedence: i8) -> Position { let ty::Ref(_, mut ty, _) = *ty.kind() else { - return Position::Other; + return Position::Other(precedence); }; loop { @@ -920,16 +933,18 @@ fn param_auto_deref_stability(ty: Ty<'_>) -> Position { | ty::GeneratorWitness(..) | ty::Never | ty::Tuple(_) - | ty::Projection(_) => Position::DerefStable, + | ty::Projection(_) => Position::DerefStable(precedence), ty::Infer(_) | ty::Error(_) | ty::Param(_) | ty::Bound(..) | ty::Opaque(..) | ty::Placeholder(_) - | ty::Dynamic(..) => Position::ReborrowStable, - ty::Adt(..) if ty.has_placeholders() || ty.has_param_types_or_consts() => Position::ReborrowStable, - ty::Adt(..) => Position::DerefStable, + | ty::Dynamic(..) => Position::ReborrowStable(precedence), + ty::Adt(..) if ty.has_placeholders() || ty.has_param_types_or_consts() => { + Position::ReborrowStable(precedence) + }, + ty::Adt(..) => Position::DerefStable(precedence), }; } } @@ -995,9 +1010,12 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data }, State::DerefedBorrow(state) => { let mut app = Applicability::MachineApplicable; - let snip = snippet_with_context(cx, expr.span, data.span.ctxt(), "..", &mut app).0; + let (snip, snip_is_macro) = snippet_with_context(cx, expr.span, data.span.ctxt(), "..", &mut app); span_lint_hir_and_then(cx, NEEDLESS_BORROW, data.hir_id, data.span, state.msg, |diag| { - let sugg = if state.position.needs_parens(expr.precedence().order()) && !has_enclosing_paren(&snip) { + let sugg = if !snip_is_macro + && expr.precedence().order() < data.position.precedence() + && !has_enclosing_paren(&snip) + { format!("({})", snip) } else { snip.into() @@ -1005,14 +1023,13 @@ 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, - deref_hir_id, - } => { - let (span, hir_id) = if cx.typeck_results().expr_ty(expr).is_ref() { - (data.span, data.hir_id) + State::ExplicitDeref { deref_span_id } => { + let (span, hir_id, precedence) = if let Some((span, hir_id)) = deref_span_id + && !cx.typeck_results().expr_ty(expr).is_ref() + { + (span, hir_id, PREC_PREFIX) } else { - (deref_span, deref_hir_id) + (data.span, data.hir_id, data.position.precedence()) }; span_lint_hir_and_then( cx, @@ -1022,8 +1039,14 @@ fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data "deref which would be done by auto-deref", |diag| { let mut app = Applicability::MachineApplicable; - let snip = snippet_with_context(cx, expr.span, span.ctxt(), "..", &mut app).0; - diag.span_suggestion(span, "try this", snip.into_owned(), app); + let (snip, snip_is_macro) = snippet_with_context(cx, expr.span, span.ctxt(), "..", &mut app); + let sugg = + if !snip_is_macro && expr.precedence().order() < precedence && !has_enclosing_paren(&snip) { + format!("({})", snip) + } else { + snip.into() + }; + diag.span_suggestion(span, "try this", sugg, app); }, ); }, From 15df2289ea1b9e90eaa37890dcecc155946bb6ac Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Mon, 7 Feb 2022 10:29:45 -0500 Subject: [PATCH 12/14] Code cleanup --- clippy_lints/src/dereference.rs | 57 +++++++++++++++------------------ clippy_utils/src/lib.rs | 11 ++----- clippy_utils/src/ty.rs | 1 + 3 files changed, 29 insertions(+), 40 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index e3529d1c9240..e80ee6af6502 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -6,8 +6,9 @@ use clippy_utils::{get_parent_expr, is_lint_allowed, path_to_local, walk_to_expr 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, Expr, ExprKind, FnRetTy, GenericArg, HirId, ImplItem, + self as hir, BindingAnnotation, Body, BodyId, BorrowKind, Expr, ExprKind, GenericArg, HirId, ImplItem, ImplItemKind, Item, ItemKind, Local, MatchSource, Mutability, Node, Pat, PatKind, Path, QPath, TraitItem, TraitItemKind, TyKind, UnOp, }; @@ -870,38 +871,32 @@ fn binding_ty_auto_deref_stability(ty: &hir::Ty<'_>, precedence: i8) -> Position // Checks whether a type is inferred at some point. // e.g. `_`, `Box<_>`, `[_]` fn ty_contains_infer(ty: &hir::Ty<'_>) -> bool { - match &ty.kind { - TyKind::Slice(ty) | TyKind::Array(ty, _) => ty_contains_infer(ty), - TyKind::Ptr(ty) | TyKind::Rptr(_, ty) => ty_contains_infer(ty.ty), - TyKind::Tup(tys) => tys.iter().any(ty_contains_infer), - TyKind::BareFn(ty) => { - if ty.decl.inputs.iter().any(ty_contains_infer) { - return true; - } - if let FnRetTy::Return(ty) = &ty.decl.output { - ty_contains_infer(ty) + struct V(bool); + impl Visitor<'_> for V { + fn visit_ty(&mut self, ty: &hir::Ty<'_>) { + if self.0 + || matches!( + ty.kind, + TyKind::OpaqueDef(..) | TyKind::Infer | TyKind::Typeof(_) | TyKind::Err + ) + { + self.0 = true; } else { - false + walk_ty(self, ty); } - }, - &TyKind::Path( - QPath::TypeRelative(_, path) - | QPath::Resolved( - _, - Path { - segments: [.., path], .. - }, - ), - ) => path.args.map_or(false, |args| { - args.args.iter().any(|arg| match arg { - GenericArg::Infer(_) => true, - GenericArg::Type(ty) => ty_contains_infer(ty), - _ => false, - }) - }), - TyKind::Path(_) | TyKind::OpaqueDef(..) | TyKind::Infer | TyKind::Typeof(_) | TyKind::Err => true, - TyKind::Never | TyKind::TraitObject(..) => false, + } + + fn visit_generic_arg(&mut self, arg: &GenericArg<'_>) { + if self.0 || matches!(arg, GenericArg::Infer(_)) { + self.0 = true; + } else if let GenericArg::Type(ty) = arg { + self.visit_ty(ty); + } + } } + let mut v = V(false); + v.visit_ty(ty); + v.0 } // Checks whether a type is stable when switching to auto dereferencing, @@ -951,7 +946,7 @@ fn param_auto_deref_stability(ty: Ty<'_>, precedence: i8) -> Position { fn ty_contains_field(ty: Ty<'_>, name: Symbol) -> bool { if let ty::Adt(adt, _) = *ty.kind() { - adt.is_struct() && adt.non_enum_variant().fields.iter().any(|f| f.name == name) + adt.is_struct() && adt.all_fields().any(|f| f.name == name) } else { false } diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index ca056e7c1a0d..a2772edf7383 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -2215,19 +2215,12 @@ pub fn walk_to_expr_usage<'tcx, T>( _ => return None, }; match parent.kind { - ExprKind::If(child, ..) | ExprKind::Match(child, ..) if child.hir_id != child_id => { - child_id = parent_id; - continue; - }, + ExprKind::If(child, ..) | ExprKind::Match(child, ..) if child.hir_id != child_id => child_id = parent_id, ExprKind::Break(Destination { target_id: Ok(id), .. }, _) => { child_id = id; iter = map.parent_iter(id); - continue; - }, - ExprKind::Block(..) => { - child_id = parent_id; - continue; }, + ExprKind::Block(..) => child_id = parent_id, _ => return None, } } diff --git a/clippy_utils/src/ty.rs b/clippy_utils/src/ty.rs index 2c19e8a7adb2..6ca36eed4e65 100644 --- a/clippy_utils/src/ty.rs +++ b/clippy_utils/src/ty.rs @@ -775,6 +775,7 @@ pub fn for_each_top_level_late_bound_region( ty.visit_with(&mut V { index: 0, f }) } +/// Gets the struct or enum variant from the given `Res` pub fn variant_of_res<'tcx>(cx: &LateContext<'tcx>, res: Res) -> Option<&'tcx VariantDef> { match res { Res::Def(DefKind::Struct, id) => Some(cx.tcx.adt_def(id).non_enum_variant()), From 85c1f74fef6b49fbaf1b8d03575fd6ee3e352d78 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Mon, 13 Jun 2022 13:09:52 -0400 Subject: [PATCH 13/14] Add `explicit_auto_deref` test for variadic function --- tests/ui/explicit_auto_deref.fixed | 9 ++++ tests/ui/explicit_auto_deref.rs | 9 ++++ tests/ui/explicit_auto_deref.stderr | 64 ++++++++++++++--------------- 3 files changed, 50 insertions(+), 32 deletions(-) diff --git a/tests/ui/explicit_auto_deref.fixed b/tests/ui/explicit_auto_deref.fixed index d4a9aa7e46c7..d4ff1b1566dc 100644 --- a/tests/ui/explicit_auto_deref.fixed +++ b/tests/ui/explicit_auto_deref.fixed @@ -59,6 +59,10 @@ fn f_str_t(_: &str, _: T) {} fn f_box_t(_: &Box) {} +extern "C" { + fn var(_: u32, ...); +} + fn main() { let s = String::new(); @@ -202,4 +206,9 @@ fn main() { f_str(&&ref_str); // `needless_borrow` will suggest removing both references f_str(&ref_str); // `needless_borrow` will suggest removing only one reference + + let x = &&40; + unsafe { + var(0, &**x); + } } diff --git a/tests/ui/explicit_auto_deref.rs b/tests/ui/explicit_auto_deref.rs index 243b8e919c6a..99294a7947bf 100644 --- a/tests/ui/explicit_auto_deref.rs +++ b/tests/ui/explicit_auto_deref.rs @@ -59,6 +59,10 @@ fn f_str_t(_: &str, _: T) {} fn f_box_t(_: &Box) {} +extern "C" { + fn var(_: u32, ...); +} + fn main() { let s = String::new(); @@ -202,4 +206,9 @@ fn main() { f_str(&&*ref_str); // `needless_borrow` will suggest removing both references f_str(&&**ref_str); // `needless_borrow` will suggest removing only one reference + + let x = &&40; + unsafe { + var(0, &**x); + } } diff --git a/tests/ui/explicit_auto_deref.stderr b/tests/ui/explicit_auto_deref.stderr index 19435eab96b7..55f956e37aed 100644 --- a/tests/ui/explicit_auto_deref.stderr +++ b/tests/ui/explicit_auto_deref.stderr @@ -1,5 +1,5 @@ error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:65:20 + --> $DIR/explicit_auto_deref.rs:69:20 | LL | let _: &str = &*s; | ^^ help: try this: `s` @@ -7,187 +7,187 @@ LL | let _: &str = &*s; = note: `-D clippy::explicit-auto-deref` implied by `-D warnings` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:69:12 + --> $DIR/explicit_auto_deref.rs:73:12 | LL | f_str(&*s); | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:73:14 + --> $DIR/explicit_auto_deref.rs:77:14 | LL | f_str_t(&*s, &*s); // Don't lint second param. | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:76:25 + --> $DIR/explicit_auto_deref.rs:80:25 | LL | let _: &Box = &**b; | ^^^ help: try this: `b` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:82:8 + --> $DIR/explicit_auto_deref.rs:86:8 | LL | c(&*s); | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:88:9 + --> $DIR/explicit_auto_deref.rs:92:9 | LL | &**x | ^^^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:92:11 + --> $DIR/explicit_auto_deref.rs:96:11 | LL | { &**x } | ^^^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:96:9 + --> $DIR/explicit_auto_deref.rs:100:9 | LL | &**{ x } | ^^^^^^^^ help: try this: `{ x }` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:100:9 + --> $DIR/explicit_auto_deref.rs:104:9 | LL | &***x | ^^^^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:117:13 + --> $DIR/explicit_auto_deref.rs:121:13 | LL | f1(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:118:13 + --> $DIR/explicit_auto_deref.rs:122:13 | LL | f2(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:119:13 + --> $DIR/explicit_auto_deref.rs:123:13 | LL | f3(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:120:28 + --> $DIR/explicit_auto_deref.rs:124:28 | LL | f4.callable_str()(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:121:13 + --> $DIR/explicit_auto_deref.rs:125:13 | LL | f5(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:122:13 + --> $DIR/explicit_auto_deref.rs:126:13 | LL | f6(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:123:28 + --> $DIR/explicit_auto_deref.rs:127:28 | LL | f7.callable_str()(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:124:26 + --> $DIR/explicit_auto_deref.rs:128:26 | LL | f8.callable_t()(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:125:13 + --> $DIR/explicit_auto_deref.rs:129:13 | LL | f9(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:126:14 + --> $DIR/explicit_auto_deref.rs:130:14 | LL | f10(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:127:27 + --> $DIR/explicit_auto_deref.rs:131:27 | LL | f11.callable_t()(&*x); | ^^ help: try this: `x` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:131:17 + --> $DIR/explicit_auto_deref.rs:135:17 | LL | let _ = S1(&*s); | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:136:22 + --> $DIR/explicit_auto_deref.rs:140:22 | LL | let _ = S2 { s: &*s }; | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:152:30 + --> $DIR/explicit_auto_deref.rs:156:30 | LL | let _ = Self::S1(&**s); | ^^^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:153:35 + --> $DIR/explicit_auto_deref.rs:157:35 | LL | let _ = Self::S2 { s: &**s }; | ^^^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:156:21 + --> $DIR/explicit_auto_deref.rs:160:21 | LL | let _ = E1::S1(&*s); | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:157:26 + --> $DIR/explicit_auto_deref.rs:161:26 | LL | let _ = E1::S2 { s: &*s }; | ^^ help: try this: `s` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:175:13 + --> $DIR/explicit_auto_deref.rs:179:13 | LL | let _ = (*b).foo; | ^^^^ help: try this: `b` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:176:13 + --> $DIR/explicit_auto_deref.rs:180:13 | LL | let _ = (**b).foo; | ^^^^^ help: try this: `b` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:191:19 + --> $DIR/explicit_auto_deref.rs:195:19 | LL | let _ = f_str(*ref_str); | ^^^^^^^^ help: try this: `ref_str` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:193:19 + --> $DIR/explicit_auto_deref.rs:197:19 | LL | let _ = f_str(**ref_ref_str); | ^^^^^^^^^^^^^ help: try this: `ref_ref_str` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:203:13 + --> $DIR/explicit_auto_deref.rs:207:13 | LL | f_str(&&*ref_str); // `needless_borrow` will suggest removing both references | ^^^^^^^^ help: try this: `ref_str` error: deref which would be done by auto-deref - --> $DIR/explicit_auto_deref.rs:204:12 + --> $DIR/explicit_auto_deref.rs:208:12 | LL | f_str(&&**ref_str); // `needless_borrow` will suggest removing only one reference | ^^^^^^^^^^ help: try this: `ref_str` From 5e2a2d3ac993cc6b89e561e69b4571d95d0f7627 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Tue, 14 Jun 2022 11:44:43 -0400 Subject: [PATCH 14/14] Fix dogfood --- clippy_dev/src/update_lints.rs | 2 +- clippy_lints/src/dereference.rs | 26 +++++++++---------- clippy_lints/src/matches/match_same_arms.rs | 2 +- .../src/matches/match_single_binding.rs | 4 +-- 4 files changed, 17 insertions(+), 17 deletions(-) diff --git a/clippy_dev/src/update_lints.rs b/clippy_dev/src/update_lints.rs index 115f5f0064fe..2e0659f42d7b 100644 --- a/clippy_dev/src/update_lints.rs +++ b/clippy_dev/src/update_lints.rs @@ -413,7 +413,7 @@ fn remove_lint_declaration(name: &str, path: &Path, lints: &mut Vec) -> io .find("declare_lint_pass!") .unwrap_or_else(|| panic!("failed to find `impl_lint_pass`")) }); - let mut impl_lint_pass_end = (&content[impl_lint_pass_start..]) + let mut impl_lint_pass_end = content[impl_lint_pass_start..] .find(']') .expect("failed to find `impl_lint_pass` terminator"); diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index e80ee6af6502..59dcc1ebf191 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -911,7 +911,18 @@ fn param_auto_deref_stability(ty: Ty<'_>, precedence: i8) -> Position { ty = ref_ty; continue; }, - ty::Bool + 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::Adt(..) + | ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) @@ -929,17 +940,6 @@ fn param_auto_deref_stability(ty: Ty<'_>, precedence: i8) -> Position { | ty::Never | ty::Tuple(_) | ty::Projection(_) => Position::DerefStable(precedence), - 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::Adt(..) => Position::DerefStable(precedence), }; } } @@ -952,7 +952,7 @@ fn ty_contains_field(ty: Ty<'_>, name: Symbol) -> bool { } } -#[expect(clippy::needless_pass_by_value)] +#[expect(clippy::needless_pass_by_value, clippy::too_many_lines)] fn report<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, state: State, data: StateData) { match state { State::DerefMethod { diff --git a/clippy_lints/src/matches/match_same_arms.rs b/clippy_lints/src/matches/match_same_arms.rs index 16fefea55201..15513de7d860 100644 --- a/clippy_lints/src/matches/match_same_arms.rs +++ b/clippy_lints/src/matches/match_same_arms.rs @@ -285,7 +285,7 @@ impl<'a> NormalizedPat<'a> { // TODO: Handle negative integers. They're currently treated as a wild match. ExprKind::Lit(lit) => match lit.node { LitKind::Str(sym, _) => Self::LitStr(sym), - LitKind::ByteStr(ref bytes) => Self::LitBytes(&**bytes), + LitKind::ByteStr(ref bytes) => Self::LitBytes(bytes), LitKind::Byte(val) => Self::LitInt(val.into()), LitKind::Char(val) => Self::LitInt(val.into()), LitKind::Int(val, _) => Self::LitInt(val), diff --git a/clippy_lints/src/matches/match_single_binding.rs b/clippy_lints/src/matches/match_single_binding.rs index 9df2db45dcf8..5ae4a65acaf3 100644 --- a/clippy_lints/src/matches/match_single_binding.rs +++ b/clippy_lints/src/matches/match_single_binding.rs @@ -55,7 +55,7 @@ pub(crate) fn check<'a>(cx: &LateContext<'a>, ex: &Expr<'a>, arms: &[Arm<'_>], e cx, (ex, expr), (bind_names, matched_vars), - &*snippet_body, + &snippet_body, &mut applicability, Some(span), ); @@ -88,7 +88,7 @@ pub(crate) fn check<'a>(cx: &LateContext<'a>, ex: &Expr<'a>, arms: &[Arm<'_>], e cx, (ex, expr), (bind_names, matched_vars), - &*snippet_body, + &snippet_body, &mut applicability, None, );