feat(rome_js_analyze): `noUnusedVariable` rule #2987

xunilrj · 2022-08-02T09:22:06Z

Summary

Closes #2979.

Test Plan

>  cargo run -p rome_cli -- check crates/rome_js_analyze/tests/specs/js/noUnusedVariables.js

cloudflare-workers-and-pages · 2022-08-02T09:34:36Z

Deploying with Cloudflare Pages

Latest commit:	`c98fcf7`
Status:	✅ Deploy successful!
Preview URL:	https://a888494d.tools-8rn.pages.dev
Branch Preview URL:	https://feature-no-unused-variable.tools-8rn.pages.dev

View logs

xunilrj · 2022-08-02T09:35:55Z

!bench_analyzer

github-actions · 2022-08-02T09:39:38Z

Playground for commit 8f39935

github-actions · 2022-08-02T09:40:48Z

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-08-02T09:48:22Z

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.00      2.4±0.12ms     4.7 MB/sec    1.03      2.5±0.08ms     4.6 MB/sec
analyzer/index.js         1.00      6.0±0.39ms     5.4 MB/sec    1.00      6.0±0.27ms     5.4 MB/sec
analyzer/lint.ts          1.00      3.3±0.10ms    12.4 MB/sec    1.02      3.4±0.14ms    12.2 MB/sec
analyzer/parser.ts        1.00      7.8±0.23ms     6.2 MB/sec    1.03      8.0±0.25ms     6.0 MB/sec
analyzer/router.ts        1.00      5.4±0.19ms    11.3 MB/sec    1.03      5.6±0.15ms    11.0 MB/sec
analyzer/statement.ts     1.00      7.4±0.31ms     4.8 MB/sec    1.04      7.6±0.25ms     4.7 MB/sec
analyzer/typescript.ts    1.00     11.2±1.12ms     4.8 MB/sec    1.00     11.3±0.35ms     4.8 MB/sec

ematipico

There are few issues with functions, we have diagnostics when in reality they are valid cases.

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

ematipico · 2022-08-02T13:18:49Z

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

+warning[js/noUnusedVariables]: This parameter is unused.
+   ┌─ noUnusedVariables.js:21:17
+   │
+21 │ function f42(a, b) {


Could we add a test case where we have a trailing comma? function f(a,b,){}

I create more tests for the "remove_*" methods, instead of polluting this one here.
https://github.com/rome/tools/pull/2987/files#diff-0902796fee286e39e6f27f612b14a08a77e0af0dee55c59871e1a0292707c0faR110

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

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

ematipico · 2022-08-02T14:00:39Z

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

+
+        Some(JsRuleAction {
+            category: ActionCategory::Refactor,
+            applicability: Applicability::Unspecified,


Considering that the applicability is Unspecified, what does it mean for the end user? Can an user apply the change via CLI?

what does it mean for the end user? Can an user apply the change via CLI?
No.

I am torn apart between Unspecifiedand MaybeIncorrect. I assumed that removing unused code is not the "best" solution.

The semantics of Applicability is something we'll need to review when we rework the diagnostics, the current version of the enum is inherited directly from rslint and indirectly rustc, but those tools are using text-based suggestions instead of tree-based.
In this case I think the Unspecified applicability comes from rustc, where the concept of "applicability" was added when the compiler already has a bunch of existing suggestions, so all those diagnostics were set to Unspecified in the refactor to be manually reviewed later on.
In our case though we should always document if a suggestion is safe to apply or not, so in practice the only variants of Applicability we should be using a MaybeIncorrect and Always (the difference is that Unspecified can never be applied automatically, while MaybeIncorrect can after the has the user has reviewed it)

ematipico · 2022-08-02T15:43:10Z

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

+        }
+
+        Some(JsRuleAction {
+            category: ActionCategory::Refactor,


Why is this a Refactor and not a QuickFix? Just asking because I don't know yet the difference

You are right. I think "Quickfix" makes more sense.

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

ematipico

Looks good to me. We should address the applicability of the rule.

* no unused variables lint rule

xunilrj requested review from leops and ematipico as code owners August 2, 2022 09:22

xunilrj force-pushed the feature/no-unused-variable branch from 4645324 to c07b191 Compare August 2, 2022 09:34

xunilrj requested a review from a team August 2, 2022 09:34

xunilrj temporarily deployed to aws August 2, 2022 09:34 Inactive

ematipico suggested changes Aug 2, 2022

View reviewed changes

ematipico reviewed Aug 2, 2022

View reviewed changes

crates/rome_js_analyze/src/semantic_analyzers/js/no_unused_variables.rs Show resolved Hide resolved

ematipico reviewed Aug 2, 2022

View reviewed changes

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

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

ematipico reviewed Aug 3, 2022

View reviewed changes

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

xunilrj temporarily deployed to aws August 3, 2022 09:11 Inactive

ematipico changed the title ~~feat(rome_js_analyze): no unused variable~~ feat(rome_js_analyze): noUnusedVariable rule Aug 3, 2022

ematipico approved these changes Aug 3, 2022

View reviewed changes

xunilrj added 6 commits August 3, 2022 11:19

no unused variables lint rule

f707a6d

fmt and clippy issues

470f397

lint doc and removing dbg

5fcdca9

remove js nodes tests

88909bc

adjustments

7de4a6b

codegen

c98fcf7

xunilrj force-pushed the feature/no-unused-variable branch from 381934d to c98fcf7 Compare August 3, 2022 10:23

xunilrj temporarily deployed to aws August 3, 2022 10:23 Inactive

xunilrj merged commit d2d5068 into main Aug 3, 2022

xunilrj deleted the feature/no-unused-variable branch August 3, 2022 10:37

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

feat(rome_js_analyze): noUnusedVariable rule (rome#2987)

1c0d779

* no unused variables lint rule

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rome_js_analyze): `noUnusedVariable` rule #2987

feat(rome_js_analyze): `noUnusedVariable` rule #2987

xunilrj commented Aug 2, 2022

cloudflare-workers-and-pages bot commented Aug 2, 2022 •

edited

Loading

xunilrj commented Aug 2, 2022

github-actions bot commented Aug 2, 2022 •

edited

Loading

github-actions bot commented Aug 2, 2022

github-actions bot commented Aug 2, 2022

ematipico left a comment

ematipico Aug 2, 2022

xunilrj Aug 3, 2022

ematipico Aug 2, 2022

xunilrj Aug 3, 2022

leops Aug 3, 2022

ematipico Aug 2, 2022

xunilrj Aug 3, 2022

ematipico left a comment

feat(rome_js_analyze): noUnusedVariable rule #2987

feat(rome_js_analyze): noUnusedVariable rule #2987

Conversation

xunilrj commented Aug 2, 2022

Summary

Test Plan

cloudflare-workers-and-pages bot commented Aug 2, 2022 • edited Loading

Deploying with Cloudflare Pages

xunilrj commented Aug 2, 2022

github-actions bot commented Aug 2, 2022 • edited Loading

github-actions bot commented Aug 2, 2022

Parser conformance results on ubuntu-latest

js/262

jsx/babel

symbols/microsoft

ts/babel

ts/microsoft

github-actions bot commented Aug 2, 2022

Analyzer Benchmark Results

ematipico left a comment

Choose a reason for hiding this comment

ematipico Aug 2, 2022

Choose a reason for hiding this comment

xunilrj Aug 3, 2022

Choose a reason for hiding this comment

ematipico Aug 2, 2022

Choose a reason for hiding this comment

xunilrj Aug 3, 2022

Choose a reason for hiding this comment

leops Aug 3, 2022

Choose a reason for hiding this comment

ematipico Aug 2, 2022

Choose a reason for hiding this comment

xunilrj Aug 3, 2022

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

feat(rome_js_analyze): `noUnusedVariable` rule #2987

feat(rome_js_analyze): `noUnusedVariable` rule #2987

cloudflare-workers-and-pages bot commented Aug 2, 2022 •

edited

Loading

github-actions bot commented Aug 2, 2022 •

edited

Loading