-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Change const eval to just return the value #69181
Changes from all commits
c423a86
c7aadcf
99a864b
821f4a9
774a029
8904bdd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -756,6 +756,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { | |
pub(super) fn const_eval( | ||
&self, | ||
gid: GlobalId<'tcx>, | ||
ty: Ty<'tcx>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So with this, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is pretty much down to lifetime parametricity/polymorphism. And because lifetimes shouldn't affect const-evaluation, and to maximize caching, we want to erase them, but we also don't want the lifetime-erased type to leak back into e.g. borrow-checking. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes me think it would be nice to provide a "lifetime substitution" instead of the full type (and compute the non-lifetime parts from cc @nikomatsakis (who I'm sure has had this idea before) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Miri doesn't care about lifetimes though. I am just worried about the additional failure mode this introduces -- generally in Miri we track the type of values so when someone uses e.g. an i32 as i64 we get an ICE (and this has caught quite a few bugs). But here we just believe the type the caller tells us. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The caller does. Or is
If this is not internal, and this matters, we could assert that the type is identical after erasing the one that might have lifetimes. If it is internal, then yeah, using whatever lifetime-erased type can be computed from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, that is exactly why I asked for the query impls to be moved out of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One call site doesn't really have the type available... the one where you used There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will change that call site to actually use the proper type, which as it's for intrinsics the type of them are known. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Skinny121 did you ever get around to actually do this? Doesn't look like it, but I might have missed it. |
||
) -> InterpResult<'tcx, OpTy<'tcx, M::PointerTag>> { | ||
// For statics we pick `ParamEnv::reveal_all`, because statics don't have generics | ||
// and thus don't care about the parameter environment. While we could just use | ||
|
@@ -777,7 +778,8 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { | |
// recursion deeper than one level, because the `tcx.const_eval` above is guaranteed to not | ||
// return `ConstValue::Unevaluated`, which is the only way that `eval_const_to_op` will call | ||
// `ecx.const_eval`. | ||
self.eval_const_to_op(val, None) | ||
let const_ = ty::Const { val: ty::ConstKind::Value(val), ty }; | ||
self.eval_const_to_op(&const_, None) | ||
} | ||
|
||
pub fn const_eval_raw( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there no caveats here? I remember @eddyb saying that "reveal all" is often not the right choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lately I've had to say almost the exact opposite (because some misunderstanding around
Reveal::All
's ability to cope with polymorphic situations: modulo bugs, it should always work), so I'm not sure what the context was for what you're thinking of.Note that
layout_of
, IIRC, useswith_reveal_all
internally, so if this does anything, it avoids extra queries.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the same logic as
Const::try_eval_bits
.