feat(eslint-plugin): [no-floating-promise] add option to ignore IIFEs

[anikethsaha]

fixes #647
added a no check for iife

• created a new function to detect iife isIife (please do suggest if the logic there doesn't cover all cases)
• added tests

[typescript-eslint]

Thanks for the PR, @anikethsaha!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

---

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

[bradzacher]

could you please gate this functionality behind an option, as per my comment: #647 (comment)

I want end-users to opt-in to this functionality so that they're consciously making the decision for a little unsafety from the rule.

[anikethsaha]

could you please gate this functionality behind an option, as per my comment: #647 (comment)

I want end-users to opt-in to this functionality so that they're consciously making the decision for a little unsafety from the rule.

ok cool, also it should only check for async iife right ? not just iife ?

[bradzacher]

it should check both - it should just be looking for an IIFE that returns a promise.

[anikethsaha]

it should check both - it should just be looking for an IIFE that returns a promise.

make sense , ok creating an options name ignoreIife

[anikethsaha]

@bradzacher I think we need to change the selector for AST as well, cause

currently in ExpressionStatement

I have this check

if (options.ignoreIife && isAsyncIife(node)) {
  return;
}

And, here is the definition of isAsyncIife

    function isAsyncIife(node: TSESTree.ExpressionStatement): Boolean {
      if(node?.expression.type === AST_NODE_TYPES.AwaitExpression){
        return true
      }
      if (node?.expression.type === AST_NODE_TYPES.CallExpression) {
        if (possibleIifeCalleType.has(node?.expression?.callee.type)) {
          if (node?.expression?.callee?.hasAsync) {
            return true ;
          }
          return node?.expression?.callee?.body?.body.some(hasPromise);
        }
      }

      return false;
    }

And here is hasPromise

    function hasPromise( node: TSESTree.ExpressionStatement | TSESTree.ReturnStatement): Boolean {
      // has promise statement
      if (node?.expression.type === "NewExpression" && node?.expression?.callee.name === "Promise") {
        return true;
      }
      if (
        // returning promise
        node.type === AST_NODE_TYPES.ReturnStatement &&
        node?.argument.type === AST_NODE_TYPES.NewExpression &&
        node?.argument?.callee.name === "Promise"
      ) {
        return true;
      }
      return false
    }

It looks like when I give input like this

//  options : [{ ignoreIife : true }]
(async function() {
  // error cause of this
  await res(1);
})();

still the there is a floating error.

I did try changing the selectors

["ExpressionStatement > :not(ExpressionStatement )"](node){
  check(node)
},
"ExpressionStatement > ExpressionStatement "(node){
      if(node?.expression.type === AST_NODE_TYPES.AwaitExpression){
        return true
      }
      check(node)
}

Still no luck

[bradzacher]

the code you've got currently in the PR doesn't look too far off.

I'd expect something along the lines of:

ExpressionStatement(node): void {
  if (options.ignoreIIFE && isIIFE(node)) {
    return;
  }

  // old code
}

function isIIFE(node: TSESTree.ExpressionStatement): boolean {
  const expr = node.expression;
  if (expr.type !== AST_NODE_TYPES.CallExpression) {
    return false;
  }

  return (
    expr.callee.type === AST_NODE_TYPES.ArrowFunctionExpression ||
    expr.callee.type === AST_NODE_TYPES.FunctionExpression
  );
}

To clarify what this option should do:

These cases should error with ignoreIIFE: false (the default)
They should not error with ignoreIIFE: true

(function foo() { return Promise.resolve() })();
(async function foo() { return Promise.resolve() })();
(async () => { return Promise.resolve() })();

However, setting the option to true should not ignore the contents of an IIFE. I.e. this should still error with ignoreIIFE: true

(async () => {
  Promise.resolve(); // error - floating promise
})();
// no other lint error

[anikethsaha]

@bradzacher should I add custom type for isAsyncIife method as I added TSESTree.ExpressionStatement | TSESTree.ArrowFunctionExpression | TSESTree.TSTypeAnnotation type to node but still typecheck is failing.
also, i tried node?.expression.typeAnnotation.typeand still it failed

[bradzacher]

a few suggestions for you

[codecov]

Codecov Report

Merging #1799 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1799   +/-   ##
=======================================
  Coverage   94.37%   94.37%           
=======================================
  Files         164      164           
  Lines        7572     7579    +7     
  Branches     2175     2178    +3     
=======================================
+ Hits         7146     7153    +7     
  Misses        183      183           
  Partials      243      243           

Flag	Coverage Δ
#unittest	94.37% <100.00%> (+<0.01%)	⬆️

Impacted Files	Coverage Δ
...es/eslint-plugin/src/rules/no-floating-promises.ts	100.00% <100.00%> (ø)

[bradzacher]

a few final comments, otherwise LGTM - thanks for this.

[bradzacher]

make sure you run prettier over your code

https://github.com/typescript-eslint/typescript-eslint/blob/master/CONTRIBUTING.md#validating-your-changes

[anikethsaha]

same issue here as well

[bradzacher]

thanks!