feat(rome_js_analyze): promote rules

[ematipico]

Summary

Closes #3503

• added support for spread props in useAltText
• updated outdated snapshots inside the CLI
• updated configuration files inside the snapshots of the CLI
• created a new group called security as per feat(rome_js_analyze): promote rules  #3471 (comment)
• created a new group called complexityas per Move `useTemplate` and `useSimplifiedLogicalExpression` to `style` group #3461
• the rule useCamelCase stays in nursery because it's still full of false positives (lots of them happen in our repo)
• moves existing rules in more suited groups
• I decided to recommend noUnreachable because we need data in order to make the CFG better, and this rule is perfect because because it reaches the boundaries of the feature. Although it means that we will need to focus out attention on possible bugs: @leops @xunilrj , what do you think?

This PR promotes the following rules:

Rule name	Old Group	New group	Reason	Recommended
noPositiveTabindex	nursery	a11y	It focuses on accessibility	true
useKeyWithMouseEvents	nursery	a11y	It focuses on accessibility	true
useAnchorContent	nursery	a11y	It focuses on accessibility	true
useBlankTarget	nursery	a11y	It focuses on accessibility	true
useValidAnchor	nursery	a11y	It focuses on accessibility	true
useKeyWithClickEvents	nursery	a11y	It focuses on accessibility	true
useButtonType	nursery	a11y	It focuses on accessibility	true
noAutofocus	nursery	a11y	It focuses on accessibility	true
useFragmentSyntax	nursery	style	It's just syntax sugar	true
useTemplate	correctness	style	Just a styling decision	true
useBlockStatements	correctness	style	Just a styling decision	true
noImplicitBoolean	correctness	style	Just a styling decision	true
useCamelCase	nursery	style	Just a styling decision	true
useOptionalOperator	nursery	style	Syntax super for checks	true
noVoidElementsWithChildren	nursery	correctness	They help to prevent bugs	true
noChildrenProp	nursery	correctness	They help to prevent bugs	true
noArrayIndexKey	nursery	correctness	They help to prevent bugs	true
noRenderReturnValue	nursery	correctness	They help to prevent bugs	true
noNewSymbol	nursery	correctness	They help to prevent bugs	true
noUselessFragments	nursery	correctness	They help to prevent bugs	true
noUnusedVariables	nursery	correctness	They help to prevent bugs	false
noUnreachable	nursery	correctness	They help to prevent bugs	true
noUnusedVariables	nursery	correctness	They help to prevent bugs	true
noRestrictedGlobals	nursery	correctness	They help to prevent bugs	true
useSimplifiedLogicExpression	correctness	complexity	Prevents complexity	true
noExtraBooleanCast	correctness	complexity	Prevents complexity	true
noDangerouslySetInnerHtml	nursery	security	Needs extra attention	true
noDangerouslySetInnerHtmlWithChildren	nursery	security	Needs extra attention	true
noDebugger	correctness	security	Needs extra attention	true

Test Plan

Snapshot tests should only have their category updated. If there's some code frame changed/removed, it means something has been incorrectly moved.

[MichaReiser]

I think we should avoid framework-specific group naming for the same reason we avoided language-specific group naming. Our configuration should have a framework section instead where it is possible to configure the used frameworks and the analyzer then enables/disabled rules based on that configuration.

For example: noDangerouslySetInnerHtml should be in the security group and can be generalized to noHtmlInjection.

I do understand that we don't have the infrastructure to accomplish this today but that means we should prioritise adding the necessary capabilities to Rome because renaming rules is a confusing experience for our users.

[ematipico]

I think we should avoid framework-specific group naming for the same reason we avoided language-specific group naming.

How would you solve a possible issue where a rule that targets React goes in conflict with another library that uses JSX?

[MichaReiser]

I think we should avoid framework-specific group naming for the same reason we avoided language-specific group naming.

How would you solve a possible issue where a rule that targets React goes in conflict with another library that uses JSX?

Our configuration should have a framework section instead where it is possible to configure the used frameworks. The analyzer then enables/disabled rules based on that configuration. For example. The react rules only get enabled if you configure the react framework (with an optional version) or, as an alternative, by setting a javascript.jsxDialect to react.

The benefit of this approach is that transformation (and maybe even optimisations and bundling) must know about the used frameworks because the transformation may slightly differ.

[Boshen]

I'll help out and test these promoted rules tomorrow (on internal code @ ByteDance).

[ematipico]

@leops and @xunilrj , could you please chime in and let us know what you think?

[MichaReiser]

!bench_analyzer

[github-actions]

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.01      2.6±0.15ms     4.4 MB/sec    1.00      2.6±0.20ms     4.4 MB/sec
analyzer/index.js         1.03      7.6±0.39ms     4.3 MB/sec    1.00      7.3±0.38ms     4.4 MB/sec
analyzer/lint.ts          1.00      3.5±0.19ms    11.9 MB/sec    1.03      3.6±0.23ms    11.5 MB/sec
analyzer/parser.ts        1.01      8.7±0.46ms     5.6 MB/sec    1.00      8.6±0.43ms     5.6 MB/sec
analyzer/router.ts        1.01      6.5±0.45ms     9.5 MB/sec    1.00      6.4±0.30ms     9.6 MB/sec
analyzer/statement.ts     1.01      9.5±0.52ms     3.7 MB/sec    1.00      9.4±0.56ms     3.8 MB/sec
analyzer/typescript.ts    1.03     15.8±0.88ms     3.4 MB/sec    1.00     15.4±0.79ms     3.5 MB/sec

[MichaReiser]

On the grouping:

• noUselessFragments: How is this preventing bugs? In my view, this is more about writing idiomatic JSX and should thus be in the style group (or maybe complexity)
• useOptionalOperator: Same as for noUselessFragements. It isn't preventing bugs but helps you write more idiomatic JavaScript. I could otherwise see this being a complexity rule but I would favour style because using ?. doesn't reduce complexity. Which is the case for clippys ? operator rule

https://github.com/rust-lang/rust-clippy

[ematipico]

useOptionalOperator: Same as for noUselessFragements. It isn't preventing bugs but helps you write more idiomatic JavaScript. I could otherwise see this being a complexity rule but I would favour style because using ?. doesn't reduce complexity. Which is the case for clippys ? operator rule

I was on the fence on this one. Initially I thought about style but then thought that styling issue could be easily overlooked by the users because it cold be seen as "an opinionated set of rules", which a user might just turn off by default. And then I thought about my previous experiences where I encountered cannot read of undefined because of a missed a check (let's leave TS out of the equation); the optional chain operator could have saved me. Having it in a style group feels like not giving enough importance... but technically, it's a style rule, ultimately.

[github-actions]

Parser conformance results on ubuntu-latest

js/262

Test result	main count	This PR count	Difference
Total	45879	45879	0
Passed	44939	44939	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	1621	1621	0
Failed	4325	4325	0
Panics	0	0	0
Coverage	27.26%	27.26%	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	12395	12395	0
Failed	3862	3862	0
Panics	0	0	0
Coverage	76.24%	76.24%	0.00%

[netlify]

✅ Deploy Preview for rometools ready!

Name	Link
🔨 Latest commit	f4eb5f6
🔍 Latest deploy log	https://app.netlify.com/sites/rometools/deploys/6360d0a1c0b783000ac84827
😎 Deploy Preview	https://deploy-preview-3471--rometools.netlify.app/docs/lint/rules/nounusedvariables
📱 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.

[MichaReiser]

Thanks for working out all the new groups. There's a merge issue in the codegen that must be resolved and I do have a question around the grouping of noDebugger:
What are the security concerns around debugger? I think if there's a security concern then the lint diagnostic should point that out. I lean towards correctness because its useless code in production or has the undesired effect of stopping execution all together

[ematipico]

Thanks for working out all the new groups. There's a merge issue in the codegen that must be resolved and I do have a question around the grouping of noDebugger:
What are the security concerns around debugger? I think if there's a security concern then the lint diagnostic should point that out. I lean towards correctness because its useless code in production or has the undesired effect of stopping execution all together

My concern is around the fact that deploying code with debugger, would inevitably trigger a breakpoint if someone opens the developer console or runs the debugger in a backend app. Imagine being a library author and a debugger gets published.

I don't consider the code useless because it's a tool someone can use and it's helpful (like console logs). Maybe a misuse of the language but it's also debatable.

[MichaReiser]

My concern is around the fact that deploying code with debugger, would inevitably trigger a breakpoint if someone opens the developer console or runs the debugger in a backend app. Imagine being a library author and a debugger gets published.

That's why I would consider this a bug/broken code. It is code with unintended effects. I share your sentiment that it doesn't perfectly falls into the correctness group but I don't see a security concern.

[ematipico]

@MichaReiser merge conflicts fixed and noDebugger moved under correctness

[calibre-analytics]

Comparing feat(rome_js_analyze): promote rules Snapshot #13 to median since last deploy of rome.tools.

	LCP?	CLS?	TBT?
Overall Median across all pages and test profiles	249ms  from 224ms	0.045 no change	20ms  from 18ms
Chrome Desktop Chrome Desktop • Cable	249ms  from 224ms	0.016  from 0.006	91ms  from 102ms
iPhone, 4G LTE iPhone 12 • 4G LTE	239ms  from 147ms	0.045 no change	0ms no change
Motorola Moto G Power, 3G connection Motorola Moto G Power • Regular 3G	648ms  from 573ms	0.049 no change	20ms  from 18ms

1 page tested

 Home

Browser previews

Chrome Desktop	iPhone, 4G LTE	Motorola Moto G Power, 3G connection

Most significant changes

	Value	Budget
Total JavaScript Size in Bytes Chrome Desktop	1.23 MB  from 86.8 KB
Total JavaScript Size in Bytes iPhone, 4G LTE	1.23 MB  from 86.8 KB
Total JavaScript Size in Bytes Motorola Moto G Power, 3G connection	1.23 MB  from 86.8 KB
Number of Requests Chrome Desktop	39  from 5
Number of Requests iPhone, 4G LTE	39  from 5

_{4 other significant changes: Number of Requests on Motorola Moto G Power, 3G connection, Total Page Size in Bytes on Chrome Desktop, Total Page Size in Bytes on iPhone, 4G LTE, Total Page Size in Bytes on Motorola Moto G Power, 3G connection}

_{Calibre: Site dashboard | View this PR | Edit settings | View documentation}

[MichaReiser]

Looking good from a code perspective.

Should we reconsider promoting noUnusedVariables considering that it still reports a vast number of false positives?

Do you plan to test the new rules on real projects before merging to verify that they meet our quality criteria?

[ematipico]

Looking good from a code perspective.

Should we reconsider promoting noUnusedVariables considering that it still reports a vast number of false positives?

The current bugs have been closed. For now I moved it to a stable group and marked as not recommended.

Do you plan to test the new rules on real projects before merging to verify that they meet our quality criteria?

Yeah I did, and fixed some false positives in other PRs. We should be good to go.