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

typeck: Prohibit RPIT types that inherit lifetimes #62849

Merged
merged 1 commit into from
Aug 14, 2019

Conversation

davidtwco
Copy link
Member

Part of #61949.

This PR prohibits return position impl Trait types that "inherit
lifetimes" from the parent scope. The intent is to forbid cases that are
challenging until they can be addressed properly.

cc @nikomatsakis

@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 Jul 21, 2019
@cramertj
Copy link
Member

@bors try (for crater)

@bors
Copy link
Contributor

bors commented Jul 23, 2019

⌛ Trying commit 62f89e0 with merge 2110aa1...

bors added a commit that referenced this pull request Jul 23, 2019
typeck: Prohibit RPIT types that inherit lifetimes

Part of #61949.

This PR prohibits return position `impl Trait` types that "inherit
lifetimes" from the parent scope. The intent is to forbid cases that are
challenging until they can be addressed properly.

cc @nikomatsakis
@bors
Copy link
Contributor

bors commented Jul 23, 2019

☀️ Try build successful - checks-azure
Build commit: 2110aa1

@cramertj
Copy link
Member

@craterbot run mode=check-only

@@ -0,0 +1,8 @@
error: `async fn` return type cannot contain a projection or `Self` that references lifetimes from a parent scope
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should try to give some clue for how to workaround this -- this is basically a rustc bug.

In the case of Self, the suggestion would be to write the Self type explicitly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've dug around for a bit and I've not been able to find a way to determine if the return type was originally Self from here. I'm happy to add a suggestion if anyone knows a way to determine this.

Copy link
Contributor

Choose a reason for hiding this comment

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

At minimum we could just -- for this particular issue -- give it an error code and direct them to some chapter in the async book where we explain in more depth (in the extended explanation). But we'd need a link first. Maybe we can just file a follow-up issue on the matter to improve diagnostics.

@Centril
Copy link
Contributor

Centril commented Jul 26, 2019

@craterbot run mode=check-only

@craterbot
Copy link
Collaborator

🚨 Error: missing start toolchain

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@edmilsonefs
Copy link

Hey! This is a ping from triage, we would like to know if you @davidtwco could give us a few more minutes to update here by fixing the conflicting files so we can move forward.

Thanks.

@rustbot modify labels to +S-waiting-on-author, -S-waiting-on-review

@rustbot rustbot 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 30, 2019
@davidtwco davidtwco force-pushed the prohibit-inheriting-lifetimes branch from 62f89e0 to 686de3a Compare July 30, 2019 08:36
@davidtwco
Copy link
Member Author

@edmilsonefs thanks, I've resolved the conflict.

@Dylan-DPC-zz Dylan-DPC-zz 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 30, 2019
@Centril
Copy link
Contributor

Centril commented Aug 1, 2019

@bors try

@bors
Copy link
Contributor

bors commented Aug 1, 2019

⌛ Trying commit 686de3a with merge b5c7f15...

bors added a commit that referenced this pull request Aug 1, 2019
typeck: Prohibit RPIT types that inherit lifetimes

Part of #61949.

This PR prohibits return position `impl Trait` types that "inherit
lifetimes" from the parent scope. The intent is to forbid cases that are
challenging until they can be addressed properly.

cc @nikomatsakis
@bors
Copy link
Contributor

bors commented Aug 1, 2019

☀️ Try build successful - checks-azure
Build commit: b5c7f15

@Centril
Copy link
Contributor

Centril commented Aug 1, 2019

@craterbot run mode=check-only

@craterbot
Copy link
Collaborator

👌 Experiment pr-62849 created and queued.
🤖 Automatically detected try build b5c7f15
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 1, 2019
@craterbot
Copy link
Collaborator

🚧 Experiment pr-62849 is now running on agent aws-1.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-62849 is completed!
📊 0 regressed and 0 fixed (70078 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@bors

This comment has been minimized.

@Centril
Copy link
Contributor

Centril commented Aug 13, 2019

@bors retry rolled up (oops, forgot to adjust prio)

@rust-highfive

This comment has been minimized.

bors added a commit that referenced this pull request Aug 13, 2019
Rollup of 10 pull requests

Successful merges:

 - #62760 (Deduplicate error messages in `librsctc_mir`)
 - #62849 (typeck: Prohibit RPIT types that inherit lifetimes)
 - #63383 (`async fn` lifetime elision tests)
 - #63421 (Implement Clone, Display for ascii::EscapeDefault)
 - #63459 (syntax: account for CVarArgs being in the argument list.)
 - #63475 (Bring back suggestion for splitting `<-` into `< -`)
 - #63485 (ci: move mirrors to their standalone bucket)
 - #63486 (Document `From` trait for `BinaryHeap`)
 - #63488 (improve DiagnosticBuilder docs)
 - #63493 (Remove unneeded comment in src/libcore/hash/mod.rs)

Failed merges:

r? @ghost
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Aug 14, 2019
…imes, r=nikomatsakis

typeck: Prohibit RPIT types that inherit lifetimes

Part of rust-lang#61949.

This PR prohibits return position `impl Trait` types that "inherit
lifetimes" from the parent scope. The intent is to forbid cases that are
challenging until they can be addressed properly.

cc @nikomatsakis
Centril added a commit to Centril/rust that referenced this pull request Aug 14, 2019
…imes, r=nikomatsakis

typeck: Prohibit RPIT types that inherit lifetimes

Part of rust-lang#61949.

This PR prohibits return position `impl Trait` types that "inherit
lifetimes" from the parent scope. The intent is to forbid cases that are
challenging until they can be addressed properly.

cc @nikomatsakis
@bors
Copy link
Contributor

bors commented Aug 14, 2019

⌛ Testing commit 861d1bb with merge ade62304ed223496e69511fa5fb9b83420b7d5c9...

Centril added a commit to Centril/rust that referenced this pull request Aug 14, 2019
…imes, r=nikomatsakis

typeck: Prohibit RPIT types that inherit lifetimes

Part of rust-lang#61949.

This PR prohibits return position `impl Trait` types that "inherit
lifetimes" from the parent scope. The intent is to forbid cases that are
challenging until they can be addressed properly.

cc @nikomatsakis
@Centril
Copy link
Contributor

Centril commented Aug 14, 2019

@bors retry rolled up.

bors added a commit that referenced this pull request Aug 14, 2019
Rollup of 17 pull requests

Successful merges:

 - #62760 (Deduplicate error messages in `librsctc_mir`)
 - #62849 (typeck: Prohibit RPIT types that inherit lifetimes)
 - #63383 (`async fn` lifetime elision tests)
 - #63421 (Implement Clone, Display for ascii::EscapeDefault)
 - #63459 (syntax: account for CVarArgs being in the argument list.)
 - #63475 (Bring back suggestion for splitting `<-` into `< -`)
 - #63485 (ci: move mirrors to their standalone bucket)
 - #63486 (Document `From` trait for `BinaryHeap`)
 - #63488 (improve DiagnosticBuilder docs)
 - #63493 (Remove unneeded comment in src/libcore/hash/mod.rs)
 - #63499 (handle elision in async fn correctly)
 - #63501 (use `ParamName` to track in-scope lifetimes instead of Ident)
 - #63508 (Do not ICE when synthesizing spans falling inside unicode chars)
 - #63511 (ci: add a check for clock drift)
 - #63512 (Provide map_ok and map_err method for Poll<Option<Result<T, E>>>)
 - #63529 (RELEASES.md: ? is one of three Kleene operators)
 - #63530 (Fix typo in error message.)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Aug 14, 2019
Rollup of 17 pull requests

Successful merges:

 - #62760 (Deduplicate error messages in `librsctc_mir`)
 - #62849 (typeck: Prohibit RPIT types that inherit lifetimes)
 - #63383 (`async fn` lifetime elision tests)
 - #63421 (Implement Clone, Display for ascii::EscapeDefault)
 - #63459 (syntax: account for CVarArgs being in the argument list.)
 - #63475 (Bring back suggestion for splitting `<-` into `< -`)
 - #63485 (ci: move mirrors to their standalone bucket)
 - #63486 (Document `From` trait for `BinaryHeap`)
 - #63488 (improve DiagnosticBuilder docs)
 - #63493 (Remove unneeded comment in src/libcore/hash/mod.rs)
 - #63499 (handle elision in async fn correctly)
 - #63501 (use `ParamName` to track in-scope lifetimes instead of Ident)
 - #63508 (Do not ICE when synthesizing spans falling inside unicode chars)
 - #63511 (ci: add a check for clock drift)
 - #63512 (Provide map_ok and map_err method for Poll<Option<Result<T, E>>>)
 - #63529 (RELEASES.md: ? is one of three Kleene operators)
 - #63530 (Fix typo in error message.)

Failed merges:

r? @ghost
@bors bors merged commit 861d1bb into rust-lang:master Aug 14, 2019
@davidtwco davidtwco deleted the prohibit-inheriting-lifetimes branch August 17, 2019 19:24
Centril added a commit to Centril/rust that referenced this pull request Aug 20, 2019
…amertj

Stabilize `async_await` in Rust 1.39.0

Here we stabilize:
- free and inherent `async fn`s,
- the `<expr>.await` expression form,
- and the `async move? { ... }` block form.

Closes rust-lang#62149.
Closes rust-lang#50547.

All the blockers are now closed.

<details>
- [x] FCP in rust-lang#62149
- [x] rust-lang#61949; PR in rust-lang#62849.
- [x] rust-lang#62517; PR in rust-lang#63376.
- [x] rust-lang#63225; PR in rust-lang#63501
- [x] rust-lang#63388; PR in rust-lang#63499
- [x] rust-lang#63500; PR in rust-lang#63501
- [x] rust-lang#62121 (comment)
    - [x] Some tests for control flow (PR rust-lang#63387):
          - `?`
          - `return` in `async` blocks
          - `break`
    - [x] rust-lang#61775 (comment), i.e. tests for rust-lang#60944 with `async fn`s instead). PR in rust-lang#63383

</details>

r? @cramertj
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 28, 2023
Stabilize `impl_trait_projections`

Closes rust-lang#115659

## TL;DR:

This allows us to mention `Self` and `T::Assoc` in async fn and return-position `impl Trait`, as you would expect you'd be able to.

Some examples:
```rust
#![feature(return_position_impl_trait_in_trait, async_fn_in_trait)]
// (just needed for final tests below)

// ---------------------------------------- //

struct Wrapper<'a, T>(&'a T);

impl Wrapper<'_, ()> {
    async fn async_fn() -> Self {
        //^ Previously rejected because it returns `-> Self`, not `-> Wrapper<'_, ()>`.
        Wrapper(&())
    }

    fn impl_trait() -> impl Iterator<Item = Self> {
        //^ Previously rejected because it mentions `Self`, not `Wrapper<'_, ()>`.
        std::iter::once(Wrapper(&()))
    }
}

// ---------------------------------------- //

trait Trait<'a> {
    type Assoc;
    fn new() -> Self::Assoc;
}
impl Trait<'_> for () {
    type Assoc = ();
    fn new() {}
}

impl<'a, T: Trait<'a>> Wrapper<'a, T> {
    async fn mk_assoc() -> T::Assoc {
        //^ Previously rejected because `T::Assoc` doesn't mention `'a` in the HIR,
        //  but ends up resolving to `<T as Trait<'a>>::Assoc`, which does rely on `'a`.
        // That's the important part -- the elided trait.
        T::new()
    }

    fn a_few_assocs() -> impl Iterator<Item = T::Assoc> {
        //^ Previously rejected for the same reason
        [T::new(), T::new(), T::new()].into_iter()
    }
}

// ---------------------------------------- //

trait InTrait {
    async fn async_fn() -> Self;

    fn impl_trait() -> impl Iterator<Item = Self>;
}

impl InTrait for &() {
    async fn async_fn() -> Self { &() }
    //^ Previously rejected just like inherent impls

    fn impl_trait() -> impl Iterator<Item = Self> {
        //^ Previously rejected just like inherent impls
        [&()].into_iter()
    }
}
```

## Technical:

Lifetimes in return-position `impl Trait` (and `async fn`) are duplicated as early-bound generics local to the opaque in order to make sure we are able to substitute any late-bound lifetimes from the function in the opaque's hidden type. (The [dev guide](https://rustc-dev-guide.rust-lang.org/return-position-impl-trait-in-trait.html#aside-opaque-lifetime-duplication) has a small section about why this is necessary -- this was written for RPITITs, but it applies to all RPITs)

Prior to rust-lang#103491, all of the early-bound lifetimes not local to the opaque were replaced with `'static` to avoid issues where relating opaques caused their *non-captured* lifetimes to be related. This `'static` replacement led to strange and possibly unsound behaviors (rust-lang#61949 (comment)) (rust-lang#53613) when referencing the `Self` type alias in an impl or indirectly referencing a lifetime parameter via a projection type (via a `T::Assoc` projection without an explicit trait), since lifetime resolution is performed on the HIR, when neither `T::Assoc`-style projections or `Self` in impls are expanded.

Therefore an error was implemented in rust-lang#62849 to deny this subtle behavior as a known limitation of the compiler. It was attempted by `@cjgillot` to fix this in rust-lang#91403, which was subsequently unlanded. Then it was re-attempted to much success (🎉) in rust-lang#103491, which is where we currently are in the compiler.

The PR above (rust-lang#103491) fixed this issue technically by *not* replacing the opaque's parent lifetimes with `'static`, but instead using variance to properly track which lifetimes are captured and are not. The PR gated any of the "side-effects" of the PR behind a feature gate (`impl_trait_projections`) presumably to avoid having to involve T-lang or T-types in the PR as well. `@cjgillot` can clarify this if I'm misunderstanding what their intention was with the feature gate.

Since we're not replacing (possibly *invariant*!) lifetimes with `'static` anymore, there are no more soundness concerns here. Therefore, this PR removes the feature gate.

Tests:
* `tests/ui/async-await/feature-self-return-type.rs`
* `tests/ui/impl-trait/feature-self-return-type.rs`
* `tests/ui/async-await/issues/issue-78600.rs`
* `tests/ui/impl-trait/capture-lifetime-not-in-hir.rs`

---

r? cjgillot on the impl (not much, just removing the feature gate)

I'm gonna mark this as FCP for T-lang and T-types.
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 28, 2023
Stabilize `impl_trait_projections`

Closes rust-lang#115659

## TL;DR:

This allows us to mention `Self` and `T::Assoc` in async fn and return-position `impl Trait`, as you would expect you'd be able to.

Some examples:
```rust
#![feature(return_position_impl_trait_in_trait, async_fn_in_trait)]
// (just needed for final tests below)

// ---------------------------------------- //

struct Wrapper<'a, T>(&'a T);

impl Wrapper<'_, ()> {
    async fn async_fn() -> Self {
        //^ Previously rejected because it returns `-> Self`, not `-> Wrapper<'_, ()>`.
        Wrapper(&())
    }

    fn impl_trait() -> impl Iterator<Item = Self> {
        //^ Previously rejected because it mentions `Self`, not `Wrapper<'_, ()>`.
        std::iter::once(Wrapper(&()))
    }
}

// ---------------------------------------- //

trait Trait<'a> {
    type Assoc;
    fn new() -> Self::Assoc;
}
impl Trait<'_> for () {
    type Assoc = ();
    fn new() {}
}

impl<'a, T: Trait<'a>> Wrapper<'a, T> {
    async fn mk_assoc() -> T::Assoc {
        //^ Previously rejected because `T::Assoc` doesn't mention `'a` in the HIR,
        //  but ends up resolving to `<T as Trait<'a>>::Assoc`, which does rely on `'a`.
        // That's the important part -- the elided trait.
        T::new()
    }

    fn a_few_assocs() -> impl Iterator<Item = T::Assoc> {
        //^ Previously rejected for the same reason
        [T::new(), T::new(), T::new()].into_iter()
    }
}

// ---------------------------------------- //

trait InTrait {
    async fn async_fn() -> Self;

    fn impl_trait() -> impl Iterator<Item = Self>;
}

impl InTrait for &() {
    async fn async_fn() -> Self { &() }
    //^ Previously rejected just like inherent impls

    fn impl_trait() -> impl Iterator<Item = Self> {
        //^ Previously rejected just like inherent impls
        [&()].into_iter()
    }
}
```

## Technical:

Lifetimes in return-position `impl Trait` (and `async fn`) are duplicated as early-bound generics local to the opaque in order to make sure we are able to substitute any late-bound lifetimes from the function in the opaque's hidden type. (The [dev guide](https://rustc-dev-guide.rust-lang.org/return-position-impl-trait-in-trait.html#aside-opaque-lifetime-duplication) has a small section about why this is necessary -- this was written for RPITITs, but it applies to all RPITs)

Prior to rust-lang#103491, all of the early-bound lifetimes not local to the opaque were replaced with `'static` to avoid issues where relating opaques caused their *non-captured* lifetimes to be related. This `'static` replacement led to strange and possibly unsound behaviors (rust-lang#61949 (comment)) (rust-lang#53613) when referencing the `Self` type alias in an impl or indirectly referencing a lifetime parameter via a projection type (via a `T::Assoc` projection without an explicit trait), since lifetime resolution is performed on the HIR, when neither `T::Assoc`-style projections or `Self` in impls are expanded.

Therefore an error was implemented in rust-lang#62849 to deny this subtle behavior as a known limitation of the compiler. It was attempted by `@cjgillot` to fix this in rust-lang#91403, which was subsequently unlanded. Then it was re-attempted to much success (🎉) in rust-lang#103491, which is where we currently are in the compiler.

The PR above (rust-lang#103491) fixed this issue technically by *not* replacing the opaque's parent lifetimes with `'static`, but instead using variance to properly track which lifetimes are captured and are not. The PR gated any of the "side-effects" of the PR behind a feature gate (`impl_trait_projections`) presumably to avoid having to involve T-lang or T-types in the PR as well. `@cjgillot` can clarify this if I'm misunderstanding what their intention was with the feature gate.

Since we're not replacing (possibly *invariant*!) lifetimes with `'static` anymore, there are no more soundness concerns here. Therefore, this PR removes the feature gate.

Tests:
* `tests/ui/async-await/feature-self-return-type.rs`
* `tests/ui/impl-trait/feature-self-return-type.rs`
* `tests/ui/async-await/issues/issue-78600.rs`
* `tests/ui/impl-trait/capture-lifetime-not-in-hir.rs`

---

r? cjgillot on the impl (not much, just removing the feature gate)

I'm gonna mark this as FCP for T-lang and T-types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.