fix(eslint-plugin): [no-use-before-define] correctly handle typeof type references #2623
Conversation
|
Thanks for the PR, @Drakota! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
Codecov Report
@@ Coverage Diff @@
## master #2623 +/- ##
=======================================
Coverage 92.82% 92.83%
=======================================
Files 293 294 +1
Lines 9619 9673 +54
Branches 2697 2714 +17
=======================================
+ Hits 8929 8980 +51
- Misses 326 327 +1
- Partials 364 366 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| function isTypeReference(reference: TSESLint.Scope.Reference): boolean { | ||
| return ( | ||
| reference.isTypeReference || | ||
| reference.identifier.parent?.parent?.type === AST_NODE_TYPES.TSTypeQuery |
There was a problem hiding this comment.
almost! but this will only match one case.
We need to match all of these cases, and more:
typeof atypeof a.btypeof a.b.c
Instead an easier way to do this would be with a loop.
Based on the defined types for TSTypeQuery, we know that the exprName will either be an Identifier or a TSQualifiedName.
typescript-eslint/packages/types/src/ts-estree.ts
Lines 1669 to 1672 in d7dc108
Based on the defined types for TSQualifiedName, we know the left will always be an Identifier or a TSQualifiedName.
typescript-eslint/packages/types/src/ts-estree.ts
Lines 1572 to 1576 in d7dc108
This means we can simply loop and look at the parents
The reason I gave you the above is it'll help you constrain the loop so it won't iterate forever
packages/eslint-plugin/tests/rules/no-use-before-define.test.ts
Outdated
Show resolved
Hide resolved
| if (node.type === AST_NODE_TYPES.TSTypeQuery) { | ||
| return true; | ||
| } else if (!node.parent) { | ||
| return false; | ||
| } | ||
|
|
||
| return referenceContainsTypeQuery(node.parent); |
There was a problem hiding this comment.
Almost there!
We can constrain this a little more so we don't recursively traverse up the whole tree unnecessarily.
We know that the node must always be either be a TSQualifiedName, Identifier or TSTypeQuery.
switch (node.type) {
case AST_NODE_TYPES.TSTypeQuery:
return true;
case AST_NODE_TYPES.TSQualifiedName:
case AST_NODE_TYPES.Identifier:
return referenceContainsTypeQuery(node.parent);
default:
// if we find a different node, there's no chance that we're in a TSTypeQuery
return false;
}There was a problem hiding this comment.
I see! I wasn't sure of what you meant in your first comment.
I've changed it.
|
Thank you for guiding me through this @bradzacher, I appreciate it 🙂 |
Fixes #2572
The
no-use-before-definerule wasn't taking into account type reference that comes from values prefixed by thetypeofoperator.Thank you @bradzacher for your help 🙂