Auto merge of #113902 - Enselic:lint-recursive-drop, r=oli-obk

Make `unconditional_recursion` warning detect recursive drops

Closes #55388

Also closes #50049 unless we want to keep it for the second example which this PR does not solve, but I think it is better to track that work in #57965.

r? `@oli-obk` since you are the mentor for #55388

Unresolved questions:
- [x] There are two false positives that must be fixed before merging (see diff). I suspect the best way to solve them is to perform analysis after drop elaboration instead of before, as now, but I have not explored that any further yet. Could that be an option? **Answer:** Yes, that solved the problem.

`@rustbot` label +T-compiler +C-enhancement +A-lint

Author: bors

Cargo.lock

@@ -4046,6 +4046,7 @@ dependencies = [
  "rustc_index",
  "rustc_macros",
  "rustc_middle",
+ "rustc_mir_build",
  "rustc_mir_dataflow",
  "rustc_serialize",
  "rustc_session",

compiler/rustc_mir_build/src/lib.rs

@@ -19,7 +19,7 @@ extern crate rustc_middle;
 mod build;
 mod check_unsafety;
 mod errors;
-mod lints;
+pub mod lints;
 pub mod thir;
 
 use rustc_middle::query::Providers;

compiler/rustc_mir_build/src/lints.rs

@@ -3,14 +3,18 @@ use rustc_data_structures::graph::iterate::{
     NodeStatus, TriColorDepthFirstSearch, TriColorVisitor,
 };
 use rustc_hir::def::DefKind;
-use rustc_middle::mir::{self, BasicBlock, BasicBlocks, Body, Operand, TerminatorKind};
-use rustc_middle::ty::{self, Instance, TyCtxt};
+use rustc_middle::mir::{self, BasicBlock, BasicBlocks, Body, Terminator, TerminatorKind};
+use rustc_middle::ty::{self, Instance, Ty, TyCtxt};
 use rustc_middle::ty::{GenericArg, GenericArgs};
 use rustc_session::lint::builtin::UNCONDITIONAL_RECURSION;
 use rustc_span::Span;
 use std::ops::ControlFlow;
 
 pub(crate) fn check<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>) {
+    check_call_recursion(tcx, body);
+}
+
+fn check_call_recursion<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>) {
     let def_id = body.source.def_id().expect_local();
 
     if let DefKind::Fn | DefKind::AssocFn = tcx.def_kind(def_id) {
@@ -23,7 +27,19 @@ pub(crate) fn check<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>) {
             _ => &[],
         };
 
-        let mut vis = Search { tcx, body, reachable_recursive_calls: vec![], trait_args };
+        check_recursion(tcx, body, CallRecursion { trait_args })
+    }
+}
+
+fn check_recursion<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    body: &Body<'tcx>,
+    classifier: impl TerminatorClassifier<'tcx>,
+) {
+    let def_id = body.source.def_id().expect_local();
+
+    if let DefKind::Fn | DefKind::AssocFn = tcx.def_kind(def_id) {
+        let mut vis = Search { tcx, body, classifier, reachable_recursive_calls: vec![] };
         if let Some(NonRecursive) =
             TriColorDepthFirstSearch::new(&body.basic_blocks).run_from_start(&mut vis)
         {
@@ -46,20 +62,66 @@ pub(crate) fn check<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>) {
     }
 }
 
+/// Requires drop elaboration to have been performed first.
+pub fn check_drop_recursion<'tcx>(tcx: TyCtxt<'tcx>, body: &Body<'tcx>) {
+    let def_id = body.source.def_id().expect_local();
+
+    // First check if `body` is an `fn drop()` of `Drop`
+    if let DefKind::AssocFn = tcx.def_kind(def_id) &&
+        let Some(trait_ref) = tcx.impl_of_method(def_id.to_def_id()).and_then(|def_id| tcx.impl_trait_ref(def_id)) &&
+        let Some(drop_trait) = tcx.lang_items().drop_trait() && drop_trait == trait_ref.instantiate_identity().def_id {
+
+        // It was. Now figure out for what type `Drop` is implemented and then
+        // check for recursion.
+        if let ty::Ref(_, dropped_ty, _) = tcx.liberate_late_bound_regions(
+            def_id.to_def_id(),
+            tcx.fn_sig(def_id).instantiate_identity().input(0),
+        ).kind() {
+            check_recursion(tcx, body, RecursiveDrop { drop_for: *dropped_ty });
+        }
+    }
+}
+
+trait TerminatorClassifier<'tcx> {
+    fn is_recursive_terminator(
+        &self,
+        tcx: TyCtxt<'tcx>,
+        body: &Body<'tcx>,
+        terminator: &Terminator<'tcx>,
+    ) -> bool;
+}
+
 struct NonRecursive;
 
-struct Search<'mir, 'tcx> {
+struct Search<'mir, 'tcx, C: TerminatorClassifier<'tcx>> {
     tcx: TyCtxt<'tcx>,
     body: &'mir Body<'tcx>,
-    trait_args: &'tcx [GenericArg<'tcx>],
+    classifier: C,
 
     reachable_recursive_calls: Vec<Span>,
 }
 
-impl<'mir, 'tcx> Search<'mir, 'tcx> {
+struct CallRecursion<'tcx> {
+    trait_args: &'tcx [GenericArg<'tcx>],
+}
+
+struct RecursiveDrop<'tcx> {
+    /// The type that `Drop` is implemented for.
+    drop_for: Ty<'tcx>,
+}
+
+impl<'tcx> TerminatorClassifier<'tcx> for CallRecursion<'tcx> {
     /// Returns `true` if `func` refers to the function we are searching in.
-    fn is_recursive_call(&self, func: &Operand<'tcx>, args: &[Operand<'tcx>]) -> bool {
-        let Search { tcx, body, trait_args, .. } = *self;
+    fn is_recursive_terminator(
+        &self,
+        tcx: TyCtxt<'tcx>,
+        body: &Body<'tcx>,
+        terminator: &Terminator<'tcx>,
+    ) -> bool {
+        let TerminatorKind::Call { func, args, .. } = &terminator.kind else {
+            return false;
+        };
+
         // Resolving function type to a specific instance that is being called is expensive. To
         // avoid the cost we check the number of arguments first, which is sufficient to reject
         // most of calls as non-recursive.
@@ -86,14 +148,30 @@ impl<'mir, 'tcx> Search<'mir, 'tcx> {
             // 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).
-            return callee == caller && &call_args[..trait_args.len()] == trait_args;
+            return callee == caller && &call_args[..self.trait_args.len()] == self.trait_args;
         }
 
         false
     }
 }
 
-impl<'mir, 'tcx> TriColorVisitor<BasicBlocks<'tcx>> for Search<'mir, 'tcx> {
+impl<'tcx> TerminatorClassifier<'tcx> for RecursiveDrop<'tcx> {
+    fn is_recursive_terminator(
+        &self,
+        tcx: TyCtxt<'tcx>,
+        body: &Body<'tcx>,
+        terminator: &Terminator<'tcx>,
+    ) -> bool {
+        let TerminatorKind::Drop { place, .. } = &terminator.kind else { return false };
+
+        let dropped_ty = place.ty(body, tcx).ty;
+        dropped_ty == self.drop_for
+    }
+}
+
+impl<'mir, 'tcx, C: TerminatorClassifier<'tcx>> TriColorVisitor<BasicBlocks<'tcx>>
+    for Search<'mir, 'tcx, C>
+{
     type BreakVal = NonRecursive;
 
     fn node_examined(
@@ -138,26 +216,23 @@ impl<'mir, 'tcx> TriColorVisitor<BasicBlocks<'tcx>> for Search<'mir, 'tcx> {
     fn node_settled(&mut self, bb: BasicBlock) -> ControlFlow<Self::BreakVal> {
         // When we examine a node for the last time, remember it if it is a recursive call.
         let terminator = self.body[bb].terminator();
-        if let TerminatorKind::Call { func, args, .. } = &terminator.kind {
-            if self.is_recursive_call(func, args) {
-                self.reachable_recursive_calls.push(terminator.source_info.span);
-            }
+        if self.classifier.is_recursive_terminator(self.tcx, self.body, terminator) {
+            self.reachable_recursive_calls.push(terminator.source_info.span);
         }
 
         ControlFlow::Continue(())
     }
 
     fn ignore_edge(&mut self, bb: BasicBlock, target: BasicBlock) -> bool {
         let terminator = self.body[bb].terminator();
-        if terminator.unwind() == Some(&mir::UnwindAction::Cleanup(target))
-            && terminator.successors().count() > 1
+        let ignore_unwind = terminator.unwind() == Some(&mir::UnwindAction::Cleanup(target))
+            && terminator.successors().count() > 1;
+        if ignore_unwind || self.classifier.is_recursive_terminator(self.tcx, self.body, terminator)
         {
             return true;
         }
-        // Don't traverse successors of recursive calls or false CFG edges.
-        match self.body[bb].terminator().kind {
-            TerminatorKind::Call { ref func, ref args, .. } => self.is_recursive_call(func, args),
-            TerminatorKind::FalseEdge { imaginary_target, .. } => imaginary_target == target,
+        match &terminator.kind {
+            TerminatorKind::FalseEdge { imaginary_target, .. } => imaginary_target == &target,
             _ => false,
         }
     }

compiler/rustc_mir_transform/Cargo.toml

@@ -18,6 +18,7 @@ rustc_hir = { path = "../rustc_hir" }
 rustc_index = { path = "../rustc_index" }
 rustc_middle = { path = "../rustc_middle" }
 rustc_const_eval = { path = "../rustc_const_eval" }
+rustc_mir_build = { path = "../rustc_mir_build" }
 rustc_mir_dataflow = { path = "../rustc_mir_dataflow" }
 rustc_serialize = { path = "../rustc_serialize" }
 rustc_session = { path = "../rustc_session" }

compiler/rustc_mir_transform/src/lib.rs

@@ -424,6 +424,10 @@ fn mir_drops_elaborated_and_const_checked(tcx: TyCtxt<'_>, def: LocalDefId) -> &
 
     run_analysis_to_runtime_passes(tcx, &mut body);
 
+    // Now that drop elaboration has been performed, we can check for
+    // unconditional drop recursion.
+    rustc_mir_build::lints::check_drop_recursion(tcx, &body);
+
     tcx.alloc_steal_mir(body)
 }
 

tests/ui/lint/lint-unconditional-drop-recursion.rs

@@ -0,0 +1,38 @@
+// Because drop recursion can only be detected after drop elaboration which
+// happens for codegen:
+// build-fail
+
+#![deny(unconditional_recursion)]
+#![allow(dead_code)]
+
+pub struct RecursiveDrop;
+
+impl Drop for RecursiveDrop {
+    fn drop(&mut self) { //~ ERROR function cannot return without recursing
+        let _ = RecursiveDrop;
+    }
+}
+
+#[derive(Default)]
+struct NotRecursiveDrop1;
+
+impl Drop for NotRecursiveDrop1 {
+    fn drop(&mut self) {
+        // Before drop elaboration, the MIR can look like a recursive drop will
+        // occur. But it will not, since forget() prevents drop() from running.
+        let taken = std::mem::take(self);
+        std::mem::forget(taken);
+    }
+}
+
+struct NotRecursiveDrop2;
+
+impl Drop for NotRecursiveDrop2 {
+    fn drop(&mut self) {
+        // Before drop elaboration, the MIR can look like a recursive drop will
+        // occur. But it will not, since this will panic.
+        std::panic::panic_any(NotRecursiveDrop2);
+    }
+}
+
+fn main() {}

tests/ui/lint/lint-unconditional-drop-recursion.stderr

@@ -0,0 +1,17 @@
+error: function cannot return without recursing
+  --> $DIR/lint-unconditional-drop-recursion.rs:11:5
+   |
+LL |     fn drop(&mut self) {
+   |     ^^^^^^^^^^^^^^^^^^ cannot return without recursing
+LL |         let _ = RecursiveDrop;
+   |                              - recursive call site
+   |
+   = help: a `loop` may express intention better if this is on purpose
+note: the lint level is defined here
+  --> $DIR/lint-unconditional-drop-recursion.rs:5:9
+   |
+LL | #![deny(unconditional_recursion)]
+   |         ^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to previous error
+