Rule proposal: Warn when await keyword might be missing #4553

mindplay-dk · 2022-02-14T14:27:41Z

Description

Add a rule that catches missing await, when the return-type of a function is a Promise.

This was previously proposed as an eslint feature, and rejected because:

Without a type system, that’s just not possible in the most common case, which is when the other async function is in another file.

However, as noted in the original issue:

Some of the meanest kinds of bugs we are running into are race conditions happening at runtime because of accidentally missing await statements.

There is some discussion in the original thread as to why TS itself doesn't type-check and help with this issue - as noted in the thread (before it was closed) the type-system does help when a return-value is assigned somewhere (to a variable or return-type declaration etc.) but I would add that it does not help when the return-type is Promise<void>.

(Just to give an example of unavoidable side-effects which may require Promise<void>: I am working on a Playwright project, and the Playwright API heavily uses Promise<void> to make sure you wait for side-effects you've requested by injecting things into the browser environment - this is just one example, I am sure there are many more, since e.g. DOM relies heavily on global state, side-effects and events.)

Fail

An example was provided with the original eslint issue:

function sleep(seconds) {
  return new Promise((resolve) => setTimeout(resolve, seconds * 1000.0));
}

async function get1() {
  await sleep(1);
  return 1;
}

async function get2(input) {
  await sleep(1);
  return input;
}

async function main() {
  const result1 = await get1();
  const result2 = get2(result1); // 👈 missing await ❌
  console.log(`done: ${result2}`);
}

main();

Pass

function sleep(seconds) {
  return new Promise((resolve) => setTimeout(resolve, seconds * 1000.0));
}

async function get1() {
  await sleep(1);
  return 1;
}

async function get2(input) {
  await sleep(1);
  return input;
}

async function main() {
  const result1 = await get1();
  const result2 = await get2(result1); // 👈 correctly awaited ✅
  console.log(`done: ${result2}`);
}

main();

The text was updated successfully, but these errors were encountered:

Josh-Cena · 2022-02-14T15:03:43Z

What's a good heuristic for "might be missing"?

Your particular example can be shot down today with the restrict-template-expressions rule: Playground

It's not totally uncommon to generate a promise without immediately awaiting it, because you may want to Promise.all several promises later, for example. So I don't think requiring await everywhere is ever worthwhile.

bradzacher · 2022-02-14T16:24:45Z

The problem with pure JS is that you don't have a typed function boundary - so you can easily mess up invisibly.
But with TS the cases where you can mess up are much fewer because it checks things for you - making a single, global lint rule mostly unnecessary.

There is no one rule that handles this, there are a number which do handle this.

restrict-template-expressions, as Josh mentioned, will handle this case as it enforces your template expression args are sanely typed.
no-floating-promises will handle promises that aren't awaited at the top level.
no-misused-promises will handle a lot of bad promise uses.
require-await will make sure you're using await or returning a promise from an Async function.
promise-function-async will make sure your functions are marked as async if they return a promise.

Josh is also right that you can't just warn on an unawaited promise anywhere because there's noting logically wrong with it. It's perfectly valid to assign and use later, or to pass to a function which in turn itself awaits.

mindplay-dk · 2022-02-15T09:31:21Z

I've been severely burned on a project using Playwright, where I forgot to await certain functions that return Promise<void> but have side-effects - this lead to race conditions, such that tests would pass locally, but would randomly fail on the CI server, just because those servers weren't quite as fast.

The problem was very difficult to identify and it's exactly the sort of thing I'd hope a linter would help with.

On this project, I would have rather the linter complained and made me manually ignore false positives - that way, I would be actively making the decision, avoiding the risk of forgetting an await that causes a race condition. The cost of doing that is far less than the time we spent trying to isolate and identify the issue - it's very, very difficult to isolate an error that only occurs 1 out of 5 times, and only a specific system that isn't set up for debugging.

Maybe this is not an inspection for everyone, and not an inspection for every project - projects aren't all the same, and that's why linters are configurable, right? It's definitely something that would help with Playwright projects, and probably others.

I looked through the list of inspections suggested by @bradzacher above - I don't see how these, or any combination of these, would help.

It's not totally uncommon to generate a promise without immediately awaiting it, because you may want to Promise.all several promises later, for example. So I don't think requiring await everywhere is e 8000 ver worthwhile.

I don't think we need to require await everywhere to get a useful inspection for this kind of issue.

So ignore for a minute the example I cross-posted from the original eslint request.

If I had to suggest the simplest possible inspection that would have caught this issue, I guess it would just need to check that you're doing something with a Promise. That is, if you create a Promise, or obtain one from a function call, the inspection would check that you're either assigning it to variable, returning it, awaiting it, or passing it.

So this would be invalid:

new Promise(resolve => { .... }); // ❌ no-unused-promise

Returning a Promise would be valid:

function createPromise() {
  return new Promise(resolve => { .... }); // ✔ returned 
}

createPromise(); // ❌ no-unused-promise

This would be valid because it's assigned - but no-unused-vars would have you covered there:

function stuff() {
  const p = new Promise(resolve => { ... }); // ❌ no-unused-vars
}

This would be valid because it's awaited:

async function stuff() {
  await new Promise(resolve => { ... }); // ✔ awaited
}

And finally, passing the promise somewhere else would be valid:

usePromise(new Promise(resolve => { ... })); // ✔ passed

Again, if the call function doesn't do anything with that promise, no-unused-vars has you covered:

function usePromise(p) {} // ❌ no-unused-vars

The typical Promise.all situation is valid as well:

const $a = makePromiseA(); // ✔ assigned
const $b = makePromiseB(); // ✔ assigned

const [a, b] = await Promise.all([$a, $b]); // ✔ passed to Promise.all()

As far as I can tell, no combination of existing inspections would have caught this issue - and no-unused-promises seems to nicely compliment no-floating-promises and no-misused-promises.

JoshuaKGoldberg · 2022-02-15T18:13:34Z

@mindplay-dk all the examples you posted are violations under @typescript-eslint/no-floating-promises when I try it out locally. See invalid test cases for the rule:

typescript-eslint/packages/eslint-plugin/tests/rules/no-floating-promises.test.ts

Line 410 in 877cc48

invalid: [

Am I missing something?

Note that the typescript-eslint playground is still pretty new and has some issues with typed rules and tsconfig lib settings. I set this up locally with...

// index.ts

new Promise(resolve => { /* ... */ });
// Promises must be awaited, end with a call to .catch,
// end with a call to .then with a rejection handler or
// be explicitly marked as ignored with the `void` operator.
// eslint: @typescript-eslint/no-floating-promises

function createPromise() {
  return new Promise(() => {
    /* ... */
  });
}

createPromise();
// Promises must be awaited, end with a call to .catch,
// end with a call to .then with a rejection handler or
// be explicitly marked as ignored with the `void` operator.
// eslint: @typescript-eslint/no-floating-promises

// .eslintrc.js
module.exports = {
  root: true,
  parser: "@typescript-eslint/parser",
  parserOptions: {
    sourceType: "module",
    project: ["./tsconfig.json"],
  },
  plugins: ["@typescript-eslint"],
  rules: {
    "@typescript-eslint/no-floating-promises": ["error"],
  },
};

// tsconfig.json
{
  "compilerOptions": {
    "lib": ["esnext"],
    "target": "esnext"
  }
}

bradzacher · 2022-02-15T18:48:14Z

Here is a playground that correctly shows both no-floating-promise, no-misused-promises, and no-unused-vars working together to highlight the problems you've been talking about

Again - there's no one rule, but there are an assortment of rules you can use to enforce better standards in your codebase.

I would have rather the linter complained and made me manually ignore false positives

That's your perspective, and you're very much in the minority - for good reason.
False positives make people stop trusting lint rules. If every time you write a promise expression the linter warned on it - it won't make you more careful with them and consider whether or not you've properly used it.
Instead you'll frustrate people, and train them to just ignore and disable the rule without considering its errors.

This is me speaking from experience at Meta. We have all manner of bad, custom, internal lint rules - many of which false positive. People just ignore them and add disable-comments. It actually trains people to start to completely ignore valid, known good lint rules as well.

The same thing applies to type systems - false positive errors in the type system train people to ignore type system errors!

JoshuaKGoldberg · 2022-04-17T18:55:13Z

Since this issue has gone two months without activity and is asking for something that seems to already exists, closing for housekeeping. Please file a new issue if you have a case not covered by existing rules.

mindplay-dk · 2022-04-19T07:12:34Z

Sorry I didn't follow up - I left my job and didn't work on that project anymore.

From the description and playground example it looks like you have these features, and I just wasn't finding them. I'm pretty thorough about reading documentation before submitting an issue - unfortunately, it's no longer fresh in my memory, so I can't say if there was something that could be improved. Sorry for taking up your time and thanks for looking into this. 🙂

mindplay-dk added enhancement: new base rule extension New base rule extension required to handle a TS specific case package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Feb 14, 2022

bradzacher added awaiting response Issues waiting for a reply from the OP or another party and removed triage Waiting for team members to take a look enhancement: new base rule extension New base rule extension required to handle a TS specific case labels Feb 14, 2022

bradzacher mentioned this issue Feb 17, 2022

Playground: incorrect type causing false negatives #4493

Closed

JoshuaKGoldberg closed this as completed Apr 17, 2022

JoshuaKGoldberg added the stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period label Apr 17, 2022

github-actions bot locked as resolved and limited conversation to collaborators May 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Rule proposal: Warn when await keyword might be missing #4553

Rule proposal: Warn when await keyword might be missing #4553

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Rule proposal: Warn when await keyword might be missing #4553

Rule proposal: Warn when await keyword might be missing #4553

Comments

Description

Fail

Pass

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!