Skip to content

Commit

Permalink
Auto merge of #132329 - compiler-errors:fn-and-destruct, r=lcnr
Browse files Browse the repository at this point in the history
Implement `~const Destruct` effect goal in the new solver

This also fixed a subtle bug/limitation of the `NeedsConstDrop` check. Specifically, the "`Qualif`" API basically treats const drops as totally structural, even though dropping something that has an explicit `Drop` implementation cannot be structurally decomposed. For example:

```rust
#![feature(const_trait_impl)]

#[const_trait] trait Foo {
    fn foo();
}

struct Conditional<T: Foo>(T);

impl Foo for () {
    fn foo() {
        println!("uh oh");
    }
}

impl<T> const Drop for Conditional<T> where T: ~const Foo {
    fn drop(&mut self) {
        T::foo();
    }
}

const FOO: () = {
    let _ = Conditional(());
    //~^ This should error.
};

fn main() {}
```

In this example, when checking if the `Conditional(())` rvalue is const-drop, since `Conditional` has a const destructor, we would previously recurse into the `()` value and determine it has nothing to drop, which means that it is considered to *not* need a const drop -- even though dropping `Conditional(())` would mean evaluating the destructor which relies on that `T: const Foo` bound to hold!

This could be fixed alternatively by banning any const conditions on `const Drop` impls, but that really sucks -- that means that basically no *interesting* const drop impls could be written. We have the capability to totally and intuitively support the right behavior, which I've implemented here.
  • Loading branch information
bors committed Nov 23, 2024
2 parents f5be3ca + 69a38de commit 743003b
Show file tree
Hide file tree
Showing 43 changed files with 524 additions and 230 deletions.
71 changes: 40 additions & 31 deletions compiler/rustc_const_eval/src/check_consts/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use rustc_span::{Span, Symbol, sym};
use rustc_trait_selection::traits::{
Obligation, ObligationCause, ObligationCauseCode, ObligationCtxt,
};
use tracing::{debug, instrument, trace};
use tracing::{instrument, trace};

use super::ops::{self, NonConstOp, Status};
use super::qualifs::{self, HasMutInterior, NeedsDrop, NeedsNonConstDrop};
Expand All @@ -47,7 +47,7 @@ impl<'mir, 'tcx> Qualifs<'mir, 'tcx> {
/// Returns `true` if `local` is `NeedsDrop` at the given `Location`.
///
/// Only updates the cursor if absolutely necessary
fn needs_drop(
pub(crate) fn needs_drop(
&mut self,
ccx: &'mir ConstCx<'mir, 'tcx>,
local: Local,
Expand Down Expand Up @@ -421,6 +421,43 @@ impl<'mir, 'tcx> Checker<'mir, 'tcx> {

true
}

pub fn check_drop_terminator(
&mut self,
dropped_place: Place<'tcx>,
location: Location,
terminator_span: Span,
) {
let ty_of_dropped_place = dropped_place.ty(self.body, self.tcx).ty;

let needs_drop = if let Some(local) = dropped_place.as_local() {
self.qualifs.needs_drop(self.ccx, local, location)
} else {
qualifs::NeedsDrop::in_any_value_of_ty(self.ccx, ty_of_dropped_place)
};
// If this type doesn't need a drop at all, then there's nothing to enforce.
if !needs_drop {
return;
}

let mut err_span = self.span;
let needs_non_const_drop = if let Some(local) = dropped_place.as_local() {
// Use the span where the local was declared as the span of the drop error.
err_span = self.body.local_decls[local].source_info.span;
self.qualifs.needs_non_const_drop(self.ccx, local, location)
} else {
qualifs::NeedsNonConstDrop::in_any_value_of_ty(self.ccx, ty_of_dropped_place)
};

self.check_op_spanned(
ops::LiveDrop {
dropped_at: terminator_span,
dropped_ty: ty_of_dropped_place,
needs_non_const_drop,
},
err_span,
);
}
}

impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
Expand Down Expand Up @@ -866,35 +903,7 @@ impl<'tcx> Visitor<'tcx> for Checker<'_, 'tcx> {
return;
}

let mut err_span = self.span;
let ty_of_dropped_place = dropped_place.ty(self.body, self.tcx).ty;

let ty_needs_non_const_drop =
qualifs::NeedsNonConstDrop::in_any_value_of_ty(self.ccx, ty_of_dropped_place);

debug!(?ty_of_dropped_place, ?ty_needs_non_const_drop);

if !ty_needs_non_const_drop {
return;
}

let needs_non_const_drop = if let Some(local) = dropped_place.as_local() {
// Use the span where the local was declared as the span of the drop error.
err_span = self.body.local_decls[local].source_info.span;
self.qualifs.needs_non_const_drop(self.ccx, local, location)
} else {
true
};

if needs_non_const_drop {
self.check_op_spanned(
ops::LiveDrop {
dropped_at: Some(terminator.source_info.span),
dropped_ty: ty_of_dropped_place,
},
err_span,
);
}
self.check_drop_terminator(*dropped_place, location, terminator.source_info.span);
}

TerminatorKind::InlineAsm { .. } => self.check_op(ops::InlineAsm),
Expand Down
40 changes: 33 additions & 7 deletions compiler/rustc_const_eval/src/check_consts/ops.rs
Original file line number Diff line number Diff line change
Expand Up @@ -459,17 +459,43 @@ impl<'tcx> NonConstOp<'tcx> for InlineAsm {

#[derive(Debug)]
pub(crate) struct LiveDrop<'tcx> {
pub dropped_at: Option<Span>,
pub dropped_at: Span,
pub dropped_ty: Ty<'tcx>,
pub needs_non_const_drop: bool,
}
impl<'tcx> NonConstOp<'tcx> for LiveDrop<'tcx> {
fn status_in_item(&self, _ccx: &ConstCx<'_, 'tcx>) -> Status {
if self.needs_non_const_drop {
Status::Forbidden
} else {
Status::Unstable {
gate: sym::const_destruct,
gate_already_checked: false,
safe_to_expose_on_stable: false,
is_function_call: false,
}
}
}

fn build_error(&self, ccx: &ConstCx<'_, 'tcx>, span: Span) -> Diag<'tcx> {
ccx.dcx().create_err(errors::LiveDrop {
span,
dropped_ty: self.dropped_ty,
kind: ccx.const_kind(),
dropped_at: self.dropped_at,
})
if self.needs_non_const_drop {
ccx.dcx().create_err(errors::LiveDrop {
span,
dropped_ty: self.dropped_ty,
kind: ccx.const_kind(),
dropped_at: self.dropped_at,
})
} else {
ccx.tcx.sess.create_feature_err(
errors::LiveDrop {
span,
dropped_ty: self.dropped_ty,
kind: ccx.const_kind(),
dropped_at: self.dropped_at,
},
sym::const_destruct,
)
}
}
}

Expand Down
57 changes: 12 additions & 45 deletions compiler/rustc_const_eval/src/check_consts/post_drop_elaboration.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,11 @@
use rustc_middle::mir::visit::Visitor;
use rustc_middle::mir::{self, BasicBlock, Location};
use rustc_middle::ty::{Ty, TyCtxt};
use rustc_span::Span;
use rustc_middle::ty::TyCtxt;
use rustc_span::symbol::sym;
use tracing::trace;

use super::ConstCx;
use super::check::Qualifs;
use super::ops::{self, NonConstOp};
use super::qualifs::{NeedsNonConstDrop, Qualif};
use crate::check_consts::check::Checker;
use crate::check_consts::rustc_allow_const_fn_unstable;

/// Returns `true` if we should use the more precise live drop checker that runs after drop
Expand Down Expand Up @@ -45,29 +42,16 @@ pub fn check_live_drops<'tcx>(tcx: TyCtxt<'tcx>, body: &mir::Body<'tcx>) {
return;
}

let mut visitor = CheckLiveDrops { ccx: &ccx, qualifs: Qualifs::default() };
// I know it's not great to be creating a new const checker, but I'd
// rather use it so we can deduplicate the error emitting logic that
// it contains.
let mut visitor = CheckLiveDrops { checker: Checker::new(&ccx) };

visitor.visit_body(body);
}

struct CheckLiveDrops<'mir, 'tcx> {
ccx: &'mir ConstCx<'mir, 'tcx>,
qualifs: Qualifs<'mir, 'tcx>,
}

// So we can access `body` and `tcx`.
impl<'mir, 'tcx> std::ops::Deref for CheckLiveDrops<'mir, 'tcx> {
type Target = ConstCx<'mir, 'tcx>;

fn deref(&self) -> &Self::Target {
self.ccx
}
}

impl<'tcx> CheckLiveDrops<'_, 'tcx> {
fn check_live_drop(&self, span: Span, dropped_ty: Ty<'tcx>) {
ops::LiveDrop { dropped_at: None, dropped_ty }.build_error(self.ccx, span).emit();
}
checker: Checker<'mir, 'tcx>,
}

impl<'tcx> Visitor<'tcx> for CheckLiveDrops<'_, 'tcx> {
Expand All @@ -87,28 +71,11 @@ impl<'tcx> Visitor<'tcx> for CheckLiveDrops<'_, 'tcx> {

match &terminator.kind {
mir::TerminatorKind::Drop { place: dropped_place, .. } => {
let dropped_ty = dropped_place.ty(self.body, self.tcx).ty;

if !NeedsNonConstDrop::in_any_value_of_ty(self.ccx, dropped_ty) {
// Instead of throwing a bug, we just return here. This is because we have to
// run custom `const Drop` impls.
return;
}

if dropped_place.is_indirect() {
self.check_live_drop(terminator.source_info.span, dropped_ty);
return;
}

// Drop elaboration is not precise enough to accept code like
// `tests/ui/consts/control-flow/drop-pass.rs`; e.g., when an `Option<Vec<T>>` is
// initialized with `None` and never changed, it still emits drop glue.
// Hence we additionally check the qualifs here to allow more code to pass.
if self.qualifs.needs_non_const_drop(self.ccx, dropped_place.local, location) {
// Use the span where the dropped local was declared for the error.
let span = self.body.local_decls[dropped_place.local].source_info.span;
self.check_live_drop(span, dropped_ty);
}
self.checker.check_drop_terminator(
*dropped_place,
location,
terminator.source_info.span,
);
}

mir::TerminatorKind::UnwindTerminate(_)
Expand Down
Loading

0 comments on commit 743003b

Please sign in to comment.