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(linter): implement useExplicitFunctionReturnType #3990

Merged
merged 14 commits into from
Sep 20, 2024
Merged
10 changes: 10 additions & 0 deletions crates/biome_cli/src/execute/migrate/eslint_any_rule_to_biome.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

42 changes: 31 additions & 11 deletions crates/biome_configuration/src/analyzer/linter/rules.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3368,6 +3368,10 @@ pub struct Nursery {
#[serde(skip_serializing_if = "Option::is_none")]
pub use_deprecated_reason:
Option<RuleConfiguration<biome_graphql_analyze::options::UseDeprecatedReason>>,
#[doc = "Require explicit return types on functions and class methods."]
#[serde(skip_serializing_if = "Option::is_none")]
pub use_explicit_function_return_type:
Option<RuleConfiguration<biome_js_analyze::options::UseExplicitFunctionReturnType>>,
#[doc = "Disallows package private imports."]
#[serde(skip_serializing_if = "Option::is_none")]
pub use_import_restrictions:
Expand Down Expand Up @@ -3431,6 +3435,7 @@ impl Nursery {
"useConsistentCurlyBraces",
"useConsistentMemberAccessibility",
"useDeprecatedReason",
"useExplicitFunctionReturnType",
"useImportRestrictions",
"useSortedClasses",
"useStrictMode",
Expand Down Expand Up @@ -3461,7 +3466,7 @@ impl Nursery {
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[21]),
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[24]),
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[25]),
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[28]),
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[29]),
];
const ALL_RULES_AS_FILTERS: &'static [RuleFilter<'static>] = &[
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[0]),
Expand Down Expand Up @@ -3495,6 +3500,7 @@ impl Nursery {
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[28]),
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[29]),
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[30]),
RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[31]),
];
#[doc = r" Retrieves the recommended rules"]
pub(crate) fn is_recommended_true(&self) -> bool {
Expand Down Expand Up @@ -3641,31 +3647,36 @@ impl Nursery {
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[25]));
}
}
if let Some(rule) = self.use_import_restrictions.as_ref() {
if let Some(rule) = self.use_explicit_function_return_type.as_ref() {
if rule.is_enabled() {
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[26]));
}
}
if let Some(rule) = self.use_sorted_classes.as_ref() {
if let Some(rule) = self.use_import_restrictions.as_ref() {
if rule.is_enabled() {
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[27]));
}
}
if let Some(rule) = self.use_strict_mode.as_ref() {
if let Some(rule) = self.use_sorted_classes.as_ref() {
if rule.is_enabled() {
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[28]));
}
}
if let Some(rule) = self.use_trim_start_end.as_ref() {
if let Some(rule) = self.use_strict_mode.as_ref() {
if rule.is_enabled() {
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[29]));
}
}
if let Some(rule) = self.use_valid_autocomplete.as_ref() {
if let Some(rule) = self.use_trim_start_end.as_ref() {
if rule.is_enabled() {
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[30]));
}
}
if let Some(rule) = self.use_valid_autocomplete.as_ref() {
if rule.is_enabled() {
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[31]));
}
}
index_set
}
pub(crate) fn get_disabled_rules(&self) -> FxHashSet<RuleFilter<'static>> {
Expand Down Expand Up @@ -3800,31 +3811,36 @@ impl Nursery {
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[25]));
}
}
if let Some(rule) = self.use_import_restrictions.as_ref() {
if let Some(rule) = self.use_explicit_function_return_type.as_ref() {
if rule.is_disabled() {
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[26]));
}
}
if let Some(rule) = self.use_sorted_classes.as_ref() {
if let Some(rule) = self.use_import_restrictions.as_ref() {
if rule.is_disabled() {
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[27]));
}
}
if let Some(rule) = self.use_strict_mode.as_ref() {
if let Some(rule) = self.use_sorted_classes.as_ref() {
if rule.is_disabled() {
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[28]));
}
}
if let Some(rule) = self.use_trim_start_end.as_ref() {
if let Some(rule) = self.use_strict_mode.as_ref() {
if rule.is_disabled() {
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[29]));
}
}
if let Some(rule) = self.use_valid_autocomplete.as_ref() {
if let Some(rule) = self.use_trim_start_end.as_ref() {
if rule.is_disabled() {
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[30]));
}
}
if let Some(rule) = self.use_valid_autocomplete.as_ref() {
if rule.is_disabled() {
index_set.insert(RuleFilter::Rule(Self::GROUP_NAME, Self::GROUP_RULES[31]));
}
}
index_set
}
#[doc = r" Checks if, given a rule name, matches one of the rules contained in this category"]
Expand Down Expand Up @@ -3965,6 +3981,10 @@ impl Nursery {
.use_deprecated_reason
.as_ref()
.map(|conf| (conf.level(), conf.get_options())),
"useExplicitFunctionReturnType" => self
.use_explicit_function_return_type
.as_ref()
.map(|conf| (conf.level(), conf.get_options())),
"useImportRestrictions" => self
.use_import_restrictions
.as_ref()
Expand Down
1 change: 1 addition & 0 deletions crates/biome_diagnostics_categories/src/categories.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ define_categories! {
"lint/nursery/useConsistentCurlyBraces": "https://biomejs.dev/linter/rules/use-consistent-curly-braces",
"lint/nursery/useConsistentMemberAccessibility": "https://biomejs.dev/linter/rules/use-consistent-member-accessibility",
"lint/nursery/useDeprecatedReason": "https://biomejs.dev/linter/rules/use-deprecated-reason",
"lint/nursery/useExplicitFunctionReturnType": "https://biomejs.dev/linter/rules/use-explicit-function-return-type",
"lint/nursery/useImportRestrictions": "https://biomejs.dev/linter/rules/use-import-restrictions",
"lint/nursery/useJsxCurlyBraceConvention": "https://biomejs.dev/linter/rules/use-jsx-curly-brace-convention",
"lint/nursery/useSortedClasses": "https://biomejs.dev/linter/rules/use-sorted-classes",
Expand Down
2 changes: 2 additions & 0 deletions crates/biome_js_analyze/src/lint/nursery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ pub mod use_aria_props_supported_by_role;
pub mod use_component_export_only_modules;
pub mod use_consistent_curly_braces;
pub mod use_consistent_member_accessibility;
pub mod use_explicit_function_return_type;
pub mod use_import_restrictions;
pub mod use_sorted_classes;
pub mod use_strict_mode;
Expand Down Expand Up @@ -50,6 +51,7 @@ declare_lint_group! {
self :: use_component_export_only_modules :: UseComponentExportOnlyModules ,
self :: use_consistent_curly_braces :: UseConsistentCurlyBraces ,
self :: use_consistent_member_accessibility :: UseConsistentMemberAccessibility ,
self :: use_explicit_function_return_type :: UseExplicitFunctionReturnType ,
self :: use_import_restrictions :: UseImportRestrictions ,
self :: use_sorted_classes :: UseSortedClasses ,
self :: use_strict_mode :: UseStrictMode ,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
use biome_analyze::{
context::RuleContext, declare_lint_rule, Ast, Rule, RuleDiagnostic, RuleSource,
};
use biome_console::markup;
use biome_js_semantic::HasClosureAstNode;
use biome_js_syntax::AnyJsBinding;
use biome_js_syntax::{AnyJsFunction, JsGetterClassMember, JsMethodClassMember};
use biome_rowan::{declare_node_union, AstNode, TextRange};

declare_lint_rule! {
/// Require explicit return types on functions and class methods.
///
/// Functions in TypeScript often don't need to be given an explicit return type annotation.
/// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

/// 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.
///
/// This rule enforces that functions do have an explicit return type annotation.
///
/// ## Examples
///
/// ### Invalid
///
/// ```ts,expect_diagnostic
/// // Should indicate that no value is returned (void)
/// function test() {
/// return;
/// }
/// ```
///
/// ```ts,expect_diagnostic
/// // Should indicate that a number is returned
/// var fn = function () {
/// return 1;
/// };
/// ```
///
/// ```ts,expect_diagnostic
/// // Should indicate that a string is returned
/// var arrowFn = () => 'test';
/// ```
///
/// ```ts,expect_diagnostic
/// class Test {
/// // Should indicate that no value is returned (void)
/// method() {
/// return;
/// }
/// }
/// ```
///
/// ```ts,expect_diagnostic
/// // Should indicate that no value is returned (void)
/// function test(a: number) {
/// a += 1;
/// }
/// ```
///
/// ### Valid
/// ```ts
/// // No return value should be expected (void)
/// function test(): void {
/// return;
/// }
/// ```
///
/// ```ts
/// // A return value of type number
/// var fn = function (): number {
/// return 1;
/// }
/// ```
///
/// ```ts
/// // A return value of type string
/// var arrowFn = (): string => 'test';
/// ```
///
/// ```ts
/// class Test {
/// // No return value should be expected (void)
/// method(): void {
/// return;
/// }
/// }
/// ```
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

///
pub UseExplicitFunctionReturnType {
version: "next",
name: "useExplicitFunctionReturnType",
language: "ts",
recommended: false,
sources: &[RuleSource::EslintTypeScript("explicit-function-return-type")],
}
}

declare_node_union! {
pub AnyJsFunctionAndMethod = AnyJsFunction | JsMethodClassMember | JsGetterClassMember
}

impl Rule for UseExplicitFunctionReturnType {
type Query = Ast<AnyJsFunctionAndMethod>;
type State = TextRange;
type Signals = Option<Self::State>;
type Options = ();

fn run(ctx: &RuleContext<Self>) -> Self::Signals {
let node = ctx.query();
match node {
AnyJsFunctionAndMethod::AnyJsFunction(func) => {
if func.return_type_annotation().is_some() {
return None;
}

let func_range = func.syntax().text_range();
if let Ok(Some(AnyJsBinding::JsIdentifierBinding(id))) = func.id() {
return Some(TextRange::new(
func_range.start(),
id.syntax().text_range().end(),
));
}

Some(func_range)
}
AnyJsFunctionAndMethod::JsMethodClassMember(method) => {
if method.return_type_annotation().is_some() {
return None;
}

Some(method.node_text_range())
}
AnyJsFunctionAndMethod::JsGetterClassMember(getter) => {
if getter.return_type().is_some() {
return None;
}

Some(getter.node_text_range())
}
}
}

fn diagnostic(_: &RuleContext<Self>, state: &Self::State) -> Option<RuleDiagnostic> {
Some(
RuleDiagnostic::new(
rule_category!(),
state,
markup! {
"Missing return type on function."
},
)
.note(markup! {
"Require explicit return types on functions and class methods."
}),
)
}
}
1 change: 1 addition & 0 deletions crates/biome_js_analyze/src/options.rs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
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;
}
}
Comment on lines +1 to +28
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Loading
Loading