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 2 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
7 changes: 4 additions & 3 deletions src/librustc_mir/interpret/eval_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -291,8 +291,9 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
ty.is_freeze(*self.tcx, self.param_env, DUMMY_SP)
}

/// Call this whenever you have a value that `needs_subst`. Not guaranteed to actually
/// monomorphize the value. If we are e.g. const propagating inside a generic function, some
/// 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,
Expand All @@ -304,7 +305,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
/// 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 compute the substs of a specific frame (that is
/// 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.

Expand Down
22 changes: 10 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,14 +535,14 @@ 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 },
};
let value = self.subst_and_normalize_erasing_regions_in_frame(val.val)?;
// Early-return cases.
match value {
ConstValue::Param(_) => throw_inval!(TooGeneric),
Expand All @@ -552,13 +556,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> {
_ => {}
}
// Other cases need layout.
let layout = from_known_layout(layout, || {
// Substituting is not a cached or O(1) operation. Substituting the type happens here,
// which may not happen if we already have a layout. Or if we use the early abort above.
// Thus we do not substitute and normalize `val` above, but only `val.val` and then
// substitute `val.ty` here.
self.layout_of(self.subst_and_normalize_erasing_regions_in_frame(val.ty)?)
})?;
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);
Expand Down Expand Up @@ -586,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
2 changes: 1 addition & 1 deletion src/librustc_mir/transform/const_prop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -287,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