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

Already on GitHub? Sign in to your account

fix(typescript_indexer): improve handling of anonymous functions #5795

Merged
merged 7 commits into from Aug 21, 2023

Conversation

nbeloglazov
Copy link
Collaborator

This PR improves function that returns TS Symbol for Node. Previously it handled only named Nodes such as functions with names. Now we can get Symbols of anonymous functions as well. This fixes certain aspects of anonymous functions such as adds missing childof edges. And additionally it brings close handling of named and anonymous entities.

Unfortunately it relies on using private Node.symbol property. I considered alternative options such as:

  1. Support two different branches: named nodes with Symbol and anonymous nodes without Symbols. That would require re-implementing a SymbolVNameStore cache to deduplicate vnames for anonymous nodes. Keeping both in sync would be challenging and confusing.

  2. For anonymous nodes, where we can't get Symbols using public API, create our own Symbol instance. Symbol is public interface so we can create objects that implement it. Would also need to introduce Node=>FakeSymbol cache.

Both approaches seem more complicated. So I decided to use private Node.symbol API. If future TS versions remove it - we'll have to switch to one of the alternative approaches.

Copy link
Contributor

@shahms shahms left a comment

Choose a reason for hiding this comment

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

Just a grammar/spelling nit.

While I worry about depending on an unstable API, it's at least more isolated than the internal Java APIs used by the Java indexer.

kythe/typescript/indexer.ts Outdated Show resolved Hide resolved
@nbeloglazov nbeloglazov enabled auto-merge (squash) August 21, 2023 17:52
@nbeloglazov nbeloglazov merged commit 2652139 into kythe:master Aug 21, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants