diff --git a/src/librustc_mir_build/build/mod.rs b/src/librustc_mir_build/build/mod.rs index 04cb509d44e4b..6e1a4ecf47a44 100644 --- a/src/librustc_mir_build/build/mod.rs +++ b/src/librustc_mir_build/build/mod.rs @@ -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 diff --git a/src/librustc_mir_build/lints.rs b/src/librustc_mir_build/lints.rs index b1628ec58c3c6..a7370c36f0b9f 100644 --- a/src/librustc_mir_build/lints.rs +++ b/src/librustc_mir_build/lints.rs @@ -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 { + 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 `>::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 +} diff --git a/src/test/ui/lint/lint-unconditional-recursion.rs b/src/test/ui/lint/lint-unconditional-recursion.rs index ab60a326cd220..d2a0329585b71 100644 --- a/src/test/ui/lint/lint-unconditional-recursion.rs +++ b/src/test/ui/lint/lint-unconditional-recursion.rs @@ -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() {}