Fix const-generics ICE related to binding

[JohnTitor]

Fixes #83765, fixes #85848
r? @jackh726 as you're familiar with Binding. I'd like to get some views if the current approach is right path.

[jackh726]

So, unfortunately this isn't correct :/

As you can see from the failed test, this indeed is a tricky case (see https://github.com/rust-lang/rust/pull/83825/files#diff-a54c982c1e255569ddede4404cdf85ecfbd93c20edfa8a090a5f2dc2bc1dff6bR119 where we were going to just shunt that BoundVarsCollector into the corner for now).

I knew that issue-64494#full was creating bound types that lose their Binder. But I just now found out that there are tests that create bound regions that lose their Binder.

Ultimately, the "solution" is to either 1) not be creating bound vars (I'm not familiar with the originating code) or 2) Ensure that we also keep the Binder (in rcvr_substs in this example) when creating the bound vars.

@camelid thinks that these are originating from the coherence code, but hasn't been able to track down more specifically.

One alternative to "fix" things for now, is to move BoundVarsCollector here as I would like in #83825 then just re-add the code that collects regions. Ideally, I would like the real fix, but I would also be fine with merging the patch. I left a few reviews on #83825; if you want to use that PR as a base, work through the review, and re-add the region code, I'm sure @camelid wouldn't mind :)

[JohnTitor]

Thanks for checking!

2. Ensure that we also keep the Binder (in rcvr_substs in this example) when creating the bound vars.

Could we achieve this by following?

- let bound_vars = tcx
-             .mk_bound_variable_kinds(std::iter::once(ty::BoundVariableKind::Region(ty::BrAnon(0))));
+ let bound_vars = trait_ref.to_poly_trait_ref().bound_vars();

And it's odd not to reproduce the issue-64494's failure on my local, how could I reproduce it?

[JohnTitor]

Okay, moved BoundVarsCollector here (didn't actually move them because it'd cause a cyclic dependency error unless we move all the changes on #83825 here), added FIXME comment, and removed bug! macro. Can this be a "fix" for now? Actually not :(

[jackh726]

I think I would rather this completely subsume the other PR, rather than just duplicate BoundVarsCollector (honestly, should be pretty quick).

[jackh726]

Could we achieve this by following?

- let bound_vars = tcx
-             .mk_bound_variable_kinds(std::iter::once(ty::BoundVariableKind::Region(ty::BrAnon(0))));
+ let bound_vars = trait_ref.to_poly_trait_ref().bound_vars();

Nope! If you look at to_poly_trait_ref, it actually wraps it in a Binder::dummy (I would like at some point to remove these functions in favor of being more explicit at the call sites)

And it's odd not to reproduce the issue-64494's failure on my local, how could I reproduce it?

Weid. When I do ./x.py test src/test/ui/const-generics it repros fine

[JohnTitor]

Thanks for the pointer! Pulled the changes from that PR applying your review comments.

Weid. When I do ./x.py test src/test/ui/const-generics it repros fine

Hmm, then maybe my config.toml or something else is wrong. For now, I'm using CI here for testing.

[jackh726]

Some comments. Sorry this ended up being a bit more involved than you probably expected. 🙃

[jackh726]

One small change (it gets removed in #85499 anyways, but would rather not make that here)

Then LGTM!

[jackh726]

@bors r+ rollup=never

There might be a slight perf improvement because of Binder::dummy, but the perf run done in #83825 suggests this is very slight, if any.

[bors]

📌 Commit 58f6cb4 has been approved by jackh726

[bors]

⌛ Testing commit 58f6cb4 with merge b3af93d71879ea99014f96f89d1179326b25447b...

[bors]

💔 Test failed - checks-actions

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[JohnTitor]

2021-07-02T23:00:37.2702922Z ##[group]Run exit 1
2021-07-02T23:00:37.3307585Z shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
2021-07-02T23:00:37.3308542Z ##[endgroup]
2021-07-02T23:00:37.3778908Z ##[error]Process completed with exit code 1.
2021-07-02T23:00:37.3810471Z Cleaning up orphan processes

@bors retry

[bors]

⌛ Testing commit 58f6cb4 with merge cd48e61...

[bors]

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing cd48e61 to master...