feat(rome_js_analyze): `useDefaultSwitchClauseLast` #3791

Conaclos · 2022-11-18T17:44:33Z

Summary

This implements ESLint's default-case-last.

Test Plan

Unit test included.

netlify · 2022-11-18T17:44:38Z

✅ Deploy Preview for docs-rometools ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`12d4488`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-rometools/deploys/637e92c3184866000879140a
😎 Deploy Preview	https://deploy-preview-3791--docs-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.

crates/rome_js_analyze/src/analyzers/nursery/use_default_case_last.rs

crates/rome_js_analyze/tests/specs/nursery/useDefaultCaseLast/invalid.js.snap

ematipico

I think we can also provide a code action that moves the default statement to the last position. We can add the code action in another PR.

ematipico · 2022-11-21T11:30:49Z

crates/rome_js_analyze/src/analyzers/nursery/use_default_case_last.rs

+
+impl Rule for UseDefaultCaseLast {
+    type Query = Ast<JsSwitchCaseList>;
+    type State = JsAnySwitchClause;


The state could be a TextRange

Is there advantages to use TextRange instead of JsAnySwitchClause?

Not particularly, simply less information to carry around :)

It is expensive to calculate range in some cases. Specially when you use the trimmed version that needs to skip trivia.

It is expensive to calculate range in some cases. Specially when you use the trimmed version that needs to skip trivia.

Computing the range in run or in diagnostic has the same cost?

Not particularly, simply less information to carry around :)

I agree (information hiding). However, I think that in this code, it is good to keep more info (here the next case). This makes the rule more consistent with others and enables to get more context (this could be useful for improving diagnostic in a future revision).

crates/rome_js_analyze/src/analyzers/nursery/use_default_case_last.rs

Conaclos · 2022-11-22T22:49:57Z

@ematipico

I think we can also provide a code action that moves the default statement to the last position. We can add the code action in another PR.

The difficulty is when a fallthrough is present. Like in the following code:

switch (foo) {
  default:
    doSomethingIfNotZero();
  case 0:
    doSomethingAnyway();
}

This requires some logic to know whether the block return/break. Have you an idea how to implement this without exploring all descendants? Or we could just handle the case where a block statement ends with return or break. This looks fair.

In fact I - in constrast to ESLint - we could allow code with default clause that fallthrough (as the previous code). Another rule (noFallthrough) could reject this code. This could make useDefaultCaseLast less restrictive.

Also, I wonder if we should rename the rule "useDefaultCaseLast" to "useDefaultClauseLast" or "useDefaultSwitchClauseLast".

MichaReiser · 2022-11-23T07:05:45Z

!bench_analyzer

MichaReiser · 2022-11-23T07:07:49Z

Also, I wonder if we should rename the rule "useDefaultCaseLast" to "useDefaultClauseLast" or "useDefaultSwitchClauseLast".

I'm in favor of renaming Case to Clause as this aligns with the terminology used in the diagnostics and useDefaultSwitchClauseLast goes well with our philosophy;

Utilize verbosity when naming commands and flags. No unnecessary and confusing abbreviations.

github-actions · 2022-11-23T07:15:28Z

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.17      2.5±0.01ms     4.7 MB/sec    1.00      2.1±0.02ms     5.5 MB/sec
analyzer/index.js         1.08      6.6±0.12ms     5.0 MB/sec    1.00      6.1±0.03ms     5.3 MB/sec
analyzer/lint.ts          1.04      3.2±0.01ms    13.1 MB/sec    1.00      3.1±0.01ms    13.6 MB/sec
analyzer/parser.ts        1.08      7.8±0.24ms     6.3 MB/sec    1.00      7.2±0.11ms     6.8 MB/sec
analyzer/router.ts        1.08      5.4±0.03ms    11.6 MB/sec    1.00      5.0±0.01ms    12.5 MB/sec
analyzer/statement.ts     1.05      7.3±0.12ms     4.8 MB/sec    1.00      7.0±0.04ms     5.1 MB/sec
analyzer/typescript.ts    1.03     10.9±0.18ms     5.0 MB/sec    1.00     10.5±0.03ms     5.2 MB/sec

Conaclos · 2022-11-23T21:51:48Z

@ematipico @MichaReiser
Ready for merging.

I will open a new PR for the code action. This allows discussing that in details.

Unfortunately, a commit in main causes a failure on the format check. Indeed, ./crates/rome_js_analyze/src/semantic_analyzers/nursery/use_exhaustive_dependencies.rs was not formatted with rustfmt.

lucasweng · 2022-11-24T04:44:02Z

Unfortunately, a commit in main causes a failure on the format check. Indeed, ./crates/rome_js_analyze/src/semantic_analyzers/nursery/use_exhaustive_dependencies.rs was not formatted with rustfmt.

Fixed in #3840

* upstream/main: (73 commits) fix(semantic_analyzers): style/noShoutyConstants does not recognize multiple uses of a constant. (rome#3789) feat(rome_js_analyze): useDefaultSwitchClauseLast (rome#3791) chore: run rustfmt and typo fix (rome#3840) feat(rome_js_analyze): use exhaustive deps support properties (rome#3581) website(docs): Fix text formatting (rome#3828) feat(rome_js_analyze): `noVoidTypeReturn` (rome#3806) feat(rome_cli): expose the `--verbose` flag to the CLI (rome#3812) fix(rome_diagnostics): allow diagnostic locations to be created without a resource (rome#3834) feat(rome_js_analyze): add noExtraNonNullAssertion rule (rome#3797) fix(rome_lsp): lsp friendly catch unwind (rome#3740) feat(rome_js_semantic): model improvements (rome#3825) feat(rome_json_parser): JSON Lexer (rome#3809) feat(rome_js_analyze): implement `noDistractingElements` (rome#3820) fix(rome_js_formatter): shothanded named import line break with default import (rome#3826) feat(rome_js_analyze): `noConstructorReturn` (rome#3805) feat(website): change enabledNurseryRules to All/Recommended select (rome#3810) feat(rome_js_analyze): noSetterReturn feat(rome_js_analyze): noConstructorReturn feat(rome_analyze): suppress rule via code actions (rome#3572) feat(rome_js_analyze): `noVar` (rome#3765) ...

Conaclos requested review from leops, ematipico and xunilrj as code owners November 18, 2022 17:44

Conaclos requested a review from a team November 18, 2022 17:44

Conaclos changed the title ~~feat(rome_js_analyze): useDefaultCaseLast~~ feat(rome_js_analyze): useDefaultCaseLast Nov 19, 2022

MichaReiser reviewed Nov 21, 2022

View reviewed changes

crates/rome_js_analyze/src/analyzers/nursery/use_default_case_last.rs Outdated Show resolved Hide resolved

MichaReiser reviewed Nov 21, 2022

View reviewed changes

crates/rome_js_analyze/tests/specs/nursery/useDefaultCaseLast/invalid.js.snap Outdated Show resolved Hide resolved

ematipico suggested changes Nov 21, 2022

View reviewed changes

Conaclos requested review from ematipico and removed request for xunilrj and leops November 22, 2022 23:06

MichaReiser added A-Linter Area: linter L-JavaScript Langauge: JavaScript labels Nov 23, 2022

MichaReiser added this to the 11.0.0 milestone Nov 23, 2022

MichaReiser approved these changes Nov 23, 2022

View reviewed changes

ematipico approved these changes Nov 23, 2022

View reviewed changes

Conaclos changed the title ~~feat(rome_js_analyze): useDefaultCaseLast~~ feat(rome_js_analyze): useDefaultSwitchClauseLast Nov 23, 2022

feat(rome_js_analyze): useDefaultSwitchClauseLast

12d4488

MichaReiser merged commit 732a32f into rome:main Nov 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rome_js_analyze): `useDefaultSwitchClauseLast` #3791

feat(rome_js_analyze): `useDefaultSwitchClauseLast` #3791

Conaclos commented Nov 18, 2022

netlify bot commented Nov 18, 2022 •

edited

Loading

ematipico left a comment

ematipico Nov 21, 2022

Conaclos Nov 22, 2022 •

edited

Loading

ematipico Nov 23, 2022

xunilrj Nov 23, 2022

Conaclos Nov 23, 2022 •

edited

Loading

Conaclos Nov 23, 2022

Conaclos commented Nov 22, 2022

MichaReiser commented Nov 23, 2022

MichaReiser commented Nov 23, 2022

github-actions bot commented Nov 23, 2022

Conaclos commented Nov 23, 2022 •

edited

Loading

lucasweng commented Nov 24, 2022

feat(rome_js_analyze): useDefaultSwitchClauseLast #3791

feat(rome_js_analyze): useDefaultSwitchClauseLast #3791

Conversation

Conaclos commented Nov 18, 2022

Summary

Test Plan

netlify bot commented Nov 18, 2022 • edited Loading

✅ Deploy Preview for docs-rometools ready!

ematipico left a comment

Choose a reason for hiding this comment

ematipico Nov 21, 2022

Choose a reason for hiding this comment

Conaclos Nov 22, 2022 • edited Loading

Choose a reason for hiding this comment

ematipico Nov 23, 2022

Choose a reason for hiding this comment

xunilrj Nov 23, 2022

Choose a reason for hiding this comment

Conaclos Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

Conaclos Nov 23, 2022

Choose a reason for hiding this comment

Conaclos commented Nov 22, 2022

MichaReiser commented Nov 23, 2022

MichaReiser commented Nov 23, 2022

github-actions bot commented Nov 23, 2022

Analyzer Benchmark Results

Conaclos commented Nov 23, 2022 • edited Loading

lucasweng commented Nov 24, 2022

feat(rome_js_analyze): `useDefaultSwitchClauseLast` #3791

feat(rome_js_analyze): `useDefaultSwitchClauseLast` #3791

netlify bot commented Nov 18, 2022 •

edited

Loading

Conaclos Nov 22, 2022 •

edited

Loading

Conaclos Nov 23, 2022 •

edited

Loading

Conaclos commented Nov 23, 2022 •

edited

Loading