-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
fix(eslint-plugin): [no-floating-promises] check top-level type assertions (and more) #9043
fix(eslint-plugin): [no-floating-promises] check top-level type assertions (and more) #9043
Conversation
Thanks for the PR, @kirkwaiblinger! 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. |
async function test() { | ||
declare const promiseValue: Promise<number>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are just because declare
is not syntactically allowed in that position (it's required to be at top level). Just some hygiene, not relevant to the changes.
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 53e7d0e. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
code: ` | ||
declare let x: any; | ||
declare const promiseArray: Array<Promise<unknown>>; | ||
x = promiseArray; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
surprisingly, assignments without a declaration were not tested for
}, | ||
{ | ||
code: ` | ||
3 as Promise<number>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a community maintainer, but I'm very much against these tests.
Even, after checking off all the options in the type-checking section, typescript is still giving error on this statement.
Either these tests be removed or should be replaced with valid ts code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relevant: #8298
+1 that if we can get away with no type error, it'd be better to.
const value = {};
value as Promise<number>;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Also, very clever with suggesting the castable {}
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The empty object type has been on my mind a lot lately... 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Syntax looks great to me! ✨ 🧹
Just waiting on the test discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Anything else is unhandled. | ||
return { isUnhandled: true }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In your summary you wrote
Changes implementation from an unhandled promise syntax allowlist to a syntax denylist.
But I don't understand what we gain from that switch
This seems... dangerous - switching to default assuming it's unhandled is probably going to lead to false positives at scale.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR Concern is valid but I tentatively think this is the right move. Would welcome a second opinion
I don't understand what we gain from that switch
implementation simplicity and future proofing 🤷 It seems more straightforward, and more correct (IMO), to opt out of flagging behaviors indicated by the docs than it does to opt in to the ones we do want to flag...
Nodes that are handled
await
expression- assignment expression (x = y)
Nodes that require special handling to determine whether they're handled or unhandled (and therefore need to be specified individually no matter what)
- CallExpression
- LogicalExpression
- SequenceExpression
- ConditionalExpression
void
expression- optional chaining expression
- iifes
Nodes that are always unhandled (if they result in a promise)
- NewExpression
- Identifier
- MemberExpression
- TaggedTemplateExpression (previously forgotten, recently fixed in fix(eslint-plugin): [no-floating-promises] handle TaggedTemplateExpression #8758)
- TSAsExpression (previously forgotten, the point of this PR)
- TSTypeAssertion (previously forgotten, the point of this PR)
- TSNonNullExpression (previously forgotten, basically equivalent to previous two)
- ... probably more - this feels like the appropriate default behavior for anything that doesn't fall into one of the other two buckets
This seems... dangerous - switching to default assuming it's unhandled is probably going to lead to false positives at scale.
To be sure, I gave this some thought before making this change, and I just couldn't think of any scenarios where this would really be problematic... Every case I thought of seemed to be beneficial. Just to be sure we're on the same page, that code is only reached if the expression results in a promise/thenable.
That does mean that this code will start flagging now
({
then(resolve: Function, reject: Function) {
return this;
}
});
and I think that makes sense, given
const unhandled = {
then(resolve: Function, reject: Function) {
return this;
}
};
unhandled;
already flags (of course, this touches upon #8433). But it's not like we're suddenly going to flag every object literal; just thenable ones.
I think the yield
cases added to the tests make sense to flag as well....
Note that we dogfood no-floating-promises
in this repo and this change didn't result in any new reports.
My conclusion - false positives are a very valid concern to explore, but for now I do think that this change is safe/beneficial. Would definitely be open to being proven wrong, though, especially if a good counterexample comes to mind! 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 yay!
PR Checklist
Overview
Changes implementation from an unhandled promise syntax allowlist to a syntax denylist.