Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(eslint-plugin): [no-use-before-define] correctly handle typeof type references #2623

Merged
merged 3 commits into from Oct 11, 2020

Conversation

@Drakota
Copy link
Contributor

@Drakota Drakota commented Oct 2, 2020

Fixes #2572

The no-use-before-define rule wasn't taking into account type reference that comes from values prefixed by the typeof operator.

Thank you @bradzacher for your help 馃檪

@typescript-eslint
Copy link

@typescript-eslint typescript-eslint bot commented Oct 2, 2020

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

@codecov codecov bot commented Oct 2, 2020

Codecov Report

Merging #2623 into master will increase coverage by 0.00%.
The diff coverage is 80.00%.

@@           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     
Flag Coverage 螖
#unittest 92.83% <80.00%> (+<0.01%) 猬嗭笍

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage 螖
...es/eslint-plugin/src/rules/no-use-before-define.ts 92.50% <80.00%> (-1.87%) 猬囷笍
...es/eslint-plugin/src/rules/no-duplicate-imports.ts 97.77% <0.00%> (酶)
@bradzacher bradzacher added the bug label Oct 2, 2020
Copy link
Member

@bradzacher bradzacher left a comment

You're most of the way there!
A few comments.
Thanks for working on this.

function isTypeReference(reference: TSESLint.Scope.Reference): boolean {
return (
reference.isTypeReference ||
reference.identifier.parent?.parent?.type === AST_NODE_TYPES.TSTypeQuery

This comment has been minimized.

@bradzacher

bradzacher Oct 2, 2020
Member

almost! but this will only match one case.

We need to match all of these cases, and more:

  • typeof a
  • typeof a.b
  • typeof 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.

export interface TSTypeQuery extends BaseNode {
type: AST_NODE_TYPES.TSTypeQuery;
exprName: EntityName;
}

Based on the defined types for TSQualifiedName, we know the left will always be an Identifier or a TSQualifiedName.

export interface TSQualifiedName extends BaseNode {
type: AST_NODE_TYPES.TSQualifiedName;
left: EntityName;
right: Identifier;
}

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

This comment has been minimized.

@Drakota

Drakota Oct 4, 2020
Author Contributor

Fixed!

if (node.type === AST_NODE_TYPES.TSTypeQuery) {
return true;
} else if (!node.parent) {
return false;
}

return referenceContainsTypeQuery(node.parent);
Comment on lines 99 to 105

This comment has been minimized.

@bradzacher

bradzacher Oct 5, 2020
Member

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;
}

This comment has been minimized.

@Drakota

Drakota Oct 10, 2020
Author Contributor

I see! I wasn't sure of what you meant in your first comment.
I've changed it.

Copy link
Member

@bradzacher bradzacher left a comment

LGTM - thanks for your contribution!

@bradzacher bradzacher changed the title fix(eslint-plugin): Added typeof value type reference check to no-use-before-define rule fix(eslint-plugin): [no-use-before-define] correctly handle typeof type references Oct 11, 2020
@bradzacher bradzacher merged commit 8e44c78 into typescript-eslint:master Oct 11, 2020
9 of 10 checks passed
9 of 10 checks passed
Typecheck
Details
Unit tests Unit tests
Details
Code style and lint
Details
Run integration tests on primary Node.js version
Details
Run unit tests on other Node.js versions (10.x)
Details
Run unit tests on other Node.js versions (14.x)
Details
Publish the latest code as a canary version
Details
codecov/patch 80.00% of diff hit (target 90.00%)
Details
Semantic Pull Request ready to be squashed
Details
codecov/project 92.83% (+0.00%) compared to d7dc108
Details
@Drakota Drakota deleted the Drakota:fix/2572 branch Oct 12, 2020
@Drakota
Copy link
Contributor Author

@Drakota Drakota commented Oct 12, 2020

Thank you for guiding me through this @bradzacher, I appreciate it 馃檪

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can鈥檛 perform that action at this time.