]> git.lizzy.rs Git - rust.git/commitdiff
Auto merge of #20373 - huonw:self-call-lint, r=luqmana
authorbors <bors@rust-lang.org>
Sun, 25 Jan 2015 08:24:47 +0000 (08:24 +0000)
committerbors <bors@rust-lang.org>
Sun, 25 Jan 2015 08:24:47 +0000 (08:24 +0000)
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.

src/librustc/lint/builtin.rs
src/librustc/lint/context.rs
src/librustc/middle/astencode.rs
src/librustc/middle/ty.rs
src/librustc/middle/ty_fold.rs
src/librustc/util/ppaux.rs
src/librustc_trans/trans/meth.rs
src/librustc_typeck/check/method/confirm.rs
src/librustc_typeck/check/method/mod.rs
src/test/compile-fail/lint-unconditional-recursion.rs [new file with mode: 0644]

index 9a7b7e0eb942ad357f37609e6a599fe3b58119a6..72f16a708198f14b6bae95367317b7d19b6cc005 100644 (file)
 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 @@ fn check_item(&mut self, cx: &Context, item: &ast::Item) {
     }
 }
 
+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,
index 4cbfcf7e91ad1dd914507eaa2afcf4818f28ef54..3728e6f4980d94059716f9be70f466da79faa6e2 100644 (file)
@@ -211,6 +211,7 @@ macro_rules! add_lint_group {
                      UnusedAllocation,
                      MissingCopyImplementations,
                      UnstableFeatures,
+                     UnconditionalRecursion,
         );
 
         add_builtin_with_new!(sess,
index 537a2b3f545a2e4579e29beaa36897965b10a26f..430b63f81c852e82eabbf9d6953902c64466da5e 100644 (file)
@@ -627,6 +627,7 @@ fn tr(&self, dcx: &DecodeContext) -> 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 @@ fn emit_method_origin<'b>(&mut self,
                             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 @@ fn read_method_origin<'b, 'c>(&mut self, dcx: &DecodeContext<'b, 'c, 'tcx>)
                                         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()
index bae41b78c08a4de7da71767c9a80cd57e0bc9bfb..d04c2d2d1a09449dff84d436fcbbbeb1cfc2fbb4 100644 (file)
@@ -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
index b4e6cff954bcc745b94aa07ce6b94f1055539b8f..0b8c77860155a5854eaf8e82656530d64939e530 100644 (file)
@@ -310,7 +310,8 @@ fn fold_with<F: TypeFolder<'tcx>>(&self, folder: &mut F) -> ty::MethodOrigin<'tc
             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) => {
index c5aced4eb86f5902a9db6a5d974874705a5b47e1..266048fce51c9ac6a90f326f3018c8be808d54f0 100644 (file)
@@ -544,7 +544,7 @@ fn repr(&self, tcx: &ctxt<'tcx>) -> String {
 
 impl<'tcx, T:Repr<'tcx>> Repr<'tcx> for P<T> {
     fn repr(&self, tcx: &ctxt<'tcx>) -> String {
-        (*self).repr(tcx)
+        (**self).repr(tcx)
     }
 }
 
index c2f19670e4f1558234e15fa575fb7174cd0eabfd..9356be1b9b410808718bfc9e983f596b084a88a6 100644 (file)
@@ -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);
index 7946077485976e4cd3bbfa8128f4f7b97865f8b0..4aa0a211221ef526afd7313d59596ae840966496 100644 (file)
@@ -256,7 +256,8 @@ fn fresh_receiver_substs(&mut self,
                         &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 @@ fn fresh_receiver_substs(&mut self,
                 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 @@ fn fresh_receiver_substs(&mut self,
                 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)
             }
         }
index 345bc5fd2aa60772cb367f8fce554789034013ab..d92cc1dfc1e95f4af178b5bc176a0e49e76db449 100644 (file)
@@ -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()
     };
diff --git a/src/test/compile-fail/lint-unconditional-recursion.rs b/src/test/compile-fail/lint-unconditional-recursion.rs
new file mode 100644 (file)
index 0000000..0c3d1c6
--- /dev/null
@@ -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 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, 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<Foo+'static> {
+    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() {}