Skip to content
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.

Sign up for GitHub

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

Jump to bottom

implied_bounds: deal with inference vars #102016

Merged
merged 2 commits into from
Sep 25, 2022
Merged

implied_bounds: deal with inference vars #102016

merged 2 commits into from
Sep 25, 2022

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Sep 19, 2022

fixes #101951

while computing implied bounds for <<T as ConstructionFirm>::Builder as BuilderFn<'_>>::Output normalization replaces a projection with an inference var (adding a Projection obligation). Until we prove that obligation, this inference var remains unknown, which caused us to miss an implied bound necessary to prove that the unnormalized projection from the trait method signature is wf.

r? types

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 19, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 19, 2022
@lcnr lcnr mentioned this pull request Sep 19, 2022
Closed
@jackh726
Copy link
Member

I'm wondering if #101680 also fixes it...

@lcnr
Copy link
Contributor Author

lcnr commented Sep 19, 2022

would assume no, because you still eagerly look at TypeOutlives obligations there 🤔 But considering that the failing region relation was from the implied bounds query your pr may still fix #101951? 🤔

There may be other examples where ignoring infer vars from normalization causes us to fail to prove some outlives obligation which isn't added by the implied bounds query itself 😅 because of that I wouldn't be too comfortable with only merging your pr, even if that were to fix this specific test

@jackh726
Copy link
Member

Just checked, it does fix it.

Now I'm wondering if I could find a minimal repro of a case that fails without my PR (and more-so with yours). I only really found the bug my PR aims to solve by making other changes (that didn't quite work anyways). I don't think yours solves that bug.

However, I have to think a little bit about why my PR does solve this test, since it seems to be because of not resolving the var before computing components. If I had to guess, the reason my PR fixes the issue, is because we properly register everything as implied, not constraints, even though it's an unresolved inference var. But then later we normalize and register everything properly.

I think the fully correct behavior is the intersection between these two PRs: we should be resolving variables before computing components for them and we shouldn't be registering implied bounds as constraints.

Two things I'd like to know about this (even though I'm not sure it's needed for this PR):

  1. If you were to make the change I was trying to make in Fix implied outlives bounds logic for projections #101680 - namely pushing constraints immediately after registering implied bounds - does the hyper benchmark from rustc-perf still fail to build (how I found the bug).
  2. With this PR, if we try to take implied bounds from normalized types (as in don't add implied bounds after normalization #99832), do we still run into that one test failure?

r? @jackh726

@rust-highfive rust-highfive assigned jackh726 and unassigned spastorino Sep 19, 2022
@jackh726
Copy link
Member

The answers to the above are 1) No, it builds fine with this PR 2) No, the test passes.

@jackh726
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Sep 21, 2022

📌 Commit 72a2102 has been approved by jackh726

It is now in the queue for this repository.

@bors 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 Sep 21, 2022
@wesleywiser
Copy link
Member

Since this fixes a regression in 1.65, I'm nominating for beta backport.

@wesleywiser wesleywiser added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 22, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 24, 2022
@matthiaskrgr
Rollup merge of rust-lang#102016 - lcnr:given-OutlivesEnvironment, r=…
02beea3 
…jackh726

implied_bounds: deal with inference vars

fixes rust-lang#101951

while computing implied bounds for `<<T as ConstructionFirm>::Builder as BuilderFn<'_>>::Output` normalization replaces a projection with an inference var (adding a `Projection` obligation). Until we prove that obligation, this inference var remains unknown, which caused us to miss an implied bound necessary to prove that the unnormalized projection from the trait method signature is wf.

r? types
@matthiaskrgr matthiaskrgr mentioned this pull request Sep 24, 2022
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Sep 25, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 25, 2022
@bors
Auto merge of rust-lang#102254 - matthiaskrgr:rollup-gitu6li, r=matth…
6f6010b 
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#102016 (implied_bounds: deal with inference vars)
 - rust-lang#102161 (Resolve async fn signature even without body (e.g., in trait))
 - rust-lang#102216 (rustdoc: Stabilize --diagnostic-width)
 - rust-lang#102240 (rustdoc: remove unused CSS `#main-content > .line-numbers`)
 - rust-lang#102242 (rustdoc: remove unused CSS `.summary`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 16de1fd into rust-lang:master Sep 25, 2022
@rustbot rustbot added this to the 1.66.0 milestone Sep 25, 2022
@lcnr lcnr deleted the given-OutlivesEnvironment branch September 26, 2022 08:30
@apiraino
Copy link
Contributor

Beta backport approved for T-compiler on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Sep 29, 2022
@cuviper cuviper mentioned this pull request Oct 4, 2022
Merged
@cuviper cuviper modified the milestones: 1.66.0, 1.65.0 Oct 4, 2022
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Oct 4, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 10, 2022
@bors
Auto merge of rust-lang#102676 - cuviper:beta-next, r=cuviper
da7ffa2 
[beta] backports

* Avoid duplicating StorageLive in let-else rust-lang#101894
* Re-add HRTB implied static bug note rust-lang#101924
* Revert "Copy stage0 binaries into stage0-sysroot" rust-lang#101942
* implied_bounds: deal with inference vars rust-lang#102016
* fix ConstProp handling of written_only_inside_own_block_locals rust-lang#102045
* Fix wrongly refactored Lift impl rust-lang#102088
* Fix a typo “pararmeter” in error message rust-lang#102119
* Deny associated type bindings within associated type bindings rust-lang#102338
* Continue migration of CSS themes rust-lang#101934
* Fix search result colors rust-lang#102369
* Fix unwind drop glue for if-then scopes rust-lang#102394
* Revert "Use getentropy when possible on all Apple platforms" rust-lang#102693
* Fix associated type bindings with anon const in GAT position rust-lang#102336
* Revert perf-regression 101620 rust-lang#102064
* `EscapeAscii` is not an `ExactSizeIterator` rust-lang#99880
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jackh726 jackh726

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.65.0
Development

Successfully merging this pull request may close these issues.

E0477 triggered with current nightly
9 participants
@lcnr @jackh726 @bors @wesleywiser @apiraino @cuviper @spastorino @rust-highfive @rustbot