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

Use origin for string literal definitions #57729

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Andarist
Copy link
Contributor

⚠️ this is a proof-of-concept right now. It's not a complete implementation.

.origin information often allows the compiler to track back how a union has been created. It's used for improved type displays by quick info and such. It's also used by some optimizations. It occurred to me that it could be used in more places - such as go to definition

Shortcomings of this:

  • not every pattern is supported by .origin. IIRC, T[K] doesn't track this under any circumstances
  • keyof { 'anonymous': 'object' } is not tracked today. Since .origin was first introduced for type-displaying purposes it didn't make sense to track this. It would make sense to track it now as it would be quite crucial to support this in inference contexts.
  • keyof SinglePropObject is simplified to a string literal type as it's not a union so naturally .origin is not tracked in such a case. It's not impossible to track this but it would raise the memory footprint. Maybe this extra .origin tracking could only be done by the tsserver?
  • "complicated" intersections/unions (such as keyof Obj1 & keyof Obj2) don't always track this information so it won't always work. Some of that is likely fixable. A good enough solution, oriented on a specific subset of patterns could also be an option (as in - without touching how and how often .origin gets created today).
  • it's just about go to definition - other features such as reference finding are unaffected by this

closes #49033

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Mar 11, 2024
@sandersn sandersn requested review from weswigham and iisaduan March 20, 2024 15:52
@sandersn
Copy link
Member

I asked @iisaduan and @weswigham to review this but I'm not sure how ready it is. Should it still be a draft? Is it nearly complete?

@Andarist
Copy link
Contributor Author

Andarist commented Mar 20, 2024

I mention the status in the PR's description:

I asked @iisaduan and @weswigham to review this but I'm not sure how ready it is. Should it still be a draft? Is it nearly complete?

I'm ready to commit time to this to work but for now, I'm just testing out the waters. I want to confirm this approach has merit before writing more code here. I'm not sure what's the definition of a draft that you are using here. It's a draft because it's not ready to be merged but it's not a draft in a sense that I'd like somebody to give this a look ;p

@sandersn
Copy link
Member

Makes sense. @weswigham and @iisaduan can you give an opinion about whether this is going in the right direction?

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

I'm not sure we'd want to rely on union .origin being around to provide services (since any type that gets intern'd or dedupe'd for performance is liable to have an incorrect one or have it removed), but using it like this to provide a nicer, more connected result when it is present certainly seems OK.

@Andarist
Copy link
Contributor Author

It will lead to quite an inconsistent behavior if we don't try to improve some of the mentioned scenarios. Especially points 2 and 3 are worrying.

@sandersn
Copy link
Member

sandersn commented May 8, 2024

Double-checking: @weswigham this is good to go, right? Although since it's not a bug fix I guess we should wait for main to switch to 5.6 to merge it.

@Andarist
Copy link
Contributor Author

I think before landing this the team should comment on the potential inconsistent behaviors resulting from this. Users won't always understand why it works in certain situations while it won't in others. I think there is even potential in tracking more .origins when the checker is in the services mode to address most of those concerns. I'm not sure how the team would feel about it though so I refrained from expanding this implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Status: Needs merge
Development

Successfully merging this pull request may close these issues.

Go to definition for TypeScript literal strings
5 participants