Tslint: Don't allow no-floating-promises to be ignored using the `void` operator

[JoshuaKGoldberg]

Bug Report

• __TSLint version__: 5.15.0
• __TypeScript version__: 3.4.0
• __Running TSLint via__: CLI

TypeScript code being linted

function activate(): void {
    void this.loadSiteSections(); // Intentional.
}

with tslint.json configuration:

{
    "rules": {
        "no-floating-promises": true
    }
}

See #3020.

Actual behavior

No complaints.

Expected behavior

There should be complaints. void is not an acceptable way to indicate a promise as not floating! If you really want to suppress the rule, use a // tslint:disable statement.

Further indication that the rule needs better docs...!

[glen-84]

void is not an acceptable way to indicate a promise as not floating!

Why not? It's even "recommended" by a TypeScript developer:

There's no need for nowait - the void operator already exists for a consuming an expression while producing no side effects (e.g. you could just write void doSomethingAsync();)

I've used this quite a lot in our application.

[JoshuaKGoldberg]

It's even "recommended" by a TypeScript developer

The section of that discussion you're referring to has a bit different of an intent than this one, I think. Ryan was referring to how you could do such a thing at the syntax level, not recommending its usage in applications.

I've used this quite a lot in our application.

Hmm, if this would actually be a significant breaking change for you and others, we should add it to the rule's config as an option, and make it more lenient by default. How about ban-void-statements?

[JoshuaKGoldberg]

Note: if folks are reading this discussion, this is not recommended:

function fireAndForget(): void {
    void startSomePromise();
}

If you have no-floating-promises enabled and wish to start a floating promise, disable it with // tslint:disable:

function fireAndForget(): void {
    // tslint:disable-next-line
    startSomePromise();
}

See also tslint-microsoft-contrib's void-zero rule.

However, the intent of this rule is that you _do not create floating promises_. It is desirable to not create floating Promises if you need to handle rejected/erroring states, such as in applications with logging. Instead of the literal interpretation of fire-and-forget in the two examples above, it is recommended you have some kind of function/class that takes in the otherwise-floating promises and handles their rejection:

function fireAndForget(): void {
    wrapBackgroundTask(startSomePromise());
}

// ...

function wrapBackgroundTask(promise) {
    promise.catch(error => console.error("Oh no!", error));
}

_(splitting into a separate message so the direct response above is shorter)_

[glen-84]

@JoshuaKGoldberg,

The section of that discussion you're referring to has a bit different of an intent than this one, I think.

If you look at the last paragraph of that issue description, the OP was requesting a keyword to be used to signal to the TypeScript compiler that it should not warn about a method being called without await, which sounds very similar to the TSLint no-floating-promises rule. Ryan stated that void could be used for this purpose (see also https://github.com/Microsoft/TypeScript/issues/13376#issuecomment-274662923).

Hmm, if this would actually be a significant breaking change for you and others ...

Well, we'll have to replace 85-90 occurrences of void with something like:

// tslint:disable-next-line:no-floating-promises -- Reason for doing this.

It's certainly a lot more verbose 🙂. Using an underscore prefix to suppress an unused parameter error in TypeScript is also more terse than using // @ts-ignore all over the place.

Note: if folks are reading this discussion, this is not recommended:

insert "y tho" meme – are there cases where you'd use void here and not want it to suppress the no-floating-promises rule?

See also tslint-microsoft-contrib's void-zero rule.

That seems unrelated, and I agree that using void 0 instead of undefined outside of minification is just weird.

It is desirable to not create floating Promises if you need to handle rejected/erroring states ...

What if the Promise-returning function handles all errors internally? Like:

async function go() {
    try {
        const result = await asyncThing();
    } catch (e) {           // This would catch everything, right?
        console.log(e);
    }
}

void go();

---

Anyway, if you decide to go ahead with this change, then I'll respect that decision and make the necessary changes. I don't know how many other developers it will affect though.

[adidahiya]

Sounds like the stricter behavior should at least have an opt-out... if we make the behavior change we should add a rule option called something like allow-void-expression

[rshea0]

Our case for needing floating promises is a bit different. Some top level files like our entry point need to fire an async function to start the app. If you're going to make void expressions not allowed, maybe have an option to allow floating promises if an await wouldn't normally be allowed in that context, such as outside any containing function.

async function bootstrap() {
  const app = await NestFactory.create(AppModule);

  await app.listen(4000);
}

// tslint:disable-next-line: no-floating-promises
bootstrap();

I'm of the opinion that a valid use case should never require a comment to disable a lint rule.

[JoshuaKGoldberg]

@PachowStudios the rule's intent is for you to use a .catch in that case. See #2774.
If NestFactory.create or app.listen throw, that will become an unhandled Promise rejection.

async function bootstrap() {
  const app = await NestFactory.create(AppModule);

  await app.listen(4000);
}

bootstrap().catch(error => console.error("Oh no!", { error });

Anyway, if there are valid cases folks are seeing no-floating-promises catch, please file a separate issue so we can discuss & track that separately.

[adidahiya]

Closing in favor of addressing the issue in typescript-eslint

[JoshuaKGoldberg]

🤖 Beep boop! 👉 TSLint is deprecated 👈 _(#4534)_ and you should switch to typescript-eslint! 🤖

🔒 This issue is being locked to prevent further unnecessary discussions. Thank you! 👋