Skip to content

Commit

Permalink
Auto merge of #54490 - wesleywiser:rewrite_it_in_mir, r=oli-obk
Browse files Browse the repository at this point in the history 
Rewrite the `UnconditionalRecursion` lint to use MIR

Part of #51002

r? @eddyb
  • Loading branch information
@bors
bors committed Oct 25, 2018
2 parents 8ec22e7 + f81d1dd commit 4bd4e41
Show file tree
Hide file tree
Showing 14 changed files with 240 additions and 287 deletions.
7 changes: 7 additions & 0 deletions src/librustc/lint/builtin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -402,6 +408,7 @@ impl LintPass for HardwiredLints {
DEPRECATED,
UNUSED_UNSAFE,
UNUSED_MUT,
UNCONDITIONAL_RECURSION,
SINGLE_USE_LIFETIMES,
UNUSED_LIFETIMES,
UNUSED_LABELS,
Expand Down
277 changes: 0 additions & 277 deletions src/librustc_lint/builtin.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -1724,7 +1448,6 @@ impl LintPass for SoftLints {
MISSING_DEBUG_IMPLEMENTATIONS,
ANONYMOUS_PARAMETERS,
UNUSED_DOC_COMMENTS,
UNCONDITIONAL_RECURSION,
PLUGIN_AS_LIBRARY,
NO_MANGLE_CONST_ITEMS,
NO_MANGLE_GENERIC_ITEMS,
Expand Down
1 change: 0 additions & 1 deletion src/librustc_lint/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
4 changes: 4 additions & 0 deletions src/librustc_mir/build/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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
})
}
Expand Down
1 change: 1 addition & 0 deletions src/librustc_mir/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ mod borrow_check;
mod build;
mod dataflow;
mod hair;
mod lints;
mod shim;
pub mod transform;
pub mod util;
Expand Down
Loading

0 comments on commit 4bd4e41

Please sign in to comment.