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.

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

Normalize all opaque types when converting ParamEnv to Reveal::All #65989

Merged
merged 4 commits into from
Jul 31, 2020

Conversation

Aaron1011
Copy link
Member

When we normalize a type using a ParamEnv with a reveal mode of
RevealMode::All, we will normalize opaque types to their underlying
types (e.g. type MyOpaque = impl Foo -> StructThatImplsFoo).
However, the ParamEnv may still have predicates referring to the
un-normalized opaque type (e.g. <T as MyTrait<MyOpaque>>). This can
cause trait projection to fail, since a type containing normalized
opaque types will not match up with the un-normalized type in the
ParamEnv.

To fix this, we now explicitly normalize all opaque types in
caller_bounds of a ParamEnv when changing its mode to
RevealMode::All. This ensures that all predicatse will refer to the
underlying types of any opaque types involved, allowing them to be
matched up properly during projection. To reflect the fact that
normalization is occuring, ParamEnv::with_reveal_all is renamed to
ParamEnv::with_reveal_all_normalized

Fixes #65918

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 31, 2019
@Centril Centril added the F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` label Oct 31, 2019
Copy link
Contributor

@Centril Centril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some nits. :)
Nice PR!

src/librustc/ty/util.rs Outdated Show resolved Hide resolved
src/librustc/ty/mod.rs Outdated Show resolved Hide resolved
src/librustc/ty/util.rs Outdated Show resolved Hide resolved
src/librustc/ty/util.rs Outdated Show resolved Hide resolved
src/librustc/ty/util.rs Outdated Show resolved Hide resolved
src/librustc/ty/util.rs Outdated Show resolved Hide resolved
src/librustc/ty/util.rs Outdated Show resolved Hide resolved
@Aaron1011
Copy link
Member Author

@Centril I've addressed your comments.

@petrochenkov
Copy link
Contributor

r? @eddyb for review or reassignment

@rust-highfive rust-highfive assigned eddyb and unassigned petrochenkov Nov 1, 2019
@eddyb
Copy link
Member

eddyb commented Nov 2, 2019

This is @rust-lang/wg-traits territory.
r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Nov 2, 2019
@JohnCSimon
Copy link
Member

Ping from triage:
@nikomatsakis can you please review this?
cc: @petrochenkov @Aaron1011 @petrochenkov

Thanks!

@JohnCSimon
Copy link
Member

Pinging again from triage:
@nikomatsakis can you please review this?
cc: @petrochenkov @Aaron1011 @petrochenkov @rust-lang/wg-traits @nikomatsakis

Thanks!

@Dylan-DPC-zz
Copy link

r? @petrochenkov

@petrochenkov
Copy link
Contributor

I'm not qualified to review this.
r? @nikomatsakis

@bors
Copy link
Contributor

bors commented Dec 1, 2019

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

@nikomatsakis
Copy link
Contributor

Will review -- sorry for radio silence

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yet another example of why it would be good to pursue an alternative normalization strategy, I suppose. I am curious though whether this has any performance impact. We may want to think about more aggressive caching.

@nikomatsakis
Copy link
Contributor

@bors try

@bors
Copy link
Contributor

bors commented Dec 7, 2019

⌛ Trying commit 10a5c93 with merge ff0aa14...

bors added a commit that referenced this pull request Dec 7, 2019
Normalize all opaque types when converting ParamEnv to Reveal::All

When we normalize a type using a ParamEnv with a reveal mode of
RevealMode::All, we will normalize opaque types to their underlying
types (e.g. `type MyOpaque = impl Foo` -> `StructThatImplsFoo`).
However, the ParamEnv may still have predicates referring to the
un-normalized opaque type (e.g. `<T as MyTrait<MyOpaque>>`). This can
cause trait projection to fail, since a type containing normalized
opaque types will not match up with the un-normalized type in the
`ParamEnv`.

To fix this, we now explicitly normalize all opaque types in
caller_bounds of a `ParamEnv` when changing its mode to
`RevealMode::All`. This ensures that all predicatse will refer to the
underlying types of any opaque types involved, allowing them to be
matched up properly during projection. To reflect the fact that
normalization is occuring, `ParamEnv::with_reveal_all` is renamed to
`ParamEnv::with_reveal_all_normalized`

Fixes #65918
@bors
Copy link
Contributor

bors commented Dec 7, 2019

☀️ Try build successful - checks-azure
Build commit: ff0aa14 (ff0aa146df927e3857e94e7206d788076f3b500b)

@Aaron1011
Copy link
Member Author

@nikomatsakis: Did you mean to do a perf run as well as a try build?

@nikomatsakis
Copy link
Contributor

@Aaron1011 dang it, yes I did

@rust-timer
Copy link
Collaborator

Queued 95dcfa37cf97c1d630a7820e31d39a7a37ae2c1d with parent 4825e12, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (95dcfa37cf97c1d630a7820e31d39a7a37ae2c1d): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

@Aaron1011
Copy link
Member Author

Aaron1011 commented Jul 22, 2020

@nikomatsakis: It looks like almost all of the perf impact is gone.

@Aaron1011 Aaron1011 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 23, 2020
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 27, 2020

📌 Commit 5e2e927 has been approved by nikomatsakis

@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 Jul 27, 2020
@bors
Copy link
Contributor

bors commented Jul 27, 2020

⌛ Testing commit 5e2e927 with merge a7b1b8ea5d0b7b7e6d162fe0066eb0f1afc94778...

@bors
Copy link
Contributor

bors commented Jul 27, 2020

💔 Test failed - checks-actions

@bors 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 Jul 27, 2020
@nikomatsakis nikomatsakis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 28, 2020
@Aaron1011
Copy link
Member Author

I think this was a spurious failure.

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jul 31, 2020

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
  • There's another pull request that is currently being tested, blocking this pull request: Enable the profiler on FreeBSD #74844

@bors
Copy link
Contributor

bors commented Jul 31, 2020

📌 Commit 5e2e927 has been approved by nikomatsakis

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 31, 2020
@bors
Copy link
Contributor

bors commented Jul 31, 2020

⌛ Testing commit 5e2e927 with merge 6e87bac...

@bors
Copy link
Contributor

bors commented Jul 31, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: nikomatsakis
Pushing 6e87bac to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 31, 2020
@bors bors merged commit 6e87bac into rust-lang:master Jul 31, 2020
@Mark-Simulacrum
Copy link
Member

This was a slight performance regression, as expected.

ehuss added a commit to ehuss/rust that referenced this pull request Apr 15, 2023
This test was fixed by rust-lang#65989
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Development

Successfully merging this pull request may close these issues.

ICE with impl Fn alias.