fix(rome_js_analyze): fix for unused flag on functional types on ts #3860

xunilrj · 2022-11-25T21:36:15Z

Summary

Fixes #3669.
This PR also:

reorganizes all the tests for noUnusedVariables;
improves suggestion to rename unused variables (taking into consideration pattern matching, for example).

I have also implemented the navigation from JsIdentifierBinding to its declaration and from parameters to its parent function. These will not only simplify lint rules, but also help in edgy and forgotten cases.

This PR also implements a hardfail for valid tests that contains "/* should not gnerate diagnostics */", but that still generate them.

Test Plan

> cargo test -p rome_js_analyze -- no_unused_variables

netlify · 2022-11-25T21:36:19Z

✅ Deploy Preview for docs-rometools canceled.

Name	Link
🔨 Latest commit	`de7d4fe`
🔍 Latest deploy log	https://app.netlify.com/sites/docs-rometools/deploys/638b96c8619be500084db57f

xunilrj · 2022-11-25T21:41:49Z

!bench_analyzer

github-actions · 2022-11-25T21:43:24Z

Parser conformance results on ubuntu-latest

js/262

Test result	`main` count	This PR count	Difference
Total	45879	45879	0
Passed	44936	44936	0
Failed	943	943	0
Panics	0	0	0
Coverage	97.94%	97.94%	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	1757	1707	❌ ⏬ -50
Failed	4189	4239	❌ ⏫ +50
Panics	0	0	0
Coverage	29.55%	28.71%	-0.84%

🔥 Regression (50):

asOperator3.symbols
asOperatorASI.symbols
blockScopedBindingsReassignedInLoop1.symbols
capturedLetConstInLoop1.symbols
capturedLetConstInLoop3.symbols
capturedLetConstInLoop3_ES6.symbols
commentOnAmbientfunction.symbols
conditionalTypeBasedContextualTypeReturnTypeWidening.symbols
conditionallyDuplicateOverloadsCausedByOverloadResolution.symbols
constraintSatisfactionWithAny2.symbols
contextualSigInstantiationRestParams.symbols
contextualSignatureInstantiation4.symbols
contextualTypingFunctionReturningFunction2.symbols
contravariantTypeAliasInference.symbols
controlFlowCommaExpressionAssertionWithinTernary.symbols
controlFlowTruthiness.symbols
downlevelLetConst17.symbols
exportEqualsOfModule.symbols
externFunc.symbols
fallFromLastCase1.symbols
fallbackToBindingPatternForTypeInference.symbols
freshLiteralTypesInIntersections.symbols
functionExpressionContextualTyping3.symbols
importCallExpressionDeclarationEmit1.symbols
inferentialTypingWithFunctionType.symbols
inferentiallyTypingAnEmptyArray.symbols
keyofModuleObjectHasCorrectKeys.symbols
letConstMatchingParameterNames.symbols
noImplicitReturnsWithProtectedBlocks1.symbols
nullishCoalescingOperator10.symbols
overloadsAndTypeArgumentArity.symbols
partiallyAnnotatedFunctionWitoutTypeParameter.symbols
propagateNonInferrableType.symbols
reachabilityCheckWithEmptyDefault.symbols
selfReference.symbols
shebangBeforeReferences.symbols
strictNullChecksNoWidening.symbols
stringLiteralTypesAndParenthesizedExpressions01.symbols
stringLiteralTypesOverloads04.symbols
subtypingWithConstructSignatures.symbols
taggedTemplateStringWithSymbolExpression01.symbols
tupleTypeInference2.symbols
typeGuardNarrowsToLiteralType.symbols
typeGuardNarrowsToLiteralTypeUnion.symbols
typeInferenceFixEarly.symbols
typeInferenceTypePredicate.symbols
typeInferenceWithTypeAnnotation.symbols
typePredicatesInUnion2.symbols
untypedArgumentInLambdaExpression.symbols
wideningTuples1.symbols

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	12397	12397	0
Failed	3860	3860	0
Panics	0	0	0
Coverage	76.26%	76.26%	0.00%

github-actions · 2022-11-25T21:51:42Z

Analyzer Benchmark Results

group                     main                                   pr
-----                     ----                                   --
analyzer/css.js           1.00      2.4±0.03ms     4.9 MB/sec    1.00      2.4±0.03ms     4.9 MB/sec
analyzer/index.js         1.00      6.2±0.01ms     5.3 MB/sec    1.00      6.3±0.05ms     5.2 MB/sec
analyzer/lint.ts          1.01      2.8±0.02ms    14.7 MB/sec    1.00      2.8±0.02ms    14.8 MB/sec
analyzer/parser.ts        1.00      7.1±0.02ms     6.8 MB/sec    1.02      7.3±0.04ms     6.7 MB/sec
analyzer/router.ts        1.00      3.1±0.01ms    11.1 MB/sec    1.01      3.1±0.02ms    11.0 MB/sec
analyzer/statement.ts     1.00      6.9±0.01ms     5.2 MB/sec    1.00      6.9±0.03ms     5.1 MB/sec
analyzer/typescript.ts    1.00     10.1±0.09ms     5.4 MB/sec    1.00     10.2±0.03ms     5.4 MB/sec

calibre-analytics · 2022-11-28T10:48:09Z

Comparing fix(rome_js_analyze): fix for unused flag on functional types on ts Snapshot #12 to median since last deploy of rome.tools.

	LCP^?	CLS^?	TBT^?
Overall ^{Median across all pages and test profiles}	2.65s ^from 282ms	0.0 ^no change	138ms ^no change
Chrome Desktop ^{Chrome Desktop • Cable}	2.65s ^from 282ms	0.0 ^no change	376ms ^from 22ms
iPhone, 4G LTE ^{iPhone 12 • 4G LTE}	1.18s ^from 240ms	0.0 ^no change	21ms ^no change
Motorola Moto G Power, 3G connection ^{Motorola Moto G Power • Regular 3G}	19.3s ^from 1.07s	0.0 ^no change	138ms ^no change

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	5.67 MB ^{from 86.8 KB}
Total JavaScript Size in Bytes iPhone, 4G LTE	5.67 MB ^{from 86.8 KB}
Total JavaScript Size in Bytes Motorola Moto G Power, 3G connection	5.67 MB ^{from 86.8 KB}
JS Parse & Compile Motorola Moto G Power, 3G connection	1.67s ^from 28ms
JS Parse & Compile iPhone, 4G LTE	511ms ^from 12ms

_{27 other significant changes: JS Parse & Compile on Chrome Desktop, Largest Contentful Paint on Motorola Moto G Power, 3G connection, First Contentful Paint on Motorola Moto G Power, 3G connection, Total Blocking Time on Chrome Desktop, Time to Interactive 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, Total CSS Size in Bytes on Chrome Desktop, Total CSS Size in Bytes on iPhone, 4G LTE, Total CSS Size in Bytes on Motorola Moto G Power, 3G connection, Time to Interactive on Chrome Desktop, First Contentful Paint on Chrome Desktop, Largest Contentful Paint on Chrome Desktop, Number of Requests on Motorola Moto G Power, 3G connection, Number of Requests on Chrome Desktop, Number of Requests on iPhone, 4G LTE, Speed Index on Motorola Moto G Power, 3G connection, Time to Interactive on iPhone, 4G LTE, First Contentful Paint on iPhone, 4G LTE, Largest Contentful Paint on iPhone, 4G LTE, Speed Index on Chrome Desktop, Total HTML Size in Bytes on Chrome Desktop, Total HTML Size in Bytes on iPhone, 4G LTE, Total HTML Size in Bytes on Motorola Moto G Power, 3G connection, Lighthouse Performance Score on Motorola Moto G Power, 3G connection, Lighthouse Performance Score on Chrome Desktop}

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

crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/validClass.ts.snap

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

MichaReiser · 2022-11-29T11:07:12Z

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

+        // Some parameters are ok to not be used
+        JsAnyBindingDeclaration::TsPropertyParameter(parameter) => {
+            let is_binding_ok =
+                is_ok_to_be_unused(parameter.parent_function()) || is_public_or_private(parameter)?;


I'm struggling to understand what the return value of is_public_or_private is. Does it return true if it is private or when it is public.

Suggestion:

rename to is_public or is_private

Add a visibility enum and change it to visibility(parameter).is_public()

Changed the name. Hopefully makes more sense now.

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

crates/rome_js_syntax/src/binding_ext.rs

leops · 2022-12-01T08:39:31Z

crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/validFunctions.ts.snap

+    1 │ // used function declaration
+  > 2 │ function a() {}
+      │          ^
+    3 │ a()


This one seems to be a false positive too, the function is being used on the next line

You have keen eyes! 👀
This was actually a scope resolution bug. Thanks!

xunilrj · 2022-12-03T18:43:58Z

We don't suggest renaming imports anymore, so we fix the broken suggestion at #3786.

xunilrj force-pushed the fix/3669-no-unused-variables branch from a3d9499 to d267622 Compare November 28, 2022 10:43

xunilrj marked this pull request as ready for review November 28, 2022 10:43

xunilrj requested review from leops, ematipico and a team as code owners November 28, 2022 10:43

leops reviewed Nov 29, 2022

View reviewed changes

crates/rome_js_analyze/tests/specs/correctness/noUnusedVariables/validClass.ts.snap Outdated Show resolved Hide resolved

MichaReiser reviewed Nov 29, 2022

View reviewed changes

xunilrj force-pushed the fix/3669-no-unused-variables branch 2 times, most recently from a73de33 to 6f7a386 Compare November 30, 2022 16:32

leops reviewed Dec 1, 2022

View reviewed changes

xunilrj force-pushed the fix/3669-no-unused-variables branch from 259278d to bfb7632 Compare December 2, 2022 13:07

xunilrj added 12 commits December 3, 2022 11:22

fix for unused flag on functional types on ts

8b87f15

codegen issues

357b7ee

codegen issues

ef7f8bd

rebase and fixing type alias

313c388

renaming node unions

6c5f6e2

renaming functions

11bb8f1

fix ts property parameter

fcd053b

renaming function

9c2bf93

clippy and fmt issues

65b63fb

codegen issues

4eb5586

clippy and fmt issues

27fe41d

clippy and fmt issues

b99354d

xunilrj force-pushed the fix/3669-no-unused-variables branch from ed2a301 to b99354d Compare December 3, 2022 11:24

xunilrj mentioned this pull request Dec 3, 2022

fix(rome_js_analyze): use exhaustive deps considering external const as stable #3926

Merged

more cases of invalid import

de7d4fe

ematipico approved these changes Dec 5, 2022

View reviewed changes

xunilrj merged commit 8b6b6d0 into main Dec 5, 2022

xunilrj deleted the fix/3669-no-unused-variables branch December 5, 2022 11:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rome_js_analyze): fix for unused flag on functional types on ts #3860

fix(rome_js_analyze): fix for unused flag on functional types on ts #3860

xunilrj commented Nov 25, 2022 •

edited

Loading

netlify bot commented Nov 25, 2022 •

edited

Loading

xunilrj commented Nov 25, 2022

github-actions bot commented Nov 25, 2022 •

edited

Loading

github-actions bot commented Nov 25, 2022

calibre-analytics bot commented Nov 28, 2022 •

edited

Loading

Browser previews

Most significant changes

MichaReiser Nov 29, 2022

xunilrj Nov 29, 2022

leops Dec 1, 2022

xunilrj Dec 2, 2022

xunilrj commented Dec 3, 2022

fix(rome_js_analyze): fix for unused flag on functional types on ts #3860

fix(rome_js_analyze): fix for unused flag on functional types on ts #3860

Conversation

xunilrj commented Nov 25, 2022 • edited Loading

Summary

Test Plan

netlify bot commented Nov 25, 2022 • edited Loading

✅ Deploy Preview for docs-rometools canceled.

xunilrj commented Nov 25, 2022

github-actions bot commented Nov 25, 2022 • edited Loading

Parser conformance results on ubuntu-latest

js/262

jsx/babel

symbols/microsoft

ts/babel

ts/microsoft

github-actions bot commented Nov 25, 2022

Analyzer Benchmark Results

calibre-analytics bot commented Nov 28, 2022 • edited Loading

1 page tested

Browser previews

Most significant changes

MichaReiser Nov 29, 2022

Choose a reason for hiding this comment

xunilrj Nov 29, 2022

Choose a reason for hiding this comment

leops Dec 1, 2022

Choose a reason for hiding this comment

xunilrj Dec 2, 2022

Choose a reason for hiding this comment

xunilrj commented Dec 3, 2022

xunilrj commented Nov 25, 2022 •

edited

Loading

netlify bot commented Nov 25, 2022 •

edited

Loading

github-actions bot commented Nov 25, 2022 •

edited

Loading

calibre-analytics bot commented Nov 28, 2022 •

edited

Loading