Skip to content
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

Only run dataflow for const qualification if type-based check would fail #71330

Merged
merged 4 commits into from
Apr 25, 2020
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
155 changes: 83 additions & 72 deletions src/librustc_mir/transform/check_consts/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
use rustc_errors::struct_span_err;
use rustc_hir::lang_items;
use rustc_hir::{def_id::DefId, HirId};
use rustc_index::bit_set::BitSet;
use rustc_infer::infer::TyCtxtInferExt;
use rustc_middle::mir::visit::{MutatingUseContext, NonMutatingUseContext, PlaceContext, Visitor};
use rustc_middle::mir::*;
Expand All @@ -28,70 +27,100 @@ use crate::dataflow::{self, Analysis};
// We are using `MaybeMutBorrowedLocals` as a proxy for whether an item may have been mutated
// through a pointer prior to the given point. This is okay even though `MaybeMutBorrowedLocals`
// kills locals upon `StorageDead` because a local will never be used after a `StorageDead`.
pub type IndirectlyMutableResults<'mir, 'tcx> =
type IndirectlyMutableResults<'mir, 'tcx> =
dataflow::ResultsCursor<'mir, 'tcx, MaybeMutBorrowedLocals<'mir, 'tcx>>;

struct QualifCursor<'a, 'mir, 'tcx, Q: Qualif> {
cursor: dataflow::ResultsCursor<'mir, 'tcx, FlowSensitiveAnalysis<'a, 'mir, 'tcx, Q>>,
in_any_value_of_ty: BitSet<Local>,
}

impl<Q: Qualif> QualifCursor<'a, 'mir, 'tcx, Q> {
pub fn new(q: Q, ccx: &'a ConstCx<'mir, 'tcx>) -> Self {
let cursor = FlowSensitiveAnalysis::new(q, ccx)
.into_engine(ccx.tcx, ccx.body, ccx.def_id)
.iterate_to_fixpoint()
.into_results_cursor(ccx.body);

let mut in_any_value_of_ty = BitSet::new_empty(ccx.body.local_decls.len());
for (local, decl) in ccx.body.local_decls.iter_enumerated() {
if Q::in_any_value_of_ty(ccx, decl.ty) {
in_any_value_of_ty.insert(local);
}
}
type QualifResults<'mir, 'tcx, Q> =
dataflow::ResultsCursor<'mir, 'tcx, FlowSensitiveAnalysis<'mir, 'mir, 'tcx, Q>>;

QualifCursor { cursor, in_any_value_of_ty }
}
#[derive(Default)]
pub struct Qualifs<'mir, 'tcx> {
has_mut_interior: Option<QualifResults<'mir, 'tcx, HasMutInterior>>,
needs_drop: Option<QualifResults<'mir, 'tcx, NeedsDrop>>,
indirectly_mutable: Option<IndirectlyMutableResults<'mir, 'tcx>>,
}

pub struct Qualifs<'a, 'mir, 'tcx> {
has_mut_interior: QualifCursor<'a, 'mir, 'tcx, HasMutInterior>,
needs_drop: QualifCursor<'a, 'mir, 'tcx, NeedsDrop>,
indirectly_mutable: IndirectlyMutableResults<'mir, 'tcx>,
}

impl Qualifs<'a, 'mir, 'tcx> {
fn indirectly_mutable(&mut self, local: Local, location: Location) -> bool {
self.indirectly_mutable.seek_before(location);
self.indirectly_mutable.get().contains(local)
impl Qualifs<'mir, 'tcx> {
fn indirectly_mutable(
&mut self,
ccx: &'mir ConstCx<'mir, 'tcx>,
local: Local,
location: Location,
) -> bool {
let indirectly_mutable = self.indirectly_mutable.get_or_insert_with(|| {
let ConstCx { tcx, body, def_id, param_env, .. } = *ccx;

// We can use `unsound_ignore_borrow_on_drop` here because custom drop impls are not
// allowed in a const.
//
// FIXME(ecstaticmorse): Someday we want to allow custom drop impls. How do we do this
Copy link
Contributor

Choose a reason for hiding this comment

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

we can lower Drop terminators for types that have a const Drop bound to a function call before doing this analysis.

// without breaking stable code?
MaybeMutBorrowedLocals::mut_borrows_only(tcx, &body, param_env)
.unsound_ignore_borrow_on_drop()
.into_engine(tcx, &body, def_id)
.iterate_to_fixpoint()
.into_results_cursor(&body)
});

indirectly_mutable.seek_before(location);
indirectly_mutable.get().contains(local)
}

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

self.needs_drop.cursor.seek_before(location);
self.needs_drop.cursor.get().contains(local) || self.indirectly_mutable(local, location)
let needs_drop = self.needs_drop.get_or_insert_with(|| {
let ConstCx { tcx, body, def_id, .. } = *ccx;

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

needs_drop.seek_before(location);
needs_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.
fn has_mut_interior(&mut self, local: Local, location: Location) -> bool {
if !self.has_mut_interior.in_any_value_of_ty.contains(local) {
fn has_mut_interior(
&mut self,
ccx: &'mir ConstCx<'mir, 'tcx>,
local: Local,
location: Location,
) -> bool {
let ty = ccx.body.local_decls[local].ty;
if !HasMutInterior::in_any_value_of_ty(ccx, ty) {
return false;
}

self.has_mut_interior.cursor.seek_before(location);
self.has_mut_interior.cursor.get().contains(local)
|| self.indirectly_mutable(local, location)
let has_mut_interior = self.has_mut_interior.get_or_insert_with(|| {
let ConstCx { tcx, body, def_id, .. } = *ccx;

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

has_mut_interior.seek_before(location);
has_mut_interior.get().contains(local) || self.indirectly_mutable(ccx, local, location)
}

fn in_return_place(&mut self, ccx: &ConstCx<'_, 'tcx>) -> ConstQualifs {
fn in_return_place(&mut self, ccx: &'mir ConstCx<'mir, 'tcx>) -> ConstQualifs {
// Find the `Return` terminator if one exists.
//
// If no `Return` terminator exists, this MIR is divergent. Just return the conservative
Expand All @@ -114,49 +143,31 @@ impl Qualifs<'a, 'mir, 'tcx> {
let return_loc = ccx.body.terminator_loc(return_block);

ConstQualifs {
needs_drop: self.needs_drop(RETURN_PLACE, return_loc),
has_mut_interior: self.has_mut_interior(RETURN_PLACE, return_loc),
needs_drop: self.needs_drop(ccx, RETURN_PLACE, return_loc),
has_mut_interior: self.has_mut_interior(ccx, RETURN_PLACE, return_loc),
}
}
}

pub struct Validator<'a, 'mir, 'tcx> {
ccx: &'a ConstCx<'mir, 'tcx>,
qualifs: Qualifs<'a, 'mir, 'tcx>,
pub struct Validator<'mir, 'tcx> {
ccx: &'mir ConstCx<'mir, 'tcx>,
qualifs: Qualifs<'mir, 'tcx>,

/// The span of the current statement.
span: Span,
}

impl Deref for Validator<'_, 'mir, 'tcx> {
impl Deref for Validator<'mir, 'tcx> {
type Target = ConstCx<'mir, 'tcx>;

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

impl Validator<'a, 'mir, 'tcx> {
pub fn new(ccx: &'a ConstCx<'mir, 'tcx>) -> Self {
let ConstCx { tcx, body, def_id, param_env, .. } = *ccx;

let needs_drop = QualifCursor::new(NeedsDrop, ccx);
let has_mut_interior = QualifCursor::new(HasMutInterior, ccx);

// We can use `unsound_ignore_borrow_on_drop` here because custom drop impls are not
// allowed in a const.
//
// FIXME(ecstaticmorse): Someday we want to allow custom drop impls. How do we do this
// without breaking stable code?
let indirectly_mutable = MaybeMutBorrowedLocals::mut_borrows_only(tcx, body, param_env)
.unsound_ignore_borrow_on_drop()
.into_engine(tcx, body, def_id)
.iterate_to_fixpoint()
.into_results_cursor(body);

let qualifs = Qualifs { needs_drop, has_mut_interior, indirectly_mutable };

Validator { span: ccx.body.span, ccx, qualifs }
impl Validator<'mir, 'tcx> {
pub fn new(ccx: &'mir ConstCx<'mir, 'tcx>) -> Self {
Validator { span: ccx.body.span, ccx, qualifs: Default::default() }
}

pub fn check_body(&mut self) {
Expand Down Expand Up @@ -239,7 +250,7 @@ impl Validator<'a, 'mir, 'tcx> {
}
}

impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> {
impl Visitor<'tcx> for Validator<'mir, 'tcx> {
fn visit_basic_block_data(&mut self, bb: BasicBlock, block: &BasicBlockData<'tcx>) {
trace!("visit_basic_block_data: bb={:?} is_cleanup={:?}", bb, block.is_cleanup);

Expand Down Expand Up @@ -345,7 +356,7 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> {
| Rvalue::AddressOf(Mutability::Not, ref place) => {
let borrowed_place_has_mut_interior = qualifs::in_place::<HasMutInterior, _>(
&self.ccx,
&mut |local| self.qualifs.has_mut_interior(local, location),
&mut |local| self.qualifs.has_mut_interior(self.ccx, local, location),
place.as_ref(),
);

Expand Down Expand Up @@ -571,7 +582,7 @@ impl Visitor<'tcx> for Validator<'_, 'mir, 'tcx> {
let needs_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(local, location)
self.qualifs.needs_drop(self.ccx, local, location)
} else {
true
};
Expand Down
21 changes: 16 additions & 5 deletions src/test/ui/issues/issue-17252.stderr
Original file line number Diff line number Diff line change
@@ -1,11 +1,22 @@
error[E0391]: cycle detected when const checking `FOO`
--> $DIR/issue-17252.rs:1:20
error[E0391]: cycle detected when normalizing `FOO`
|
note: ...which requires const-evaluating + checking `FOO`...
--> $DIR/issue-17252.rs:1:1
|
LL | const FOO: usize = FOO;
| ^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires const-evaluating + checking `FOO`...
--> $DIR/issue-17252.rs:1:1
|
LL | const FOO: usize = FOO;
| ^^^
| ^^^^^^^^^^^^^^^^^^^^^^^
note: ...which requires const-evaluating `FOO`...
--> $DIR/issue-17252.rs:1:1
|
= note: ...which again requires const checking `FOO`, completing the cycle
note: cycle used when const checking `main::{{constant}}#0`
LL | const FOO: usize = FOO;
| ^^^^^^^^^^^^^^^^^^^^^^^
= note: ...which again requires normalizing `FOO`, completing the cycle
note: cycle used when const-evaluating `main::{{constant}}#0`
--> $DIR/issue-17252.rs:4:18
|
LL | let _x: [u8; FOO]; // caused stack overflow prior to fix
Expand Down
17 changes: 14 additions & 3 deletions src/test/ui/issues/issue-23302-1.stderr
Original file line number Diff line number Diff line change
@@ -1,15 +1,26 @@
error[E0391]: cycle detected when const checking `X::A::{{constant}}#0`
error[E0391]: cycle detected when const-evaluating + checking `X::A::{{constant}}#0`
--> $DIR/issue-23302-1.rs:4:9
|
LL | A = X::A as isize,
| ^^^^^^^^^^^^^
|
= note: ...which again requires const checking `X::A::{{constant}}#0`, completing the cycle
note: cycle used when processing `X::A::{{constant}}#0`
note: ...which requires const-evaluating + checking `X::A::{{constant}}#0`...
--> $DIR/issue-23302-1.rs:4:9
|
LL | A = X::A as isize,
| ^^^^^^^^^^^^^
note: ...which requires const-evaluating `X::A::{{constant}}#0`...
--> $DIR/issue-23302-1.rs:4:9
|
LL | A = X::A as isize,
| ^^^^^^^^^^^^^
= note: ...which requires normalizing `X::A as isize`...
= note: ...which again requires const-evaluating + checking `X::A::{{constant}}#0`, completing the cycle
note: cycle used when collecting item types in top-level module
--> $DIR/issue-23302-1.rs:3:1
|
LL | enum X {
| ^^^^^^

error: aborting due to previous error

Expand Down
17 changes: 14 additions & 3 deletions src/test/ui/issues/issue-23302-2.stderr
Original file line number Diff line number Diff line change
@@ -1,15 +1,26 @@
error[E0391]: cycle detected when const checking `Y::A::{{constant}}#0`
error[E0391]: cycle detected when const-evaluating + checking `Y::A::{{constant}}#0`
--> $DIR/issue-23302-2.rs:4:9
|
LL | A = Y::B as isize,
| ^^^^^^^^^^^^^
|
= note: ...which again requires const checking `Y::A::{{constant}}#0`, completing the cycle
note: cycle used when processing `Y::A::{{constant}}#0`
note: ...which requires const-evaluating + checking `Y::A::{{constant}}#0`...
--> $DIR/issue-23302-2.rs:4:9
|
LL | A = Y::B as isize,
| ^^^^^^^^^^^^^
note: ...which requires const-evaluating `Y::A::{{constant}}#0`...
--> $DIR/issue-23302-2.rs:4:9
|
LL | A = Y::B as isize,
| ^^^^^^^^^^^^^
= note: ...which requires normalizing `Y::B as isize`...
= note: ...which again requires const-evaluating + checking `Y::A::{{constant}}#0`, completing the cycle
note: cycle used when collecting item types in top-level module
--> $DIR/issue-23302-2.rs:3:1
|
LL | enum Y {
| ^^^^^^

error: aborting due to previous error

Expand Down
36 changes: 27 additions & 9 deletions src/test/ui/issues/issue-23302-3.stderr
Original file line number Diff line number Diff line change
@@ -1,20 +1,38 @@
error[E0391]: cycle detected when const checking `A`
--> $DIR/issue-23302-3.rs:1:16
error[E0391]: cycle detected when const-evaluating + checking `A`
--> $DIR/issue-23302-3.rs:1:1
|
LL | const A: i32 = B;
| ^
| ^^^^^^^^^^^^^^^^^
|
note: ...which requires const checking `B`...
--> $DIR/issue-23302-3.rs:3:16
note: ...which requires const-evaluating + checking `A`...
--> $DIR/issue-23302-3.rs:1:1
|
LL | const B: i32 = A;
| ^
= note: ...which again requires const checking `A`, completing the cycle
note: cycle used when processing `A`
LL | const A: i32 = B;
| ^^^^^^^^^^^^^^^^^
note: ...which requires const-evaluating `A`...
--> $DIR/issue-23302-3.rs:1:1
|
LL | const A: i32 = B;
| ^^^^^^^^^^^^^^^^^
= note: ...which requires normalizing `B`...
note: ...which requires const-evaluating + checking `B`...
--> $DIR/issue-23302-3.rs:3:1
|
LL | const B: i32 = A;
| ^^^^^^^^^^^^^^^^^
note: ...which requires const-evaluating + checking `B`...
--> $DIR/issue-23302-3.rs:3:1
|
LL | const B: i32 = A;
| ^^^^^^^^^^^^^^^^^
note: ...which requires const-evaluating `B`...
--> $DIR/issue-23302-3.rs:3:1
|
LL | const B: i32 = A;
| ^^^^^^^^^^^^^^^^^
= note: ...which requires normalizing `A`...
= note: ...which again requires const-evaluating + checking `A`, completing the cycle
= note: cycle used when running analysis passes on this crate

error: aborting due to previous error

Expand Down
Loading