Add a lint unconditional_recursion to detect unconditional recursion.

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,

src/librustc/lint/context.rs

@@ -211,6 +211,7 @@ impl LintStore {
                      UnusedAllocation,
                      MissingCopyImplementations,
                      UnstableFeatures,
+                     UnconditionalRecursion,
         );
 
         add_builtin_with_new!(sess,

src/librustc/middle/astencode.rs

@@ -627,6 +627,7 @@ impl<'tcx> tr for MethodOrigin<'tcx> {
                         // def-id is already translated when we read it out
                         trait_ref: mp.trait_ref.clone(),
                         method_num: mp.method_num,
+                        impl_def_id: mp.impl_def_id.tr(dcx),
                     }
                 )
             }
@@ -879,6 +880,16 @@ impl<'a, 'tcx> rbml_writer_helpers<'tcx> for Encoder<'a> {
                             try!(this.emit_struct_field("method_num", 0, |this| {
                                 this.emit_uint(p.method_num)
                             }));
+                            try!(this.emit_struct_field("impl_def_id", 0, |this| {
+                                this.emit_option(|this| {
+                                    match p.impl_def_id {
+                                        None => this.emit_option_none(),
+                                        Some(did) => this.emit_option_some(|this| {
+                                            Ok(this.emit_def_id(did))
+                                        })
+                                    }
+                                })
+                            }));
                             Ok(())
                         })
                     })
@@ -1452,6 +1463,17 @@ impl<'a, 'tcx> rbml_decoder_decoder_helpers<'tcx> for reader::Decoder<'a> {
                                         this.read_struct_field("method_num", 1, |this| {
                                             this.read_uint()
                                         }).unwrap()
+                                    },
+                                    impl_def_id: {
+                                        this.read_struct_field("impl_def_id", 2, |this| {
+                                            this.read_option(|this, b| {
+                                                if b {
+                                                    Ok(Some(this.read_def_id(dcx)))
+                                                } else {
+                                                    Ok(None)
+                                                }
+                                            })
+                                        }).unwrap()
                                     }
                                 }))
                         }).unwrap()

src/librustc/middle/ty.rs

@@ -453,9 +453,14 @@ pub struct MethodParam<'tcx> {
     // never contains bound regions; those regions should have been
     // instantiated with fresh variables at this point.
     pub trait_ref: Rc<ty::TraitRef<'tcx>>,
-
     // index of uint in the list of methods for the trait
     pub method_num: uint,
+
+    /// The impl for the trait from which the method comes. This
+    /// should only be used for certain linting/heuristic purposes
+    /// since there is no guarantee that this is Some in every
+    /// situation that it could/should be.
+    pub impl_def_id: Option<ast::DefId>,
 }
 
 // details for a method invoked with a receiver whose type is an object

src/librustc/middle/ty_fold.rs

@@ -310,7 +310,8 @@ impl<'tcx> TypeFoldable<'tcx> for ty::MethodOrigin<'tcx> {
             ty::MethodTypeParam(ref param) => {
                 ty::MethodTypeParam(ty::MethodParam {
                     trait_ref: param.trait_ref.fold_with(folder),
-                    method_num: param.method_num
+                    method_num: param.method_num,
+                    impl_def_id: param.impl_def_id,
                 })
             }
             ty::MethodTraitObject(ref object) => {

src/librustc/util/ppaux.rs

@@ -544,7 +544,7 @@ impl<'tcx, T:Repr<'tcx>> Repr<'tcx> for Option<T> {
 
 impl<'tcx, T:Repr<'tcx>> Repr<'tcx> for P<T> {
     fn repr(&self, tcx: &ctxt<'tcx>) -> String {
-        (*self).repr(tcx)
+        (**self).repr(tcx)
     }
 }
 

src/librustc_trans/trans/meth.rs

@@ -132,7 +132,8 @@ pub fn trans_method_callee<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
 
         ty::MethodTypeParam(ty::MethodParam {
             ref trait_ref,
-            method_num
+            method_num,
+            impl_def_id: _
         }) => {
             let trait_ref = ty::Binder(bcx.monomorphize(trait_ref));
             let span = bcx.tcx().map.span(method_call.expr_id);

src/librustc_typeck/check/method/confirm.rs

@@ -256,7 +256,8 @@ impl<'a,'tcx> ConfirmContext<'a,'tcx> {
                         &impl_polytype.substs,
                         &ty::impl_trait_ref(self.tcx(), impl_def_id).unwrap());
                 let origin = MethodTypeParam(MethodParam { trait_ref: impl_trait_ref.clone(),
-                                                           method_num: method_num });
+                                                           method_num: method_num,
+                                                           impl_def_id: Some(impl_def_id) });
                 (impl_trait_ref.substs.clone(), origin)
             }
 
@@ -275,7 +276,8 @@ impl<'a,'tcx> ConfirmContext<'a,'tcx> {
                 let trait_ref =
                     Rc::new(ty::TraitRef::new(trait_def_id, self.tcx().mk_substs(substs.clone())));
                 let origin = MethodTypeParam(MethodParam { trait_ref: trait_ref,
-                                                           method_num: method_num });
+                                                           method_num: method_num,
+                                                           impl_def_id: None });
                 (substs, origin)
             }
 
@@ -285,7 +287,8 @@ impl<'a,'tcx> ConfirmContext<'a,'tcx> {
                 let trait_ref = self.replace_late_bound_regions_with_fresh_var(&*poly_trait_ref);
                 let substs = trait_ref.substs.clone();
                 let origin = MethodTypeParam(MethodParam { trait_ref: trait_ref,
-                                                           method_num: method_num });
+                                                           method_num: method_num,
+                                                           impl_def_id: None });
                 (substs, origin)
             }
         }

src/librustc_typeck/check/method/mod.rs

@@ -287,7 +287,8 @@ pub fn lookup_in_trait_adjusted<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
 
     let callee = MethodCallee {
         origin: MethodTypeParam(MethodParam{trait_ref: trait_ref.clone(),
-                                            method_num: method_num}),
+                                            method_num: method_num,
+                                            impl_def_id: None}),
         ty: fty,
         substs: trait_ref.substs.clone()
     };