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

&self lifetime elision with two parameters does not work with async fn #63388

Closed
Centril opened this issue Aug 8, 2019 · 7 comments · Fixed by #63499
Closed

&self lifetime elision with two parameters does not work with async fn #63388

Centril opened this issue Aug 8, 2019 · 7 comments · Fixed by #63499
Assignees
Labels
A-async-await Area: Async & Await A-inference Area: Type inference A-lifetimes Area: Lifetimes / regions A-typesystem Area: The type system AsyncAwait-Polish Async-await issues that are part of the "polish" area C-bug Category: This is a bug. requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Centril
Copy link
Contributor

Centril commented Aug 8, 2019

Originally reported in #63383 (comment).

#![feature(async_await)]

#![feature(nll)]
// Without it you also get //~^ ERROR cannot infer an appropriate lifetime

struct A;

impl A {
    async fn foo(&self, f: &u32) -> &A {
        self
    }
}

should compile (it does if you remove async or write async fn foo<'a, 'b>(&'a self, f: &'b u32) -> &'a A) but does not (playground):

error[E0106]: missing lifetime specifier
 --> src/lib.rs:7:37
  |
7 |     async fn foo(&self, f: &u32) -> &A {
  |                                     ^
  |
  = note: return-position elided lifetimes require exactly one input-position elided lifetime, found multiple.

This seems like a rather serious usability bug as compared to what you expect from normal Rust.

cc #63209
cc @nikomatsakis @cramertj

@Centril Centril added A-typesystem Area: The type system A-lifetimes Area: Lifetimes / regions T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-inference Area: Type inference C-bug Category: This is a bug. A-async-await Area: Async & Await AsyncAwait-Polish Async-await issues that are part of the "polish" area F-async_await requires-nightly This issue requires a nightly compiler in some way. labels Aug 8, 2019
@Centril Centril changed the title &self lifetime elision with two arguments does not work with async fn &self lifetime elision with two parameters does not work with async fn Aug 8, 2019
@cramertj
Copy link
Member

cramertj commented Aug 8, 2019

The code that calculates LtReplacement in lowering is wrong-- it should give preference to the lifetime coming from self over other lifetimes from the function signature.

@nikomatsakis
Copy link
Contributor

OK, so, a few thoughts:

I agree this is a serious usability bug. It's not a forward a compatibility hazard, though.

Regarding the code itself, it's a shame that we have to duplicate the elision code -- there are currently two bits of code doing the same check. The one in lowering is the one giving us the error here. The error is also phrased slightly differently than the one from lifetime resolution.

We could probably get this to work correctly for &self and &mut self. The logic for cases like self: &Self, as we've seen, is somewhat more complex, and before trying to handle that, I'd want to see if we can't reduce the duplication/

I feel like ultimately I'd hate to hold off on shipping async-await owing to this bug, but it'd be good to fix it.

@nikomatsakis
Copy link
Contributor

@cramertj so I guess the buggy code is this:

// Calculate the `LtReplacement` to use for any return-position elided
// lifetimes based on the elided lifetime parameters introduced in the args.
let lt_replacement = get_elided_lt_replacement(
&self.lifetimes_to_define[lifetime_count_before_args..]
);

presumably the way to make it work for &self and &mut self would be to check the implicit_self field of the decl parameter. If it is ImmRef or MutRef, then I would guess we should just use LtReplacement::Some(self.lifetimes_to_define[lifetime_count_before_args]) always.

@nikomatsakis
Copy link
Contributor

Er, my mistake, you can't match on the implicit_self field, but rather promote this computation earlier:

implicit_self: decl.inputs.get(0).map_or(
hir::ImplicitSelfKind::None,
|arg| {
let is_mutable_pat = match arg.pat.node {
PatKind::Ident(BindingMode::ByValue(mt), _, _) |
PatKind::Ident(BindingMode::ByRef(mt), _, _) =>
mt == Mutability::Mutable,
_ => false,
};
match arg.ty.node {
TyKind::ImplicitSelf if is_mutable_pat => hir::ImplicitSelfKind::Mut,
TyKind::ImplicitSelf => hir::ImplicitSelfKind::Imm,
// Given we are only considering `ImplicitSelf` types, we needn't consider
// the case where we have a mutable pattern to a reference as that would
// no longer be an `ImplicitSelf`.
TyKind::Rptr(_, ref mt) if mt.ty.node.is_implicit_self() &&
mt.mutbl == ast::Mutability::Mutable =>
hir::ImplicitSelfKind::MutRef,
TyKind::Rptr(_, ref mt) if mt.ty.node.is_implicit_self() =>
hir::ImplicitSelfKind::ImmRef,
_ => hir::ImplicitSelfKind::None,
}
},
),

@nikomatsakis
Copy link
Contributor

(TBH I'm not 100% sure this code is right, so I'm doing a quick test locally to see if I'm remembering things correctly.)

@nikomatsakis
Copy link
Contributor

Actually, the behavior of the current code is (I think) worse than the issue suggests. If I understand what it's doing -- it is looking for exactly one lifetime parameter that was not explicitly declared, and using that as the elided lifetime.

so for example this code compiles which should not (playground):

// build-pass (FIXME(62277): could be check-pass?)
// edition:2018

#![feature(async_await)]

struct Xyz {
    a: u64,
}

trait Foo {}

impl Xyz {
    async fn do_sth<'a>(
        &'a self, foo: &dyn Foo
    ) -> &dyn Foo
    {
        foo
    }
}

fn main() {}

as does this (playground):

// build-pass (FIXME(62277): could be check-pass?)
// edition:2018

#![feature(async_await)]

struct Xyz {
    a: u64,
}

trait Foo {}

impl Xyz {
    async fn do_sth<'a>(
        foo: &dyn Foo, bar: &'a dyn Foo
    ) -> &dyn Foo
    {
        foo
    }
}

fn main() {}

@nikomatsakis
Copy link
Contributor

I have a nice fix now.

@nikomatsakis nikomatsakis self-assigned this Aug 13, 2019
Centril added a commit to Centril/rust that referenced this issue Aug 13, 2019
…elision-self-mut-self, r=cramertj

handle elision in async fn correctly

We now always make fresh lifetimne parameters for all elided
lifetimes, whether they are in the inputs or outputs. But then
we generate `'_` in the case of elided lifetimes from the outputs.

Example:

```rust
async fn foo<'a>(x: &'a u32) -> &u32 { .. }
```

becomes

```rust
type Foo<'a, 'b> = impl Future<Output = &'b u32>;
fn foo<'a>(x: &'a u32) -> Foo<'a, '_>
```

Fixes rust-lang#63388
Centril added a commit to Centril/rust that referenced this issue Aug 14, 2019
…elision-self-mut-self, r=cramertj

handle elision in async fn correctly

We now always make fresh lifetimne parameters for all elided
lifetimes, whether they are in the inputs or outputs. But then
we generate `'_` in the case of elided lifetimes from the outputs.

Example:

```rust
async fn foo<'a>(x: &'a u32) -> &u32 { .. }
```

becomes

```rust
type Foo<'a, 'b> = impl Future<Output = &'b u32>;
fn foo<'a>(x: &'a u32) -> Foo<'a, '_>
```

Fixes rust-lang#63388
Centril added a commit to Centril/rust that referenced this issue Aug 14, 2019
…elision-self-mut-self, r=cramertj

handle elision in async fn correctly

We now always make fresh lifetimne parameters for all elided
lifetimes, whether they are in the inputs or outputs. But then
we generate `'_` in the case of elided lifetimes from the outputs.

Example:

```rust
async fn foo<'a>(x: &'a u32) -> &u32 { .. }
```

becomes

```rust
type Foo<'a, 'b> = impl Future<Output = &'b u32>;
fn foo<'a>(x: &'a u32) -> Foo<'a, '_>
```

Fixes rust-lang#63388
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
vsrinivas pushed a commit to vsrinivas/fuchsia that referenced this issue Nov 7, 2019
Rust previously had a bug [1] where these lifetimes were incorrectly
inferred, and required manual annotations. The rust issue has now been
resolved, so we can now elide these lifetimes.

[1]: rust-lang/rust#63388

Change-Id: I26ceead7fe5e4165b07280705567fbef0e56c7b0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await A-inference Area: Type inference A-lifetimes Area: Lifetimes / regions A-typesystem Area: The type system AsyncAwait-Polish Async-await issues that are part of the "polish" area C-bug Category: This is a bug. requires-nightly This issue requires a nightly compiler in some way. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants