[no-misused-promises] Not marking as error when combined with a logical operator variable

[LeandroDG]

• I have tried restarting my IDE and the issue persists.
• I have updated to the latest version of the packages.
• I have read the FAQ and my problem is not listed.

Repro

  rules: {
    // "spaced-comment": ["warn", "always"],
    semi: ["error", "always"],
    quotes: ["warn", "double"],
    "prefer-const": "warn",
    "no-underscore-dangle": "warn",
    "@typescript-eslint/explicit-function-return-type": "warn",
    "@typescript-eslint/no-explicit-any": "off",
    "@typescript-eslint/type-annotation-spacing": "off",
    "@typescript-eslint/no-empty-function": "warn",
    "@typescript-eslint/no-use-before-define": "off",
    "@typescript-eslint/camelcase": "off",
    **"@typescript-eslint/no-misused-promises": "error",**
    "@typescript-eslint/no-floating-promises": "error"
  }

  async function reproduceIssue(parameterConditional: boolean) {
    const myPromise = Promise.resolve(true);
    const localTrue = true;
    const localFalse = false;

    // This correctly shows a bad usage
    if (myPromise) {
      console.log("Bad usage");
    }

    // This correctly shows a bad usage
    if (localTrue && myPromise) {
      console.log("Bad usage");
    }

    // This correctly not marks a bad usage
    if (localFalse && myPromise) {
      console.log("Bad usage");
    }

    // This should show a bad usage
    if (parameterConditional && myPromise) {
      console.log("Bad usage");
    }

    // This should show a bad usage
    if (returnTrue() && myPromise) {
      console.log("Bad usage");
    }
  }

  function returnTrue(): boolean {
    return true;
  }

Expected Result

These lines should mark the promise as misused, but as they are applied an AND with an unknown boolean parameter or function result, they don't:

if (parameterConditional && myPromise) {
if (this.returnTrue() && myPromise) {

Actual Result

Those lines are not marked as misuses of the promise but they are.

Additional Info

Versions

package	version
@typescript-eslint/eslint-plugin	4.1.0
@typescript-eslint/parser	4.1.0
TypeScript	3.9.7
ESLint	7.8.1
node	12.8.3

[bradzacher]

The rule doesn't do any sophisticated inspection - it just looks for a Promise typed thing in a place it shouldn't be.

In this instance.

    // This correctly not marks a bad usage
    if (localFalse && myPromise) { // typeof === false - hence it doesn't detect it
      console.log("Bad usage");
    }

    // This should show a bad usage
    if (parameterConditional && myPromise) { // typeof = false & Promise<boolean> - hence it doesn't detect it
      console.log("Bad usage");
    }

    // This should show a bad usage
    if (this.returnTrue() && myPromise) { // ditto - false & Promise
      console.log("Bad usage");
    }

Happy to accept a PR to make the rule smarter!

[tadhgmister]

I don't think this is a matter of "the rule just isn't sophisticated enough", the rule specifically checks that all union parts are then-able:

// Variation on the thenable check which requires all forms of the type (read:
// alternates in a union) to be thenable. Otherwise, you might be trying to
// check if something is defined or undefined and get caught because one of the
// branches is thenable.
function isAlwaysThenable(checker: ts.TypeChecker, node: ts.Node): boolean {
    ... 
    // If one of the alternates has no then property, it is not thenable in all
    // cases.
    if (thenProp === undefined) {
      return false;
    }

I'm not sure what the correct logic should be, some options are:

1. if any member of the union is then-able in a condition raise a warning, this is easiest but goes against some of the initial design (maybe as a config option?)
2. if a conditional has a union where one member is a promise that resolves to a boolean warn about that, otherwise only warn if all union members are promises. (is Promise<boolean | null> etc. caught?)
3. when checking a logical expression conditionally check the right hand side if the expression is directly inside a condition location, will need to handle nesting like if(a && (b || c)){} correctly
4. something more complicated like raise a warning if the only other parts of the union are boolean, seems more obscure though.

also minor note, this can be simplified at the same time:

-      UnaryExpression(node) {
+     'UnaryExpression[operator="!"]'(node: TSESTree.UnaryExpression) {
-        if (node.operator === '!') {
          checkConditional(node.argument);
-        }
      },

[bradzacher]

The issue here is that the rule does not properly analyse logical expressions within a conditional test context - it's not being sophisticated enough.

It does do some checks on LogicalExpression, but it only checks the left expression, never the right. It ignores the right because it makes the assumption that the right value is either:

• being used as per a short-circuit
• handled by the base conditional test

However with this simple logic, it does not work well for all cases.
For example - how should the boolean | Promise type be handled?

Imagine the following code:

async function foo(arg: false | Promise<boolean>) {
  if (arg) {
    return await arg;
  }
}

Is this a misused promise? The answer is no - you need to use the conditional to narrow the type first. So the rule correctly handles this.
(sure you could use typeof instead to narrow it - but that shouldn't be required).

However if I do this:

async function foo(bool: boolean, prom: Promise<boolean>) {
  if (bool && prom) {
    return await prom;
  }
}

Is this a misused promise? The answer is yes - it's an unnecessary condition AND it's a misused promise.

The rule right now does not deeply inspect the logical - just the result of the logical.
The return type of the logical here is false | Promise<boolean>, which is the exact same type as the first example (which we decided was correctly handled).

One final example to further drive the point home:

async function foo(bool: true, prom: Promise<boolean>) {
  if (bool || prom) {
    return await prom;
  }
}

Is this a misused promise? The answer is yes - again it's an unnecessary condition AND it's a misused promise.
However the return type of the logical here is true, which the rule will just ignore as it's not thenable.

---

The rule is not doing any sophisticated checks - it's just doing a naive check of the logical expression. It's checking the runtime value of the logical and ignoring the code itself.

So the fix here is as follows:

In a conditional test context the rule should deeply inspect a LogicalExpression and individually test each and every expression separately.

[sonallux]

I am working this

[LeandroDG]

Thank you so much for your help with this =)