Skip to content

Make auto-imports more likely to be valid for the file (including JS) & project settings #32684

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

Merged
merged 8 commits into from
Aug 2, 2019
Merged
5 changes: 4 additions & 1 deletion src/harness/fourslash.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2322,7 +2322,10 @@ namespace FourSlash {
public applyCodeActionFromCompletion(markerName: string, options: FourSlashInterface.VerifyCompletionActionOptions) {
this.goToMarker(markerName);

const details = this.getCompletionEntryDetails(options.name, options.source, options.preferences)!;
const details = this.getCompletionEntryDetails(options.name, options.source, options.preferences);
if (!details) {
return this.raiseError(`No completions were found for the given name, source, and preferences.`);
}
const codeActions = details.codeActions!;
if (codeActions.length !== 1) {
this.raiseError(`Expected one code action, got ${codeActions.length}`);
Expand Down
75 changes: 58 additions & 17 deletions src/services/codefixes/importFixes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,8 @@ namespace ts.codefix {
Named,
Default,
Namespace,
Equals
Equals,
ConstEquals
}

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

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

const defaultInfo = getDefaultLikeExportInfo(moduleSymbol, checker, compilerOptions);
const defaultInfo = getDefaultLikeExportInfo(importingFile, moduleSymbol, checker, compilerOptions);
if (defaultInfo && defaultInfo.name === symbolName && skipAlias(defaultInfo.symbol, checker) === exportedSymbol) {
result.push({ moduleSymbol, importKind: defaultInfo.kind, exportedSymbolIsTypeOnly: isTypeOnlySymbol(defaultInfo.symbol, checker) });
}
Expand Down Expand Up @@ -329,7 +330,7 @@ namespace ts.codefix {
if (!umdSymbol) return undefined;
const symbol = checker.getAliasedSymbol(umdSymbol);
const symbolName = umdSymbol.name;
const exportInfos: ReadonlyArray<SymbolExportInfo> = [{ moduleSymbol: symbol, importKind: getUmdImportKind(program.getCompilerOptions()), exportedSymbolIsTypeOnly: false }];
const exportInfos: ReadonlyArray<SymbolExportInfo> = [{ moduleSymbol: symbol, importKind: getUmdImportKind(sourceFile, program.getCompilerOptions()), exportedSymbolIsTypeOnly: false }];
const fixes = getFixForImport(exportInfos, symbolName, isIdentifier(token) ? token.getStart(sourceFile) : undefined, program, sourceFile, host, preferences);
return { fixes, symbolName };
}
Expand All @@ -345,7 +346,7 @@ namespace ts.codefix {
: undefined;
}

function getUmdImportKind(compilerOptions: CompilerOptions): ImportKind {
function getUmdImportKind(importingFile: SourceFile, compilerOptions: CompilerOptions): ImportKind {
// Import a synthetic `default` if enabled.
if (getAllowSyntheticDefaultImports(compilerOptions)) {
return ImportKind.Default;
Expand All @@ -357,7 +358,10 @@ namespace ts.codefix {
case ModuleKind.AMD:
case ModuleKind.CommonJS:
case ModuleKind.UMD:
return ImportKind.Equals;
if (isInJSFile(importingFile)) {
return isExternalModule(importingFile) ? ImportKind.Namespace : ImportKind.ConstEquals;
}
return ImportKind.Equals;
case ModuleKind.System:
case ModuleKind.ES2015:
case ModuleKind.ESNext:
Expand Down Expand Up @@ -403,7 +407,7 @@ namespace ts.codefix {
forEachExternalModuleToImportFrom(checker, sourceFile, program.getSourceFiles(), moduleSymbol => {
cancellationToken.throwIfCancellationRequested();

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

function getDefaultLikeExportInfo(
moduleSymbol: Symbol, checker: TypeChecker, compilerOptions: CompilerOptions,
): { readonly symbol: Symbol, readonly symbolForMeaning: Symbol, readonly name: string, readonly kind: ImportKind.Default | ImportKind.Equals } | undefined {
const exported = getDefaultLikeExportWorker(moduleSymbol, checker);
importingFile: SourceFile, moduleSymbol: Symbol, checker: TypeChecker, compilerOptions: CompilerOptions,
): { readonly symbol: Symbol, readonly symbolForMeaning: Symbol, readonly name: string, readonly kind: ImportKind } | undefined {
const exported = getDefaultLikeExportWorker(importingFile, moduleSymbol, checker, compilerOptions);
if (!exported) return undefined;
const { symbol, kind } = exported;
const info = getDefaultExportInfoWorker(symbol, moduleSymbol, checker, compilerOptions);
return info && { symbol, kind, ...info };
}

function getDefaultLikeExportWorker(moduleSymbol: Symbol, checker: TypeChecker): { readonly symbol: Symbol, readonly kind: ImportKind.Default | ImportKind.Equals } | undefined {
function getDefaultLikeExportWorker(importingFile: SourceFile, moduleSymbol: Symbol, checker: TypeChecker, compilerOptions: CompilerOptions): { readonly symbol: Symbol, readonly kind: ImportKind } | undefined {
const defaultExport = checker.tryGetMemberInModuleExports(InternalSymbolName.Default, moduleSymbol);
if (defaultExport) return { symbol: defaultExport, kind: ImportKind.Default };
const exportEquals = checker.resolveExternalModuleSymbol(moduleSymbol);
return exportEquals === moduleSymbol ? undefined : { symbol: exportEquals, kind: ImportKind.Equals };
return exportEquals === moduleSymbol ? undefined : { symbol: exportEquals, kind: getExportEqualsImportKind(importingFile, compilerOptions, checker) };
}

function getExportEqualsImportKind(importingFile: SourceFile, compilerOptions: CompilerOptions, checker: TypeChecker): ImportKind {
if (getAllowSyntheticDefaultImports(compilerOptions) && getEmitModuleKind(compilerOptions) >= ModuleKind.ES2015) {
return ImportKind.Default;
}
if (isInJSFile(importingFile)) {
return isExternalModule(importingFile) ? ImportKind.Default : ImportKind.ConstEquals;
}
for (const statement of importingFile.statements) {
if (isImportEqualsDeclaration(statement)) {
return ImportKind.Equals;
}
if (isImportDeclaration(statement) && statement.importClause && statement.importClause.name) {
const moduleSymbol = checker.getImmediateAliasedSymbol(statement.importClause.symbol);
if (moduleSymbol && moduleSymbol.name !== InternalSymbolName.Default) {
return ImportKind.Default;
}
}
}
return ImportKind.Equals;
}

function getDefaultExportInfoWorker(defaultExport: Symbol, moduleSymbol: Symbol, checker: TypeChecker, compilerOptions: CompilerOptions): { readonly symbolForMeaning: Symbol, readonly name: string } | undefined {
Expand Down Expand Up @@ -543,7 +568,10 @@ namespace ts.codefix {
interface ImportsCollection {
readonly defaultImport: string | undefined;
readonly namedImports: string[];
readonly namespaceLikeImport: { readonly importKind: ImportKind.Equals | ImportKind.Namespace, readonly name: string } | undefined;
readonly namespaceLikeImport: {
readonly importKind: ImportKind.Equals | ImportKind.Namespace | ImportKind.ConstEquals;
readonly name: string;
} | undefined;
}
function addNewImports(changes: textChanges.ChangeTracker, sourceFile: SourceFile, moduleSpecifier: string, quotePreference: QuotePreference, { defaultImport, namedImports, namespaceLikeImport }: ImportsCollection): void {
const quotedModuleSpecifier = makeStringLiteral(moduleSpecifier, quotePreference);
Expand All @@ -554,12 +582,25 @@ namespace ts.codefix {
namedImports.map(n => createImportSpecifier(/*propertyName*/ undefined, createIdentifier(n))), moduleSpecifier, quotePreference));
}
if (namespaceLikeImport) {
insertImport(changes, sourceFile, namespaceLikeImport.importKind === ImportKind.Equals
? createImportEqualsDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, createIdentifier(namespaceLikeImport.name), createExternalModuleReference(quotedModuleSpecifier))
: createImportDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, createImportClause(/*name*/ undefined, createNamespaceImport(createIdentifier(namespaceLikeImport.name))), quotedModuleSpecifier));
insertImport(
changes,
sourceFile,
namespaceLikeImport.importKind === ImportKind.Equals ? createImportEqualsDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, createIdentifier(namespaceLikeImport.name), createExternalModuleReference(quotedModuleSpecifier)) :
namespaceLikeImport.importKind === ImportKind.ConstEquals ? createConstEqualsRequireDeclaration(namespaceLikeImport.name, quotedModuleSpecifier) :
createImportDeclaration(/*decorators*/ undefined, /*modifiers*/ undefined, createImportClause(/*name*/ undefined, createNamespaceImport(createIdentifier(namespaceLikeImport.name))), quotedModuleSpecifier));
}
}

function createConstEqualsRequireDeclaration(name: string, quotedModuleSpecifier: StringLiteral): VariableStatement {
return createVariableStatement(/*modifiers*/ undefined, createVariableDeclarationList([
createVariableDeclaration(
createIdentifier(name),
/*type*/ undefined,
createCall(createIdentifier("require"), /*typeArguments*/ undefined, [quotedModuleSpecifier])
)
], NodeFlags.Const));
}

function symbolHasMeaning({ declarations }: Symbol, meaning: SemanticMeaning): boolean {
return some(declarations, decl => !!(getMeaningFromDeclaration(decl) & meaning));
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/// <reference path="fourslash.ts" />

// @Module: commonjs
// @EsModuleInterop: true

// @Filename: /foo.d.ts
////declare module "bar" {
//// const bar: number;
//// export = bar;
////}
////declare module "foo" {
//// const foo: number;
//// export = foo;
////}
////declare module "es" {
//// const es = 0;
//// export default es;
////}

// @Filename: /a.ts
////import bar from "bar";
////
////foo

// @Filename: /b.ts
////foo

// @Filename: /c.ts
////import es from "es";
////
////foo

// 1. Should match existing imports of 'export ='
goTo.file('/a.ts');
verify.importFixAtPosition([`import bar from "bar";
import foo from "foo";

foo`]);

// 2. Should default to ImportEquals
goTo.file('/b.ts');
verify.importFixAtPosition([`import foo = require("foo");

foo`]);

// 3. Importing an 'export default' doesn’t count toward the usage heursitic
goTo.file('/c.ts');
verify.importFixAtPosition([`import es from "es";
import foo = require("foo");

foo`]);
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/// <reference path="fourslash.ts" />

// @Module: esnext

// @Filename: /foo.d.ts
////declare module "foo" {
//// const foo: number;
//// export = foo;
////}

// @Filename: /index.ts
////foo

goTo.file('/index.ts');
verify.importFixAtPosition([`import foo = require("foo");

foo`]);
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/// <reference path="fourslash.ts" />

// @EsModuleInterop: true
// @Module: es2015

// @Filename: /foo.d.ts
////declare module "foo" {
//// const foo: number;
//// export = foo;
////}

// @Filename: /index.ts
////[|foo|]

goTo.file('/index.ts');
verify.importFixAtPosition([`import foo from "foo";

foo`]);
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
/// <reference path="fourslash.ts" />

// @allowJs: true
// @checkJs: true

// @Filename: /foo.d.ts
////declare module "foo" {
//// const foo: number;
//// export = foo;
////}

// @Filename: /a.js
////foo

// @Filename: /b.js
////import "";
////
////foo

// 1. JavaScript should default to 'const ... = require...' without compiler flags set
goTo.file('/a.js');
verify.importFixAtPosition([`const foo = require("foo");

foo`]);

// 2. If there are any ImportDeclarations, assume a default import is fine
goTo.file('/b.js');
verify.importFixAtPosition([`import "";
import foo from "foo";

foo`]);
21 changes: 21 additions & 0 deletions tests/cases/fourslash/importNameCodeFixUMDGlobalJavaScript.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
/// <reference path="fourslash.ts" />

// @AllowSyntheticDefaultImports: false
// @Module: commonjs
// @CheckJs: true
// @AllowJs: true

// @Filename: a/f1.js
//// [|export function test() { };
//// bar1/*0*/.bar;|]

// @Filename: a/foo.d.ts
//// export declare function bar(): number;
//// export as namespace bar1;

verify.importFixAtPosition([
`import * as bar1 from "./foo";

export function test() { };
bar1.bar;`
]);