Skip to content

Commit 62f65a7

Browse files
andrewbranchandrewbranch
authored
Make auto-imports more likely to be valid for the file (including JS) & project settings (microsoft#32684)
* Add failing tests * Use default import or namespace import for import fixes when compiler options allow * Don’t do import * for export=, ever * Only do import default for export equals if nothing else will work * Never do import/require in a JavaScript file * Update tests for changes in master * Add const/require fix for JS and select based on usage heuristic * Fix JS UMD import
1 parent 725321f commit 62f65a7

7 files changed

+200
-18
lines changed

‎src/harness/fourslash.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2322,7 +2322,10 @@ namespace FourSlash {
23222322
public applyCodeActionFromCompletion(markerName: string, options: FourSlashInterface.VerifyCompletionActionOptions) {
23232323
this.goToMarker(markerName);
23242324

2325-
const details = this.getCompletionEntryDetails(options.name, options.source, options.preferences)!;
2325+
const details = this.getCompletionEntryDetails(options.name, options.source, options.preferences);
2326+
if (!details) {
2327+
return this.raiseError(`No completions were found for the given name, source, and preferences.`);
2328+
}
23262329
const codeActions = details.codeActions!;
23272330
if (codeActions.length !== 1) {
23282331
this.raiseError(`Expected one code action, got ${codeActions.length}`);

‎src/services/codefixes/importFixes.ts

Lines changed: 58 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,8 @@ namespace ts.codefix {
135135
Named,
136136
Default,
137137
Namespace,
138-
Equals
138+
Equals,
139+
ConstEquals
139140
}
140141

141142
/** Information about how a symbol is exported from a module. (We don't need to store the exported symbol, just its module.) */
@@ -163,7 +164,7 @@ namespace ts.codefix {
163164
position: number,
164165
preferences: UserPreferences,
165166
): { readonly moduleSpecifier: string, readonly codeAction: CodeAction } {
166-
const exportInfos = getAllReExportingModules(exportedSymbol, moduleSymbol, symbolName, sourceFile, program.getCompilerOptions(), program.getTypeChecker(), program.getSourceFiles());
167+
const exportInfos = getAllReExportingModules(sourceFile, exportedSymbol, moduleSymbol, symbolName, sourceFile, program.getCompilerOptions(), program.getTypeChecker(), program.getSourceFiles());
167168
Debug.assert(exportInfos.some(info => info.moduleSymbol === moduleSymbol));
168169
// We sort the best codefixes first, so taking `first` is best for completions.
169170
const moduleSpecifier = first(getNewImportInfos(program, sourceFile, position, exportInfos, host, preferences)).moduleSpecifier;
@@ -175,15 +176,15 @@ namespace ts.codefix {
175176
return { description, changes, commands };
176177
}
177178

178-
function getAllReExportingModules(exportedSymbol: Symbol, exportingModuleSymbol: Symbol, symbolName: string, sourceFile: SourceFile, compilerOptions: CompilerOptions, checker: TypeChecker, allSourceFiles: ReadonlyArray<SourceFile>): ReadonlyArray<SymbolExportInfo> {
179+
function getAllReExportingModules(importingFile: SourceFile, exportedSymbol: Symbol, exportingModuleSymbol: Symbol, symbolName: string, sourceFile: SourceFile, compilerOptions: CompilerOptions, checker: TypeChecker, allSourceFiles: ReadonlyArray<SourceFile>): ReadonlyArray<SymbolExportInfo> {
179180
const result: SymbolExportInfo[] = [];
180181
forEachExternalModule(checker, allSourceFiles, (moduleSymbol, moduleFile) => {
181182
// Don't import from a re-export when looking "up" like to `./index` or `../index`.
182183
if (moduleFile && moduleSymbol !== exportingModuleSymbol && startsWith(sourceFile.fileName, getDirectoryPath(moduleFile.fileName))) {
183184
return;
184185
}
185186

186-
const defaultInfo = getDefaultLikeExportInfo(moduleSymbol, checker, compilerOptions);
187+
const defaultInfo = getDefaultLikeExportInfo(importingFile, moduleSymbol, checker, compilerOptions);
187188
if (defaultInfo && defaultInfo.name === symbolName && skipAlias(defaultInfo.symbol, checker) === exportedSymbol) {
188189
result.push({ moduleSymbol, importKind: defaultInfo.kind, exportedSymbolIsTypeOnly: isTypeOnlySymbol(defaultInfo.symbol, checker) });
189190
}
@@ -329,7 +330,7 @@ namespace ts.codefix {
329330
if (!umdSymbol) return undefined;
330331
const symbol = checker.getAliasedSymbol(umdSymbol);
331332
const symbolName = umdSymbol.name;
332-
const exportInfos: ReadonlyArray<SymbolExportInfo> = [{ moduleSymbol: symbol, importKind: getUmdImportKind(program.getCompilerOptions()), exportedSymbolIsTypeOnly: false }];
333+
const exportInfos: ReadonlyArray<SymbolExportInfo> = [{ moduleSymbol: symbol, importKind: getUmdImportKind(sourceFile, program.getCompilerOptions()), exportedSymbolIsTypeOnly: false }];
333334
const fixes = getFixForImport(exportInfos, symbolName, isIdentifier(token) ? token.getStart(sourceFile) : undefined, program, sourceFile, host, preferences);
334335
return { fixes, symbolName };
335336
}
@@ -345,7 +346,7 @@ namespace ts.codefix {
345346
: undefined;
346347
}
347348

348-
function getUmdImportKind(compilerOptions: CompilerOptions): ImportKind {
349+
function getUmdImportKind(importingFile: SourceFile, compilerOptions: CompilerOptions): ImportKind {
349350
// Import a synthetic `default` if enabled.
350351
if (getAllowSyntheticDefaultImports(compilerOptions)) {
351352
return ImportKind.Default;
@@ -357,7 +358,10 @@ namespace ts.codefix {
357358
case ModuleKind.AMD:
358359
case ModuleKind.CommonJS:
359360
case ModuleKind.UMD:
360-
return ImportKind.Equals;
361+
if (isInJSFile(importingFile)) {
362+
return isExternalModule(importingFile) ? ImportKind.Namespace : ImportKind.ConstEquals;
363+
}
364+
return ImportKind.Equals;
361365
case ModuleKind.System:
362366
case ModuleKind.ES2015:
363367
case ModuleKind.ESNext:
@@ -403,7 +407,7 @@ namespace ts.codefix {
403407
forEachExternalModuleToImportFrom(checker, sourceFile, program.getSourceFiles(), moduleSymbol => {
404408
cancellationToken.throwIfCancellationRequested();
405409

406-
const defaultInfo = getDefaultLikeExportInfo(moduleSymbol, checker, program.getCompilerOptions());
410+
const defaultInfo = getDefaultLikeExportInfo(sourceFile, moduleSymbol, checker, program.getCompilerOptions());
407411
if (defaultInfo && defaultInfo.name === symbolName && symbolHasMeaning(defaultInfo.symbolForMeaning, currentTokenMeaning)) {
408412
addSymbol(moduleSymbol, defaultInfo.symbol, defaultInfo.kind);
409413
}
@@ -418,20 +422,41 @@ namespace ts.codefix {
418422
}
419423

420424
function getDefaultLikeExportInfo(
421-
moduleSymbol: Symbol, checker: TypeChecker, compilerOptions: CompilerOptions,
422-
): { readonly symbol: Symbol, readonly symbolForMeaning: Symbol, readonly name: string, readonly kind: ImportKind.Default | ImportKind.Equals } | undefined {
423-
const exported = getDefaultLikeExportWorker(moduleSymbol, checker);
425+
importingFile: SourceFile, moduleSymbol: Symbol, checker: TypeChecker, compilerOptions: CompilerOptions,
426+
): { readonly symbol: Symbol, readonly symbolForMeaning: Symbol, readonly name: string, readonly kind: ImportKind } | undefined {
427+
const exported = getDefaultLikeExportWorker(importingFile, moduleSymbol, checker, compilerOptions);
424428
if (!exported) return undefined;
425429
const { symbol, kind } = exported;
426430
const info = getDefaultExportInfoWorker(symbol, moduleSymbol, checker, compilerOptions);
427431
return info && { symbol, kind, ...info };
428432
}
429433

430-
function getDefaultLikeExportWorker(moduleSymbol: Symbol, checker: TypeChecker): { readonly symbol: Symbol, readonly kind: ImportKind.Default | ImportKind.Equals } | undefined {
434+
function getDefaultLikeExportWorker(importingFile: SourceFile, moduleSymbol: Symbol, checker: TypeChecker, compilerOptions: CompilerOptions): { readonly symbol: Symbol, readonly kind: ImportKind } | undefined {
431435
const defaultExport = checker.tryGetMemberInModuleExports(InternalSymbolName.Default, moduleSymbol);
432436
if (defaultExport) return { symbol: defaultExport, kind: ImportKind.Default };
433437
const exportEquals = checker.resolveExternalModuleSymbol(moduleSymbol);
434-
return exportEquals === moduleSymbol ? undefined : { symbol: exportEquals, kind: ImportKind.Equals };
438+
return exportEquals === moduleSymbol ? undefined : { symbol: exportEquals, kind: getExportEqualsImportKind(importingFile, compilerOptions, checker) };
439+
}
440+
441+
function getExportEqualsImportKind(importingFile: SourceFile, compilerOptions: CompilerOptions, checker: TypeChecker): ImportKind {
442+
if (getAllowSyntheticDefaultImports(compilerOptions) && getEmitModuleKind(compilerOptions) >= ModuleKind.ES2015) {
443+
return ImportKind.Default;
444+
}
445+
if (isInJSFile(importingFile)) {
446+
return isExternalModule(importingFile) ? ImportKind.Default : ImportKind.ConstEquals;
447+
}
448+
for (const statement of importingFile.statements) {
449+
if (isImportEqualsDeclaration(statement)) {
450+
return ImportKind.Equals;
451+
}
452+
if (isImportDeclaration(statement) && statement.importClause && statement.importClause.name) {
453+
const moduleSymbol = checker.getImmediateAliasedSymbol(statement.importClause.symbol);
454+
if (moduleSymbol && moduleSymbol.name !== InternalSymbolName.Default) {
455+
return ImportKind.Default;
456+
}
457+
}
458+
}
459+
return ImportKind.Equals;
435460
}
436461

437462
function getDefaultExportInfoWorker(defaultExport: Symbol, moduleSymbol: Symbol, checker: TypeChecker, compilerOptions: CompilerOptions): { readonly symbolForMeaning: Symbol, readonly name: string } | undefined {
@@ -543,7 +568,10 @@ namespace ts.codefix {
543568
interface ImportsCollection {
544569
readonly defaultImport: string | undefined;
545570
readonly namedImports: string[];
546-
readonly namespaceLikeImport: { readonly importKind: ImportKind.Equals | ImportKind.Namespace, readonly name: string } | undefined;
571+
readonly namespaceLikeImport: {
572+
readonly importKind: ImportKind.Equals | ImportKind.Namespace | ImportKind.ConstEquals;
573+
readonly name: string;
574+
} | undefined;
547575
}
548576
function addNewImports(changes: textChanges.ChangeTracker, sourceFile: SourceFile, moduleSpecifier: string, quotePreference: QuotePreference, { defaultImport, namedImports, namespaceLikeImport }: ImportsCollection): void {
549577
const quotedModuleSpecifier = makeStringLiteral(moduleSpecifier, quotePreference);
@@ -554,12 +582,25 @@ namespace ts.codefix {
554582
namedImports.map(n => createImportSpecifier(/*propertyName*/ undefined, createIdentifier(n))), moduleSpecifier, quotePreference));
555583
}
556584
if (namespaceLikeImport) {
557-
insertImport(changes, sourceFile, namespaceLikeImport.importKind === ImportKind.Equals
558-
? createImportEqualsDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, createIdentifier(namespaceLikeImport.name), createExternalModuleReference(quotedModuleSpecifier))
559-
: createImportDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, createImportClause(/*name*/ undefined, createNamespaceImport(createIdentifier(namespaceLikeImport.name))), quotedModuleSpecifier));
585+
insertImport(
586+
changes,
587+
sourceFile,
588+
namespaceLikeImport.importKind === ImportKind.Equals ? createImportEqualsDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, createIdentifier(namespaceLikeImport.name), createExternalModuleReference(quotedModuleSpecifier)) :
589+
namespaceLikeImport.importKind === ImportKind.ConstEquals ? createConstEqualsRequireDeclaration(namespaceLikeImport.name, quotedModuleSpecifier) :
590+
createImportDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, createImportClause(/*name*/ undefined, createNamespaceImport(createIdentifier(namespaceLikeImport.name))), quotedModuleSpecifier));
560591
}
561592
}
562593

594+
function createConstEqualsRequireDeclaration(name: string, quotedModuleSpecifier: StringLiteral): VariableStatement {
595+
return createVariableStatement(/*modifiers*/ undefined, createVariableDeclarationList([
596+
createVariableDeclaration(
597+
createIdentifier(name),
598+
/*type*/ undefined,
599+
createCall(createIdentifier("require"), /*typeArguments*/ undefined, [quotedModuleSpecifier])
600+
)
601+
], NodeFlags.Const));
602+
}
603+
563604
function symbolHasMeaning({ declarations }: Symbol, meaning: SemanticMeaning): boolean {
564605
return some(declarations, decl => !!(getMeaningFromDeclaration(decl) & meaning));
565606
}

‎tests/cases/fourslash/importNameCodeFixNewImportExportEqualsCommonJSInteropOn.ts

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// @Module: commonjs
4+
// @EsModuleInterop: true
5+
6+
// @Filename: /foo.d.ts
7+
////declare module "bar" {
8+
//// const bar: number;
9+
//// export = bar;
10+
////}
11+
////declare module "foo" {
12+
//// const foo: number;
13+
//// export = foo;
14+
////}
15+
////declare module "es" {
16+
//// const es = 0;
17+
//// export default es;
18+
////}
19+
20+
// @Filename: /a.ts
21+
////import bar from "bar";
22+
////
23+
////foo
24+
25+
// @Filename: /b.ts
26+
////foo
27+
28+
// @Filename: /c.ts
29+
////import es from "es";
30+
////
31+
////foo
32+
33+
// 1. Should match existing imports of 'export ='
34+
goTo.file('/a.ts');
35+
verify.importFixAtPosition([`import bar from "bar";
36+
import foo from "foo";
37+
38+
foo`]);
39+
40+
// 2. Should default to ImportEquals
41+
goTo.file('/b.ts');
42+
verify.importFixAtPosition([`import foo = require("foo");
43+
44+
foo`]);
45+
46+
// 3. Importing an 'export default' doesn’t count toward the usage heursitic
47+
goTo.file('/c.ts');
48+
verify.importFixAtPosition([`import es from "es";
49+
import foo = require("foo");
50+
51+
foo`]);

‎tests/cases/fourslash/importNameCodeFixNewImportExportEqualsESNextInteropOff.ts

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// @Module: esnext
4+
5+
// @Filename: /foo.d.ts
6+
////declare module "foo" {
7+
//// const foo: number;
8+
//// export = foo;
9+
////}
10+
11+
// @Filename: /index.ts
12+
////foo
13+
14+
goTo.file('/index.ts');
15+
verify.importFixAtPosition([`import foo = require("foo");
16+
17+
foo`]);

‎tests/cases/fourslash/importNameCodeFixNewImportExportEqualsESNextInteropOn.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// @EsModuleInterop: true
4+
// @Module: es2015
5+
6+
// @Filename: /foo.d.ts
7+
////declare module "foo" {
8+
//// const foo: number;
9+
//// export = foo;
10+
////}
11+
12+
// @Filename: /index.ts
13+
////[|foo|]
14+
15+
goTo.file('/index.ts');
16+
verify.importFixAtPosition([`import foo from "foo";
17+
18+
foo`]);

‎tests/cases/fourslash/importNameCodeFixNewImportExportEqualsJavaScript.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// @allowJs: true
4+
// @checkJs: true
5+
6+
// @Filename: /foo.d.ts
7+
////declare module "foo" {
8+
//// const foo: number;
9+
//// export = foo;
10+
////}
11+
12+
// @Filename: /a.js
13+
////foo
14+
15+
// @Filename: /b.js
16+
////import "";
17+
////
18+
////foo
19+
20+
// 1. JavaScript should default to 'const ... = require...' without compiler flags set
21+
goTo.file('/a.js');
22+
verify.importFixAtPosition([`const foo = require("foo");
23+
24+
foo`]);
25+
26+
// 2. If there are any ImportDeclarations, assume a default import is fine
27+
goTo.file('/b.js');
28+
verify.importFixAtPosition([`import "";
29+
import foo from "foo";
30+
31+
foo`]);

‎tests/cases/fourslash/importNameCodeFixUMDGlobalJavaScript.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
/// <reference path="fourslash.ts" />
2+
3+
// @AllowSyntheticDefaultImports: false
4+
// @Module: commonjs
5+
// @CheckJs: true
6+
// @AllowJs: true
7+
8+
// @Filename: a/f1.js
9+
//// [|export function test() { };
10+
//// bar1/*0*/.bar;|]
11+
12+
// @Filename: a/foo.d.ts
13+
//// export declare function bar(): number;
14+
//// export as namespace bar1;
15+
16+
verify.importFixAtPosition([
17+
`import * as bar1 from "./foo";
18+
19+
export function test() { };
20+
bar1.bar;`
21+
]);

0 commit comments

Comments
 (0)