Fix completion details auto-imports crashes #52686
Merged
Conversation
This file contains 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
|
@typescript-bot user test tsserver |
|
Heya @andrewbranch, I've started to run the diff-based user code test suite (tsserver) on this PR at a1d36cd. You can monitor the build here. Update: The results are in! |
|
@andrewbranch Here are the results of running the user test suite comparing Something interesting changed - please have a look. Detailscreate-react-app
|
Actually, that shortcut is the other stack trace that shows up in the user tests. I suspect they’re different root causes, but maybe I’ll try to tackle that one too in this PR. |
DanielRosenwasser
approved these changes
Feb 8, 2023
…mpler altogether for modern clients
|
@typescript-bot user test tsserver |
|
Heya @andrewbranch, I've started to run the perf test suite on this PR at 605b0d9. You can monitor the build here. Update: The results are in! |
|
Heya @andrewbranch, I've started to run the diff-based user code test suite (tsserver) on this PR at 605b0d9. You can monitor the build here. Update: The results are in! |
|
@andrewbranch Here are the results of running the user test suite comparing Something interesting changed - please have a look. Detailsnpm
|
andrewbranch
changed the title
|
@andrewbranch Here they are:
CompilerComparison Report - main..52686
System
Hosts
Scenarios
TSServerComparison Report - main..52686
System
Hosts
Scenarios
StartupComparison Report - main..52686
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@typescript-bot user test tsserver |
|
Heya @andrewbranch, I've started to run the diff-based user code test suite (tsserver) on this PR at 6c85f52. You can monitor the build here. Update: The results are in! |
|
@andrewbranch Here are the results of running the user test suite comparing Something interesting changed - please have a look. Detailsnpm
|
|
Hm, my last commit fixed the npm repro I was able to get locally. Not sure what’s going on there now. |
|
I can’t repro those last two crashes locally, so I’m giving up on them for now. This fixes the large majority of the ones reported at #52660. |
andrewbranch
commented
Feb 10, 2023
| }); | ||
|
|
||
| verify.applyCodeActionFromCompletion("", { | ||
| }).andApplyCodeAction({ |
There was a problem hiding this comment.
This is a new fourslash API for calling completionEntryDetails with the exact data that was returned in the completionInfo response, as an editor would do. I was considering making sending data for auto-import completion details requests mandatory, breaking super old editors and about 40 old tests like this one, but ultimately decided against it.
jakebailey
approved these changes
Feb 10, 2023
| const getChecker = createGetChecker(program, host); | ||
| return getExportInfoMap(importingFile, host, program, preferences, cancellationToken) | ||
| .search(importingFile.path, preferCapitalized, name => name === symbolName, info => { | ||
| if (skipAlias(info[0].symbol, getChecker(info[0].isFromPackageJson)) === symbol) { | ||
| if (skipAlias(info[0].symbol, getChecker(info[0].isFromPackageJson)) === symbol && info.some(i => i.moduleSymbol === moduleSymbol || i.symbol.parent === moduleSymbol)) { |
There was a problem hiding this comment.
Can't recall but I think our own utility for checking if something is in an array is faster.
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 #52661
The ExportInfoMap is a multimap that contains information about every way that every export is re-exported from different places. Usually, a single symbol that gets re-exported multiple times has a single key pointing to an array of each place it’s exported from. When auto-imports are displayed in completions, we show one completion per key, and we simply pick the “best” re-exporting location out of that array. This lets us prefer short import paths over deep imports. (When and whether this is actually a good heuristic is beside the point.)
The exception is when a single symbol is exported by an ambient module. Ambient modules are considered a stronger signal that the name of the module is significant. For example, when
"node:fs"re-exports everything from"fs", we don’t want to be forced to pick which module is the “better” place to importreadFilefrom—we’d rather show a completion for both, even though the exported symbol is the same in both. So each of these get their own keys in the multimap.When
completionEntryDetailsis called and we have to re-find the export we showed in the previouscompletionInforequest, we never used to check the module symbol when searching the ExportInfoMap—instead, we asserted on it later, issuing the message seen in the telemetry bug. The fix is simply to check that the module symbol matches as part of our search instead of returning the first export info where just the export symbol itself matches.The fix sounds obvious, so why was this not more broken before? Well, when
completionEntryDetailsis called for an import from an ambient module, we were able to take shortcut that skips this search through the ExportInfoMap containing the faulty code. So the only way for the bug to manifest is when we request details for a symbol that a non-ambient module re-exports from an ambient module, and the iteration order mattered—it could have been correct by coincidence sometimes.