feat(eslint-plugin): [no-misused-promises] check subtype methods against heritage type methods #8765
Conversation
…ks for subtypes implementing interfaces, and interfaces extending classes, but not classes extending classes)
… new subtypes option
|
Thanks for the PR, @alythobani! 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. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
checksVoidReturn.subtypes option added to no-misused-promises rule (checks methods against heritage type methods)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8765 +/- ##
=======================================
Coverage 87.91% 87.91%
=======================================
Files 402 402
Lines 13725 13775 +50
Branches 3998 4019 +21
=======================================
+ Hits 12066 12110 +44
- Misses 1352 1358 +6
Partials 307 307
Flags with carried forward coverage won't be shown. Click here to find out more.
|
+1, this is a nice spot! |
…anatory comments to the code that ignores unnamed methods
- checksVoidReturn sub-options each get their own subsection - option descriptions go under Options subsection instead of Examples
There was a problem hiding this comment.
I have been mollified. 😄
This really is a lovely PR with a super interesting bug case. Thanks for all your work on it @alythobani! 🙌
|
😁 Sweet, and thanks for such a detailed review @JoshuaKGoldberg! Really appreciate all the catches and suggestions and all the time you put into it! And for putting up with my overly long explanations 😇 |
condensing undefined and 0 check into one
There was a problem hiding this comment.
Looks great to me, thanks! Since @kirkwaiblinger was also reviewing, will wait a bit for Kirk to review.
| if (memberName === undefined) { | ||
| // Call/construct/index signatures don't have names. TS allows call signatures to mismatch, | ||
| // and construct signatures can't be async. | ||
| // TODO - Once we're able to use `checker.isTypeAssignableTo` (v8), we can check an index |
There was a problem hiding this comment.
v8 is upon us! Though of course this PR has been in the works since long before that.
@alythobani, let's merge as-is, but would you mind filing a followup (that we now know will not be blocked 🙂)
There was a problem hiding this comment.
Sweet, yeah definitely! Do you mean filing an issue for this? And I can start looking into a proper implementation too if you'd like
8304cef
There was a problem hiding this comment.
All right!~ This is looking great! Thanks so much for working with us a bunch on it @alythobani. Your explanations were really helpful and I'm super happy where this went. Awesome job.
Merging now to be released later today in our weekly release! 🙌
|
Sorry again for the late replies here, life is a little hectic these days! But amaaazing, so good to see this merged! Thanks again so much for all the collaboration here @JoshuaKGoldberg and @kirkwaiblinger !! Obviously please keep me updated on any issues that arise, but hopefully there won't be any :) |
PR Checklist
Overview
This PR adds a new
heritageTypesoption (underchecksVoidReturn) to theno-misused-promisesrule, defaulted totrue. If enabled, it checksClassDeclaration/ClassExpression/InterfaceDeclarationnodes for any methods that returnPromises, and checks those methods against any matching methods in their declaredextends .../implements ...heritage types. Error outputted when a heritage type has the given method declared/implemented as returningvoidinstead of aPromise.(The issue #8739 I filed only mentioned abstract classes/methods, but I realized the topic is more generalizable to heritage types in general.)
Note that this is my first PR here and first time diving into this repo / the Typescript source code. So hopefully I didn't miss any cases to test / handle, but it's possible I did! There did end up being small edge cases I realized weren't handled as I iterated and had to tweak things. E.g., calling
checker.getTypeAtLocation(heritageTypeExpression)ended up being necessary in order to properly check against aClassExpressionheritage type's members;checker.getTypeFromTypeNode(heritageTypeExpression)worked for all other heritage types, just not class expressions.