Skip to content

Fix re-exported defaults in ExportInfoMap #58837

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 2 commits into from
Jun 12, 2024
Merged

Fix re-exported defaults in ExportInfoMap #58837

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9328ce2
Add failing test
andrewbranch Jun 10, 2024
32d44af
Find better names for re-exported default/export= in ExportInfoMap
andrewbranch Jun 12, 2024
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
4 changes: 2 additions & 2 deletions src/services/codefixes/importFixes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1187,7 +1187,7 @@ function getNewImportFixes(
const exportEquals = checker.resolveExternalModuleSymbol(exportInfo.moduleSymbol);
let namespacePrefix;
if (exportEquals !== exportInfo.moduleSymbol) {
namespacePrefix = forEachNameOfDefaultExport(exportEquals, checker, compilerOptions, /*preferCapitalizedNames*/ false, identity)!;
namespacePrefix = forEachNameOfDefaultExport(exportEquals, checker, getEmitScriptTarget(compilerOptions), identity)!;
}
namespacePrefix ||= moduleSymbolToValidIdentifier(
exportInfo.moduleSymbol,
Expand Down Expand Up @@ -1544,7 +1544,7 @@ function getExportInfos(
if (
defaultInfo
&& symbolFlagsHaveMeaning(checker.getSymbolFlags(defaultInfo.symbol), currentTokenMeaning)
&& forEachNameOfDefaultExport(defaultInfo.symbol, checker, compilerOptions, isJsxTagName, name => name === symbolName)
&& forEachNameOfDefaultExport(defaultInfo.symbol, checker, getEmitScriptTarget(compilerOptions), (name, capitalizedName) => (isJsxTagName ? capitalizedName ?? name : name) === symbolName)
) {
addSymbol(moduleSymbol, sourceFile, defaultInfo.symbol, defaultInfo.exportKind, program, isFromPackageJson);
}
Expand Down
22 changes: 16 additions & 6 deletions src/services/exportInfoMap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import {
append,
arrayIsEqualTo,
CancellationToken,
CompilerOptions,
consumesNodeCoreModules,
createMultiMap,
Debug,
Expand All @@ -18,9 +17,7 @@ import {
GetCanonicalFileName,
getDefaultLikeExportNameFromDeclaration,
getDirectoryPath,
getEmitScriptTarget,
getLocalSymbolForExportDefault,
getNamesForExportedSymbol,
getNodeModulePathParts,
getPackageNameFromTypesPackageName,
getRegexFromPattern,
Expand All @@ -46,6 +43,7 @@ import {
Path,
pathContainsNodeModules,
Program,
ScriptTarget,
skipAlias,
SourceFile,
startsWith,
Expand Down Expand Up @@ -198,7 +196,7 @@ export function createCacheableExportInfoMap(host: CacheableExportInfoMapHost):
// get a better name.
const names = exportKind === ExportKind.Named || isExternalModuleSymbol(namedSymbol)
? unescapeLeadingUnderscores(symbolTableKey)
: getNamesForExportedSymbol(namedSymbol, /*scriptTarget*/ undefined);
: getNamesForExportedSymbol(namedSymbol, checker, /*scriptTarget*/ undefined);

const symbolName = typeof names === "string" ? names : names[0];
const capitalizedSymbolName = typeof names === "string" ? undefined : names[1];
Expand Down Expand Up @@ -558,12 +556,21 @@ function isImportableSymbol(symbol: Symbol, checker: TypeChecker) {
return !checker.isUndefinedSymbol(symbol) && !checker.isUnknownSymbol(symbol) && !isKnownSymbol(symbol) && !isPrivateIdentifierSymbol(symbol);
}

function getNamesForExportedSymbol(defaultExport: Symbol, checker: TypeChecker, scriptTarget: ScriptTarget | undefined) {
let names: string | string[] | undefined;
forEachNameOfDefaultExport(defaultExport, checker, scriptTarget, (name, capitalizedName) => {
names = capitalizedName ? [name, capitalizedName] : name;
return true;
});
return Debug.checkDefined(names);
}

/**
* @internal
* May call `cb` multiple times with the same name.
* Terminates when `cb` returns a truthy value.
*/
export function forEachNameOfDefaultExport<T>(defaultExport: Symbol, checker: TypeChecker, compilerOptions: CompilerOptions, preferCapitalizedNames: boolean, cb: (name: string) => T | undefined): T | undefined {
export function forEachNameOfDefaultExport<T>(defaultExport: Symbol, checker: TypeChecker, scriptTarget: ScriptTarget | undefined, cb: (name: string, capitalizedName?: string) => T | undefined): T | undefined {
let chain: Symbol[] | undefined;
let current: Symbol | undefined = defaultExport;

Expand All @@ -588,7 +595,10 @@ export function forEachNameOfDefaultExport<T>(defaultExport: Symbol, checker: Ty

for (const symbol of chain ?? emptyArray) {
if (symbol.parent && isExternalModuleSymbol(symbol.parent)) {
const final = cb(moduleSymbolToValidIdentifier(symbol.parent, getEmitScriptTarget(compilerOptions), preferCapitalizedNames));
const final = cb(
moduleSymbolToValidIdentifier(symbol.parent, scriptTarget, /*forceCapitalize*/ false),
moduleSymbolToValidIdentifier(symbol.parent, scriptTarget, /*forceCapitalize*/ true),
);
if (final) return final;
}
}
Expand Down
31 changes: 11 additions & 20 deletions src/services/utilities.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4024,22 +4024,13 @@ export function firstOrOnly<T>(valueOrArray: T | readonly T[]): T {
return isArray(valueOrArray) ? first(valueOrArray) : valueOrArray;
}

/** @internal */
export function getNamesForExportedSymbol(symbol: Symbol, scriptTarget: ScriptTarget | undefined): string | [lowercase: string, capitalized: string] {
if (needsNameFromDeclaration(symbol)) {
const fromDeclaration = getDefaultLikeExportNameFromDeclaration(symbol);
if (fromDeclaration) return fromDeclaration;
const fileNameCase = moduleSymbolToValidIdentifier(getSymbolParentOrFail(symbol), scriptTarget, /*forceCapitalize*/ false);
const capitalized = moduleSymbolToValidIdentifier(getSymbolParentOrFail(symbol), scriptTarget, /*forceCapitalize*/ true);
if (fileNameCase === capitalized) return fileNameCase;
return [fileNameCase, capitalized];
}
return symbol.name;
}

/** @internal */
/**
* If a type checker and multiple files are available, consider using `forEachNameOfDefaultExport`
* instead, which searches for names of re-exported defaults/namespaces in target files.
* @internal
*/
export function getNameForExportedSymbol(symbol: Symbol, scriptTarget: ScriptTarget | undefined, preferCapitalized?: boolean) {
if (needsNameFromDeclaration(symbol)) {
if (symbol.escapedName === InternalSymbolName.ExportEquals || symbol.escapedName === InternalSymbolName.Default) {
// Names for default exports:
// - export default foo => foo
// - export { foo as default } => foo
Expand All @@ -4050,11 +4041,11 @@ export function getNameForExportedSymbol(symbol: Symbol, scriptTarget: ScriptTar
return symbol.name;
}

function needsNameFromDeclaration(symbol: Symbol) {
return !(symbol.flags & SymbolFlags.Transient) && (symbol.escapedName === InternalSymbolName.ExportEquals || symbol.escapedName === InternalSymbolName.Default);
}

/** @internal */
/**
* If a type checker and multiple files are available, consider using `forEachNameOfDefaultExport`
* instead, which searches for names of re-exported defaults/namespaces in target files.
* @internal
*/
export function getDefaultLikeExportNameFromDeclaration(symbol: Symbol): string | undefined {
return firstDefined(symbol.declarations, d => {
// "export default" in this case. See `ExportAssignment`for more details.
Expand Down
30 changes: 28 additions & 2 deletions ...selines/reference/tsserver/fourslashServer/completionsImport_jsModuleExportsAssignment.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ Info seq [hh:mm:ss:mss] getCompletionData: Is inside comment: *
Info seq [hh:mm:ss:mss] getCompletionData: Get previous token: *
Info seq [hh:mm:ss:mss] getExportInfoMap: cache miss or empty; calculating new results
Info seq [hh:mm:ss:mss] getExportInfoMap: done in * ms
Info seq [hh:mm:ss:mss] collectAutoImports: resolved 0 module specifiers, plus 0 ambient and 3 from cache
Info seq [hh:mm:ss:mss] collectAutoImports: resolved 0 module specifiers, plus 0 ambient and 4 from cache
Info seq [hh:mm:ss:mss] collectAutoImports: response is incomplete
Info seq [hh:mm:ss:mss] collectAutoImports: *
Info seq [hh:mm:ss:mss] getCompletionData: Semantic work: *
Expand Down Expand Up @@ -1053,6 +1053,19 @@ Info seq [hh:mm:ss:mss] response:
"fileName": "/third_party/marked/src/defaults.js"
}
},
{
"name": "defaults",
"kind": "property",
"kindModifiers": "",
"sortText": "16",
"hasAction": true,
"source": "/third_party/marked/src/defaults",
"data": {
"exportName": "export=",
"exportMapKey": "8 * defaults ",
"fileName": "/third_party/marked/src/defaults.js"
}
},
{
"name": "defaults",
"kind": "alias",
Expand Down Expand Up @@ -1257,7 +1270,7 @@ Info seq [hh:mm:ss:mss] getCompletionData: Get current token: *
Info seq [hh:mm:ss:mss] getCompletionData: Is inside comment: *
Info seq [hh:mm:ss:mss] getCompletionData: Get previous token: *
Info seq [hh:mm:ss:mss] getExportInfoMap: cache hit
Info seq [hh:mm:ss:mss] collectAutoImports: resolved 0 module specifiers, plus 0 ambient and 3 from cache
Info seq [hh:mm:ss:mss] collectAutoImports: resolved 0 module specifiers, plus 0 ambient and 4 from cache
Info seq [hh:mm:ss:mss] collectAutoImports: response is incomplete
Info seq [hh:mm:ss:mss] collectAutoImports: *
Info seq [hh:mm:ss:mss] getCompletionData: Semantic work: *
Expand Down Expand Up @@ -1943,6 +1956,19 @@ Info seq [hh:mm:ss:mss] response:
"fileName": "/third_party/marked/src/defaults.js"
}
},
{
"name": "defaults",
"kind": "property",
"kindModifiers": "",
"sortText": "16",
"hasAction": true,
"source": "/third_party/marked/src/defaults",
"data": {
"exportName": "export=",
"exportMapKey": "8 * defaults ",
"fileName": "/third_party/marked/src/defaults.js"
}
Comment on lines +1960 to +1970
Copy link
Member Author

Choose a reason for hiding this comment

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

This was being filtered out by the !(symbol.flags & SymbolFlags.Transient) condition in utilities.ts that I removed here. That code path was no longer exercised by the name generation code in the ExportInfoMap construction, but I was curious why it was there, and traced it back to andrewbranch@258be21 to see if it was going to be a problem. So much has changed around this in the last three years that it seems like the crash is no longer an issue, so I removed the transient symbol filtering from the other function where it was happening.

},
{
"name": "defaults",
"kind": "alias",
Expand Down
39 changes: 39 additions & 0 deletions tests/cases/fourslash/completionsImport_reExportDefault2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/// <reference path="fourslash.ts" />

// @module: preserve
// @checkJs: true

// @Filename: /node_modules/example/package.json
//// { "name": "example", "version": "1.0.0", "main": "dist/index.js" }

// @Filename: /node_modules/example/dist/nested/module.d.ts
//// declare const defaultExport: () => void;
//// declare const namedExport: () => void;
////
//// export default defaultExport;
//// export { namedExport };

// @Filename: /node_modules/example/dist/index.d.ts
//// export { default, namedExport } from "./nested/module";
Copy link
Member Author

Choose a reason for hiding this comment

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

The ExportInfoMap stores chains of re-exports of a given symbol for as long as the symbol has “the same name” in every link in the chain. And by “name,” we mean ones that the user would want to reference a symbol by, not default or export=. So when we see default or export=, we try to figure out a better name to offer as the auto-import, by trying to resolve the default export to another named declaration, or making a name out of the filename as a last resort. We do this (among other places) as we’re building the chains of re-exports for the ExportInfoMap, with the goal of being able to answer the question “Give me all the modules from which I can import a given symbol by a given name.” For this test to work, we need to understand that we can import defaultExport declared in example/dist/nested/module.d.ts both from there and from this re-export right here ☝️. But before this PR, when processing this re-exported default symbol, we didn’t search its target file for local names. Instead, we just decided to call it dist, after dist/index.d.ts, and so it created a new chain in the map instead of being joined to defaultExport from example/dist/nested/module.d.ts.


// @Filename: /index.mjs
//// import { namedExport } from "example";
//// defaultExp/**/

verify.completions({
marker: "",
exact: completion.globalsInJsPlus([
"namedExport",
{
name: "defaultExport",
source: "example",
sourceDisplay: "example",
hasAction: true,
sortText: completion.SortText.AutoImportSuggestions
},
]),
preferences: {
includeCompletionsForModuleExports: true,
allowIncompleteCompletions: true,
},
});