Fix an ICE encountered in clippy that will be possible to trigger in rustc in the future #61041
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
|
r? @estebank (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
Centril
reviewed
May 22, 2019
| @@ -0,0 +1,51 @@ | |||
| error[E0601]: `main` function not found in crate `too_generic_eval_ice` | |||
There was a problem hiding this comment.
add fn main() {} to get rid of this unnecessary error?
estebank
approved these changes
May 22, 2019
eddyb
reviewed
May 22, 2019
| } | ||
| // Stack frames beyond the first stackframe have their own set of substs and it would be | ||
| // absolutely wrong to use them for any further substitutions. So we just yield the substs | ||
| // after region erasing and let the caller figure out if that is good enough for them. |
There was a problem hiding this comment.
I'm confused, why is the thing to substitute being called substs? Usually we call it something generic like value.
Also, this doesn't seem right.
What I would do, if there is any stack frame, is always call subst_and_normalize_erasing_regions and then check if that still needs_subst - because if it does, the entire execution (i.e. what self.param_env applies to) is "too generic".
There was a problem hiding this comment.
Consider the following function
fn foo<U>() -> usize {
let v: Vec<U> = Vec::new();
v.len()
}When evaluating Vec::new() e.g. due to const propagation, we'd create a stack frame for Vec::new. If the type were Vec<i32> the substs would be [i32] and we'd substitute and be monomorphic from now on. Since the type is Vec<U>, we'd substitute the T in Vec<T>::new with U (our substs are [U]). This is why we need to substitute the [T] substs of Vec<T>::new with [U].
We can preemptively return TooGeneric as you suggested, but that would prevent us from const propagating calls to foo, even though that is totally fine to evaluate even though it's still polymorphic.
There was a problem hiding this comment.
Okay then you should just never return TooGeneric, even when there are no frames?
And in your example, with the code as written, you would have [T] and not substitute it to/with [U] because you're just not calling subst_and_normalize_erasing_regions anymore in that case.
Am I missing something?
|
r? @eddyb |
|
What's the status of this? |
|
I'm still trying to figure out how it all works. @eddyb raises extremely valid points, and I need to think about how to do this. The current "fix" is not correct and will just ICE in other situations. |
|
☔ The latest upstream changes (presumably #61625) made this pull request unmergeable. Please resolve the merge conflicts. |
Dylan-DPC-zz
added
S-waiting-on-author
|
ping from triage @oli-obk any updates? |
|
I think I ran into this with RalfJung@a4fbd43: after that change, I get an ICE when building So maybe that commit could be added here, then we'll have a testcase already? |
3 tasks
|
@RalfJung Among other things that there are existing uses of |
|
Oh, that's good to know. I thought Seems like something worth tracking in the type then? |
|
@eddyb I am still seeing this ICE after removing the double-monomorphization with RalfJung@a8754d4d33. ICE backtraceThe backtrace shows that it is the first |
|
@RalfJung It can't be idempotent, because it's a substitution from one domain to another (just realized that's a better phrasing. The name What constprop should do is use identity |
|
Wait this PR is changing a function called |
eddyb
reviewed
Jul 18, 2019
eddyb
reviewed
Jul 18, 2019
| // after region erasing and let the caller figure out if that is good enough for them. | ||
| if self.stack.len() > 1 { | ||
| return Ok(self.tcx.erase_regions(&substs)); | ||
| } | ||
| match self.stack.last() { | ||
| Some(frame) => Ok(self.tcx.subst_and_normalize_erasing_regions( | ||
| frame.instance.substs, | ||
| self.param_env, | ||
| &substs, | ||
| )), |
There was a problem hiding this comment.
New rounds of reviews: I'm pretty sure everything should use at least one frame, or you should keep a "top-level substs" just like param_env`.
There was a problem hiding this comment.
I went with the top level substs, because adding a new frame is way too expensive (not just in perf but also in "useless" API complexity) in some cases. We may be able to clean that up in the future.
eddyb
reviewed
Jul 18, 2019
| } else { | ||
| Ok(substs) | ||
| }, | ||
| None => err!(TooGeneric).into(), |
There was a problem hiding this comment.
New rounds of reviews: this should never happen (see other comments as to why).
But also (most/all?) TooGeneric errors should come from LayoutError::Unknown (which you should either turn into TooGeneric or handle like TooGeneric everywhere).
There was a problem hiding this comment.
There's also the problem that if we call tcx.normalize_and_erase_regions, at some point we'll end up hitting this assertion:
normalize_and_erase_regions with an if value.needs_subst() { return err!(TooGeneric); }. I'm not sure how important that assertion is or why it's there. I checked the PR that created it, but it didn't explain the assert.
|
Ping from triage @oli-obk any updates on this? |
RalfJung
reviewed
Aug 12, 2019
| @@ -291,22 +291,37 @@ 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 `needs_subst`. Not guaranteed to actually | |||
There was a problem hiding this comment.
This advice seems wrong. It should be called whenever (and only when) you took something from the MIR of the current frame.
Maybe something like
/// Call this on things you got out of the current frame's MIR, to bring it into the proper
/// environment for this interpreter.
///
/// This is not guarantee to actually monomorphize [...]
…polymorphic things
|
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 |
|
IMO this PR should be retitled: clippy is just a coincidence, there are many things broken in miri wrt substitutions. For the record, I've started my own attempt, based on @RalfJung's take on this, at https://github.com/rust-lang/rust/compare/master...eddyb:miri-subst?expand=1 And I've hit this peculiar bug: #56137 (comment) EDIT: opened #63495 to fix that bug. |
eddyb
reviewed
Aug 12, 2019
| @@ -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>, | |||
Centril
added a commit
to Centril/rust
that referenced
this pull request
Aug 14, 2019
Remove redundant `ty` fields from `mir::Constant` and `hair::pattern::PatternRange`. Fixes rust-lang#56137. As a side-effect, associated const literals have the correct type now, which should make things easier for rust-lang#61041. r? @oli-obk / @matthewjasper cc @davidtwco @varkor
Centril
added a commit
to Centril/rust
that referenced
this pull request
Aug 14, 2019
Remove redundant `ty` fields from `mir::Constant` and `hair::pattern::PatternRange`. Fixes rust-lang#56137. As a side-effect, associated const literals have the correct type now, which should make things easier for rust-lang#61041. r? @oli-obk / @matthewjasper cc @davidtwco @varkor
Centril
added a commit
to Centril/rust
that referenced
this pull request
Aug 14, 2019
Remove redundant `ty` fields from `mir::Constant` and `hair::pattern::PatternRange`. Fixes rust-lang#56137. As a side-effect, associated const literals have the correct type now, which should make things easier for rust-lang#61041. r? @oli-obk / @matthewjasper cc @davidtwco @varkor
Centril
added a commit
to Centril/rust
that referenced
this pull request
Aug 14, 2019
Remove redundant `ty` fields from `mir::Constant` and `hair::pattern::PatternRange`. Fixes rust-lang#56137. As a side-effect, associated const literals have the correct type now, which should make things easier for rust-lang#61041. r? @oli-obk / @matthewjasper cc @davidtwco @varkor
Centril
added a commit
to Centril/rust
that referenced
this pull request
Aug 15, 2019
Remove redundant `ty` fields from `mir::Constant` and `hair::pattern::PatternRange`. Fixes rust-lang#56137. As a side-effect, associated const literals have the correct type now, which should make things easier for rust-lang#61041. r? @oli-obk / @matthewjasper cc @davidtwco @varkor
Centril
added a commit
to Centril/rust
that referenced
this pull request
Aug 16, 2019
Remove redundant `ty` fields from `mir::Constant` and `hair::pattern::PatternRange`. Fixes rust-lang#56137. As a side-effect, associated const literals have the correct type now, which should make things easier for rust-lang#61041. r? @oli-obk / @matthewjasper cc @davidtwco @varkor
eddyb
added
S-waiting-on-author
|
(waiting on @oli-obk to deduplicate this with my own PR) |
|
I think this PR will fix #61432, maybe you can create a test case for it. |
eddyb
added a commit
to eddyb/rust
that referenced
this pull request
Aug 19, 2019
Centril
added a commit
to Centril/rust
that referenced
this pull request
Aug 20, 2019
rustc_mir: fix miri substitution/"universe" discipline. Alternative to rust-lang#61041, based on @RalfJung's own attempt at it. I haven't done a full audit, but I believe everything is fixed now. Fixes rust-lang#61432. Closes rust-lang#61336, as a drive-by fix (for a subset of rust-lang#43408, that is already special-cased). r? @oli-obk / @RalfJung cc @varkor @yodaldevoid
bors
added a commit
that referenced
this pull request
Aug 20, 2019
rustc_mir: fix miri substitution/"universe" discipline. Alternative to #61041, based on @RalfJung's own attempt at it. I haven't done a full audit, but I believe everything is fixed now. Fixes #61432. Closes #61336, as a drive-by fix (for a subset of #43408, that is already special-cased). r? @oli-obk / @RalfJung cc @varkor @yodaldevoid
|
☔ The latest upstream changes (presumably #63497) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Closing since #63497 fixed this in a cleaner way |
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.
cc rust-lang/rust-clippy#4121
I have not been able to build code that errors today. In order to have that we'd need to be running const eval with generic substitutions in scope. We may be able to trigger the code via const propagation, but I think that just bails very early in many cases.