feat(biome_js_analyze): `useShorthandFunctionType` #670

emab · 2023-11-04T08:48:53Z

Summary

Fixes 📎 Implement lint/useShorthandFunctionType - typescript-eslint/prefer-function-type #48

WIP

Test Plan

Conaclos

Thanks for taking this! It is not an easy rule!

I suggested some change. Don't hesitate to ask more details if you need them :)

crates/biome_js_analyze/src/analyzers/nursery/use_shorthand_function_type.rs

Conaclos · 2023-11-04T10:12:10Z

crates/biome_js_analyze/src/analyzers/nursery/use_shorthand_function_type.rs

+}
+
+impl Rule for UseShorthandFunctionType {
+    type Query = Semantic<Query>;


I think we can directly query TsCallSignatureTypeMember?
This could save unnecessary lookup :)

Moreover, we can use Ast instead of Semantic. Semantic is required when you use the semantic model ctx.model().

Suggested change

type Query = Semantic<Query>;

type Query = Ast<TsCallSignatureTypeMember>;

Conaclos · 2023-11-04T10:22:02Z

crates/biome_js_analyze/src/analyzers/nursery/use_shorthand_function_type.rs

+        let (range, message, note) = match state {
+            RuleState::TsInterfaceDeclaration(range, parameters, return_type_syntax) => (
+                range,
+                markup! {
+                    "Prefer function type over interface."
+                },
+                markup! {"Interface only has a call signature, you should use a function type instead."},
+            ),
+            RuleState::TsObjectType(range, parameters, return_type_syntax) => (
+                range,
+                markup! {
+                    "Prefer function type over object type."
+                },
+                markup! {"Object only has a call signature, you should use a function type instead."},
+            ),
+        };
+        Some(RuleDiagnostic::new(rule_category!(), range, message).note(note))


i think we can use the same diagnostic for interface and object type. What do you think?

Suggested change

let (range, message, note) = match state {

RuleState::TsInterfaceDeclaration(range, parameters, return_type_syntax) => (

range,

markup! {

"Prefer function type over interface."

},

markup! {"Interface only has a call signature, you should use a function type instead."},

),

RuleState::TsObjectType(range, parameters, return_type_syntax) => (

range,

markup! {

"Prefer function type over object type."

},

markup! {"Object only has a call signature, you should use a function type instead."},

),

};

Some(RuleDiagnostic::new(rule_category!(), range, message).note(note))

Some(RuleDiagnostic::new(rule_category!(), ctx.query().range(), markup! {

"Use a function type instead of a call signature."

}).note(markup { "Types containing only a call signature can be shortened to a function type." }))

Conaclos · 2023-11-04T10:27:04Z

crates/biome_js_analyze/src/analyzers/nursery/use_shorthand_function_type.rs

+
+        match query {
+            Query::TsInterfaceDeclaration(interface_declaration) => {
+                if interface_declaration.members().len() == 1
+                    && interface_declaration.extends_clause().is_none()
+                {
+                    if let Some(TsCallSignatureTypeMember(call_signature)) =
+                        interface_declaration.members().first()
+                    {
+                        return Some(RuleState::TsInterfaceDeclaration(
+                            query.range(),
+                            call_signature.parameters().ok()?.items(),
+                            call_signature.return_type_annotation()?.ty(),
+                        ));
+                    }
+                }
+                None
+            }
+            Query::JsFormalParameter(parameter) => {
+                if let Some(TsObjectType(ts_object)) = parameter.type_annotation()?.ty().ok() {
+                    if ts_object.members().len() == 1 {
+                        if let Some(ts_call_signature_type_member) = ts_object
+                            .members()
+                            .first()?
+                            .as_ts_call_signature_type_member()
+                        {
+                            return Some(RuleState::TsObjectType(
+                                ts_object.range(),
+                                ts_call_signature_type_member.parameters().ok()?.items(),
+                                ts_call_signature_type_member
+                                    .return_type_annotation()?.ty(),
+                            ));
+                        }
+                    }
+                }
+                None
+            }
+        }


If we change the query to TsCallSignatureTypeMember, we have to verify that it is the only member of the type.
Taking a look at js.ungramar, We note that TsCallSignatureTypeMember is part of the union AnyTsMember, AnyTsMember is only used in TsTypeMemberList. Thus, we could just retrieve the list and check that there is only one entry.

Note that here I assumed that State = ().

By the way, I think you are right about your concern on returning this. We should verify that the call signature doesn't return this. If it returns this, then we ignore the code.

Suggested change

match query {

Query::TsInterfaceDeclaration(interface_declaration) => {

if interface_declaration.members().len() == 1

&& interface_declaration.extends_clause().is_none()

{

if let Some(TsCallSignatureTypeMember(call_signature)) =

interface_declaration.members().first()

{

return Some(RuleState::TsInterfaceDeclaration(

query.range(),

call_signature.parameters().ok()?.items(),

call_signature.return_type_annotation()?.ty(),

));

}

}

None

}

Query::JsFormalParameter(parameter) => {

if let Some(TsObjectType(ts_object)) = parameter.type_annotation()?.ty().ok() {

if ts_object.members().len() == 1 {

if let Some(ts_call_signature_type_member) = ts_object

.members()

.first()?

.as_ts_call_signature_type_member()

{

return Some(RuleState::TsObjectType(

ts_object.range(),

ts_call_signature_type_member.parameters().ok()?.items(),

ts_call_signature_type_member

.return_type_annotation()?.ty(),

));

}

}

}

None

}

}

(query.parent::<TsTypeMemberList>()?.len() == 1).then_some(())

Wow this is much more simple! I'll need to add some more logic to cover a couple of edge cases.

Conaclos · 2023-11-04T10:32:05Z

crates/biome_js_analyze/src/analyzers/nursery/use_shorthand_function_type.rs

+        Some(RuleDiagnostic::new(rule_category!(), range, message).note(note))
+    }
+
+    fn action(ctx: &RuleContext<Self>, state: &Self::State) -> Option<JsRuleAction> {


I let you rework this part with the new suggested changes :)

emab · 2023-11-05T10:15:15Z

Thanks for taking this! It is not an easy rule!

I suggested some change. Don't hesitate to ask more details if you need them :)

Brilliant, thanks for the comments. I'll have a look at this later today hopefully!

emab · 2023-11-05T21:31:52Z

@Conaclos I've just pushed up a new (and hopefully improved!) version.

emab · 2023-11-06T09:32:04Z

crates/biome_js_analyze/src/analyzers/nursery/use_shorthand_function_type.rs

+        if let Some(interface_decl) = ts_type_member_list.parent::<TsInterfaceDeclaration>() {
+            let type_alias_declaration = ts_type_alias_declaration(
+                make::token(T![type]).with_trailing_trivia([(TriviaPieceKind::Whitespace, " ")]),
+                interface_decl.id().ok()?,
+                make::token(T![=]).with_trailing_trivia([(TriviaPieceKind::Whitespace, " ")]),
+                AnyTsType::from(convert_ts_call_signature_type_member_to_function_type(
+                    node,
+                )?),
+            )
+            .build();
+
+            mutation.replace_node(
+                AnyJsDeclarationClause::from(interface_decl),
+                AnyJsDeclarationClause::from(type_alias_declaration),
+            );
+
+            return Some(JsRuleAction {
+                category: ActionCategory::QuickFix,
+                applicability: Applicability::Always,
+                message: markup! { "Convert empty interface to type alias." }.to_owned(),
+                mutation,
+            });
+        }
+
+        if let Some(ts_object_type) = ts_type_member_list.parent::<TsObjectType>() {
+            let new_function_type = convert_ts_call_signature_type_member_to_function_type(node)?;
+
+            mutation.replace_node(
+                AnyTsType::from(ts_object_type),
+                AnyTsType::from(new_function_type),
+            );
+
+            return Some(JsRuleAction {
+                category: ActionCategory::QuickFix,
+                applicability: Applicability::Always,
+                message: markup! { "Convert object type to type alias." }.to_owned(),
+                mutation,
+            });
+        }


I wanted to use match here, but I ran into an issue where the expected parent type was AnyJsStatement so I couldn't also compare to the TsObjectType which is the parent when the function signature is inside of the object type element.

I think it is ok :)

Conaclos

Sorry for the delay.

I think we are almost there :)

I think it lacks a last tthing: examples with comments.

// Comment
interface Example {
 (): string;
}

// Comment
type F = { (): number } // Trailing comment

Also, we should not suggest a fix when a comment is removed. I think it is the case for examples for inner comments:

interface Example2 {
 // Inner comment
 (): string; // Inner trailing comment
}

type G = {
    // Inner comment
    (): number // Inner trailing comment
}

You can check if a node have comments with something like: node.syntax().has_comments().

crates/biome_js_analyze/src/analyzers/nursery/use_shorthand_function_type.rs

Conaclos · 2023-11-13T12:47:17Z

crates/biome_js_analyze/src/analyzers/nursery/use_shorthand_function_type.rs

+        if let Some(interface_decl) = ts_type_member_list.parent::<TsInterfaceDeclaration>() {
+            let type_alias_declaration = ts_type_alias_declaration(
+                make::token(T![type]).with_trailing_trivia([(TriviaPieceKind::Whitespace, " ")]),
+                interface_decl.id().ok()?,
+                make::token(T![=]).with_trailing_trivia([(TriviaPieceKind::Whitespace, " ")]),
+                AnyTsType::from(convert_ts_call_signature_type_member_to_function_type(
+                    node,
+                )?),
+            )
+            .build();
+
+            mutation.replace_node(
+                AnyJsDeclarationClause::from(interface_decl),
+                AnyJsDeclarationClause::from(type_alias_declaration),
+            );
+
+            return Some(JsRuleAction {
+                category: ActionCategory::QuickFix,
+                applicability: Applicability::Always,
+                message: markup! { "Convert empty interface to type alias." }.to_owned(),
+                mutation,
+            });
+        }
+
+        if let Some(ts_object_type) = ts_type_member_list.parent::<TsObjectType>() {
+            let new_function_type = convert_ts_call_signature_type_member_to_function_type(node)?;
+
+            mutation.replace_node(
+                AnyTsType::from(ts_object_type),
+                AnyTsType::from(new_function_type),
+            );
+
+            return Some(JsRuleAction {
+                category: ActionCategory::QuickFix,
+                applicability: Applicability::Always,
+                message: markup! { "Convert object type to type alias." }.to_owned(),
+                mutation,
+            });
+        }


I think it is ok :)

unvalley · 2023-11-25T08:37:43Z

@emab gentle bump, are you still interested in this? I can follow if you are busy :)

emab · 2023-11-25T08:49:02Z

Oops, didn't see the comment. I'll tidy this up and get it ready when I can!

netlify · 2023-12-04T17:23:10Z

✅ Deploy Preview for rad-torte-839a59 ready!

Name	Link
🔨 Latest commit	`e7a18f7`
🔍 Latest deploy log	https://app.netlify.com/sites/rad-torte-839a59/deploys/656e0fb123d6e7000912051f
😎 Deploy Preview	https://deploy-preview-670--rad-torte-839a59.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

emab · 2023-12-05T08:22:54Z

Thanks for taking this over, unfortunately I've had very little time 😞

github-actions bot added A-Project Area: project A-Linter Area: linter A-Website Area: website L-JavaScript Language: JavaScript and super languages A-Diagnostic Area: diagnostocis labels Nov 4, 2023

Conaclos reviewed Nov 4, 2023

View reviewed changes

emab changed the title ~~feat(biome_js_analyze): useShorthandFunctionType~~ feat(biome_js_analyze): useShorthandFunctionType Nov 5, 2023

feat(biome_js_analyze): useShorthandFunctionType

0c1bbb1

emab force-pushed the main branch from f6536e1 to 0c1bbb1 Compare November 5, 2023 21:31

emab commented Nov 6, 2023

View reviewed changes

Conaclos reviewed Nov 13, 2023

View reviewed changes

ematipico added 2 commits December 4, 2023 17:18

Merge remote-tracking branch 'origin/main' into emab/main

e6a5fb9

address feedback

b622584

ematipico added 2 commits December 4, 2023 17:24

Merge remote-tracking branch 'origin/main' into emab/main

fae7b0e

fix another merge

6168ca5

ematipico mentioned this pull request Dec 4, 2023

📎 More tests cases for useShorthandFunctionType #1045

Closed

update snapshots

e7a18f7

ematipico merged commit 81761f8 into biomejs:main Dec 4, 2023
11 of 13 checks passed

This was referenced Dec 26, 2023

📎 Implement lint/useShorthandFunctionType - typescript-eslint/prefer-function-type #354

Closed

chore(lint/useShorthandFunctionType): ignore if node has inner comments #1365

Merged

Conaclos added the A-Changelog Area: changelog label Jan 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(biome_js_analyze): `useShorthandFunctionType` #670

feat(biome_js_analyze): `useShorthandFunctionType` #670

emab commented Nov 4, 2023

Conaclos left a comment

Conaclos Nov 4, 2023

Conaclos Nov 4, 2023

Conaclos Nov 4, 2023

emab Nov 5, 2023

Conaclos Nov 4, 2023

emab commented Nov 5, 2023

emab commented Nov 5, 2023

emab Nov 6, 2023

Conaclos Nov 13, 2023

Conaclos left a comment

Conaclos Nov 13, 2023

unvalley commented Nov 25, 2023

emab commented Nov 25, 2023

netlify bot commented Dec 4, 2023 •

edited

Loading

emab commented Dec 5, 2023

	type Query = Semantic<Query>;
	type Query = Ast<TsCallSignatureTypeMember>;

feat(biome_js_analyze): useShorthandFunctionType #670

feat(biome_js_analyze): useShorthandFunctionType #670

Conversation

emab commented Nov 4, 2023

Summary

Test Plan

Conaclos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emab commented Nov 5, 2023

emab commented Nov 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Conaclos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unvalley commented Nov 25, 2023

emab commented Nov 25, 2023

netlify bot commented Dec 4, 2023 • edited Loading

✅ Deploy Preview for rad-torte-839a59 ready!

emab commented Dec 5, 2023

feat(biome_js_analyze): `useShorthandFunctionType` #670

feat(biome_js_analyze): `useShorthandFunctionType` #670

netlify bot commented Dec 4, 2023 •

edited

Loading