fix(eslint-plugin): [no-misused-promises] avoid unnecessary calls to getContextualType #6193

scottarver · 2022-12-09T20:50:25Z

PR Checklist

Addresses an existing open issue: fixes Enhancement: no-misused-promises small performance improvement, avoiding unnecessary call to getContextualType #6186
That issue was marked as accepting prs
Steps in CONTRIBUTING.md were taken

Overview

It appears that returnsThenable is faster than getContextualType in almost all cases.
This PR only calls getContextualType when needed, after returnsThenable returns true.

I ran numbers on how often this is the case: Out of 19,425 total checks, only 96 (33+63) needed a call to getContextualType, this saved 19329 (17289 + 2040 ) calls to getContextualType by short circuiting with returnsThenable
Also of note on average, returnsThenable took 0.12891074558698418ms to run vs getContextualType taking 0.5058407855855643ms, saving 9.8 seconds.

total time saved:  9817.560766001232
{
  thenableAndContextual: 33,
  NotThenableAndContextual: 17289,
  thenableAndNotContextual: 63,
  NotThenableAndNotContextual: 2040
}
thenable count:  19425
thenable average: 0.12891074558698418
contextualType count:  19425
contextualType average: 0.5058407855855643

Using the chrome profiler to compare: ran with node --inspect-brk ./node_modules/.bin/eslint src
Each check* fn has to be looked at together as a whole, in the table below on a single run it saved 1.2 seconds

Before	After

	before	after
checkTestConditional	73.6	77
checkConditional	2.4	3.6
checkAssignment	1.7	2.9
checkVariableDeclaration	1160.5	1207.4
checkProperty	74.4	40.2
checkReturnStatement	9.2	56.1
checkJSXAttribute	2076.9	754.7
checkArguments	121.6	125.6
checkSpread	5.5	4.4	ms diff	seconds
	3525.8	2271.9	1253.9	1.2539

typescript-eslint · 2022-12-09T20:50:27Z

Thanks for the PR, @scottarver!

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.

nx-cloud · 2022-12-09T20:50:27Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit c02724c. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

🟥 Failed Commands
`Node 14 - nx test typescript-estree --coverage=false`
`nx run-many --target=lint --parallel`

✅ Successfully ran 28 targets

Sent with 💌 from NxCloud.

netlify · 2022-12-09T20:50:29Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`8797d46`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/640f8c67d239b500088d641b
😎 Deploy Preview	https://deploy-preview-6193--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

codecov · 2022-12-09T21:04:33Z

Codecov Report

Merging #6193 (b039619) into main (2b2a075) will increase coverage by 6.06%.
The diff coverage is 63.63%.

❗ Current head b039619 differs from pull request most recent head 8797d46. Consider uploading reports for the commit 8797d46 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6193      +/-   ##
==========================================
+ Coverage   85.16%   91.23%   +6.06%     
==========================================
  Files         383      366      -17     
  Lines       13025    12442     -583     
  Branches     3839     3645     -194     
==========================================
+ Hits        11093    11351     +258     
+ Misses       1570      780     -790     
+ Partials      362      311      -51

Flag	Coverage Δ
unittest	`91.23% <63.63%> (+6.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ges/eslint-plugin/src/rules/no-misused-promises.ts	`95.63% <63.63%> (-2.85%)`	⬇️

... and 65 files with indirect coverage changes

JoshuaKGoldberg

This started out as a nice performance improvement - and now is also an unnecessary code removal and/or test expansion opportunity! Great either way 😄

packages/eslint-plugin/src/rules/no-misused-promises.ts

JoshuaKGoldberg · 2023-02-05T01:55:55Z

👋 ping @scottarver - is this still something you have time for? No worries if not, just checking in.

nx-cloud · 2023-03-13T14:55:57Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 8797d46. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 25 targets

Sent with 💌 from NxCloud.

JoshuaKGoldberg · 2023-03-13T16:17:05Z

Ooh, I removed some more calls and all of the unit tests are failing... but now we've got lint failures in the project itself. So we've got to add unit tests! 🙌

https://cloud.nx.app/runs/NHi3D9Xh0H

/home/runner/work/typescript-eslint/typescript-eslint/packages/eslint-plugin/src/rules/adjacent-overload-signatures.ts
  172:18  error  Promise-returning function provided to property where a void return was expected  @typescript-eslint/no-misused-promises
  173:16  error  Promise-returning function provided to property where a void return was expected  @typescript-eslint/no-misused-promises
  174:22  error  Promise-returning function provided to property where a void return was expected  @typescript-eslint/no-misused-promises
  175:22  error  Promise-returning function provided to property where a void return was expected  @typescript-eslint/no-misused-promises
  176:24  error  Promise-returning function provided to property where a void return was expected  @typescript-eslint/no-misused-promises
  177:23  error  Promise-returning function provided to property where a void return was expected  @typescript-eslint/no-misused-promises

/home/runner/work/typescript-eslint/typescript-eslint/packages/eslint-plugin/src/rules/ban-types.ts
  203:41  error  Promise-returning function provided to variable where a void return was expected  @typescript-eslint/no-misused-promises

/home/runner/work/typescript-eslint/typescript-eslint/packages/eslint-plugin/src/rules/block-spacing.ts
...

Edit: 8797d46 ✅

bradzacher · 2023-03-13T21:13:23Z

It's worth noting that the two functions in question have different usecases. returnsThenable uses the "apparent type" which is sometimes different to the contextual type (I believe??)

Cc @jakebailey this is an example of a piece of the TS API that isn't very clear - type vs apparent type vs instantiated type vs contextual type. I couldn't find any documentation about what to use and when.

jakebailey · 2023-03-13T21:29:14Z

Truthfully I didn't even realize these APIs were available, let alone useful externally. Clearly, I'm wrong.

I think someone more experienced than me will have to write out explanations of those. I'll file an issue to track this request; meant to do that post-meeting but forgot.

avoid unnecessary calls to getContextualType

5310996

Merge branch 'main' into noMisusedPromises

b039619

scottarver changed the title ~~avoid unnecessary calls to getContextualType~~ fir(eslint-plugin): avoid unnecessary calls to getContextualType Dec 9, 2022

scottarver changed the title ~~fir(eslint-plugin): avoid unnecessary calls to getContextualType~~ fix(eslint-plugin): avoid unnecessary calls to getContextualType Dec 9, 2022

scottarver mentioned this pull request Dec 12, 2022

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

Closed

4 tasks

JoshuaKGoldberg requested changes Dec 12, 2022

View reviewed changes

packages/eslint-plugin/src/rules/no-misused-promises.ts Outdated Show resolved Hide resolved

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Dec 12, 2022

Removed a few more

d25c078

JoshuaKGoldberg force-pushed the noMisusedPromises branch from 811ba41 to d25c078 Compare March 13, 2023 04:54

Merge branch 'main' into noMisusedPromises

c02724c

JoshuaKGoldberg self-requested a review March 13, 2023 05:32

Merge branch 'main' into noMisusedPromises

d0035e9

JoshuaKGoldberg changed the title ~~fix(eslint-plugin): avoid unnecessary calls to getContextualType~~ fix(eslint-plugin): [no-misused-promises] avoid unnecessary calls to getContextualType Mar 13, 2023

Added back necessary untested logic, with tests

8797d46

JoshuaKGoldberg merged commit 745cfe4 into typescript-eslint:main Mar 13, 2023

jakebailey mentioned this pull request Mar 13, 2023

Document behavior, gotchas, etc, for public TypeChecker API microsoft/TypeScript#53239

Open

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): [no-misused-promises] avoid unnecessary calls to getContextualType #6193

fix(eslint-plugin): [no-misused-promises] avoid unnecessary calls to getContextualType #6193

scottarver commented Dec 9, 2022

typescript-eslint bot commented Dec 9, 2022

nx-cloud bot commented Dec 9, 2022 •

edited

Loading

netlify bot commented Dec 9, 2022 •

edited

Loading

codecov bot commented Dec 9, 2022 •

edited

Loading

JoshuaKGoldberg left a comment

JoshuaKGoldberg commented Feb 5, 2023

nx-cloud bot commented Mar 13, 2023 •

edited

Loading

JoshuaKGoldberg commented Mar 13, 2023 •

edited

Loading

bradzacher commented Mar 13, 2023

jakebailey commented Mar 13, 2023

fix(eslint-plugin): [no-misused-promises] avoid unnecessary calls to getContextualType #6193

fix(eslint-plugin): [no-misused-promises] avoid unnecessary calls to getContextualType #6193

Conversation

scottarver commented Dec 9, 2022

PR Checklist

Overview

typescript-eslint bot commented Dec 9, 2022

nx-cloud bot commented Dec 9, 2022 • edited Loading

☁️ Nx Cloud Report

netlify bot commented Dec 9, 2022 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

codecov bot commented Dec 9, 2022 • edited Loading

Codecov Report

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Feb 5, 2023

nx-cloud bot commented Mar 13, 2023 • edited Loading

☁️ Nx Cloud Report

JoshuaKGoldberg commented Mar 13, 2023 • edited Loading

bradzacher commented Mar 13, 2023

jakebailey commented Mar 13, 2023

nx-cloud bot commented Dec 9, 2022 •

edited

Loading

netlify bot commented Dec 9, 2022 •

edited

Loading

codecov bot commented Dec 9, 2022 •

edited

Loading

nx-cloud bot commented Mar 13, 2023 •

edited

Loading

JoshuaKGoldberg commented Mar 13, 2023 •

edited

Loading