-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Enhancement: [no-floating-promises] add an 'allowForKnownSafeCalls' option #8404
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
Comments
// should be ignored
it('', () => {
// can sometimes be ignored
it(() => {});
}) The API design isn't a clear cut thing, sadly. As we've discussed on discord - I'm begrudgingly okay with adding an allowlist - but we need to be very clear and explicit that it can and likely will create bug vectors and is a footgun unless you're 100% certain the promise is never floating. |
I have a patch that I've been running with for years now. I'd love to see this become available upstream in some shape or form and I'm willing to make a PR if we align on the desired format for the config diff --git a/dist/rules/no-floating-promises.js b/dist/rules/no-floating-promises.js
index cb063383ac566611aa1fe4ba0b12a292609cd597..90cba508e71cd905a9cd2a93c037bd38088aeef3 100644
--- a/dist/rules/no-floating-promises.js
+++ b/dist/rules/no-floating-promises.js
@@ -246,7 +246,12 @@ function isPromiseLike(checker, node) {
const type = checker.getTypeAtLocation(node);
for (const ty of tsutils.unionTypeParts(checker.getApparentType(type))) {
const then = ty.getProperty('then');
- if (then === undefined) {
+ const typename = `${ty?.symbol?.parent?.escapedName}.${ty?.symbol?.escapedName}`;
+ if (
+ then === undefined
+ || typename === "Knex.QueryBuilder"
+ || typename === "SuperTestStatic.Test"
+ ) {
continue;
}
const thenType = checker.getTypeOfSymbolAtLocation(then, node);
diff --git a/dist/rules/no-misused-promises.js b/dist/rules/no-misused-promises.js
index 54aa4f5192898c2bd906e5fd5cfa08f301eefcd8..cbba226b1becffc6868956762efb2cf72f32d41f 100644
--- a/dist/rules/no-misused-promises.js
+++ b/dist/rules/no-misused-promises.js
@@ -340,12 +340,23 @@ exports.default = (0, util_1.createRule)({
function isSometimesThenable(checker, node) {
const type = checker.getTypeAtLocation(node);
for (const subType of tsutils.unionTypeParts(checker.getApparentType(type))) {
- if (tsutils.isThenableType(checker, node, subType)) {
+ if (isThenableType(checker, node, subType)) {
return true;
}
}
return false;
}
+
+function isAllowListedThenable(type) {
+ const typename = `${type?.symbol?.parent?.escapedName}.${type?.symbol?.escapedName}`;
+ return typename === "Knex.QueryBuilder" || typename === "SuperTestStatic.Test"
+}
+
+function isThenableType(checker, node, type) {
+ const types = tsutils.unionTypeParts(type)
+ return types.filter(type => !isAllowListedThenable(type)).some(type => tsutils.isThenableType(checker, node, type))
+}
+
// Variation on the thenable check which requires all forms of the type (read:
// alternates in a union) to be thenable. Otherwise, you might be trying to
// check if something is defined or undefined and get caught because one of the
@@ -356,7 +367,7 @@ function isAlwaysThenable(checker, node) {
const thenProp = subType.getProperty('then');
// If one of the alternates has no then property, it is not thenable in all
// cases.
- if (thenProp === undefined) {
+ if (thenProp === undefined || isAllowListedThenable(subType)) {
return false;
}
// We walk through each variation of the then property. Since we know it
@@ -475,7 +486,7 @@ function voidFunctionArguments(checker, node) {
function anySignatureIsThenableType(checker, node, type) {
for (const signature of type.getCallSignatures()) {
const returnType = signature.getReturnType();
- if (tsutils.isThenableType(checker, node, returnType)) {
+ if (isThenableType(checker, node, returnType)) {
return true;
}
}
@@ -502,7 +513,7 @@ function isVoidReturningFunctionType(checker, node, type) {
const returnType = signature.getReturnType();
// If a certain positional argument accepts both thenable and void returns,
// a promise-returning function is valid
- if (tsutils.isThenableType(checker, node, returnType)) {
+ if (isThenableType(checker, node, returnType)) {
return false;
}
hadVoidReturn ||= tsutils.isTypeFlagSet(returnType, ts.TypeFlags.Void); |
I'd love this for |
Fixed in v8 branch in #9234 |
Before You File a Proposal Please Confirm You Have Done The Following...
My proposal is suitable for this project
Link to the rule's documentation
https://typescript-eslint.io/rules/no-floating-promises
Description
#7008's
allowForKnownSafePromises
is a type-oriented approach to allowlisting safe types to be floating. For frameworks that have those Promises show up in many places (e.g. reduxjs/redux-toolkit#4101), marking the type as safe is a reasonable solution.But (paraphrasing private remarks from @Josh-Cena): that requires framework authors to set things up in a very specific way that feels like jumping through hoops specific to another, tangential technology (us). For example, nodejs/node#51292 tracks how
node:test
'sit
& similar return aPromise
. Even if the Node types get some fancy brandedSafePromise
type added (one inconvenience), users really just want a way to mark thatit()
calls are safe. Asking users to know the Node.js-specific types is more learning curve for them (another inconvenience).In other words, we think users would really want to mark specific functions as safe to call. A kind of
allowForKnownSafePromiseReturns
option.Fail
Pass
Additional Info
No response
The text was updated successfully, but these errors were encountered: