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

Emit declarations using alias chains #56100

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Andarist
Copy link
Contributor

@Andarist Andarist commented Oct 13, 2023

fixes #57415

@Andarist Andarist force-pushed the resolve-alias-chains-in-containers branch from 1f223aa to a9f6ea6 Compare October 13, 2023 20:50
@sandersn sandersn added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Nov 1, 2023
@weswigham
Copy link
Member

Obligatory: Is there a linked issue?

AFAIK this is sensible, but probably expensive - getAliasForSymbolInContainer is already costly, since it's an exhaustive traversal of all symbols in scope looking for aliases that refer to the target symbol - making it recursive is going to increase the worst case cost exponentially without some very smart caching. And, perhaps moreover, getAliasForSymbolInContainer is basically just a duplicated tiny slice of the inner loop of getAccessibleSymbolChain without all the recursion, safety, or caching rigamarole, to try and prevent the need for the (still expensive) getAccessibleSymbolChain call. By making it return a chain like this, isn't it basically just a copy of getAccessibleSymbolChain that preferences aliases, without all the aforementioned safety valves? Maybe it'd just be better to modify the useExternalAliasingOnly parameter into an alias-preference-flags parameter so getAccessibleSymbolChain is suitable for use here directly?

I say that, but I'm also actively working on a replacement for getAccessibleSymbolChain itself that's a bit less.... bad. Exhaustive scope traversal on lookup is not a good thing (there are many scopes with many symbols), and I'd like to remove it; while we don't have the perf numbers for it, getAccessibleSymbolChain has basically organically grown in function since TS 1.8 or earlier, making it the oldest part of the declaration emit pipeline that's never seen a major refactor, and its' implementation is pretty unoptimized (but with 5+ years of edge cases written into it, hgggg) - it's come up as a hotspot in specific examples multiple times in the past, but we've always worked around the issue by adding limiters higher up in the declaration emit or checking process.

I'd also say "we want to check the perf on this, at least", but declaration emit performance is still a bit of a blind spot in our perf suite. It's usually only lightly exercised by the error messages generated by the perf tests. 😭

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.

Yeah, definitely see if this can be integrated into the normal getAccessibleSymbolChain function. It has some inefficiencies that I'm looking into fixing, but so does this solution as-written, so better to keep them in one place.

@fubhy
Copy link

fubhy commented Mar 20, 2024

Also linked (atm. unclear if this fixes it though): #55021

@typescript-bot typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Mar 20, 2024
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: Waiting on author
Development

Successfully merging this pull request may close these issues.

[TS 5.4.0-beta] .d.ts emit adding an inline type import instead of utilising existing import
6 participants