Skip to content

Commit 09c21cd

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

File tree

8 files changed

+458
-0
lines changed

8 files changed

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

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)