feat(lint): `noMisleadingInstantiator` #215

unvalley · 2023-09-09T20:02:29Z

Summary

Closes #46

As I described in the rule doc, this rule triggers warnings in the following scenarios:

When a class has a method named new.
When an interface defines a method named constructor or new that returns the interface type.
When a type alias has a constructor method.

~~So, the lint name can be changed more descriptive, but no-misused-new-and-constructor is a bit subtle.~~
~~no-misleading-instantiation?~~
no-misleading-instantiator

Test Plan

cargo test -p rome_js_analyze -- no_misleading_instantiator

crates/rome_js_analyze/src/analyzers/nursery/no_misused_new.rs

Conaclos · 2023-09-09T20:17:36Z

crates/rome_js_analyze/src/analyzers/nursery/no_misused_new.rs

+            }
+            DeclarationQuery::TsDeclareStatement(decl) => {
+                let decl = decl.declaration().ok()?;
+                let decl = decl.as_js_class_declaration()?;


This class declaration should already be handled in DeclarationQuery::JsClassDeclaration? Am I missing something?

That's not handled by JsClassDeclaration but I don't know why, need to investigate it.

crates/rome_js_analyze/src/analyzers/nursery/no_misused_new.rs

Conaclos · 2023-09-09T20:30:49Z

crates/rome_js_analyze/src/analyzers/nursery/no_misused_new.rs

+                            let any_ts_type = construct.type_annotation()?.ty().ok()?;
+                            let reference_type = any_ts_type.as_ts_reference_type()?;
+                            let return_type_ident = extract_return_type_ident(&reference_type)?;
+                            if interface_ident.text_trimmed() == return_type_ident.text_trimmed() {


Shoudl we also handle the case where the type is the this type?
For example:

interface I { new(): this; }

Conaclos · 2023-09-09T20:35:47Z

crates/rome_js_analyze/src/analyzers/nursery/no_misused_new.rs

+                        }
+                        AnyTsTypeMember::TsMethodSignatureTypeMember(method) => {
+                            let method_name = method.name().ok()?;
+                            let method_name = method_name.as_js_literal_member_name()?.syntax();


To simplify the code, we could implement a method AnyJsClassMemberName::name with a similar implementation to AnyJsObjectMemberName::name.

Conaclos · 2023-09-09T20:40:52Z

crates/rome_js_analyze/src/analyzers/nursery/no_misused_new.rs

+}
+
+declare_node_union! {
+    pub(crate) DeclarationQuery = TsInterfaceDeclaration |  TsTypeAliasDeclaration | JsClassDeclaration | TsDeclareStatement


ARe there some advantages to query on these types instead of querying AnyTsTypeMember / AnyJsClassMember?

To know class, interface, type aliases identifier, I think we need to hit the node types.
for example,

interface I { // we can access this `I` through the TsInterfaceDeclaration new (): I; }

so I set those node types.

Conaclos · 2023-09-09T20:46:05Z

crates/rome_js_analyze/src/analyzers/nursery/no_misused_new.rs

+        }
+    }
+
+    fn note(&self) -> MarkupBuf {


This 'notes' seems more helpful than the previous "messages". I think we could use them as main diagnostic? We could add a note in some cases. For example for a misued new in a class, we could ask the user if they mean constructor. What do you think?

Thanks, I feel that in the existing rules, the title is simplified and explain the detailed reason in the note.
But useful information should be show fast and we need to define roles for title and note. I think that in the previous rules, title stands for what, note for why.

@ematipico Have you some opinion to share?

I have no strong opinions, but I think the current implementation is fine

Conaclos · 2023-09-09T21:12:23Z

crates/rome_js_analyze/src/analyzers/nursery/no_misused_new.rs

+            })
+            .to_owned(),
+            RuleState::InterfaceMisusedNew(_) => (markup! {
+                "`new` is misleading in interface."


We could use a more detailled message such as new makes an interface a constructor. The returned type should certainly a different type from its constructor.. Any thought?

I agree with that. I'll update it more descriptive.

Conaclos · 2023-09-09T21:15:28Z

So, the lint name can be changed more descriptive, but no-misused-new-and-constructor is a bit subtle.
no-misleading-instantiation?

instantiation makes me think about a new C().
Maybe noMisleadingConstructor or noMisleadingInstantiator?

ematipico · 2023-09-10T09:02:08Z

+1 for noMisleadingInstantiator

unvalley · 2023-09-10T13:22:54Z

yes, I also like noMisleadingInstantiator, I'll change the name to it.

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

CHANGELOG.md

chore: codegen chore: documentation wip chore: resolve conflicts refactor: filter_map to flat_map Discard changes to crates/biome_js_unicode_table/src/tables.rs Discard changes to crates/biome_js_formatter/src/generated.rs chore: update message refactor: impl name method for AnyJsClassMemberName refactor: use name method and update test chore: rename to no-misleading-instantiator feat: handle returning this case refactor: note and method names chore: codegen and changelog Discard changes to CHANGELOG.md chore: changelog chore: changelog.mdx chore: codegen Discard changes to crates/biome_js_formatter/src/generated.rs Discard changes to crates/biome_js_unicode_table/src/tables.rs Discard changes to crates/biome_json_formatter/src/generated.rs

…stantiator.rs Co-authored-by: Victorien Elvinger <[email protected]>

Co-authored-by: Victorien Elvinger <[email protected]>

…to no-misused-new

Conaclos reviewed Sep 9, 2023

View reviewed changes

unvalley force-pushed the no-misused-new branch from 15e5ac8 to ee4746d Compare September 12, 2023 16:26

github-actions bot added the A-Formatter Area: formatter label Sep 12, 2023

unvalley changed the title ~~feat(lint): noMisusedNew~~ feat(lint): noMisleadingInstantiator Sep 13, 2023

github-actions bot added the A-Parser Area: parser label Sep 13, 2023

unvalley force-pushed the no-misused-new branch from 52754b9 to 6e028df Compare September 14, 2023 16:04

github-actions bot added A-Changelog Area: changelog and removed A-Changelog Area: changelog A-Formatter Area: formatter labels Sep 14, 2023

unvalley force-pushed the no-misused-new branch 2 times, most recently from 18f85b1 to e607cf7 Compare September 15, 2023 16:50

github-actions bot added A-Formatter Area: formatter and removed A-Formatter Area: formatter labels Sep 15, 2023

unvalley force-pushed the no-misused-new branch from 1e6173a to ae65f27 Compare September 15, 2023 17:24

github-actions bot added A-Formatter Area: formatter and removed A-Formatter Area: formatter labels Sep 15, 2023

unvalley force-pushed the no-misused-new branch from 196eaac to ef40630 Compare September 15, 2023 17:41

github-actions bot added A-Formatter Area: formatter and removed A-Formatter Area: formatter labels Sep 15, 2023

unvalley marked this pull request as ready for review September 15, 2023 18:30

unvalley requested a review from Conaclos September 15, 2023 18:31

Conaclos approved these changes Sep 16, 2023

View reviewed changes

crates/biome_js_analyze/src/analyzers/nursery/no_misleading_instantiator.rs Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

unvalley and others added 9 commits September 17, 2023 03:21

chore: codegen and resolve conflicts

74e850c

chore: codegen documentation

fa17524

Discard changes to crates/biome_js_formatter/src/generated.rs

8a55495

Discard changes to crates/biome_js_unicode_table/src/tables.rs

4246aaf

Discard changes to crates/biome_json_formatter/src/generated.rs

a025836

fix: AnyJsClassMemberName.name() doctest"

f5a3d23

Update crates/biome_js_analyze/src/analyzers/nursery/no_misleading_in…

78786c2

…stantiator.rs Co-authored-by: Victorien Elvinger <[email protected]>

Update CHANGELOG.md

7cb3a1b

Co-authored-by: Victorien Elvinger <[email protected]>

unvalley force-pushed the no-misused-new branch from 1a4c75e to 7cb3a1b Compare September 16, 2023 18:23

github-actions bot added the A-Formatter Area: formatter label Sep 16, 2023

unvalley and others added 5 commits September 17, 2023 03:24

Discard changes to crates/biome_json_formatter/src/generated.rs

5a32fde

chore: restore change for json formatter generated

c1d2f70

Merge branch 'no-misused-new' of https://github.com/unvalley/biome in…

2d56969

…to no-misused-new

chore: codegen

8ff1305

chore: restore change for js formatter generated

f4c6e17

github-actions bot removed the A-Formatter Area: formatter label Sep 17, 2023

unvalley merged commit d1488cb into biomejs:main Sep 17, 2023
15 checks passed

unvalley deleted the no-misused-new branch September 17, 2023 04:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(lint): `noMisleadingInstantiator` #215

feat(lint): `noMisleadingInstantiator` #215

unvalley commented Sep 9, 2023 •

edited

Loading

Conaclos Sep 9, 2023

unvalley Sep 14, 2023

Conaclos Sep 9, 2023 •

edited

Loading

Conaclos Sep 9, 2023

Conaclos Sep 9, 2023

unvalley Sep 12, 2023 •

edited

Loading

Conaclos Sep 9, 2023

unvalley Sep 12, 2023

Conaclos Sep 12, 2023

ematipico Sep 14, 2023

Conaclos Sep 9, 2023

unvalley Sep 12, 2023

Conaclos commented Sep 9, 2023 •

edited

Loading

ematipico commented Sep 10, 2023

unvalley commented Sep 10, 2023

feat(lint): noMisleadingInstantiator #215

feat(lint): noMisleadingInstantiator #215

Conversation

unvalley commented Sep 9, 2023 • edited Loading

Summary

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Conaclos Sep 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unvalley Sep 12, 2023 • edited Loading

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

Choose a reason for hiding this comment

Conaclos commented Sep 9, 2023 • edited Loading

ematipico commented Sep 10, 2023

unvalley commented Sep 10, 2023

feat(lint): `noMisleadingInstantiator` #215

feat(lint): `noMisleadingInstantiator` #215

unvalley commented Sep 9, 2023 •

edited

Loading

Conaclos Sep 9, 2023 •

edited

Loading

unvalley Sep 12, 2023 •

edited

Loading

Conaclos commented Sep 9, 2023 •

edited

Loading