Skip to content

Do not promote values with const drop that need to be dropped #89988

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 37 additions & 8 deletions compiler/rustc_const_eval/src/transform/check_consts/check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use std::mem;
use std::ops::Deref;

use super::ops::{self, NonConstOp, Status};
use super::qualifs::{self, CustomEq, HasMutInterior, NeedsNonConstDrop};
use super::qualifs::{self, CustomEq, HasMutInterior, NeedsDrop, NeedsNonConstDrop};
use super::resolver::FlowSensitiveAnalysis;
use super::{is_lang_panic_fn, is_lang_special_const_fn, ConstCx, Qualif};
use crate::const_eval::is_unstable_const_fn;
Expand All @@ -39,7 +39,8 @@ type QualifResults<'mir, 'tcx, Q> =
#[derive(Default)]
pub struct Qualifs<'mir, 'tcx> {
has_mut_interior: Option<QualifResults<'mir, 'tcx, HasMutInterior>>,
needs_drop: Option<QualifResults<'mir, 'tcx, NeedsNonConstDrop>>,
needs_drop: Option<QualifResults<'mir, 'tcx, NeedsDrop>>,
needs_non_const_drop: Option<QualifResults<'mir, 'tcx, NeedsNonConstDrop>>,
indirectly_mutable: Option<IndirectlyMutableResults<'mir, 'tcx>>,
}

Expand Down Expand Up @@ -80,14 +81,14 @@ impl Qualifs<'mir, 'tcx> {
location: Location,
) -> bool {
let ty = ccx.body.local_decls[local].ty;
if !NeedsNonConstDrop::in_any_value_of_ty(ccx, ty) {
if !NeedsDrop::in_any_value_of_ty(ccx, ty) {
return false;
}

let needs_drop = self.needs_drop.get_or_insert_with(|| {
let ConstCx { tcx, body, .. } = *ccx;

FlowSensitiveAnalysis::new(NeedsNonConstDrop, ccx)
FlowSensitiveAnalysis::new(NeedsDrop, ccx)
.into_engine(tcx, &body)
.iterate_to_fixpoint()
.into_results_cursor(&body)
Expand All @@ -97,6 +98,33 @@ impl Qualifs<'mir, 'tcx> {
needs_drop.get().contains(local) || self.indirectly_mutable(ccx, local, location)
}

/// Returns `true` if `local` is `NeedsNonConstDrop` at the given `Location`.
///
/// Only updates the cursor if absolutely necessary
pub fn needs_non_const_drop(
&mut self,
ccx: &'mir ConstCx<'mir, 'tcx>,
local: Local,
location: Location,
) -> bool {
let ty = ccx.body.local_decls[local].ty;
if !NeedsNonConstDrop::in_any_value_of_ty(ccx, ty) {
return false;
}

let needs_non_const_drop = self.needs_non_const_drop.get_or_insert_with(|| {
let ConstCx { tcx, body, .. } = *ccx;

FlowSensitiveAnalysis::new(NeedsNonConstDrop, ccx)
.into_engine(tcx, &body)
.iterate_to_fixpoint()
.into_results_cursor(&body)
});

needs_non_const_drop.seek_before_primary_effect(location);
needs_non_const_drop.get().contains(local) || self.indirectly_mutable(ccx, local, location)
}

/// Returns `true` if `local` is `HasMutInterior` at the given `Location`.
///
/// Only updates the cursor if absolutely necessary.
Expand Down Expand Up @@ -173,6 +201,7 @@ impl Qualifs<'mir, 'tcx> {

ConstQualifs {
needs_drop: self.needs_drop(ccx, RETURN_PLACE, return_loc),
needs_non_const_drop: self.needs_non_const_drop(ccx, RETURN_PLACE, return_loc),
has_mut_interior: self.has_mut_interior(ccx, RETURN_PLACE, return_loc),
custom_eq,
error_occured,
Expand Down Expand Up @@ -999,7 +1028,7 @@ impl Visitor<'tcx> for Checker<'mir, 'tcx> {
}

// Forbid all `Drop` terminators unless the place being dropped is a local with no
// projections that cannot be `NeedsDrop`.
// projections that cannot be `NeedsNonConstDrop`.
TerminatorKind::Drop { place: dropped_place, .. }
| TerminatorKind::DropAndReplace { place: dropped_place, .. } => {
// If we are checking live drops after drop-elaboration, don't emit duplicate
Expand All @@ -1019,15 +1048,15 @@ impl Visitor<'tcx> for Checker<'mir, 'tcx> {
return;
}

let needs_drop = if let Some(local) = dropped_place.as_local() {
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_drop(self.ccx, local, location)
self.qualifs.needs_non_const_drop(self.ccx, local, location)
} else {
true
};

if needs_drop {
if needs_non_const_drop {
self.check_op_spanned(
ops::LiveDrop { dropped_at: Some(terminator.source_info.span) },
err_span,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ impl Visitor<'tcx> for CheckLiveDrops<'mir, 'tcx> {
// `src/test/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_drop(self.ccx, dropped_place.local, location) {
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);
Expand Down
33 changes: 28 additions & 5 deletions compiler/rustc_const_eval/src/transform/check_consts/qualifs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ pub fn in_any_value_of_ty(
) -> ConstQualifs {
ConstQualifs {
has_mut_interior: HasMutInterior::in_any_value_of_ty(cx, ty),
needs_drop: NeedsNonConstDrop::in_any_value_of_ty(cx, ty),
needs_drop: NeedsDrop::in_any_value_of_ty(cx, ty),
needs_non_const_drop: NeedsNonConstDrop::in_any_value_of_ty(cx, ty),
custom_eq: CustomEq::in_any_value_of_ty(cx, ty),
error_occured,
}
Expand Down Expand Up @@ -98,17 +99,39 @@ impl Qualif for HasMutInterior {
}

/// Constant containing an ADT that implements `Drop`.
/// This must be ruled out (a) because we cannot run `Drop` during compile-time
/// as that might not be a `const fn`, and (b) because implicit promotion would
/// remove side-effects that occur as part of dropping that value.
/// This must be ruled out because implicit promotion would remove side-effects
/// that occur as part of dropping that value. N.B., the implicit promotion has
/// to reject const Drop implementations because even if side-effects are ruled
/// out through other means, the execution of the drop could diverge.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"diverge" means "running into an endless loop", right? Panics seem like a much simpler example to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The sentence was intended to encompass panicking. In the context of rustc we generally use diverge to describe a computation that does not return normally, but I can see where you are coming from.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. Yeah I think that is not standard terminology, at least not in my peer group. ;)

pub struct NeedsDrop;

impl Qualif for NeedsDrop {
const ANALYSIS_NAME: &'static str = "flow_needs_drop";
const IS_CLEARED_ON_MOVE: bool = true;

fn in_qualifs(qualifs: &ConstQualifs) -> bool {
qualifs.needs_drop
}

fn in_any_value_of_ty(cx: &ConstCx<'_, 'tcx>, ty: Ty<'tcx>) -> bool {
ty.needs_drop(cx.tcx, cx.param_env)
}

fn in_adt_inherently(cx: &ConstCx<'_, 'tcx>, adt: &'tcx AdtDef, _: SubstsRef<'tcx>) -> bool {
adt.has_dtor(cx.tcx)
}
}

/// Constant containing an ADT that implements non-const `Drop`.
/// This must be ruled out because we cannot run `Drop` during compile-time.
pub struct NeedsNonConstDrop;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have both NeedsDrop and NeedsNonConrstDrop? That seems rather redundant.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only want to detect nonconst drops in const items, as const drops can actually get executed just fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So why does check_consts have NeedsDrop then? Promotion is a separate pass after all.


impl Qualif for NeedsNonConstDrop {
const ANALYSIS_NAME: &'static str = "flow_needs_nonconst_drop";
const IS_CLEARED_ON_MOVE: bool = true;

fn in_qualifs(qualifs: &ConstQualifs) -> bool {
qualifs.needs_drop
qualifs.needs_non_const_drop
}

fn in_any_value_of_ty(cx: &ConstCx<'_, 'tcx>, mut ty: Ty<'tcx>) -> bool {
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_const_eval/src/transform/promote_consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,7 @@ impl<'tcx> Validator<'_, 'tcx> {

// We cannot promote things that need dropping, since the promoted value
// would not get dropped.
if self.qualif_local::<qualifs::NeedsNonConstDrop>(place.local) {
if self.qualif_local::<qualifs::NeedsDrop>(place.local) {
return Err(Unpromotable);
}

Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_middle/src/mir/query.rs
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ pub struct BorrowCheckResult<'tcx> {
pub struct ConstQualifs {
pub has_mut_interior: bool,
pub needs_drop: bool,
pub needs_non_const_drop: bool,
pub custom_eq: bool,
pub error_occured: Option<ErrorReported>,
}
Expand Down
15 changes: 15 additions & 0 deletions src/test/ui/consts/promoted-const-drop.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#![feature(const_trait_impl)]
#![feature(const_mut_refs)]

struct A();

impl const Drop for A {
fn drop(&mut self) {}
}

const C: A = A();

fn main() {
let _: &'static A = &A(); //~ ERROR temporary value dropped while borrowed
let _: &'static [A] = &[C]; //~ ERROR temporary value dropped while borrowed
}
24 changes: 24 additions & 0 deletions src/test/ui/consts/promoted-const-drop.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
error[E0716]: temporary value dropped while borrowed
--> $DIR/promoted-const-drop.rs:13:26
|
LL | let _: &'static A = &A();
| ---------- ^^^ creates a temporary which is freed while still in use
| |
| type annotation requires that borrow lasts for `'static`
LL | let _: &'static [A] = &[C];
LL | }
| - temporary value is freed at the end of this statement

error[E0716]: temporary value dropped while borrowed
--> $DIR/promoted-const-drop.rs:14:28
|
LL | let _: &'static [A] = &[C];
| ------------ ^^^ creates a temporary which is freed while still in use
| |
| type annotation requires that borrow lasts for `'static`
LL | }
| - temporary value is freed at the end of this statement

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0716`.