feat(rome_js_analyze): no shadow restricted lint rule #2975

xunilrj · 2022-07-31T09:10:19Z

Summary

Closes #2897.

I created this benchmark https://github.com/rome/tools/pull/2975/files#diff-02554aa7d6b22992e92b6b8fe6366b78706e2a98f0f421cfd4641f79fa7fec8c expecting that a fancy algorithm would be faster, but end up testifying that cache rules and a simple slice::contains was the faster for the small number os strings we test.

The binary that calls this benchmark is only because iai do not support setup yet (bheisler/iai#24) Basically it runs the test twice. First only the setup, and second the whole test and take the difference.

Codegen

I changed the codegen for analyzers prefixing it with "self" to avoid collisions with some crates. "regex" is an example.
I also implemented a "codegen all" to make it simpler to update everything. But running changed things in the formatted and the parser.

Parser errors

Some examples are commented because they generate parser errors.

//function arguments() {}
//function eval() {}

I think our parser should accept these, and let the linter complain. I can change this in another PR.

Test Plan

> cargo rome_cli -- check crates/rome_js_analyze/tests/specs/js/noShadowRestrictedNames.js

cloudflare-workers-and-pages · 2022-07-31T09:15:54Z

Deploying with Cloudflare Pages

Latest commit:	`70c68b3`
Status:	✅ Deploy successful!
Preview URL:	https://629760e9.tools-8rn.pages.dev
Branch Preview URL:	https://feature-no-shadow-restricted.tools-8rn.pages.dev

View logs

github-actions · 2022-07-31T09:17:33Z

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%

github-actions · 2022-07-31T09:21:33Z

Playground for commit f7818c3

IWANABETHATGUY · 2022-07-31T09:27:55Z

Would you mind adding memchr to your benchmark suite? https://github.com/BurntSushi/memchr, memchr using the simd to accelerate your search routine.

IWANABETHATGUY · 2022-07-31T09:49:59Z

And also I would recommend benching our suite using criterion again, because memory allocation is also a critical factor that will impact performance, It seems iai could not inspect the memory allocation.

xunilrj · 2022-07-31T10:29:37Z

Results using criterion

contains_hashset        time:   [102.34 ns 102.40 ns 102.47 ns]   
contains_btreeset       time:   [127.64 ns 130.65 ns 134.23 ns] 
contains_bloom          time:   [336.88 ns 337.71 ns 338.64 ns]  
contains_trie           time:   [48.507 ns 48.907 ns 49.405 ns]  
contains_slice          time:   [32.514 ns 32.878 ns 33.340 ns]   
contains_fst            time:   [378.70 ns 384.44 ns 392.06 ns]  
contains_binary_search  time:   [98.531 ns 99.631 ns 100.95 ns]   
contains_memchr         time:   [395.70 ns 397.14 ns 399.21 ns]

and running

> cargo run -p rome_js_analyze --bin contains_iai

contains_trie
        diff inst: 608 is 9292 - 8684
        diff l1: 723 is 12809 - 12086
        diff l2: 6 is 110 - 104
        diff ram: 7 is 148 - 141
        diff cycles: 998 is 18539 - 17541
contains_hashset
        diff inst: 1695 is 11465 - 9770
        diff l1: 2121 is 15375 - 13254
        diff l2: 0 is 115 - 115
        diff ram: 13 is 168 - 155
        diff cycles: 2576 is 21830 - 19254
contains_slice
        diff inst: 344 is 6110 - 5766
        diff l1: 397 is 8320 - 7923
        diff l2: 3 is 102 - 99
        diff ram: 16 is 102 - 86
        diff cycles: 972 is 12400 - 11428
contains_binary_search
        diff inst: 1089 is 7326 - 6237
        diff l1: 1309 is 9861 - 8552
        diff l2: 5 is 104 - 99
        diff ram: 30 is 127 - 97
        diff cycles: 2384 is 14826 - 12442
contains_bloom
        diff inst: 1903 is 9657 - 7754
        diff l1: 2278 is 12510 - 10232
        diff l2: 4 is 110 - 106
        diff ram: 14 is 173 - 159
        diff cycles: 2788 is 19115 - 16327
contains_btreeset
        diff inst: 1434 is 9606 - 8172
        diff l1: 1717 is 13019 - 11302
        diff l2: 1 is 105 - 104
        diff ram: 33 is 161 - 128
        diff cycles: 2877 is 19179 - 16302
contains_fst
        diff inst: 4154 is 949559 - 945405
        diff l1: 5760 is 1319441 - 1313681
        diff l2: 23 is 15577 - 15554
        diff ram: 30 is 15410 - 15380
        diff cycles: 6925 is 1936676 - 1929751
contains_memchr
        diff inst: 5975 is 12212 - 6237
        diff l1: 7088 is 15639 - 8551
        diff l2: 10 is 110 - 100
        diff ram: 13 is 110 - 97
        diff cycles: 7593 is 20039 - 12446

IWANABETHATGUY · 2022-07-31T11:00:49Z

Results using criterion

contains_hashset        time:   [102.34 ns 102.40 ns 102.47 ns]   
contains_btreeset       time:   [127.64 ns 130.65 ns 134.23 ns] 
contains_bloom          time:   [336.88 ns 337.71 ns 338.64 ns]  
contains_trie           time:   [48.507 ns 48.907 ns 49.405 ns]  
contains_slice          time:   [32.514 ns 32.878 ns 33.340 ns]   
contains_fst            time:   [378.70 ns 384.44 ns 392.06 ns]  
contains_binary_search  time:   [98.531 ns 99.631 ns 100.95 ns]   
contains_memchr         time:   [395.70 ns 397.14 ns 399.21 ns]

and running

> cargo run -p rome_js_analyze --bin contains_iai

contains_trie
        diff inst: 608 is 9292 - 8684
        diff l1: 723 is 12809 - 12086
        diff l2: 6 is 110 - 104
        diff ram: 7 is 148 - 141
        diff cycles: 998 is 18539 - 17541
contains_hashset
        diff inst: 1695 is 11465 - 9770
        diff l1: 2121 is 15375 - 13254
        diff l2: 0 is 115 - 115
        diff ram: 13 is 168 - 155
        diff cycles: 2576 is 21830 - 19254
contains_slice
        diff inst: 344 is 6110 - 5766
        diff l1: 397 is 8320 - 7923
        diff l2: 3 is 102 - 99
        diff ram: 16 is 102 - 86
        diff cycles: 972 is 12400 - 11428
contains_binary_search
        diff inst: 1089 is 7326 - 6237
        diff l1: 1309 is 9861 - 8552
        diff l2: 5 is 104 - 99
        diff ram: 30 is 127 - 97
        diff cycles: 2384 is 14826 - 12442
contains_bloom
        diff inst: 1903 is 9657 - 7754
        diff l1: 2278 is 12510 - 10232
        diff l2: 4 is 110 - 106
        diff ram: 14 is 173 - 159
        diff cycles: 2788 is 19115 - 16327
contains_btreeset
        diff inst: 1434 is 9606 - 8172
        diff l1: 1717 is 13019 - 11302
        diff l2: 1 is 105 - 104
        diff ram: 33 is 161 - 128
        diff cycles: 2877 is 19179 - 16302
contains_fst
        diff inst: 4154 is 949559 - 945405
        diff l1: 5760 is 1319441 - 1313681
        diff l2: 23 is 15577 - 15554
        diff ram: 30 is 15410 - 15380
        diff cycles: 6925 is 1936676 - 1929751
contains_memchr
        diff inst: 5975 is 12212 - 6237
        diff l1: 7088 is 15639 - 8551
        diff l2: 10 is 110 - 100
        diff ram: 13 is 110 - 97
        diff cycles: 7593 is 20039 - 12446

Nice

xunilrj · 2022-07-31T13:50:29Z

Nice

but quite boring! I was expecting to use fst with transducers and SIMD running in multiple GPU with Raytracing. Everything mega fast. In the end: slice::contains. 😄

IWANABETHATGUY · 2022-07-31T13:54:11Z

Nice

but quite boring! I was expecting to use fst with transducers and SIMD running in multiple GPU with Raytracing. Everything mega fast. In the end: slice::contains. smile

lol

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

crates/rome_js_analyze/Cargo.toml

ematipico

I just realized now that this rule was removed from rome classic (the documentation was not deleted). I can't remember why it was removed though. Should we keep the rule? @sebmck , do you remember why it was removed?

ematipico · 2022-08-02T06:33:29Z

crates/rome_js_analyze/Cargo.toml

@@ -19,15 +19,35 @@ rome_console = { path = "../rome_console" }
 rome_diagnostics = { path = "../rome_diagnostics" }
 roaring = "0.9.0"
 rustc-hash = "1.1.0"
-iai = "0.1.1"
+regex = { version = "1.6.0" }


This crate should be moved in dev-dependencies

ematipico · 2022-08-02T06:38:02Z

crates/rome_js_analyze/src/analyzers/js/no_shadow_restricted_names.rs

+    }
+}
+
+const RESTRICTEDNAMES: [&str; 9] = [


RESTRICED_NAMES

ematipico · 2022-08-02T06:39:17Z

crates/rome_js_analyze/src/analyzers/js/no_shadow_restricted_names.rs

+    "NaN",
+    "undefined",
+    "Infinity",
+    "arguments",
+    "eval",
+    "Set",
+    "Object",
+    "Array",
+    "JSON",


I believe the list of restricted names is not enough. Here's what we had in rome classic: https://github.com/rome/tools/blob/archived-js/internal/compiler/scope/globals.ts

And which version do we want? Does the configuration file have the version?

We don't have it yet, I suppose it will become more important once we have transformations.

So for the moment we should use the latest version, because we target/support up to ES2022.

MichaReiser

Woah, awesome work on the extensive benchmark !

MichaReiser · 2022-08-02T06:48:48Z

crates/rome_js_analyze/Cargo.toml

@@ -19,15 +19,35 @@ rome_console = { path = "../rome_console" }
 rome_diagnostics = { path = "../rome_diagnostics" }
 roaring = "0.9.0"
 rustc-hash = "1.1.0"
-iai = "0.1.1"
+regex = { version = "1.6.0" }


Is it necessary to add regex as a prod dependency. Isn't it only used as part of the benchmarks?

I moved everything to another crate.

MichaReiser · 2022-08-02T06:50:18Z

crates/rome_js_analyze/Cargo.toml

+fastbloom-rs = "0.3.0"
+qp-trie = "0.8.0"
+fst = "0.4.7"
+criterion = "0.3.5"
+memchr = "2.5.0"
+
+[[bench]]
+name = "to_camel_case"
+harness = false

 [[bench]]
-name = "iai"
+name = "contains_iai"
 harness = false
+
+[[bench]]
+name = "contains_criterion"
+harness = false
+
+[[bin]]
+name = "contains_iai"
+path = "bins/contains_iai.rs"
+test = false
+bench = false


This is adding a lot of dependencies only for the sake fo the benchmark. How about you split out the benchmark out into its own PR and close it (without merging). This would keep the benchmark around for historic sake (you could e.g. add a link in a comment) without pulling in a large number of new dependencies.

I can move this to xtask/bench, given that this is a generic benchmark. I think will be nice to keep this around, as we will have multiple cases of these "micro benchmarks"

I moved everything to another crate.

xunilrj · 2022-08-03T14:35:56Z

!bench_analyzer

github-actions · 2022-08-03T14:44:29Z

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.00      2.1±0.01ms     5.5 MB/sec    1.01      2.1±0.00ms     5.5 MB/sec
analyzer/index.js         1.00      5.1±0.01ms     6.4 MB/sec    1.01      5.1±0.01ms     6.3 MB/sec
analyzer/lint.ts          1.00      2.9±0.01ms    14.5 MB/sec    1.01      2.9±0.01ms    14.3 MB/sec
analyzer/parser.ts        1.00      6.8±0.02ms     7.1 MB/sec    1.01      6.9±0.02ms     7.0 MB/sec
analyzer/router.ts        1.00      4.8±0.01ms    12.8 MB/sec    1.01      4.9±0.03ms    12.6 MB/sec
analyzer/statement.ts     1.00      6.5±0.01ms     5.4 MB/sec    1.02      6.6±0.04ms     5.3 MB/sec
analyzer/typescript.ts    1.00      9.8±0.01ms     5.6 MB/sec    1.01      9.8±0.01ms     5.5 MB/sec

* no shadow restricted lint rule

xunilrj requested review from leops, ematipico and MichaReiser as code owners July 31, 2022 09:10

xunilrj requested a review from a team July 31, 2022 09:10

xunilrj temporarily deployed to aws July 31, 2022 09:10 Inactive

xunilrj force-pushed the feature/no-shadow-restricted branch from c5b9fbf to 9d37d5b Compare July 31, 2022 09:16

xunilrj temporarily deployed to aws July 31, 2022 09:16 Inactive

xunilrj force-pushed the feature/no-shadow-restricted branch from 9d37d5b to 196e4fd Compare July 31, 2022 09:17

xunilrj temporarily deployed to aws July 31, 2022 09:17 Inactive

xunilrj temporarily deployed to aws July 31, 2022 10:32 Inactive

xunilrj temporarily deployed to aws July 31, 2022 10:45 Inactive

xunilrj temporarily deployed to aws July 31, 2022 13:38 Inactive

xunilrj changed the title ~~no shadow restricted lint rule~~ feat(rome_js_analyze): no shadow restricted lint rule Jul 31, 2022

leops reviewed Aug 1, 2022

View reviewed changes

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

crates/rome_js_analyze/Cargo.toml Outdated Show resolved Hide resolved

xunilrj temporarily deployed to aws August 1, 2022 12:35 Inactive

xunilrj temporarily deployed to aws August 1, 2022 19:45 Inactive

xunilrj temporarily deployed to aws August 2, 2022 04:46 Inactive

ematipico suggested changes Aug 2, 2022

View reviewed changes

MichaReiser reviewed Aug 2, 2022

View reviewed changes

xunilrj added 2 commits August 3, 2022 14:55

no shadow restricted lint rule

c6c500b

generating doc and bench using criterion

7116fdc

xunilrj added 8 commits August 3, 2022 14:55

memchr benchmark

55010a6

ftm and clippy issues

0d58068

memchr to dev-dependencies

3992d77

no shadow restricted do not need semantic model

e5fd5ee

no shadow restricted do not need semantic model

5b880ec

fmt and clippy issues

cd0d3f3

moving all benchmarks to another crate

12dc629

adjustments

3969553

xunilrj force-pushed the feature/no-shadow-restricted branch from d5d0451 to 3969553 Compare August 3, 2022 14:06

xunilrj temporarily deployed to aws August 3, 2022 14:06 Inactive

leops approved these changes Aug 3, 2022

View reviewed changes

ematipico approved these changes Aug 3, 2022

View reviewed changes

lintdoc

70c68b3

xunilrj temporarily deployed to aws August 3, 2022 21:30 Inactive

xunilrj merged commit 6af5d2b into main Aug 3, 2022

xunilrj deleted the feature/no-shadow-restricted branch August 3, 2022 21:40

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

feat(rome_js_analyze): no shadow restricted lint rule (rome#2975)

8b3baa0

* no shadow restricted lint rule

xunilrj mentioned this pull request Oct 6, 2022

feat(rome_js_semantic): simple globals resolution #3354

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rome_js_analyze): no shadow restricted lint rule #2975

feat(rome_js_analyze): no shadow restricted lint rule #2975

xunilrj commented Jul 31, 2022 •

edited

Loading

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

edited

Loading

github-actions bot commented Jul 31, 2022

github-actions bot commented Jul 31, 2022 •

edited

Loading

IWANABETHATGUY commented Jul 31, 2022

IWANABETHATGUY commented Jul 31, 2022

xunilrj commented Jul 31, 2022 •

edited

Loading

IWANABETHATGUY commented Jul 31, 2022

xunilrj commented Jul 31, 2022

IWANABETHATGUY commented Jul 31, 2022

ematipico left a comment

ematipico Aug 2, 2022

ematipico Aug 2, 2022

ematipico Aug 2, 2022

xunilrj Aug 3, 2022

ematipico Aug 3, 2022

MichaReiser left a comment

MichaReiser Aug 2, 2022

xunilrj Aug 3, 2022

MichaReiser Aug 2, 2022

IWANABETHATGUY Aug 2, 2022

xunilrj Aug 3, 2022

xunilrj Aug 3, 2022

xunilrj commented Aug 3, 2022

github-actions bot commented Aug 3, 2022

feat(rome_js_analyze): no shadow restricted lint rule #2975

feat(rome_js_analyze): no shadow restricted lint rule #2975

Conversation

xunilrj commented Jul 31, 2022 • edited Loading

Summary

Codegen

Parser errors

Test Plan

cloudflare-workers-and-pages bot commented Jul 31, 2022 • edited Loading

Deploying with Cloudflare Pages

github-actions bot commented Jul 31, 2022

Parser conformance results on ubuntu-latest

js/262

jsx/babel

symbols/microsoft

ts/babel

ts/microsoft

github-actions bot commented Jul 31, 2022 • edited Loading

IWANABETHATGUY commented Jul 31, 2022

IWANABETHATGUY commented Jul 31, 2022

xunilrj commented Jul 31, 2022 • edited Loading

IWANABETHATGUY commented Jul 31, 2022

xunilrj commented Jul 31, 2022

IWANABETHATGUY commented Jul 31, 2022

ematipico 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

MichaReiser 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

Choose a reason for hiding this comment

xunilrj commented Aug 3, 2022

github-actions bot commented Aug 3, 2022

Analyzer Benchmark Results

xunilrj commented Jul 31, 2022 •

edited

Loading

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

edited

Loading

github-actions bot commented Jul 31, 2022 •

edited

Loading

xunilrj commented Jul 31, 2022 •

edited

Loading