diff --git a/CHANGELOG.md b/CHANGELOG.md index 0cfe89ad3787..a3426476c8b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5721,6 +5721,7 @@ Released 2018-09-13 [`derive_ord_xor_partial_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#derive_ord_xor_partial_ord [`derive_partial_eq_without_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#derive_partial_eq_without_eq [`derived_hash_with_manual_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#derived_hash_with_manual_eq +[`direct_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#direct_recursion [`disallowed_macros`]: https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_macros [`disallowed_method`]: https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_method [`disallowed_methods`]: https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_methods diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 1e3907d9ede0..6c2e9f7f6f41 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -107,6 +107,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::derive::DERIVE_PARTIAL_EQ_WITHOUT_EQ_INFO, crate::derive::EXPL_IMPL_CLONE_ON_COPY_INFO, crate::derive::UNSAFE_DERIVE_DESERIALIZE_INFO, + crate::direct_recursion::DIRECT_RECURSION_INFO, crate::disallowed_macros::DISALLOWED_MACROS_INFO, crate::disallowed_methods::DISALLOWED_METHODS_INFO, crate::disallowed_names::DISALLOWED_NAMES_INFO, diff --git a/clippy_lints/src/direct_recursion.rs b/clippy_lints/src/direct_recursion.rs new file mode 100644 index 000000000000..c02844439f54 --- /dev/null +++ b/clippy_lints/src/direct_recursion.rs @@ -0,0 +1,164 @@ +use clippy_utils::diagnostics::{span_lint, span_lint_hir}; +use clippy_utils::get_parent_expr; +use rustc_hir::def::{DefKind, Res}; +use rustc_hir::{Body, Expr, ExprKind, QPath}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::Instance; +use rustc_session::impl_lint_pass; +use rustc_span::def_id::DefId; + +declare_clippy_lint! { + /// ### What it does + /// Checks for functions that call themselves from their body. + /// + /// ### Why restrict this? + /// In Safety Critical contexts, recursive calls can lead to catastrophic + /// crashes if they happen to overflow the stack. + /// + /// In most contexts, this is not an issue, so this lint is allow-by-default. + /// + /// ### Notes + /// + /// #### Triggers + /// There are two things that trigger this lint: + /// - Function calls from a function (or method) to itself, + /// - Function pointer bindings from a function (or method) to itself. + /// + /// #### Independent of control flow + /// This lint triggers whenever the conditions above are met, regardless of + /// control flow and other such constructs. + /// + /// #### Blessing a recursive call + /// The user may choose to bless a recursive call or binding using the + /// attribute #[clippy::allowed_recursion] + /// + /// #### Indirect calls + /// This lint **does not** detect indirect recursive calls. + /// + /// ### Examples + /// This function will trigger the lint: + /// ```no_run + /// fn i_call_myself_in_a_bounded_way(bound: u8) { + /// if bound > 0 { + /// // This line will trigger the lint + /// i_call_myself_in_a_bounded_way(bound - 1); + /// } + /// } + /// ``` + /// Using #[clippy::allowed_recursion] lets it pass: + /// ```no_run + /// fn i_call_myself_in_a_bounded_way(bound: u8) { + /// if bound > 0 { + /// #[clippy::allowed_recursion] + /// i_call_myself_in_a_bounded_way(bound - 1); + /// } + /// } + /// ``` + /// This triggers the lint when `fibo` is bound to a function pointer + /// inside `fibo`'s body + /// ```no_run + /// fn fibo(a: u32) -> u32 { + /// if a < 2 { a } else { (a - 2..a).map(fibo).sum() } + /// } + /// ``` + #[clippy::version = "1.89.0"] + pub DIRECT_RECURSION, + restriction, + "functions shall not call themselves directly" +} + +#[derive(Default)] +pub struct DirectRecursion { + fn_id_stack: Vec, +} + +impl_lint_pass!(DirectRecursion => [DIRECT_RECURSION]); + +impl<'tcx> LateLintPass<'tcx> for DirectRecursion { + /// Whenever we enter a Body, we push its owner's `DefId` into the stack + fn check_body(&mut self, cx: &LateContext<'tcx>, body: &Body<'_>) { + self.fn_id_stack + .push(cx.tcx.hir_body_owner_def_id(body.id()).to_def_id()); + } + + /// We then revert this when we exit said `Body` + fn check_body_post(&mut self, _: &LateContext<'tcx>, _: &Body<'_>) { + _ = self.fn_id_stack.pop(); + } + + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { + match expr.kind { + ExprKind::MethodCall(_, _, _, _) => { + let typeck = cx.typeck_results(); + if let Some(basic_id) = typeck.type_dependent_def_id(expr.hir_id) { + // This finds default Trait implementations of methods + if self.fn_id_stack.contains(&basic_id) { + span_lint(cx, DIRECT_RECURSION, expr.span, "this method contains a call to itself"); + } + // Whereas this finds non-default implementations + else if let args = typeck.node_args(expr.hir_id) + && let Ok(Some(fn_def)) = Instance::try_resolve(cx.tcx, cx.typing_env(), basic_id, args) + && let type_resolved_def_id = fn_def.def_id() + && self.fn_id_stack.contains(&type_resolved_def_id) + { + span_lint(cx, DIRECT_RECURSION, expr.span, "this method contains a call to itself"); + } + } + }, + ExprKind::Path(QPath::TypeRelative(_, _)) => { + // I'm still not sure this is proper. + // It definitely finds the right `DefId`, though. + let typeck = cx.typeck_results(); + if let Some(id) = typeck.type_dependent_def_id(expr.hir_id) + && let args = typeck.node_args(expr.hir_id) + && let Ok(Some(fn_def)) = Instance::try_resolve(cx.tcx, cx.typing_env(), id, args) + { + let type_resolved_def_id = fn_def.def_id(); + + if self.fn_id_stack.contains(&type_resolved_def_id) { + emit_lint(cx, expr); + } + } + }, + // This branch takes care of finding bindings of function and method names + // into fn pointers. + ExprKind::Path(QPath::Resolved(_, path)) => { + // Now we know that this Path is fully resolved. + // We now must check if it points to a function or a method's definition. + if let Res::Def(DefKind::Fn | DefKind::AssocFn, fn_path_id) = path.res + // 1) Now we know that the path we've found is of a function or method definition. + // + // 2) We will now check if it corresponds to the path of a function we're inside + // of. + // + // 3) Thankfully, we've kept track of the functions that surround us, in + //`self.fn_id_stack`. + // + // 4) If the path that we've captured from `expr` coincides with one of the functions + // in the stack, then we know we have a recursive loop. + + && self.fn_id_stack.contains(&fn_path_id) + { + emit_lint(cx, expr); + } + }, + _ => {}, + } + } +} + +fn emit_lint<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { + let (node_id, span, msg) = if let Some(parent_expr) = get_parent_expr(cx, expr) + && let ExprKind::Call(func, _) = parent_expr.kind + && func.hir_id == expr.hir_id + { + ( + parent_expr.hir_id, + parent_expr.span, + "this function contains a call to itself", + ) + } else { + (expr.hir_id, expr.span, "this function creates a reference to itself") + }; + span_lint_hir(cx, DIRECT_RECURSION, node_id, span, msg); +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index d8bd9dc8d2a6..f28c218d156c 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -114,6 +114,7 @@ mod default_union_representation; mod dereference; mod derivable_impls; mod derive; +mod direct_recursion; mod disallowed_macros; mod disallowed_methods; mod disallowed_names; @@ -951,5 +952,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { store.register_late_pass(|_| Box::new(cloned_ref_to_slice_refs::ClonedRefToSliceRefs::new(conf))); store.register_late_pass(|_| Box::new(infallible_try_from::InfallibleTryFrom)); store.register_late_pass(|_| Box::new(coerce_container_to_any::CoerceContainerToAny)); + store.register_late_pass(|_| Box::::default()); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/tests/ui/direct_recursion.rs b/tests/ui/direct_recursion.rs new file mode 100644 index 000000000000..b402f34127b6 --- /dev/null +++ b/tests/ui/direct_recursion.rs @@ -0,0 +1,171 @@ +#![deny(clippy::direct_recursion)] +#![feature(stmt_expr_attributes)] + +// Basic Cases // + +#[allow(unconditional_recursion, reason = "We're not testing for that lint")] +fn i_call_myself_always() { + i_call_myself_always(); + //~^ direct_recursion +} + +fn i_call_myself_conditionally(do_i: bool) { + if do_i { + i_call_myself_conditionally(false); + //~^ direct_recursion + } +} + +// Basic Counterexamples // + +fn i_get_called_by_others() {} + +fn i_call_something_else() { + i_get_called_by_others(); +} + +// Elaborate Cases // + +// Case 1: Blessing // +// Here we check that we're allowed to bless specific recursive calls. +// A fine-grained control of where to allow recursion is desirable. +// This is a test of such a feature. +fn i_call_myself_in_a_bounded_way(bound: u8) { + if bound > 0 { + #[expect(clippy::direct_recursion)] + i_call_myself_in_a_bounded_way(bound - 1); + } +} + +// Case 2: Blessing is Bounded // +// Here we check that blessing a specific recursive call doesn't +// let other recursive calls go through. +fn i_have_one_blessing_but_two_calls(bound: u8) { + if bound > 25 { + #[expect(clippy::direct_recursion)] + i_have_one_blessing_but_two_calls(bound - 1); + } else if bound > 0 { + // "WIP: we still need to audit this part of the function" + i_have_one_blessing_but_two_calls(bound - 2) + //~^ direct_recursion + } +} + +// Case 3: Blessing is Recursive // +// Here we check that blessing a specific expression will +// bless everything inside of that expression as well. +fn ackermann(m: u64, n: u64) -> u64 { + if m == 0 { + n + 1 + } else if n == 0 { + #[expect(clippy::direct_recursion)] + ackermann(m - 1, 1) + } else { + #[expect(clippy::direct_recursion)] + ackermann(m, ackermann(m + 1, n)) + } +} + +// Case 4: Linting is Recursive // +// Here we check that linting a specific expression will +// not block other expressions inside of it from being linted. +fn ackermann_2_electric_recursion(m: u64, n: u64) -> u64 { + if m == 0 { + n + 1 + } else if n == 0 { + #[expect(clippy::direct_recursion)] + ackermann_2_electric_recursion(m - 1, 1) + } else { + ackermann_2_electric_recursion( + //~^ direct_recursion + m, + ackermann_2_electric_recursion(m + 1, n), + //~^ direct_recursion + ) + } +} + +// Case 5: Nesting Functions // +// Here we check that nested functions calling their parents are still +// linted +fn grand_parent() { + fn parent() { + fn child() { + parent(); + //~^ direct_recursion + grand_parent(); + //~^ direct_recursion + } + grand_parent(); + //~^ direct_recursion + } +} + +// Case 6: Binding of path to a Fn pointer // +// Here we check that we are able to detect bindings of function names +// as edges for the function call graph. +fn fibo(a: u32) -> u32 { + if a < 2 { a } else { (a - 2..a).map(fibo).sum() } + //~^ direct_recursion +} + +// Case 7: Linting on Associated Function Implementations // +// Here we check that different implementations of the same trait don't go +// linting calls to functions of implementations that are not their own. +trait RecSum { + fn rec_sum(n: u32) -> u32; +} + +struct Summer; + +// Recursive Call: should be linted +impl RecSum for Summer { + fn rec_sum(n: u32) -> u32 { + if n == 0 { + 0 + } else { + // Notice how this is a recursive call, whereas the next one isn't + n + Self::rec_sum(n - 1) + //~^ direct_recursion + } + } +} + +struct Winter; + +// Not a Recursive Call: should be ignored. +impl RecSum for Winter { + fn rec_sum(n: u32) -> u32 { + // This should NOT trigger the lint, because even though it's calling the same + // function (or "the same symbol"), it's not recursively calling its own implementation. + if n == 0 { 0 } else { n + Summer::rec_sum(n - 1) } + } +} + +// Case 8: Linting on Default Trait Method Implementations // +// Here we check that recursion in trait methods is also captured by the lint +trait MyTrait { + fn myfun(&self, num: i32) { + if num > 0 { + self.myfun(num - 1); + //~^ direct_recursion + } + } +} + +// Case 9: Linting on Trait Method Implementations // + +struct T(u32); + +trait W { + fn f(&self); +} + +impl W for T { + fn f(&self) { + if self.0 > 0 { + self.f() + //~^ direct_recursion + } + } +} diff --git a/tests/ui/direct_recursion.stderr b/tests/ui/direct_recursion.stderr new file mode 100644 index 000000000000..425be4a23458 --- /dev/null +++ b/tests/ui/direct_recursion.stderr @@ -0,0 +1,85 @@ +error: this function contains a call to itself + --> tests/ui/direct_recursion.rs:8:5 + | +LL | i_call_myself_always(); + | ^^^^^^^^^^^^^^^^^^^^^^ + | +note: the lint level is defined here + --> tests/ui/direct_recursion.rs:1:9 + | +LL | #![deny(clippy::direct_recursion)] + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this function contains a call to itself + --> tests/ui/direct_recursion.rs:14:9 + | +LL | i_call_myself_conditionally(false); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this function contains a call to itself + --> tests/ui/direct_recursion.rs:49:9 + | +LL | i_have_one_blessing_but_two_calls(bound - 2) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this function contains a call to itself + --> tests/ui/direct_recursion.rs:79:9 + | +LL | / ackermann_2_electric_recursion( +LL | | +LL | | m, +LL | | ackermann_2_electric_recursion(m + 1, n), +LL | | +LL | | ) + | |_________^ + +error: this function contains a call to itself + --> tests/ui/direct_recursion.rs:82:13 + | +LL | ackermann_2_electric_recursion(m + 1, n), + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: this function contains a call to itself + --> tests/ui/direct_recursion.rs:94:13 + | +LL | parent(); + | ^^^^^^^^ + +error: this function contains a call to itself + --> tests/ui/direct_recursion.rs:96:13 + | +LL | grand_parent(); + | ^^^^^^^^^^^^^^ + +error: this function contains a call to itself + --> tests/ui/direct_recursion.rs:99:9 + | +LL | grand_parent(); + | ^^^^^^^^^^^^^^ + +error: this function creates a reference to itself + --> tests/ui/direct_recursion.rs:108:42 + | +LL | if a < 2 { a } else { (a - 2..a).map(fibo).sum() } + | ^^^^ + +error: this function contains a call to itself + --> tests/ui/direct_recursion.rs:128:17 + | +LL | n + Self::rec_sum(n - 1) + | ^^^^^^^^^^^^^^^^^^^^ + +error: this method contains a call to itself + --> tests/ui/direct_recursion.rs:150:13 + | +LL | self.myfun(num - 1); + | ^^^^^^^^^^^^^^^^^^^ + +error: this method contains a call to itself + --> tests/ui/direct_recursion.rs:167:13 + | +LL | self.f() + | ^^^^^^^^ + +error: aborting due to 12 previous errors +