From 65defdb47482e26037ff9e5ca311522c7823372f Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Sun, 10 Mar 2024 19:40:31 +0000 Subject: [PATCH] [`unconditional_recursion`]: catch `From` -> `Into` -> `From` --- clippy_lints/src/unconditional_recursion.rs | 54 ++++++-- clippy_utils/src/lib.rs | 19 ++- tests/ui/unconditional_recursion.rs | 50 ++++++- tests/ui/unconditional_recursion.stderr | 136 ++++++++++---------- 4 files changed, 174 insertions(+), 85 deletions(-) diff --git a/clippy_lints/src/unconditional_recursion.rs b/clippy_lints/src/unconditional_recursion.rs index 224ec475c51..1ca14ca04d3 100644 --- a/clippy_lints/src/unconditional_recursion.rs +++ b/clippy_lints/src/unconditional_recursion.rs @@ -1,5 +1,5 @@ use clippy_utils::diagnostics::span_lint_and_then; -use clippy_utils::{expr_or_init, get_trait_def_id, path_def_id}; +use clippy_utils::{expr_or_init, fn_def_id_with_node_args, path_def_id}; use rustc_ast::BinOpKind; use rustc_data_structures::fx::FxHashMap; use rustc_hir as hir; @@ -19,11 +19,11 @@ declare_clippy_lint! { /// ### What it does - /// Checks that there isn't an infinite recursion in `PartialEq` trait - /// implementation. + /// Checks that there isn't an infinite recursion in trait + /// implementations. /// /// ### Why is this bad? - /// This is a hard to find infinite recursion which will crashing any code + /// This is a hard to find infinite recursion that will crash any code. /// using it. /// /// ### Example @@ -201,7 +201,6 @@ fn check_partial_eq(cx: &LateContext<'_>, method_span: Span, method_def_id: Loca } } -#[allow(clippy::unnecessary_def_path)] fn check_to_string(cx: &LateContext<'_>, method_span: Span, method_def_id: LocalDefId, name: Ident, expr: &Expr<'_>) { let args = cx .tcx @@ -224,7 +223,7 @@ fn check_to_string(cx: &LateContext<'_>, method_span: Span, method_def_id: Local && let Some(trait_) = impl_.of_trait && let Some(trait_def_id) = trait_.trait_def_id() // The trait is `ToString`. - && Some(trait_def_id) == get_trait_def_id(cx, &["alloc", "string", "ToString"]) + && cx.tcx.is_diagnostic_item(sym::ToString, trait_def_id) { let is_bad = match expr.kind { ExprKind::MethodCall(segment, _receiver, &[_arg], _) if segment.ident.name == name.name => { @@ -291,7 +290,6 @@ fn nested_visit_map(&mut self) -> Self::Map { self.map } - #[allow(clippy::unnecessary_def_path)] fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { if self.found_default_call { return; @@ -303,7 +301,7 @@ fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { && is_default_method_on_current_ty(self.cx.tcx, qpath, self.implemented_ty_id) && let Some(method_def_id) = path_def_id(self.cx, f) && let Some(trait_def_id) = self.cx.tcx.trait_of_item(method_def_id) - && Some(trait_def_id) == get_trait_def_id(self.cx, &["core", "default", "Default"]) + && self.cx.tcx.is_diagnostic_item(sym::Default, trait_def_id) { self.found_default_call = true; span_error(self.cx, self.method_span, expr); @@ -312,10 +310,9 @@ fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { } impl UnconditionalRecursion { - #[allow(clippy::unnecessary_def_path)] fn init_default_impl_for_type_if_needed(&mut self, cx: &LateContext<'_>) { if self.default_impl_for_type.is_empty() - && let Some(default_trait_id) = get_trait_def_id(cx, &["core", "default", "Default"]) + && let Some(default_trait_id) = cx.tcx.get_diagnostic_item(sym::Default) { let impls = cx.tcx.trait_impls_of(default_trait_id); for (ty, impl_def_ids) in impls.non_blanket_impls() { @@ -394,6 +391,34 @@ fn check_default_new<'tcx>( } } +fn check_from(cx: &LateContext<'_>, method_span: Span, method_def_id: LocalDefId, expr: &Expr<'_>) { + let Some(sig) = cx + .typeck_results() + .liberated_fn_sigs() + .get(cx.tcx.local_def_id_to_hir_id(method_def_id)) + else { + return; + }; + + // Check if we are calling `Into::into` where the node args match with our `From::from` signature: + // From::from signature: fn(S1) -> S2 + // >::into(s1), node_args=[S1, S2] + // If they do match, then it must mean that it is the blanket impl, + // which calls back into our `From::from` again (`Into` is not specializable). + // rustc's unconditional_recursion already catches calling `From::from` directly + if let Some((fn_def_id, node_args)) = fn_def_id_with_node_args(cx, expr) + && let [s1, s2] = **node_args + && let (Some(s1), Some(s2)) = (s1.as_type(), s2.as_type()) + && let Some(trait_def_id) = cx.tcx.trait_of_item(fn_def_id) + && cx.tcx.is_diagnostic_item(sym::Into, trait_def_id) + && get_impl_trait_def_id(cx, method_def_id) == cx.tcx.get_diagnostic_item(sym::From) + && s1 == sig.inputs()[0] + && s2 == sig.output() + { + span_error(cx, method_span, expr); + } +} + impl<'tcx> LateLintPass<'tcx> for UnconditionalRecursion { fn check_fn( &mut self, @@ -410,10 +435,11 @@ fn check_fn( // Doesn't have a conditional return. && !has_conditional_return(body, expr) { - if name.name == sym::eq || name.name == sym::ne { - check_partial_eq(cx, method_span, method_def_id, name, expr); - } else if name.name == sym::to_string { - check_to_string(cx, method_span, method_def_id, name, expr); + match name.name { + sym::eq | sym::ne => check_partial_eq(cx, method_span, method_def_id, name, expr), + sym::to_string => check_to_string(cx, method_span, method_def_id, name, expr), + sym::from => check_from(cx, method_span, method_def_id, expr), + _ => {}, } self.check_default_new(cx, decl, body, method_span, method_def_id); } diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 708037a4655..b013e4196ea 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -2249,8 +2249,21 @@ pub fn fn_has_unsatisfiable_preds(cx: &LateContext<'_>, did: DefId) -> bool { /// Returns the `DefId` of the callee if the given expression is a function or method call. pub fn fn_def_id(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option { + fn_def_id_with_node_args(cx, expr).map(|(did, _)| did) +} + +/// Returns the `DefId` of the callee if the given expression is a function or method call, +/// as well as its node args. +pub fn fn_def_id_with_node_args<'tcx>( + cx: &LateContext<'tcx>, + expr: &Expr<'_>, +) -> Option<(DefId, rustc_ty::GenericArgsRef<'tcx>)> { + let typeck = cx.typeck_results(); match &expr.kind { - ExprKind::MethodCall(..) => cx.typeck_results().type_dependent_def_id(expr.hir_id), + ExprKind::MethodCall(..) => Some(( + typeck.type_dependent_def_id(expr.hir_id)?, + typeck.node_args(expr.hir_id), + )), ExprKind::Call( Expr { kind: ExprKind::Path(qpath), @@ -2262,9 +2275,9 @@ pub fn fn_def_id(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option { // Only return Fn-like DefIds, not the DefIds of statics/consts/etc that contain or // deref to fn pointers, dyn Fn, impl Fn - #8850 if let Res::Def(DefKind::Fn | DefKind::Ctor(..) | DefKind::AssocFn, id) = - cx.typeck_results().qpath_res(qpath, *path_hir_id) + typeck.qpath_res(qpath, *path_hir_id) { - Some(id) + Some((id, typeck.node_args(*path_hir_id))) } else { None } diff --git a/tests/ui/unconditional_recursion.rs b/tests/ui/unconditional_recursion.rs index 35275e81ded..70b390b00e2 100644 --- a/tests/ui/unconditional_recursion.rs +++ b/tests/ui/unconditional_recursion.rs @@ -1,7 +1,11 @@ //@no-rustfix #![warn(clippy::unconditional_recursion)] -#![allow(clippy::partialeq_ne_impl, clippy::default_constructed_unit_structs)] +#![allow( + clippy::partialeq_ne_impl, + clippy::default_constructed_unit_structs, + clippy::only_used_in_recursion +)] enum Foo { A, @@ -350,4 +354,48 @@ fn eq(&self, other: &T) -> bool { } } +// From::from -> Into::into -> From::from +struct BadFromTy1<'a>(&'a ()); +struct BadIntoTy1<'b>(&'b ()); +impl<'a> From> for BadIntoTy1<'static> { + fn from(f: BadFromTy1<'a>) -> Self { + f.into() + } +} + +// Using UFCS syntax +struct BadFromTy2<'a>(&'a ()); +struct BadIntoTy2<'b>(&'b ()); +impl<'a> From> for BadIntoTy2<'static> { + fn from(f: BadFromTy2<'a>) -> Self { + Into::into(f) + } +} + +// Different Into impl (>), so no infinite recursion +struct BadFromTy3; +impl From for i32 { + fn from(f: BadFromTy3) -> Self { + Into::into(1i16) + } +} + +// A conditional return that ends the recursion +struct BadFromTy4; +impl From for i32 { + fn from(f: BadFromTy4) -> Self { + if true { + return 42; + } + f.into() + } +} + +// Types differ in refs, don't lint +impl From<&BadFromTy4> for i32 { + fn from(f: &BadFromTy4) -> Self { + BadFromTy4.into() + } +} + fn main() {} diff --git a/tests/ui/unconditional_recursion.stderr b/tests/ui/unconditional_recursion.stderr index 3fd6c91000e..03c27bd8ed8 100644 --- a/tests/ui/unconditional_recursion.stderr +++ b/tests/ui/unconditional_recursion.stderr @@ -1,5 +1,5 @@ error: function cannot return without recursing - --> tests/ui/unconditional_recursion.rs:42:5 + --> tests/ui/unconditional_recursion.rs:46:5 | LL | fn ne(&self, other: &Self) -> bool { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing @@ -12,7 +12,7 @@ LL | self.ne(other) = help: to override `-D warnings` add `#[allow(unconditional_recursion)]` error: function cannot return without recursing - --> tests/ui/unconditional_recursion.rs:46:5 + --> tests/ui/unconditional_recursion.rs:50:5 | LL | fn eq(&self, other: &Self) -> bool { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing @@ -23,7 +23,7 @@ LL | self.eq(other) = help: a `loop` may express intention better if this is on purpose error: function cannot return without recursing - --> tests/ui/unconditional_recursion.rs:211:5 + --> tests/ui/unconditional_recursion.rs:215:5 | LL | fn to_string(&self) -> String { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing @@ -34,7 +34,7 @@ LL | self.to_string() = help: a `loop` may express intention better if this is on purpose error: function cannot return without recursing - --> tests/ui/unconditional_recursion.rs:221:5 + --> tests/ui/unconditional_recursion.rs:225:5 | LL | fn to_string(&self) -> String { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing @@ -45,7 +45,7 @@ LL | x.to_string() = help: a `loop` may express intention better if this is on purpose error: function cannot return without recursing - --> tests/ui/unconditional_recursion.rs:232:5 + --> tests/ui/unconditional_recursion.rs:236:5 | LL | fn to_string(&self) -> String { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing @@ -56,7 +56,7 @@ LL | (self as &Self).to_string() = help: a `loop` may express intention better if this is on purpose error: function cannot return without recursing - --> tests/ui/unconditional_recursion.rs:12:5 + --> tests/ui/unconditional_recursion.rs:16:5 | LL | / fn ne(&self, other: &Self) -> bool { LL | | @@ -65,7 +65,7 @@ LL | | } | |_____^ | note: recursive call site - --> tests/ui/unconditional_recursion.rs:14:9 + --> tests/ui/unconditional_recursion.rs:18:9 | LL | self != other | ^^^^^^^^^^^^^ @@ -73,7 +73,7 @@ LL | self != other = help: to override `-D warnings` add `#[allow(clippy::unconditional_recursion)]` error: function cannot return without recursing - --> tests/ui/unconditional_recursion.rs:16:5 + --> tests/ui/unconditional_recursion.rs:20:5 | LL | / fn eq(&self, other: &Self) -> bool { LL | | @@ -82,13 +82,13 @@ LL | | } | |_____^ | note: recursive call site - --> tests/ui/unconditional_recursion.rs:18:9 + --> tests/ui/unconditional_recursion.rs:22:9 | LL | self == other | ^^^^^^^^^^^^^ error: function cannot return without recursing - --> tests/ui/unconditional_recursion.rs:28:5 + --> tests/ui/unconditional_recursion.rs:32:5 | LL | / fn ne(&self, other: &Self) -> bool { LL | | self != &Foo2::B // no error here @@ -96,13 +96,13 @@ LL | | } | |_____^ | note: recursive call site - --> tests/ui/unconditional_recursion.rs:29:9 + --> tests/ui/unconditional_recursion.rs:33:9 | LL | self != &Foo2::B // no error here | ^^^^^^^^^^^^^^^^ error: function cannot return without recursing - --> tests/ui/unconditional_recursion.rs:31:5 + --> tests/ui/unconditional_recursion.rs:35:5 | LL | / fn eq(&self, other: &Self) -> bool { LL | | self == &Foo2::B // no error here @@ -110,13 +110,13 @@ LL | | } | |_____^ | note: recursive call site - --> tests/ui/unconditional_recursion.rs:32:9 + --> tests/ui/unconditional_recursion.rs:36:9 | LL | self == &Foo2::B // no error here | ^^^^^^^^^^^^^^^^ error: function cannot return without recursing - --> tests/ui/unconditional_recursion.rs:42:5 + --> tests/ui/unconditional_recursion.rs:46:5 | LL | / fn ne(&self, other: &Self) -> bool { LL | | @@ -125,27 +125,13 @@ LL | | } | |_____^ | note: recursive call site - --> tests/ui/unconditional_recursion.rs:44:9 + --> tests/ui/unconditional_recursion.rs:48:9 | LL | self.ne(other) | ^^^^^^^^^^^^^^ -error: parameter is only used in recursion - --> tests/ui/unconditional_recursion.rs:42:18 - | -LL | fn ne(&self, other: &Self) -> bool { - | ^^^^^ help: if this is intentional, prefix it with an underscore: `_other` - | -note: parameter used here - --> tests/ui/unconditional_recursion.rs:44:17 - | -LL | self.ne(other) - | ^^^^^ - = note: `-D clippy::only-used-in-recursion` implied by `-D warnings` - = help: to override `-D warnings` add `#[allow(clippy::only_used_in_recursion)]` - error: function cannot return without recursing - --> tests/ui/unconditional_recursion.rs:46:5 + --> tests/ui/unconditional_recursion.rs:50:5 | LL | / fn eq(&self, other: &Self) -> bool { LL | | @@ -154,25 +140,13 @@ LL | | } | |_____^ | note: recursive call site - --> tests/ui/unconditional_recursion.rs:48:9 + --> tests/ui/unconditional_recursion.rs:52:9 | LL | self.eq(other) | ^^^^^^^^^^^^^^ -error: parameter is only used in recursion - --> tests/ui/unconditional_recursion.rs:46:18 - | -LL | fn eq(&self, other: &Self) -> bool { - | ^^^^^ help: if this is intentional, prefix it with an underscore: `_other` - | -note: parameter used here - --> tests/ui/unconditional_recursion.rs:48:17 - | -LL | self.eq(other) - | ^^^^^ - error: function cannot return without recursing - --> tests/ui/unconditional_recursion.rs:90:5 + --> tests/ui/unconditional_recursion.rs:94:5 | LL | / fn ne(&self, other: &Self) -> bool { LL | | @@ -181,13 +155,13 @@ LL | | } | |_____^ | note: recursive call site - --> tests/ui/unconditional_recursion.rs:92:9 + --> tests/ui/unconditional_recursion.rs:96:9 | LL | other != self | ^^^^^^^^^^^^^ error: function cannot return without recursing - --> tests/ui/unconditional_recursion.rs:94:5 + --> tests/ui/unconditional_recursion.rs:98:5 | LL | / fn eq(&self, other: &Self) -> bool { LL | | @@ -196,13 +170,13 @@ LL | | } | |_____^ | note: recursive call site - --> tests/ui/unconditional_recursion.rs:96:9 + --> tests/ui/unconditional_recursion.rs:100:9 | LL | other == self | ^^^^^^^^^^^^^ error: function cannot return without recursing - --> tests/ui/unconditional_recursion.rs:104:5 + --> tests/ui/unconditional_recursion.rs:108:5 | LL | / fn ne(&self, other: &Self) -> bool { LL | | @@ -211,13 +185,13 @@ LL | | } | |_____^ | note: recursive call site - --> tests/ui/unconditional_recursion.rs:106:9 + --> tests/ui/unconditional_recursion.rs:110:9 | LL | other != other | ^^^^^^^^^^^^^^ error: equal expressions as operands to `!=` - --> tests/ui/unconditional_recursion.rs:106:9 + --> tests/ui/unconditional_recursion.rs:110:9 | LL | other != other | ^^^^^^^^^^^^^^ @@ -225,7 +199,7 @@ LL | other != other = note: `#[deny(clippy::eq_op)]` on by default error: function cannot return without recursing - --> tests/ui/unconditional_recursion.rs:108:5 + --> tests/ui/unconditional_recursion.rs:112:5 | LL | / fn eq(&self, other: &Self) -> bool { LL | | @@ -234,19 +208,19 @@ LL | | } | |_____^ | note: recursive call site - --> tests/ui/unconditional_recursion.rs:110:9 + --> tests/ui/unconditional_recursion.rs:114:9 | LL | other == other | ^^^^^^^^^^^^^^ error: equal expressions as operands to `==` - --> tests/ui/unconditional_recursion.rs:110:9 + --> tests/ui/unconditional_recursion.rs:114:9 | LL | other == other | ^^^^^^^^^^^^^^ error: function cannot return without recursing - --> tests/ui/unconditional_recursion.rs:117:5 + --> tests/ui/unconditional_recursion.rs:121:5 | LL | / fn ne(&self, _other: &Self) -> bool { LL | | @@ -255,19 +229,19 @@ LL | | } | |_____^ | note: recursive call site - --> tests/ui/unconditional_recursion.rs:119:9 + --> tests/ui/unconditional_recursion.rs:123:9 | LL | self != self | ^^^^^^^^^^^^ error: equal expressions as operands to `!=` - --> tests/ui/unconditional_recursion.rs:119:9 + --> tests/ui/unconditional_recursion.rs:123:9 | LL | self != self | ^^^^^^^^^^^^ error: function cannot return without recursing - --> tests/ui/unconditional_recursion.rs:121:5 + --> tests/ui/unconditional_recursion.rs:125:5 | LL | / fn eq(&self, _other: &Self) -> bool { LL | | @@ -276,19 +250,19 @@ LL | | } | |_____^ | note: recursive call site - --> tests/ui/unconditional_recursion.rs:123:9 + --> tests/ui/unconditional_recursion.rs:127:9 | LL | self == self | ^^^^^^^^^^^^ error: equal expressions as operands to `==` - --> tests/ui/unconditional_recursion.rs:123:9 + --> tests/ui/unconditional_recursion.rs:127:9 | LL | self == self | ^^^^^^^^^^^^ error: function cannot return without recursing - --> tests/ui/unconditional_recursion.rs:149:13 + --> tests/ui/unconditional_recursion.rs:153:13 | LL | / fn eq(&self, other: &Self) -> bool { LL | | @@ -300,7 +274,7 @@ LL | impl_partial_eq!(S5); | -------------------- in this macro invocation | note: recursive call site - --> tests/ui/unconditional_recursion.rs:151:17 + --> tests/ui/unconditional_recursion.rs:155:17 | LL | self == other | ^^^^^^^^^^^^^ @@ -310,7 +284,7 @@ LL | impl_partial_eq!(S5); = note: this error originates in the macro `impl_partial_eq` (in Nightly builds, run with -Z macro-backtrace for more info) error: function cannot return without recursing - --> tests/ui/unconditional_recursion.rs:178:5 + --> tests/ui/unconditional_recursion.rs:182:5 | LL | / fn eq(&self, other: &Self) -> bool { LL | | @@ -321,13 +295,13 @@ LL | | } | |_____^ | note: recursive call site - --> tests/ui/unconditional_recursion.rs:182:9 + --> tests/ui/unconditional_recursion.rs:186:9 | LL | mine == theirs | ^^^^^^^^^^^^^^ error: function cannot return without recursing - --> tests/ui/unconditional_recursion.rs:247:5 + --> tests/ui/unconditional_recursion.rs:251:5 | LL | / fn new() -> Self { LL | | @@ -336,13 +310,13 @@ LL | | } | |_____^ | note: recursive call site - --> tests/ui/unconditional_recursion.rs:249:9 + --> tests/ui/unconditional_recursion.rs:253:9 | LL | Self::default() | ^^^^^^^^^^^^^^^ error: function cannot return without recursing - --> tests/ui/unconditional_recursion.rs:286:5 + --> tests/ui/unconditional_recursion.rs:290:5 | LL | / fn eq(&self, other: &Self) -> bool { LL | | @@ -353,10 +327,38 @@ LL | | } | |_____^ | note: recursive call site - --> tests/ui/unconditional_recursion.rs:290:9 + --> tests/ui/unconditional_recursion.rs:294:9 | LL | mine.eq(theirs) | ^^^^^^^^^^^^^^^ +error: function cannot return without recursing + --> tests/ui/unconditional_recursion.rs:361:5 + | +LL | / fn from(f: BadFromTy1<'a>) -> Self { +LL | | f.into() +LL | | } + | |_____^ + | +note: recursive call site + --> tests/ui/unconditional_recursion.rs:362:9 + | +LL | f.into() + | ^^^^^^^^ + +error: function cannot return without recursing + --> tests/ui/unconditional_recursion.rs:370:5 + | +LL | / fn from(f: BadFromTy2<'a>) -> Self { +LL | | Into::into(f) +LL | | } + | |_____^ + | +note: recursive call site + --> tests/ui/unconditional_recursion.rs:371:9 + | +LL | Into::into(f) + | ^^^^^^^^^^^^^ + error: aborting due to 27 previous errors