Skip to content
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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jul 16, 2024
Merged

fix(eslint-plugin): [no-floating-promises] check top-level type assertions (and more) #9043

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2ec68c2
check lots of syntaxes
kirkwaiblinger May 4, 2024
3dd7f67
fixup
kirkwaiblinger May 4, 2024
79fa0f6
more test
kirkwaiblinger May 4, 2024
01c550d
Merge branch 'main' into flag-on-type-assertions
kirkwaiblinger Jun 21, 2024
53e7d0e
pr feedback
kirkwaiblinger Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 15 additions & 15 deletions packages/eslint-plugin/src/rules/no-floating-promises.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,10 @@ export default createRule<Options, MessageId>({
nonFunctionHandler?: boolean;
promiseArray?: boolean;
} {
if (node.type === AST_NODE_TYPES.AssignmentExpression) {
return { isUnhandled: false };
}

// First, check expressions whose resulting types may not be promise-like
if (node.type === AST_NODE_TYPES.SequenceExpression) {
// Any child in a comma expression could return a potentially unhandled
Expand Down Expand Up @@ -267,6 +271,15 @@ export default createRule<Options, MessageId>({
return { isUnhandled: true, promiseArray: true };
}

// await expression addresses promises, but not promise arrays.
if (node.type === AST_NODE_TYPES.AwaitExpression) {
// you would think this wouldn't be strictly necessary, since we're
// anyway checking the type of the expression, but, unfortunately TS
// reports the result of `await (promise as Promise<number> & number)`
kirkwaiblinger marked this conversation as resolved.
Show resolved Hide resolved
// as `Promise<number> & number` instead of `number`.
return { isUnhandled: false };
}

if (!isPromiseLike(tsNode)) {
return { isUnhandled: false };
}
Expand Down Expand Up @@ -300,8 +313,6 @@ export default createRule<Options, MessageId>({

// All other cases are unhandled.
return { isUnhandled: true };
} else if (node.type === AST_NODE_TYPES.TaggedTemplateExpression) {
return { isUnhandled: true };
} else if (node.type === AST_NODE_TYPES.ConditionalExpression) {
// We must be getting the promise-like value from one of the branches of the
// ternary. Check them directly.
Expand All @@ -310,15 +321,6 @@ export default createRule<Options, MessageId>({
return alternateResult;
}
return isUnhandledPromise(checker, node.consequent);
} else if (
node.type === AST_NODE_TYPES.MemberExpression ||
node.type === AST_NODE_TYPES.Identifier ||
node.type === AST_NODE_TYPES.NewExpression
) {
// If it is just a property access chain or a `new` call (e.g. `foo.bar` or
// `new Promise()`), the promise is not handled because it doesn't have the
// necessary then/catch call at the end of the chain.
return { isUnhandled: true };
} else if (node.type === AST_NODE_TYPES.LogicalExpression) {
const leftResult = isUnhandledPromise(checker, node.left);
if (leftResult.isUnhandled) {
Expand All @@ -327,10 +329,8 @@ export default createRule<Options, MessageId>({
return isUnhandledPromise(checker, node.right);
}

// We conservatively return false for all other types of expressions because
// we don't want to accidentally fail if the promise is handled internally but
// we just can't tell.
return { isUnhandled: false };
// Anything else is unhandled.
return { isUnhandled: true };
Comment on lines +332 to +333
Copy link
Member

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.

Copy link
Member Author

@kirkwaiblinger kirkwaiblinger Jun 23, 2024 •
edited
Loading

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! 🙂

}

function isPromiseArray(node: ts.Node): boolean {
Expand Down
128 changes: 107 additions & 21 deletions packages/eslint-plugin/tests/rules/no-floating-promises.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,8 @@ async function test() {
}
`,
`
declare const promiseValue: Promise<number>;
async function test() {
declare const promiseValue: Promise<number>;
Copy link
Member Author

@kirkwaiblinger kirkwaiblinger May 4, 2024 •
edited
Loading

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.


await promiseValue;
promiseValue.then(
() => {},
Expand All @@ -158,9 +157,8 @@ async function test() {
}
`,
`
declare const promiseUnion: Promise<number> | number;
async function test() {
declare const promiseUnion: Promise<number> | number;

await promiseUnion;
promiseUnion.then(
() => {},
Expand All @@ -177,9 +175,8 @@ async function test() {
}
`,
`
declare const promiseIntersection: Promise<number> & number;
async function test() {
declare const promiseIntersection: Promise<number> & number;

await promiseIntersection;
promiseIntersection.then(
() => {},
Expand Down Expand Up @@ -210,12 +207,12 @@ async function test() {
}
`,
`
declare const intersectionPromise: Promise<number> & number;
async function test() {
await (Math.random() > 0.5 ? numberPromise : 0);
await (Math.random() > 0.5 ? foo : 0);
await (Math.random() > 0.5 ? bar : 0);

declare const intersectionPromise: Promise<number> & number;
await intersectionPromise;
}
`,
Expand Down Expand Up @@ -308,8 +305,8 @@ async function test() {

// optional chaining
`
declare const returnsPromise: () => Promise<void> | null;
async function test() {
declare const returnsPromise: () => Promise<void> | null;
await returnsPromise?.();
returnsPromise()?.then(
() => {},
Expand Down Expand Up @@ -712,6 +709,42 @@ myTag\`abc\`;
},
{
code: `
declare let x: any;
declare const promiseArray: Array<Promise<unknown>>;
x = promiseArray;
Copy link
Member Author

@kirkwaiblinger kirkwaiblinger May 4, 2024 •
edited
Loading

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: `
declare let x: Promise<number>;
x = Promise.resolve(2);
`,
},
{
code: `
declare const promiseArray: Array<Promise<unknown>>;
async function f() {
return promiseArray;
}
`,
},
{
code: `
declare const promiseArray: Array<Promise<unknown>>;
async function* generator() {
yield* promiseArray;
}
`,
},
{
code: `
async function* generator() {
yield Promise.resolve();
}
`,
},
{
code: `
interface SafeThenable<T> {
then<TResult1 = T, TResult2 = never>(
onfulfilled?:
Expand Down Expand Up @@ -1189,9 +1222,9 @@ async function test() {
},
{
code: `
async function test() {
declare const promiseValue: Promise<number>;
declare const promiseValue: Promise<number>;

async function test() {
promiseValue;
promiseValue.then(() => {});
promiseValue.catch();
Expand Down Expand Up @@ -1219,9 +1252,9 @@ async function test() {
},
{
code: `
async function test() {
declare const promiseUnion: Promise<number> | number;
declare const promiseUnion: Promise<number> | number;

async function test() {
promiseUnion;
}
`,
Expand All @@ -1234,9 +1267,9 @@ async function test() {
},
{
code: `
async function test() {
declare const promiseIntersection: Promise<number> & number;
declare const promiseIntersection: Promise<number> & number;

async function test() {
promiseIntersection;
promiseIntersection.then(() => {});
promiseIntersection.catch();
Expand Down Expand Up @@ -1444,13 +1477,13 @@ async function test() {
},
{
code: `
(async function () {
declare const promiseIntersection: Promise<number> & number;
promiseIntersection;
promiseIntersection.then(() => {});
promiseIntersection.catch();
promiseIntersection.finally();
})();
declare const promiseIntersection: Promise<number> & number;
(async function () {
promiseIntersection;
promiseIntersection.then(() => {});
promiseIntersection.catch();
promiseIntersection.finally();
})();
`,
options: [{ ignoreIIFE: true }],
errors: [
Expand Down Expand Up @@ -1979,6 +2012,16 @@ void promiseArray;
},
{
code: `
declare const promiseArray: Array<Promise<unknown>>;
async function f() {
await promiseArray;
}
`,
options: [{ ignoreVoid: false }],
errors: [{ line: 4, messageId: 'floatingPromiseArray' }],
},
{
code: `
[1, 2, Promise.reject(), 3];
`,
errors: [{ line: 2, messageId: 'floatingPromiseArrayVoid' }],
Expand Down Expand Up @@ -2181,5 +2224,48 @@ myTag\`abc\`;
options: [{ allowForKnownSafePromises: [{ from: 'file', name: 'Foo' }] }],
errors: [{ line: 4, messageId: 'floatingVoid' }],
},
{
code: `
declare const x: any;
function* generator(): Generator<number, void, Promise<number>> {
yield x;
}
`,
errors: [{ messageId: 'floatingVoid' }],
},
{
code: `
declare const x: Generator<number, Promise<number>, void>;
function* generator(): Generator<number, void, void> {
yield* x;
}
`,
errors: [{ messageId: 'floatingVoid' }],
},
{
code: `
const value = {};
value as Promise<number>;
`,
errors: [{ messageId: 'floatingVoid', line: 3 }],
},
{
code: `
({}) as Promise<number> & number;
`,
errors: [{ messageId: 'floatingVoid', line: 2 }],
},
{
code: `
({}) as Promise<number> & { yolo?: string };
`,
errors: [{ messageId: 'floatingVoid', line: 2 }],
},
{
code: `
<Promise<number>>{};
`,
errors: [{ messageId: 'floatingVoid', line: 2 }],
},
],
});
Loading