feat(rome_js_analyze): `noDuplicateClassMembers` #4137

nissy-dev · 2023-01-04T13:59:42Z

Summary

Test Plan

I made the same test cases as in ESLint. In this PR, this rule doesn't support computed and numerical properties.

I refer to the following comments.

tools/crates/rome_js_analyze/tests/specs/nursery/noDuplicateObjectKeys.js

Lines 27 to 29 in b6e7a39

    
           // This particular simple computed property case with just a string literal would be easy to catch, 
        
           // but we don't want to open Pandora's static analysis box so we have to draw a line somewhere 
        
           ({ a: 1, ["a"]: 1 });

Documentation

The PR requires documentation
I will create a new PR to update the documentation

netlify · 2023-01-04T14:07:09Z

✅ Deploy Preview for docs-rometools ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`aa6e194`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-rometools/deploys/63dd20bd5f196c0008c87ce0
😎 Deploy Preview	https://deploy-preview-4137--docs-rometools.netlify.app/playground
📱 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 settings.

Conaclos · 2023-01-04T18:25:57Z

It could be nice to have an action for removing the overwritten member (Similarly to noDuplicateObjectKeys). You are free to take inspiration from my closed PR.

nissy-dev · 2023-01-05T00:48:32Z

It could be nice to have an action for removing the overwritten member (Similarly to noDuplicateObjectKeys). You are free to take inspiration from my #4130.

Thank for your suggestion. I think the action for removing the overwritten member is not needed. This is because it is not enough to delete the last overwritten member about duplicated members . In some cases, users want to delete the member defined firstly. This is not up to us.

ESLint and other rules(like noDuplicateJsxProps) also doesn't have the action.

ematipico

You might want to look at https://github.com/rome/tools/pull/4130/files , maybe some logic can be joined

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

ematipico · 2023-01-05T12:51:58Z

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

+    JsMethodModifierList(JsMethodModifierList),
+}
+
+fn get_static_type(modifier_list: AnyModifierList) -> StaticType {


You can avoid the enum AnyModifierList by accepting a JsSyntaxList instead. From a typed list - for example, JsPropertyModifierList - you can retrieve a generic list using syntax_list.

I refactored get_static_type to is_static_member which accepts JsSyntaxList.

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

ematipico · 2023-01-05T13:08:07Z

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

+                    let static_type = member_def.static_type();
+                    let member_type = member_def.member_type();
+                    defined_members
+                        .entry((member_name, static_type, access_type))


What's the point of saving static_type, access_type and member_name if they are all computed from member_def? I think it's better to save member_def at this point and implement Eq, PartialEq

I try to implement PartialEq and Eq for AnyClassMemberDefinition, but I couldn't because declare_node_union macro have already implemented PartialEq and Eq.

tools/crates/rome_rowan/src/macros.rs

Line 69 in 44a0933

#[derive(Clone, PartialEq, Eq, Hash)]

How do I resolve this error?

error[E0119]: conflicting implementations of trait `std::cmp::PartialEq` for type `analyzers::nursery::no_duplicate_class_members::AnyClassMemberDefinition` --> crates/rome_js_analyze/src/analyzers/nursery/no_duplicate_class_members.rs:144:1 | 144 | / declare_node_union! { 145 | | pub(crate) AnyClassMemberDefinition = JsGetterClassMember | JsMethodClassMember | JsPropertyClassMember | JsSetterClassMember 146 | | } | |_^ conflicting implementation for `analyzers::nursery::no_duplicate_class_members::AnyClassMemberDefinition` ... 174 | impl PartialEq for AnyClassMemberDefinition { | ------------------------------------------- first implementation here | = note: this error originates in the derive macro `PartialEq` which comes from the expansion of the macro `declare_node_union` (in Nightly builds, run with -Z macro-backtrace for more info) For more information about this error, try `rustc --explain E0119`. error: could not compile `rome_js_analyze` due to previous error

Oh sorry, then there's no need to manually implement it :)

What's the point of saving static_type, access_type and member_name if they are all computed from member_def?

These values are necessary for an unique key about judging whether the class member has already defined or not. I seem AnyClassMemberDefinition is not able to used as a key. If you think AnyClassMemberDefinition could be used as a key of HashMap, can you show more details?

…s_members.rs Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

nissy-dev · 2023-01-06T15:47:36Z

Thank you for your reviews! I refactored this PR based on your comments. I would appreciate if you could review again.

nissy-dev · 2023-01-07T11:59:43Z

~~I updated the PR for #4143 and this PR is ready for review.~~

I see #4143 (comment) and updated again. This PR is ready for review.

github-actions · 2023-01-24T12:03:40Z

This PR is stale because it has been open 14 days with no activity.

ematipico

Once rebased, we can merge it!

nissy-dev · 2023-02-03T15:04:36Z

I resolved conflicts. This PR is ready to merge 🚀

nissy-dev added 3 commits January 4, 2023 22:52

feat: init lint rule

71f2a58

feat: update docs

2cc6076

feat: impl no-dupe-class-members rule

70c1865

nissy-dev requested review from leops, ematipico, xunilrj and a team as code owners January 4, 2023 13:59

feat: add codegen

c0a5ee5

nissy-dev added 2 commits January 4, 2023 23:19

fix: codegen

87949bd

fix: only js syntax support

81feb1b

fix: small refactor

ed8e2e5

ematipico reviewed Jan 5, 2023

View reviewed changes

nissy-dev and others added 5 commits January 5, 2023 22:39

Update crates/rome_js_analyze/src/analyzers/nursery/no_duplicate_clas…

76b69cb

…s_members.rs Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

fix: ClassMemberDefinition to AnyClassMemberDefinition

92e9a93

feat: more test cases

c6d9d10

fix: try_from to From trait

be2fc37

feat: add more refactor

1bcf95a

nissy-dev force-pushed the impl-no-dupe-class-members branch from a365b19 to 1bcf95a Compare January 6, 2023 15:30

nissy-dev added 4 commits January 7, 2023 00:34

Merge branch 'main' into impl-no-dupe-class-members

25b2c5b

fix: codegen

bb83917

fix: fmt error

35d2980

fix: lint error

87cc268

nissy-dev added 2 commits January 7, 2023 09:54

fix: some cases

42b27eb

fix: add more test cases

24626be

nissy-dev mentioned this pull request Jan 7, 2023

🐛 Parser doesn't throw an error about the duplicate private class member #4143

Closed

1 task

Merge branch 'main' into impl-no-dupe-class-members

732ec84

nissy-dev force-pushed the impl-no-dupe-class-members branch from 987d96f to 732ec84 Compare January 7, 2023 12:09

nissy-dev added 3 commits January 9, 2023 23:16

feat: use hashset

a43d6c9

Merge branch 'main' into impl-no-dupe-class-members

6905d87

fix: codegen

bbabf68

github-actions bot added the S-Stale label Jan 24, 2023

ematipico approved these changes Feb 3, 2023

View reviewed changes

github-actions bot removed the S-Stale label Feb 3, 2023

Merge branch 'main' into impl-no-dupe-class-members

aa6e194

ematipico merged commit c3fb1a8 into rome:main Feb 3, 2023

	// This particular simple computed property case with just a string literal would be easy to catch,
	// but we don't want to open Pandora's static analysis box so we have to draw a line somewhere
	({ a: 1, ["a"]: 1 });

feat(rome_js_analyze): noDuplicateClassMembers #4137

feat(rome_js_analyze): noDuplicateClassMembers #4137

Uh oh!

Conversation

nissy-dev commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Documentation

Uh oh!

netlify bot commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for docs-rometools ready!

Uh oh!

Conaclos commented Jan 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nissy-dev commented Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ematipico left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ematipico Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

nissy-dev Jan 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ematipico Jan 5, 2023

Choose a reason for hiding this comment

Uh oh!

nissy-dev Jan 6, 2023

Choose a reason for hiding this comment

Uh oh!

ematipico Jan 7, 2023

Choose a reason for hiding this comment

Uh oh!

nissy-dev Jan 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nissy-dev commented Jan 6, 2023

Uh oh!

nissy-dev commented Jan 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 24, 2023

Uh oh!

ematipico left a comment

Choose a reason for hiding this comment

Uh oh!

nissy-dev commented Feb 3, 2023

Uh oh!

Uh oh!

feat(rome_js_analyze): `noDuplicateClassMembers` #4137

feat(rome_js_analyze): `noDuplicateClassMembers` #4137

nissy-dev commented Jan 4, 2023 •

edited

Loading

netlify bot commented Jan 4, 2023 •

edited

Loading

Conaclos commented Jan 4, 2023 •

edited

Loading

nissy-dev commented Jan 5, 2023 •

edited

Loading

nissy-dev Jan 7, 2023 •

edited

Loading

nissy-dev commented Jan 7, 2023 •

edited

Loading