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

feat(lint): support assigned functions in noFloatingPromises #4958

Merged
merged 1 commit into from
Jan 24, 2025
Merged

feat(lint): support assigned functions in noFloatingPromises #4958

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
141 changes: 100 additions & 41 deletions crates/biome_js_analyze/src/lint/nursery/no_floating_promises.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@ use biome_js_factory::make;
use biome_js_semantic::SemanticModel;
use biome_js_syntax::{
binding_ext::AnyJsBindingDeclaration, AnyJsExpression, AnyJsName, AnyTsName, AnyTsReturnType,
AnyTsType, JsArrowFunctionExpression, JsCallExpression, JsExpressionStatement,
JsFunctionDeclaration, JsMethodClassMember, JsMethodObjectMember, JsStaticMemberExpression,
JsSyntaxKind, TsReturnTypeAnnotation,
AnyTsType, AnyTsVariableAnnotation, JsArrowFunctionExpression, JsCallExpression,
JsExpressionStatement, JsFunctionDeclaration, JsMethodClassMember, JsMethodObjectMember,
JsStaticMemberExpression, JsSyntaxKind, JsVariableDeclarator, TsReturnTypeAnnotation,
};
use biome_rowan::{AstNode, AstSeparatedList, BatchMutationExt, SyntaxNodeCast, TriviaPieceKind};

Expand Down Expand Up @@ -45,6 +45,15 @@ declare_lint_rule! {
/// returnsPromise().then(() => {});
/// ```
///
/// ```ts,expect_diagnostic
/// const returnsPromise = async (): Promise<string> => {
/// return 'value';
/// }
/// async function returnsPromiseInAsyncFunction() {
/// returnsPromise().then(() => {});
/// }
/// ```
///
/// ### Valid
///
/// ```ts
Expand Down Expand Up @@ -195,24 +204,27 @@ fn is_callee_a_promise(callee: &AnyJsExpression, model: &SemanticModel) -> bool
let Some(any_js_binding_decl) = binding.tree().declaration() else {
return false;
};

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

return is_function_a_promise(&func_decl);
match any_js_binding_decl {
AnyJsBindingDeclaration::JsFunctionDeclaration(func_decl) => {
is_function_a_promise(&func_decl)
}
AnyJsBindingDeclaration::JsVariableDeclarator(js_var_decl) => {
is_variable_initializer_a_promise(&js_var_decl)
|| is_variable_annotation_a_promise(&js_var_decl)
}
_ => false,
}
}
AnyJsExpression::JsStaticMemberExpression(static_member_expr) => {
return is_member_expression_callee_a_promise(static_member_expr, model);
is_member_expression_callee_a_promise(static_member_expr, model)
}
_ => {}
_ => false,
}
false
}

fn is_function_a_promise(func_decl: &JsFunctionDeclaration) -> bool {
func_decl.async_token().is_some() || is_return_type_promise(func_decl.return_type_annotation())
func_decl.async_token().is_some()
|| is_return_type_a_promise(func_decl.return_type_annotation())
}

/// Checks if a TypeScript return type annotation is a `Promise`.
Expand Down Expand Up @@ -240,7 +252,7 @@ fn is_function_a_promise(func_decl: &JsFunctionDeclaration) -> bool {
/// ```typescript
/// function doesNotReturnPromise(): void {}
/// ```
fn is_return_type_promise(return_type: Option<TsReturnTypeAnnotation>) -> bool {
fn is_return_type_a_promise(return_type: Option<TsReturnTypeAnnotation>) -> bool {
return_type
.and_then(|ts_return_type_anno| ts_return_type_anno.ty().ok())
.and_then(|any_ts_return_type| match any_ts_return_type {
Expand Down Expand Up @@ -360,32 +372,7 @@ fn is_member_expression_callee_a_promise(
return false;
};

match callee {
AnyJsExpression::JsStaticMemberExpression(static_member_expr) => {
return is_member_expression_callee_a_promise(&static_member_expr, model);
}
AnyJsExpression::JsIdentifierExpression(ident_expr) => {
let Some(reference) = ident_expr.name().ok() else {
return false;
};
let Some(binding) = model.binding(&reference) else {
return false;
};

let Some(any_js_binding_decl) = binding.tree().declaration() else {
return false;
};

let AnyJsBindingDeclaration::JsFunctionDeclaration(func_decl) = any_js_binding_decl
else {
return false;
};
return is_function_a_promise(&func_decl);
}
_ => {}
}

false
is_callee_a_promise(&callee, model)
}

/// Checks if the given `JsExpressionStatement` is within an async function.
Expand Down Expand Up @@ -423,3 +410,75 @@ fn is_in_async_function(node: &JsExpressionStatement) -> bool {
})
.is_some()
}

/// Checks if the initializer of a `JsVariableDeclarator` is an async function.
///
/// Example TypeScript code that would return `true`:
///
/// ```typescript
/// const returnsPromise = async (): Promise<string> => {
/// return 'value';
/// }
///
/// const returnsPromise = async function (): Promise<string> {
/// return 'value'
/// }
/// ```
fn is_variable_initializer_a_promise(js_variable_declarator: &JsVariableDeclarator) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

It's been a while that you're contributing to the code base, so you might have noticed that you have to deal with many Result coming from the syntax.

I strongly suggest changing all these functions to return a Option instead, and start doing .ok()? for all those syntax APIs that return SyntaxResult.

This is an accepted approach inside Biome, especially inside the lint rules. Doing so, you low down the cognitive complexity of the code, and the code style it's in line with the existing rules.

In this case:

fn is_variable_initializer_a_promise(js_variable_declarator: &JsVariableDeclarator) -> Option<bool> {}

is_variable_initializer_a_promise(node)?

You're free to apply this suggestion to the whole file in the next PR, or in this PR, but we should do it eventually.

Copy link
Contributor Author

@kaykdm kaykdm Jan 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the review!
That makes sense. I'll apply that suggestion to the whole file in the next PR.

let Some(initializer_clause) = &js_variable_declarator.initializer() else {
return false;
};
let Ok(expr) = initializer_clause.expression() else {
return false;
};
match expr {
AnyJsExpression::JsArrowFunctionExpression(arrow_func) => {
arrow_func.async_token().is_some()
|| is_return_type_a_promise(arrow_func.return_type_annotation())
}
AnyJsExpression::JsFunctionExpression(func_expr) => {
func_expr.async_token().is_some()
|| is_return_type_a_promise(func_expr.return_type_annotation())
}
_ => false,
}
}

/// Checks if a `JsVariableDeclarator` has a TypeScript type annotation of `Promise`.
///
///
/// Example TypeScript code that would return `true`:
/// ```typescript
/// const returnsPromise: () => Promise<string> = () => {
/// return Promise.resolve("value")
/// }
/// ```
fn is_variable_annotation_a_promise(js_variable_declarator: &JsVariableDeclarator) -> bool {
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Change it to Option<bool>, use .ok()? every time you extract a type from a node, and the caller will use ?

js_variable_declarator
.variable_annotation()
.and_then(|anno| match anno {
AnyTsVariableAnnotation::TsTypeAnnotation(type_anno) => Some(type_anno),
_ => None,
})
.and_then(|ts_type_anno| ts_type_anno.ty().ok())
.and_then(|any_ts_type| match any_ts_type {
AnyTsType::TsFunctionType(func_type) => {
func_type
.return_type()
.ok()
.and_then(|return_type| match return_type {
AnyTsReturnType::AnyTsType(AnyTsType::TsReferenceType(ref_type)) => {
ref_type.name().ok().map(|name| match name {
AnyTsName::JsReferenceIdentifier(identifier) => {
identifier.has_name("Promise")
}
_ => false,
})
}
_ => None,
})
}
_ => None,
})
.unwrap_or(false)
}
23 changes: 22 additions & 1 deletion crates/biome_js_analyze/tests/specs/nursery/noFloatingPromises/invalid.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,25 @@ function returnsPromiseWithoutAsync(): Promise<string> {
}


returnsPromiseWithoutAsync()
returnsPromiseWithoutAsync()


const returnsPromiseAssignedArrowFunction = async (): Promise<string> => {
return 'value';
};

returnsPromiseAssignedArrowFunction();

const returnsPromiseAssignedFunction = async function (): Promise<string> {
return 'value'
}

async function returnsPromiseAssignedFunctionInAsyncFunction(): Promise<void> {
returnsPromiseAssignedFunction().then(() => { })
}

const returnsPromiseAssignedArrowFunctionAnnotatedType: () => Promise<string> = () => {
return Promise.resolve('value');
};

returnsPromiseAssignedArrowFunctionAnnotatedType();
74 changes: 74 additions & 0 deletions crates/biome_js_analyze/tests/specs/nursery/noFloatingPromises/invalid.ts.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,27 @@ function returnsPromiseWithoutAsync(): Promise<string> {


returnsPromiseWithoutAsync()


const returnsPromiseAssignedArrowFunction = async (): Promise<string> => {
return 'value';
};

returnsPromiseAssignedArrowFunction();

const returnsPromiseAssignedFunction = async function (): Promise<string> {
return 'value'
}

async function returnsPromiseAssignedFunctionInAsyncFunction(): Promise<void> {
returnsPromiseAssignedFunction().then(() => { })
}

const returnsPromiseAssignedArrowFunctionAnnotatedType: () => Promise<string> = () => {
return Promise.resolve('value');
};

returnsPromiseAssignedArrowFunctionAnnotatedType();
```

# Diagnostics
Expand Down Expand Up @@ -136,6 +157,59 @@ invalid.ts:27:1 lint/nursery/noFloatingPromises ━━━━━━━━━━

> 27 │ returnsPromiseWithoutAsync()
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
28 │

i This happens when a Promise is not awaited, lacks a `.catch` or `.then` rejection handler, or is not explicitly ignored using the `void` operator.


```

```
invalid.ts:34:1 lint/nursery/noFloatingPromises ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

! A "floating" Promise was found, meaning it is not properly handled and could lead to ignored errors or unexpected behavior.

32 │ };
33 │
> 34 │ returnsPromiseAssignedArrowFunction();
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
35 │
36 │ const returnsPromiseAssignedFunction = async function (): Promise<string> {

i This happens when a Promise is not awaited, lacks a `.catch` or `.then` rejection handler, or is not explicitly ignored using the `void` operator.


```

```
invalid.ts:41:3 lint/nursery/noFloatingPromises FIXABLE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

! A "floating" Promise was found, meaning it is not properly handled and could lead to ignored errors or unexpected behavior.

40 │ async function returnsPromiseAssignedFunctionInAsyncFunction(): Promise<void> {
> 41 │ returnsPromiseAssignedFunction().then(() => { })
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
42 │ }
43 │

i This happens when a Promise is not awaited, lacks a `.catch` or `.then` rejection handler, or is not explicitly ignored using the `void` operator.

i Unsafe fix: Add await operator.

41 │ ··await·returnsPromiseAssignedFunction().then(()·=>·{·})
│ ++++++

```

```
invalid.ts:48:1 lint/nursery/noFloatingPromises ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

! A "floating" Promise was found, meaning it is not properly handled and could lead to ignored errors or unexpected behavior.

46 │ };
47 │
> 48 │ returnsPromiseAssignedArrowFunctionAnnotatedType();
│ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

i This happens when a Promise is not awaited, lacks a `.catch` or `.then` rejection handler, or is not explicitly ignored using the `void` operator.

Expand Down
Loading