Skip to content

Commit

Permalink
Rollup merge of #70822 - jonas-schievink:curse-of-the-recursion, r=ec…
Browse files Browse the repository at this point in the history 
…static-morse

Don't lint for self-recursion when the function can diverge

Fixes #54444
  • Loading branch information
@Centril
Centril committed Apr 9, 2020
2 parents ba50bc5 + b8f416d commit a209b4f
Show file tree
Hide file tree
Showing 3 changed files with 152 additions and 97 deletions.
4 changes: 2 additions & 2 deletions src/librustc_mir_build/build/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,11 +178,11 @@ fn mir_build(tcx: TyCtxt<'_>, def_id: DefId) -> BodyAndCache<'_> {
build::construct_const(cx, body_id, return_ty, return_ty_span)
};

lints::check(tcx, &body, def_id);

let mut body = BodyAndCache::new(body);
body.ensure_predecessors();

lints::check(tcx, &body.unwrap_read_only(), def_id);

// The borrow checker will replace all the regions here with its own
// inference variables. There's no point having non-erased regions here.
// The exception is `body.user_type_annotations`, which is used unmodified
Expand Down
227 changes: 132 additions & 95 deletions src/librustc_mir_build/lints.rs
View file
Original file line number Diff line number Diff line change
@@ -1,138 +1,175 @@
use rustc_hir::def_id::DefId;
use rustc_hir::intravisit::FnKind;
use rustc_index::bit_set::BitSet;
use rustc_index::vec::IndexVec;
use rustc_middle::hir::map::blocks::FnLikeNode;
use rustc_middle::mir::{self, Body, TerminatorKind};
use rustc_middle::mir::{BasicBlock, Body, ReadOnlyBodyAndCache, TerminatorKind, START_BLOCK};
use rustc_middle::ty::subst::InternalSubsts;
use rustc_middle::ty::{self, AssocItem, AssocItemContainer, Instance, TyCtxt};
use rustc_session::lint::builtin::UNCONDITIONAL_RECURSION;
use std::collections::VecDeque;

crate fn check<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>, def_id: DefId) {
crate fn check<'tcx>(tcx: TyCtxt<'tcx>, body: &ReadOnlyBodyAndCache<'_, 'tcx>, def_id: DefId) {
let hir_id = tcx.hir().as_local_hir_id(def_id).unwrap();

if let Some(fn_like_node) = FnLikeNode::from_node(tcx.hir().get(hir_id)) {
check_fn_for_unconditional_recursion(tcx, fn_like_node.kind(), body, def_id);
if let FnKind::Closure(_) = fn_like_node.kind() {
// closures can't recur, so they don't matter.
return;
}

check_fn_for_unconditional_recursion(tcx, body, def_id);
}
}

fn check_fn_for_unconditional_recursion<'tcx>(
tcx: TyCtxt<'tcx>,
fn_kind: FnKind<'_>,
body: &Body<'tcx>,
body: &ReadOnlyBodyAndCache<'_, 'tcx>,
def_id: DefId,
) {
if let FnKind::Closure(_) = fn_kind {
// closures can't recur, so they don't matter.
return;
}
let self_calls = find_blocks_calling_self(tcx, &body, def_id);

//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 = body.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());
// Stores a list of `Span`s for every basic block. Those are the spans of self-calls where we
// know that one of them will definitely be reached. If the list is empty, the block either
// wasn't processed yet or will not always go to a self-call.
let mut results = IndexVec::from_elem_n(vec![], body.basic_blocks().len());

let param_env = tcx.param_env(def_id);
let trait_substs_count = match tcx.opt_associated_item(def_id) {
Some(AssocItem { container: AssocItemContainer::TraitContainer(trait_def_id), .. }) => {
tcx.generics_of(trait_def_id).count()
}
_ => 0,
};
let caller_substs = &InternalSubsts::identity_for_item(tcx, def_id)[..trait_substs_count];
// We start the analysis at the self calls and work backwards.
let mut queue: VecDeque<_> = self_calls.iter().collect();

while let Some(bb) = reachable_without_self_call_queue.pop() {
if !visited.insert(bb) {
//already done
while let Some(bb) = queue.pop_front() {
if !results[bb].is_empty() {
// Already propagated.
continue;
}

let block = &basic_blocks[bb];

if let Some(ref terminator) = block.terminator {
match terminator.kind {
TerminatorKind::Call { ref func, .. } => {
let func_ty = func.ty(body, tcx);

if let ty::FnDef(fn_def_id, substs) = func_ty.kind {
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;
}
}
let locations = if self_calls.contains(bb) {
// `bb` *is* a self-call.
// We don't look at successors here because they are irrelevant here and we don't want
// to lint them (eg. `f(); f()` should only lint the first call).
vec![bb]
} else {
// If *all* successors of `bb` lead to a self-call, emit notes at all of their
// locations.

// Determine all "relevant" successors. We ignore successors only reached via unwinding.
let terminator = body[bb].terminator();
let relevant_successors = match &terminator.kind {
TerminatorKind::Call { destination: None, .. }
| TerminatorKind::Yield { .. }
| TerminatorKind::GeneratorDrop => None.into_iter().chain(&[]),
TerminatorKind::SwitchInt { targets, .. } => None.into_iter().chain(targets),
TerminatorKind::Goto { target }
| TerminatorKind::Drop { target, .. }
| TerminatorKind::DropAndReplace { target, .. }
| TerminatorKind::Assert { target, .. }
| TerminatorKind::FalseEdges { real_target: target, .. }
| TerminatorKind::FalseUnwind { real_target: target, .. }
| TerminatorKind::Call { destination: Some((_, target)), .. } => {
Some(target).into_iter().chain(&[])
}
TerminatorKind::Abort | TerminatorKind::Return => {
//found a path!
reached_exit_without_self_call = true;
break;
TerminatorKind::Resume
| TerminatorKind::Abort
| TerminatorKind::Return
| TerminatorKind::Unreachable => {
// We propagate backwards, so these should never be encountered here.
unreachable!("unexpected terminator {:?}", terminator.kind)
}
_ => {}
};

// If all our successors are known to lead to self-calls, then we do too.
let all_are_self_calls =
relevant_successors.clone().all(|&succ| !results[succ].is_empty());

if all_are_self_calls {
// We'll definitely lead to a self-call. Merge all call locations of the successors
// for linting them later.
relevant_successors.flat_map(|&succ| results[succ].iter().copied()).collect()
} else {
// At least 1 successor does not always lead to a self-call, so we also don't.
vec![]
}
};

for successor in terminator.successors() {
reachable_without_self_call_queue.push(*successor);
}
if !locations.is_empty() {
// This is a newly confirmed-to-always-reach-self-call block.
results[bb] = locations;

// Propagate backwards through the CFG.
debug!("propagate loc={:?} in {:?} -> {:?}", results[bb], bb, body.predecessors()[bb]);
queue.extend(body.predecessors()[bb].iter().copied());
}
}

// 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() {
debug!("unconditional recursion results: {:?}", results);

let self_call_locations = &mut results[START_BLOCK];
self_call_locations.sort();
self_call_locations.dedup();

if !self_call_locations.is_empty() {
let hir_id = tcx.hir().as_local_hir_id(def_id).unwrap();
let sp = tcx.sess.source_map().guess_head_span(tcx.hir().span(hir_id));
tcx.struct_span_lint_hir(UNCONDITIONAL_RECURSION, hir_id, sp, |lint| {
let mut db = lint.build("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");
for bb in self_call_locations {
let span = body.basic_blocks()[*bb].terminator().source_info.span;
db.span_label(span, "recursive call site");
}
db.help("a `loop` may express intention better if this is on purpose");
db.emit();
});
}
}

/// Finds blocks with `Call` terminators that would end up calling back into the same method.
fn find_blocks_calling_self<'tcx>(
tcx: TyCtxt<'tcx>,
body: &Body<'tcx>,
def_id: DefId,
) -> BitSet<BasicBlock> {
let param_env = tcx.param_env(def_id);

// If this is trait/impl method, extract the trait's substs.
let trait_substs_count = match tcx.opt_associated_item(def_id) {
Some(AssocItem { container: AssocItemContainer::TraitContainer(trait_def_id), .. }) => {
tcx.generics_of(trait_def_id).count()
}
_ => 0,
};
let trait_substs = &InternalSubsts::identity_for_item(tcx, def_id)[..trait_substs_count];

let mut self_calls = BitSet::new_empty(body.basic_blocks().len());

for (bb, data) in body.basic_blocks().iter_enumerated() {
if let TerminatorKind::Call { func, .. } = &data.terminator().kind {
let func_ty = func.ty(body, tcx);

if let ty::FnDef(fn_def_id, substs) = func_ty.kind {
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)
};

// FIXME(#57965): Make this work across function boundaries

// If this is a trait fn, the substs on the trait have to match, or we might be
// calling into an entirely different method (for example, a call from the default
// method in the trait to `<A as Trait<B>>::method`, where `A` and/or `B` are
// specific types).
let is_self_call =
call_fn_id == def_id && &call_substs[..trait_substs.len()] == trait_substs;

if is_self_call {
self_calls.insert(bb);
}
}
}
}

self_calls
}
18 changes: 18 additions & 0 deletions src/test/ui/lint/lint-unconditional-recursion.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,4 +131,22 @@ trait Bar {
}
}

// Do not trigger on functions that may diverge instead of self-recursing (#54444)

pub fn loops(x: bool) {
if x {
loops(x);
} else {
loop {}
}
}

pub fn panics(x: bool) {
if x {
panics(!x);
} else {
panic!("panics");
}
}

fn main() {}

0 comments on commit a209b4f

Please sign in to comment.