revert #75443, update mir validator #78410

lcnr · 2020-10-26T19:57:01Z

This PR reverts #75443 to fix #75992 and instead uses #75419 to fix #75313.

Adapts #75419 to correctly deal with unevaluated constants as otherwise some feature(const_evaluatable_checked) tests would ICE.

Note that #72793 was also fixed by #75443, but as that issue only concerns feature(type_alias_impl_trait) I deleted that test case for now and would reopen that issue.

#75443 may have also allowed some other code to now successfully compile which would make this revert a breaking change after 2 stable versions, but I hope that this is a purely theoretical concern.

See https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/generator.20upvars/near/214617274 for more reasoning about this.

r? @nikomatsakis @eddyb @RalfJung

yotamofek · 2020-10-26T21:19:10Z

@lcnr the binary that has been taking ~30mins for me to build is now taking 2:20 minutes to build from scratch, using rustc with your branch!!!!!!

thank you so much for ALL your work on this! I read on zulip you mentioned that it's the first time you felt unpleasant opening a PR... well, if it means anything, this PR is very much pleasant to me. I've been waiting for this for a while now! I'm so far from understanding the compiler internals to know what the underlying issue was, but just wanted to express my gratitude as an end-user of the amazing Rust language. And also let you know that it certainly fixed the issue I was experiencing :P

Mark-Simulacrum · 2020-10-26T21:31:56Z

#75443 may have also allowed some other code to now successfully compile which would make this revert a breaking change after 2 stable versions, but I hope that this is a purely theoretical concern.

Can you say more about this? Maybe a code example if possible? I think if we are to land a breaking change I'd prefer to have pre-prepared a note to include in the release notes, at least.

RalfJung · 2020-10-27T15:31:52Z

compiler/rustc_mir/src/transform/validate.rs

+            }),
+        )
+    };
+    tcx.infer_ctxt().enter(|infcx| infcx.can_eq(param_env, normalize(src), normalize(dest)).is_ok())


This seems reasonable -- it is pretty close to what I had before we went for TypeRelation, except for this infcx.can_eq thing which I never saw before so I cannot comment on it.^^

Using normalize(src) == normalize(dest) would fail to unify 2 generic consts, so the following would fail mir validation

#[feature(const_generics, lazy_normalization_consts)] fn test<const N: usize>() -> [u8; N + 1] { [0; N + 1] }

So generic consts are not interned the same way as other types? Or what does the difference stem from?

the 2 N + 1 are two distinct mir bodies so we have ConstKind::Unevaluated(DefId1, substs: [N]) and ConstKind::Unevaluated(DefId2, substs: [N]) which should still be considered equal. This doesn't really exist for types afaik as we can partially normalize them.

We may want to change this in the future but I don't yet know how we would do that

"which would still be considered unequal", you mean?

ConstKind::Unevaluated(DefId1, substs: [N]) and ConstKind::Unevaluated(DefId2, substs: [N]) are considered unequal when using Ty == Ty atm but are - and should be - considered equal with feature(const_evaluatable_checked).

I see. Yes, that makes sense. The point here is that normalize doesn't do any kind of evaluation of unevaluated constants, so simple equality is not especially meaningful.

lcnr · 2020-10-28T09:37:48Z

Can you say more about this? Maybe a code example if possible?

I tried to find something which worked because of #75443 on stable but wasn't able to.
My concern is something like #76956

use std::ops::Deref;

fn use_data(v: &'static i32, user: &dyn for<'a> Fn(<Box<&'a i32> as Deref>::Target)) {
    //~^ ERROR expected function, found `&dyn for<'a> Fn(<Box<&'a i32> as Deref>::Target)`
    user(v)
}

fn use_data(v: &'static i32, user: &dyn for<'a> Fn(&'a i32)) {
    // OK
    user(v)
}

where we somehow use an opaque type instead of <Box<&'a i32> as Deref>::Target. But it looks like that isn't possible on stable yet so we might be safe.

pnkfelix · 2020-10-29T12:34:22Z

beta-nominating to discuss whether we would consider back porting this based on the degree of the stable-to-stable compilation-time regression

oli-obk · 2020-10-29T14:11:16Z

cc @rust-lang/wg-mir-opt

bors · 2020-11-01T04:15:40Z

☔ The latest upstream changes (presumably #78147) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

nikomatsakis

Generally looks reasonable, though I think I'm also missing some context to fully understand what's going on.

nikomatsakis · 2020-11-02T15:52:45Z

compiler/rustc_mir/src/transform/validate.rs

+            param_env,
+            ty.fold_with(&mut BottomUpFolder {
+                tcx,
+                // We just erase all late-bound lifetimes, but this is not fully correct (FIXME):


I don't fully understand this comment. I'm not sure how much it matters, but it'd be nice to have a more detailed explanation of how lifetimes could matter. I guess it depends on a bit of context that I seem to be missing, about the broader role of this function.

This is the concern that @eddyb keeps raising; another instance of that is here. It's something about using lifetimes in invariant positions to force two different trait implementations to be used which could have different associated types which could make this check unsound. I don't have more than a fuzzy understanding of this, and I forgot if we ever were able to come up with a concrete example.

Apologies for not providing an example initially, given how trivial it is to construct "transmute in safe code" gadgets from associated type unsoundness (the hard part is you have to intentionally make rustc unsound enough to truly confirm something else doesn't defeat the example).
I've just posted #77196 (comment), hope that helps.

@eddyb also pointed out that we should probably clarify the comment to explicitly mention projections -- specifically, if I understood correctly, the concern is about erasing a lifetime in T when the type is T::Assoc.

I see, so the concern here is that we are not preserving the binding levels? Let me re-read this code then.

I see, so the concern here is that we are not preserving the binding levels? Let me re-read this code then.

Binding levels? I have no idea what you are talking about.^^

I guess the question is why we are not using an existing function to erase or normalize regions and instead rolling our own here.

I tried all the existing ones (that I could find) and none of them worked.^^ (As in, none of them accepted all the code out there.) Then I asked @eddyb what to do and we ended up with this.

I don't know that it has to block this PR from landing

No, it certainly is not a blocker. This is an old FIXME that was removed when we switched to TypeRelation and now re-appears since we are switching back to BottomUpFolder.

What I meant by "binding level" is that for<'a> fn(fn(&'a u32)) and fn(for<'a> fn(&'a u32)) are considered distinct types (and fn(&'a u32), where 'a appears free, is also distinct) by various parts of Rust. We've been slowly changing the model here but I think we'll not be able to change this completely. In particular, the coherence code accepts distinct impls (as eddyb demonstrated) for some such cases.

see also #56105 and #72493

What I meant by "binding level" is that for<'a> fn(fn(&'a u32)) and fn(for<'a> fn(&'a u32)) are considered distinct types

Sure, it would be really strange if those would be the same type. The outer function in the latter may pick different lifetimes for different calls of the inner function; that is not the case for the former.

nikomatsakis · 2020-11-02T15:53:20Z

compiler/rustc_mir/src/transform/validate.rs

+            }),
+        )
+    };
+    tcx.infer_ctxt().enter(|infcx| infcx.can_eq(param_env, normalize(src), normalize(dest)).is_ok())


"which would still be considered unequal", you mean?

RalfJung · 2020-11-05T16:15:30Z

compiler/rustc_mir/src/transform/validate.rs

+            ty.fold_with(&mut BottomUpFolder {
+                tcx,
+                // We just erase all late-bound lifetimes, but this is not fully correct (FIXME):
+                // lifetimes in invariant positions could matter (e.g. through associated types).


Suggested change

// lifetimes in invariant positions could matter (e.g. through associated types).

// lifetimes in projections could matter (e.g. `T<'a>::Assoc`).

We should clarify this comment, but I am not sure how to best do that.^^

Maybe we should add a link to #77196 (comment).

I'd probably inline the comment, but I agree we can improve the wording here. Something like:

FIXME: We erase all late-bound lifetimes, but this is not fully correct. If you have a type like
<for<'a> fn(&'a u32) as SomeTrait>::Assoc, this is not necessarily equivalent to <fn(&'static u32) as SomeTrait>::Assoc, since one may have an impl SomeTrait for fn(&32) and impl SomeTrait for fn(&'static u32) at the same time which specify distinct values for Assoc. (See also #56105)

nikomatsakis · 2020-11-05T18:32:39Z

r=me with improved comment wording (Cc @lcnr)

spastorino · 2020-11-05T19:43:03Z

discussed in T-compiler meeting.

@rustbot modify labels: beta-accepted

Mark-Simulacrum · 2020-11-06T22:35:04Z

@lcnr should we pull the new commit into the beta backport as well?

lcnr · 2020-11-06T22:44:51Z

It only changes a comment, so while it wouldn't hurt to do so I don't really think it matters

…ulacrum [beta] backports This backports a number of PRs to beta: * Add delay_span_bug to no longer ICE rust-lang#78645 * Do not ICE on invalid input rust-lang#78422 * revert rust-lang#75443, update mir validator rust-lang#78410 * Do not try to report on closures to avoid ICE rust-lang#78268 * Disable "optimization to avoid load of address" in InstCombine rust-lang#78195 * Disable MatchBranchSimplification rust-lang#78151 * Do not ICE with TraitPredicates containing [type error] rust-lang#77930 * Tweak `if let` suggestion to be more liberal with suggestion and to not ICE rust-lang#77283 * Use different mirror for linux headers in musl-toolchain CI script. rust-lang#78316

Dylan-DPC-zz · 2020-11-08T02:27:31Z

@bors p=4
(giving this a bit of a priority bump)

bors · 2020-11-08T07:02:51Z

⌛ Testing commit e06785b with merge f57cb8446dce8d2bc698be641fb2b42da883dce5...

bors · 2020-11-08T11:17:51Z

💥 Test timed out

RalfJung · 2020-11-08T11:27:01Z

@bors retry

bors · 2020-11-08T11:27:12Z

⌛ Testing commit e06785b with merge 87a0997...

bors · 2020-11-08T13:49:11Z

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing 87a0997 to master...

A severe performance regression with `async fn` was fixed in 1.48.0, which greatly improves compilation times on many people's machines. rust-lang/rust#78410 As such, we bump the `rust-toolchain` file to 1.48.0 stable for a smoother developer experience, even if 1.46.0 is still technically supported features-wise.

rust-highfive assigned nikomatsakis Oct 26, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 26, 2020

lcnr added I-nominated T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 26, 2020

Mark-Simulacrum added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 26, 2020

RalfJung reviewed Oct 27, 2020

View reviewed changes

pnkfelix mentioned this pull request Oct 29, 2020

Increasingly slow compilation as more levels of async are added in Rust 1.46 #75992

Open

pnkfelix added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 29, 2020

lcnr changed the title ~~revert #75443 update mir validator~~ revert #75443, update mir validator Oct 29, 2020

oli-obk added A-mir-opt Area: MIR optimizations A-typesystem Area: The type system labels Oct 29, 2020

nikomatsakis approved these changes Nov 2, 2020

View reviewed changes

revert rust-lang#75443 update mir validator

cc19df6

lcnr force-pushed the revert75443 branch from e43a6b9 to cc19df6 Compare November 2, 2020 22:57

RalfJung reviewed Nov 5, 2020

View reviewed changes

rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Nov 5, 2020

spastorino removed the I-nominated label Nov 5, 2020

Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 5, 2020

Mark-Simulacrum mentioned this pull request Nov 5, 2020

[beta] backports #78783

Merged

improve fixme

e06785b

lcnr force-pushed the revert75443 branch from c0ee57a to e06785b Compare November 6, 2020 21:37

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Nov 6, 2020

Mark-Simulacrum added this to the 1.48.0 milestone Nov 7, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 8, 2020

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 8, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 8, 2020

bors merged commit 87a0997 into rust-lang:master Nov 8, 2020

rustbot modified the milestones: 1.48.0, 1.49.0 Nov 8, 2020

lcnr deleted the revert75443 branch November 8, 2020 14:39

Mark-Simulacrum modified the milestones: 1.49.0, 1.48.0 Nov 8, 2020

elpiel mentioned this pull request Nov 16, 2020

(UPDATE to 1.48 when released) Rust 1.46 breaks the build, while 1.45 works just fine AmbireTech/adex-validator-stack-rust#330

Closed

JohnTitor mentioned this pull request Dec 4, 2020

ICE (OutputTypeParameterMismatch) with type_alias_impl_trait #72793

Closed

SNCPlay42 mentioned this pull request Jan 5, 2021

ICE(stable): thread 'rustc' panicked at 'called Option::unwrap() on a None value', compiler/rustc_middle/src/ty/instance.rs:434:85 #80706

Closed

lcnr mentioned this pull request Feb 17, 2023

performance regressions after stabilization lcnr/solver-woes#1

Open

matthiaskrgr mentioned this pull request Jun 13, 2024

ICE: inconsistent resolution for an import #126389

Closed

	// lifetimes in invariant positions could matter (e.g. through associated types).
	// lifetimes in projections could matter (e.g. `T<'a>::Assoc`).

revert #75443, update mir validator #78410

revert #75443, update mir validator #78410

Conversation

lcnr commented Oct 26, 2020 • edited Loading

yotamofek commented Oct 26, 2020

Mark-Simulacrum commented Oct 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Oct 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcnr commented Oct 28, 2020

pnkfelix commented Oct 29, 2020

oli-obk commented Oct 29, 2020

bors commented Nov 1, 2020

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikomatsakis commented Nov 5, 2020

spastorino commented Nov 5, 2020

Mark-Simulacrum commented Nov 6, 2020

lcnr commented Nov 6, 2020

Dylan-DPC-zz commented Nov 8, 2020

bors commented Nov 8, 2020

bors commented Nov 8, 2020

RalfJung commented Nov 8, 2020

bors commented Nov 8, 2020

bors commented Nov 8, 2020

lcnr commented Oct 26, 2020 •

edited

Loading

RalfJung Oct 27, 2020 •

edited

Loading