Change const eval to just return the value #69181
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ype inside it shouldn't be used.
…e` instead of `Const` as the type in the returned const isn't needed.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
eddyb
reviewed
Feb 15, 2020
| ConstValue::Scalar(Scalar::Ptr(ptr)) => collect_miri(tcx, ptr.alloc_id, output), | ||
| ConstValue::Slice { data: alloc, start: _, end: _ } | ConstValue::ByRef { alloc, .. } => { | ||
| for &((), id) in alloc.relocations().values() { | ||
| collect_miri(tcx, id, output); |
There was a problem hiding this comment.
Unrelated, but collect_miri should probably be called collect_miri_alloc.
RalfJung
reviewed
Feb 15, 2020
| param_env: ParamEnv<'tcx>, | ||
| ty: Ty<'tcx>, | ||
| ) -> Option<u128> { | ||
| let size = tcx.layout_of(param_env.with_reveal_all().and(ty)).ok()?.size; |
There was a problem hiding this comment.
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.
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, uses with_reveal_all internally, so if this does anything, it avoids extra queries.
There was a problem hiding this comment.
I used the same logic as Const::try_eval_bits.
RalfJung
reviewed
Feb 15, 2020
RalfJung
reviewed
Feb 15, 2020
| @@ -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.
So with this, the gid is not sufficient to determine the type of the constant it identifies? That seems somewhat odd to me, given that it is enough to determine the value.
There was a problem hiding this comment.
This is pretty much down to lifetime parametricity/polymorphism.
Basically the entire type is determinable except lifetimes.
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.
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 GlobalId), but that might need a lot more infrastructure than this PR (in general we might want lifetimes slots to be placeholders for e.g. MIR borrowck to work with).
cc @nikomatsakis (who I'm sure has had this idea before)
There was a problem hiding this comment.
Basically the entire type is determinable except lifetimes.
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.
Miri doesn't care about lifetimes though.
The caller does. Or is const_eval an internal API? (It's sometimes hard to tell on GitHub).
But here we just believe the type the caller tells us.
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 GlobalId would be fine.
There was a problem hiding this comment.
Indeed, that is exactly why I asked for the query impls to be moved out of rustc_mir::interp way back when.
There was a problem hiding this comment.
The gid should be sufficient to determine the type, at least a type that is good enough for miri, though getting the type of a promoted globals seems like it would be a little bit difficult. I decided as this is an internal API and as both call sites has the type available, it would just be easier to pass it through.
There was a problem hiding this comment.
One call site doesn't really have the type available... the one where you used dest.layout.ty, previously this was checking that the type of the constant was the same as the type of the place it was put into. Now there is no check any more, you are just assuming this to be the case.
There was a problem hiding this comment.
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.
@Skinny121 did you ever get around to actually do this? Doesn't look like it, but I might have missed it.
oli-obk
reviewed
Feb 15, 2020
| @@ -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.
Yes, this is mir interp internal. Rustc never calls this manually
oli-obk
reviewed
Feb 15, 2020
src/librustdoc/clean/utils.rs
Outdated
| @@ -493,8 +493,8 @@ pub fn print_evaluated_const(cx: &DocContext<'_>, def_id: DefId) -> Option<Strin | |||
| (_, &ty::Ref(..)) => None, | |||
| (ConstValue::Scalar(_), &ty::Adt(_, _)) => None, | |||
| (ConstValue::Scalar(_), _) => { | |||
| let const_ = ty::Const { val: ty::ConstKind::Value(val), ty }; | |||
There was a problem hiding this comment.
Here you can keep the original, as the value isbnot required to be interned
There was a problem hiding this comment.
I think it actually needs to be interned to be printed, I get the 'could not lift for printing' otherwise.
|
@bors r+ |
|
📌 Commit 8904bdd has been approved by |
bors
added
the
S-waiting-on-bors
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Feb 18, 2020
Change const eval to just return the value As discussed in rust-lang#68505 (comment), the type of consts shouldn't be returned from const eval queries. r? @eddyb cc @nikomatsakis
JohnTitor
added a commit
to JohnTitor/rust-clippy
that referenced
this pull request
Feb 18, 2020
bors
added a commit
to rust-lang/rust-clippy
that referenced
this pull request
Feb 18, 2020
Rustup to rust-lang/rust#69181 changelog: none
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Feb 19, 2020
Changes: ```` Rustup to rust-lang#69194 Rustup to rust-lang#69181 Add `LOG2_10` and `LOG10_2` to `approx_const` lint Clean up imports Use `Vec::with_capacity()` as possible needless_doctest_main: False positive for async fn Remove use of `TyKind`. Use `if_chain`. Fix ICE. Add tests and improve checks. Add `Future` detection for `missing_errors_doc`. ```` Fixes rust-lang#69269
bors
added a commit
that referenced
this pull request
Feb 19, 2020
submodules: update clippy from b91ae16 to 2855b21 Changes: ```` Rustup to #69194 Rustup to #69181 Add `LOG2_10` and `LOG10_2` to `approx_const` lint Clean up imports Use `Vec::with_capacity()` as possible needless_doctest_main: False positive for async fn Remove use of `TyKind`. Use `if_chain`. Fix ICE. Add tests and improve checks. Add `Future` detection for `missing_errors_doc`. ```` Fixes #69269
RalfJung
reviewed
Apr 9, 2020
| @@ -536,7 +536,7 @@ impl<'mir, 'tcx, M: Machine<'mir, 'tcx>> InterpCx<'mir, 'tcx, M> { | |||
| // potentially requiring the current static to be evaluated again. This is not a | |||
| // problem here, because we are building an operand which means an actual read is | |||
| // happening. | |||
| return Ok(OpTy::from(self.const_eval(GlobalId { instance, promoted })?)); | |||
| return Ok(self.const_eval(GlobalId { instance, promoted }, val.ty)?); | |||
There was a problem hiding this comment.
Curiously, this is now using the use-site type instead of the def-site type, which leads to changing the way we ICE in #70942 (we should be ICEing earlier, but after this PR we only ICE very late).
There was a problem hiding this comment.
The bug is argably unsound Instance::resolve behavior, as the mistyped associated const in the impl is resolved, despite being the wrong type.
There was a problem hiding this comment.
Hmm, in an Unevaluated(def_id, substs, None), tcx.type_of(def_id).subst(tcx, substs) should match the type of the constant, right? Maybe if Instance::resolve returns a different DefId (i.e. of an associated const in an impl), we can take the type from there?
I'd say "we should require the type is the same between trait and impl" but I'm worried that is too strict, given how the type-checking requirement works (it's using an inference context).
Oh, but unlike an associated fn, an associated const can't have its own generics, so maybe there's no case in which the types would be equal.
We can use delay_span_bug + returning unnormalized constant (or even tcx.consts.err).
There was a problem hiding this comment.
I keep forgetting things to throw in here, oops! I meant to say that I will try to prepare a patch.
There was a problem hiding this comment.
I was about to ask for clarification, but I think a patch is indeed the best kind of clarification. ;)
There was a problem hiding this comment.
Ended up doing it in Instance::resolve outright: #70970.
flip1995
pushed a commit
to flip1995/rust-clippy
that referenced
this pull request
May 5, 2020
Changes: ```` Rustup to rust-lang/rust#69194 Rustup to rust-lang/rust#69181 Add `LOG2_10` and `LOG10_2` to `approx_const` lint Clean up imports Use `Vec::with_capacity()` as possible needless_doctest_main: False positive for async fn Remove use of `TyKind`. Use `if_chain`. Fix ICE. Add tests and improve checks. Add `Future` detection for `missing_errors_doc`. ```` Fixes #69269
16 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As discussed in #68505 (comment), the type of consts shouldn't be returned from const eval queries.
r? @eddyb
cc @nikomatsakis