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.

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(useExplicitFunctionReturnType): support higher-order function #4117

Merged
merged 2 commits into from
Sep 30, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@ use biome_analyze::{
use biome_console::markup;
use biome_js_semantic::HasClosureAstNode;
use biome_js_syntax::{
AnyJsBinding, AnyJsExpression, AnyJsFunctionBody, AnyTsType, JsFileSource, JsSyntaxKind,
AnyJsBinding, AnyJsExpression, AnyJsFunctionBody, AnyJsStatement, AnyTsType, JsFileSource,
JsStatementList, JsSyntaxKind,
};
use biome_js_syntax::{
AnyJsFunction, JsGetterClassMember, JsGetterObjectMember, JsMethodClassMember,
Expand Down Expand Up @@ -68,6 +69,16 @@ declare_lint_rule! {
/// const func = (value: number) => ({ type: 'X', value }) as any;
/// ```
///
/// ```ts,expect_diagnostic
/// const arrowFn = () => () => {};
/// ```
///
/// ```ts,expect_diagnostic
/// const arrowFn = () => {
/// return () => { };
/// }
/// ```
///
/// ### Valid
/// ```ts
/// // No return value should be expected (void)
Expand Down Expand Up @@ -110,6 +121,15 @@ declare_lint_rule! {
/// (() => {})();
/// ```
///
/// ```ts
Copy link
Member

Choose a reason for hiding this comment

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

It is good to add examples to demonstrate the exceptions to the rule. However, I think we should also describe the exceptions to the rule in the description (before ## Examples). We should also document the previous exceptions to the rule introduced in your previous PRs.

/// const arrowFn = () => (): void => {};
/// ```
///
/// ```ts
/// const arrowFn = () => {
/// return (): void => { };
/// }
///
pub UseExplicitFunctionReturnType {
version: "next",
name: "useExplicitFunctionReturnType",
Expand Down Expand Up @@ -150,6 +170,10 @@ impl Rule for UseExplicitFunctionReturnType {
return None;
}

if is_higher_order_function(func) {
return None;
}

let func_range = func.syntax().text_range();
if let Ok(Some(AnyJsBinding::JsIdentifierBinding(id))) = func.id() {
return Some(TextRange::new(
Expand Down Expand Up @@ -266,3 +290,65 @@ fn is_function_used_in_argument_or_expression_list(func: &AnyJsFunction) -> bool
)
)
}

/// Checks whether the given function is a higher-order function, i.e., a function
/// that returns another function either directly in its body or as an expression.
///
/// A higher-order function is one that returns either a regular function or an arrow
/// function from within its body.
///
/// # Arguments
///
/// * `func` - A reference to an `AnyJsFunction` that represents the JavaScript function to inspect.
///
/// # Returns
///
/// * `true` if the function returns another function (either a regular function or an arrow function).
/// * `false` if it does not return a function or if the body is not a valid returnable function expression.
///
/// # Note
///
/// This function currently **does not support** detecting a return of a function
/// inside other statements like `if` statements or `switch` statements. It only detects
/// direct returns of functions or function returns in a straightforward function body.
fn is_higher_order_function(func: &AnyJsFunction) -> bool {
match func.body().ok() {
Some(AnyJsFunctionBody::AnyJsExpression(expr)) => {
matches!(
expr,
AnyJsExpression::JsArrowFunctionExpression(_)
| AnyJsExpression::JsFunctionExpression(_)
)
}
Some(AnyJsFunctionBody::JsFunctionBody(func_body)) => {
check_statements_for_function_return(func_body.statements())
}
_ => false,
}
}

/// Checks whether the given list of JavaScript statements contains a return statement
/// that returns a function expression (either a regular function or an arrow function).
///
/// # Arguments
///
/// * `statements` - A list of JavaScript statements (`JsStatementList`) to inspect.
///
/// # Returns
///
/// * `true` if the list contains a return statement with a function expression as its argument.
/// * `false` if no such return statement is found or if the list is empty.
fn check_statements_for_function_return(statements: JsStatementList) -> bool {
statements.into_iter().any(|statement| {
if let AnyJsStatement::JsReturnStatement(return_stmt) = statement {
if let Some(args) = return_stmt.argument() {
return matches!(
args,
AnyJsExpression::JsFunctionExpression(_)
| AnyJsExpression::JsArrowFunctionExpression(_)
);
}
}
false
})
Copy link
Member

Choose a reason for hiding this comment

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

I think we should check only the first statement because we have to report code like:

function f() {
  if (x) {
    return 0;
  }
  return () => {}
}

Don't forget to add a test to avoid any regression here.

}
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,41 @@ const func = (value: number) => ({ type: 'X', value }) as any;
const func = (value: number) => ({ type: 'X', value }) as Action;

export default () => {};
export default function () {}
export default function () {}

// check higher order functions
const arrowFn = () => () => {};
const arrowFn = () => function() {}
function fn() {
let str = "hey";
return function () {
return str;
};
}
const arrowFn = () => {
return () => { };
}

// does not support detecting a return of a function inside other statements like if, switch, etc.
const arrowFn = (a: number) => {
if (a === 1) {
return (): void => { };
} else {
return (): number => {
return a + 2
}
}
}
const arrowFn = (a: number) => {
switch (a) {
case 1: {
return (): void => { };
}
case 2: {
return (): void => { };
}
default: {
return (): void => { };
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,43 @@ const func = (value: number) => ({ type: 'X', value }) as Action;

export default () => {};
export default function () {}

// check higher order functions
const arrowFn = () => () => {};
const arrowFn = () => function() {}
function fn() {
let str = "hey";
return function () {
return str;
};
}
const arrowFn = () => {
return () => { };
}

// does not support detecting a return of a function inside other statements like if, switch, etc.
const arrowFn = (a: number) => {
if (a === 1) {
return (): void => { };
} else {
return (): number => {
return a + 2
}
}
}
const arrowFn = (a: number) => {
switch (a) {
case 1: {
return (): void => { };
}
case 2: {
return (): void => { };
}
default: {
return (): void => { };
}
}
}
```

# Diagnostics
Expand Down Expand Up @@ -307,6 +344,7 @@ invalid.ts:45:16 lint/nursery/useExplicitFunctionReturnType ━━━━━━
> 45 │ export default () => {};
│ ^^^^^^^^
46 │ export default function () {}
47 │

i Declaring the return type makes the code self-documenting and can speed up TypeScript type checking.

Expand All @@ -323,6 +361,133 @@ invalid.ts:46:16 lint/nursery/useExplicitFunctionReturnType ━━━━━━
45 │ export default () => {};
> 46 │ export default function () {}
│ ^^^^^^^^^^^^^^
47 │
48 │ // check higher order functions

i Declaring the return type makes the code self-documenting and can speed up TypeScript type checking.

i Add a return type annotation.


```

```
invalid.ts:49:23 lint/nursery/useExplicitFunctionReturnType ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

! Missing return type on function.

48 │ // check higher order functions
> 49 │ const arrowFn = () => () => {};
│ ^^^^^^^^
50 │ const arrowFn = () => function() {}
51 │ function fn() {

i Declaring the return type makes the code self-documenting and can speed up TypeScript type checking.

i Add a return type annotation.


```

```
invalid.ts:50:23 lint/nursery/useExplicitFunctionReturnType ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

! Missing return type on function.

48 │ // check higher order functions
49 │ const arrowFn = () => () => {};
> 50 │ const arrowFn = () => function() {}
│ ^^^^^^^^^^^^^
51 │ function fn() {
52 │ let str = "hey";

i Declaring the return type makes the code self-documenting and can speed up TypeScript type checking.

i Add a return type annotation.


```

```
invalid.ts:53:10 lint/nursery/useExplicitFunctionReturnType ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

! Missing return type on function.

51 │ function fn() {
52 │ let str = "hey";
> 53 │ return function () {
│ ^^^^^^^^^^^^^
> 54 │ return str;
> 55 │ };
│ ^
56 │ }
57 │ const arrowFn = () => {

i Declaring the return type makes the code self-documenting and can speed up TypeScript type checking.

i Add a return type annotation.


```

```
invalid.ts:58:10 lint/nursery/useExplicitFunctionReturnType ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

! Missing return type on function.

56 │ }
57 │ const arrowFn = () => {
> 58 │ return () => { };
│ ^^^^^^^^^
59 │ }
60 │

i Declaring the return type makes the code self-documenting and can speed up TypeScript type checking.

i Add a return type annotation.


```

```
invalid.ts:62:17 lint/nursery/useExplicitFunctionReturnType ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

! Missing return type on function.

61 │ // does not support detecting a return of a function inside other statements like if, switch, etc.
> 62 │ const arrowFn = (a: number) => {
│ ^^^^^^^^^^^^^^^^
> 63 │ if (a === 1) {
...
> 69 │ }
> 70 │ }
│ ^
71 │ const arrowFn = (a: number) => {
72 │ switch (a) {

i Declaring the return type makes the code self-documenting and can speed up TypeScript type checking.

i Add a return type annotation.


```

```
invalid.ts:71:17 lint/nursery/useExplicitFunctionReturnType ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━

! Missing return type on function.

69 │ }
70 │ }
> 71 │ const arrowFn = (a: number) => {
│ ^^^^^^^^^^^^^^^^
> 72 │ switch (a) {
...
> 80 │ return (): void => { };
> 81 │ }
> 82 │ }
> 83 │ }
│ ^

i Declaring the return type makes the code self-documenting and can speed up TypeScript type checking.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,18 @@ fn(function () {});
(() => {
console.log("This is an IIFE");
})();
setTimeout(function() { console.log("Hello!"); }, 1000);
setTimeout(function() { console.log("Hello!"); }, 1000);


// check higher order functions
const arrowFn = () => (): void => {};
const arrowFn = () => function(): void {}
function fn() {
let str = "hey";
return function (): string {
return str;
};
}
const arrowFn = () => {
return (): void => { };
}
Original file line number Diff line number Diff line change
Expand Up @@ -61,4 +61,18 @@ fn(function () {});
console.log("This is an IIFE");
})();
setTimeout(function() { console.log("Hello!"); }, 1000);


// check higher order functions
const arrowFn = () => (): void => {};
const arrowFn = () => function(): void {}
function fn() {
let str = "hey";
return function (): string {
return str;
};
}
const arrowFn = () => {
return (): void => { };
}
```
Loading