From f92d6699b8600c037c15d2035065c3922e21099e Mon Sep 17 00:00:00 2001 From: Martin Nordholts Date: Thu, 20 Jul 2023 16:17:20 +0200 Subject: [PATCH 1/3] Avoid unneeded `terminator()` call in `fn ignore_edge()` --- compiler/rustc_mir_build/src/lints.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/rustc_mir_build/src/lints.rs b/compiler/rustc_mir_build/src/lints.rs index 3f0cc69ec59..1dabc41a59f 100644 --- a/compiler/rustc_mir_build/src/lints.rs +++ b/compiler/rustc_mir_build/src/lints.rs @@ -155,9 +155,9 @@ fn ignore_edge(&mut self, bb: BasicBlock, target: BasicBlock) -> bool { 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::Call { func, args, .. } => self.is_recursive_call(func, args), + TerminatorKind::FalseEdge { imaginary_target, .. } => imaginary_target == &target, _ => false, } } From eed34b8bc1ef39fd3d76fc72e54d346f90a26ef0 Mon Sep 17 00:00:00 2001 From: Martin Nordholts Date: Thu, 20 Jul 2023 16:29:26 +0200 Subject: [PATCH 2/3] Add is_recursive_terminator() helper for `unconditional_recursion` lint --- compiler/rustc_mir_build/src/lints.rs | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/compiler/rustc_mir_build/src/lints.rs b/compiler/rustc_mir_build/src/lints.rs index 1dabc41a59f..25a6ef42215 100644 --- a/compiler/rustc_mir_build/src/lints.rs +++ b/compiler/rustc_mir_build/src/lints.rs @@ -3,7 +3,7 @@ NodeStatus, TriColorDepthFirstSearch, TriColorVisitor, }; use rustc_hir::def::DefKind; -use rustc_middle::mir::{self, BasicBlock, BasicBlocks, Body, Operand, TerminatorKind}; +use rustc_middle::mir::{self, BasicBlock, BasicBlocks, Body, Operand, Terminator, TerminatorKind}; use rustc_middle::ty::{self, Instance, TyCtxt}; use rustc_middle::ty::{GenericArg, GenericArgs}; use rustc_session::lint::builtin::UNCONDITIONAL_RECURSION; @@ -57,6 +57,13 @@ struct Search<'mir, 'tcx> { } impl<'mir, 'tcx> Search<'mir, 'tcx> { + fn is_recursive_terminator(&self, terminator: &Terminator<'tcx>) -> bool { + match &terminator.kind { + TerminatorKind::Call { func, args, .. } => self.is_recursive_call(func, args), + _ => false, + } + } + /// 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; @@ -138,10 +145,8 @@ fn node_examined( fn node_settled(&mut self, bb: BasicBlock) -> ControlFlow { // 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.is_recursive_terminator(terminator) { + self.reachable_recursive_calls.push(terminator.source_info.span); } ControlFlow::Continue(()) @@ -149,14 +154,12 @@ fn node_settled(&mut self, bb: BasicBlock) -> ControlFlow { 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.is_recursive_terminator(terminator) { return true; } - // Don't traverse successors of recursive calls or false CFG edges. match &terminator.kind { - TerminatorKind::Call { func, args, .. } => self.is_recursive_call(func, args), TerminatorKind::FalseEdge { imaginary_target, .. } => imaginary_target == &target, _ => false, } From b4b33df983b4badac5c2578756cd42a76236be8d Mon Sep 17 00:00:00 2001 From: Martin Nordholts Date: Thu, 20 Jul 2023 21:01:27 +0200 Subject: [PATCH 3/3] Make `unconditional_recursion` warning detect recursive drops --- Cargo.lock | 1 + compiler/rustc_mir_build/src/lib.rs | 2 +- compiler/rustc_mir_build/src/lints.rs | 108 +++++++++++++++--- compiler/rustc_mir_transform/Cargo.toml | 1 + compiler/rustc_mir_transform/src/lib.rs | 4 + .../lint/lint-unconditional-drop-recursion.rs | 38 ++++++ .../lint-unconditional-drop-recursion.stderr | 17 +++ 7 files changed, 152 insertions(+), 19 deletions(-) create mode 100644 tests/ui/lint/lint-unconditional-drop-recursion.rs create mode 100644 tests/ui/lint/lint-unconditional-drop-recursion.stderr diff --git a/Cargo.lock b/Cargo.lock index 8c2c3f5e628..3e372115e13 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3989,6 +3989,7 @@ dependencies = [ "rustc_index", "rustc_macros", "rustc_middle", + "rustc_mir_build", "rustc_mir_dataflow", "rustc_serialize", "rustc_session", diff --git a/compiler/rustc_mir_build/src/lib.rs b/compiler/rustc_mir_build/src/lib.rs index 4fdc3178c4e..099fefbf068 100644 --- a/compiler/rustc_mir_build/src/lib.rs +++ b/compiler/rustc_mir_build/src/lib.rs @@ -19,7 +19,7 @@ mod build; mod check_unsafety; mod errors; -mod lints; +pub mod lints; pub mod thir; use rustc_middle::query::Providers; diff --git a/compiler/rustc_mir_build/src/lints.rs b/compiler/rustc_mir_build/src/lints.rs index 25a6ef42215..7fb73b5c7b2 100644 --- a/compiler/rustc_mir_build/src/lints.rs +++ b/compiler/rustc_mir_build/src/lints.rs @@ -3,14 +3,18 @@ NodeStatus, TriColorDepthFirstSearch, TriColorVisitor, }; use rustc_hir::def::DefKind; -use rustc_middle::mir::{self, BasicBlock, BasicBlocks, Body, Operand, Terminator, 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,27 +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, } -impl<'mir, 'tcx> Search<'mir, 'tcx> { - fn is_recursive_terminator(&self, terminator: &Terminator<'tcx>) -> bool { - match &terminator.kind { - TerminatorKind::Call { func, args, .. } => self.is_recursive_call(func, args), - _ => false, - } - } +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. @@ -93,14 +148,30 @@ fn is_recursive_call(&self, func: &Operand<'tcx>, args: &[Operand<'tcx>]) -> boo // 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). - 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> 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> + for Search<'mir, 'tcx, C> +{ type BreakVal = NonRecursive; fn node_examined( @@ -145,7 +216,7 @@ fn node_examined( fn node_settled(&mut self, bb: BasicBlock) -> ControlFlow { // When we examine a node for the last time, remember it if it is a recursive call. let terminator = self.body[bb].terminator(); - if self.is_recursive_terminator(terminator) { + if self.classifier.is_recursive_terminator(self.tcx, self.body, terminator) { self.reachable_recursive_calls.push(terminator.source_info.span); } @@ -156,7 +227,8 @@ fn ignore_edge(&mut self, bb: BasicBlock, target: BasicBlock) -> bool { let terminator = self.body[bb].terminator(); let ignore_unwind = terminator.unwind() == Some(&mir::UnwindAction::Cleanup(target)) && terminator.successors().count() > 1; - if ignore_unwind || self.is_recursive_terminator(terminator) { + if ignore_unwind || self.classifier.is_recursive_terminator(self.tcx, self.body, terminator) + { return true; } match &terminator.kind { diff --git a/compiler/rustc_mir_transform/Cargo.toml b/compiler/rustc_mir_transform/Cargo.toml index eca5f98a2c0..f1198d9bfd3 100644 --- a/compiler/rustc_mir_transform/Cargo.toml +++ b/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" } diff --git a/compiler/rustc_mir_transform/src/lib.rs b/compiler/rustc_mir_transform/src/lib.rs index d419329f2d6..abbac8f2554 100644 --- a/compiler/rustc_mir_transform/src/lib.rs +++ b/compiler/rustc_mir_transform/src/lib.rs @@ -446,6 +446,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) } diff --git a/tests/ui/lint/lint-unconditional-drop-recursion.rs b/tests/ui/lint/lint-unconditional-drop-recursion.rs new file mode 100644 index 00000000000..348cd280139 --- /dev/null +++ b/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() {} diff --git a/tests/ui/lint/lint-unconditional-drop-recursion.stderr b/tests/ui/lint/lint-unconditional-drop-recursion.stderr new file mode 100644 index 00000000000..76f95481605 --- /dev/null +++ b/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 +