mathieu/os-rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Rewrite the UnconditionalRecursion lint to use MIR

Browse source 
Part of #51002
This commit is contained in:
Wesley Wiser 2018-09-11 21:35:08 -04:00
parent 4bf883b5e7
commit f81d1dd294
14 changed files with 240 additions and 287 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
src/librustc/lint/builtin.rs
View file

@ -233,6 +233,12 @@ declare_lint! {
"detect mut variables which don't need to be mutable"
}
declare_lint! {
pub UNCONDITIONAL_RECURSION,
Warn,
"functions that cannot return without calling themselves"
}
declare_lint! {
pub SINGLE_USE_LIFETIMES,
Allow,
@ -396,6 +402,7 @@ impl LintPass for HardwiredLints {
DEPRECATED,
UNUSED_UNSAFE,
UNUSED_MUT,
UNCONDITIONAL_RECURSION,
SINGLE_USE_LIFETIMES,
UNUSED_LIFETIMES,
UNUSED_LABELS,

277
src/librustc_lint/builtin.rs
View file

@ -30,10 +30,7 @@
use rustc::hir::def::Def;
use rustc::hir::def_id::DefId;
use rustc::cfg;
use rustc::ty::subst::Substs;
use rustc::ty::{self, Ty};
use rustc::traits;
use hir::Node;
use util::nodemap::NodeSet;
use lint::{LateContext, LintContext, LintArray};
@ -844,279 +841,6 @@ impl EarlyLintPass for UnusedDocComment {
}
}
declare_lint! {
pub UNCONDITIONAL_RECURSION,
Warn,
"functions that cannot return without calling themselves"
}
#[derive(Copy, Clone)]
pub struct UnconditionalRecursion;
impl LintPass for UnconditionalRecursion {
fn get_lints(&self) -> LintArray {
lint_array![UNCONDITIONAL_RECURSION]
}
}
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnconditionalRecursion {
fn check_fn(&mut self,
cx: &LateContext,
fn_kind: FnKind,
_: &hir::FnDecl,
body: &hir::Body,
sp: Span,
id: ast::NodeId) {
let method = match fn_kind {
FnKind::ItemFn(..) => None,
FnKind::Method(..) => {
Some(cx.tcx.associated_item(cx.tcx.hir.local_def_id(id)))
}
// closures can't recur, so they don't matter.
FnKind::Closure(_) => return,
};
// 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 itself 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 recursing after an assert.
let cfg = cfg::CFG::new(cx.tcx, &body);
let mut work_queue = vec![cfg.entry];
let mut reached_exit_without_self_call = false;
let mut self_call_spans = vec![];
let mut visited = FxHashSet::default();
while let Some(idx) = work_queue.pop() {
if idx == cfg.exit {
// found a path!
reached_exit_without_self_call = true;
break;
}
let cfg_id = idx.node_id();
if visited.contains(&cfg_id) {
// already done
continue;
}
visited.insert(cfg_id);
// is this a recursive call?
let local_id = cfg.graph.node_data(idx).id();
if local_id != hir::DUMMY_ITEM_LOCAL_ID {
let node_id = cx.tcx.hir.hir_to_node_id(hir::HirId {
owner: body.value.hir_id.owner,
local_id
});
let self_recursive = match method {
Some(ref method) => expr_refers_to_this_method(cx, method, node_id),
None => expr_refers_to_this_fn(cx, id, node_id),
};
if self_recursive {
self_call_spans.push(cx.tcx.hir.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.
for (_, edge) in cfg.graph.outgoing_edges(idx) {
let target_idx = edge.target();
let target_cfg_id = target_idx.node_id();
if !visited.contains(&target_cfg_id) {
work_queue.push(target_idx)
}
}
}
// Check the number of self 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.is_empty() {
let sp = cx.tcx.sess.source_map().def_span(sp);
let mut db = cx.struct_span_lint(UNCONDITIONAL_RECURSION,
sp,
"function cannot return without recursing");
db.span_label(sp, "cannot return without recursing");
// offer some help to the programmer.
for call in &self_call_spans {
db.span_label(*call, "recursive call site");
}
db.help("a `loop` may express intention better if this is on purpose");
db.emit();
}
// all done
return;
// Functions for identifying if the given Expr NodeId `id`
// represents a call to the function `fn_id`/method `method`.
fn expr_refers_to_this_fn(cx: &LateContext, fn_id: ast::NodeId, id: ast::NodeId) -> bool {
match cx.tcx.hir.get(id) {
Node::Expr(&hir::Expr { node: hir::ExprKind::Call(ref callee, _), .. }) => {
let def = if let hir::ExprKind::Path(ref qpath) = callee.node {
cx.tables.qpath_def(qpath, callee.hir_id)
} else {
return false;
};
match def {
Def::Local(..) | Def::Upvar(..) => false,
_ => def.def_id() == cx.tcx.hir.local_def_id(fn_id)
}
}
_ => false,
}
}
// Check if the expression `id` performs a call to `method`.
fn expr_refers_to_this_method(cx: &LateContext,
method: &ty::AssociatedItem,
id: ast::NodeId)
-> bool {
use rustc::ty::adjustment::*;
// Ignore non-expressions.
let expr = if let Node::Expr(e) = cx.tcx.hir.get(id) {
e
} else {
return false;
};
// Check for overloaded autoderef method calls.
let mut source = cx.tables.expr_ty(expr);
for adjustment in cx.tables.expr_adjustments(expr) {
if let Adjust::Deref(Some(deref)) = adjustment.kind {
let (def_id, substs) = deref.method_call(cx.tcx, source);
if method_call_refers_to_method(cx, method, def_id, substs, id) {
return true;
}
}
source = adjustment.target;
}
// Check for method calls and overloaded operators.
if cx.tables.is_method_call(expr) {
let hir_id = cx.tcx.hir.definitions().node_to_hir_id(id);
if let Some(def) = cx.tables.type_dependent_defs().get(hir_id) {
let def_id = def.def_id();
let substs = cx.tables.node_substs(hir_id);
if method_call_refers_to_method(cx, method, def_id, substs, id) {
return true;
}
} else {
cx.tcx.sess.delay_span_bug(expr.span,
"no type-dependent def for method call");
}
}
// Check for calls to methods via explicit paths (e.g. `T::method()`).
match expr.node {
hir::ExprKind::Call(ref callee, _) => {
let def = if let hir::ExprKind::Path(ref qpath) = callee.node {
cx.tables.qpath_def(qpath, callee.hir_id)
} else {
return false;
};
match def {
Def::Method(def_id) => {
let substs = cx.tables.node_substs(callee.hir_id);
method_call_refers_to_method(cx, method, def_id, substs, id)
}
_ => false,
}
}
_ => false,
}
}
// Check if the method call to the method with the ID `callee_id`
// and instantiated with `callee_substs` refers to method `method`.
fn method_call_refers_to_method<'a, 'tcx>(cx: &LateContext<'a, 'tcx>,
method: &ty::AssociatedItem,
callee_id: DefId,
callee_substs: &Substs<'tcx>,
expr_id: ast::NodeId)
-> bool {
let tcx = cx.tcx;
let callee_item = tcx.associated_item(callee_id);
match callee_item.container {
// This is an inherent method, so the `def_id` refers
// directly to the method definition.
ty::ImplContainer(_) => callee_id == method.def_id,
// A trait method, from any number of possible sources.
// Attempt to select a concrete impl before checking.
ty::TraitContainer(trait_def_id) => {
let trait_ref = ty::TraitRef::from_method(tcx, trait_def_id, callee_substs);
let trait_ref = ty::Binder::bind(trait_ref);
let span = tcx.hir.span(expr_id);
let obligation =
traits::Obligation::new(traits::ObligationCause::misc(span, expr_id),
cx.param_env,
trait_ref.to_poly_trait_predicate());
tcx.infer_ctxt().enter(|infcx| {
let mut selcx = traits::SelectionContext::new(&infcx);
match selcx.select(&obligation) {
// The method comes from a `T: Trait` bound.
// If `T` is `Self`, then this call is inside
// a default method definition.
Ok(Some(traits::VtableParam(_))) => {
let on_self = trait_ref.self_ty().is_self();
// We can only be recursing in a default
// method if we're being called literally
// on the `Self` type.
on_self && callee_id == method.def_id
}
// The `impl` is known, so we check that with a
// special case:
Ok(Some(traits::VtableImpl(vtable_impl))) => {
let container = ty::ImplContainer(vtable_impl.impl_def_id);
// It matches if it comes from the same impl,
// and has the same method name.
container == method.container &&
callee_item.ident.name == method.ident.name
}
// There's no way to know if this call is
// recursive, so we assume it's not.
_ => false,
}
})
}
}
}
}
}
declare_lint! {
PLUGIN_AS_LIBRARY,
Warn,
@ -1783,7 +1507,6 @@ impl LintPass for SoftLints {
MISSING_DEBUG_IMPLEMENTATIONS,
ANONYMOUS_PARAMETERS,
UNUSED_DOC_COMMENTS,
UNCONDITIONAL_RECURSION,
PLUGIN_AS_LIBRARY,
PRIVATE_NO_MANGLE_FNS,
PRIVATE_NO_MANGLE_STATICS,

1
src/librustc_lint/lib.rs
View file

@ -144,7 +144,6 @@ pub fn register_builtins(store: &mut lint::LintStore, sess: Option<&Session>) {
UnusedAllocation: UnusedAllocation,
MissingCopyImplementations: MissingCopyImplementations,
UnstableFeatures: UnstableFeatures,
UnconditionalRecursion: UnconditionalRecursion,
InvalidNoMangleItems: InvalidNoMangleItems,
PluginAsLibrary: PluginAsLibrary,
MutableTransmutes: MutableTransmutes,

4
src/librustc_mir/build/mod.rs
View file

@ -35,6 +35,8 @@ use syntax_pos::Span;
use transform::MirSource;
use util as mir_util;
use super::lints;
/// Construct the MIR for a given def-id.
pub fn mir_build<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> Mir<'tcx> {
let id = tcx.hir.as_local_node_id(def_id).unwrap();
@ -176,6 +178,8 @@ pub fn mir_build<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> Mir<'t
mir_util::dump_mir(tcx, None, "mir_map", &0,
MirSource::item(def_id), &mir, |_, _| Ok(()) );
lints::check(tcx, &mir, def_id);
mir
})
}

1
src/librustc_mir/lib.rs
View file

@ -79,6 +79,7 @@ mod borrow_check;
mod build;
mod dataflow;
mod hair;
mod lints;
mod shim;
pub mod transform;
pub mod util;

156
src/librustc_mir/lints.rs Normal file
View file

@ -0,0 +1,156 @@
// Copyright 2018 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.
use rustc_data_structures::bit_set::BitSet;
use rustc::hir::def_id::DefId;
use rustc::hir::intravisit::FnKind;
use rustc::hir::map::blocks::FnLikeNode;
use rustc::lint::builtin::UNCONDITIONAL_RECURSION;
use rustc::mir::{self, Mir, TerminatorKind};
use rustc::ty::{AssociatedItem, AssociatedItemContainer, Instance, TyCtxt, TyKind};
use rustc::ty::subst::Substs;
pub fn check(tcx: TyCtxt<'a, 'tcx, 'tcx>,
mir: &Mir<'tcx>,
def_id: DefId) {
let node_id = tcx.hir.as_local_node_id(def_id).unwrap();
if let Some(fn_like_node) = FnLikeNode::from_node(tcx.hir.get(node_id)) {
check_fn_for_unconditional_recursion(tcx, fn_like_node.kind(), mir, def_id);
}
}
fn check_fn_for_unconditional_recursion(tcx: TyCtxt<'a, 'tcx, 'tcx>,
fn_kind: FnKind,
mir: &Mir<'tcx>,
def_id: DefId) {
if let FnKind::Closure(_) = fn_kind {
// closures can't recur, so they don't matter.
return;
}
//FIXME(#54444) rewrite this lint to use the dataflow framework
// 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 itself 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 recursing after an assert.
let basic_blocks = mir.basic_blocks();
let mut reachable_without_self_call_queue = vec![mir::START_BLOCK];
let mut reached_exit_without_self_call = false;
let mut self_call_locations = vec![];
let mut visited = BitSet::new_empty(basic_blocks.len());
let param_env = tcx.param_env(def_id);
let trait_substs_count =
match tcx.opt_associated_item(def_id) {
Some(AssociatedItem {
container: AssociatedItemContainer::TraitContainer(trait_def_id),
..
}) => tcx.generics_of(trait_def_id).count(),
_ => 0
};
let caller_substs = &Substs::identity_for_item(tcx, def_id)[..trait_substs_count];
while let Some(bb) = reachable_without_self_call_queue.pop() {
if visited.contains(bb) {
//already done
continue;
}
visited.insert(bb);
let block = &basic_blocks[bb];
if let Some(ref terminator) = block.terminator {
match terminator.kind {
TerminatorKind::Call { ref func, .. } => {
let func_ty = func.ty(mir, tcx);
if let TyKind::FnDef(fn_def_id, substs) = func_ty.sty {
let (call_fn_id, call_substs) =
if let Some(instance) = Instance::resolve(tcx,
param_env,
fn_def_id,
substs) {
(instance.def_id(), instance.substs)
} else {
(fn_def_id, substs)
};
let is_self_call =
call_fn_id == def_id &&
&call_substs[..caller_substs.len()] == caller_substs;
if is_self_call {
self_call_locations.push(terminator.source_info);
//this is a self call so we shouldn't explore
//further down this path
continue;
}
}
},
TerminatorKind::Abort | TerminatorKind::Return => {
//found a path!
reached_exit_without_self_call = true;
break;
}
_ => {}
}
for successor in terminator.successors() {
reachable_without_self_call_queue.push(*successor);
}
}
}
// Check the number of self 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_locations.is_empty() {
let node_id = tcx.hir.as_local_node_id(def_id).unwrap();
let sp = tcx.sess.source_map().def_span(tcx.hir.span(node_id));
let mut db = tcx.struct_span_lint_node(UNCONDITIONAL_RECURSION,
node_id,
sp,
"function cannot return without recursing");
db.span_label(sp, "cannot return without recursing");
// offer some help to the programmer.
for location in &self_call_locations {
db.span_label(location.span, "recursive call site");
}
db.help("a `loop` may express intention better if this is on purpose");
db.emit();
}
}

14
src/test/ui/did_you_mean/issue-31424.nll.stderr
View file

@ -7,8 +7,20 @@ LL | (&mut self).bar(); //~ ERROR cannot borrow
| cannot borrow as mutable
| try removing `&mut` here
warning: function cannot return without recursing
--> $DIR/issue-31424.rs:22:5
|
LL | fn bar(self: &mut Self) {
| ^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
LL | //~^ WARN function cannot return without recursing
LL | (&mut self).bar(); //~ ERROR cannot borrow
| ----------------- recursive call site
|
= note: #[warn(unconditional_recursion)] on by default
= help: a `loop` may express intention better if this is on purpose
error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable
--> $DIR/issue-31424.rs:23:9
--> $DIR/issue-31424.rs:24:9
|
LL | (&mut self).bar(); //~ ERROR cannot borrow
| ^^^^^^^^^^^

1
src/test/ui/did_you_mean/issue-31424.rs
View file

@ -20,6 +20,7 @@ impl Struct {
// In this case we could keep the suggestion, but to distinguish the
// two cases is pretty hard. It's an obscure case anyway.
fn bar(self: &mut Self) {
//~^ WARN function cannot return without recursing
(&mut self).bar(); //~ ERROR cannot borrow
}
}

14
src/test/ui/did_you_mean/issue-31424.stderr
View file

@ -7,8 +7,20 @@ LL | (&mut self).bar(); //~ ERROR cannot borrow
| cannot reborrow mutably
| try removing `&mut` here
warning: function cannot return without recursing
--> $DIR/issue-31424.rs:22:5
|
LL | fn bar(self: &mut Self) {
| ^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
LL | //~^ WARN function cannot return without recursing
LL | (&mut self).bar(); //~ ERROR cannot borrow
| ----------------- recursive call site
|
= note: #[warn(unconditional_recursion)] on by default
= help: a `loop` may express intention better if this is on purpose
error[E0596]: cannot borrow immutable argument `self` as mutable
--> $DIR/issue-31424.rs:23:15
--> $DIR/issue-31424.rs:24:15
|
LL | (&mut self).bar(); //~ ERROR cannot borrow
| ^^^^ cannot borrow mutably

1
src/test/ui/nll/issue-51191.rs
View file

@ -14,6 +14,7 @@ struct Struct;
impl Struct {
fn bar(self: &mut Self) {
//~^ WARN function cannot return without recursing
(&mut self).bar();
//~^ ERROR cannot borrow `self` as mutable, as it is not declared as mutable [E0596]
}

22
src/test/ui/nll/issue-51191.stderr
View file

@ -1,5 +1,17 @@
warning: function cannot return without recursing
--> $DIR/issue-51191.rs:16:5
|
LL | fn bar(self: &mut Self) {
| ^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
LL | //~^ WARN function cannot return without recursing
LL | (&mut self).bar();
| ----------------- recursive call site
|
= note: #[warn(unconditional_recursion)] on by default
= help: a `loop` may express intention better if this is on purpose
error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable
--> $DIR/issue-51191.rs:17:9
--> $DIR/issue-51191.rs:18:9
|
LL | (&mut self).bar();
| ^^^^^^^^^^^
@ -8,7 +20,7 @@ LL | (&mut self).bar();
| try removing `&mut` here
error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable
--> $DIR/issue-51191.rs:22:9
--> $DIR/issue-51191.rs:23:9
|
LL | fn imm(self) {
| ---- help: consider changing this to be mutable: `mut self`
@ -16,19 +28,19 @@ LL | (&mut self).bar();
| ^^^^^^^^^^^ cannot borrow as mutable
error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable
--> $DIR/issue-51191.rs:31:9
--> $DIR/issue-51191.rs:32:9
|
LL | (&mut self).bar();
| ^^^^^^^^^^^ cannot borrow as mutable
error[E0596]: cannot borrow data in a `&` reference as mutable
--> $DIR/issue-51191.rs:31:9
--> $DIR/issue-51191.rs:32:9
|
LL | (&mut self).bar();
| ^^^^^^^^^^^ cannot borrow as mutable
error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable
--> $DIR/issue-51191.rs:37:9
--> $DIR/issue-51191.rs:38:9
|
LL | (&mut self).bar();
| ^^^^^^^^^^^

14
src/test/ui/regions/region-bound-on-closure-outlives-call.nll.stderr
View file

@ -1,5 +1,17 @@
warning: function cannot return without recursing
--> $DIR/region-bound-on-closure-outlives-call.rs:11:1
|
LL | fn call_rec<F>(mut f: F) -> usize where F: FnMut(usize) -> usize {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
LL | //~^ WARN function cannot return without recursing
LL | (|x| f(x))(call_rec(f)) //~ ERROR cannot move out of `f`
| ----------- recursive call site
|
= note: #[warn(unconditional_recursion)] on by default
= help: a `loop` may express intention better if this is on purpose
error[E0505]: cannot move out of `f` because it is borrowed
--> $DIR/region-bound-on-closure-outlives-call.rs:12:25
--> $DIR/region-bound-on-closure-outlives-call.rs:13:25
|
LL | (|x| f(x))(call_rec(f)) //~ ERROR cannot move out of `f`
| --------------------^--

1
src/test/ui/regions/region-bound-on-closure-outlives-call.rs
View file

@ -9,6 +9,7 @@
// except according to those terms.
fn call_rec<F>(mut f: F) -> usize where F: FnMut(usize) -> usize {
//~^ WARN function cannot return without recursing
(|x| f(x))(call_rec(f)) //~ ERROR cannot move out of `f`
}

14
src/test/ui/regions/region-bound-on-closure-outlives-call.stderr
View file

@ -1,5 +1,17 @@
warning: function cannot return without recursing
--> $DIR/region-bound-on-closure-outlives-call.rs:11:1
|
LL | fn call_rec<F>(mut f: F) -> usize where F: FnMut(usize) -> usize {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot return without recursing
LL | //~^ WARN function cannot return without recursing
LL | (|x| f(x))(call_rec(f)) //~ ERROR cannot move out of `f`
| ----------- recursive call site
|
= note: #[warn(unconditional_recursion)] on by default
= help: a `loop` may express intention better if this is on purpose
error[E0505]: cannot move out of `f` because it is borrowed
--> $DIR/region-bound-on-closure-outlives-call.rs:12:25
--> $DIR/region-bound-on-closure-outlives-call.rs:13:25
|
LL | (|x| f(x))(call_rec(f)) //~ ERROR cannot move out of `f`
| --- ^ move out of `f` occurs here