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

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

Closed
CGQAQ opened this issue Nov 25, 2019 · 21 comments · Fixed by #90939
Assignees
Labels
A-async-await A-diagnostics AsyncAwait-Triaged C-enhancement D-papercut E-medium E-mentor F-on_unimplemented P-medium T-compiler

Comments

@CGQAQ
Copy link

@CGQAQ CGQAQ commented Nov 25, 2019

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

@CGQAQ CGQAQ changed the title More friendly error msg when await on NONE ASYNC fn/block More friendly error msg when await on NONE ASYNC fn/block or return a obj that implements deprecated Future Nov 25, 2019
@estebank estebank added A-async-await A-diagnostics T-compiler F-on_unimplemented D-papercut labels Nov 25, 2019
@estebank
Copy link
Contributor

@estebank estebank commented Nov 26, 2019

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

@Areredify
Copy link
Contributor

@Areredify Areredify commented Nov 26, 2019

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

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Dec 3, 2019

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 nikomatsakis added AsyncAwait-OnDeck AsyncAwait-Triaged labels Dec 3, 2019
@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Dec 3, 2019

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
Copy link
Contributor

@nikomatsakis nikomatsakis commented Dec 3, 2019

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
Copy link
Contributor

@estebank estebank commented Dec 3, 2019

CC #61076

@JohnTitor JohnTitor added the C-enhancement label Jan 12, 2020
@tmandry tmandry added this to To do in wg-async work Feb 11, 2020
@tmandry tmandry added the P-medium label Mar 3, 2020
@tmandry tmandry removed this from To do in wg-async work Mar 3, 2020
@tmandry tmandry added E-medium E-mentor labels Mar 3, 2020
@nellshamrell nellshamrell self-assigned this May 4, 2020
@tmandry tmandry added this to On deck in wg-async work via automation May 5, 2020
@tmandry tmandry moved this from On deck to Claimed in wg-async work May 5, 2020
@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jun 15, 2020

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:

/// match unsafe { ::std::future::Future::poll(
/// <::std::pin::Pin>::new_unchecked(&mut pinned),
/// ::std::future::get_context(task_context),
/// ) } {

but what we really want is the expression here:

/// match <expr> {

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:

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

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
Copy link
Contributor

@estebank estebank commented Jun 16, 2020

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
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jun 16, 2020

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

@tmandry tmandry moved this from Claimed to In progress in wg-async work Jun 16, 2020
@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jun 16, 2020

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

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Mar 13, 2021

I was talking to @henryboisdequin a bit on Zulip and wanted to leave a few notes for detailed reference:

Roughly the code we want to be editing is

fn report_selection_error(
&self,
obligation: &PredicateObligation<'tcx>,
error: &SelectionError<'tcx>,
fallback_has_occurred: bool,
points_at_arg: bool,
) {

and the general idea is something like

  • notice that this is an error about the Future trait
  • check out the span (the user code it came from) and see if is a "DesugaringKind::Await" span
  • if so, extract the hir-id of the await call and check what kind of expression it is
  • if it is a function call, check what function it is calling and see if it's an "async fn"
  • if not, suggest making it one

@tmandry tmandry moved this from On deck to Claimed in wg-async work Jul 2, 2021
@tmandry
Copy link
Contributor

@tmandry tmandry commented Jul 2, 2021

Doing triage cleanup.

@rustbot release-assignment

@henryboisdequin if you're still working on this, feel free to re-claim

@tmandry tmandry moved this from Claimed to On deck in wg-async work Jul 2, 2021
@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jul 2, 2021

Here is my memory of how far we got:

@henryboisdequin was investigating and discovered that adding the HirId to DesugaringKind was a bit harder than it looks, because that type is defined in rustc_span, but HirId is defined in rustc_hir (which depends on rustc_span). We pursued the idea of adding a (u32, u32) into the span and converting that to a HIR, but it didn't feel very nice.

It occurs to me that we we could also split out the definition of the various "ids" from rustc_hir and move them into a crate that both rustc_hir and rustc_span can reference, I don't know whether @henryboisdequin tried anything like that (can't remember).

I think at this point I would be ok with either solution, but it's probably worth trying to split out the types first.

@estebank
Copy link
Contributor

@estebank estebank commented Sep 16, 2021

Current output:

error[E0277]: `()` is not a future
  --> src/lib.rs:7:5
   |
7  |     boo().await;
   |     ^^^^^^^^^^^ `()` is not a future
   |
   = help: the trait `Future` is not implemented for `()`
note: required by `poll`

Desired output:


error[E0277]: `()` is not a future
  --> src/lib.rs:7:5
   |
7  |     boo().await;
   |     ^^^^^^^^^^^ `()` is not a future
   |
   = note: `()` cannot be `await`ed
help: consider removing the yield point
   |
7  -     boo().await;
7  +     boo();
   |
help: otherwise, consider making `boo` return a `Future` by turning it into an async function
  |
3 | async fn boo() {}
  | +++++

@estebank
Copy link
Contributor

@estebank estebank commented Oct 14, 2021

@rustbot claim

@estebank estebank moved this from On deck to Claimed in wg-async work Oct 14, 2021
estebank added a commit to estebank/rust that referenced this issue Nov 16, 2021
Keep track of the origin of a `T: Future` obligation when caused by an
`.await` expression.

Address rust-lang#66731.
@estebank
Copy link
Contributor

@estebank estebank commented Nov 16, 2021

Opened #90939. Identifying that the .awaited expression is an fn call is still outstanding.

Edit: fully addressed:

error[E0277]: `()` is not a future
  --> $DIR/unnecessary-await.rs:9:10
   |
LL |     boo().await;
   |     -----^^^^^^ `()` is not a future
   |     |
   |     this call returns `()`
   |
   = help: the trait `Future` is not implemented for `()`
help: do not `.await` the expression
   |
LL -     boo().await;
LL +     boo();
   | 
help: alternatively, consider making `fn boo` asynchronous
   |
LL | async fn boo () {}
   | +++++

@estebank estebank moved this from Claimed to In progress (current sprint) in wg-async work Nov 16, 2021
estebank added a commit to estebank/rust that referenced this issue Nov 16, 2021
Keep track of the origin of a `T: Future` obligation when caused by an
`.await` expression.

Address rust-lang#66731.
estebank added a commit to estebank/rust that referenced this issue Nov 17, 2021
Keep track of the origin of a `T: Future` obligation when caused by an
`.await` expression.

Address rust-lang#66731.
estebank added a commit to estebank/rust that referenced this issue Nov 20, 2021
Keep track of the origin of a `T: Future` obligation when caused by an
`.await` expression.

Address rust-lang#66731.
estebank added a commit to estebank/rust that referenced this issue Nov 22, 2021
Keep track of the origin of a `T: Future` obligation when caused by an
`.await` expression.

Address rust-lang#66731.
estebank added a commit to estebank/rust that referenced this issue Dec 2, 2021
Keep track of the origin of a `T: Future` obligation when caused by an
`.await` expression.

Address rust-lang#66731.
estebank added a commit to estebank/rust that referenced this issue Dec 10, 2021
Keep track of the origin of a `T: Future` obligation when caused by an
`.await` expression.

Address rust-lang#66731.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 11, 2021
Tweak errors coming from `for`-loop, `?` and `.await` desugaring

 * Suggest removal of `.await` on non-`Future` expression
 * Keep track of obligations introduced by desugaring
 * Remove span pointing at method for obligation errors coming from desugaring
 * Point at called local sync `fn` and suggest making it `async`

```
error[E0277]: `()` is not a future
  --> $DIR/unnecessary-await.rs:9:10
   |
LL |     boo().await;
   |     -----^^^^^^ `()` is not a future
   |     |
   |     this call returns `()`
   |
   = help: the trait `Future` is not implemented for `()`
help: do not `.await` the expression
   |
LL -     boo().await;
LL +     boo();
   |
help: alternatively, consider making `fn boo` asynchronous
   |
LL | async fn boo () {}
   | +++++
```

Fix rust-lang#66731.
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 12, 2021
Tweak errors coming from `for`-loop, `?` and `.await` desugaring

 * Suggest removal of `.await` on non-`Future` expression
 * Keep track of obligations introduced by desugaring
 * Remove span pointing at method for obligation errors coming from desugaring
 * Point at called local sync `fn` and suggest making it `async`

```
error[E0277]: `()` is not a future
  --> $DIR/unnecessary-await.rs:9:10
   |
LL |     boo().await;
   |     -----^^^^^^ `()` is not a future
   |     |
   |     this call returns `()`
   |
   = help: the trait `Future` is not implemented for `()`
help: do not `.await` the expression
   |
LL -     boo().await;
LL +     boo();
   |
help: alternatively, consider making `fn boo` asynchronous
   |
LL | async fn boo () {}
   | +++++
```

Fix rust-lang#66731.
estebank added a commit to estebank/rust that referenced this issue Dec 13, 2021
Keep track of the origin of a `T: Future` obligation when caused by an
`.await` expression.

Address rust-lang#66731.
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 14, 2021
Tweak errors coming from `for`-loop, `?` and `.await` desugaring

 * Suggest removal of `.await` on non-`Future` expression
 * Keep track of obligations introduced by desugaring
 * Remove span pointing at method for obligation errors coming from desugaring
 * Point at called local sync `fn` and suggest making it `async`

```
error[E0277]: `()` is not a future
  --> $DIR/unnecessary-await.rs:9:10
   |
LL |     boo().await;
   |     -----^^^^^^ `()` is not a future
   |     |
   |     this call returns `()`
   |
   = help: the trait `Future` is not implemented for `()`
help: do not `.await` the expression
   |
LL -     boo().await;
LL +     boo();
   |
help: alternatively, consider making `fn boo` asynchronous
   |
LL | async fn boo () {}
   | +++++
```

Fix rust-lang#66731.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Dec 14, 2021
Tweak errors coming from `for`-loop, `?` and `.await` desugaring

 * Suggest removal of `.await` on non-`Future` expression
 * Keep track of obligations introduced by desugaring
 * Remove span pointing at method for obligation errors coming from desugaring
 * Point at called local sync `fn` and suggest making it `async`

```
error[E0277]: `()` is not a future
  --> $DIR/unnecessary-await.rs:9:10
   |
LL |     boo().await;
   |     -----^^^^^^ `()` is not a future
   |     |
   |     this call returns `()`
   |
   = help: the trait `Future` is not implemented for `()`
help: do not `.await` the expression
   |
LL -     boo().await;
LL +     boo();
   |
help: alternatively, consider making `fn boo` asynchronous
   |
LL | async fn boo () {}
   | +++++
```

Fix rust-lang#66731.
wg-async work automation moved this from In progress (current sprint) to Done Dec 15, 2021
flip1995 pushed a commit to flip1995/rust that referenced this issue Dec 17, 2021
Tweak errors coming from `for`-loop, `?` and `.await` desugaring

 * Suggest removal of `.await` on non-`Future` expression
 * Keep track of obligations introduced by desugaring
 * Remove span pointing at method for obligation errors coming from desugaring
 * Point at called local sync `fn` and suggest making it `async`

```
error[E0277]: `()` is not a future
  --> $DIR/unnecessary-await.rs:9:10
   |
LL |     boo().await;
   |     -----^^^^^^ `()` is not a future
   |     |
   |     this call returns `()`
   |
   = help: the trait `Future` is not implemented for `()`
help: do not `.await` the expression
   |
LL -     boo().await;
LL +     boo();
   |
help: alternatively, consider making `fn boo` asynchronous
   |
LL | async fn boo () {}
   | +++++
```

Fix rust-lang#66731.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await A-diagnostics AsyncAwait-Triaged C-enhancement D-papercut E-medium E-mentor F-on_unimplemented P-medium T-compiler
Projects
Development

Successfully merging a pull request may close this issue.

8 participants