Rollup merge of #70822 - jonas-schievink:curse-of-the-recursion, r=ecstatic-morse

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

Fixes #54444

Author: Centril

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

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<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
+}

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() {}