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

Fix an ICE encountered in clippy that will be possible to trigger in rustc in the future #61041

Closed
wants to merge 23 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
16911bd
Fix an ICE encountered in clippy integration tests
oli-obk May 22, 2019
d38fb10
A comment mentioned the wrong function
oli-obk Jul 28, 2019
b535090
Typo
oli-obk Jul 28, 2019
e849b83
Document all the things
oli-obk Jul 28, 2019
600d679
WIP: Unbrittle the miri engine around unsubstituted values
oli-obk Jul 28, 2019
27320e3
Privatize `subst_and_normalize_erasing_regions`
oli-obk Jul 28, 2019
b84ed30
Clarify doc comments some more
oli-obk Jul 28, 2019
7fac3a1
Simplify `subst_and_normalize_erasing_regions`
oli-obk Jul 28, 2019
8ac4cb5
Fix incremental test
oli-obk Jul 29, 2019
fe2013c
Substitute const generics so we can evaluate them
oli-obk Jul 29, 2019
1d26fda
Explain partial substitution
oli-obk Jul 31, 2019
b917c43
Abort evaluation when substuting a value still requires more substitu…
oli-obk Aug 1, 2019
5a8c9d3
Substituting with identity substs is equivalent to not substituting a…
oli-obk Aug 12, 2019
faa342f
Reintroduce sanity assert
oli-obk Aug 12, 2019
51e1343
Remove now-useless impl
oli-obk Aug 12, 2019
034939b
Unbreak a line
oli-obk Aug 12, 2019
685e0e5
Unbreak more lines
oli-obk Aug 12, 2019
fdf5450
Address review comments and unbreak more lines
oli-obk Aug 12, 2019
3afcb1e
Adjust incremental test
oli-obk Aug 12, 2019
f0a16c4
Address review comments
oli-obk Aug 12, 2019
3aa550a
We don't have polymorphic things without a stackframe anymore
oli-obk Aug 12, 2019
82db274
Only monomorphize things from the current frame's MIR, not arbitrary …
oli-obk Aug 12, 2019
70124ec
Work around a MIR bug
oli-obk Aug 12, 2019
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
4 changes: 2 additions & 2 deletions src/librustc_mir/const_eval.rs
Original file line number Diff line number Diff line change
Expand Up @@ -505,7 +505,7 @@ pub fn const_field<'tcx>(
trace!("const_field: {:?}, {:?}", field, value);
let ecx = mk_eval_cx(tcx, DUMMY_SP, param_env);
// get the operand again
let op = ecx.eval_const_to_op(value, None).unwrap();
let op = ecx.eval_const_to_op(value.val, value.ty, None).unwrap();
// downcast
let down = match variant {
None => op,
Expand All @@ -527,7 +527,7 @@ pub fn const_variant_index<'tcx>(
) -> VariantIdx {
trace!("const_variant_index: {:?}", val);
let ecx = mk_eval_cx(tcx, DUMMY_SP, param_env);
let op = ecx.eval_const_to_op(val, None).unwrap();
let op = ecx.eval_const_to_op(val.val, val.ty, None).unwrap();
ecx.read_discriminant(op).unwrap().1
}

Expand Down
1 change: 0 additions & 1 deletion src/librustc_mir/interpret/cast.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// The src operand does not matter, just its type
match src.layout.ty.sty {
ty::Closure(def_id, substs) => {
let substs = self.subst_and_normalize_erasing_regions(substs)?;
let instance = ty::Instance::resolve_closure(
*self.tcx,
def_id,
Expand Down
81 changes: 33 additions & 48 deletions src/librustc_mir/interpret/eval_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use rustc::mir;
use rustc::ty::layout::{
self, Size, Align, HasDataLayout, LayoutOf, TyLayout
};
use rustc::ty::subst::{Subst, SubstsRef};
use rustc::ty::subst::{SubstsRef, Subst};
use rustc::ty::{self, Ty, TyCtxt, TypeFoldable};
use rustc::ty::query::TyCtxtAt;
use rustc_data_structures::indexed_vec::IndexVec;
Expand Down Expand Up @@ -291,22 +291,38 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
ty.is_freeze(*self.tcx, self.param_env, DUMMY_SP)
}

pub(super) fn subst_and_normalize_erasing_regions<T: TypeFoldable<'tcx>>(
/// Call this whenever you have a value that you took from the current frame's `mir::Body`.
/// Not guaranteed to actually monomorphize the value.
/// If we are e.g. const propagating inside a generic function, some
/// things may depend on a generic parameter and thus can never be monomorphized.
pub(super) fn subst_and_normalize_erasing_regions_in_frame<T: TypeFoldable<'tcx>>(
Copy link
Member

Choose a reason for hiding this comment

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

@RalfJung came up with subst_from_frame_and_normalize_erasing_regions, I kinda like that more?

Copy link
Member

Choose a reason for hiding this comment

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

In particular this finally made it click for me in terms of: we need this when taking something from the current frame to move it into the InterpCx "universe".

Also see the comments I wrote for my own experiments here and here; I think it would be useful to carry those over.

&self,
substs: T,
value: T,
) -> InterpResult<'tcx, T> {
match self.stack.last() {
Some(frame) => Ok(self.tcx.subst_and_normalize_erasing_regions(
frame.instance.substs,
self.param_env,
&substs,
)),
None => if substs.needs_subst() {
throw_inval!(TooGeneric)
} else {
Ok(substs)
},
self.subst_and_normalize_erasing_regions(self.frame().instance.substs, value)
}

/// Same thing as `subst_and_normalize_erasing_regions_in_frame` but not taking its substs
/// from the top stack frame, but requiring you to pass specific substs.
///
/// Only call this function if you want to apply the substs of a specific frame (that is
/// definitely not the frame currently being evaluated). You need to make sure to pass correct
/// substs.
fn subst_and_normalize_erasing_regions<T: TypeFoldable<'tcx>>(
Copy link
Member

@eddyb eddyb Aug 12, 2019

Choose a reason for hiding this comment

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

This function shouldn't be needed (as a separate thing).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's needed for layout_of_local

Copy link
Member

Choose a reason for hiding this comment

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

I'd inline it there.

&self,
param_substs: SubstsRef<'tcx>,
value: T,
) -> InterpResult<'tcx, T> {
let substituted = value.subst(self.tcx.tcx, param_substs);
// we duplicate the body of `TyCtxt::subst_and_normalize_erasing_regions` here, because
// we can't normalize values with generic parameters. The difference between this function
// and the `TyCtxt` version is this early abort
if substituted.needs_subst() {
// FIXME(oli-obk): This aborts evaluating `fn foo<T>() -> i32 { 42 }` inside an
// associated constant of a generic trait, even though that should be doable.
throw_inval!(TooGeneric);
Copy link
Member

Choose a reason for hiding this comment

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

Does anything break if you remove this?

}
Ok(self.tcx.normalize_erasing_regions(self.param_env, substituted))
}

pub(super) fn resolve(
Expand All @@ -315,9 +331,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
substs: SubstsRef<'tcx>
) -> InterpResult<'tcx, ty::Instance<'tcx>> {
trace!("resolve: {:?}, {:#?}", def_id, substs);
trace!("param_env: {:#?}", self.param_env);
let substs = self.subst_and_normalize_erasing_regions(substs)?;
trace!("substs: {:#?}", substs);
ty::Instance::resolve(
*self.tcx,
self.param_env,
Expand Down Expand Up @@ -349,36 +362,6 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
}
}

pub(super) fn monomorphize<T: TypeFoldable<'tcx> + Subst<'tcx>>(
&self,
t: T,
) -> InterpResult<'tcx, T> {
match self.stack.last() {
Some(frame) => Ok(self.monomorphize_with_substs(t, frame.instance.substs)?),
None => if t.needs_subst() {
throw_inval!(TooGeneric)
} else {
Ok(t)
},
}
}

fn monomorphize_with_substs<T: TypeFoldable<'tcx> + Subst<'tcx>>(
&self,
t: T,
substs: SubstsRef<'tcx>
) -> InterpResult<'tcx, T> {
// miri doesn't care about lifetimes, and will choke on some crazy ones
// let's simply get rid of them
let substituted = t.subst(*self.tcx, substs);

if substituted.needs_subst() {
throw_inval!(TooGeneric)
}

Ok(self.tcx.normalize_erasing_regions(ty::ParamEnv::reveal_all(), substituted))
}

pub fn layout_of_local(
&self,
frame: &Frame<'mir, 'tcx, M::PointerTag, M::FrameExtra>,
Expand All @@ -391,7 +374,9 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
None => {
let layout = crate::interpret::operand::from_known_layout(layout, || {
let local_ty = frame.body.local_decls[local].ty;
let local_ty = self.monomorphize_with_substs(local_ty, frame.instance.substs)?;
let local_ty = self.subst_and_normalize_erasing_regions(
frame.instance.substs, local_ty,
)?;
self.layout_of(local_ty)
})?;
if let Some(state) = frame.locals.get(local) {
Expand Down
25 changes: 13 additions & 12 deletions src/librustc_mir/interpret/operand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

use std::convert::TryInto;

use rustc::{mir, ty};
use rustc::{mir, ty::{self, Ty}};
use rustc::ty::layout::{
self, Size, LayoutOf, TyLayout, HasDataLayout, IntegerExt, VariantIdx,
};
Expand Down Expand Up @@ -511,7 +511,11 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
Move(ref place) =>
self.eval_place_to_op(place, layout)?,

Constant(ref constant) => self.eval_const_to_op(constant.literal, layout)?,
Constant(ref constant) => {
let val = self.subst_and_normalize_erasing_regions_in_frame(constant.literal.val)?;
let ty = self.subst_and_normalize_erasing_regions_in_frame(constant.ty)?;
self.eval_const_to_op(val, ty, layout)?
},
};
trace!("{:?}: {:?}", mir_op, *op);
Ok(op)
Expand All @@ -531,18 +535,17 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
// in patterns via the `const_eval` module
crate fn eval_const_to_op(
&self,
val: &'tcx ty::Const<'tcx>,
value: ConstValue<'tcx>,
ty: Ty<'tcx>,
Copy link
Member

Choose a reason for hiding this comment

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

Please add a HACK/FIXME comment. Or even better, wait for #56137 to be fixed.

Copy link
Member

Choose a reason for hiding this comment

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

That is, rebase this on top of #63495.

layout: Option<TyLayout<'tcx>>,
) -> InterpResult<'tcx, OpTy<'tcx, M::PointerTag>> {
let tag_scalar = |scalar| match scalar {
Scalar::Ptr(ptr) => Scalar::Ptr(self.tag_static_base_pointer(ptr)),
Scalar::Raw { data, size } => Scalar::Raw { data, size },
};
// Early-return cases.
match val.val {
ConstValue::Param(_) =>
// FIXME(oli-obk): try to monomorphize
throw_inval!(TooGeneric),
match value {
ConstValue::Param(_) => throw_inval!(TooGeneric),
ConstValue::Unevaluated(def_id, substs) => {
let instance = self.resolve(def_id, substs)?;
return Ok(OpTy::from(self.const_eval_raw(GlobalId {
Expand All @@ -553,10 +556,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
_ => {}
}
// Other cases need layout.
let layout = from_known_layout(layout, || {
self.layout_of(self.monomorphize(val.ty)?)
})?;
let op = match val.val {
let layout = from_known_layout(layout, || self.layout_of(ty))?;
let op = match value {
ConstValue::ByRef { alloc, offset } => {
let id = self.tcx.alloc_map.lock().create_memory_alloc(alloc);
// We rely on mutability being set correctly in that allocation to prevent writes
Expand All @@ -583,7 +584,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
ConstValue::Infer(..) |
ConstValue::Placeholder(..) |
ConstValue::Unevaluated(..) =>
bug!("eval_const_to_op: Unexpected ConstValue {:?}", val),
bug!("eval_const_to_op: Unexpected ConstValue {:?}", value),
};
Ok(OpTy { op, layout })
}
Expand Down
5 changes: 3 additions & 2 deletions src/librustc_mir/interpret/place.rs
Original file line number Diff line number Diff line change
Expand Up @@ -616,10 +616,11 @@ where
Some(return_place) => {
// We use our layout to verify our assumption; caller will validate
// their layout on return.
let ret_ty = self.frame().body.return_ty();
let ret_ty = self.subst_and_normalize_erasing_regions_in_frame(ret_ty)?;
PlaceTy {
place: *return_place,
layout: self
.layout_of(self.monomorphize(self.frame().body.return_ty())?)?,
layout: self.layout_of(ret_ty)?,
}
}
None => throw_unsup!(InvalidNullPointerUsage),
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/interpret/step.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
}

NullaryOp(mir::NullOp::SizeOf, ty) => {
let ty = self.monomorphize(ty)?;
let ty = self.subst_and_normalize_erasing_regions_in_frame(ty)?;
let layout = self.layout_of(ty)?;
assert!(!layout.is_unsized(),
"SizeOf nullary MIR operator called for unsized type");
Expand Down
2 changes: 1 addition & 1 deletion src/librustc_mir/interpret/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
for (i, method) in methods.iter().enumerate() {
if let Some((def_id, substs)) = *method {
// resolve for vtable: insert shims where needed
let substs = self.subst_and_normalize_erasing_regions(substs)?;
let substs = self.subst_and_normalize_erasing_regions_in_frame(substs)?;
let instance = ty::Instance::resolve_for_vtable(
*self.tcx,
self.param_env,
Expand Down
5 changes: 3 additions & 2 deletions src/librustc_mir/transform/const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,11 +162,12 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
let def_id = source.def_id();
let param_env = tcx.param_env(def_id);
let span = tcx.def_span(def_id);
let substs = InternalSubsts::identity_for_item(tcx, def_id);
let mut ecx = mk_eval_cx(tcx, span, param_env);
let can_const_prop = CanConstProp::check(body);

ecx.push_stack_frame(
Instance::new(def_id, &InternalSubsts::identity_for_item(tcx, def_id)),
Instance::new(def_id, &substs),
span,
dummy_body,
None,
Expand Down Expand Up @@ -286,7 +287,7 @@ impl<'mir, 'tcx> ConstPropagator<'mir, 'tcx> {
c: &Constant<'tcx>,
) -> Option<Const<'tcx>> {
self.ecx.tcx.span = c.span;
match self.ecx.eval_const_to_op(c.literal, None) {
match self.ecx.eval_const_to_op(c.literal.val, c.literal.ty, None) {
Ok(op) => {
Some(op)
},
Expand Down
10 changes: 10 additions & 0 deletions src/test/incremental/no_mangle2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// revisions:cfail1 cfail2
// check-pass
// compile-flags: --crate-type staticlib

#![deny(unused_attributes)]

#[no_mangle]
pub extern "C" fn rust_no_mangle() -> i32 {
42
}
13 changes: 13 additions & 0 deletions src/test/ui/consts/too_generic_eval_ice.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
pub struct Foo<A, B>(A, B);

impl<A, B> Foo<A, B> {
const HOST_SIZE: usize = std::mem::size_of::<B>();

pub fn crash() -> bool {
[5; Self::HOST_SIZE] == [6; 0] //~ ERROR no associated item named `HOST_SIZE`
//~^ the size for values of type `A` cannot be known
//~| the size for values of type `B` cannot be known
}
}

fn main() {}
47 changes: 47 additions & 0 deletions src/test/ui/consts/too_generic_eval_ice.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
error[E0599]: no associated item named `HOST_SIZE` found for type `Foo<A, B>` in the current scope
--> $DIR/too_generic_eval_ice.rs:7:19
|
LL | pub struct Foo<A, B>(A, B);
| --------------------------- associated item `HOST_SIZE` not found for this
...
LL | [5; Self::HOST_SIZE] == [6; 0]
| ^^^^^^^^^ associated item not found in `Foo<A, B>`
|
= note: the method `HOST_SIZE` exists but the following trait bounds were not satisfied:
`A : std::marker::Sized`
`B : std::marker::Sized`

error[E0277]: the size for values of type `A` cannot be known at compilation time
--> $DIR/too_generic_eval_ice.rs:7:13
|
LL | [5; Self::HOST_SIZE] == [6; 0]
| ^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
|
= help: the trait `std::marker::Sized` is not implemented for `A`
= note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
= help: consider adding a `where A: std::marker::Sized` bound
note: required by `Foo`
--> $DIR/too_generic_eval_ice.rs:1:1
|
LL | pub struct Foo<A, B>(A, B);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0277]: the size for values of type `B` cannot be known at compilation time
--> $DIR/too_generic_eval_ice.rs:7:13
|
LL | [5; Self::HOST_SIZE] == [6; 0]
| ^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
|
= help: the trait `std::marker::Sized` is not implemented for `B`
= note: to learn more, visit <https://doc.rust-lang.org/book/ch19-04-advanced-types.html#dynamically-sized-types-and-the-sized-trait>
= help: consider adding a `where B: std::marker::Sized` bound
note: required by `Foo`
--> $DIR/too_generic_eval_ice.rs:1:1
|
LL | pub struct Foo<A, B>(A, B);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 3 previous errors

Some errors have detailed explanations: E0277, E0599.
For more information about an error, try `rustc --explain E0277`.
4 changes: 2 additions & 2 deletions src/test/ui/issues/issue-58435-ice-with-assoc-const.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// run-pass
// The const-evaluator was at one point ICE'ing while trying to
// evaluate the body of `fn id` during the `s.id()` call in main.
// The const propagator was at one point ICE'ing while trying to
// propagate constants in the body of `fn id` during the `s.id()` call in main.

struct S<T>(T);

Expand Down