GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — fix(44179): disentangle the meaning of `SymbolLinks.target` by lowr · Pull Request #47098 · microsoft/TypeScript

Conversation

@lowr

@lowr LowR (lowr) commented Dec 10, 2021

Copy link
Copy Markdown
Contributor

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 target of newly created Symbol. I ran all tests but couldn't find any code in which alias Symbol is substituted to target like the issue, so I left them intact.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@typescript-bot TypeScript Bot (typescript-bot) added the For Backlog Bug PRs that fix a backlog bug label Dec 10, 2021
@sandersn
Nathan Shively-Sanders (sandersn) requested review from Gabriela Araujo Britto (gabritto) and Wesley Wigham (weswigham) January 31, 2022 23:08
@sandersn Nathan Shively-Sanders (sandersn) assigned Wesley Wigham (weswigham) Jan 31, 2022
weswigham
Wesley Wigham (weswigham) requested changes Feb 7, 2022
View reviewed changes
Comment thread src/compiler/checker.ts Outdated
result.declarations = symbol.declarations;
result.parent = symbol.parent;
result.target = symbol;
result.target = resolveSymbol(symbol);

@weswigham Wesley Wigham (weswigham) Feb 7, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@lowr LowR (lowr) Feb 9, 2022

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@weswigham Wesley Wigham (weswigham) Feb 9, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@lowr LowR (lowr) Feb 10, 2022

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gabritto
Gabriela Araujo Britto (gabritto) removed their request for review February 9, 2022 20:34
@sandersn

Nathan Shively-Sanders (sandersn) commented Mar 31, 2022

Copy link
Copy Markdown
Member

Do you want to keep working on this? Otherwise I'd like to close it to reduce the number of open PRs.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lowr

LowR (lowr) commented Apr 1, 2022

Copy link
Copy Markdown
Contributor Author

Sorry for the huge delay. I'll work on this over the weekend.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lowr
disentangle the meaning of SymbolLinks.target
4aa6220
@lowr
LowR (lowr) force-pushed the fix/44179 branch from f80d310 to 4aa6220 Compare April 3, 2022 12:14
@lowr

LowR (lowr) commented Apr 7, 2022

Copy link
Copy Markdown
Contributor Author

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?

tryGetAliasTarget() was the sole reason some fourslash tests were failing with your patch. It seems its purpose is to retrieve root .target of symbols rather than .aliasTarget despite its name. I renamed it to avoid further confusion.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@lowr
LowR (lowr) requested a review from Wesley Wigham (weswigham) April 7, 2022 18:04
weswigham
Wesley Wigham (weswigham) approved these changes Apr 7, 2022
View reviewed changes

@weswigham Wesley Wigham (weswigham) left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@weswigham
Wesley Wigham (weswigham) requested review from Andrew Branch (andrewbranch) and Nathan Shively-Sanders (sandersn) April 7, 2022 18:13
@lowr LowR (lowr) changed the title fix(44179): resolve target symbol before substituting fix(44179): disentangle the meaning of SymbolLinks.target Apr 7, 2022
andrewbranch
Andrew Branch (andrewbranch) approved these changes Apr 7, 2022
View reviewed changes

@andrewbranch Andrew Branch (andrewbranch) left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ah, this is great. The overloading of this property definitely caused me confusion before I learned to expect it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
andrewbranch
Andrew Branch (andrewbranch) requested changes Apr 7, 2022
View reviewed changes
Comment thread src/compiler/checker.ts
return undefined;
}
function tryGetAliasTarget(symbol: Symbol): Symbol | undefined {
function tryGetTarget(symbol: Symbol): Symbol | undefined {

@andrewbranch Andrew Branch (andrewbranch) Apr 7, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Seems like this function didn't need to be renamed?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@weswigham Wesley Wigham (weswigham) Apr 7, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It was renamed because it handles .target rather than .aliasTarget lookups, for clarity.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@andrewbranch Andrew Branch (andrewbranch) Apr 7, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@weswigham
Wesley Wigham (weswigham) merged commit b45cf0d into microsoft:main Apr 7, 2022
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@weswigham Wesley Wigham (weswigham) weswigham approved these changes
@andrewbranch Andrew Branch (andrewbranch) andrewbranch requested changes
@sandersn Nathan Shively-Sanders (sandersn) Awaiting requested review from sandersn

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@weswigham Wesley Wigham (weswigham)

Labels

For Backlog Bug PRs that fix a backlog bug

Projects

PR Backlog
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

The inferred return type of a function with a generic that spreads an import * as doesn't include some exports when they're exported seperately to the declaration

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@lowr @sandersn @weswigham @andrewbranch @typescript-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.