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

Lazy type-alias-impl-trait #92007

Merged
merged 53 commits into from
Feb 8, 2022
Merged

Lazy type-alias-impl-trait #92007

merged 53 commits into from
Feb 8, 2022

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Dec 16, 2021

Previously opaque types were processed by

  1. replacing all mentions of them with inference variables
  2. memorizing these inference variables in a side-table
  3. at the end of typeck, resolve the inference variables in the side table and use the resolved type as the hidden type of the opaque type

This worked okayish for impl Trait in return position, but required lots of roundabout type inference hacks and processing.

This PR instead stops this process of replacing opaque types with inference variables, and just keeps the opaque types around.
Whenever an opaque type O is compared with another type T, we make the comparison succeed and record T as the hidden type. If O is compared to U while there is a recorded hidden type for it, we grab the recorded type (T) and compare that against U. This makes implementing

much simpler (previous attempts on the inference based scheme were very prone to ICEs and general misbehaviour that was not explainable except by random implementation defined oddities).

r? @nikomatsakis

fixes #93411
fixes #88236

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 16, 2021
@jackh726
Copy link
Member

Woohoo. Nice job. This is a doosy to review though. 😰

@oli-obk
Copy link
Contributor Author

oli-obk commented Dec 17, 2021

Yea. The problem is that while I can pull some changes into master, they will be dead code and we usually don't accept such code. I think I may be able to restructure into reviewable commits though

@jackh726
Copy link
Member

I personally am mostly fine with splitting out changes here into separate PRs, even if it they're mostly dead code, given that there is this PR which clearly defines to "end goal". But, another approach would be to only split this into meaningful commits, but that's still really prone to bitrot.

@oli-obk oli-obk force-pushed the lazy_tait2 branch 2 times, most recently from cfb113a to be6eaf7 Compare January 12, 2022 17:01
@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 12, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 20, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 20, 2022
@bors
Copy link
Contributor

bors commented Jan 20, 2022

⌛ Testing commit 488ef96531f45667626fb808b45037b633f00e76 with merge c41542603add4dcd0e350ffbde4a1c19a29d0bd6...

@pietroalbini
Copy link
Member

@bors r- retry

Why did this PR start testing :/

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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 Jan 20, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 20, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@bors
Copy link
Contributor

bors commented Jan 20, 2022

⌛ Trying commit 488ef96531f45667626fb808b45037b633f00e76 with merge ebeceacb2f4cac04463864a910b2490c0ae511c8...

@bors
Copy link
Contributor

bors commented Jan 20, 2022

☀️ Try build successful - checks-actions
Build commit: ebeceacb2f4cac04463864a910b2490c0ae511c8 (ebeceacb2f4cac04463864a910b2490c0ae511c8)

@Mark-Simulacrum
Copy link
Member

perf is also indicating this PR broke the gll benchmark (as indicated in the results) - I'll look into that this morning.

@lqd
Copy link
Member

lqd commented Feb 8, 2022

We should add that information to the summary the bot posts

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 8, 2022

Fixing the failure first, then looking into perf

help: consider further restricting this bound
|
LL | fn bad(x: impl Into<u32> + std::fmt::Debug) -> impl Into<impl Debug> { x }
| +++++++++++++++++
Copy link
Member

Choose a reason for hiding this comment

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

What happened to these errors? Why x should implement debug? Why are they emitted, if impl Into<impl Debug> is invalid anyway?

@Mark-Simulacrum
Copy link
Member

We should add that information to the summary the bot posts

Posted rust-lang/rustc-perf#1169 to do this.

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 8, 2022

I fixed the gll problem: #93783 it hasn't hit nightly yet, so we could p=1 it if we wanted to.

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 8, 2022
…ackh726

Fix regression from lazy opaque types

The breakage was found in rust-lang#92007 (comment) and has not hit nightly yet.
Alexendoo added a commit to rust-lang/glacier that referenced this pull request Feb 9, 2022
oli-obk added a commit to oli-obk/rust that referenced this pull request Feb 11, 2022
…sakis"

This reverts commit e7cc3bd, reversing
changes made to 734368a.
@oli-obk oli-obk mentioned this pull request Feb 11, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 11, 2022
Revert lazy TAIT PR

Revert rust-lang#92306 (sorry `@Aaron1011,` will include your changes in the fix PR)
Revert rust-lang#93783
Revert rust-lang#92007

fixes rust-lang#93788
fixes rust-lang#93794
fixes rust-lang#93821
fixes rust-lang#93831
fixes rust-lang#93841
matthiaskrgr added a commit to matthiaskrgr/glacier that referenced this pull request Feb 12, 2022
…t#92007)"

This reverts commit 06c8426, reversing
changes made to 68630fb.

The fix at rustc has been reverted in rust-lang/rust#93893 due to other problems
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Feb 24, 2022
Revert lazy TAIT PR

Revert rust-lang/rust#92306 (sorry `@Aaron1011,` will include your changes in the fix PR)
Revert rust-lang/rust#93783
Revert rust-lang/rust#92007

fixes rust-lang/rust#93788
fixes rust-lang/rust#93794
fixes rust-lang/rust#93821
fixes rust-lang/rust#93831
fixes rust-lang/rust#93841
flip1995 pushed a commit to flip1995/rust that referenced this pull request Feb 24, 2022
…sakis"

This reverts commit e7cc3bd, reversing
changes made to 734368a.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 30, 2022
…sakis

Lazy type-alias-impl-trait take two

### user visible change 1: RPIT inference from recursive call sites

Lazy TAIT has an insta-stable change. The following snippet now compiles, because opaque types can now have their hidden type set from wherever the opaque type is mentioned.

```rust
fn bar(b: bool) -> impl std::fmt::Debug {
    if b {
        return 42
    }
    let x: u32 = bar(false); // this errors on stable
    99
}
```

The return type of `bar` stays opaque, you can't do `bar(false) + 42`, you need to actually mention the hidden type.

### user visible change 2: divergence between RPIT and TAIT in return statements

Note that `return` statements and the trailing return expression are special with RPIT (but not TAIT). So

```rust
#![feature(type_alias_impl_trait)]
type Foo = impl std::fmt::Debug;

fn foo(b: bool) -> Foo {
    if b {
        return vec![42];
    }
    std::iter::empty().collect() //~ ERROR `Foo` cannot be built from an iterator
}

fn bar(b: bool) -> impl std::fmt::Debug {
    if b {
        return vec![42]
    }
    std::iter::empty().collect() // Works, magic (accidentally stabilized, not intended)
}
```

But when we are working with the return value of a recursive call, the behavior of RPIT and TAIT is the same:

```rust
type Foo = impl std::fmt::Debug;

fn foo(b: bool) -> Foo {
    if b {
        return vec![];
    }
    let mut x = foo(false);
    x = std::iter::empty().collect(); //~ ERROR `Foo` cannot be built from an iterator
    vec![]
}

fn bar(b: bool) -> impl std::fmt::Debug {
    if b {
        return vec![];
    }
    let mut x = bar(false);
    x = std::iter::empty().collect(); //~ ERROR `impl Debug` cannot be built from an iterator
    vec![]
}
```

### user visible change 3: TAIT does not merge types across branches

In contrast to RPIT, TAIT does not merge types across branches, so the following does not compile.

```rust
type Foo = impl std::fmt::Debug;

fn foo(b: bool) -> Foo {
    if b {
        vec![42_i32]
    } else {
        std::iter::empty().collect()
        //~^ ERROR `Foo` cannot be built from an iterator over elements of type `_`
    }
}
```

It is easy to support, but we should make an explicit decision to include the additional complexity in the implementation (it's not much, see a721052457cf513487fb4266e3ade65c29b272d2 which needs to be reverted to enable this).

### PR formalities

previous attempt: rust-lang#92007

This PR also includes rust-lang#92306 and rust-lang#93783, as they were reverted along with rust-lang#92007 in rust-lang#93893

fixes rust-lang#93411
fixes rust-lang#88236
fixes rust-lang#89312
fixes rust-lang#87340
fixes rust-lang#86800
fixes rust-lang#86719
fixes rust-lang#84073
fixes rust-lang#83919
fixes rust-lang#82139
fixes rust-lang#77987
fixes rust-lang#74282
fixes rust-lang#67830
fixes rust-lang#62742
fixes rust-lang#54895
Alexendoo added a commit to Alexendoo/glacier that referenced this pull request Mar 31, 2022
Alexendoo added a commit to rust-lang/glacier that referenced this pull request Mar 31, 2022
…lang/rust#92007)""

This reverts commit c34cdca.

89312 is fixed by rust-lang/rust#95471

The rest by lazy TAIT take two rust-lang/rust#94081

Closes #1189
Closes #1190
Closes #1191
Closes #1192
Closes #1193
Closes #1194
Closes #1195
Closes #1196
Closes #1197
Closes #1198
Closes #1199
Closes #1200
Closes #1201
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. 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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compiler segfault when using type_alias_impl_trait ICE: impl for<'a> Hrtb<'a, Assoc = impl Send + 'a>