New rule: no-floating-promises

[JoshuaKGoldberg]

Promises must be stored or passed to a function.

The idea here is that floating (unhandled) Promises can cause unexpected behavior. For example, in tests, they might resolve at unexpected times (like after another test following their parent test has started).

Fixes microsoft/tslint-microsoft-contrib#174 (PRing here because I'm not sure they support the type checker).

[JoshuaKGoldberg]

Note: added configurable options in 6f932dd so additional class names of Promises can be added.

[nchen63]

I think populating the rationale property here would be pretty useful. What does it mean to be handled "appropriately"? What happens when it's not handled?

[nchen63]

add test cases for returnsPromiseFunction().then() and calling static Promise functions

[JoshuaKGoldberg]

Following up on this PR: I haven't had time to get to it, but will in the next month.

[andy-hanson]

This should fix #892?

[JoshuaKGoldberg]

@andy-hanson unfortunately no. Specifically ensuring async functions are awaited is a different matter than generally ensuring functions that return Promises have their results dealt with.

The following code should be flagged by 892's solution, but not by this one:

async function getSomething() {
    return Promise.resolve(7);
}

async function runTest() {
    const result = getSomething();
    return result; // you may think this is a number, but it's actually `Promise<number>`
}

[andy-hanson]

Are you referring to syntax? Semantically "async functions" and "functions that return Promises" are the same, right?

I don't think there's anything wrong with the code example you show here. Returning a Promise from an async function is OK. return result; and return await result; should have the same effect, and it won't change the return type since async functions always return Promises anyway.

I interpreted the issue there as just not wanting any foo(); where the return type of foo() is Promise<void> rather than void.

Update: Looks like the issue may be dealt with by TypeScript itself anyway, so no need for lint rules? microsoft/TypeScript#13376 (comment)
But it looks like they will only enforce it inside async functions, while of course we want to avoid doing this anywhere. Nevermind.

[alitaheri]

@andy-hanson Actually it is recommended that you return the promise directly as appending await will create a promise that is resolved with the result of await expression. that means one unnecessary idempotent promise created.

Arguably it should better be disallowed. return await asyncStuff() is not only semantically equivalent to return asyncStuff() but also is less memory/cpu efficient.

[ajafff]

@alitaheri

Arguably it should better be disallowed. return await asyncStuff() is not only semantically equivalent to return asyncStuff() but also is less memory/cpu efficient.

I'm currently working on a rule for this.

[JoshuaKGoldberg]

@nchen63 I believe your feedback has been resolved. Could you please take another look?

I switched from a whitelist of allowed parent types to a blacklist, since there are much more places using a Promise generator would be allowed.

[nchen63]

can you rebase?

[JoshuaKGoldberg]

Rebased, but now the build is failing with node_modules/@types/underscore/index.d.ts(3824,43): error TS2304: Cannot find name 'Element'.. Guessing that's external to this PR?

[nchen63]

try rebuild without cache

[andy-hanson]

Do you know if there's a way to permanently clear the cache in CircleCI? I have to rebuild without cache every time I push.

[JoshuaKGoldberg]

Passed CircleCI, thanks for the tip.

[nchen63]

@JoshuaKGoldberg thanks!

[robfreundlich]

When turning on no-floating-promises, code like the following is getting flagged as a problem:

something_that_produces_a_promise().then(() => fulfillPromise());
function fulfillPromise()
{
  // do some stuff ...
  resolve(whatever);
}

That seems wrong to me - the Promise is getting handled by a then(). Granted, the then() technically returns a Promise, but if TSLint is going to complain about that, then isn't this rule sort of impossible to not trigger?

Am I misunderstanding something, or is this a bug?

Thanks!

[JoshuaKGoldberg]

So your source looks something like this?

return new Promise((resolve, reject) => {
    something_that_produces_a_promise().then(() => fulfillPromise());

    function fulfillPromise() {
        // do some stuff ...
        resolve(whatever);
    }
});

Is there a reason you can't use async/await or rewrite it to not use resolve?

return something_that_produces_a_promise()
    .then(() => fulfillPromise())
    .then(() => {
        // do some stuff ...
        return whatever;
    });

I'm not convinced there's a frequently valid use case for using resolve/reject. async/await simplifies everything, and even if you can't use it yet, it's (as I understand it) more Promise-like to return things through instead of manually calling resolves.

Also, this feels like a bug/feature request that might be better as a separate issue?

[robfreundlich]

I'd love to use async/await, but we have to support (at least for a while) older versions of IE that don't do well when debugging them. /sigh

The code does look like you described, though, and now I'm wondering whether I can just rewrite it to not use resolve(whatever). As long as I declare whatever before the return, it's available (I know, it's available even before its declaration unless I turn on that TSLint rule, which I'd love to, but I got overruled on that debate, but I have using variables before they're declared, but I digress), that will work.

I imagine I could also just return whatever directly from ```fulfillPromise()`` as well, right? (Still figuring out all of the intricacies of Promises).

I'll give that a shot and let you know how it turns out. Thanks!

[sbking]

@JoshuaKGoldberg The following code is flagged:

async function run () {
  await Promise.resolve()
}

run()

How are you supposed to call the top level async function in an application? The following also fails:

run.then(() => console.log('foo'))

What am I missing? Do you have to have a regular JavaScript file to call the top level function which returns a promise? Or just do a tslint ignore comment?

[JoshuaKGoldberg]

Yup, either works. This should ideally only happen once per application.

[andy-hanson]

I think we should allow a promise as long as its errors are caught. See #2774.