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

Jump to bottom

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+122 −36
Open

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
3 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
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 16 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 @@ -226,6 +226,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 @@ -257,6 +261,16 @@ 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)`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typescript gives a warning

'await' has no effect on the type of this expression.

Lets not include example of those cases for which the ts compiler gives us a warning/error (?)

Minimum repro for the warning , hover over await keyword in line 2.

// as `Promise<number> & number` instead of `number`. In any case,
// this saves us a bit of type checking, anyway.
return { isUnhandled: false };
}

if (!isPromiseLike(checker, tsNode)) {
return { isUnhandled: false };
}
Expand Down Expand Up @@ -290,8 +304,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 @@ -300,15 +312,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 @@ -317,10 +320,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 };
}
},
});
Expand Down
127 changes: 106 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

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 @@ -516,6 +513,42 @@ declare const myTag: (strings: TemplateStringsArray) => string;
myTag\`abc\`;
`,
},
{
code: `
declare let x: any;
declare const promiseArray: Array<Promise<unknown>>;
x = promiseArray;
Copy link
Member Author

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();
}
`,
},
],

invalid: [
Expand Down Expand Up @@ -972,9 +1005,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 @@ -1002,9 +1035,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 @@ -1017,9 +1050,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 @@ -1227,13 +1260,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 @@ -1762,6 +1795,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 @@ -1855,5 +1898,47 @@ cursed();
`,
errors: [{ line: 3, messageId: 'floatingPromiseArrayVoid' }],
},
{
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: `
3 as Promise<number>;
Copy link
Contributor

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.

Playground link

Either these tests be removed or should be replaced with valid ts code.

`,
errors: [{ messageId: 'floatingVoid', line: 2 }],
},
{
code: `
3 as Promise<number> & number;
`,
errors: [{ messageId: 'floatingVoid', line: 2 }],
},
{
code: `
3 as Promise<number> & { yolo?: string };
`,
errors: [{ messageId: 'floatingVoid', line: 2 }],
},
{
code: `
<Promise<number>>3;
`,
errors: [{ messageId: 'floatingVoid', line: 2 }],
},
],
});