Enhancement: no-misused-promises small performance improvement, avoiding unnecessary call to getContextualType

[scottarver]

Before You File a Bug Report Please Confirm You Have Done The Following...

• I have tried restarting my IDE and the issue persists.
• I have updated to the latest version of the packages.
• I have searched for related issues and found none that matched my issue.
• I have read the FAQ and my problem is not listed.

Issue Description

I profiled my eslint to see what it is slow, and it lead me to @typescript-eslint/no-misused-promises, and the culprate inside is getContextualType
it appears that returnsThenable can be called first before getContextualType for checkJSXAttribute

profiling a smaller component:

https://github.com/typescript-eslint/typescript-eslint/blob/v5.46.0/packages/eslint-plugin/src/rules/no-misused-promises.ts#L378-L387

It looks like getContextualType is more expensive than returnsThenable

Inside I saw that there was potential for an optimization to not call checker.getContextualType(value) every time. I made this change locally and my lints on my big project went from ~130 seconds to ~125

There appears to be some other optimizations that can be done to calls to getContextualType but I only looked at 1 place, and I'm not sure of the technicals of getContextualType and returnsThenable yet

on my large private repo, I got 4311 short circuits and 15 calls to getContextualType. so it saved 4311 calls to getContextualType.

I am willing to make the PR

diff --git a/packages/eslint-plugin/src/rules/no-misused-promises.ts b/packages/eslint-plugin/src/rules/no-misused-promises.ts
index b6914ae2..0e85656e 100644
--- a/packages/eslint-plugin/src/rules/no-misused-promises.ts
+++ b/packages/eslint-plugin/src/rules/no-misused-promises.ts
@@ -375,11 +375,16 @@ export default util.createRule<Options, MessageId>({
       ) {
         return;
       }
+      if(!returnsThenable(checker, value.expression)){
+        return;
+      }
       const contextualType = checker.getContextualType(value);
       if (
         contextualType !== undefined &&
-        isVoidReturningFunctionType(checker, value, contextualType) &&
-        returnsThenable(checker, value.expression)
+        isVoidReturningFunctionType(checker, value, contextualType)
       ) {
         context.report({
           messageId: 'voidReturnAttribute',

Reproduction Repository Link

N/A

Repro Steps

1. clone the repo
2. yarn install
3. yarn lint
4. run NODE_OPTIONS="--inspect-brk" prefixing the eslint command
5. observe the slowest inside the chrome profiler

Versions

package	version
@typescript-eslint/eslint-plugin	5.45.1
@typescript-eslint/parser	5.45.1
TypeScript	4.9.3
ESLint	8.29.0
node	v16.17.0

[JoshuaKGoldberg]

What a lovely issue! Thanks so much for the great investigation & deep dive @scottarver - this sounds like a great improvement to the rule.

If there's no need to call getContextualType early, then +1. Let's take the refactor.

I'll note, though, that oftentimes it's the first get*Type* call from the type checker that shows up as taking the most time. Subsequent calls tend to use cached type information inside TypeScript. So while this change is generally a good idea, in practice it might not always impact much. The reduction of 130 seconds to 125 is nice though.

[eyal1990]

I use this rule on quite a large codebase and the rule is taking about 40x time than the average rule.
It takes ~12 seconds which account to 23% out of all the rules I use, and I have about 80 rules in total.

I don't know if the fix suggested will bring the rule down to the level of the average rule, but it sure seems that this rule has a performance issue.

[TKasperczyk]

In the case of my React project, here's the performance report:

Rule                                    | Time (ms) | Relative
:---------------------------------------|----------:|--------:
@typescript-eslint/no-misused-promises  | 25848.525 |    94.9%
import/namespace                        |  1163.663 |     4.3%
@typescript-eslint/no-unsafe-assignment |    49.083 |     0.2%
indent                                  |    34.619 |     0.1%
@typescript-eslint/no-unsafe-argument   |    20.519 |     0.1%
import/order                            |    12.161 |     0.0%
react/display-name                      |     8.322 |     0.0%
import/default                          |     7.655 |     0.0%
import/no-named-as-default-member       |     7.405 |     0.0%
@typescript-eslint/no-floating-promises |     6.929 |     0.0%

I'm analyzing only one file:

TIMING=1 npx eslint --ignore-path .gitignore --debug --fix --cache --cache-file /tmp/eslintcache2 ./src/features/hr/users/edit/Edit.tsx

Waiting over 25 seconds for a single rule isn't worth having it enabled.

[scottarver]

Try disabling the rule, and see how the time on the others is affected.

[TKasperczyk]

Try disabling the rule, and see how the time on the others is affected.

Unfortunately, after disabling it, no-unsafe-assignment started to perform poorly - it took 26 seconds, which suggests that no-misused-promises is not the cause of this problem, and that the information in the rule performance report is incorrect. Every time I disable the rule that is at the top (i.e. taking the longest to process), another seemingly random rule is taking its place after re-running eslint.

[scottarver]

#5495 (comment)

[eyal1990]

I use this rule on quite a large codebase and the rule is taking about 40x time than the average rule. It takes ~12 seconds which account to 23% out of all the rules I use, and I have about 80 rules in total.

I don't know if the fix suggested will bring the rule down to the level of the average rule, but it sure seems that this rule has a performance issue.

When I remove the rule it cuts 10s(out of 12s it took before) from the entire eslint run time.
So at least in my case it seems that there is a major performance hit using this rule.

After using the updated code it took 13s (1s+). I guess it's just that there is a variance to the result and not that the change actually hurts the performance.
But the point is that there is a performance problem with the rule, even after the fix.

[bradzacher]

Certain type-aware rules are going to be more expensive than others.
This occurs because type info is computed lazily - so the more types a rule consumes, the slower it is.

no-misused-promises is expensive because there are so many ways in which you can misuse a promise. The rule will check:

• every single conditional test (if, while, for, ternary, etc)
• every single fn call argument
• every single jsx attr or obj prop
• every single return
• every single assignment or variable decl
• every single spread

Which all up means the rule is checking a huge percentage of a codebase. More checks means more types which means which means it'll be slower than other rules which are hyper-focused (like restrict-plus-operands which only checks + binary expressions).

Sadly performance hit is just a fact of life though. It's up to you to decide if the extra safety is worth the time it takes. For some it is, for others it isn't. We recommend you use it because a little extra time on your lint saves a lot of potentially painful debugging time later - but we definitely understand those that can't or don't want it!

---

OFC are very open to (and encourage) people helping out here and looking for ways we can improve the performance of rules!

[scottarver]

I made this to try to help it go a bit faster
#6193

it looks like checking the context is more expensive than checking if something is returning a promise.