Skip to content

Fix find-all-refs for import("mod").Foo where Foo is a typedef #23881

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
3 commits merged into from
May 4, 2018

Conversation

ghost
Copy link

@ghost ghost commented May 3, 2018

Fixes #23863

Needed to fix a few bugs:

  • Previously the import tracker was just ignoring ImportType nodes -- added a Debug.assertNever to prevent this from happening in the future.
  • We need to consider typedef tags as exports in the import tracker after Always export typedefs #23723
  • An ImportType should be considered a type reference.

Discovered #23879 while working on this.

@ghost ghost requested review from sandersn, weswigham and sheetalkamat May 3, 2018 22:54
@weswigham
Copy link
Member

An ImportType should be considered a type reference.

Specifically, an ImportType without typeof is effectively a type reference. One with typeof is effectively a type query.

@@ -179,6 +179,7 @@ namespace ts {

switch (node.parent.kind) {
case SyntaxKind.TypeReference:
case SyntaxKind.ImportType:
Copy link
Member

Choose a reason for hiding this comment

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

This should probably only be true if !(node.parent as ImportTypeNode).isTypeOf, otherwise it acts like a type query instead.

Copy link
Author

Choose a reason for hiding this comment

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

We do normally consider typeof uses to be references though, e.g.:

const [|x|] = 0;
const y: typeof [|x|] = 0;

Copy link
Member

Choose a reason for hiding this comment

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

Will the results still be classified correctly if that's picked up here and not where type queries are looked at?

Copy link
Author

Choose a reason for hiding this comment

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

There actually isn't a specific place in find-all-references where we look for type queries -- we basically just look for identifiers with the same name and check for a symbol match.

Copy link
Member

Choose a reason for hiding this comment

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

But this function is called from getMeaningFromLocation so that means we need to handle this irrespectively to avoid any wrong usage being seeping out.

break;

default:
Debug.assertNever(direct);
Copy link
Member

Choose a reason for hiding this comment

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

I'd include a helpful message here as the second parameter to assertNever, just in case (if the types drift out of sync with reality because of an invalid cast, for example).

@ghost ghost merged commit df9e262 into master May 4, 2018
@ghost ghost deleted the findAllRefsTypedef_importType branch May 4, 2018 20:32
@microsoft microsoft locked and limited conversation to collaborators Jul 31, 2018
This pull request was closed.
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.

In JS, typedef rename and find-all-refs doesn't work across files
2 participants