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

Fix regression from lazy opaque types #93783

Merged
merged 1 commit into from
Feb 9, 2022

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Feb 8, 2022

The breakage was found in #92007 (comment) and has not hit nightly yet.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 8, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 8, 2022
@rust-log-analyzer

This comment has been minimized.

@oli-obk oli-obk force-pushed the lazy_tait_regression_fix branch from 6c47458 to 239f1e7 Compare February 8, 2022 17:01
@Mark-Simulacrum
Copy link
Member

@bors p=1 rollup=never as this fixes broken perf, but I don't feel comfortable approving this myself right now.

@jackh726
Copy link
Member

jackh726 commented Feb 8, 2022

@bors r+

This is hacky, but I think better to land this and come up with a better fix than to let this hit nightly.

@bors
Copy link
Contributor

bors commented Feb 8, 2022

📌 Commit 239f1e7 has been approved by jackh726

@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 Feb 8, 2022
@bors
Copy link
Contributor

bors commented Feb 8, 2022

⌛ Testing commit 239f1e7 with merge 97a8e8b5b5eec9a7cc44b553855cfe3c5dfd421e...

@bors
Copy link
Contributor

bors commented Feb 8, 2022

💔 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 Feb 8, 2022
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
.......... (50/59)
........  (59/59)


/checkout/src/test/rustdoc-gui/search-filter.goml search-filter... FAILED
[ERROR] (line 6) TimeoutError: waiting for selector "#titles" failed: timeout 30000ms exceeded: for command `wait-for: "#titles"`
Build completed unsuccessfully in 0:00:45

@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 8, 2022

@bors retry

@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 Feb 8, 2022
@lqd
Copy link
Member

lqd commented Feb 8, 2022

note for t-infra and tracking, the retry was because of #93784

@bors
Copy link
Contributor

bors commented Feb 8, 2022

⌛ Testing commit 239f1e7 with merge cc38176...

@bors
Copy link
Contributor

bors commented Feb 9, 2022

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 9, 2022
@bors bors merged commit cc38176 into rust-lang:master Feb 9, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 9, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (cc38176): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@oli-obk oli-obk deleted the lazy_tait_regression_fix branch February 10, 2022 15:49
@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
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
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
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. 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
Development

Successfully merging this pull request may close these issues.

10 participants