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

Conversation

kaykdm
Copy link
Contributor

@kaykdm kaykdm commented Sep 29, 2024

Summary

related: #2017

This PR introduces the ability to detect higher-order functions in the useExplicitFunctionReturnType rule, specifically functions that return other functions.

Limitations:

Nested statement handling is not implemented: The current implementation does not cover cases where functions are returned inside conditional or branching logic (e.g., if statements or switch statements).

Example of unsupported case:

function example() {
    if (condition) {
        return function() {}; // This return inside the 'if' statement is not detected
    }
}

Question:

Should biome support nested statements like if or switch when checking for higher-order functions?
This would involve inspecting return statements within those control structures, adding complexity but potentially broadening the detection capability.

@github-actions github-actions bot added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Sep 29, 2024
…ed statements) in useExplicitFunctionReturnType
@kaykdm kaykdm force-pushed the support-higher-order-functions branch from 456e464 to dd41388 Compare September 29, 2024 12:30
Copy link

codspeed-hq bot commented Sep 29, 2024

CodSpeed Performance Report

Merging #4117 will improve performances by 7.61%

Comparing kaykdm:support-higher-order-functions (ed3ec30) with main (295efb9)

Summary

⚡ 1 improvements
✅ 100 untouched benchmarks

Benchmarks breakdown

Benchmark main kaykdm:support-higher-order-functions Change
db_17847247775464589309.json[cached] 13.7 ms 12.8 ms +7.61%

@Conaclos
Copy link
Member

Conaclos commented Sep 29, 2024

Should biome support nested statements like if or switch when checking for higher-order functions?

Isolated Declarations and the ESLint rule seem to recognize these patterns.
However, this could add a lot of complexity to the rule.

I think the reason d'être for allowing higher order function is to simplify the writing of functional code with curried functions. It is fulfilled by checking only body with a single return statement function f() { return function():void {} } or arrow expressions such as () => (): void => {}.

We can still reevaluate this decision once users make the case for it.

@kaykdm
Copy link
Contributor Author

kaykdm commented Sep 30, 2024

Should biome support nested statements like if or switch when checking for higher-order functions?

Isolated Declarations and the ESLint rule seem to recognize these patterns. However, this could add a lot of complexity to the rule.

I think the reason d'être for allowing haigher order function is to simplify the writing of functional code with curried functions. It is fulfilled by checking only body with a single return statement function f() { return function():void {} } or arrow expressions such as () => (): void => {}.

We can still reevaluate this decision once users make the case for it.

I agree with you, adding this could introduce a lot of complexity. Since we can always reevaluate later based on user feedback, I think this PR is ready for review.

@@ -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.

Comment on lines 342 to 353
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.

@Conaclos Conaclos changed the title feat(linter): support higher-order function detection (excluding nest… feat(useExplicitFunctionReturnType): support higher-order function Sep 30, 2024
@Conaclos Conaclos added the A-Changelog Area: changelog label Sep 30, 2024
@Conaclos Conaclos merged commit 53f1420 into biomejs:main Sep 30, 2024
13 checks passed
@kaykdm kaykdm deleted the support-higher-order-functions branch October 1, 2024 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Changelog Area: changelog A-Linter Area: linter L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants