Repository navigation
fix(44179): disentangle the meaning of SymbolLinks.target - #47098
Conversation
TypeScript Bot (typescript-bot)
added
the
For Backlog Bug
| result.declarations = symbol.declarations; | ||
| result.parent = symbol.parent; | ||
| result.target = symbol; | ||
| result.target = resolveSymbol(symbol); |
There was a problem hiding this comment.
instantiateSymbol should absolutely not be eagerly resolving symbols like this, much as it may fix an issue like this, it's liable to cause other (performance) issues elsewhere. Instead, the code doing the spread copying needs to be resolving the symbols it's copying the members of, otherwise it can't guarantee they're all present.
There was a problem hiding this comment.
It seems the code doing the spread copying is already resolving the symbols it's copying the members of, but not the members themselves, which may be aliases and set to target, causing the problem. Resolving every member of a symbol upon checking the spread copying isn't what we want either, right?
I do understand now that instantiateSymbol shouldn't be resolving those symbols. How about not setting result.target when symbol is an alias? This fixes the issue in proper way, I believe, in that it also defers the resolution of member symbols maximally.
There was a problem hiding this comment.
Hmm, I think the fundamental issue is that symbol instantiation and alias resolution shouldn't both be using the same field (for referring to the original symbol and for caching the alias resolution result, respectively). Maybe there was a time when it was safe because instantiated (or copied) alias symbols couldn't exist, but that time is certainly not now. IMO, we should swap the usage in resolveAlias to use a new member (aliasTarget?) to cache results, rather than .target, and see if we can fix up what breaks and disentangle the two meanings of a .target. Something like https://gist.github.com/weswigham/22acfad2595f1c1eab3d357385907e90 looks like a promising start to that, though some more changes may be needed to fixup some assumptions in the language service.
There was a problem hiding this comment.
Disentangling the meanings of target sounds really nice and would make the code cleaner! Thanks for the patch, I've applied it and saw some fourslash tests failing as you anticipated. Let me fix things up in the coming days.
|
Do you want to keep working on this? Otherwise I'd like to close it to reduce the number of open PRs. |
|
Sorry for the huge delay. I'll work on this over the weekend. |
|
Wesley Wigham (@weswigham) I'm sorry I couldn't update this PR earlier. I've updated it based on your suggestion. Would you review it again?
|
Wesley Wigham (weswigham)
left a comment
Wesley Wigham (weswigham)
left a comment
There was a problem hiding this comment.
Nice, thanks for helping wrap this up.
Nathan Shively-Sanders (@sandersn) and Andrew Branch (@andrewbranch) you wanna be extra eyes on this for a merge, since I helped author it?
LowR (lowr)
changed the title
SymbolLinks.target
Andrew Branch (andrewbranch)
left a comment
Andrew Branch (andrewbranch)
left a comment
There was a problem hiding this comment.
Ah, this is great. The overloading of this property definitely caused me confusion before I learned to expect it.
| return undefined; | ||
| } | ||
| function tryGetAliasTarget(symbol: Symbol): Symbol | undefined { | ||
| function tryGetTarget(symbol: Symbol): Symbol | undefined { |
There was a problem hiding this comment.
Seems like this function didn't need to be renamed?
There was a problem hiding this comment.
It was renamed because it handles .target rather than .aliasTarget lookups, for clarity.
There was a problem hiding this comment.
Ack, my comment was misguided because of GitHub's silent truncation of the diff. It looked like this function was handling .aliasTarget still. All good.
Fixes #44179
FWIW, there are several other places in checker (L.21013, L.27167, L.27391 as of 48228b8) where "unresolved" Symbol is set to
targetof newly created Symbol. I ran all tests but couldn't find any code in which alias Symbol is substituted totargetlike the issue, so I left them intact.