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

Revert lazy TAIT PR #93893

Merged
merged 3 commits into from
Feb 11, 2022
Merged

Revert lazy TAIT PR #93893

merged 3 commits into from
Feb 11, 2022

Conversation

oli-obk
Copy link
Contributor

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

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

fixes #93788
fixes #93794
fixes #93821
fixes #93831
fixes #93841

@rust-highfive
Copy link
Collaborator

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

@rustbot rustbot added 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. labels Feb 11, 2022
@rust-highfive
Copy link
Collaborator

r? @matthewjasper

(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 Feb 11, 2022
@oli-obk
Copy link
Contributor Author

oli-obk commented Feb 11, 2022

@bors r+ p=1 fixes lots recent stable to nightly regressions

@bors
Copy link
Contributor

bors commented Feb 11, 2022

📌 Commit d54195d has been approved by oli-obk

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

bors commented Feb 11, 2022

⌛ Testing commit d54195d with merge 5ed0a0ad14a648aa93b3d8a18cd0b9b91d32cfca...

@pro465
Copy link
Contributor

pro465 commented Feb 11, 2022

i admit i dont know anything about rust internals,
but why not fix it by actually checking if the type actually implements the traits specified, instead of just "assigning" the type to the opaque type(which i assume is referring to impl Trait, idk if i am right though) the first time they get compared and returning the equivalent of true?

@bors
Copy link
Contributor

bors commented Feb 11, 2022

💥 Test timed out

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

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

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

@lqd
Copy link
Member

lqd commented Feb 11, 2022

@bors retry timeout

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

bors commented Feb 11, 2022

⌛ Testing commit d54195d with merge 6499c5e...

@bors
Copy link
Contributor

bors commented Feb 11, 2022

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 6499c5e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 11, 2022
@bors bors merged commit 6499c5e into rust-lang:master Feb 11, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6499c5e): comparison url.

Summary: This benchmark run shows 24 relevant improvements 🎉 but 12 relevant regressions 😿 to instruction counts.

  • Average relevant regression: 1.0%
  • Average relevant improvement: -1.2%
  • Largest improvement in instruction counts: -1.8% on full builds of deeply-nested-async check
  • Largest regression in instruction counts: 1.6% on full builds of wg-grammar check

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

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

@rustbot rustbot added the perf-regression Performance regression. label Feb 11, 2022
@oli-obk oli-obk deleted the sad_revert branch February 11, 2022 21:50
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
@pnkfelix
Copy link
Member

We had to revert the TAIT PR, the fallout from it was too high.

@rustbot label: +perf-regression-triaged

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Feb 16, 2022
flip1995 pushed a commit to flip1995/rust that referenced this pull request Feb 24, 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
oli-obk added a commit to oli-obk/rust that referenced this pull request Mar 28, 2022
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
flip1995 pushed a commit to flip1995/rust that referenced this pull request Apr 7, 2022
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. perf-regression-triaged The performance regression has been triaged. 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
10 participants