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

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

Closed
3 tasks done
LeandroDG opened this issue Sep 11, 2020 · 5 comments · Fixed by #2682
Closed
3 tasks done
Labels
bug Something isn't working good first issue Good for newcomers package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@LeandroDG
Copy link

LeandroDG commented Sep 11, 2020

  • 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
@LeandroDG LeandroDG added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Sep 11, 2020
@bradzacher
Copy link
Member

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!

@bradzacher bradzacher added bug Something isn't working good first issue Good for newcomers and removed triage Waiting for team members to take a look labels Sep 12, 2020
@tadhgmister
Copy link
Contributor

tadhgmister commented Sep 15, 2020

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
Copy link
Member

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
Copy link
Contributor

I am working this

@LeandroDG
Copy link
Author

Thank you so much for your help with this =)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working good first issue Good for newcomers package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants