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

Inherent async fn returning Self treats type's lifetime parameters as 'static #61949

Open
Arnavion opened this issue Jun 19, 2019 · 17 comments
Open

Comments

@Arnavion
Copy link

@Arnavion Arnavion commented Jun 19, 2019

rustc 1.37.0-nightly (2887008e0 2019-06-12) and playground's 2019-06-17 b25ee644971a168287ee

Playground

#![feature(async_await)]

pub struct Foo<'a> {
    pub bar: &'a i32,
}

impl<'a> Foo<'a> {
    pub async fn new(bar: &'a i32) -> Self {
        Foo {
            bar
        }
    }
}
error[E0495]: cannot infer an appropriate lifetime due to conflicting requirements
 --> src/lib.rs:8:22
  |
8 |     pub async fn new(bar: &'a i32) -> Self {
  |                      ^^^
  |
note: first, the lifetime cannot outlive the lifetime 'a as defined on the impl at 7:6...
 --> src/lib.rs:7:6
  |
7 | impl<'a> Foo<'a> {
  |      ^^
  = note: ...so that the expression is assignable:
          expected &i32
             found &'a i32
  = note: but, the lifetime must be valid for the static lifetime...
  = note: ...so that the types are compatible:
          expected Foo<'_>
             found Foo<'static>

It works to either change the signature to take bar: &'static i32, or to change the body of the fn to use a static borrow like bar: &5. So the compiler really does want the function to return a Foo<'static>, even though Self is a Foo<'a>


The workaround is to not use Self:

-    pub async fn new(bar: &'a i32) -> Self {
+    pub async fn new(bar: &'a i32) -> Foo<'a> {

Mentoring notes: See notes here on Zulip.

@Arnavion
Copy link
Author

@Arnavion Arnavion commented Jun 19, 2019

jebrosen pointed out on IRC that it also happens with RPIT in general, so this might be the same as #53613 (from async fn expanding to -> impl Future<Output = ...>)

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jul 5, 2019

As I pointed out in #53613, there is a more dangerous variant of this involving projections:

#![feature(async_await)]

pub trait HasItem<'a> {
    type Item;
}

// Does not compile:
async fn example1<'a, T: HasItem<'a>>(item: T::Item) -> T::Item {
    item
}


fn main() { }

Note that this version compiles just fine

async fn example1<'a, T: HasItem<'a>>(item: T::Item) -> <T as HasItem<'a>>::Item {
    item
}

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jul 5, 2019

There are some forward compatibility concerns here. In particular, callers can (somewhat incorrectly) assume that the returned futures are 'static. In other words, the callee is committing to returning a future that does not capture 'a in these examples -- right now, that typically means callee doesn't compile, but it could happen that the callee happens to compile, and then callers would be able to use this future in places where 'a is out of scope. If we later fix this bug, those callers will stop compiling.

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jul 5, 2019

The bug involving Self is rather straightforward (but grungy) to fix. It's also possible for us to, temporarily, just create an error if you return a type mentioning Self in an async fn.

But fixing the T::Item case is rather more complex. We don't know until type-checking that T::Item resolves to <T as HasItem<'a>>::Item. And, if I had my druthers, we might not know until even later than that. I suppose we could do a forward compatibility restriction of some kind.

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jul 9, 2019

An example of something which compiles now but I think should not (and would not, if this were fixed):

#![feature(async_await)]

pub struct Foo<'a> {
    pub bar: &'a i32,
}

impl<'a> Foo<'a> {
    pub async fn new(_bar: &'a i32) -> Self {
        Foo {
            bar: &22
        }
    }
}

async fn foo() {
  let x = {
    let bar = 22;
    Foo::new(&bar).await
  };
  drop(x);
}

fn main() { }

the problem here is that the type of x should be Foo<'bar> -- i.e., it should be constrained to the lifetime of bar, but it is actually Foo<'static>.

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jul 9, 2019

I left some notes on how to fix this on Zulip

@davidtwco
Copy link
Member

@davidtwco davidtwco commented Jul 9, 2019

@rustbot claim

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jul 9, 2019

OK so having thought this over I have come to the conclusion that the right behavior here is to give an error if people use Self or T::Foo projections from within an impl Trait such that they wind up "inheriting lifetimes". It's not an ideal solution but it'll permit many uses while forbidding the cases that are causing trouble and giving us time to fix properly.

The way I think we should do this:

During type checking, there is a check_opaque function that currently checks for cyclic opaque types.

In the case of existential types whose origin is either ReturnImplTrait or AsyncFn, we want to enforce this condition.

What we would do is to access the tcx.predicates_of the existential def-id. The predicates field of the result will contain the "bounds" of the impl trait (with the self-type being an opaque type). (Note that we don't want to inspect the parent, which will contain predicates defined in the enclosing scope.)

Actually, we may want to introduce a new query (existential_bounds_of or something) that just returns those predicates that tcx.predicates_of would return..? I guess it's fine to invoke tcx.predicates_of, it just feels a touch fragile. Ah well, that's what tests are for. =)

Anyway, we basically iterate over that predicates field using a type visitor. In there, we look for references to regions that are ReEarlyBound but where the index indicates it is coming from the parent. We would report an error if we found any such region.

One thing to watch out for though while we are visiting the type. If we have something like impl Foo, that will be represented by a predicate O: Foo where O is the opaque identity type. Such a type is constructed here:

let substs = InternalSubsts::identity_for_item(tcx, def_id);
let opaque_ty = tcx.mk_opaque(def_id, substs);

Such a type will contain the illegal regions and we need to ignore it. We basically want to ignore instances of the self-type when visiting -- i.e., we can construct the opaque identity type (call it opaque_identity_ty) and override visit_ty to skip visiting the contents if ty == opaque_identity_ty.

bors added a commit that referenced this issue 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 added a commit that referenced this issue 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
@Centril
Copy link
Contributor

@Centril Centril commented Aug 8, 2019

From Lang team meeting: @cramertj will do a more targeted fix for async fn based on #62849. If crater completes in time then we can generalize #62849.

Centril added a commit to Centril/rust that referenced this issue Aug 12, 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 issue Aug 13, 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 added a commit that referenced this issue Aug 13, 2019
…omatsakis

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
Centril added a commit to Centril/rust that referenced this issue Aug 13, 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 added a commit that referenced this issue Aug 13, 2019
…omatsakis

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
Centril added a commit to Centril/rust that referenced this issue 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
@Arnavion
Copy link
Author

@Arnavion Arnavion commented Nov 7, 2019

Did the new error in #62849 not make it to stable 1.39.0 ? I still see the old error about lifetime being inferred as 'static and not matching, rather than the explicit "async fn return type cannot contain a projection or Self..." error added by that PR.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=06a794508330334476e5b9f298ea233b

Edit: I do see the error from that PR if I change the body to S(&22) instead of S(i), just like the test case in that PR. Was it intentional to only have the new error fire when the input lifetime isn't used in the fn body? I feel like the general case would benefit from it as well.

@SergioBenitez
Copy link
Contributor

@SergioBenitez SergioBenitez commented Feb 4, 2020

I'm very surprised that this is currently shipping in stable due to the forwards/backward compatibility hazards mentioned by @nikomatsakis. The error message ostensibly introduced by #62849 is not triggered, at least for the T::Item case, which means that a lifetime of 'static is being assumed (and the error message indicates as such). Surprisingly, passing -Z unpretty=mir to the compiler results in the error message from #62849 appearing!

Even with the error message, however, I'd be quite surprised if users intuit what the core issue is, in this case, that the compiler is unable to fill in the holes correctly, and that providing more type information resolves the issue. At minimum, an error code with a --explain should be added that indicates how to resolve the error in the interim.

@Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Feb 4, 2020

I believe this is all known information, but unmarking as triaged so that it is seen again by the async WG.

@tmandry
Copy link
Contributor

@tmandry tmandry commented Feb 12, 2020

I did some investigating, leaving notes here.

The reason the example above does not emit the new error message is because the compile fails during type collection and never makes it to type checking. This is a bug fixed by #68884.

With the above patch applied, we now get the new error message that was introduced by #62849. We also get the more confusing error message about inferring appropriate lifetimes (this happens while checking the function body), but I'm not sure if we can avoid that without a lot of effort.

Agreed that the error message could be better. I think we should give this error a hint ("try spelling out the type instead of using Self"), along with an error code / longer description saying that this is a limitation of the compiler.

@tmandry
Copy link
Contributor

@tmandry tmandry commented Feb 18, 2020

Hello from triage.. downgrading from Focus since there is no forward-compat issue. We may keep this as OnDeck for now to track the diagnostics improvement.

@tmandry
Copy link
Contributor

@tmandry tmandry commented Feb 19, 2020

Opened #69276 to track the diagnostics improvement separately.

@Kimundi
Copy link
Member

@Kimundi Kimundi commented Mar 3, 2021

The situation here is still confusing, as the explanation for the error message, https://doc.rust-lang.org/nightly/error-index.html#E0760, talks about async, but the issue also arised with non-async code:

pub struct Foo<'a>(&'a ());

impl<'a> Foo<'a> {
    pub fn works<T>(self) -> impl FnOnce(T) -> Foo<'a> {
        move |_| todo!()
    }
    pub fn errors<T>(self) -> impl FnOnce(T) -> Self {
        move |_| todo!()
    }
}

EDIT: Actually, it talks about both async and impl Trait, it just a very terse sentence that makes it easy to glance over that, while only having example code that uses async.

@tmandry tmandry removed their assignment Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
10 participants