Skip to content

Misc. improvements to addImplementationReferences#23507

Merged
4 commits merged into
masterfrom
addImplementationReferences
Apr 18, 2018
Merged

Misc. improvements to addImplementationReferences#23507
4 commits merged into
masterfrom
addImplementationReferences

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Apr 18, 2018

  • getContainingClassIfInHeritageClause: An ExpressionWithTypeArguments can only appear in a HeritageClause, and every node must have a parent if it's not a SourceFile, so remove some unnecessary checks.
  • getContainingTypeReference: Previously this would climb all the way to the root and keep track of the last thing that was a type. We only need to climb to the first thing that's not in a type. This is a one-liner using findAncestor.
  • Add an addIfImplementation helper since all uses of isImplementationExpression were immediately followed by addReference.

@ghost ghost requested review from armanio123 and sheetalkamat April 18, 2018 15:44
@ghost ghost force-pushed the addImplementationReferences branch from b9b21d7 to c3e7350 Compare April 18, 2018 17:48
const containingTypeReference = getContainingTypeReference(refNode);
if (containingTypeReference && state.markSeenContainingTypeReference(containingTypeReference)) {
const parent = containingTypeReference.parent;
if (hasType(parent) && parent.type === containingTypeReference && hasInitializer(parent) && isImplementationExpression(parent.initializer)) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Dont you still need some kind of equivalent for parent.type === containingTypeReference otherwise doing this on anything eg node is initializer but parent has type would pass through this? (May be its some form of assert?)

Comment thread src/services/findAllReferences.ts Outdated
if (hasType(parent) && parent.type === containingTypeReference && hasInitializer(parent) && isImplementationExpression(parent.initializer)) {
addReference(parent.initializer);
// Find the first node whose parent isn't a type node -- i.e., the highest type node.
const typeNode = findAncestor(refNode.parent, a => !isQualifiedName(a.parent) && !isTypeNode(a.parent) && !isTypeElement(a.parent));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

`It should be findAncestor(refNode since you are looking at parents now

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

👍

@ghost ghost merged commit 2f6b59e into master Apr 18, 2018
@ghost ghost deleted the addImplementationReferences branch April 18, 2018 22:24
@microsoft microsoft locked and limited conversation to collaborators Jul 30, 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.

1 participant