feat(rome_js_analyze): rule `noDupeArgs` #2972

IWANABETHATGUY · 2022-07-30T09:13:24Z

Summary

Resolved 📎 (rome_js_analyze): noDupeArgs #2735

Test Plan

Should pass all test cases newly added.

IWANABETHATGUY · 2022-07-30T09:58:03Z

Wait #2973 to be merged.

crates/rome_js_analyze/src/semantic_analyzers/js/no_dupe_args.rs

IWANABETHATGUY · 2022-07-31T11:09:16Z

!bench_analyhzer

IWANABETHATGUY · 2022-07-31T11:40:06Z

!bench_analyzer

github-actions · 2022-07-31T11:48:44Z

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.00      2.3±0.00ms     4.9 MB/sec    1.02      2.4±0.00ms     4.9 MB/sec
analyzer/index.js         1.00      5.6±0.01ms     5.8 MB/sec    1.02      5.7±0.06ms     5.6 MB/sec
analyzer/lint.ts          1.00      3.2±0.00ms    12.8 MB/sec    1.02      3.3±0.00ms    12.6 MB/sec
analyzer/parser.ts        1.00      7.5±0.01ms     6.5 MB/sec    1.01      7.5±0.04ms     6.4 MB/sec
analyzer/router.ts        1.00      5.2±0.01ms    11.7 MB/sec    1.01      5.3±0.01ms    11.6 MB/sec
analyzer/statement.ts     1.00      7.1±0.03ms     5.0 MB/sec    1.01      7.2±0.03ms     4.9 MB/sec
analyzer/typescript.ts    1.00     10.5±0.08ms     5.2 MB/sec    1.01     10.6±0.02ms     5.1 MB/sec

crates/rome_js_analyze/src/semantic_analyzers/js/no_dupe_args.rs

IWANABETHATGUY · 2022-08-11T07:44:35Z

!bench_analyzer

github-actions · 2022-08-11T07:56:34Z

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.00      2.5±0.09ms     4.7 MB/sec    1.06      2.6±0.02ms     4.5 MB/sec
analyzer/index.js         1.00      6.7±0.13ms     4.8 MB/sec    1.05      7.1±0.31ms     4.6 MB/sec
analyzer/lint.ts          1.02      3.5±0.06ms    11.8 MB/sec    1.00      3.5±0.10ms    12.0 MB/sec
analyzer/parser.ts        1.00      8.1±0.13ms     6.0 MB/sec    1.04      8.4±0.10ms     5.8 MB/sec
analyzer/router.ts        1.05      5.8±0.10ms    10.6 MB/sec    1.00      5.5±0.18ms    11.1 MB/sec
analyzer/statement.ts     1.03      8.0±0.11ms     4.4 MB/sec    1.00      7.8±0.19ms     4.6 MB/sec
analyzer/typescript.ts    1.02     11.8±0.43ms     4.6 MB/sec    1.00     11.6±0.32ms     4.7 MB/sec

IWANABETHATGUY · 2022-08-11T09:32:08Z

@ematipico I rewrite the algorithm as you said before, using a recursive pattern matching algorithm to find the duplicate params. Would you mind helping with reviewing?

leops · 2022-08-11T10:06:45Z

So this may be more of a discussion we could have in the corresponding issue for this rule rather than in the PR, but I'm wondering if there's a way we could make the implementation for this rule more generic, into a noNameShadowing rule that would disallow duplicate binding names in the same scope whether that's two arguments, variables, functions, object / class member ...

crates/rome_js_analyze/src/semantic_analyzers/js/no_dupe_args.rs

crates/rome_js_analyze/tests/specs/js/noDupeArgs.js

crates/rome_js_analyze/src/semantic_analyzers/js/no_dupe_args.rs

t

crates/rome_js_analyze/src/semantic_analyzers/js/no_dupe_args.rs

IWANABETHATGUY · 2022-08-19T06:57:25Z

!bench_analyzer

github-actions · 2022-08-19T07:08:35Z

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.00   1929.8±6.90µs     6.0 MB/sec    1.01   1943.3±6.87µs     6.0 MB/sec
analyzer/index.js         1.00      5.2±0.05ms     6.2 MB/sec    1.01      5.3±0.03ms     6.1 MB/sec
analyzer/lint.ts          1.00      2.8±0.04ms    14.9 MB/sec    1.00      2.8±0.03ms    14.9 MB/sec
analyzer/parser.ts        1.00      6.6±0.15ms     7.4 MB/sec    1.00      6.5±0.13ms     7.4 MB/sec
analyzer/router.ts        1.13      5.2±0.10ms    11.8 MB/sec    1.00      4.6±0.10ms    13.3 MB/sec
analyzer/statement.ts     1.00      6.2±0.12ms     5.7 MB/sec    1.16      7.2±0.35ms     4.9 MB/sec
analyzer/typescript.ts    1.01      9.2±0.12ms     5.9 MB/sec    1.00      9.1±0.08ms     6.0 MB/sec

ematipico

@xunilrj would you mind a give a second view and merge if you think it's fine?

xunilrj · 2022-08-22T08:07:03Z

@xunilrj would you mind a give a second view and merge if you think it's fine?

Done.

IWANABETHATGUY force-pushed the feat/no-dupe-args branch from 78d1f9b to 67269ba Compare July 30, 2022 09:59

IWANABETHATGUY changed the title ~~feat(rome_js_analyze): useDupeArgs~~ feat(rome_js_analyze):noDupeArgs Jul 31, 2022

IWANABETHATGUY force-pushed the feat/no-dupe-args branch from 67269ba to a3c3f7f Compare July 31, 2022 04:12

IWANABETHATGUY marked this pull request as ready for review July 31, 2022 04:25

IWANABETHATGUY requested review from leops and xunilrj as code owners July 31, 2022 04:25

IWANABETHATGUY requested a review from a team July 31, 2022 04:25

xunilrj reviewed Jul 31, 2022

View reviewed changes

crates/rome_js_analyze/src/semantic_analyzers/js/no_dupe_args.rs Outdated Show resolved Hide resolved

xunilrj approved these changes Jul 31, 2022

View reviewed changes

ematipico changed the title ~~feat(rome_js_analyze):noDupeArgs~~ feat(rome_js_analyze): rule noDupeArgs Aug 2, 2022

ematipico suggested changes Aug 2, 2022

View reviewed changes

crates/rome_js_analyze/src/semantic_analyzers/js/no_dupe_args.rs Outdated Show resolved Hide resolved

crates/rome_js_analyze/src/semantic_analyzers/js/no_dupe_args.rs Outdated Show resolved Hide resolved

ematipico reviewed Aug 3, 2022

View reviewed changes

crates/rome_js_analyze/src/semantic_analyzers/js/no_dupe_args.rs Outdated Show resolved Hide resolved

IWANABETHATGUY force-pushed the feat/no-dupe-args branch 2 times, most recently from 5894c1d to e1fe0db Compare August 6, 2022 13:32

IWANABETHATGUY requested review from MichaReiser and ematipico August 11, 2022 09:32

ematipico reviewed Aug 15, 2022

View reviewed changes

MichaReiser reviewed Aug 16, 2022

View reviewed changes

crates/rome_js_analyze/src/semantic_analyzers/js/no_dupe_args.rs Outdated Show resolved Hide resolved

crates/rome_js_analyze/src/semantic_analyzers/js/no_dupe_args.rs Outdated Show resolved Hide resolved

IWANABETHATGUY added 4 commits August 17, 2022 23:20

fix: 🐛 function expression should push new scope

4f8f6d2

fix: 🐛 remove mainn

e44bf86

fix: 🐛 pop scope when leave function expr

931385d

style: 💄 fmt

a9b4757

IWANABETHATGUY added 13 commits August 17, 2022 23:21

chore: 🤖 revert

688c83b

chore: 🤖 rever

b06b253

t

chore: 🤖 init

534c879

test: 💍 add more testcase

e9dd8b1

fix: 🐛 test case for export_default_function_declaration

a1dfbf9

chore: 🤖 update snapshot

7abd139

chore: 🤖 codegen

3e67814

chore: 🤖 revert

65227bf

chore: 🤖 change version and configuration

e5bc961

chore: 🤖 using traverse algorithm instead of semantic model

9b1fc1f

style: 💄 fmt

910bdab

test: 💍 add more testcases about JsObjectBindingPatternProperty

14c24bd

style: 💄 fmt

61c190a

IWANABETHATGUY force-pushed the feat/no-dupe-args branch from b752cd9 to 61c190a Compare August 17, 2022 15:22

IWANABETHATGUY added 6 commits August 17, 2022 23:45

chore: 🤖 fix cr issues

6c6532b

chore: 🤖 recover

65f16d1

chore: 🤖 revert

386c549

chore: 🤖 update snapshot

da02c55

test: 💍 add more test case about objectMethod and class method

c79eb8a

chore: 🤖 update snapshot

e5782a5

ematipico reviewed Aug 18, 2022

View reviewed changes

IWANABETHATGUY added 2 commits August 19, 2022 13:25

fix: 🐛 cr issues

0cc28b5

fix: 🐛 cr issue

afdc01b

ematipico approved these changes Aug 19, 2022

View reviewed changes

xunilrj merged commit 4ca30ed into rome:main Aug 22, 2022

IWANABETHATGUY deleted the feat/no-dupe-args branch August 22, 2022 08:07

IWANABETHATGUY added a commit to IWANABETHATGUY/tools that referenced this pull request Aug 22, 2022

feat(rome_js_analyze): rule noDupeArgs (rome#2972)

b270977

feat(rome_js_analyze): rule noDupeArgs #2972

feat(rome_js_analyze): rule noDupeArgs #2972

Uh oh!

Conversation

IWANABETHATGUY commented Jul 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

IWANABETHATGUY commented Jul 30, 2022

Uh oh!

Uh oh!

IWANABETHATGUY commented Jul 31, 2022

Uh oh!

IWANABETHATGUY commented Jul 31, 2022

Uh oh!

github-actions bot commented Jul 31, 2022

Analyzer Benchmark Results

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IWANABETHATGUY commented Aug 11, 2022

Uh oh!

github-actions bot commented Aug 11, 2022

Analyzer Benchmark Results

Uh oh!

IWANABETHATGUY commented Aug 11, 2022

Uh oh!

leops commented Aug 11, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

IWANABETHATGUY commented Aug 19, 2022

Uh oh!

github-actions bot commented Aug 19, 2022

Analyzer Benchmark Results

Uh oh!

ematipico left a comment

Choose a reason for hiding this comment

Uh oh!

xunilrj commented Aug 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

feat(rome_js_analyze): rule `noDupeArgs` #2972

feat(rome_js_analyze): rule `noDupeArgs` #2972

IWANABETHATGUY commented Jul 30, 2022 •

edited

Loading