feat(rome_js_analyze): rule `noConstAssign` #3409

ematipico · 2022-10-12T13:38:22Z

Summary

I thought that the entity of the work to implement this rule was bigger. I also checked the eslint implementation, and it's in line with ours.

Test Plan

Added some cases, please let me know if you have more cases that we should add.

netlify · 2022-10-12T13:38:27Z

✅ Deploy Preview for rometools ready!

Name	Link
🔨 Latest commit	`2e1821c`
🔍 Latest deploy log	https://app.netlify.com/sites/rometools/deploys/634d10a431798d0007efd156
😎 Deploy Preview	https://deploy-preview-3409--rometools.netlify.app
📱 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.

github-actions · 2022-10-12T13:44:32Z

Playground for commit 11b0d52

MichaReiser · 2022-10-12T15:49:28Z

!bench_analyzer

crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs

MichaReiser · 2022-10-12T15:53:23Z

crates/rome_js_analyze/src/semantic_analyzers/nursery/no_const_assign.rs

+        let node = ctx.query();
+        let model = ctx.model();
+
+        let declared_binding = model.declaration(node)?;


I'm a bit worried that this will have a significant performance cost because it requires querying the model for every single identifier assignment.

I'm not familiar with our semantic model but wonder if it would be cheaper to

Match on all variable declarations

test if it is a const declaration

test if there are any write references for that variable.

That could be cheaper assuming there are fewer declarations than assignments. But this is only a hypothesis and also heavily depends on how the semantic model is implemented internally.

We should able to use the semantic model straight from the rule. Let me if I am up to the task

github-actions · 2022-10-12T16:00:55Z

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.05      2.7±0.02ms     4.3 MB/sec    1.00      2.6±0.05ms     4.5 MB/sec
analyzer/index.js         1.05      7.8±0.03ms     4.2 MB/sec    1.00      7.4±0.18ms     4.4 MB/sec
analyzer/lint.ts          1.04      3.6±0.03ms    11.6 MB/sec    1.00      3.5±0.11ms    12.0 MB/sec
analyzer/parser.ts        1.01      9.0±0.07ms     5.4 MB/sec    1.00      8.9±0.15ms     5.5 MB/sec
analyzer/router.ts        1.01      6.5±0.07ms     9.5 MB/sec    1.00      6.4±0.14ms     9.6 MB/sec
analyzer/statement.ts     1.00      9.6±0.11ms     3.7 MB/sec    1.01      9.8±0.15ms     3.6 MB/sec
analyzer/typescript.ts    1.05     15.3±0.15ms     3.6 MB/sec    1.00     14.6±0.33ms     3.7 MB/sec

MichaReiser · 2022-10-12T16:03:46Z

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.05      2.7±0.02ms     4.3 MB/sec    1.00      2.6±0.05ms     4.5 MB/sec
analyzer/index.js         1.05      7.8±0.03ms     4.2 MB/sec    1.00      7.4±0.18ms     4.4 MB/sec
analyzer/lint.ts          1.04      3.6±0.03ms    11.6 MB/sec    1.00      3.5±0.11ms    12.0 MB/sec
analyzer/parser.ts        1.01      9.0±0.07ms     5.4 MB/sec    1.00      8.9±0.15ms     5.5 MB/sec
analyzer/router.ts        1.01      6.5±0.07ms     9.5 MB/sec    1.00      6.4±0.14ms     9.6 MB/sec
analyzer/statement.ts     1.00      9.6±0.11ms     3.7 MB/sec    1.01      9.8±0.15ms     3.6 MB/sec
analyzer/typescript.ts    1.05     15.3±0.15ms     3.6 MB/sec    1.00     14.6±0.33ms     3.7 MB/sec

This is surprising... Do nursery rules run as part of our benchmark?

ematipico · 2022-10-12T16:05:58Z

This is surprising... Do nursery rules run as part of our benchmark?

I don't think so

xunilrj · 2022-10-13T11:50:33Z

Should this be a lint or a "parser/semantic" error? We already have one case #2960 that should go into the semantic model.

JS runtimes give an error to this.

ematipico · 2022-10-13T12:26:57Z

Should this be a lint or a "parser/semantic" error? We already have one case #2960 that should go into the semantic model.

JS runtimes give an error to this.

I think it should be a lint rule, mostly because:

the parser doesn't have a semantic model baked in, it would need to track all the const inside the file
the semantic model is an opt-in service/feature, which means if hypothetically all rules that use semantic services are tuned off, we should not expect any error?

But if our semantic model is powerful enough to have these "runtime" checks, then I am up for it.

leops · 2022-10-13T12:45:23Z

Should this be a lint or a "parser/semantic" error? We already have one case #2960 that should go into the semantic model.

JS runtimes give an error to this.

I think it should stay as a lint rule rather than a syntax rule, although for a slightly different reason.
Syntax rules are meant to emit a diagnostic if they encounter something that would cause a SyntaxError according to the ECMAScript spec, for instance re-declaration errors like const a = 1; const a = 2; are a good use case for these as they are a lot easier to track in the semantic model than in the parser.
However assigning to a constant is not a SyntaxError but a TypeError (the main difference is that it is emitted at runtime, so function foo() { const a = 1; const a = 2; } will fail immediately on load while function foo() { const a = 1; a = 2; } will only fail when the foo() function is called).
Since catching potential runtime errors is a task for the linter (conceptually the syntax analyzer is actually a part of the parser, not the linter), the noConstAssign rule should remain in the Lint category.

ematipico requested review from leops and xunilrj as code owners October 12, 2022 13:38

ematipico requested a review from a team October 12, 2022 13:38

ematipico temporarily deployed to netlify-playground October 12, 2022 13:38 Inactive

ematipico force-pushed the feature/no-const-assign branch from f737408 to 6205c2c Compare October 12, 2022 13:38

ematipico temporarily deployed to netlify-playground October 12, 2022 13:38 Inactive

ematipico force-pushed the feature/no-const-assign branch from 6205c2c to 69efbcc Compare October 12, 2022 14:20

ematipico temporarily deployed to netlify-playground October 12, 2022 14:20 Inactive

leops approved these changes Oct 12, 2022

View reviewed changes

MichaReiser closed this Oct 12, 2022

MichaReiser reopened this Oct 12, 2022

MichaReiser temporarily deployed to netlify-playground October 12, 2022 15:49 Inactive

MichaReiser reviewed Oct 12, 2022

View reviewed changes

ematipico force-pushed the feature/no-const-assign branch from 69efbcc to 221f31c Compare October 14, 2022 15:57

ematipico temporarily deployed to netlify-playground October 14, 2022 15:57 Inactive

ematipico added 3 commits October 17, 2022 09:19

feat(rome_js_analyze): rule noConstAssign

7698922

chore: rebase

fc6c576

chore: rebase

2e1821c

ematipico force-pushed the feature/no-const-assign branch from 221f31c to 2e1821c Compare October 17, 2022 08:21

ematipico temporarily deployed to netlify-playground October 17, 2022 08:21 Inactive

ematipico merged commit e38e773 into main Oct 17, 2022

ematipico deleted the feature/no-const-assign branch October 17, 2022 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(rome_js_analyze): rule `noConstAssign` #3409

feat(rome_js_analyze): rule `noConstAssign` #3409

Uh oh!

ematipico commented Oct 12, 2022 •

edited

Loading

Uh oh!

netlify bot commented Oct 12, 2022 •

edited

Loading

Uh oh!

github-actions bot commented Oct 12, 2022 •

edited

Loading

Uh oh!

MichaReiser commented Oct 12, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaReiser Oct 12, 2022

Uh oh!

ematipico Oct 12, 2022

Uh oh!

github-actions bot commented Oct 12, 2022

Uh oh!

MichaReiser commented Oct 12, 2022

Analyzer Benchmark Results

Uh oh!

ematipico commented Oct 12, 2022

Uh oh!

xunilrj commented Oct 13, 2022 •

edited

Loading

Uh oh!

ematipico commented Oct 13, 2022

Uh oh!

leops commented Oct 13, 2022

Uh oh!

Uh oh!

feat(rome_js_analyze): rule noConstAssign #3409

feat(rome_js_analyze): rule noConstAssign #3409

Uh oh!

Conversation

ematipico commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

netlify bot commented Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for rometools ready!

Uh oh!

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

Uh oh!

Uh oh!

MichaReiser commented Oct 12, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaReiser Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

ematipico Oct 12, 2022

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 12, 2022

Analyzer Benchmark Results

Uh oh!

MichaReiser commented Oct 12, 2022

Analyzer Benchmark Results

Uh oh!

ematipico commented Oct 12, 2022

Uh oh!

xunilrj commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ematipico commented Oct 13, 2022

Uh oh!

leops commented Oct 13, 2022

Uh oh!

Uh oh!

feat(rome_js_analyze): rule `noConstAssign` #3409

feat(rome_js_analyze): rule `noConstAssign` #3409

ematipico commented Oct 12, 2022 •

edited

Loading

netlify bot commented Oct 12, 2022 •

edited

Loading

github-actions bot commented Oct 12, 2022 •

edited

Loading

xunilrj commented Oct 13, 2022 •

edited

Loading