Skip to content

Commit

Permalink
Auto merge of rust-lang#113902 - Enselic:lint-recursive-drop, r=oli-obk
Browse files Browse the repository at this point in the history
Make `unconditional_recursion` warning detect recursive drops

Closes rust-lang#55388

Also closes rust-lang#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 rust-lang#57965.

r? `@oli-obk` since you are the mentor for rust-lang#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
  • Loading branch information
bors committed Aug 7, 2023
2 parents 139b49b + b4b33df commit 84ec263
Show file tree
Hide file tree
Showing 7 changed files with 157 additions and 21 deletions.
1 change: 1 addition & 0 deletions Cargo.lock
Original file line number Diff line number Diff line change
Expand Up @@ -4046,6 +4046,7 @@ dependencies = [
"rustc_index",
"rustc_macros",
"rustc_middle",
"rustc_mir_build",
"rustc_mir_dataflow",
"rustc_serialize",
"rustc_session",
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_build/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
115 changes: 95 additions & 20 deletions compiler/rustc_mir_build/src/lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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)
{
Expand All @@ -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.
Expand All @@ -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(
Expand Down Expand Up @@ -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,
}
}
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_mir_transform/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_mir_transform/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
38 changes: 38 additions & 0 deletions tests/ui/lint/lint-unconditional-drop-recursion.rs
Original file line number Diff line number Diff line change
@@ -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() {}
17 changes: 17 additions & 0 deletions tests/ui/lint/lint-unconditional-drop-recursion.stderr
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit 84ec263

Please sign in to comment.