feat(lint): implement `noFloatingPromises` rule for reguler `async` function #4911

kaykdm · 2025-01-19T11:43:01Z

Summary

related: #3187 and #4956
This pull request introduces the initial implementation of the no-floating-promises rule.

The rule reports Promise-valued statements that are not treated in one of the following ways:

Calling .then() with two arguments
Calling .catch() with one argument
Awaiting the Promise
Returning
Using the void operator

Note

Current Scope:

This PR currently covers simple async functions.
Currently, reassigning functions is not supported

Example of invalid code:

async function returnsPromise(): Promise<string> {
  return 'value';
}
returnsPromise().then(() => {});

Example of valid code:

async function returnsPromise(): Promise<string> {
  return 'value';
}

await returnsPromise();
void returnsPromise();

// Calling .then() with two arguments
returnsPromise().then(
  () => {},
  () => {}
);

Remaining TODOs for Future PRs

Support arrow functions
Support promises like Promise.all, etc.
Support class methods
Support object methods

codspeed-hq · 2025-01-19T12:24:41Z

CodSpeed Performance Report

Merging #4911 will not alter performance

_{Comparing kaykdm:no-floating-promises (3768f87) with next (069cbb4)}

Summary

✅ 95 untouched benchmarks

kaykdm · 2025-01-20T09:01:14Z

crates/biome_js_analyze/src/lint/nursery/no_floating_promises.rs

+                return false;
+            };
+
+            let AnyJsBindingDeclaration::JsFunctionDeclaration(func_decl) = any_js_binding_decl


For now, only checking a function declaration

Not considering reassigned variables like this

async function returnsPromise(): Promise<string> { return 'value'; } let reassigned = returnsPromise; reassigned();

We have AnyFunctionLike that you can use for detecting other kinds of function/method declarations. I understand without further type-level inference there will be few situations where you can use this for now, so feel free to leave methods for a future improvement, although I think at least all the AnyJsFunction cases can be handled already.

Thank you for letting me know!
I’ll revisit this when I work on implementing support for other functions or methods

arendjr

Nice work! Just left a few comments.

crates/biome_js_analyze/src/lint/nursery/no_floating_promises.rs

arendjr · 2025-01-20T10:54:46Z

crates/biome_js_analyze/src/lint/nursery/no_floating_promises.rs

+            ctx.metadata().applicability(),
+            markup! { "Add await operator." }.to_owned(),
+            mutation,
+        ))


I think this may potentially create invalid syntax if it's done inside a non-async function. Can we check for that?

@arendjr thank you for the review!
That's a valid point. Applying the change inside a non-async function could indeed lead to invalid syntax.
Is there an easy way to handle this in Biome?

For typeScript-eslint, it offers 2 fixes:

Add a void operator to safely ignore the result.

Add an await operator to make it asynchronous.

see: playground example

Providing two fixes seems like a reasonable and user-friendly approach.
Is it possible to provide multiple fixes using the action() function in Biome?

I think lint rules can only offer a single fix mechanism, but we have assists which might be better to what you're trying to do. What do you think is better here, @ematipico ?

When you say 'assists,' do you mean they are another kind of note or suggestion that the diagnostic function can generate? Or is there a separate mechanism for assists altogether?

At the moment our infrastructure doesn't offer multiple fixes from a single rule, that's valid for the whole analyzer infrastructure (linter and assist).

Some rules change their fix based on a situational need, maybe it's something we could evaluate.

I believe this is the first time we are in need of such a case. We could definitely evaluate adding such a feature, but not in this PR, as it requires some thoughts about how they should behave.

I see, so you’d like me to traverse up the tree from the expression node to find the nearest function and check if it is async, correct?

Yeah, that’s probably the easiest solution.

For typeScript-eslint, it offers 2 fixes: ... see: playground example

Sorry if I'm butting in annoyingly, but just to answer about what ESLint / typescript-eslint is doing:

We intentionally don't have adding await as a fix, because that's not a safe fix to apply in code - even if it would be syntactically valid

Example: the code might have been relying on Promise to float, like in a fire-and-forget pattern

ESLint / typescript-eslint doesn't have the ability to provide multiple fixes. Those two "fix" buttons are to apply suggestions, which are not surfaced in the CLI - only in integrations such as editors.

I do think it's kind of confusing that the typescript-eslint playground phrases a suggestion as "fix". Filed typescript-eslint/typescript-eslint#10700. Thanks! 💙

Thanks for the great advice! I think the suggestions you speak of indeed map best to Biome’s assists, although we support unsafe fixes too, so I wouldn’t mind if the rule offered the await fix as an unsafe one.

If it gets too hairy, I also don’t mind merging this PR without any fixes/assists, so we can focus on improving the analysis part first.

@arendjr
I have added a check to determine whether an expression is in an async function before suggesting an unsafe fix

arendjr · 2025-01-20T10:56:40Z

crates/biome_js_analyze/src/lint/nursery/no_floating_promises.rs

+                return false;
+            };
+
+            let AnyJsBindingDeclaration::JsFunctionDeclaration(func_decl) = any_js_binding_decl


We have AnyFunctionLike that you can use for detecting other kinds of function/method declarations. I understand without further type-level inference there will be few situations where you can use this for now, so feel free to leave methods for a future improvement, although I think at least all the AnyJsFunction cases can be handled already.

ematipico · 2025-01-20T11:59:05Z

This PR should go against the next branch. Can you please do so @kaykdm ?

Also, the instructions for providing a changelog are different there, can you please follow them?

…tion

arendjr · 2025-01-23T11:35:58Z

Thank you!

kaykdm · 2025-01-23T13:52:36Z

@arendjr
Thank you for the review!
Next, I will work on implementing support for arrow functions and will likely create a PR tomorrow.

ematipico · 2025-01-23T14:02:17Z

@kaykdm can you create an issue task to keep track of these works?

kaykdm · 2025-01-24T01:16:21Z

@kaykdm can you create an issue task to keep track of these works?

created!
#4956

github-actions bot added A-CLI Area: CLI A-Project Area: project A-Linter Area: linter L-JavaScript Language: JavaScript and super languages A-Diagnostic Area: diagnostocis labels Jan 19, 2025

dyc3 changed the title ~~feat(lint): implment no-floating-promises rule for reguler async func…~~ feat(lint): implment no-floating-promises rule for reguler async function Jan 19, 2025

kaykdm commented Jan 20, 2025

View reviewed changes

kaykdm marked this pull request as ready for review January 20, 2025 09:45

arendjr reviewed Jan 20, 2025

View reviewed changes

kaykdm changed the base branch from main to next January 20, 2025 12:31

kaykdm force-pushed the no-floating-promises branch from 93549da to a56a19d Compare January 20, 2025 13:26

arendjr changed the title ~~feat(lint): implment no-floating-promises rule for reguler async function~~ feat(lint): implement no-floating-promises rule for reguler async function Jan 20, 2025

arendjr changed the title ~~feat(lint): implement no-floating-promises rule for reguler async function~~ feat(lint): implement noFloatingPromises rule for reguler async function Jan 20, 2025

kaykdm added 5 commits January 22, 2025 21:28

feat(lint): implment no-floating-promises rule for reguler async func…

e8e9143

…tion

fix some docs and enable the rule for JavaScript files

70e50cc

rebase onto next

a110c4f

Add test case for non-async function that returns Promise

1cb349f

Add Changelog for noFloatingPromises

c9ca841

kaykdm force-pushed the no-floating-promises branch from 3c411a0 to c9ca841 Compare January 22, 2025 12:34

JoshuaKGoldberg mentioned this pull request Jan 22, 2025

Website: Playground buttons for applying suggestions should not say "fix" typescript-eslint/typescript-eslint#10700

Closed

2 tasks

suggest unsafe-fix only when the expression is in an async function

3768f87

arendjr approved these changes Jan 23, 2025

View reviewed changes

arendjr merged commit d400d69 into biomejs:next Jan 23, 2025
12 checks passed

kaykdm deleted the no-floating-promises branch January 23, 2025 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(lint): implement `noFloatingPromises` rule for reguler `async` function #4911

feat(lint): implement `noFloatingPromises` rule for reguler `async` function #4911

kaykdm commented Jan 19, 2025 •

edited

Loading

codspeed-hq bot commented Jan 19, 2025 •

edited

Loading

kaykdm Jan 20, 2025

arendjr Jan 20, 2025

kaykdm Jan 20, 2025

arendjr left a comment

arendjr Jan 20, 2025

kaykdm Jan 20, 2025 •

edited

Loading

arendjr Jan 20, 2025

kaykdm Jan 22, 2025

ematipico Jan 22, 2025 •

edited

Loading

kaykdm Jan 22, 2025

arendjr Jan 22, 2025

JoshuaKGoldberg Jan 22, 2025

arendjr Jan 23, 2025 •

edited

Loading

kaykdm Jan 23, 2025

arendjr Jan 20, 2025

ematipico commented Jan 20, 2025

arendjr commented Jan 23, 2025

kaykdm commented Jan 23, 2025

ematipico commented Jan 23, 2025

kaykdm commented Jan 24, 2025

feat(lint): implement noFloatingPromises rule for reguler async function #4911

feat(lint): implement noFloatingPromises rule for reguler async function #4911

Conversation

kaykdm commented Jan 19, 2025 • edited Loading

Summary

Current Scope:

Example of invalid code:

Example of valid code:

Remaining TODOs for Future PRs

codspeed-hq bot commented Jan 19, 2025 • edited Loading

Merging #4911 will not alter performance

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arendjr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaykdm Jan 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arendjr Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico commented Jan 20, 2025

arendjr commented Jan 23, 2025

kaykdm commented Jan 23, 2025

ematipico commented Jan 23, 2025

kaykdm commented Jan 24, 2025

feat(lint): implement `noFloatingPromises` rule for reguler `async` function #4911

feat(lint): implement `noFloatingPromises` rule for reguler `async` function #4911

kaykdm commented Jan 19, 2025 •

edited

Loading

codspeed-hq bot commented Jan 19, 2025 •

edited

Loading

kaykdm Jan 20, 2025 •

edited

Loading

ematipico Jan 22, 2025 •

edited

Loading

arendjr Jan 23, 2025 •

edited

Loading