From 000dc07f71baafc31da74e392ad5530fb6de9757 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Thu, 1 Jan 2015 03:03:14 +1100 Subject: [PATCH 1/3] Store a method-from-trait's impl in some cases when it is known. This allows one to look at an `ExprMethodCall` `foo.bar()` where `bar` is a method in some trait and (sometimes) extract the `impl` that `bar` is defined in, e.g. trait Foo { fn bar(&self); } impl Foo for uint { // fn bar(&self) {} } fn main() { 1u.bar(); // impl_def_id == Some() } This definitely doesn't handle all cases, but is correct when it is known, meaning it should only be used for certain linting/heuristic purposes; no safety analysis. --- src/librustc/middle/astencode.rs | 22 +++++++++++++++++++++ src/librustc/middle/ty.rs | 7 ++++++- src/librustc/middle/ty_fold.rs | 3 ++- src/librustc_trans/trans/meth.rs | 3 ++- src/librustc_typeck/check/method/confirm.rs | 9 ++++++--- src/librustc_typeck/check/method/mod.rs | 3 ++- 6 files changed, 40 insertions(+), 7 deletions(-) diff --git a/src/librustc/middle/astencode.rs b/src/librustc/middle/astencode.rs index 537a2b3f545..430b63f81c8 100644 --- a/src/librustc/middle/astencode.rs +++ b/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() diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs index 83bbdf14e4a..5b5b53d1246 100644 --- a/src/librustc/middle/ty.rs +++ b/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>, - // 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, } // details for a method invoked with a receiver whose type is an object diff --git a/src/librustc/middle/ty_fold.rs b/src/librustc/middle/ty_fold.rs index b4e6cff954b..0b8c7786015 100644 --- a/src/librustc/middle/ty_fold.rs +++ b/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) => { diff --git a/src/librustc_trans/trans/meth.rs b/src/librustc_trans/trans/meth.rs index c2f19670e4f..9356be1b9b4 100644 --- a/src/librustc_trans/trans/meth.rs +++ b/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); diff --git a/src/librustc_typeck/check/method/confirm.rs b/src/librustc_typeck/check/method/confirm.rs index 79460774859..4aa0a211221 100644 --- a/src/librustc_typeck/check/method/confirm.rs +++ b/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) } } diff --git a/src/librustc_typeck/check/method/mod.rs b/src/librustc_typeck/check/method/mod.rs index 345bc5fd2aa..d92cc1dfc1e 100644 --- a/src/librustc_typeck/check/method/mod.rs +++ b/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() }; From fbef241709093b3fb18cb12500fd33e4ab964b62 Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Thu, 1 Jan 2015 03:06:38 +1100 Subject: [PATCH 2/3] 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 fef1017b782..531bdc2941e 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 4cbfcf7e91a..3728e6f4980 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 00000000000..0c3d1c6adea --- /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() {} From 0684c8ebf92933e6382d1ac13a77999296411dac Mon Sep 17 00:00:00 2001 From: Huon Wilson Date: Sun, 25 Jan 2015 00:05:42 +1100 Subject: [PATCH 3/3] Fix infinite recursion in the compiler. This was detected by the unconditional_recursion lint. --- src/librustc/util/ppaux.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/librustc/util/ppaux.rs b/src/librustc/util/ppaux.rs index c5aced4eb86..266048fce51 100644 --- a/src/librustc/util/ppaux.rs +++ b/src/librustc/util/ppaux.rs @@ -544,7 +544,7 @@ impl<'tcx, T:Repr<'tcx>> Repr<'tcx> for Option { impl<'tcx, T:Repr<'tcx>> Repr<'tcx> for P { fn repr(&self, tcx: &ctxt<'tcx>) -> String { - (*self).repr(tcx) + (**self).repr(tcx) } }