Skip to content
This repository was archived by the owner on Aug 31, 2023. It is now read-only.

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

Merged
merged 4 commits into from
Dec 7, 2022

Conversation

xunilrj
Copy link
Contributor

@xunilrj xunilrj commented Dec 3, 2022

Summary

This PR closes #3727.
The main issue is that before we only considered const as stable if the initializer is constant. The specific example was calling a function, which we assume is never constant.

Now, all const declarator outside of component are considered as stable.

I also did a small refactor of the code using the new Binding and .declarator().

This is done on top of #3860 and will wait for it before rebasing.

Test Plan

> cargo test -p rome_js_analyze -- use_exhaustive_dependencies

@xunilrj xunilrj requested review from leops, ematipico and a team as code owners December 3, 2022 13:06
@netlify
Copy link

netlify bot commented Dec 3, 2022

Deploy Preview for docs-rometools ready!

Name Link
🔨 Latest commit 2a1f4d5
🔍 Latest deploy log https://app.netlify.com/sites/docs-rometools/deploys/638e7f331d84c600098f7271
😎 Deploy Preview https://deploy-preview-3926--docs-rometools.netlify.app
📱 Preview on mobile
Toggle QR Code...

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
Copy link

github-actions bot commented Dec 3, 2022

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 1684 ❌ ⏬ -73
Failed 4189 4262 ❌ ⏫ +73
Panics 0 0 0
Coverage 29.55% 28.32% -1.23%
🔥 Regression (73):
aliasInaccessibleModule.symbols
aliasInaccessibleModule2.symbols
asOperator3.symbols
asOperatorASI.symbols
blockScopedBindingsReassignedInLoop1.symbols
capturedLetConstInLoop1.symbols
capturedLetConstInLoop3.symbols
capturedLetConstInLoop3_ES6.symbols
collisionCodeGenModuleWithConstructorChildren.symbols
collisionCodeGenModuleWithFunctionChildren.symbols
collisionCodeGenModuleWithModuleChildren.symbols
commentOnAmbientfunction.symbols
conditionalTypeBasedContextualTypeReturnTypeWidening.symbols
conditionallyDuplicateOverloadsCausedByOverloadResolution.symbols
constraintSatisfactionWithAny2.symbols
contextualSigInstantiationRestParams.symbols
contextualSignatureInstantiation4.symbols
contextualTypingFunctionReturningFunction2.symbols
contravariantTypeAliasInference.symbols
controlFlowCommaExpressionAssertionWithinTernary.symbols
controlFlowTruthiness.symbols
declFileTypeofModule.symbols
declarationEmitImportInExportAssignmentModule.symbols
downlevelLetConst17.symbols
duplicateVarAndImport.symbols
es6ImportEqualsDeclaration2.symbols
exportAssignedNamespaceIsVisibleInDeclarationEmit.symbols
exportAssignmentError.symbols
exportDefaultForNonInstantiatedModule.symbols
exportEqualsOfModule.symbols
externFunc.symbols
fallFromLastCase1.symbols
fallbackToBindingPatternForTypeInference.symbols
freshLiteralTypesInIntersections.symbols
functionExpressionContextualTyping3.symbols
importCallExpressionDeclarationEmit1.symbols
importedModuleClassNameClash.symbols
inferentialTypingWithFunctionType.symbols
inferentiallyTypingAnEmptyArray.symbols
keyofModuleObjectHasCorrectKeys.symbols
letConstMatchingParameterNames.symbols
mergedModuleDeclarationCodeGen.symbols
mergedModuleDeclarationWithSharedExportedVar.symbols
moduleWithTryStatement1.symbols
nameCollisionWithBlockScopedVariable1.symbols
noImplicitReturnsWithProtectedBlocks1.symbols
nullishCoalescingOperator10.symbols
overloadsAndTypeArgumentArity.symbols
partiallyAnnotatedFunctionWitoutTypeParameter.symbols
privacyCheckTypeOfInvisibleModuleError.symbols
privacyCheckTypeOfInvisibleModuleNoError.symbols
propagateNonInferrableType.symbols
reachabilityCheckWithEmptyDefault.symbols
recursiveMods.symbols
reservedNameOnModuleImport.symbols
reuseInnerModuleMember.symbols
selfReference.symbols
shebangBeforeReferences.symbols
strictNullChecksNoWidening.symbols
stringLiteralTypesAndParenthesizedExpressions01.symbols
stringLiteralTypesOverloads04.symbols
structuralTypeInDeclareFileForModule.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%

@xunilrj xunilrj force-pushed the fix/3727-use-exhaustive-deps branch from 495b611 to df51b51 Compare December 5, 2022 22:19
Copy link
Contributor

@ematipico ematipico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Are the regressions in the parser compliance action expected?

@xunilrj
Copy link
Contributor Author

xunilrj commented Dec 7, 2022

Looks good to me. Are the regressions in the parser compliance action expected?

No, but I will revisit this test suite. And see what we can improve there.

@xunilrj xunilrj merged commit d3f2cdc into main Dec 7, 2022
@xunilrj xunilrj deleted the fix/3727-use-exhaustive-deps branch December 7, 2022 19:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🐛 useExhaustiveDependencies incorrectly flags external constants
3 participants