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

Comments

@Centril
Copy link
Member

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 referenced this issue Aug 8, 2019
9 of 9 tasks complete

@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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

(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

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

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

Rollup merge of rust-lang#63499 - nikomatsakis:issuee-63388-async-fn-…
…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

Rollup merge of rust-lang#63499 - nikomatsakis:issuee-63388-async-fn-…
…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

Rollup merge of rust-lang#63499 - nikomatsakis:issuee-63388-async-fn-…
…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

@bors bors closed this in #63499 Aug 14, 2019

Centril added a commit to Centril/rust that referenced this issue Aug 20, 2019

Rollup merge of rust-lang#63209 - Centril:stabilize-async-await, r=cr…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.