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

type inference doesn't work in async fn that return Box<dyn SomeTrait> #60424

Closed
macpp opened this issue Apr 30, 2019 · 14 comments
Closed

type inference doesn't work in async fn that return Box<dyn SomeTrait> #60424

macpp opened this issue Apr 30, 2019 · 14 comments

Comments

@macpp
Copy link

@macpp macpp commented Apr 30, 2019

i have following fn that works completly fine:

pub fn test1() -> Box<dyn Debug> {
    Box::new("asdf")
}

if i try trivialy make it async just by adding async keyword i get build error:

error[E0271]: type mismatch resolving `<impl core::future::future::Future as core::future::future::Future>::Output == std::boxed::Box<(dyn std::fmt::Debug + 'static)>`
  --> comix_downloader_lib_async/src/lib.rs:49:25
   |
49 | pub async fn test1() -> Box<dyn Debug> {
   |                         ^^^^^^^^^^^^^^ expected &str, found trait std::fmt::Debug
   |
   = note: expected type `std::boxed::Box<&str>`
              found type `std::boxed::Box<(dyn std::fmt::Debug + 'static)>`
   = note: the return type of a function must have a statically known size

i can work around this by defining types of variables explicitly:

pub async fn test3() -> Box<dyn Debug> {
    let tmp : Box<dyn Debug> = Box::new("asdf");
    tmp
}

but for obvious reasons this is not as nice as sync version
maybe related to #60414 ?

rustc --version : rustc 1.36.0-nightly (00859e3e6 2019-04-29)

@macpp macpp changed the title type inferance doesn't work in async fn that return Box<dyn SomeTrait> type inference doesn't work in async fn that return Box<dyn SomeTrait> Apr 30, 2019
@cramertj
Copy link
Member

@cramertj cramertj commented May 1, 2019

I'd put this under a similar camp as #54326 -- the issue here is that we need to fix the output of the Future to the return type as provided, rather than trying to infer it from the context (which isn't smart enough in this case to see that a trait object coercion is required).

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jul 9, 2019

Discussed in meeting today. Adding the "unclear" tag: this one is tricky to fix and it's not clear that it should block stabilization. It would definitely be good to have, though. In particular, it seems unlikely to be a forwards compatibility issue, since this is basically a problem of failing to do the coercion that is needed and hence getting more compilation errors (but it would be helpful if people have counterexamples in mind where behavior might change, seems possible).

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jul 25, 2019

Discussed in the rust-lang/lang meeting -- we decided this is, ultimately, not a blocker but we ought to "try real hard" to fix it.

@leo60228
Copy link
Contributor

@leo60228 leo60228 commented Sep 6, 2019

This even more minimal example has the same problem:

async fn func(s: &str) -> &[&str] {
    &[s]
}

@cramertj
Copy link
Member

@cramertj cramertj commented Sep 6, 2019

@leo60228 Yup, you need

async fn func(s: &str) -> &[&str] {
    &[s] as &[&str]
}

which correctly generates an error:

error[E0515]: cannot return value referencing temporary value
 --> src/lib.rs:2:5
  |
2 |     &[s] as &[&str]
  |     ^---^^^^^^^^^^^
  |     ||
  |     |temporary value created here
  |     returns a value referencing data owned by the current function

@leo60228
Copy link
Contributor

@leo60228 leo60228 commented Sep 6, 2019

@cramertj Whoops, I didn't test the non-async version... This does have the problem I intended to demonstrate, though:

async fn func() -> &'static [&'static str] {
    &["hi"]
}

@blitzerr
Copy link
Contributor

@blitzerr blitzerr commented Sep 18, 2019

@nikomatsakis Any notes on this one ?

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Sep 25, 2019

OK, so I have been doing a bit of digging into this. Our previous efforts to fix those were focused on keeping the existing desugaring. But @Centril was suggesting that we should modify the HIR and I think they are correct.

Let's walk through what happens here. First off, I'll run with this example:

async fn func() -> &'static [&'static str] {
    &["hi"]
}

This presently gets desugared to an async block, but in the compiler that async block is itself desugared into a call to from_generator with a "generator closure" as argument:

fn func() -> &'static [&'static str] {
  ::std::future::from_generator(gen || {
    &["hi"]
  })
}

I think what we want to do, in short, is to generate a return type annotation on that generator. You can see that this will fix the compile error in this manually desugared version from the playground.

However, I think the right way to do this is probably to extend the HIR type with some kind of "current function return type" node for internal use that expands to the return type from the current function. The reason to do this is that we can't just "copy-n-paste" the return type from the function declaration into this position -- the references to lifetimes will not work out correctly, unfortunately. In particular, elided lifetimes are handled by a later pass and they would be treated differently in this position.

A few more tips. This desugaring is done here:

let async_expr = this.make_async_expr(
CaptureBy::Value, closure_id, None, body.span,
|this| {
// Create a block from the user's function body:
let user_body = this.lower_block_expr(body);
// Transform into `drop-temps { <user-body> }`, an expression:
let desugared_span = this.mark_span_with_reason(
DesugaringKind::Async,
user_body.span,
None,
);
let user_body = this.expr_drop_temps(
desugared_span,
P(user_body),
ThinVec::new(),
);
// As noted above, create the final block like
//
// ```
// {
// let $param_pattern = $raw_param;
// ...
// drop-temps { <user-body> }
// }
// ```
let body = this.block_all(
desugared_span,
statements.into(),
Some(P(user_body)),
);
this.expr_block(P(body), ThinVec::new())
});
(HirVec::from(parameters), this.expr(body.span, async_expr, ThinVec::new()))
})

This is a call to make_async_expr. Note the 3rd argument, ret_ty, in particular:

ret_ty: Option<AstP<Ty>>,

This argument supplies the return type annotation on the closure. When we invoke it from async fn, it is None. We can't just supply Some, however, because this expects an AST type and we would want to give it a HIR type, I think, at least if we're going to be adding some special HIR node representing the "return type of the current function". I have to dig a bit into this part, another option might be to try and convince the lifetime elision code to do the right thing so that we can just copy-and-paste the type.

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Sep 25, 2019

Ah, actually, I realize something even easier. We already supply a flag to the generator code that indicates this is a generator. If we supply a flag that indicates that this is the "async move from an async fn body", we could supply the correct typing hints in type-check.

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Sep 25, 2019

I may take a stab at this in a few, else I'll poke somebody to give it a try in Zulip.

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Sep 25, 2019

Oh dang it, I just realized the obvious complication -- the return type of the function is actually a future, so what we really need to do is to extract the Output of that. This is going to be an annoying amount of code, though it remains plausible.

@cramertj
Copy link
Member

@cramertj cramertj commented Sep 25, 2019

I'm sure this has already been considered, but this bug would be trivially fixable if it were possible to have types from the AST appear twice in the HIR. It also seems plausible that such a thing would be useful elsewhere. Is there a sensible path to allowing this?

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Oct 2, 2019

Got a pending fix in #64999

Centril added a commit to Centril/rust that referenced this issue Oct 2, 2019
…rn-inference, r=cramertj

extract expected return type for async fn generators

Fixes rust-lang#60424

cc @Centril, I know you've been eager to see this fixed.

r? @cramertj
Centril added a commit to Centril/rust that referenced this issue Oct 3, 2019
…rn-inference, r=cramertj

extract expected return type for async fn generators

Fixes rust-lang#60424

cc @Centril, I know you've been eager to see this fixed.

r? @cramertj
Centril added a commit to Centril/rust that referenced this issue Oct 3, 2019
…rn-inference, r=cramertj

extract expected return type for async fn generators

Fixes rust-lang#60424

cc @Centril, I know you've been eager to see this fixed.

r? @cramertj
@bors bors closed this in cfb6d84 Oct 3, 2019
dvic added a commit to dvic/warp that referenced this issue Jan 22, 2020
The explicit boxing is not needed anymore as rust-lang/rust#60424
has been fixed.
dvic added a commit to dvic/warp that referenced this issue Jan 22, 2020
The cast is not needed anymore as rust-lang/rust#60424
has been fixed.
jxs added a commit to seanmonstar/warp that referenced this issue Jan 23, 2020
The cast is not needed anymore as rust-lang/rust#60424
has been fixed.
tsing added a commit to tsing/async-book that referenced this issue Apr 28, 2020
The bug has been fixed at rust-lang/rust#60424.
cramertj added a commit to rust-lang/async-book that referenced this issue Apr 28, 2020
The bug has been fixed at rust-lang/rust#60424.
huangjj27 added a commit to huangjj27/async-book that referenced this issue Aug 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

7 participants