Auto merge of #20373 - huonw:self-call-lint, r=luqmana

E.g. `fn foo() { foo() }`, or, more subtlely

    impl Foo for Box<Foo+'static> {
        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.

---

This is highly non-perfect, in particular, my wording above is quite precise, and I have some unresolved questions: This currently will warn about

```rust
fn foo() {
    if bar { loop {} }

    foo()
}
```

even though `foo` may never be called (i.e. our apparent "unconditional" recursion is actually conditional). I don't know if we should handle this case, and ones like it with `panic!()` instead of `loop` (or anything else that "returns" `!`).

However, strictly speaking, it seems to me that changing the above to not warn will require changing

```rust
fn foo() {
    while bar {}
    foo()
}
```

to also not warn since it could be that the `while` is an infinite loop and doesn't ever hit the `foo`.

I'm inclined to think we let these cases warn since true edge cases like the first one seem rare, and if they do occur they seem like they would usually be typos in the function call. (I could imagine someone accidentally having code like `fn foo() { assert!(bar()); foo() /* meant to be boo() */ }` which won't warn if the `loop` case is "fixed".)

(Part of the reason this is unresolved is wanting feedback, part of the reason is I couldn't devise a strategy that worked in all cases.)

---

The name `unconditional_self_calls` is kinda clunky; and this reconstructs the CFG for each function that is linted which may or may not be very expensive, I don't know.

Author: bors

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()
     };