Skip to content

fix autoimports crash: generate namespace and other module symbol imports #60333

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 8 commits into from
Nov 1, 2024
Merged

fix autoimports crash: generate namespace and other module symbol imports #60333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
616c423
fix moveToNewFIle
iisaduan Oct 14, 2024
1261800
Merge branch 'main' of https://github.com/microsoft/TypeScript into m…
iisaduan Oct 15, 2024
1c1a88e
paste edits server tests
iisaduan Oct 21, 2024
03ffc58
fix comments
iisaduan Oct 21, 2024
90e89e6
add test
iisaduan Oct 21, 2024
3ca07d1
add test
iisaduan Oct 22, 2024
a20c2bb
use exportinfo
iisaduan Oct 23, 2024
21a6598
fix test
iisaduan Oct 24, 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
78 changes: 76 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 @@ -80,12 +80,15 @@ import {
ImportSpecifier,
insertImports,
InternalSymbolName,
isDefaultImport,
isExternalModuleReference,
isFullSourceFile,
isIdentifier,
isImportable,
isImportClause,
isImportDeclaration,
isImportEqualsDeclaration,
isImportSpecifier,
isIntrinsicJsxName,
isJSDocImportTag,
isJsxClosingElement,
Expand Down Expand Up @@ -228,6 +231,7 @@ export interface ImportAdder {
addImportFromDiagnostic: (diagnostic: DiagnosticWithLocation, context: CodeFixContextBase) => void;
addImportFromExportedSymbol: (exportedSymbol: Symbol, isValidTypeOnlyUseSite?: boolean, referenceImport?: ImportOrRequireAliasDeclaration) => void;
addImportForNonExistentExport: (exportName: string, exportingFileName: string, exportKind: ExportKind, exportedMeanings: SymbolFlags, isImportUsageValidAsTypeOnly: boolean) => void;
addImportForModuleSymbol: (symbolAlias: Symbol, isValidTypeOnlyUseSite: boolean, referenceImport: ImportOrRequireAliasDeclaration) => void;
addImportForUnresolvedIdentifier: (context: CodeFixContextBase, symbolToken: Identifier, useAutoImportProvider: boolean) => void;
addVerbatimImport: (declaration: AnyImportOrRequireStatement | ImportOrRequireAliasDeclaration) => void;
removeExistingImport: (declaration: ImportOrRequireAliasDeclaration) => void;
Expand Down Expand Up @@ -257,7 +261,7 @@ function createImportAdderWorker(sourceFile: SourceFile | FutureSourceFile, prog
type NewImportsKey = `${0 | 1}|${string}`;
/** Use `getNewImportEntry` for access */
const newImports = new Map<NewImportsKey, Mutable<ImportsCollection & { useRequire: boolean; }>>();
return { addImportFromDiagnostic, addImportFromExportedSymbol, writeFixes, hasFixes, addImportForUnresolvedIdentifier, addImportForNonExistentExport, removeExistingImport, addVerbatimImport };
return { addImportFromDiagnostic, addImportFromExportedSymbol, addImportForModuleSymbol, writeFixes, hasFixes, addImportForUnresolvedIdentifier, addImportForNonExistentExport, removeExistingImport, addVerbatimImport };

function addVerbatimImport(declaration: AnyImportOrRequireStatement | ImportOrRequireAliasDeclaration) {
verbatimImports.add(declaration);
Expand All @@ -276,7 +280,7 @@ function createImportAdderWorker(sourceFile: SourceFile | FutureSourceFile, prog
}

function addImportFromExportedSymbol(exportedSymbol: Symbol, isValidTypeOnlyUseSite?: boolean, referenceImport?: ImportOrRequireAliasDeclaration) {
const moduleSymbol = Debug.checkDefined(exportedSymbol.parent);
const moduleSymbol = Debug.checkDefined(exportedSymbol.parent, "Expected exported symbol to have module symbol as parent");
const symbolName = getNameForExportedSymbol(exportedSymbol, getEmitScriptTarget(compilerOptions));
const checker = program.getTypeChecker();
const symbol = checker.getMergedSymbol(skipAlias(exportedSymbol, checker));
Expand Down Expand Up @@ -317,6 +321,74 @@ function createImportAdderWorker(sourceFile: SourceFile | FutureSourceFile, prog
}
}

function addImportForModuleSymbol(symbolAlias: Symbol, isValidTypeOnlyUseSite: boolean, referenceImport: ImportOrRequireAliasDeclaration) {
// Adds import for module, import alias will be symbolAlias.name
const checker = program.getTypeChecker();
const moduleSymbol = checker.getAliasedSymbol(symbolAlias);
Debug.assert(moduleSymbol.flags & SymbolFlags.Module, "Expected symbol to be a module");
const moduleSpecifierResolutionHost = createModuleSpecifierResolutionHost(program, host);
const moduleSpecifierResult = moduleSpecifiers.getModuleSpecifiersWithCacheInfo(moduleSymbol, checker, compilerOptions, sourceFile, moduleSpecifierResolutionHost, preferences, /*options*/ undefined, /*forAutoImport*/ true);

const useRequire = shouldUseRequire(sourceFile, program);

// Copy the type-only status from the reference import
let addAsTypeOnly = getAddAsTypeOnly(
isValidTypeOnlyUseSite,
/*isForNewImportDeclaration*/ true,
/*symbol*/ undefined,
symbolAlias.flags,
program.getTypeChecker(),
compilerOptions,
);
addAsTypeOnly = addAsTypeOnly === AddAsTypeOnly.Allowed && isTypeOnlyImportDeclaration(referenceImport) ? AddAsTypeOnly.Required : AddAsTypeOnly.Allowed;

// Copy the kind of import
const importKind = isImportDeclaration(referenceImport) ?
isDefaultImport(referenceImport) ? ImportKind.Default : ImportKind.Namespace :
isImportSpecifier(referenceImport) ? ImportKind.Named :
isImportClause(referenceImport) && !!referenceImport.name ? ImportKind.Default : ImportKind.Namespace;

const exportInfo = [{
symbol: symbolAlias,
moduleSymbol,
moduleFileName: moduleSymbol.declarations?.[0]?.getSourceFile()?.fileName,
exportKind: ExportKind.Module,
targetFlags: symbolAlias.flags,
isFromPackageJson: false,
}];

const existingFix = getImportFixForSymbol(
sourceFile,
exportInfo,
program,
/*position*/ undefined,
!!isValidTypeOnlyUseSite,
useRequire,
host,
preferences,
);

let fix: FixAddNewImport | ImportFixWithModuleSpecifier;
if (existingFix && importKind !== ImportKind.Namespace) {
fix = {
...existingFix,
addAsTypeOnly,
importKind,
};
}
else {
fix = {
kind: ImportFixKind.AddNew,
moduleSpecifierKind: existingFix !== undefined ? existingFix.moduleSpecifierKind : moduleSpecifierResult.kind,
moduleSpecifier: existingFix !== undefined ? existingFix.moduleSpecifier : first(moduleSpecifierResult.moduleSpecifiers),
importKind,
addAsTypeOnly,
useRequire,
};
}
addImport({ fix, symbolName: symbolAlias.name, errorIdentifierText: undefined });
}

function addImportForNonExistentExport(exportName: string, exportingFileName: string, exportKind: ExportKind, exportedMeanings: SymbolFlags, isImportUsageValidAsTypeOnly: boolean) {
const exportingSourceFile = program.getSourceFile(exportingFileName);
const useRequire = shouldUseRequire(sourceFile, program);
Expand Down Expand Up @@ -1451,6 +1523,8 @@ export function getImportKind(importingFile: SourceFile | FutureSourceFile, expo
return getExportEqualsImportKind(importingFile, program.getCompilerOptions(), !!forceImportKeyword);
case ExportKind.UMD:
return getUmdImportKind(importingFile, program, !!forceImportKeyword);
case ExportKind.Module:
return ImportKind.Namespace;
Copy link
Member

Choose a reason for hiding this comment

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

I was concerned that there are times this should return ImportKind.CommonJS, but I think the critical cases are already covered by the condition at the top of this function. It’s probably fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think cases that should return ImportKind.CommonJs still crash. #60333 (comment)

Fixes #59240 (comment) (unfortunately original js case still crashes)

default:
return Debug.assertNever(exportKind);
}
Expand Down
1 change: 1 addition & 0 deletions src/services/exportInfoMap.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ export const enum ExportKind {
Default,
ExportEquals,
UMD,
Module,
}

/** @internal */
Expand Down
4 changes: 4 additions & 0 deletions src/services/refactors/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,10 @@ export function addTargetFileImports(
if (checker.isUnknownSymbol(targetSymbol)) {
importAdder.addVerbatimImport(Debug.checkDefined(declaration ?? findAncestor(symbol.declarations?.[0], isAnyImportOrRequireStatement)));
}
else if (targetSymbol.parent === undefined) {
Debug.assert(declaration !== undefined, "expected module symbol to have a declaration");
importAdder.addImportForModuleSymbol(symbol, isValidTypeOnlyUseSite, declaration);
}
Comment on lines +83 to +86
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why we can't use isExternalModuleSymbol() as a check?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean in the if statement or the debug statement? The debug is needed since the intent of the importAdder function is to copy from an existing declaration

Copy link
Member Author

@iisaduan iisaduan Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

For the if statement, namespaces are specifically not external module symbols, so isExternalModuleSymbol wouldn't work if trying to export something that was defined as a namespace (React for example)

else {
importAdder.addImportFromExportedSymbol(targetSymbol, isValidTypeOnlyUseSite, declaration);
}
Expand Down
32 changes: 16 additions & 16 deletions tests/baselines/reference/tsserver/fourslashServer/pasteEdits_blankTargetFile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,28 +22,28 @@ console.log(abc);

console.log("abc");

//// [/home/src/workspaces/project/c.ts]
//// [/home/src/workspaces/project/folder/c.ts]


//// [/home/src/workspaces/project/tsconfig.json]
{ "files": ["c.ts", "a.ts", "b.ts"] }
{ "files": ["folder/c.ts", "a.ts", "b.ts"] }


Info seq [hh:mm:ss:mss] request:
{
"seq": 0,
"type": "request",
"arguments": {
"file": "/home/src/workspaces/project/c.ts"
"file": "/home/src/workspaces/project/folder/c.ts"
},
"command": "open"
}
Info seq [hh:mm:ss:mss] getConfigFileNameForFile:: File: /home/src/workspaces/project/c.ts ProjectRootPath: undefined:: Result: /home/src/workspaces/project/tsconfig.json
Info seq [hh:mm:ss:mss] getConfigFileNameForFile:: File: /home/src/workspaces/project/folder/c.ts ProjectRootPath: undefined:: Result: /home/src/workspaces/project/tsconfig.json
Info seq [hh:mm:ss:mss] Creating ConfiguredProject: /home/src/workspaces/project/tsconfig.json, currentDirectory: /home/src/workspaces/project
Info seq [hh:mm:ss:mss] FileWatcher:: Added:: WatchInfo: /home/src/workspaces/project/tsconfig.json 2000 undefined Project: /home/src/workspaces/project/tsconfig.json WatchType: Config file
Info seq [hh:mm:ss:mss] Config: /home/src/workspaces/project/tsconfig.json : {
"rootNames": [
"/home/src/workspaces/project/c.ts",
"/home/src/workspaces/project/folder/c.ts",
"/home/src/workspaces/project/a.ts",
"/home/src/workspaces/project/b.ts"
],
Expand All @@ -58,7 +58,7 @@ Info seq [hh:mm:ss:mss] event:
"event": "projectLoadingStart",
"body": {
"projectName": "/home/src/workspaces/project/tsconfig.json",
"reason": "Creating possible configured project for /home/src/workspaces/project/c.ts to open"
"reason": "Creating possible configured project for /home/src/workspaces/project/folder/c.ts to open"
}
}
Info seq [hh:mm:ss:mss] FileWatcher:: Added:: WatchInfo: /home/src/workspaces/project/a.ts 500 undefined WatchType: Closed Script info
Expand All @@ -81,7 +81,7 @@ Info seq [hh:mm:ss:mss] Files (6)
/home/src/tslibs/TS/Lib/lib.d.ts Text-1 lib.d.ts-Text
/home/src/tslibs/TS/Lib/lib.decorators.d.ts Text-1 lib.decorators.d.ts-Text
/home/src/tslibs/TS/Lib/lib.decorators.legacy.d.ts Text-1 lib.decorators.legacy.d.ts-Text
/home/src/workspaces/project/c.ts SVC-1-0 ""
/home/src/workspaces/project/folder/c.ts SVC-1-0 ""
/home/src/workspaces/project/a.ts Text-1 "export const abc = 10;"
/home/src/workspaces/project/b.ts Text-1 "import { abc } from \"./a\";\n\nconsole.log(abc);\n\n\nconsole.log(\"abc\");"

Expand All @@ -92,7 +92,7 @@ Info seq [hh:mm:ss:mss] Files (6)
Library referenced via 'decorators' from file '../../tslibs/TS/Lib/lib.d.ts'
../../tslibs/TS/Lib/lib.decorators.legacy.d.ts
Library referenced via 'decorators.legacy' from file '../../tslibs/TS/Lib/lib.d.ts'
c.ts
folder/c.ts
Part of 'files' list in tsconfig.json
a.ts
Part of 'files' list in tsconfig.json
Expand All @@ -116,7 +116,7 @@ Info seq [hh:mm:ss:mss] event:
"type": "event",
"event": "configFileDiag",
"body": {
"triggerFile": "/home/src/workspaces/project/c.ts",
"triggerFile": "/home/src/workspaces/project/folder/c.ts",
"configFile": "/home/src/workspaces/project/tsconfig.json",
"diagnostics": []
}
Expand All @@ -126,7 +126,7 @@ Info seq [hh:mm:ss:mss] Files (6)

Info seq [hh:mm:ss:mss] -----------------------------------------------
Info seq [hh:mm:ss:mss] Open files:
Info seq [hh:mm:ss:mss] FileName: /home/src/workspaces/project/c.ts ProjectRootPath: undefined
Info seq [hh:mm:ss:mss] FileName: /home/src/workspaces/project/folder/c.ts ProjectRootPath: undefined
Info seq [hh:mm:ss:mss] Projects: /home/src/workspaces/project/tsconfig.json
Info seq [hh:mm:ss:mss] response:
{
Expand Down Expand Up @@ -191,7 +191,7 @@ ScriptInfos::
version: Text-1
containingProjects: 1
/home/src/workspaces/project/tsconfig.json
/home/src/workspaces/project/c.ts (Open) *new*
/home/src/workspaces/project/folder/c.ts (Open) *new*
version: SVC-1-0
containingProjects: 1
/home/src/workspaces/project/tsconfig.json *default*
Expand Down Expand Up @@ -242,7 +242,7 @@ Info seq [hh:mm:ss:mss] request:
"seq": 2,
"type": "request",
"arguments": {
"file": "/home/src/workspaces/project/c.ts",
"file": "/home/src/workspaces/project/folder/c.ts",
"pastedText": [
"console.log(abc);"
],
Expand Down Expand Up @@ -283,7 +283,7 @@ Info seq [hh:mm:ss:mss] Files (6)
/home/src/tslibs/TS/Lib/lib.d.ts Text-1 lib.d.ts-Text
/home/src/tslibs/TS/Lib/lib.decorators.d.ts Text-1 lib.decorators.d.ts-Text
/home/src/tslibs/TS/Lib/lib.decorators.legacy.d.ts Text-1 lib.decorators.legacy.d.ts-Text
/home/src/workspaces/project/c.ts SVC-1-1 "console.log(abc);"
/home/src/workspaces/project/folder/c.ts SVC-1-1 "console.log(abc);"
/home/src/workspaces/project/a.ts Text-1 "export const abc = 10;"
/home/src/workspaces/project/b.ts Text-1 "import { abc } from \"./a\";\n\nconsole.log(abc);\n\n\nconsole.log(\"abc\");"

Expand All @@ -303,7 +303,7 @@ Info seq [hh:mm:ss:mss] response:
"body": {
"edits": [
{
"fileName": "/home/src/workspaces/project/c.ts",
"fileName": "/home/src/workspaces/project/folder/c.ts",
"textChanges": [
{
"start": {
Expand All @@ -314,7 +314,7 @@ Info seq [hh:mm:ss:mss] response:
"line": 1,
"offset": 1
},
"newText": "import { abc } from \"./a\";\n\n"
"newText": "import { abc } from \"../a\";\n\n"
},
{
"start": {
Expand Down Expand Up @@ -362,7 +362,7 @@ ScriptInfos::
version: Text-1
containingProjects: 1
/home/src/workspaces/project/tsconfig.json
/home/src/workspaces/project/c.ts (Open) *changed*
/home/src/workspaces/project/folder/c.ts (Open) *changed*
version: SVC-1-2 *changed*
containingProjects: 1
/home/src/workspaces/project/tsconfig.json *default*
Loading