Set hasAddedOrRemovedSymlinks when discovering an existing file by its link #46569
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
typescript-bot
added
Author: Team
For Milestone Bug
andrewbranch
commented
Oct 29, 2021
Comment on lines
+3
to
+5
| // Notable lack of package.json referencing `mylib` as dependency here. | ||
| // This is not accurate to the original repro, but I think that's a separate | ||
| // bug, which, if fixed, would prevent the later crash. |
There was a problem hiding this comment.
Need to follow up on this separately
There was a problem hiding this comment.
Maybe link to the repro? Or provide an issue #.
DanielRosenwasser
approved these changes
Oct 29, 2021
amcasey
approved these changes
Oct 29, 2021
mprobst
pushed a commit
to mprobst/TypeScript
that referenced
this pull request
Jan 10, 2022
…s link (microsoft#46569) * Set hasAddedOrRemovedSymlinks when discovering an existing file by its link * Make it optional
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #46564
Symlinks are such a mess... I’m pretty sure
resolutionCache.tsis not the right place for this, but it was direct and easy... considered doing this in Program’stryReuseStructureFromOldProgramorresolveModuleNamesReusingOldState, but would have to do an extra iteration through all its newly resolved modules (it does them in a batch, and iterates through them only enough to see if there are any changes—I would at least have to continue that iteration until seeing a resolution with a symlink change or through the end). I’m open to that approach, though I really hope there’s an even better place for this I’m not seeing.What’s happening is actually fairly simple. We currently clear the symlink and module specifier caches in a Project during
updateGraphWorkerif we’ve added or removed any ScriptInfo for a symlinked file. However, this doesn’t cover the case where we discover a symlink that links back to a file that already has a ScriptInfo (i.e., we resolve an existing file at a new, symlinked path). The test case first references/packages/mylib/index.tsby a relative import to its realpath, and builds a module specifier cache with that information. Then, an edit swaps../packages/mylibformylib, which resolves to a symlink pointing at the same file. The resolution succeeds, but we fail to notice that this change has updated our knowledge of existing symlinks, which should always make us recompute those caches. My fix simply sets the flag on Project saying we need to clear those caches when the ModuleResolutionCache makes a new resolution to a symlink.