feat(linter): implement useExplicitFunctionReturnType #3990

kaykdm · 2024-09-19T11:53:25Z

Summary

Related issue: #2017

This PR introduces the @typescript-eslint/explicit-function-return-type rule to Biome. Currently, it only enforces the basic cases without any additional options configured.

In future PRs, I will implement each option for this rule separately. Due to the complexity of the implementation, handling each option individually will allow for thorough testing and easier review.

original implementation

Todos

~~I will implement each option for this rule separately in different PRs~~

~~Implement allowExpressions option~~
~~Implement allowTypedFunctionExpression soption~~
~~Implement allowHigherOrderFunctions option~~
~~Implement allowDirectConstAssertionInArrowFunctions option~~
~~Implement allowConciseArrowFunctionExpressionsStartingWithVoid option~~
~~Implement allowFunctionsWithoutTypeParameters option~~
~~Implement allowedNames option~~
~~Implement allowIIFEs option~~

kaykdm · 2024-09-19T11:53:51Z

crates/biome_js_analyze/tests/specs/nursery/useExplicitFunctionReturnType/valid.ts

+function test(): void {
+  return;
+}
+
+var fn = function (): number {
+  return 1;
+};
+
+var arrowFn = (): string => 'test';
+
+class Test {
+  constructor() {}
+  get prop(): number {
+    return 1;
+  }
+  set prop() {}
+  method(): void {
+    return;
+  }
+  arrow = (): string => 'arrow';
+}


same test cases
https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/tests/rules/explicit-function-return-type.test.ts#L10-L41

kaykdm · 2024-09-19T11:54:31Z

crates/biome_js_analyze/tests/specs/nursery/useExplicitFunctionReturnType/invalid.ts

+function test(a: number, b: number) {
+	return;
+}
+
+function test() {
+	return;
+}
+
+var fn = function () {
+	return 1;
+};
+
+var arrowFn = () => "test";
+
+class Test {
+	constructor() {}
+	get prop() {
+		return 1;
+	}
+	set prop() {}
+	method() {
+		return;
+	}
+	arrow = () => "arrow";
+	private method() {
+		return;
+	}
+}


same test cases
https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/tests/rules/explicit-function-return-type.test.ts#L808-L917

arendjr

Love to have this rule!

arendjr · 2024-09-19T11:59:42Z

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

+    /// Leaving off the return type is less code to read or write and allows the compiler to infer it from the contents of the function.
+    ///
+    /// However, explicit return types do make it visually more clear what type is returned by a function.
+    /// They can also speed up TypeScript type checking performance in large codebases with many large functions.


Might be nice to add that explicit return types also reduce the chance of bugs by asserting the return type and it avoids surprising "action at a distance", where changing the body of one function may cause failures inside another function.

Thanks for the suggestion! I've added that point to the description!

arendjr · 2024-09-19T12:01:05Z

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

+    ///     return;
+    ///   }
+    /// }
+    /// ```


What about a function without any explicit return statement? Is the type annotation required there too? If not, might be good to point out in the examples.

What about a function without any explicit return statement? Is the type annotation required there too?

Yes it is required, but I think it's nice to include it in the example. I added it to the invalid example!

codspeed-hq · 2024-09-19T19:35:48Z

CodSpeed Performance Report

Merging #3990 will degrade performances by 6.5%

_{Comparing kaykdm:use-explicit-function-return-type (b7a1fe4) with main (d137533)}

Summary

⚡ 1 improvements
❌ 1 regressions
✅ 105 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`main`	`kaykdm:use-explicit-function-return-type`	Change
❌	`js_analyzer[lint_13640784270757307929.ts]`	30.2 ms	32.3 ms	-6.5%
⚡	`react.production.min_3378072959512366797.js[uncached]`	2.2 ms	2.1 ms	+6.12%

arendjr · 2024-09-20T07:59:15Z

Great stuff! Do you want to create a CHANGELOG entry?

Conaclos · 2024-09-20T10:22:59Z

Hi! Thanks for the good stuff!

In future PRs, I will implement each option for this rule separately. Due to the complexity of the implementation, handling each option individually will allow for thorough testing and easier review.

We usually refrain from implementing a rule's options unless they are widely used in the community. The number of options provided by explicit-function-return-type looks like a code smell to me.

Taking a look at GitHub Code search:

explicit-function-return-type is used 74k times
allowConciseArrowFunctionExpressionsStartingWithVoid is used 800 times
allowDirectConstAssertionInArrowFunctions is used 600 times
allowFunctionsWithoutTypeParameters is used 50 times
allowHigherOrderFunctions is used [1.6k times](path:eslint "explicit-function-return-type" allowHigherOrderFunctions)
allowIIFEs is used 90 times
allowTypedFunctionExpressions is used 2.8k times
allowedNames is used 750 times
allowExpressions is used 10k times

Without taking account that some of these occurrences actually set the default value...
Only allowExpressions seems widely used. We could skip its implementation and diverge from ESLint by allowing callbacks without return types by default.

Moreover, I would like that useExplicitFunctionReturnType be compatible with the new isolated declaration mode of TypeScript. Some options can conflict with this mode.

Thus, I think we should not implement any option for this rule.
Note that we should still accept:

Higher order function without return type (ESLint default behavior) - We could restrict our implementation to arrow function.
Typed function expression (ESLint default behavior)
Direct const assertion in arrow function (ESLint default behavior)
Callbacks without return types (ESLint allowExpressions)

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

…return type

kaykdm · 2024-09-20T11:52:28Z

Thus, I think we should not implement any option for this rule.
Note that we should still accept:

Higher order function without return type (ESLint default behavior) - We could restrict our implementation to arrow function.
Typed function expression (ESLint default behavior)
Direct const assertion in arrow function (ESLint default behavior)
Callbacks without return types (ESLint allowExpressions)

I totally agree with this. I implemented the same rule in Oxc Linter, but it was quite painful and I ran into some issues. That being said, I’m happy to work on these options you mentioned in separate PRs.

Conaclos

Thanks! LGTM :)

kaykdm added 5 commits September 16, 2024 21:38

Run just new-js-lintrule useExplicitFunctionReturnType

0f211fe

Add rule description and examples

9543f15

Handle base cases

e16f055

Handle class method

c067a6d

Merge branch 'main' into use-explicit-function-return-type

7d41348

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

kaykdm commented Sep 19, 2024

View reviewed changes

arendjr approved these changes Sep 19, 2024

View reviewed changes

kaykdm added 5 commits September 20, 2024 09:36

Add a valid example of a function without any explicit return statement

8a300e4

Add more description

2fe2884

Add more description

ab88eee

Merge branch 'main' into use-explicit-function-return-type

6d15b19

Run just gen-lint

899fd56

github-actions bot added the A-CLI Area: CLI label Sep 20, 2024

Conaclos reviewed Sep 20, 2024

View reviewed changes

crates/biome_js_analyze/src/lint/nursery/use_explicit_function_return_type.rs Outdated Show resolved Hide resolved

crates/biome_js_analyze/src/lint/nursery/use_explicit_function_return_type.rs Outdated Show resolved Hide resolved

kaykdm and others added 3 commits September 20, 2024 20:22

Add JsMethodObjectMember and JsGetterObjectMember to handle function …

a3353cb

…return type

Update diagnostic notes

999f7ba

Merge branch 'main' into use-explicit-function-return-type

c1141c7

Run cargo insta review

b7a1fe4

Conaclos approved these changes Sep 20, 2024

View reviewed changes

Conaclos merged commit b76cb41 into biomejs:main Sep 20, 2024
11 of 12 checks passed

kaykdm deleted the use-explicit-function-return-type branch September 21, 2024 01:34

kaykdm mentioned this pull request Sep 21, 2024

📎 Implement nursery/useExplicitTypes - typescript-eslint/explicit-module-boundary-types and typescript-eslint/explicit-function-return-type #2017

Open

eMerzh added a commit to eMerzh/biome that referenced this pull request Sep 21, 2024

feat(linter): implement noLowerCaseEnum (biomejs#3990)

e8ca1e8

WomB0ComB0 mentioned this pull request Nov 1, 2024

[Snyk] Upgrade @biomejs/biome from 1.9.2 to 1.9.3 WomB0ComB0/portfolio#157

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(linter): implement useExplicitFunctionReturnType #3990

feat(linter): implement useExplicitFunctionReturnType #3990

kaykdm commented Sep 19, 2024 •

edited

Loading

kaykdm Sep 19, 2024

kaykdm Sep 19, 2024

arendjr left a comment

arendjr Sep 19, 2024

kaykdm Sep 20, 2024

arendjr Sep 19, 2024

kaykdm Sep 20, 2024

codspeed-hq bot commented Sep 19, 2024 •

edited

Loading

arendjr commented Sep 20, 2024

Conaclos commented Sep 20, 2024

kaykdm commented Sep 20, 2024

Conaclos left a comment

feat(linter): implement useExplicitFunctionReturnType #3990

feat(linter): implement useExplicitFunctionReturnType #3990

Conversation

kaykdm commented Sep 19, 2024 • edited Loading

Summary

Todos

kaykdm Sep 19, 2024

Choose a reason for hiding this comment

kaykdm Sep 19, 2024

Choose a reason for hiding this comment

arendjr left a comment

Choose a reason for hiding this comment

arendjr Sep 19, 2024

Choose a reason for hiding this comment

kaykdm Sep 20, 2024

Choose a reason for hiding this comment

arendjr Sep 19, 2024

Choose a reason for hiding this comment

kaykdm Sep 20, 2024

Choose a reason for hiding this comment

codspeed-hq bot commented Sep 19, 2024 • edited Loading

CodSpeed Performance Report

Merging #3990 will degrade performances by 6.5%

Summary

Benchmarks breakdown

arendjr commented Sep 20, 2024

Conaclos commented Sep 20, 2024

kaykdm commented Sep 20, 2024

Conaclos left a comment

Choose a reason for hiding this comment

kaykdm commented Sep 19, 2024 •

edited

Loading

codspeed-hq bot commented Sep 19, 2024 •

edited

Loading