From 87d859af24f49496e6aa2f65833e1cc2909ccf52 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sun, 5 Apr 2020 21:04:31 +0200 Subject: [PATCH 1/6] Don't lint for self-recursion when the function can diverge --- src/librustc_mir_build/build/mod.rs | 4 +- src/librustc_mir_build/lints.rs | 202 ++++++++++-------- .../ui/lint/lint-unconditional-recursion.rs | 18 ++ 3 files changed, 132 insertions(+), 92 deletions(-) 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..01dd575c51ed1 100644 --- a/src/librustc_mir_build/lints.rs +++ b/src/librustc_mir_build/lints.rs @@ -1,13 +1,15 @@ 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)) { @@ -18,7 +20,7 @@ crate fn check<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>, def_id: DefId) { 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 { @@ -26,113 +28,133 @@ fn check_fn_for_unconditional_recursion<'tcx>( 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 = 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()); + let self_calls = find_blocks_calling_self(tcx, &body, def_id); + let mut results = IndexVec::from_elem_n(vec![], body.basic_blocks().len()); + let mut queue: VecDeque<_> = self_calls.iter().collect(); - 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]; - - 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. + vec![bb] + } else { + // If *all* successors of `bb` lead to a self-call, emit notes at all of their + // locations. + + // Converging successors without unwind paths. + let terminator = body[bb].terminator(); + let relevant_successors = match &terminator.kind { + TerminatorKind::Call { destination: Some((_, dest)), .. } => { + Some(dest).into_iter().chain(&[]) } - TerminatorKind::Abort | TerminatorKind::Return => { - //found a path! - reached_exit_without_self_call = true; - break; + TerminatorKind::Call { destination: None, .. } => None.into_iter().chain(&[]), + TerminatorKind::SwitchInt { targets, .. } => None.into_iter().chain(targets), + TerminatorKind::Goto { target } + | TerminatorKind::Drop { target, .. } + | TerminatorKind::DropAndReplace { target, .. } + | TerminatorKind::Assert { target, .. } => Some(target).into_iter().chain(&[]), + TerminatorKind::Yield { .. } | TerminatorKind::GeneratorDrop => { + None.into_iter().chain(&[]) } - _ => {} - } + TerminatorKind::FalseEdges { real_target, .. } + | TerminatorKind::FalseUnwind { real_target, .. } => { + Some(real_target).into_iter().chain(&[]) + } + TerminatorKind::Resume + | TerminatorKind::Abort + | TerminatorKind::Return + | TerminatorKind::Unreachable => { + unreachable!("unexpected terminator {:?}", terminator.kind) + } + }; + + let all_are_self_calls = + relevant_successors.clone().all(|&succ| !results[succ].is_empty()); - for successor in terminator.successors() { - reachable_without_self_call_queue.push(*successor); + if all_are_self_calls { + relevant_successors.flat_map(|&succ| results[succ].iter().copied()).collect() + } else { + vec![] } + }; + + 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); + 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]; + + 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 + + let is_self_call = + call_fn_id == def_id && &call_substs[..caller_substs.len()] == caller_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() {} From 9dd18a31e266b773a8c2b2308f21e7e39139754a Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Sun, 5 Apr 2020 21:01:38 +0200 Subject: [PATCH 2/6] Move closure check upwards --- src/librustc_mir_build/lints.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/librustc_mir_build/lints.rs b/src/librustc_mir_build/lints.rs index 01dd575c51ed1..38cae5793d764 100644 --- a/src/librustc_mir_build/lints.rs +++ b/src/librustc_mir_build/lints.rs @@ -13,21 +13,20 @@ crate fn check<'tcx>(tcx: TyCtxt<'tcx>, body: &ReadOnlyBodyAndCache<'_, 'tcx>, d 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: &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); let mut results = IndexVec::from_elem_n(vec![], body.basic_blocks().len()); let mut queue: VecDeque<_> = self_calls.iter().collect(); From 60e9927125f6fb191275b7f93f3ed34241fc35ef Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Mon, 6 Apr 2020 01:07:33 +0200 Subject: [PATCH 3/6] Merge redundant match arms --- src/librustc_mir_build/lints.rs | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/librustc_mir_build/lints.rs b/src/librustc_mir_build/lints.rs index 38cae5793d764..1b7a8e6d7291f 100644 --- a/src/librustc_mir_build/lints.rs +++ b/src/librustc_mir_build/lints.rs @@ -47,21 +47,18 @@ fn check_fn_for_unconditional_recursion<'tcx>( // Converging successors without unwind paths. let terminator = body[bb].terminator(); let relevant_successors = match &terminator.kind { - TerminatorKind::Call { destination: Some((_, dest)), .. } => { - Some(dest).into_iter().chain(&[]) - } - TerminatorKind::Call { destination: None, .. } => None.into_iter().chain(&[]), + 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, .. } => Some(target).into_iter().chain(&[]), - TerminatorKind::Yield { .. } | TerminatorKind::GeneratorDrop => { - None.into_iter().chain(&[]) - } - TerminatorKind::FalseEdges { real_target, .. } - | TerminatorKind::FalseUnwind { real_target, .. } => { - Some(real_target).into_iter().chain(&[]) + | TerminatorKind::Assert { target, .. } + | TerminatorKind::FalseEdges { real_target: target, .. } + | TerminatorKind::FalseUnwind { real_target: target, .. } + | TerminatorKind::Call { destination: Some((_, target)), .. } => { + Some(target).into_iter().chain(&[]) } TerminatorKind::Resume | TerminatorKind::Abort From dec90784ac143aba2eed9363e212a9197482f0ed Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Mon, 6 Apr 2020 01:52:51 +0200 Subject: [PATCH 4/6] Add some comments and rename variable --- src/librustc_mir_build/lints.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir_build/lints.rs b/src/librustc_mir_build/lints.rs index 1b7a8e6d7291f..3e0028a66d452 100644 --- a/src/librustc_mir_build/lints.rs +++ b/src/librustc_mir_build/lints.rs @@ -64,6 +64,7 @@ fn check_fn_for_unconditional_recursion<'tcx>( | TerminatorKind::Abort | TerminatorKind::Return | TerminatorKind::Unreachable => { + // We propagate backwards, so these should never be encountered here. unreachable!("unexpected terminator {:?}", terminator.kind) } }; @@ -118,13 +119,15 @@ fn find_blocks_calling_self<'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 caller_substs = &InternalSubsts::identity_for_item(tcx, def_id)[..trait_substs_count]; + let trait_substs = &InternalSubsts::identity_for_item(tcx, def_id)[..trait_substs_count]; let mut self_calls = BitSet::new_empty(body.basic_blocks().len()); @@ -142,8 +145,12 @@ fn find_blocks_calling_self<'tcx>( // 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[..caller_substs.len()] == caller_substs; + call_fn_id == def_id && &call_substs[..trait_substs.len()] == trait_substs; if is_self_call { self_calls.insert(bb); From b30d906a983f24e4c03a64b032485ff9950d83b2 Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Tue, 7 Apr 2020 01:10:49 +0200 Subject: [PATCH 5/6] Add some more comments --- src/librustc_mir_build/lints.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/librustc_mir_build/lints.rs b/src/librustc_mir_build/lints.rs index 3e0028a66d452..74ab602541596 100644 --- a/src/librustc_mir_build/lints.rs +++ b/src/librustc_mir_build/lints.rs @@ -28,7 +28,12 @@ fn check_fn_for_unconditional_recursion<'tcx>( def_id: DefId, ) { let self_calls = find_blocks_calling_self(tcx, &body, def_id); + + // Stores a list of `Span`s for every basic block. Those are the spans of `Call` terminators + // where we know that one of them will definitely be reached. let mut results = IndexVec::from_elem_n(vec![], body.basic_blocks().len()); + + // We start the analysis at the self calls and work backwards. let mut queue: VecDeque<_> = self_calls.iter().collect(); while let Some(bb) = queue.pop_front() { @@ -39,6 +44,8 @@ fn check_fn_for_unconditional_recursion<'tcx>( 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 @@ -69,12 +76,16 @@ fn check_fn_for_unconditional_recursion<'tcx>( } }; + // 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![] } }; From b8f416d67ff77d6eb71902895b59abbfb50737db Mon Sep 17 00:00:00 2001 From: Jonas Schievink Date: Tue, 7 Apr 2020 19:30:16 +0200 Subject: [PATCH 6/6] Further improve comments --- src/librustc_mir_build/lints.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/librustc_mir_build/lints.rs b/src/librustc_mir_build/lints.rs index 74ab602541596..a7370c36f0b9f 100644 --- a/src/librustc_mir_build/lints.rs +++ b/src/librustc_mir_build/lints.rs @@ -29,8 +29,9 @@ fn check_fn_for_unconditional_recursion<'tcx>( ) { let self_calls = find_blocks_calling_self(tcx, &body, def_id); - // Stores a list of `Span`s for every basic block. Those are the spans of `Call` terminators - // where we know that one of them will definitely be reached. + // 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()); // We start the analysis at the self calls and work backwards. @@ -51,7 +52,7 @@ fn check_fn_for_unconditional_recursion<'tcx>( // If *all* successors of `bb` lead to a self-call, emit notes at all of their // locations. - // Converging successors without unwind paths. + // 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, .. }