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

Better error and quick fix for missing await #30646

Closed
5 tasks done
bterlson opened this issue Mar 29, 2019 · 7 comments · Fixed by #32356
Closed
5 tasks done

Better error and quick fix for missing await #30646

bterlson opened this issue Mar 29, 2019 · 7 comments · Fixed by #32356
Assignees
Labels
Domain: Error Messages The issue relates to error messaging Experience Enhancement Noncontroversial enhancements Suggestion An idea for TypeScript

Comments

@bterlson
Copy link
Member

bterlson commented Mar 29, 2019

Search Terms

missing await error refactoring

Suggestion

Missing an await can cause some confusion especially for people who haven't used async/await before. A helpful error message when you have a Promise<T> instead of T that suggests maybe an await is missing might help address this. There are at least a few common cases this comes up:

  • Using operators, and T is a primitive type. If T were say an object type, it seems more likely something else is wrong with the code to me.
  • Passing to a function, and T is a valid parameter at that position.
  • for-of and T is some array type
  • The target of a member expression and T has the member (or maybe the property name is not in Promise).

A quick fix could also be considered. When in an async function and an error such as above appears, a quick fix that adds an await before the source of the promise within that function.

Use Cases

It helps people realize when they need an await.

Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.
@bterlson bterlson changed the title Better error and quick fix Mar 29, 2019
@bterlson
Copy link
Member Author

bterlson commented Apr 1, 2019

Ugh, realized html ate a <T> above. The request should hopefully be more clear now.

@DanielRosenwasser
Copy link
Member

It's funny because I filed #22923 but not this I guess?

@DanielRosenwasser DanielRosenwasser added Domain: Error Messages The issue relates to error messaging Experience Enhancement Noncontroversial enhancements labels Apr 5, 2019
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 3.6.0 milestone May 25, 2019
@RyanCavanaugh RyanCavanaugh added the Suggestion An idea for TypeScript label Jun 3, 2019
@voliva
Copy link

voliva commented Jun 4, 2019

Another suggestion related to this one is the opposite: When there's "await" on things that are not Promises.

const calculateSum = (a: number, b: number) => a + b;

async function someAsyncFn() {
     // ...
    const result = await calculateSum(a, b);
    // ...
}

Those are definitely not errors, but I think it helps on readability if synchronous stuff is kept in a synchronous way.
Keep in mind that in case calculateSum returns number | Promise<number> then this warning shouldn't show up.

I'm really not sure if this can/should be added into this issue - For one part it feels closely related (using or not using await on promises) but on the other hand the original issue is tackling something that can potentially lead to errors whereas this one is just.... style.

Yes, now I'm more convinced this shouldn't be here.

@movedoa
Copy link

movedoa commented Jun 17, 2019

@voliva Since most of the legwork should already be done at that point i see no reson to not add this.

@ghiscoding
Copy link

ghiscoding commented Jun 23, 2019

Another suggestion related to this one is the opposite: When there's "await" on things that are not Promises.

const calculateSum = (a: number, b: number) => a + b;

async function someAsyncFn() {
     // ...
    const result = await calculateSum(a, b);
    // ...
}

Personally I would prefer to see a warning (or an error) on the const result = await calculateSum(a, b); telling the user that the await is unnecessary since TypeScript already knows, by inference, that the outcome of that call is synchronous and there is nothing to await. That would push the user to cleanup his code and remove the unnecessary async/await all together.

@aslatter
Copy link

Would a missing-await from an expression-statement also be in-scope? For example:

foo(); // foo: () => Promise<void>

Or is that a whole different class of detectable problems?

@andrewbranch
Copy link
Member

@aslatter it’s perfectly well detectable, but unlike the other cases mentioned here, it’s not an error. It’s possibly something we could add a suggestion diagnostic for (like unnecessary await at #32363), but I think it’s even less clear than the unnecessary await case that it’s a mistake. It could very well be a “fire and forget” Promise for something that needn’t interfere with the primary control flow path of the program and doesn’t resolve to anything that you care to observe, e.g. firing a telemetry request or setting something non-critical in a Redis cache. You should probably still have a .catch in cases like that to avoid unhandled rejections, but I believe I’ve seen linter rules that look for that kind of thing exactly.

That said, it is extremely easy to introduce hard-to-find bugs by writing exactly that when you did mean to await it, so I think a suggestion might be nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Error Messages The issue relates to error messaging Experience Enhancement Noncontroversial enhancements Suggestion An idea for TypeScript
8 participants