Skip to content

Store symbol table map key in CachedSymbolExportInfo #45289

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 4, 2021
Merged

Store symbol table map key in CachedSymbolExportInfo #45289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7aa08f6
Store symbol table map key in CachedSymbolExportInfo
andrewbranch Aug 2, 2021
e640207
Remove debug assertion
andrewbranch Aug 2, 2021
55974d6
Filter out known symbols (again) and private identifiers
andrewbranch Aug 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,6 +580,7 @@ namespace ts {
getEmitResolver,
getExportsOfModule: getExportsOfModuleAsArray,
getExportsAndPropertiesOfModule,
forEachExportAndPropertyOfModule,
getSymbolWalker: createGetSymbolWalker(
getRestTypeOfSignature,
getTypePredicateOfSignature,
Expand Down Expand Up @@ -3530,6 +3531,24 @@ namespace ts {
return exports;
}

function forEachExportAndPropertyOfModule(moduleSymbol: Symbol, cb: (symbol: Symbol, key: __String) => void): void {
const exports = getExportsOfModule(moduleSymbol);
exports.forEach((symbol, key) => {
if (!isReservedMemberName(key)) {
cb(symbol, key);
}
});
const exportEquals = resolveExternalModuleSymbol(moduleSymbol);
if (exportEquals !== moduleSymbol) {
const type = getTypeOfSymbol(exportEquals);
if (shouldTreatPropertiesOfExternalModuleAsExports(type)) {
getPropertiesOfType(type).forEach(symbol => {
cb(symbol, symbol.escapedName);
Copy link
Member Author

@andrewbranch andrewbranch Aug 2, 2021

Choose a reason for hiding this comment

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

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).

Copy link
Member

@sandersn sandersn Aug 4, 2021

Choose a reason for hiding this comment

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

What's the penalty for providing the wrong key in this PR?

Copy link
Member Author

@andrewbranch andrewbranch Aug 4, 2021

Choose a reason for hiding this comment

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

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.

});
}
}
}

function tryGetMemberInModuleExports(memberName: __String, moduleSymbol: Symbol): Symbol | undefined {
const symbolTable = getExportsOfModule(moduleSymbol);
if (symbolTable) {
Expand Down
1 change: 1 addition & 0 deletions src/compiler/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4223,6 +4223,7 @@ namespace ts {
getExportsOfModule(moduleSymbol: Symbol): Symbol[];
/** Unlike `getExportsOfModule`, this includes properties of an `export =` value. */
/* @internal */ getExportsAndPropertiesOfModule(moduleSymbol: Symbol): Symbol[];
/* @internal */ forEachExportAndPropertyOfModule(moduleSymbol: Symbol, cb: (symbol: Symbol, key: __String) => void): void;
getJsxIntrinsicTagNamesAt(location: Node): Symbol[];
isOptionalParameter(node: ParameterDeclaration): boolean;
getAmbientModules(): Symbol[];
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3289,6 +3289,10 @@ namespace ts {
return startsWith(symbol.escapedName as string, "__@");
}

export function isPrivateIdentifierSymbol(symbol: Symbol): boolean {
Copy link
Member

@sandersn sandersn Aug 4, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@andrewbranch andrewbranch Aug 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

return startsWith(symbol.escapedName as string, "__#");
}

/**
* Includes the word "Symbol" with unicode escapes
*/
Expand Down
1 change: 1 addition & 0 deletions src/services/completions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1930,6 +1930,7 @@ namespace ts.Completions {
!!importCompletionNode,
context => {
exportInfo.forEach(sourceFile.path, (info, symbolName, isFromAmbientModule) => {
if (!isIdentifierText(symbolName, getEmitScriptTarget(host.getCompilationSettings()))) return;
if (!detailsEntryId && isStringANonContextualKeyword(symbolName)) return;
// `targetFlags` should be the same for each `info`
if (!isTypeOnlyLocation && !importCompletionNode && !(info[0].targetFlags & SymbolFlags.Value)) return;
Expand Down
34 changes: 23 additions & 11 deletions src/services/exportInfoMap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ namespace ts {
// Used to rehydrate `symbol` and `moduleSymbol` when transient
id: number;
symbolName: string;
symbolTableKey: __String;
moduleName: string;
moduleFile: SourceFile | undefined;

Expand All @@ -44,7 +45,7 @@ namespace ts {
export interface ExportInfoMap {
isUsableByFile(importingFile: Path): boolean;
clear(): void;
add(importingFile: Path, symbol: Symbol, moduleSymbol: Symbol, moduleFile: SourceFile | undefined, exportKind: ExportKind, isFromPackageJson: boolean, scriptTarget: ScriptTarget, checker: TypeChecker): void;
add(importingFile: Path, symbol: Symbol, key: __String, moduleSymbol: Symbol, moduleFile: SourceFile | undefined, exportKind: ExportKind, isFromPackageJson: boolean, scriptTarget: ScriptTarget, checker: TypeChecker): void;
get(importingFile: Path, importedName: string, symbol: Symbol, moduleName: string, checker: TypeChecker): readonly SymbolExportInfo[] | undefined;
forEach(importingFile: Path, action: (info: readonly SymbolExportInfo[], name: string, isFromAmbientModule: boolean) => void): void;
releaseSymbols(): void;
Expand All @@ -71,13 +72,19 @@ namespace ts {
symbols.clear();
usableByFileName = undefined;
},
add: (importingFile, symbol, moduleSymbol, moduleFile, exportKind, isFromPackageJson, scriptTarget, checker) => {
add: (importingFile, symbol, symbolTableKey, moduleSymbol, moduleFile, exportKind, isFromPackageJson, scriptTarget, checker) => {
if (importingFile !== usableByFileName) {
cache.clear();
usableByFileName = importingFile;
}
const isDefault = exportKind === ExportKind.Default;
const importedName = getNameForExportedSymbol(isDefault && getLocalSymbolForExportDefault(symbol) || symbol, scriptTarget);
const namedSymbol = isDefault && getLocalSymbolForExportDefault(symbol) || symbol;
// A re-export merged with an export from a module augmentation can result in `symbol`
// being an external module symbol; the name it is re-exported by will be `symbolTableKey`
// (which comes from the keys of `moduleSymbol.exports`.)
const importedName = isExternalModuleSymbol(namedSymbol)
? unescapeLeadingUnderscores(symbolTableKey)
: getNameForExportedSymbol(namedSymbol, scriptTarget);
const moduleName = stripQuotes(moduleSymbol.name);
const id = exportInfoId++;
const storedSymbol = symbol.flags & SymbolFlags.Transient ? undefined : symbol;
Expand All @@ -86,6 +93,7 @@ namespace ts {

exportInfo.add(key(importedName, symbol, moduleName, checker), {
id,
symbolTableKey,
symbolName: importedName,
moduleName,
moduleFile,
Expand Down Expand Up @@ -160,12 +168,10 @@ namespace ts {
const moduleSymbol = info.moduleSymbol || cachedModuleSymbol || Debug.checkDefined(info.moduleFile
? checker.getMergedSymbol(info.moduleFile.symbol)
: checker.tryFindAmbientModule(info.moduleName));
const symbolName = exportKind === ExportKind.Default
? InternalSymbolName.Default
: info.symbolName;
const symbol = info.symbol || cachedSymbol || Debug.checkDefined(exportKind === ExportKind.ExportEquals
? 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}`);
Copy link
Member Author

@andrewbranch andrewbranch Aug 2, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member

@amcasey amcasey Aug 2, 2021

Choose a reason for hiding this comment

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

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.

symbols.set(id, [symbol, moduleSymbol]);
return {
symbol,
Expand Down Expand Up @@ -330,30 +336,32 @@ namespace ts {
const defaultInfo = getDefaultLikeExportInfo(moduleSymbol, checker, compilerOptions);
// Note: I think we shouldn't actually see resolved module symbols here, but weird merges
// can cause it to happen: see 'completionsImport_mergedReExport.ts'
if (defaultInfo && !checker.isUndefinedSymbol(defaultInfo.symbol) && !isExternalModuleSymbol(defaultInfo.symbol)) {
if (defaultInfo && isImportableSymbol(defaultInfo.symbol, checker)) {
cache.add(
importingFile.path,
defaultInfo.symbol,
defaultInfo.exportKind === ExportKind.Default ? InternalSymbolName.Default : InternalSymbolName.ExportEquals,
moduleSymbol,
moduleFile,
defaultInfo.exportKind,
isFromPackageJson,
scriptTarget,
checker);
}
for (const exported of checker.getExportsAndPropertiesOfModule(moduleSymbol)) {
if (exported !== defaultInfo?.symbol && !isKnownSymbol(exported) && !isExternalModuleSymbol(exported) && addToSeen(seenExports, exported)) {
checker.forEachExportAndPropertyOfModule(moduleSymbol, (exported, key) => {
if (exported !== defaultInfo?.symbol && isImportableSymbol(exported, checker) && addToSeen(seenExports, exported)) {
cache.add(
importingFile.path,
exported,
key,
moduleSymbol,
moduleFile,
ExportKind.Named,
isFromPackageJson,
scriptTarget,
checker);
}
}
});
});

host.log?.(`getExportInfoMap: done in ${timestamp() - start} ms`);
Expand All @@ -368,6 +376,10 @@ namespace ts {
return info && { symbol, exportKind, ...info };
}

function isImportableSymbol(symbol: Symbol, checker: TypeChecker) {
return !checker.isUndefinedSymbol(symbol) && !checker.isUnknownSymbol(symbol) && !isKnownSymbol(symbol) && !isPrivateIdentifierSymbol(symbol);
}

function getDefaultLikeExportWorker(moduleSymbol: Symbol, checker: TypeChecker): { readonly symbol: Symbol, readonly exportKind: ExportKind } | undefined {
const exportEquals = checker.resolveExternalModuleSymbol(moduleSymbol);
if (exportEquals !== moduleSymbol) return { symbol: exportEquals, exportKind: ExportKind.ExportEquals };
Expand Down
23 changes: 13 additions & 10 deletions tests/cases/fourslash/server/completionsImport_mergedReExport.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,26 +30,29 @@

verify.completions({
marker: "",
includes: [{
name: "Config",
source: "/node_modules/@jest/types/index",
hasAction: true,
sortText: completion.SortText.AutoImportSuggestions,
}],
preferences: {
includeCompletionsForModuleExports: true,
},
});

edit.insert("o");

// Should not crash
verify.completions({
marker: "",
includes: [{
name: "Config",
source: "@jest/types",
hasAction: true,
sortText: completion.SortText.AutoImportSuggestions,
}],
preferences: {
includeCompletionsForModuleExports: true,
allowIncompleteCompletions: true,
},
});

// Because of the way `Config` is merged, we are actually not including it
// in completions here, though it would be better if we could. The `exports`
// of "@jest/types/index" would contain an alias symbol named `Config` without
// the merge from ts-jest, but with the merge, the `exports` contains the merge
// of `namespace Config` and the "@jest/types/Config" module symbol. This is
// unexpected (to me) and difficult to work with, and might be wrong? My
// expectation would have been to preserve the export alias symbol, but let it
// *resolve* to the merge of the SourceFile and the namespace declaration.