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 #4647

Closed
wants to merge 2 commits into from
Closed
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

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.

209 changes: 161 additions & 48 deletions crates/biome_js_analyze/src/lint/nursery/use_explicit_type.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
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,
Expand All @@ -11,23 +11,23 @@ use biome_js_syntax::{
};
use biome_js_syntax::{
AnyJsFunction, JsGetterClassMember, JsGetterObjectMember, JsMethodClassMember,
JsMethodObjectMember, TsCallSignatureTypeMember, TsDeclareFunctionDeclaration,
JsMethodObjectMember, JsRestParameter, TsCallSignatureTypeMember, TsDeclareFunctionDeclaration,
TsDeclareFunctionExportDefaultDeclaration, TsGetterSignatureClassMember,
TsMethodSignatureClassMember, TsMethodSignatureTypeMember,
TsMethodSignatureClassMember, TsMethodSignatureTypeMember, TsThisParameter,
};
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 @@ -156,6 +156,45 @@ 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;
/// }
/// ```
///
/// ```ts,expect_diagnostic
/// class Test {
/// constructor(a) {}
/// }
/// ```
///
/// ```ts,expect_diagnostic
/// declare module "foo" {
/// export default function bar(a): string;
/// }
/// ```
///
///
/// ### Valid
/// ```ts
/// // No return value should be expected (void)
Expand Down Expand Up @@ -258,12 +297,12 @@ declare_lint_rule! {
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 AnyCallableWithReturn =
pub FunctionSignaturePart =
AnyJsFunction
| JsMethodClassMember
| JsMethodObjectMember
Expand All @@ -275,11 +314,60 @@ declare_node_union! {
| TsGetterSignatureClassMember
| TsDeclareFunctionDeclaration
| TsDeclareFunctionExportDefaultDeclaration
| JsFormalParameter
| JsRestParameter
| TsThisParameter
Comment on lines +317 to +319
Copy link
Contributor Author

@kaykdm kaykdm Nov 27, 2024

Choose a reason for hiding this comment

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

to continue the discussion from this

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

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

@Conaclos
Thank you for the suggestion! I decided to implement matching directly against JsFormalParameter, JsRestParameter, and TsThisParameter because it keeps the code simpler and more straightforward.

Copy link
Member

@Conaclos Conaclos Nov 28, 2024

Choose a reason for hiding this comment

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

The issue with (1) is that you have to check again if the argument can be untyped (That corresponds to the same heuristics that allows untyped return type). I noticed you didn't implemnt these exceptions.

For insatnce, teh following code should be accepted:

const f: MyFunction = (a) => a;

Also, creating a diagnostic for every argument / return type seems noisy?

}

pub enum UseExplicitTypeState {
MissingReturnType(TextRange),
MissingArgumentnType(TextRange, String),
Copy link
Contributor Author

@kaykdm kaykdm Nov 27, 2024

Choose a reason for hiding this comment

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

to continue the discussion from this

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

@Conaclos

Thank you for the feedback! I decided to keep the same implementation for the following reasons:

  • Even if we use the token directly instead of passing a String, we still need to call the to_string() method to pass the value to markup! {}, which will allocate a string regardless. So, both approaches result in string allocation.
  • Additionally, in cases like function foo({a: b}), we need to handle tokens differently. For instance, the first token in this scenario would be '{'.

}

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

fn note_reason(&self) -> MarkupBuf {
match &self {
UseExplicitTypeState::MissingReturnType(_) => {
(markup! {"Declaring the return type makes the code self-documenting and can speed up TypeScript type checking."}).to_owned()
}
UseExplicitTypeState::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 {
UseExplicitTypeState::MissingReturnType(_) => {
(markup! {"Add a return type annotation."}).to_owned()
}
UseExplicitTypeState::MissingArgumentnType(_, _) => {
(markup! {"Add type annotations to the function arguments."}).to_owned()
}
}
}
}

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

Expand All @@ -291,7 +379,7 @@ impl Rule for UseExplicitType {

let node = ctx.query();
match node {
AnyCallableWithReturn::AnyJsFunction(func) => {
FunctionSignaturePart::AnyJsFunction(func) => {
if func.return_type_annotation().is_some() {
return None;
}
Expand All @@ -318,97 +406,122 @@ impl Rule for UseExplicitType {

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

Some(func_range)
Some(UseExplicitTypeState::MissingReturnType(func_range))
}
AnyCallableWithReturn::JsMethodClassMember(method) => {
FunctionSignaturePart::JsMethodClassMember(method) => {
if method.return_type_annotation().is_some() {
return None;
}

Some(method.node_text_range())
Some(UseExplicitTypeState::MissingReturnType(
method.node_text_range(),
))
}
AnyCallableWithReturn::JsGetterClassMember(getter) => {
FunctionSignaturePart::JsGetterClassMember(getter) => {
if getter.return_type().is_some() {
return None;
}

Some(getter.node_text_range())
Some(UseExplicitTypeState::MissingReturnType(
getter.node_text_range(),
))
}
AnyCallableWithReturn::JsMethodObjectMember(method) => {
FunctionSignaturePart::JsMethodObjectMember(method) => {
if method.return_type_annotation().is_some() {
return None;
}

Some(method.node_text_range())
Some(UseExplicitTypeState::MissingReturnType(
method.node_text_range(),
))
}
AnyCallableWithReturn::JsGetterObjectMember(getter) => {
FunctionSignaturePart::JsGetterObjectMember(getter) => {
if getter.return_type().is_some() {
return None;
}

Some(getter.node_text_range())
Some(UseExplicitTypeState::MissingReturnType(
getter.node_text_range(),
))
}
AnyCallableWithReturn::TsMethodSignatureTypeMember(member) => {
FunctionSignaturePart::TsMethodSignatureTypeMember(member) => {
if member.return_type_annotation().is_some() {
return None;
}

Some(member.range())
Some(UseExplicitTypeState::MissingReturnType(member.range()))
}
AnyCallableWithReturn::TsCallSignatureTypeMember(member) => {
FunctionSignaturePart::TsCallSignatureTypeMember(member) => {
if member.return_type_annotation().is_some() {
return None;
}
Some(member.range())
Some(UseExplicitTypeState::MissingReturnType(member.range()))
}
AnyCallableWithReturn::TsMethodSignatureClassMember(member) => {
FunctionSignaturePart::TsMethodSignatureClassMember(member) => {
if member.return_type_annotation().is_some() {
return None;
}
Some(member.range())
Some(UseExplicitTypeState::MissingReturnType(member.range()))
}
AnyCallableWithReturn::TsGetterSignatureClassMember(member) => {
FunctionSignaturePart::TsGetterSignatureClassMember(member) => {
if member.return_type().is_some() {
return None;
}
Some(member.range())
Some(UseExplicitTypeState::MissingReturnType(member.range()))
}
AnyCallableWithReturn::TsDeclareFunctionDeclaration(decl) => {
FunctionSignaturePart::TsDeclareFunctionDeclaration(decl) => {
if decl.return_type_annotation().is_some() {
return None;
}
Some(decl.range())
Some(UseExplicitTypeState::MissingReturnType(decl.range()))
}
AnyCallableWithReturn::TsDeclareFunctionExportDefaultDeclaration(decl) => {
FunctionSignaturePart::TsDeclareFunctionExportDefaultDeclaration(decl) => {
if decl.return_type_annotation().is_some() {
return None;
}
Some(decl.range())
Some(UseExplicitTypeState::MissingReturnType(decl.range()))
}
FunctionSignaturePart::JsFormalParameter(param) => {
if param.type_annotation().is_some() {
return None;
}
Some(UseExplicitTypeState::MissingArgumentnType(
param.range(),
param.text(),
))
}
FunctionSignaturePart::JsRestParameter(param) => {
if param.type_annotation().is_some() {
return None;
}
Some(UseExplicitTypeState::MissingArgumentnType(
param.range(),
param.text(),
))
}
FunctionSignaturePart::TsThisParameter(param) => {
if param.type_annotation().is_some() {
return None;
}
Some(UseExplicitTypeState::MissingArgumentnType(
param.range(),
param.text(),
))
}
}
}

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
Loading