Skip to content
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.

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

Fix completion details auto-imports crashes #52686

Merged
merged 8 commits into from
Feb 10, 2023
17 changes: 8 additions & 9 deletions src/services/codefixes/importFixes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -230,9 +230,9 @@ function createImportAdderWorker(sourceFile: SourceFile, program: Program, useAu
const symbolName = getNameForExportedSymbol(exportedSymbol, getEmitScriptTarget(compilerOptions));
const checker = program.getTypeChecker();
const symbol = checker.getMergedSymbol(skipAlias(exportedSymbol, checker));
const exportInfo = getAllExportInfoForSymbol(sourceFile, symbol, symbolName, /*isJsxTagName*/ false, program, host, preferences, cancellationToken);
const exportInfo = getAllExportInfoForSymbol(sourceFile, symbol, symbolName, moduleSymbol, /*isJsxTagName*/ false, program, host, preferences, cancellationToken);
const useRequire = shouldUseRequire(sourceFile, program);
const fix = getImportFixForSymbol(sourceFile, Debug.checkDefined(exportInfo), moduleSymbol, program, /*position*/ undefined, !!isValidTypeOnlyUseSite, useRequire, host, preferences);
const fix = getImportFixForSymbol(sourceFile, Debug.checkDefined(exportInfo), program, /*position*/ undefined, !!isValidTypeOnlyUseSite, useRequire, host, preferences);
if (fix) {
addImport({ fix, symbolName, errorIdentifierText: undefined });
}
Expand Down Expand Up @@ -504,12 +504,12 @@ export function getImportCompletionAction(

const exportInfos = pathIsBareSpecifier(stripQuotes(moduleSymbol.name))
? [getSingleExportInfoForSymbol(targetSymbol, moduleSymbol, program, host)]
: getAllExportInfoForSymbol(sourceFile, targetSymbol, symbolName, isJsxTagName, program, host, preferences, cancellationToken);
: getAllExportInfoForSymbol(sourceFile, targetSymbol, symbolName, moduleSymbol, isJsxTagName, program, host, preferences, cancellationToken);

Debug.assertIsDefined(exportInfos);
Debug.assertIsDefined(exportInfos, "Some exportInfo should match the specified symbol / moduleSymbol");
const useRequire = shouldUseRequire(sourceFile, program);
const isValidTypeOnlyUseSite = isValidTypeOnlyAliasUseSite(getTokenAtPosition(sourceFile, position));
const fix = Debug.checkDefined(getImportFixForSymbol(sourceFile, exportInfos, moduleSymbol, program, position, isValidTypeOnlyUseSite, useRequire, host, preferences));
const fix = Debug.checkDefined(getImportFixForSymbol(sourceFile, exportInfos, program, position, isValidTypeOnlyUseSite, useRequire, host, preferences));
return {
moduleSpecifier: fix.moduleSpecifier,
codeAction: codeFixActionToCodeAction(codeActionForFix(
Expand All @@ -532,8 +532,7 @@ export function getPromoteTypeOnlyCompletionAction(sourceFile: SourceFile, symbo
return fix && codeFixActionToCodeAction(codeActionForFix({ host, formatContext, preferences }, sourceFile, symbolName, fix, includeSymbolNameInDescription, compilerOptions, preferences));
}

function getImportFixForSymbol(sourceFile: SourceFile, exportInfos: readonly SymbolExportInfo[], moduleSymbol: Symbol, program: Program, position: number | undefined, isValidTypeOnlyUseSite: boolean, useRequire: boolean, host: LanguageServiceHost, preferences: UserPreferences) {
Debug.assert(exportInfos.some(info => info.moduleSymbol === moduleSymbol || info.symbol.parent === moduleSymbol), "Some exportInfo should match the specified moduleSymbol");
function getImportFixForSymbol(sourceFile: SourceFile, exportInfos: readonly SymbolExportInfo[], program: Program, position: number | undefined, isValidTypeOnlyUseSite: boolean, useRequire: boolean, host: LanguageServiceHost, preferences: UserPreferences) {
const packageJsonImportFilter = createPackageJsonImportFilter(sourceFile, preferences, host);
return getBestFix(getImportFixes(exportInfos, position, isValidTypeOnlyUseSite, useRequire, program, sourceFile, host, preferences).fixes, sourceFile, program, packageJsonImportFilter, host);
}
Expand All @@ -542,11 +541,11 @@ function codeFixActionToCodeAction({ description, changes, commands }: CodeFixAc
return { description, changes, commands };
}

function getAllExportInfoForSymbol(importingFile: SourceFile, symbol: Symbol, symbolName: string, preferCapitalized: boolean, program: Program, host: LanguageServiceHost, preferences: UserPreferences, cancellationToken: CancellationToken | undefined): readonly SymbolExportInfo[] | undefined {
function getAllExportInfoForSymbol(importingFile: SourceFile, symbol: Symbol, symbolName: string, moduleSymbol: Symbol, preferCapitalized: boolean, program: Program, host: LanguageServiceHost, preferences: UserPreferences, cancellationToken: CancellationToken | undefined): readonly SymbolExportInfo[] | undefined {
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)) {
Copy link
Member

Choose a reason for hiding this comment

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

Can't recall but I think our own utility for checking if something is in an array is faster.

return info;
}
});
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/// <reference path="../fourslash.ts" />

// @Filename: /tsconfig.json
//// {
//// "compilerOptions": {
//// "module": "commonjs"
//// }
//// }

// @Filename: /node_modules/@types/node/index.d.ts
//// declare module "fs" {
//// export function accessSync(path: string): void;
//// }

// @Filename: /node_modules/@types/fs-extra/index.d.ts
//// export * from "fs";

// @Filename: /index.ts
//// access/**/

verify.completions({
marker: "",
includes: [{
name: "accessSync",
source: "fs",
sourceDisplay: "fs",
hasAction: true,
sortText: completion.SortText.AutoImportSuggestions
}, {
name: "accessSync",
source: "fs-extra",
sourceDisplay: "fs-extra",
hasAction: true,
sortText: completion.SortText.AutoImportSuggestions
}],
preferences: {
includeCompletionsForModuleExports: true,
allowIncompleteCompletions: true
}
});

verify.applyCodeActionFromCompletion("", {
name: "accessSync",
source: "fs-extra",
description: `Add import from "fs-extra"`,
newFileContent: `import { accessSync } from "fs-extra";\r\n\r\naccess`,
data: {
exportName: "accessSync",
fileName: "/node_modules/@types/fs-extra/index.d.ts",
moduleSpecifier: "fs-extra",
}
});