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

@nikomatsakis

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

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.

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.

@cramertj

@cramertj so I guess the buggy code is this:

rust/src/librustc/hir/lowering.rs

Lines 2159 to 2163 in 60960a2

    
           // 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.

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

rust/src/librustc/hir/lowering.rs

Lines 2190 to 2214 in 60960a2

    
           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, 
        
                   } 
        
               }, 
        
           ),

(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.)

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() {}

I have a nice fix now.

Rollup merge of rust-lang#63499 - nikomatsakis:issuee-63388-async-fn-…

Rollup merge of rust-lang#63209 - Centril:stabilize-async-await, r=cr…

Navigation Menu

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

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	// 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..]
	);

	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,
	}
	},
	),

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

Description

Activity

cramertj commented on Aug 8, 2019

nikomatsakis commented on Aug 12, 2019

nikomatsakis commented on Aug 12, 2019

nikomatsakis commented on Aug 12, 2019

nikomatsakis commented on Aug 12, 2019

nikomatsakis commented on Aug 12, 2019

nikomatsakis commented on Aug 12, 2019

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions

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