From fbef241709093b3fb18cb12500fd33e4ab964b62 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Thu, 1 Jan 2015 03:06:38 +1100 Subject: [PATCH] Add a lint to detect unconditional recursion. E.g. `fn foo() { foo() }`, or, more subtlely impl Foo for Box { fn bar(&self) { self.bar(); } } The compiler will warn and point out the points where recursion occurs, if it determines that the function cannot return without calling itself. Closes #17899. --- src/librustc/lint/builtin.rs | 192 +++++++++++++++++- src/librustc/lint/context.rs | 1 + .../lint-unconditional-recursion.rs | 66 ++++++ 3 files changed, 258 insertions(+), 1 deletion(-) create mode 100644 src/test/compile-fail/lint-unconditional-recursion.rs diff --git a/src/librustc/lint/builtin.rs b/src/librustc/lint/builtin.rs index fef1017b78285..531bdc2941e04 100644 --- a/src/librustc/lint/builtin.rs +++ b/src/librustc/lint/builtin.rs @@ -32,10 +32,12 @@ use middle::subst::Substs; use middle::ty::{self, Ty}; use middle::{def, pat_util, stability}; use middle::const_eval::{eval_const_expr_partial, const_int, const_uint}; +use middle::cfg; use util::ppaux::{ty_to_string}; use util::nodemap::{FnvHashMap, NodeSet}; -use lint::{Context, LintPass, LintArray, Lint}; +use lint::{Level, Context, LintPass, LintArray, Lint}; +use std::collections::BitvSet; use std::collections::hash_map::Entry::{Occupied, Vacant}; use std::num::SignedInt; use std::{cmp, slice}; @@ -1788,6 +1790,194 @@ impl LintPass for Stability { } } +declare_lint! { + pub UNCONDITIONAL_RECURSION, + Warn, + "functions that cannot return without calling themselves" +} + +#[derive(Copy)] +pub struct UnconditionalRecursion; + + +impl LintPass for UnconditionalRecursion { + fn get_lints(&self) -> LintArray { + lint_array![UNCONDITIONAL_RECURSION] + } + + fn check_fn(&mut self, cx: &Context, fn_kind: visit::FnKind, _: &ast::FnDecl, + blk: &ast::Block, sp: Span, id: ast::NodeId) { + type F = for<'tcx> fn(&ty::ctxt<'tcx>, + ast::NodeId, ast::NodeId, ast::Ident, ast::NodeId) -> bool; + + let (name, checker) = match fn_kind { + visit::FkItemFn(name, _, _, _) => (name, id_refers_to_this_fn as F), + visit::FkMethod(name, _, _) => (name, id_refers_to_this_method as F), + // closures can't recur, so they don't matter. + visit::FkFnBlock => return + }; + + let impl_def_id = ty::impl_of_method(cx.tcx, ast_util::local_def(id)) + .unwrap_or(ast_util::local_def(ast::DUMMY_NODE_ID)); + assert!(ast_util::is_local(impl_def_id)); + let impl_node_id = impl_def_id.node; + + // Walk through this function (say `f`) looking to see if + // every possible path references itself, i.e. the function is + // called recursively unconditionally. This is done by trying + // to find a path from the entry node to the exit node that + // *doesn't* call `f` by traversing from the entry while + // pretending that calls of `f` are sinks (i.e. ignoring any + // exit edges from them). + // + // NB. this has an edge case with non-returning statements, + // like `loop {}` or `panic!()`: control flow never reaches + // the exit node through these, so one can have a function + // that never actually calls itselfs but is still picked up by + // this lint: + // + // fn f(cond: bool) { + // if !cond { panic!() } // could come from `assert!(cond)` + // f(false) + // } + // + // In general, functions of that form may be able to call + // itself a finite number of times and then diverge. The lint + // considers this to be an error for two reasons, (a) it is + // easier to implement, and (b) it seems rare to actually want + // to have behaviour like the above, rather than + // e.g. accidentally recurring after an assert. + + let cfg = cfg::CFG::new(cx.tcx, blk); + + let mut work_queue = vec![cfg.entry]; + let mut reached_exit_without_self_call = false; + let mut self_call_spans = vec![]; + let mut visited = BitvSet::new(); + + while let Some(idx) = work_queue.pop() { + let cfg_id = idx.node_id(); + if idx == cfg.exit { + // found a path! + reached_exit_without_self_call = true; + break + } else if visited.contains(&cfg_id) { + // already done + continue + } + visited.insert(cfg_id); + let node_id = cfg.graph.node_data(idx).id; + + // is this a recursive call? + if node_id != ast::DUMMY_NODE_ID && checker(cx.tcx, impl_node_id, id, name, node_id) { + + self_call_spans.push(cx.tcx.map.span(node_id)); + // this is a self call, so we shouldn't explore past + // this node in the CFG. + continue + } + // add the successors of this node to explore the graph further. + cfg.graph.each_outgoing_edge(idx, |_, edge| { + let target_idx = edge.target(); + let target_cfg_id = target_idx.node_id(); + if !visited.contains(&target_cfg_id) { + work_queue.push(target_idx) + } + true + }); + } + + // check the number of sell calls because a function that + // doesn't return (e.g. calls a `-> !` function or `loop { /* + // no break */ }`) shouldn't be linted unless it actually + // recurs. + if !reached_exit_without_self_call && self_call_spans.len() > 0 { + cx.span_lint(UNCONDITIONAL_RECURSION, sp, + "function cannot return without recurring"); + + // FIXME #19668: these could be span_lint_note's instead of this manual guard. + if cx.current_level(UNCONDITIONAL_RECURSION) != Level::Allow { + let sess = cx.sess(); + // offer some help to the programmer. + for call in self_call_spans.iter() { + sess.span_note(*call, "recursive call site") + } + sess.span_help(sp, "a `loop` may express intention better if this is on purpose") + } + } + + // all done + return; + + // Functions for identifying if the given NodeId `id` + // represents a call to the function `fn_id`/method + // `method_id`. + + fn id_refers_to_this_fn<'tcx>(tcx: &ty::ctxt<'tcx>, + _: ast::NodeId, + fn_id: ast::NodeId, + _: ast::Ident, + id: ast::NodeId) -> bool { + tcx.def_map.borrow().get(&id) + .map_or(false, |def| { + let did = def.def_id(); + ast_util::is_local(did) && did.node == fn_id + }) + } + + // check if the method call `id` refers to method `method_id` + // (with name `method_name` contained in impl `impl_id`). + fn id_refers_to_this_method<'tcx>(tcx: &ty::ctxt<'tcx>, + impl_id: ast::NodeId, + method_id: ast::NodeId, + method_name: ast::Ident, + id: ast::NodeId) -> bool { + let did = match tcx.method_map.borrow().get(&ty::MethodCall::expr(id)) { + None => return false, + Some(m) => match m.origin { + // There's no way to know if a method call via a + // vtable is recursion, so we assume it's not. + ty::MethodTraitObject(_) => return false, + + // This `did` refers directly to the method definition. + ty::MethodStatic(did) | ty::MethodStaticUnboxedClosure(did) => did, + + // MethodTypeParam are methods from traits: + + // The `impl ... for ...` of this method call + // isn't known, e.g. it might be a default method + // in a trait, so we get the def-id of the trait + // method instead. + ty::MethodTypeParam( + ty::MethodParam { ref trait_ref, method_num, impl_def_id: None, }) => { + ty::trait_item(tcx, trait_ref.def_id, method_num).def_id() + } + + // The `impl` is known, so we check that with a + // special case: + ty::MethodTypeParam( + ty::MethodParam { impl_def_id: Some(impl_def_id), .. }) => { + + let name = match tcx.map.expect_expr(id).node { + ast::ExprMethodCall(ref sp_ident, _, _) => sp_ident.node, + _ => tcx.sess.span_bug( + tcx.map.span(id), + "non-method call expr behaving like a method call?") + }; + // it matches if it comes from the same impl, + // and has the same method name. + return ast_util::is_local(impl_def_id) + && impl_def_id.node == impl_id + && method_name.name == name.name + } + } + }; + + ast_util::is_local(did) && did.node == method_id + } + } +} + declare_lint! { pub UNUSED_IMPORTS, Warn, diff --git a/src/librustc/lint/context.rs b/src/librustc/lint/context.rs index 4cbfcf7e91ad1..3728e6f4980d9 100644 --- a/src/librustc/lint/context.rs +++ b/src/librustc/lint/context.rs @@ -211,6 +211,7 @@ impl LintStore { UnusedAllocation, MissingCopyImplementations, UnstableFeatures, + UnconditionalRecursion, ); add_builtin_with_new!(sess, diff --git a/src/test/compile-fail/lint-unconditional-recursion.rs b/src/test/compile-fail/lint-unconditional-recursion.rs new file mode 100644 index 0000000000000..0c3d1c6adea40 --- /dev/null +++ b/src/test/compile-fail/lint-unconditional-recursion.rs @@ -0,0 +1,66 @@ +// Copyright 2014 The Rust Project Developers. See the COPYRIGHT +// file at the top-level directory of this distribution and at +// http://rust-lang.org/COPYRIGHT. +// +// Licensed under the Apache License, Version 2.0 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![deny(unconditional_recursion)] +#![allow(dead_code)] +fn foo() { //~ ERROR function cannot return without recurring + foo(); //~ NOTE recursive call site +} + +fn bar() { + if true { + bar() + } +} + +fn baz() { //~ ERROR function cannot return without recurring + if true { + baz() //~ NOTE recursive call site + } else { + baz() //~ NOTE recursive call site + } +} + +fn qux() { + loop {} +} + +fn quz() -> bool { //~ ERROR function cannot return without recurring + if true { + while quz() {} //~ NOTE recursive call site + true + } else { + loop { quz(); } //~ NOTE recursive call site + } +} + +trait Foo { + fn bar(&self) { //~ ERROR function cannot return without recurring + self.bar() //~ NOTE recursive call site + } +} + +impl Foo for Box { + fn bar(&self) { //~ ERROR function cannot return without recurring + loop { + self.bar() //~ NOTE recursive call site + } + } + +} + +struct Baz; +impl Baz { + fn qux(&self) { //~ ERROR function cannot return without recurring + self.qux(); //~ NOTE recursive call site + } +} + +fn main() {}