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(useExplicitType): support explicit function argument types #4463

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
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

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

2 changes: 1 addition & 1 deletion crates/biome_configuration/src/analyzer/linter/rules.rs

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

211 changes: 171 additions & 40 deletions crates/biome_js_analyze/src/lint/nursery/use_explicit_type.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
use biome_analyze::{
context::RuleContext, declare_lint_rule, Ast, Rule, RuleDiagnostic, RuleSource,
};
use biome_console::markup;
use biome_console::{markup, MarkupBuf};
use biome_js_semantic::HasClosureAstNode;
use biome_js_syntax::{
AnyJsBinding, AnyJsExpression, AnyJsFunctionBody, AnyJsStatement, AnyTsType, JsCallExpression,
JsFileSource, JsFormalParameter, JsInitializerClause, JsLanguage, JsObjectExpression,
JsParenthesizedExpression, JsPropertyClassMember, JsPropertyObjectMember, JsStatementList,
JsSyntaxKind, JsVariableDeclarator,
JsParameters, JsParenthesizedExpression, JsPropertyClassMember, JsPropertyObjectMember,
JsStatementList, JsSyntaxKind, JsVariableDeclarator,
};
use biome_js_syntax::{
AnyJsFunction, JsGetterClassMember, JsGetterObjectMember, JsMethodClassMember,
Expand All @@ -16,16 +16,16 @@ use biome_js_syntax::{
use biome_rowan::{declare_node_union, AstNode, SyntaxNode, SyntaxNodeOptionExt, TextRange};

declare_lint_rule! {
/// Require explicit return types on functions and class methods.
/// Require explicit argument and 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.
/// However, explicit argument and return types make it visually more clear what types a function accepts and returns.
/// They can also speed up TypeScript type checking performance in large codebases with many large functions.
/// 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.
/// Explicit types also reduce the chance of bugs by asserting both input and output types, 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.
/// This rule enforces that functions have explicit type annotations for both their arguments and return type.
///
/// ## Examples
///
Expand Down Expand Up @@ -105,6 +105,32 @@ declare_lint_rule! {
/// }
/// ```
///
/// The following pattern is considered incorrect code for missing an argument type on an function:
///
/// ```ts,expect_diagnostic
/// export function test(a: number, b): void {
/// return;
/// }
/// ```
///
/// ```ts,expect_diagnostic
/// export const test = (a): void => {
/// return;
/// }
/// ```
///
/// ```ts,expect_diagnostic
/// export default function test(a): void {
/// return;
/// }
/// ```
///
/// ```ts,expect_diagnostic
/// export default (a): void => {
/// return;
/// }
/// ```
///
/// ### Valid
/// ```ts
/// // No return value should be expected (void)
Expand All @@ -115,8 +141,8 @@ declare_lint_rule! {
///
/// ```ts
/// // A return value of type number
/// var fn = function (): number {
/// return 1;
/// var fn = function (a: number): number {
/// return a + 1;
/// }
/// ```
///
Expand Down Expand Up @@ -202,22 +228,87 @@ declare_lint_rule! {
/// }
/// ```
///
/// The following pattern is considered correct code because it includes both a return type and an argument type on an function:
///
/// ```ts
/// export function test(a: number, b: number): void {
/// return;
/// }
/// ```
///
/// ```ts
/// export default function test(obj: {a: string}): void {
/// return;
/// }
/// ```
///
/// ```ts
/// export default (a: SomeType): void => {
/// return;
/// }
/// ```
///
pub UseExplicitType {
version: "1.9.3",
name: "useExplicitType",
language: "ts",
recommended: false,
sources: &[RuleSource::EslintTypeScript("explicit-function-return-type")],
sources: &[RuleSource::EslintTypeScript("explicit-function-return-type"), RuleSource::EslintTypeScript("explicit-module-boundary-types")],
}
}

declare_node_union! {
pub AnyJsFunctionWithReturnType = AnyJsFunction | JsMethodClassMember | JsMethodObjectMember | JsGetterClassMember | JsGetterObjectMember
pub AnyJsFunctionWithReturnTypeOrJsParameters = AnyJsFunction | JsMethodClassMember | JsMethodObjectMember | JsGetterClassMember | JsGetterObjectMember | JsParameters
Copy link
Member

Choose a reason for hiding this comment

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

I've just noticed that some types are missing for checking if the return type is explicitly set:

  • TsMethodSignatureTypeMember
  • TsCallSignatureTypeMember
  • TsMethodSignatureClassMember
  • TsSetterSignatureClassMember
  • TsDeclareFunctionDeclaration
  • TsDeclareFunctionExportDefaultDeclaration

I wonder if directly matching against JsParameters is a good idea. I see two possible alternative approaches:

  1. matching directly against JsFormalParameter, JsRestParameter, and TsThisParameter
  2. matching against functions with parameters (We already have most of them, we need to add setter and constructors).

Personally I could choose (2) because this allows us to issue a single diagnostic when a parameter or a return type is absent on a function.
Also, similarly to return type, we should ignore callbacks.

Besides, we should think to a better name for this union because we will have to add properties and top-level variables. Maybe AnyJsTypeableEntity? Have you a better suggestion?

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 review, and apologies for the late response.

I've just noticed that some types are missing for checking if the return type is explicitly set:

I understand. I'll create a separate for this this.

I wonder if directly matching against JsParameters is a good idea. I see two possible alternative approaches:

I see your point. I'll revisit this after implementing the missing return types.

Maybe AnyJsTypeableEntity? Do you have a better suggestion?

AnyJsTypeableEntity looks good to me.

For now, I will make this PR draft.

}

pub struct UseExplicitTypeState {
range: TextRange,
cause: UseExplicitTypeCause,
}

enum UseExplicitTypeCause {
MissingReturnType,
MissingArgumentnType(String),
Copy link
Member

Choose a reason for hiding this comment

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

Usually we try to avoid allocating a string. You could directly take the token:

Suggested change
MissingArgumentnType(String),
MissingArgumentnType(JsSyntaxToken),

In this case we could rewrite UseExplicitTypeState as:

pub enum UseExplicitTypeState {
    MissingReturnType(TextRange),
    MissingArgumentnType(JsSyntaxToken),
}

Because we can get the range from a token.

}

impl UseExplicitTypeState {
fn title(&self) -> MarkupBuf {
match &self.cause {
UseExplicitTypeCause::MissingReturnType => {
(markup! {"Missing return type on function."}).to_owned()
}
UseExplicitTypeCause::MissingArgumentnType(name) => {
(markup! {"Argument '"{name}"' should be typed."}).to_owned()
}
}
}

fn note_reason(&self) -> MarkupBuf {
match &self.cause {
UseExplicitTypeCause::MissingReturnType => {
(markup! {"Declaring the return type makes the code self-documenting and can speed up TypeScript type checking."}).to_owned()
}
UseExplicitTypeCause::MissingArgumentnType(_) => {
(markup! {"Declaring the argument types makes the code self-documenting and can speed up TypeScript type checking."}).to_owned()
}
}
}

fn note_action(&self) -> MarkupBuf {
match &self.cause {
UseExplicitTypeCause::MissingReturnType => {
(markup! {"Add a return type annotation."}).to_owned()
}
UseExplicitTypeCause::MissingArgumentnType(_) => {
(markup! {"Add type annotations to the function arguments."}).to_owned()
}
}
}
}

impl Rule for UseExplicitType {
type Query = Ast<AnyJsFunctionWithReturnType>;
type State = TextRange;
type Query = Ast<AnyJsFunctionWithReturnTypeOrJsParameters>;
type State = UseExplicitTypeState;
type Signals = Option<Self::State>;
type Options = ();

Expand All @@ -229,7 +320,7 @@ impl Rule for UseExplicitType {

let node = ctx.query();
match node {
AnyJsFunctionWithReturnType::AnyJsFunction(func) => {
AnyJsFunctionWithReturnTypeOrJsParameters::AnyJsFunction(func) => {
if func.return_type_annotation().is_some() {
return None;
}
Expand All @@ -256,60 +347,71 @@ impl Rule for UseExplicitType {

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(),
));
return Some(UseExplicitTypeState {
range: TextRange::new(func_range.start(), id.syntax().text_range().end()),
cause: UseExplicitTypeCause::MissingReturnType,
});
}

Some(func_range)
Some(UseExplicitTypeState {
range: func_range,
cause: UseExplicitTypeCause::MissingReturnType,
})
}
AnyJsFunctionWithReturnType::JsMethodClassMember(method) => {
AnyJsFunctionWithReturnTypeOrJsParameters::JsMethodClassMember(method) => {
if method.return_type_annotation().is_some() {
return None;
}

Some(method.node_text_range())
Some(UseExplicitTypeState {
range: method.node_text_range(),
cause: UseExplicitTypeCause::MissingReturnType,
})
}
AnyJsFunctionWithReturnType::JsGetterClassMember(getter) => {
AnyJsFunctionWithReturnTypeOrJsParameters::JsGetterClassMember(getter) => {
if getter.return_type().is_some() {
return None;
}

Some(getter.node_text_range())
Some(UseExplicitTypeState {
range: getter.node_text_range(),
cause: UseExplicitTypeCause::MissingReturnType,
})
}
AnyJsFunctionWithReturnType::JsMethodObjectMember(method) => {
AnyJsFunctionWithReturnTypeOrJsParameters::JsMethodObjectMember(method) => {
if method.return_type_annotation().is_some() {
return None;
}

Some(method.node_text_range())
Some(UseExplicitTypeState {
range: method.node_text_range(),
cause: UseExplicitTypeCause::MissingReturnType,
})
}
AnyJsFunctionWithReturnType::JsGetterObjectMember(getter) => {
AnyJsFunctionWithReturnTypeOrJsParameters::JsGetterObjectMember(getter) => {
if getter.return_type().is_some() {
return None;
}

Some(getter.node_text_range())
Some(UseExplicitTypeState {
range: getter.node_text_range(),
cause: UseExplicitTypeCause::MissingReturnType,
})
}
AnyJsFunctionWithReturnTypeOrJsParameters::JsParameters(params) => {
if let Some(params_diagnostic) = check_function_parameters_type(params) {
return Some(params_diagnostic);
}
None
}
}
}

fn diagnostic(_: &RuleContext<Self>, state: &Self::State) -> Option<RuleDiagnostic> {
Some(
RuleDiagnostic::new(
rule_category!(),
state,
markup! {
"Missing return type on function."
},
)
.note(markup! {
"Declaring the return type makes the code self-documenting and can speed up TypeScript type checking."
})
.note(markup! {
"Add a return type annotation."
}),
RuleDiagnostic::new(rule_category!(), state.range, state.title())
.note(state.note_reason())
.note(state.note_action()),
)
}
}
Expand Down Expand Up @@ -550,3 +652,32 @@ fn is_type_assertion(syntax: &SyntaxNode<JsLanguage>) -> bool {
}
})
}

/// Checks if all parameters in a function have explicit type annotations.
///
/// This function iterates over the provided function parameters and examines each one
/// to determine if it has a type annotation. If any parameter is missing a type annotation,
/// the function returns a `Some(UseExplicitTypeState)` indicating the range of the parameter
/// and the cause of the issue. If all parameters have type annotations, it returns `None`.
///
/// # Arguments
///
/// * `parameters` - A reference to a `JsParameters` object representing the parameters of a function.
///
/// # Returns
///
/// * `Option<UseExplicitTypeState>` - `Some(UseExplicitTypeState)` if a parameter is missing a type annotation,
/// or `None` if all parameters have explicit type annotations.
fn check_function_parameters_type(parameters: &JsParameters) -> Option<UseExplicitTypeState> {
for p in parameters.items() {
let param = p.ok()?;
Comment on lines +672 to +673
Copy link
Member

Choose a reason for hiding this comment

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

We could avoid returning as soon as we met an error by iterating only over node without errors:

Suggested change
for p in parameters.items() {
let param = p.ok()?;
for p in parameters.items().into_iter().flatten() {

let formal_param = param.as_any_js_formal_parameter()?;
Copy link
Member

Choose a reason for hiding this comment

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

We should certainly handle the other parameter kinds (rest parameter and this parameter).

if formal_param.type_annotation().is_none() {
return Some(UseExplicitTypeState {
range: formal_param.range(),
cause: UseExplicitTypeCause::MissingArgumentnType(formal_param.text()),
});
}
}
None
}
Original file line number Diff line number Diff line change
Expand Up @@ -92,4 +92,16 @@ function fn() {
}

const x = { prop: () => {} }
const x = { bar: { prop: () => {} } }
const x = { bar: { prop: () => {} } }

export default (a): void => {
return;
}
export function test(a: number, b) {
return;
}
export class Test {
method(a): void {
return;
}
}
Loading