feat(rome_js_analyze): useCamelCase #2926

xunilrj · 2022-07-25T12:08:03Z

Summary

Implements #2903.

Test Plan

> cargo rome-cli check ./crates/rome_js_analyze/tests/specs/js/useCamelCase.js

Camel Case

This PR implements only the happy path for camel case. It does not support nice camel case for abbreviations and acronyms.

Renaming and conflicts

Given that we rename different bindings to possibly the same name, they can conflict. The renaming is correctly dealing with this. So if we have:

let snake_case
snake_case = 1;
let _snake_case
console.log(_snake_case);

After renaming everything in one go, we have:

let snakeCase
snakeCase = 1;
let snakeCase2
console.log(snakeCase2);

But this is not reflected in the diagnostic phase. Because there is no conflict there. This is something we may need to address in another PR.

warning[js/useCamelCase]: Prefer symbols names in camel case.
  ┌─ ./crates/rome_js_analyze/tests/specs/js/useCamelCase.js:1:5
  │
1 │ let snake_case;
  │     ----------
2 │ snake_case = 1;
  │ ---------- Used here.

Safe fix: Rename this symbol to camel case
    | @@ -1,5 +1,5 @@
0   | - let snake_case;
1   | - snake_case = 1;
  0 | + let snakeCase;
  1 | + snakeCase = 1;
2 2 |   let _snake_case;
3 3 |   console.log(_snake_case);
4 4 |   


warning[js/useCamelCase]: Prefer symbols names in camel case.
  ┌─ ./crates/rome_js_analyze/tests/specs/js/useCamelCase.js:3:5
  │
3 │ let _snake_case;
  │     -----------
4 │ console.log(_snake_case);
  │             ----------- Used here.

Safe fix: Rename this symbol to camel case
    | @@ -1,7 +1,7 @@
0 0 |   let snake_case;
1 1 |   snake_case = 1;
2   | - let _snake_case;
3   | - console.log(_snake_case);
  2 | + let snakeCase;
  3 | + console.log(snakeCase);
4 4 |   
5 5 |   function f(snake_case, PascalCase) {}
6 6 |

Performance

I compared this PR to https://github.com/SkylerLipthay/case. But focusing on being faster when everything is ok. Now we are only slower when we actually need to transform the string. We run fewer instructions, but unfortunately, we access the memory more times.

rome_all_lowercase
  Instructions:                 194 
  L1 Accesses:                  226
  L2 Accesses:                    4 
  RAM Accesses:                   7
  Estimated Cycles:             491

case_all_lowercase
  Instructions:                 613
  L1 Accesses:                  768
  L2 Accesses:                    5 
  RAM Accesses:                  12
  Estimated Cycles:            1213 

rome_already_camel_case
  Instructions:                 194 
  L1 Accesses:                  226 
  L2 Accesses:                    3 
  RAM Accesses:                   8 
  Estimated Cycles:             521 

case_already_camel_case
  Instructions:                 613 
  L1 Accesses:                  769 
  L2 Accesses:                    4 
  RAM Accesses:                  12 
  Estimated Cycles:            1209 

rome_pascal_case
  Instructions:                 574 
  L1 Accesses:                  767 
  L2 Accesses:                    7 
  RAM Accesses:                  25 
  Estimated Cycles:            1677 

case_pascal_case
  Instructions:                 613 
  L1 Accesses:                  771 
  L2 Accesses:                    3 
  RAM Accesses:                  11 
  Estimated Cycles:            1171

github-actions · 2022-07-25T12:12:49Z

Playground for commit 7959ea3

github-actions · 2022-07-25T12:14:43Z

Parser conformance results on ubuntu-latest

js/262

Test result	`main` count	This PR count	Difference
Total	45878	45878	0
Passed	44938	44938	0
Failed	940	940	0
Panics	0	0	0
Coverage	97.95%	97.95%	0.00%

jsx/babel

Test result	`main` count	This PR count	Difference
Total	39	39	0
Passed	36	36	0
Failed	3	3	0
Panics	0	0	0
Coverage	92.31%	92.31%	0.00%

symbols/microsoft

Test result	`main` count	This PR count	Difference
Total	5946	5946	0
Passed	395	395	0
Failed	5551	5551	0
Panics	0	0	0
Coverage	6.64%	6.64%	0.00%

ts/babel

Test result	`main` count	This PR count	Difference
Total	588	588	0
Passed	519	519	0
Failed	69	69	0
Panics	0	0	0
Coverage	88.27%	88.27%	0.00%

ts/microsoft

Test result	`main` count	This PR count	Difference
Total	16257	16257	0
Passed	12393	12393	0
Failed	3864	3864	0
Panics	0	0	0
Coverage	76.23%	76.23%	0.00%

cloudflare-workers-and-pages · 2022-07-25T12:17:51Z

Deploying with Cloudflare Pages

Latest commit:	`51d682d`
Status:	✅ Deploy successful!
Preview URL:	https://227535e1.tools-8rn.pages.dev
Branch Preview URL:	https://feature-use-camel-case.tools-8rn.pages.dev

View logs

xunilrj · 2022-07-25T12:21:44Z

Relevant for: #2925

ematipico · 2022-07-25T12:40:16Z

crates/rome_js_analyze/Cargo.toml

@@ -19,8 +19,14 @@ rome_console = { path = "../rome_console" }
 rome_diagnostics = { path = "../rome_diagnostics" }
 roaring = "0.9.0"
 rustc-hash = "1.1.0"
+iai = "0.1.1"


Does it have to be a dependency? Can it be moved in dev-dependencies?

Yes. Wrong place.

ematipico · 2022-07-25T12:41:02Z

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

+    const CATEGORY: RuleCategory = RuleCategory::Lint;
+
+    type Query = Semantic<JsIdentifierBinding>;
+    type State = (String, Vec<Reference>);


Could you document the state please?

Changed for a struct.

ematipico · 2022-07-25T12:44:17Z

crates/rome_js_analyze/src/utils/rename.rs

+    /// let candidates = once(Cow::from(new_name)).chain(candidates);
+    /// batch.try_rename_node_declaration_until_success(model, node, candidates);
+    /// ```
+    fn try_rename_node_declaration_until_success<S, I>(


I think using try_* should hint to return Result, at least that's what the convention seems to be.

The name of the function seems different from what it actually does. If inside candidates we have more than one eligible for renaming, only the first gets renamed and the rest don't. I think we should review the logic or the name of the function.

I renamed this function.

crates/rome_js_analyze/src/utils/rename.rs

crates/rome_js_analyze/tests/specs/js/useCamelCase.js.snap

crates/rome_js_analyze/src/utils.rs

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

ematipico · 2022-07-28T12:42:24Z

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

+    pub(crate) UseCamelCase {
+        version: "0.8.0",
+        name: "useCamelCase",
+        recommended: true


What do you think if we don't recommend the rule? Mostly because it's not finished as you suggested. But I don't mind having it recommended too. No strong opinions, as long as the current implementation is reliable.

IWANABETHATGUY · 2022-07-30T01:50:59Z

!bench_analyzer

github-actions · 2022-07-30T02:02:40Z

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.00   1980.8±4.15µs     5.9 MB/sec    1.37      2.7±0.02ms     4.3 MB/sec
analyzer/index.js         1.00      4.7±0.00ms     6.8 MB/sec    1.31      6.2±0.10ms     5.2 MB/sec
analyzer/lint.ts          1.00      2.7±0.01ms    15.6 MB/sec    1.39      3.7±0.01ms    11.3 MB/sec
analyzer/parser.ts        1.00      6.4±0.01ms     7.6 MB/sec    1.35      8.6±0.03ms     5.6 MB/sec
analyzer/router.ts        1.00      4.5±0.01ms    13.7 MB/sec    1.34      6.0±0.01ms    10.2 MB/sec
analyzer/statement.ts     1.00      6.0±0.06ms     5.9 MB/sec    1.30      7.8±0.01ms     4.6 MB/sec
analyzer/typescript.ts    1.00      8.8±0.02ms     6.2 MB/sec    1.30     11.4±0.02ms     4.8 MB/sec

* lint useCamelCase working

xunilrj requested review from leops and MichaReiser as code owners July 25, 2022 12:08

xunilrj requested a review from a team July 25, 2022 12:08

xunilrj temporarily deployed to aws July 25, 2022 12:08 Inactive

xunilrj force-pushed the feature/use-camel-case branch from 4a47d05 to 5c67f07 Compare July 25, 2022 12:17

xunilrj temporarily deployed to aws July 25, 2022 12:17 Inactive

ematipico reviewed Jul 25, 2022

View reviewed changes

IWANABETHATGUY reviewed Jul 25, 2022

View reviewed changes

crates/rome_js_analyze/src/utils.rs Show resolved Hide resolved

IWANABETHATGUY reviewed Jul 25, 2022

View reviewed changes

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

xunilrj mentioned this pull request Jul 28, 2022

fix(rome_js_semantic): bug when hoisting function scope #2940

Merged

xunilrj added 7 commits July 28, 2022 13:17

lint useCamelCase working

1006a1d

lint documentation

7ba341a

fmt and clippy issues

716f73b

better retries when renaming

ec256bf

updating lintdoc

72fee15

rebasing

1a67bab

fmt and clippy issues

7da9e99

xunilrj force-pushed the feature/use-camel-case branch from 5c67f07 to 7da9e99 Compare July 28, 2022 12:36

xunilrj temporarily deployed to aws July 28, 2022 12:36 Inactive

typo

ff0f611

xunilrj temporarily deployed to aws July 28, 2022 12:38 Inactive

dealing with symbols that start with underscore

9816112

ematipico reviewed Jul 28, 2022

View reviewed changes

xunilrj temporarily deployed to aws July 28, 2022 12:42 Inactive

fmt and clippy issues

03438f5

xunilrj temporarily deployed to aws July 28, 2022 12:43 Inactive

changing to recommended false

feb7e14

xunilrj temporarily deployed to aws July 28, 2022 12:45 Inactive

lintdoc

51d682d

xunilrj temporarily deployed to aws July 28, 2022 12:53 Inactive

leops approved these changes Jul 29, 2022

View reviewed changes

xunilrj merged commit 2f17ecf into main Jul 29, 2022

xunilrj deleted the feature/use-camel-case branch July 29, 2022 16:57

IWANABETHATGUY mentioned this pull request Jul 30, 2022

useCamelCase #2903

Closed

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

feat(rome_js_analyze): useCamelCase (rome#2926)

94e591d

* lint useCamelCase working

feat(rome_js_analyze): useCamelCase #2926

feat(rome_js_analyze): useCamelCase #2926

Uh oh!

Conversation

xunilrj commented Jul 25, 2022 • edited by ematipico Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Camel Case

Renaming and conflicts

Performance

Uh oh!

github-actions bot commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 25, 2022

Parser conformance results on ubuntu-latest

js/262

jsx/babel

symbols/microsoft

ts/babel

ts/microsoft

Uh oh!

cloudflare-workers-and-pages bot commented Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying with Cloudflare Pages

Uh oh!

xunilrj commented Jul 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IWANABETHATGUY commented Jul 30, 2022

Uh oh!

github-actions bot commented Jul 30, 2022

Analyzer Benchmark Results

Uh oh!

Uh oh!

xunilrj commented Jul 25, 2022 •

edited by ematipico

Loading

github-actions bot commented Jul 25, 2022 •

edited

Loading

cloudflare-workers-and-pages bot commented Jul 25, 2022 •

edited

Loading