Rust: More friendly error msg when await on NONE ASYNC fn/block or return a obj that implements deprecated Future

[CGQAQ]

fn bar () -> impl futures::future::Future<Item=(), Error=()> {}

fn boo (){}

async fn foo() -> std::io::Result<()> {
    boo().await;
    bar().await;
    std::io::Result::Ok(())
}

error[E0277]: the trait bound `(): std::future::Future` is not satisfied
   --> src/main.rs:6:5
    |
6   |     boo().await;
    |     ^^^^^^^^^^^ the trait `std::future::Future` is not implemented for `()`

error[E0277]: the trait bound `impl futures::future::Future: std::future::Future` is not satisfied
   --> src/main.rs:7:5
    |
7   |     bar().await;
    |     ^^^^^^^^^^^ the trait `std::future::Future` is not implemented for `impl futures::future::Future`

error: aborting due to 2 previous errors

1. First error should more likeawait need a async function, async block or return a type that impl std::future::Future, did you forget add async before fn boo() ?
2. Second error should more like You are return a Future that has been droped support or deprecated, maybe try to upgrade to latest version of futures crate?
3. if 3rd party crate depends on derpecated crate that uses deprecated Future trait, the compiler should also tell use explicitly by some error message. (e.g. reqwest 0.9.22 depends on futures = "0.1.23")

I think more friendly error message like these will help new comers from javascript -> front end devs to more quickly solve problem, because they don't have multiple Promise implements, current error messages will make them very confusing and frustrating

[estebank]

After #66651 is merged, adding a rustc_on_unimplemented message on std::future::Future with these two cases will be trivial.

[Areredify]

@estebank I don't think it will, enclosing_scope lets you add a message to the foo function, not bar or boo.

[nikomatsakis]

Handling the second case (a future from some older version of the futures crate) doesn't seem worth the effort. I think those will vanish over time. Handling the case like boo().await seems worthwhile.

[nikomatsakis]

Marking as "on-deck", I think that helping people identify cases where they put .await on something that is not a future seems pretty good.

[nikomatsakis]

One note: I'm not sure if "on unimplemented" is the right implementation approach. Ideally, we would suggest that the user make fn boo an async fn boo. To do that, we'd want to look for cases like <function-call>.await. I'm not sure what it would take to identify such a case, we might be able to detect it in type-check and thread in a special "obligation cause" so that we can give a more targeted hint. (But the desugaring may also get in the way here.)

[estebank]

CC https://github.com/rust-lang/rust/issues/61076

[nikomatsakis]

So @nellshamrell and I did a bit of a deep-dive. We added the "on unimplemented" annotation to at least give an error message like "() is not a future", but I was thinking about how we could do better. In particular, I'd like to detect a case where the source of the type is bar().await and report something like:

error[E0277]: the trait bound `(): std::future::Future` is not satisfied
   --> src/main.rs:6:5
    |
6   |     boo().await;
    |     ^^^^^^^^^^^ the trait `std::future::Future` is not implemented for `()`
help: maybe you want to make `boo` an async fn?
3  |   async fn boo (){}

I was looking into how we can do this but it's tricky. Currently, the message we get has the ObligationCauseCode with ItemObligation (i.e., "it came from a where-clause on Future::poll") which is pretty uninteresting to us. Even if we could track the source expression of the obligation, because of desugaring, it wouldn't be that interesting. We would be getting this call to Future::poll, basically:

https://github.com/rust-lang/rust/blob/f315c35a77e40bd11ce81fedc0556be0f410bbf4/src/librustc_ast_lowering/expr.rs#L578-L581

but what we really want is the expression here:

https://github.com/rust-lang/rust/blob/f315c35a77e40bd11ce81fedc0556be0f410bbf4/src/librustc_ast_lowering/expr.rs#L576

However, I did have an interesting idea -- or maybe an evil one? I realized that if we reorder these lines that are part of lowering expr.await, we could potentially embed the hir-id of the "awaited expression" into the DesugaringKind:

https://github.com/rust-lang/rust/blob/f315c35a77e40bd11ce81fedc0556be0f410bbf4/src/librustc_ast_lowering/expr.rs#L606-L612

In other words, this might look something like:

        let expr = self.lower_expr(expr);
        let span = self.mark_span_with_reason(
            DesugaringKind::Await(expr.hir_id), await_span, None);
        let gen_future_span = self.mark_span_with_reason(
            DesugaringKind::Await(expr.hir_id),
            await_span,
            self.allow_gen_future.clone(),
        );

This would then allow us to inspect the "desugaring kind" from within the trait code and uncover the hir-id we are interested in. I imagine a similar technique might be useful for things like the desugaring of foo? and other such expressions. In general, it seems like it's useful, when you see an error that arises out of a desugaring, to be able to pull out and find bits of the desugaring's inputs.

I'm curious @estebank what you think of that idea. At first, it surprised me, because it seemed strange to have DesugaringKind embed specific bits of information about the expression being desugared. But then I was thinking -- why not? Like, what can get more tied to a specific location in the source than a span?

[estebank]

That seems like a potentially quite useful pattern for DesugaringKind that could be leveraged in multiple places. I never thought about doing that because of size concerns, but a single HirId shouldn't have any big adverse effects.

[nikomatsakis]

OK, do you think it merits an MCP? I was debating about it. =)

[nikomatsakis]

@petrochenkov I'm curious to get your reaction to this idea

[nellshamrell]

I am happy to start drafting an MCP (Major Change Proposal)

[nikomatsakis]

@nellshamrell Great. You just file an issue with the MCP template, shouldn't have to be too long or involved.

[tmandry]

@nellshamrell Are you still planning to work on this?

[tmandry]

Clearing assignment since there hasn't been progress, but feel free to re-claim.