Skip to content

Commit d6e4917

Browse files
committed
Add new lint: direct-recursion Lint
changelog: new lint: [`direct_recursion`]
1 parent f2922e7 commit d6e4917

File tree

8 files changed

+472
-0
lines changed

8 files changed

+472
-0
lines changed

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5721,6 +5721,7 @@ Released 2018-09-13
57215721
[`derive_ord_xor_partial_ord`]: https://rust-lang.github.io/rust-clippy/master/index.html#derive_ord_xor_partial_ord
57225722
[`derive_partial_eq_without_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#derive_partial_eq_without_eq
57235723
[`derived_hash_with_manual_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#derived_hash_with_manual_eq
5724+
[`direct_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#direct_recursion
57245725
[`disallowed_macros`]: https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_macros
57255726
[`disallowed_method`]: https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_method
57265727
[`disallowed_methods`]: https://rust-lang.github.io/rust-clippy/master/index.html#disallowed_methods

clippy_lints/src/declared_lints.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
107107
crate::derive::DERIVE_PARTIAL_EQ_WITHOUT_EQ_INFO,
108108
crate::derive::EXPL_IMPL_CLONE_ON_COPY_INFO,
109109
crate::derive::UNSAFE_DERIVE_DESERIALIZE_INFO,
110+
crate::direct_recursion::DIRECT_RECURSION_INFO,
110111
crate::disallowed_macros::DISALLOWED_MACROS_INFO,
111112
crate::disallowed_methods::DISALLOWED_METHODS_INFO,
112113
crate::disallowed_names::DISALLOWED_NAMES_INFO,

clippy_lints/src/direct_recursion.rs

Lines changed: 214 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,214 @@
1+
use crate::rustc_lint::LintContext;
2+
use clippy_config::Conf;
3+
use clippy_utils::diagnostics::{span_lint, span_lint_hir};
4+
use clippy_utils::{get_attr, get_parent_expr, sym};
5+
use rustc_hir::def::{DefKind, Res};
6+
use rustc_hir::{Body, Expr, ExprKind, HirId, QPath};
7+
use rustc_lint::{LateContext, LateLintPass};
8+
use rustc_middle::ty::Instance;
9+
use rustc_session::impl_lint_pass;
10+
use rustc_span::def_id::DefId;
11+
12+
declare_clippy_lint! {
13+
/// ### What it does
14+
/// Checks for functions that call themselves from their body.
15+
///
16+
/// ### Why restrict this?
17+
/// In Safety Critical contexts, recursive calls can lead to catastrophic
18+
/// crashes if they happen to overflow the stack.
19+
///
20+
/// In most contexts, this is not an issue, so this lint is allow-by-default.
21+
///
22+
/// ### Notes
23+
///
24+
/// #### Triggers
25+
/// There are two things that trigger this lint:
26+
/// - Function calls from a function (or method) to itself,
27+
/// - Function pointer bindings from a function (or method) to itself.
28+
///
29+
/// #### Independent of control flow
30+
/// This lint triggers whenever the conditions above are met, regardless of
31+
/// control flow and other such constructs.
32+
///
33+
/// #### Blessing a recursive call
34+
/// The user may choose to bless a recursive call or binding using the
35+
/// attribute #[clippy::allowed_recursion]
36+
///
37+
/// #### Indirect calls
38+
/// This lint **does not** detect indirect recursive calls.
39+
///
40+
/// ### Examples
41+
/// This function will trigger the lint:
42+
/// ```no_run
43+
/// fn i_call_myself_in_a_bounded_way(bound: u8) {
44+
/// if bound > 0 {
45+
/// // This line will trigger the lint
46+
/// i_call_myself_in_a_bounded_way(bound - 1);
47+
/// }
48+
/// }
49+
/// ```
50+
/// Using #[clippy::allowed_recursion] lets it pass:
51+
/// ```no_run
52+
/// fn i_call_myself_in_a_bounded_way(bound: u8) {
53+
/// if bound > 0 {
54+
/// #[clippy::allowed_recursion]
55+
/// i_call_myself_in_a_bounded_way(bound - 1);
56+
/// }
57+
/// }
58+
/// ```
59+
/// This triggers the lint when `fibo` is bound to a function pointer
60+
/// inside `fibo`'s body
61+
/// ```no_run
62+
/// fn fibo(a: u32) -> u32 {
63+
/// if a < 2 { a } else { (a - 2..a).map(fibo).sum() }
64+
/// }
65+
/// ```
66+
#[clippy::version = "1.89.0"]
67+
pub DIRECT_RECURSION,
68+
restriction,
69+
"functions shall not call themselves directly"
70+
}
71+
72+
pub struct DirectRecursion {
73+
fn_id_stack: Vec<DefId>,
74+
expr_check_blocker: Option<HirId>,
75+
}
76+
77+
impl DirectRecursion {
78+
pub fn new(_: &'static Conf) -> Self {
79+
Self {
80+
// We preallocate a stack of size 4, because we'll probably need more than 2
81+
// but I really don't expect us to ever see more than 4 nested functions
82+
fn_id_stack: Vec::new(),
83+
expr_check_blocker: None,
84+
}
85+
}
86+
87+
/// Blocks checking more expressions, using `expr` as the key.
88+
fn block_with_expr(&mut self, expr: &Expr<'_>) {
89+
self.expr_check_blocker = Some(expr.hir_id);
90+
}
91+
92+
/// Tells whether we're currently allowed to check expressions or not
93+
fn is_blocked(&self) -> bool {
94+
self.expr_check_blocker.is_some()
95+
}
96+
97+
/// Tries opening the lock using `expr` as the key.
98+
fn try_unlocking_with(&mut self, expr: &Expr<'_>) {
99+
if let Some(key) = self.expr_check_blocker
100+
&& key == expr.hir_id
101+
{
102+
self.expr_check_blocker = None;
103+
}
104+
}
105+
}
106+
107+
impl_lint_pass!(DirectRecursion => [DIRECT_RECURSION]);
108+
109+
impl<'tcx> LateLintPass<'tcx> for DirectRecursion {
110+
/// Whenever we enter a Body, we push its owner's `DefId` into the stack
111+
fn check_body(&mut self, cx: &LateContext<'tcx>, body: &Body<'_>) {
112+
self.fn_id_stack
113+
.push(cx.tcx.hir_body_owner_def_id(body.id()).to_def_id());
114+
}
115+
116+
/// We then revert this when we exit said `Body`
117+
fn check_body_post(&mut self, _: &LateContext<'tcx>, _: &Body<'_>) {
118+
_ = self.fn_id_stack.pop();
119+
}
120+
121+
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
122+
// We use this inner lock so that we avoid recursing if we've found
123+
// the `clippy::allowed_recursion` attribute above the current node.
124+
// This lock will be opened after we exit the node that closed it
125+
// (see `check_expr_post`)
126+
if self.is_blocked() {
127+
return;
128+
}
129+
130+
// Before running the lint, we look up the attributes of this Expr.
131+
// If it has been marked with `clippy::allowed_recursion`, then
132+
// we ignore it, as well as everything inside it; someone has
133+
// decided that the recursive calls within it are fine.
134+
let attrs = cx.tcx.hir_attrs(expr.hir_id);
135+
if get_attr(cx.sess(), attrs, sym::allowed_recursion).next().is_some() {
136+
self.block_with_expr(expr); // This lock ensures we stop recursing inside of it
137+
return;
138+
}
139+
140+
match expr.kind {
141+
ExprKind::MethodCall(_, _, _, _) => {
142+
// Uniquely identifying the `DefId` of method calls requires doing type checking.
143+
// `cx` takes care of that, and then we can ask for the `type_dependent_def_id`
144+
// of the `expr` whose kind is `ExprKind::MethodCall`.
145+
let typeck = cx.typeck_results();
146+
if let Some(type_resolved_def_id) = typeck.type_dependent_def_id(expr.hir_id) {
147+
if self.fn_id_stack.contains(&type_resolved_def_id) {
148+
span_lint(cx, DIRECT_RECURSION, expr.span, "this method contains a call to itself");
149+
}
150+
}
151+
},
152+
ExprKind::Path(QPath::TypeRelative(_, _)) => {
153+
// I'm still not sure this is proper.
154+
// It definitely finds the right `DefId`, though.
155+
let typeck = cx.typeck_results();
156+
if let Some(id) = typeck.type_dependent_def_id(expr.hir_id)
157+
&& let args = typeck.node_args(expr.hir_id)
158+
&& let Ok(Some(fn_def)) = Instance::try_resolve(cx.tcx, cx.typing_env(), id, args)
159+
{
160+
let type_resolved_def_id = fn_def.def_id();
161+
162+
if self.fn_id_stack.contains(&type_resolved_def_id) {
163+
emit_lint(cx, expr);
164+
}
165+
}
166+
},
167+
// This branch takes care of finding bindings of function and method names
168+
// into fn pointers.
169+
ExprKind::Path(QPath::Resolved(_, path)) => {
170+
// Now we know that this Path is fully resolved.
171+
// We now must check if it points to a function or a method's definition.
172+
if let Res::Def(DefKind::Fn | DefKind::AssocFn, fn_path_id) = path.res {
173+
// 1) Now we know that the path we've found is of a function or method definition.
174+
//
175+
// 2) We will now check if it corresponds to the path of a function we're inside
176+
// of.
177+
//
178+
// 3) Thankfully, we've kept track of the functions that surround us, in
179+
//`self.fn_id_stack`.
180+
//
181+
// 4) If the path that we've captured from `expr` coincides with one of the functions
182+
// in the stack, then we know we have a recursive loop.
183+
184+
if self.fn_id_stack.contains(&fn_path_id) {
185+
emit_lint(cx, expr);
186+
}
187+
}
188+
},
189+
_ => {},
190+
}
191+
}
192+
193+
/// Every time we exit an `Expr` node, we see if we can unlock our Visitor
194+
/// using it as the key, just in case we blocked it after we entered it.
195+
fn check_expr_post(&mut self, _: &LateContext<'tcx>, expr: &Expr<'tcx>) {
196+
self.try_unlocking_with(expr);
197+
}
198+
}
199+
200+
fn emit_lint<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
201+
let (node_id, span, msg) = if let Some(parent_expr) = get_parent_expr(cx, expr)
202+
&& let ExprKind::Call(func, _) = parent_expr.kind
203+
&& func.hir_id == expr.hir_id
204+
{
205+
(
206+
parent_expr.hir_id,
207+
parent_expr.span,
208+
"this function contains a call to itself",
209+
)
210+
} else {
211+
(expr.hir_id, expr.span, "this function creates a reference to itself")
212+
};
213+
span_lint_hir(cx, DIRECT_RECURSION, node_id, span, msg);
214+
}

clippy_lints/src/lib.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,7 @@ mod default_union_representation;
114114
mod dereference;
115115
mod derivable_impls;
116116
mod derive;
117+
mod direct_recursion;
117118
mod disallowed_macros;
118119
mod disallowed_methods;
119120
mod disallowed_names;
@@ -951,5 +952,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
951952
store.register_late_pass(|_| Box::new(cloned_ref_to_slice_refs::ClonedRefToSliceRefs::new(conf)));
952953
store.register_late_pass(|_| Box::new(infallible_try_from::InfallibleTryFrom));
953954
store.register_late_pass(|_| Box::new(coerce_container_to_any::CoerceContainerToAny));
955+
store.register_late_pass(move |_| Box::new(direct_recursion::DirectRecursion::new(conf)));
954956
// add lints here, do not remove this comment, it's used in `new_lint`
955957
}

clippy_utils/src/attrs.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ pub const BUILTIN_ATTRIBUTES: &[(Symbol, DeprecationStatus)] = &[
2828
(sym::cyclomatic_complexity, DeprecationStatus::Replaced("cognitive_complexity")),
2929
(sym::dump, DeprecationStatus::None),
3030
(sym::msrv, DeprecationStatus::None),
31+
(sym::allowed_recursion, DeprecationStatus::None),
3132
// The following attributes are for the 3rd party crate authors.
3233
// See book/src/attribs.md
3334
(sym::has_significant_drop, DeprecationStatus::None),

clippy_utils/src/sym.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ generate! {
7777
Weak,
7878
abs,
7979
align_of,
80+
allowed_recursion,
8081
ambiguous_glob_reexports,
8182
append,
8283
arg,

0 commit comments

Comments
 (0)