Store symbol table map key in CachedSymbolExportInfo #45289
Conversation
| ? checker.resolveExternalModuleSymbol(moduleSymbol) | ||
| : checker.tryGetMemberInModuleExportsAndProperties(symbolName, moduleSymbol)); | ||
| : checker.tryGetMemberInModuleExportsAndProperties(unescapeLeadingUnderscores(info.symbolTableKey), moduleSymbol), | ||
| `Could not find symbol '${info.symbolName}' by key '${info.symbolTableKey}' in module ${moduleSymbol.name}`); |
There was a problem hiding this comment.
Double checking with @amcasey that this is ok to put in an error message. info.symbolName may be a variable name written by a user and moduleSymbol.name may be most of a file path. This info will help discover a repro if a user sees it in their TS Server log and can share it.
There was a problem hiding this comment.
Obviously, the less customer content, the better but, as we've discussed, we have reason to believe this will be broadly useful and the telemetry for the resulting exception will be sanitized appropriately.
| const type = getTypeOfSymbol(exportEquals); | ||
| if (shouldTreatPropertiesOfExternalModuleAsExports(type)) { | ||
| getPropertiesOfType(type).forEach(symbol => { | ||
| cb(symbol, symbol.escapedName); |
There was a problem hiding this comment.
I would have had to refactor or duplicate a lot of checker code to get the actual map key of symbol.members here, so I’m hoping that the map key for a property is always symbol.escapedName, which is not always true for an export. I added a debug assertion here while running tests and it never triggered, but I don’t want to leave it in production code because this is an extremely hot path (can easily hit tens of thousands of times while building a large export info map).
There was a problem hiding this comment.
What's the penalty for providing the wrong key in this PR?
There was a problem hiding this comment.
It will crash in rehydrateCachedInfo, for the same reason it does before this PR—no change. The cases I’ve identified where symbol.escapedName was not the key were all in exports, not members, which definitely gets the real map key.
|
@typescript-bot pack this |
|
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at e640207. You can monitor the build here. |
|
@typescript-bot pack this |
|
Heya @andrewbranch, I've started to run the tarball bundle task on this PR at 55974d6. You can monitor the build here. |
|
Hey @andrewbranch, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
Confirmed this fixes #45200 |
andrewbranch
added
Author: Team
For Milestone Bug
sandersn
left a comment
sandersn
left a comment
There was a problem hiding this comment.
As far as I understand auto imports/module exports, this seems like an improvement. I have a question about what happens if the key isn't symbol.escapedName.
| return startsWith(symbol.escapedName as string, "__@"); | ||
| } | ||
|
|
||
| export function isPrivateIdentifierSymbol(symbol: Symbol): boolean { |
There was a problem hiding this comment.
hmm. I had to figure this out recently and I thought the Right Way was symbol.valueDeclaration?.kind === SyntaxKind.PrivateIdentifier or something similar. But I guess this way works too.
There was a problem hiding this comment.
I would readily believe, or at least hope, that those are equivalent, but this implementation gets to the core of what went wrong in the linked crash. You cannot ask for a name starting with __# in checker.tryGetInModuleExportsAndProperties and expect to get the private identifier symbol it came from back out, because the checker will underscore-escape that string and try to find a property whose actual name starts with __#. So it’s very important that we don’t store any symbols with escapedNames like this, somewhat regardless of what it represents.
| const type = getTypeOfSymbol(exportEquals); | ||
| if (shouldTreatPropertiesOfExternalModuleAsExports(type)) { | ||
| getPropertiesOfType(type).forEach(symbol => { | ||
| cb(symbol, symbol.escapedName); |
There was a problem hiding this comment.
What's the penalty for providing the wrong key in this PR?
4 participants
Fixes #45101, fixes #45200.