From 0389207036e661c8838d5d6a5876b55e64cc5b37 Mon Sep 17 00:00:00 2001 From: Paul van Brenk Date: Wed, 19 Oct 2016 18:03:22 -0700 Subject: [PATCH 01/14] Add codefix for missing imports + tests --- src/compiler/diagnosticMessages.json | 12 + src/services/codefixes/codeFixProvider.ts | 3 +- src/services/codefixes/fixes.ts | 1 + src/services/codefixes/importFixes.ts | 284 ++++++++++++++++++ src/services/services.ts | 5 +- .../importNameCodeFixExistingImport0.ts | 10 + .../importNameCodeFixExistingImport1.ts | 11 + .../importNameCodeFixExistingImport10.ts | 21 ++ .../importNameCodeFixExistingImport11.ts | 20 ++ .../importNameCodeFixExistingImport12.ts | 12 + .../importNameCodeFixExistingImport2.ts | 10 + .../importNameCodeFixExistingImport3.ts | 11 + .../importNameCodeFixExistingImport4.ts | 11 + .../importNameCodeFixExistingImport5.ts | 10 + .../importNameCodeFixExistingImport6.ts | 13 + .../importNameCodeFixExistingImport7.ts | 10 + .../importNameCodeFixExistingImport8.ts | 12 + .../importNameCodeFixExistingImport9.ts | 17 ++ .../fourslash/importNameCodeFixNewImport0.ts | 14 + .../fourslash/importNameCodeFixNewImport1.ts | 28 ++ .../fourslash/importNameCodeFixNewImport2.ts | 20 ++ .../fourslash/importNameCodeFixNewImport3.ts | 12 + .../fourslash/importNameCodeFixNewImport4.ts | 17 ++ .../fourslash/importNameCodeFixNewImport5.ts | 12 + .../fourslash/importNameCodeFixNewImport6.ts | 18 ++ .../fourslash/importNameCodeFixNewImport7.ts | 15 + .../fourslash/importNameCodeFixNewImport8.ts | 30 ++ .../fourslash/importNameCodeFixNewImport9.ts | 24 ++ 28 files changed, 660 insertions(+), 3 deletions(-) create mode 100644 src/services/codefixes/importFixes.ts create mode 100644 tests/cases/fourslash/importNameCodeFixExistingImport0.ts create mode 100644 tests/cases/fourslash/importNameCodeFixExistingImport1.ts create mode 100644 tests/cases/fourslash/importNameCodeFixExistingImport10.ts create mode 100644 tests/cases/fourslash/importNameCodeFixExistingImport11.ts create mode 100644 tests/cases/fourslash/importNameCodeFixExistingImport12.ts create mode 100644 tests/cases/fourslash/importNameCodeFixExistingImport2.ts create mode 100644 tests/cases/fourslash/importNameCodeFixExistingImport3.ts create mode 100644 tests/cases/fourslash/importNameCodeFixExistingImport4.ts create mode 100644 tests/cases/fourslash/importNameCodeFixExistingImport5.ts create mode 100644 tests/cases/fourslash/importNameCodeFixExistingImport6.ts create mode 100644 tests/cases/fourslash/importNameCodeFixExistingImport7.ts create mode 100644 tests/cases/fourslash/importNameCodeFixExistingImport8.ts create mode 100644 tests/cases/fourslash/importNameCodeFixExistingImport9.ts create mode 100644 tests/cases/fourslash/importNameCodeFixNewImport0.ts create mode 100644 tests/cases/fourslash/importNameCodeFixNewImport1.ts create mode 100644 tests/cases/fourslash/importNameCodeFixNewImport2.ts create mode 100644 tests/cases/fourslash/importNameCodeFixNewImport3.ts create mode 100644 tests/cases/fourslash/importNameCodeFixNewImport4.ts create mode 100644 tests/cases/fourslash/importNameCodeFixNewImport5.ts create mode 100644 tests/cases/fourslash/importNameCodeFixNewImport6.ts create mode 100644 tests/cases/fourslash/importNameCodeFixNewImport7.ts create mode 100644 tests/cases/fourslash/importNameCodeFixNewImport8.ts create mode 100644 tests/cases/fourslash/importNameCodeFixNewImport9.ts diff --git a/src/compiler/diagnosticMessages.json b/src/compiler/diagnosticMessages.json index 21f04da488d9b..4412f0e850b8a 100644 --- a/src/compiler/diagnosticMessages.json +++ b/src/compiler/diagnosticMessages.json @@ -3112,5 +3112,17 @@ "Implement inherited abstract class": { "category": "Message", "code": 90007 + }, + "Import {0} from {1}": { + "category": "Message", + "code": 90008 + }, + "Change {0} to {1}": { + "category": "Message", + "code": 90009 + }, + "Add {0} to existing import declaration from {1}": { + "category": "Message", + "code": 90010 } } diff --git a/src/services/codefixes/codeFixProvider.ts b/src/services/codefixes/codeFixProvider.ts index c61cbe1b19ea5..e5a88acb18868 100644 --- a/src/services/codefixes/codeFixProvider.ts +++ b/src/services/codefixes/codeFixProvider.ts @@ -1,4 +1,4 @@ -/* @internal */ +/* @internal */ namespace ts { export interface CodeFix { errorCodes: number[]; @@ -11,6 +11,7 @@ namespace ts { span: TextSpan; program: Program; newLineCharacter: string; + host: LanguageServiceHost; } export namespace codefix { diff --git a/src/services/codefixes/fixes.ts b/src/services/codefixes/fixes.ts index d64a99ca1b9e6..e1a34431e448f 100644 --- a/src/services/codefixes/fixes.ts +++ b/src/services/codefixes/fixes.ts @@ -1 +1,2 @@ /// +/// diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts new file mode 100644 index 0000000000000..8dabcd7d0bc85 --- /dev/null +++ b/src/services/codefixes/importFixes.ts @@ -0,0 +1,284 @@ +/* @internal */ +namespace ts.codefix { + const nodeModulesDir = "node_modules"; + + registerCodeFix({ + errorCodes: [Diagnostics.Cannot_find_name_0.code], + getCodeActions: (context: CodeFixContext) => { + const sourceFile = context.sourceFile; + const checker = context.program.getTypeChecker(); + const allFiles = context.program.getSourceFiles(); + const readFile = context.host.readFile; + const useCaseSensitiveFileNames = context.host.useCaseSensitiveFileNames ? context.host.useCaseSensitiveFileNames() : false; + + const token = getTokenAtPosition(sourceFile, context.span.start); + const name = token.getText(); + const allActions: CodeAction[] = []; + + // Get existing ImportDeclarations from the source file + const imports: ImportDeclaration[] = []; + if (sourceFile.statements) { + for (const statement of sourceFile.statements) { + if (statement.kind === SyntaxKind.ImportDeclaration) { + imports.push(statement); + } + } + } + + // Check if a matching symbol is exported by any ambient modules that has been declared + const ambientModules = checker.getAmbientModules(); + for (const moduleSymbol of ambientModules) { + const exports = checker.getExportsOfModule(moduleSymbol) || []; + for (const exported of exports) { + if (exported.name === name) { + allActions.push(getCodeActionForImport(removeQuotes(moduleSymbol.getName()))); + } + } + } + + // Check if a matching symbol is exported by any files known to the compiler + for (const file of allFiles) { + const exports = file.symbol && file.symbol.exports; + if (exports) { + for (const exported in exports) { + if (exported === name) { + let moduleSpecifier: string; + const sourceDir = getDirectoryPath(sourceFile.fileName); + if (file.fileName.indexOf(nodeModulesDir) !== -1) { + moduleSpecifier = convertPathToModuleSpecifier(file.fileName, { sourceFile, readFile, useCaseSensitiveFileNames }); + } + else { + // Try and convert the file path into one relative to the source file + + const pathName = getRelativePathToDirectoryOrUrl( + sourceDir, + file.fileName, + sourceDir, + createGetCanonicalFileName(useCaseSensitiveFileNames), + /* isAbsolutePathAnUrl */ false + ); + + // Make sure we got back a path that can be a valid module specifier + const isRootedOrRelative = isExternalModuleNameRelative(pathName) || isRootedDiskPath(pathName); + moduleSpecifier = removeFileExtension(isRootedOrRelative ? pathName : combinePaths(".", pathName)); + } + + allActions.push(getCodeActionForImport(moduleSpecifier, (a, b) => + compareModuleSpecifiers(a, b, sourceDir, useCaseSensitiveFileNames) + )); + } + } + } + } + + return allActions; + + function getCodeActionForImport(moduleName: string, isEqual: (a: string, b: string) => Comparison = compareStrings): CodeAction { + // Check to see if there are already imports being made from this source in the current file + const existing = forEach(imports, (importDeclaration) => { + if (isEqual(removeQuotes(importDeclaration.moduleSpecifier.getText()), moduleName) === Comparison.EqualTo) { + return importDeclaration; + } + }); + + if (existing) { + return getCodeActionForExistingImport(existing); + } + + return getCodeActionForNewImport(); + + function getCodeActionForExistingImport(declaration: ImportDeclaration): CodeAction { + const moduleSpecifier = declaration.moduleSpecifier.getText(); + + // We have to handle all of the different import declaration forms + if (declaration.importClause) { + if (declaration.importClause.namedBindings) { + const namedBindings = declaration.importClause.namedBindings; + if (namedBindings.kind === SyntaxKind.NamespaceImport) { + /** + * Cases: + * import * as ns from "mod" + * import d, * as ns from "mod" + * + * Because there is no import list, we alter the reference to include the + * namespace instead of altering the import declaration. For example, "foo" would + * become "ns.foo" + */ + const ns = (namedBindings).name.getText(); + return createCodeAction( + Diagnostics.Change_0_to_1, + [name, `${ns}.${name}`], + `${ns}.`, + { start: token.getStart(), length: 0 }, + sourceFile.fileName + ); + } + else if (namedBindings.kind === SyntaxKind.NamedImports) { + /** + * Cases: + * import { a, b as x } from "mod" + * import d, { a, b as x } from "mod" + * + * Because there is already an import list, just insert the identifier into it + */ + const textChange = getTextChangeForImportList(namedBindings); + return createCodeAction( + Diagnostics.Add_0_to_existing_import_declaration_from_1, + [name, moduleSpecifier], + textChange.newText, + textChange.span, + sourceFile.fileName + ); + } + } + else if (declaration.importClause.name) { + /** + * Case: import d from "mod" + * + * Add a list of imports after the default import + */ + return createCodeAction( + Diagnostics.Add_0_to_existing_import_declaration_from_1, + [name, moduleSpecifier], + `, { ${name} }`, + { start: declaration.importClause.name.getEnd(), length: 0 }, + sourceFile.fileName + ); + } + + function getTextChangeForImportList(importList: NamedImports): TextChange { + if (importList.elements.length === 0) { + const start = importList.getStart(); + return { + newText: `{ ${name} }`, + span: { start, length: importList.getEnd() - start } + }; + } + + // Insert after the last element + const insertPoint = importList.elements[importList.elements.length - 1].getEnd(); + + // If the import list has one import per line, preserve that. Otherwise, insert on same line as last element + let oneImportPerLine: boolean; + + if (importList.elements.length === 1) { + /** + * If there is only one symbol being imported, still check to see if it's set up for multi-line imports like this: + * import { + * foo + * } from "./module"; + */ + const startLine = getLineOfLocalPosition(sourceFile, importList.getStart()); + const endLine = getLineOfLocalPosition(sourceFile, importList.getEnd()); + + oneImportPerLine = endLine - startLine >= 2; + } + else { + const startLine = getLineOfLocalPosition(sourceFile, importList.elements[0].getStart()); + const endLine = getLineOfLocalPosition(sourceFile, insertPoint); + + oneImportPerLine = endLine - startLine >= importList.elements.length - 1; + } + + return { + newText: oneImportPerLine ? `, ${context.newLineCharacter}${name}` : `,${name}`, + span: { start: insertPoint, length: 0 } + }; + } + + } + + return createCodeAction( + Diagnostics.Add_0_to_existing_import_declaration_from_1, + [name, moduleSpecifier], + `{ ${name} } from `, + { start: declaration.moduleSpecifier.getStart(), length: 0 }, + sourceFile.fileName + ); + } + + function getCodeActionForNewImport(): CodeAction { + // Try to insert after any existing imports + let lastDeclaration: ImportDeclaration; + let lastEnd: number; + for (const declaration of imports) { + const end = declaration.getEnd(); + if (!lastDeclaration || end > lastEnd) { + lastDeclaration = declaration; + lastEnd = end; + } + } + + let newText = `import { ${name} } from "${moduleName}";`; + newText = lastDeclaration ? context.newLineCharacter + newText : newText + context.newLineCharacter; + + return createCodeAction( + Diagnostics.Import_0_from_1, + [name, `"${moduleName}"`], + newText, + { start: lastDeclaration ? lastEnd : sourceFile.getStart(), length: 0 }, + sourceFile.fileName + ); + } + } + + function convertPathToModuleSpecifier(path: string, host: { sourceFile: SourceFile, readFile: any, useCaseSensitiveFileNames: boolean }): string { + const i = path.lastIndexOf(nodeModulesDir); + const moduleSpecifier = i !== -1 ? removeFileExtension(path.substring(i + nodeModulesDir.length)) : path; + + // If this is a node module, check to see if the given file is the main export of the module or not. If so, + // it can be referenced by just the module name. + if (i !== -1) { + const moduleDir = getDirectoryPath(path); + let nodePackage: any; + try { + nodePackage = JSON.parse(host.readFile(combinePaths(moduleDir, "package.json"))); + } + catch (e) { } + + // If no main export is explicitly defined, check for the default (index.js) + const mainExport = (nodePackage && nodePackage.main) || "index.js"; + const mainExportPath = isRootedDiskPath(mainExport) ? mainExport : combinePaths(moduleDir, mainExport); + + const baseDir = getDirectoryPath(host.sourceFile.fileName); + + if (compareModuleSpecifiers(path, mainExportPath, baseDir, host.useCaseSensitiveFileNames) === Comparison.EqualTo) { + return getDirectoryPath(moduleSpecifier); + } + } + + return moduleSpecifier; + } + + function removeQuotes(name: string): string { + if ((startsWith(name, "\"") && endsWith(name, "\"")) || (startsWith(name, "'") && endsWith(name, "'"))) { + return name.substr(1, name.length - 2); + } + else { + return name; + } + } + + function compareModuleSpecifiers(a: string, b: string, basePath: string, useCaseSensitiveFileNames: boolean): Comparison { + // Paths to modules can be relative or absolute and may optionally include the file + // extension of the module + a = removeFileExtension(a); + b = removeFileExtension(b); + return comparePaths(a, b, basePath, !useCaseSensitiveFileNames); + } + + function createCodeAction(description: DiagnosticMessage, diagnosticArgs: string[], newText: string, span: TextSpan, fileName: string): CodeAction { + return { + description: formatMessage.apply(undefined, [undefined, description].concat(diagnosticArgs)), + changes: [{ + fileName, + textChanges: [{ + newText, + span + }] + }] + }; + } + } + }); +} diff --git a/src/services/services.ts b/src/services/services.ts index 9ed73318d8e50..fd84acb5fe249 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -1,4 +1,4 @@ -/// +/// /// /// @@ -1676,7 +1676,8 @@ namespace ts { sourceFile: sourceFile, span: span, program: program, - newLineCharacter: newLineChar + newLineCharacter: newLineChar, + host: host }; const fixes = codefix.getFixes(context); diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport0.ts b/tests/cases/fourslash/importNameCodeFixExistingImport0.ts new file mode 100644 index 0000000000000..42f8737468f33 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixExistingImport0.ts @@ -0,0 +1,10 @@ +/// + +//// import [|{ v1 }|] from "./module"; +//// f1/*0*/(); + +// @Filename: module.ts +//// export function f1() {} +//// export var v1 = 5; + +verify.codeFixAtPosition(`{ v1, f1 }`); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport1.ts b/tests/cases/fourslash/importNameCodeFixExistingImport1.ts new file mode 100644 index 0000000000000..cc376025a736f --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixExistingImport1.ts @@ -0,0 +1,11 @@ +/// + +//// import d, [|{ v1 }|] from "./module"; +//// f1/*0*/(); + +// @Filename: module.ts +//// export function f1() {} +//// export var v1 = 5; +//// export default var d1 = 6; + +verify.codeFixAtPosition(`{ v1, f1 }`); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport10.ts b/tests/cases/fourslash/importNameCodeFixExistingImport10.ts new file mode 100644 index 0000000000000..e78b279e178da --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixExistingImport10.ts @@ -0,0 +1,21 @@ +/// + +//// import [|{ +//// v1, +//// v2 +//// }|] from "./module"; +//// f1/*0*/(); + +// @Filename: module.ts +//// export function f1() {} +//// export var v1 = 5; +//// export var v2 = 5; +//// export var v3 = 5; + +verify.codeFixAtPosition( +`{ + v1, + v2, + f1 + }` +); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport11.ts b/tests/cases/fourslash/importNameCodeFixExistingImport11.ts new file mode 100644 index 0000000000000..0030ad9f3b320 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixExistingImport11.ts @@ -0,0 +1,20 @@ +/// + +//// import [|{ +//// v1, v2, +//// v3 +//// }|] from "./module"; +//// f1/*0*/(); + +// @Filename: module.ts +//// export function f1() {} +//// export var v1 = 5; +//// export var v2 = 5; +//// export var v3 = 5; + +verify.codeFixAtPosition( +`{ + v1, v2, + v3, f1 + }` +); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport12.ts b/tests/cases/fourslash/importNameCodeFixExistingImport12.ts new file mode 100644 index 0000000000000..d5d2feeb8c17c --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixExistingImport12.ts @@ -0,0 +1,12 @@ +/// + +//// import [|{}|] from "./module"; +//// f1/*0*/(); + +// @Filename: module.ts +//// export function f1() {} +//// export var v1 = 5; +//// export var v2 = 5; +//// export var v3 = 5; + +verify.codeFixAtPosition(`{ f1 }`); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport2.ts b/tests/cases/fourslash/importNameCodeFixExistingImport2.ts new file mode 100644 index 0000000000000..179d80a7ecfe1 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixExistingImport2.ts @@ -0,0 +1,10 @@ +/// + +//// import * as ns from "./module"; +//// [|f1|]/*0*/(); + +// @Filename: module.ts +//// export function f1() {} +//// export var v1 = 5; + +verify.codeFixAtPosition(`ns.f1`); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport3.ts b/tests/cases/fourslash/importNameCodeFixExistingImport3.ts new file mode 100644 index 0000000000000..34773cac9d563 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixExistingImport3.ts @@ -0,0 +1,11 @@ +/// + +//// import d, * as ns from "./module"; +//// [|f1|]/*0*/(); + +// @Filename: module.ts +//// export function f1() {} +//// export var v1 = 5; +//// export default var d1 = 6; + +verify.codeFixAtPosition(`ns.f1`); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport4.ts b/tests/cases/fourslash/importNameCodeFixExistingImport4.ts new file mode 100644 index 0000000000000..a56cab1337d78 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixExistingImport4.ts @@ -0,0 +1,11 @@ +/// + +//// [|import d from "./module"|]; +//// f1/*0*/(); + +// @Filename: module.ts +//// export function f1() {} +//// export var v1 = 5; +//// export default var d1 = 6; + +verify.codeFixAtPosition(`import d, { f1 } from "./module"`); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport5.ts b/tests/cases/fourslash/importNameCodeFixExistingImport5.ts new file mode 100644 index 0000000000000..4fef16e7dc16d --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixExistingImport5.ts @@ -0,0 +1,10 @@ +/// + +//// [|import "./module";|] +//// f1/*0*/(); + +// @Filename: module.ts +//// export function f1() {} +//// export var v1 = 5; + +verify.codeFixAtPosition(`import { f1 } from "./module";`); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport6.ts b/tests/cases/fourslash/importNameCodeFixExistingImport6.ts new file mode 100644 index 0000000000000..141a8cac9468a --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixExistingImport6.ts @@ -0,0 +1,13 @@ +/// + +//// import [|{ v1 }|] from "fake-module"; +//// f1/*0*/(); + +// @Filename: ../package.json +//// { "dependencies": { "fake-module": "latest" } } + +// @Filename: ../node_modules/fake-module/index.ts +//// export var v1 = 5; +//// export function f1(); + +verify.codeFixAtPosition(`{ v1, f1 }`); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport7.ts b/tests/cases/fourslash/importNameCodeFixExistingImport7.ts new file mode 100644 index 0000000000000..56a172ecfe0c1 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixExistingImport7.ts @@ -0,0 +1,10 @@ +/// + +//// import [|{ v1 }|] from "../other_dir/module"; +//// f1/*0*/(); + +// @Filename: ../other_dir/module.ts +//// export var v1 = 5; +//// export function f1(); + +verify.codeFixAtPosition(`{ v1, f1 }`); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport8.ts b/tests/cases/fourslash/importNameCodeFixExistingImport8.ts new file mode 100644 index 0000000000000..971e90e7056b2 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixExistingImport8.ts @@ -0,0 +1,12 @@ +/// + +//// import [|{v1, v2, v3,}|] from "./module"; +//// f1/*0*/(); + +// @Filename: module.ts +//// export function f1() {} +//// export var v1 = 5; +//// export var v2 = 5; +//// export var v3 = 5; + +verify.codeFixAtPosition(`{v1, v2, v3, f1,}`); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport9.ts b/tests/cases/fourslash/importNameCodeFixExistingImport9.ts new file mode 100644 index 0000000000000..6d0508e9f4fbb --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixExistingImport9.ts @@ -0,0 +1,17 @@ +/// + +//// import [|{ +//// v1 +//// }|] from "./module"; +//// f1/*0*/(); + +// @Filename: module.ts +//// export function f1() {} +//// export var v1 = 5; + +verify.codeFixAtPosition( +`{ + v1, + f1 + }` +); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImport0.ts b/tests/cases/fourslash/importNameCodeFixNewImport0.ts new file mode 100644 index 0000000000000..1b0d4b3ce1904 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImport0.ts @@ -0,0 +1,14 @@ +/// + +//// [|f1/*0*/();|] + +// @Filename: ambientModule.ts +//// declare module "ambient-module" { +//// export function f1(); +//// export var v1; +//// } + +verify.codeFixAtPosition( +`import { f1 } from "ambient-module"; +f1();` +); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImport1.ts b/tests/cases/fourslash/importNameCodeFixNewImport1.ts new file mode 100644 index 0000000000000..469ac353f0e7a --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImport1.ts @@ -0,0 +1,28 @@ +/// + +//// import d from "other-ambient-module"; +//// [|import * as ns from "yet-another-ambient-module"; +//// var x = v1/*0*/ + 5;|] + +// @Filename: ambientModule.ts +//// declare module "ambient-module" { +//// export function f1(); +//// export var v1; +//// } + +// @Filename: otherAmbientModule.ts +//// declare module "other-ambient-module" { +//// export default function f2(); +//// } + +// @Filename: yetAnotherAmbientModule.ts +//// declare module "yet-another-ambient-module" { +//// export function f3(); +//// export var v3; +//// } + +verify.codeFixAtPosition( +`import * as ns from "yet-another-ambient-module"; +import { v1 } from "ambient-module"; +var x = v1 + 5;` +); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImport2.ts b/tests/cases/fourslash/importNameCodeFixNewImport2.ts new file mode 100644 index 0000000000000..a809ba014c26e --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImport2.ts @@ -0,0 +1,20 @@ +/// + +//// [|/* +//// * I'm a license or something +//// */ +//// f1/*0*/();|] + +// @Filename: ambientModule.ts +//// declare module "ambient-module" { +//// export function f1(); +//// export var v1; +//// } + +verify.codeFixAtPosition( +`/* +* I'm a license or something +*/ +import { f1 } from "ambient-module"; +f1();` +); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImport3.ts b/tests/cases/fourslash/importNameCodeFixNewImport3.ts new file mode 100644 index 0000000000000..3e8dcc1ae0818 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImport3.ts @@ -0,0 +1,12 @@ +/// + +//// [|f1/*0*/();|] + +// @Filename: module.ts +//// export function f1() {} +//// export var v1 = 5; + +verify.codeFixAtPosition( +`import { f1 } from "./module"; +f1();` +); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImport4.ts b/tests/cases/fourslash/importNameCodeFixNewImport4.ts new file mode 100644 index 0000000000000..20ec7ff793fd9 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImport4.ts @@ -0,0 +1,17 @@ +/// + +//// [|/// +//// f1/*0*/();|] + +// @Filename: module.ts +//// export function f1() {} +//// export var v1 = 5; + +// @Filename: tripleSlashReference.ts +//// var x = 5;/*dummy*/ + +verify.codeFixAtPosition( +`/// +import { f1 } from "./module"; +f1();` +); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImport5.ts b/tests/cases/fourslash/importNameCodeFixNewImport5.ts new file mode 100644 index 0000000000000..d41594b1355c7 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImport5.ts @@ -0,0 +1,12 @@ +/// + +//// [|f1/*0*/();|] + +// @Filename: ../../other_dir/module.ts +//// export var v1 = 5; +//// export function f1(); + +verify.codeFixAtPosition( +`import { f1 } from "../../other_dir/module"; +f1();` +); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImport6.ts b/tests/cases/fourslash/importNameCodeFixNewImport6.ts new file mode 100644 index 0000000000000..7a99663657b6f --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImport6.ts @@ -0,0 +1,18 @@ +/// + +//// [|f1/*0*/();|] + +// @Filename: ../package.json +//// { "dependencies": { "fake-module": "latest" } } + +// @Filename: ../node_modules/fake-module/index.ts +//// export var v1 = 5; +//// export function f1(); + +// @Filename: ../node_modules/fake-module/package.json +//// {} + +verify.codeFixAtPosition( +`import { f1 } from "fake-module"; +f1();` +); diff --git a/tests/cases/fourslash/importNameCodeFixNewImport7.ts b/tests/cases/fourslash/importNameCodeFixNewImport7.ts new file mode 100644 index 0000000000000..88dead5339fd0 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImport7.ts @@ -0,0 +1,15 @@ +/// + +//// [|f1/*0*/();|] + +// @Filename: ../package.json +//// { "dependencies": { "fake-module": "latest" } } + +// @Filename: ../node_modules/fake-module/nested.ts +//// export var v1 = 5; +//// export function f1(); + +verify.codeFixAtPosition( +`import { f1 } from "fake-module/nested"; +f1();` +); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImport8.ts b/tests/cases/fourslash/importNameCodeFixNewImport8.ts new file mode 100644 index 0000000000000..5ccafa9141188 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImport8.ts @@ -0,0 +1,30 @@ +/// + +//// let a = "I am a non-trivial statement that appears before imports"; +//// import d from "other-ambient-module"; +//// [|import * as ns from "yet-another-ambient-module"; +//// var x = v1/*0*/ + 5;|] + +// @Filename: ambientModule.ts +//// declare module "ambient-module" { +//// export function f1(); +//// export var v1; +//// } + +// @Filename: otherAmbientModule.ts +//// declare module "other-ambient-module" { +//// export default function f2(); +//// } + +// @Filename: yetAnotherAmbientModule.ts +//// declare module "yet-another-ambient-module" { +//// export function f3(); +//// export var v3; +//// } + +verify.codeFixAtPosition( +` +import * as ns from "yet-another-ambient-module"; +import { v1 } from "ambient-module"; +var x = v1 + 5;` +); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImport9.ts b/tests/cases/fourslash/importNameCodeFixNewImport9.ts new file mode 100644 index 0000000000000..5629d5ad188e5 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImport9.ts @@ -0,0 +1,24 @@ +/// + +//// [|f1/*0*/();|] + +// @Filename: ../package.json +//// { "dependencies": { "fake-module": "latest" } } + +// @Filename: ../node_modules/fake-module/notindex.d.ts +//// export var v1 = 5; +//// export function f1(); + +// @Filename: ../node_modules/fake-module/notindex.js +//// module.exports = { +//// v1: 5, +//// f1: function () {} +//// }; + +// @Filename: ../node_modules/fake-module/package.json +//// { "main":"./notindex.js", "typings":"./notindex.d.ts" } + +verify.codeFixAtPosition( +`import { f1 } from "fake-module"; +f1();` +); \ No newline at end of file From eb468861f8b1d2f88628e8899817a9c77120065f Mon Sep 17 00:00:00 2001 From: Paul van Brenk Date: Thu, 20 Oct 2016 16:47:01 -0700 Subject: [PATCH 02/14] Re-order and cleanup --- src/services/codefixes/importFixes.ts | 73 +++++++++++++++------------ 1 file changed, 40 insertions(+), 33 deletions(-) diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 8dabcd7d0bc85..bba4fb6de83c4 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -8,7 +8,6 @@ namespace ts.codefix { const sourceFile = context.sourceFile; const checker = context.program.getTypeChecker(); const allFiles = context.program.getSourceFiles(); - const readFile = context.host.readFile; const useCaseSensitiveFileNames = context.host.useCaseSensitiveFileNames ? context.host.useCaseSensitiveFileNames() : false; const token = getTokenAtPosition(sourceFile, context.span.start); @@ -45,7 +44,7 @@ namespace ts.codefix { let moduleSpecifier: string; const sourceDir = getDirectoryPath(sourceFile.fileName); if (file.fileName.indexOf(nodeModulesDir) !== -1) { - moduleSpecifier = convertPathToModuleSpecifier(file.fileName, { sourceFile, readFile, useCaseSensitiveFileNames }); + moduleSpecifier = convertPathToModuleSpecifier(file.fileName); } else { // Try and convert the file path into one relative to the source file @@ -73,6 +72,38 @@ namespace ts.codefix { return allActions; + function convertPathToModuleSpecifier(modulePath: string): string { + const i = modulePath.lastIndexOf(nodeModulesDir); + + // This is not a module from the node_module folder, + // so just retun the path + if (i === -1) { + return modulePath; + } + + // If this is a node module, check to see if the given file is the main export of the module or not. If so, + // it can be referenced by just the module name. + const moduleDir = getDirectoryPath(modulePath); + let nodePackage: any; + try { + nodePackage = JSON.parse(context.host.readFile(combinePaths(moduleDir, "package.json"))); + } + catch (e) { } + + // If no main export is explicitly defined, check for the default (index.js) + const mainExport = (nodePackage && nodePackage.main) || "index.js"; + const mainExportPath = normalizePath(isRootedDiskPath(mainExport) ? mainExport : combinePaths(moduleDir, mainExport)); + + const moduleSpecifier = removeFileExtension(modulePath.substring(i + nodeModulesDir.length + 1)); + + if (compareModuleSpecifiers(modulePath, mainExportPath, getDirectoryPath(sourceFile.fileName), useCaseSensitiveFileNames) === Comparison.EqualTo) { + return getDirectoryPath(moduleSpecifier); + } + else { + return moduleSpecifier; + } + } + function getCodeActionForImport(moduleName: string, isEqual: (a: string, b: string) => Comparison = compareStrings): CodeAction { // Check to see if there are already imports being made from this source in the current file const existing = forEach(imports, (importDeclaration) => { @@ -84,8 +115,9 @@ namespace ts.codefix { if (existing) { return getCodeActionForExistingImport(existing); } - - return getCodeActionForNewImport(); + else { + return getCodeActionForNewImport(); + } function getCodeActionForExistingImport(declaration: ImportDeclaration): CodeAction { const moduleSpecifier = declaration.moduleSpecifier.getText(); @@ -216,39 +248,14 @@ namespace ts.codefix { Diagnostics.Import_0_from_1, [name, `"${moduleName}"`], newText, - { start: lastDeclaration ? lastEnd : sourceFile.getStart(), length: 0 }, + { + start: lastDeclaration ? lastEnd : sourceFile.getStart(), + length: 0 + }, sourceFile.fileName ); } } - - function convertPathToModuleSpecifier(path: string, host: { sourceFile: SourceFile, readFile: any, useCaseSensitiveFileNames: boolean }): string { - const i = path.lastIndexOf(nodeModulesDir); - const moduleSpecifier = i !== -1 ? removeFileExtension(path.substring(i + nodeModulesDir.length)) : path; - - // If this is a node module, check to see if the given file is the main export of the module or not. If so, - // it can be referenced by just the module name. - if (i !== -1) { - const moduleDir = getDirectoryPath(path); - let nodePackage: any; - try { - nodePackage = JSON.parse(host.readFile(combinePaths(moduleDir, "package.json"))); - } - catch (e) { } - - // If no main export is explicitly defined, check for the default (index.js) - const mainExport = (nodePackage && nodePackage.main) || "index.js"; - const mainExportPath = isRootedDiskPath(mainExport) ? mainExport : combinePaths(moduleDir, mainExport); - - const baseDir = getDirectoryPath(host.sourceFile.fileName); - - if (compareModuleSpecifiers(path, mainExportPath, baseDir, host.useCaseSensitiveFileNames) === Comparison.EqualTo) { - return getDirectoryPath(moduleSpecifier); - } - } - - return moduleSpecifier; - } function removeQuotes(name: string): string { if ((startsWith(name, "\"") && endsWith(name, "\"")) || (startsWith(name, "'") && endsWith(name, "'"))) { From 80d7f5c3cad8cf0fdc650a4dd65fe997ebc9dc4d Mon Sep 17 00:00:00 2001 From: zhengbli Date: Wed, 2 Nov 2016 23:41:26 -0700 Subject: [PATCH 03/14] refactor --- src/services/codefixes/codeFixProvider.ts | 6 +- src/services/codefixes/importFixes.ts | 196 ++++++++-------------- src/services/services.ts | 2 +- 3 files changed, 78 insertions(+), 126 deletions(-) diff --git a/src/services/codefixes/codeFixProvider.ts b/src/services/codefixes/codeFixProvider.ts index e5a88acb18868..a3d0766bc64c4 100644 --- a/src/services/codefixes/codeFixProvider.ts +++ b/src/services/codefixes/codeFixProvider.ts @@ -2,7 +2,7 @@ namespace ts { export interface CodeFix { errorCodes: number[]; - getCodeActions(context: CodeFixContext): CodeAction[] | undefined; + getCodeActions(context: CodeFixContext, cancellationToken?: CancellationToken): CodeAction[] | undefined; } export interface CodeFixContext { @@ -32,12 +32,12 @@ namespace ts { return Object.keys(codeFixes); } - export function getFixes(context: CodeFixContext): CodeAction[] { + export function getFixes(context: CodeFixContext, cancellationToken: CancellationToken): CodeAction[] { const fixes = codeFixes[context.errorCode]; let allActions: CodeAction[] = []; forEach(fixes, f => { - const actions = f.getCodeActions(context); + const actions = f.getCodeActions(context, cancellationToken); if (actions && actions.length > 0) { allActions = allActions.concat(actions); } diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index bba4fb6de83c4..4484b950d9e81 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -1,124 +1,62 @@ /* @internal */ namespace ts.codefix { - const nodeModulesDir = "node_modules"; + const nodeModulesFolderName = "node_modules"; registerCodeFix({ errorCodes: [Diagnostics.Cannot_find_name_0.code], - getCodeActions: (context: CodeFixContext) => { + getCodeActions: (context: CodeFixContext, cancellationToken: CancellationToken) => { const sourceFile = context.sourceFile; const checker = context.program.getTypeChecker(); - const allFiles = context.program.getSourceFiles(); + const allSourceFiles = context.program.getSourceFiles(); const useCaseSensitiveFileNames = context.host.useCaseSensitiveFileNames ? context.host.useCaseSensitiveFileNames() : false; const token = getTokenAtPosition(sourceFile, context.span.start); const name = token.getText(); const allActions: CodeAction[] = []; - // Get existing ImportDeclarations from the source file - const imports: ImportDeclaration[] = []; - if (sourceFile.statements) { - for (const statement of sourceFile.statements) { - if (statement.kind === SyntaxKind.ImportDeclaration) { - imports.push(statement); - } - } - } + const allPotentialModules = concatenate(checker.getAmbientModules(), map(allSourceFiles, sf => sf.symbol)); + + for (const moduleSymbol of allPotentialModules) { + cancellationToken.throwIfCancellationRequested(); - // Check if a matching symbol is exported by any ambient modules that has been declared - const ambientModules = checker.getAmbientModules(); - for (const moduleSymbol of ambientModules) { const exports = checker.getExportsOfModule(moduleSymbol) || []; for (const exported of exports) { if (exported.name === name) { - allActions.push(getCodeActionForImport(removeQuotes(moduleSymbol.getName()))); - } - } - } - - // Check if a matching symbol is exported by any files known to the compiler - for (const file of allFiles) { - const exports = file.symbol && file.symbol.exports; - if (exports) { - for (const exported in exports) { - if (exported === name) { - let moduleSpecifier: string; - const sourceDir = getDirectoryPath(sourceFile.fileName); - if (file.fileName.indexOf(nodeModulesDir) !== -1) { - moduleSpecifier = convertPathToModuleSpecifier(file.fileName); - } - else { - // Try and convert the file path into one relative to the source file - - const pathName = getRelativePathToDirectoryOrUrl( - sourceDir, - file.fileName, - sourceDir, - createGetCanonicalFileName(useCaseSensitiveFileNames), - /* isAbsolutePathAnUrl */ false - ); - - // Make sure we got back a path that can be a valid module specifier - const isRootedOrRelative = isExternalModuleNameRelative(pathName) || isRootedDiskPath(pathName); - moduleSpecifier = removeFileExtension(isRootedOrRelative ? pathName : combinePaths(".", pathName)); - } - - allActions.push(getCodeActionForImport(moduleSpecifier, (a, b) => - compareModuleSpecifiers(a, b, sourceDir, useCaseSensitiveFileNames) - )); - } + allActions.push(getCodeActionForImport(moduleSymbol)); } } } return allActions; - function convertPathToModuleSpecifier(modulePath: string): string { - const i = modulePath.lastIndexOf(nodeModulesDir); - - // This is not a module from the node_module folder, - // so just retun the path - if (i === -1) { - return modulePath; - } - - // If this is a node module, check to see if the given file is the main export of the module or not. If so, - // it can be referenced by just the module name. - const moduleDir = getDirectoryPath(modulePath); - let nodePackage: any; - try { - nodePackage = JSON.parse(context.host.readFile(combinePaths(moduleDir, "package.json"))); - } - catch (e) { } - - // If no main export is explicitly defined, check for the default (index.js) - const mainExport = (nodePackage && nodePackage.main) || "index.js"; - const mainExportPath = normalizePath(isRootedDiskPath(mainExport) ? mainExport : combinePaths(moduleDir, mainExport)); - - const moduleSpecifier = removeFileExtension(modulePath.substring(i + nodeModulesDir.length + 1)); - - if (compareModuleSpecifiers(modulePath, mainExportPath, getDirectoryPath(sourceFile.fileName), useCaseSensitiveFileNames) === Comparison.EqualTo) { - return getDirectoryPath(moduleSpecifier); - } - else { - return moduleSpecifier; - } - } - - function getCodeActionForImport(moduleName: string, isEqual: (a: string, b: string) => Comparison = compareStrings): CodeAction { + function getCodeActionForImport(moduleSymbol: Symbol): CodeAction { // Check to see if there are already imports being made from this source in the current file - const existing = forEach(imports, (importDeclaration) => { - if (isEqual(removeQuotes(importDeclaration.moduleSpecifier.getText()), moduleName) === Comparison.EqualTo) { - return importDeclaration; + const existingDeclaration = forEach(sourceFile.imports, importModuleSpecifier => { + const importSymbol = checker.getSymbolAtLocation(importModuleSpecifier); + if (importSymbol === moduleSymbol) { + return getImportDeclaration(importModuleSpecifier); } }); - if (existing) { - return getCodeActionForExistingImport(existing); + if (existingDeclaration) { + return getCodeActionForExistingImport(existingDeclaration); } else { return getCodeActionForNewImport(); } + function getImportDeclaration(moduleSpecifier: LiteralExpression) { + let node: Node = moduleSpecifier; + while (node) { + if (node.kind !== SyntaxKind.ImportDeclaration) { + node = node.parent; + } + + return node; + } + return undefined; + } + function getCodeActionForExistingImport(declaration: ImportDeclaration): CodeAction { const moduleSpecifier = declaration.moduleSpecifier.getText(); @@ -231,59 +169,73 @@ namespace ts.codefix { function getCodeActionForNewImport(): CodeAction { // Try to insert after any existing imports - let lastDeclaration: ImportDeclaration; - let lastEnd: number; - for (const declaration of imports) { - const end = declaration.getEnd(); - if (!lastDeclaration || end > lastEnd) { - lastDeclaration = declaration; - lastEnd = end; + let lastModuleSpecifierEnd = -1; + for (const moduleSpecifier of sourceFile.imports) { + const end = moduleSpecifier.getEnd(); + if (!lastModuleSpecifierEnd || end > lastModuleSpecifierEnd) { + lastModuleSpecifierEnd = end; } } - let newText = `import { ${name} } from "${moduleName}";`; - newText = lastDeclaration ? context.newLineCharacter + newText : newText + context.newLineCharacter; + const moduleSpecifier = getModuleSpecifierForNewImport(); + let newText = `import { ${name} } from "${moduleSpecifier}";`; + newText = lastModuleSpecifierEnd ? context.newLineCharacter + newText : newText + context.newLineCharacter; return createCodeAction( Diagnostics.Import_0_from_1, - [name, `"${moduleName}"`], + [name, `"${moduleSpecifier}"`], newText, { - start: lastDeclaration ? lastEnd : sourceFile.getStart(), + start: lastModuleSpecifierEnd >= 0 ? lastModuleSpecifierEnd + 1 : sourceFile.getStart(), length: 0 }, sourceFile.fileName ); } - } - function removeQuotes(name: string): string { - if ((startsWith(name, "\"") && endsWith(name, "\"")) || (startsWith(name, "'") && endsWith(name, "'"))) { - return name.substr(1, name.length - 2); - } - else { - return name; - } - } + function getModuleSpecifierForNewImport(): string { + const sourceDir = getDirectoryPath(sourceFile.fileName); + + // If the module is from a module file, then there are typically two cases: + // 1. from a source file in your program + // 2. from a declaration file in a node_modules folder: + // 2.1 the node_modules folder is in the sourceDir or above (can be found by the module resolution) + // 2.2 the node_modules folder is in a subfolder of the sourceDir (cannot be found by the module resolution) + // for case 1 and 2.2, we would return the relative file path as the module specifier; + // for case 2.1, we would just use the module name instead. + if (moduleSymbol.valueDeclaration.kind === SyntaxKind.SourceFile) { + const moduleFileName = moduleSymbol.valueDeclaration.getSourceFile().fileName; + + // case 2.1 + if (moduleFileName.indexOf(combinePaths(sourceDir, nodeModulesFolderName)) === 0 || + (moduleFileName.indexOf(sourceDir) < 0 && moduleFileName.indexOf(`${directorySeparator}${nodeModulesFolderName}${directorySeparator}`) >= 0)) { + return moduleSymbol.name; + } - function compareModuleSpecifiers(a: string, b: string, basePath: string, useCaseSensitiveFileNames: boolean): Comparison { - // Paths to modules can be relative or absolute and may optionally include the file - // extension of the module - a = removeFileExtension(a); - b = removeFileExtension(b); - return comparePaths(a, b, basePath, !useCaseSensitiveFileNames); + // case 1 and case 2.2 + const relativePath = getRelativePathToDirectoryOrUrl( + sourceDir, + moduleFileName, + /*currentDirectory*/ sourceDir, + createGetCanonicalFileName(useCaseSensitiveFileNames), + /*isAbsolutePathAnUrl*/ false + ); + + // Make sure we got back a path that can be a valid module specifier + const isRootedOrRelative = isExternalModuleNameRelative(relativePath) || isRootedDiskPath(relativePath); + return removeFileExtension(isRootedOrRelative ? relativePath : combinePaths(".", relativePath)); + + } + + // the module is not from a module file, so just return the module name. + return moduleSymbol.name; + } } function createCodeAction(description: DiagnosticMessage, diagnosticArgs: string[], newText: string, span: TextSpan, fileName: string): CodeAction { return { description: formatMessage.apply(undefined, [undefined, description].concat(diagnosticArgs)), - changes: [{ - fileName, - textChanges: [{ - newText, - span - }] - }] + changes: [{ fileName, textChanges: [{ newText, span }] }] }; } } diff --git a/src/services/services.ts b/src/services/services.ts index fd84acb5fe249..1b822f954699a 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -1680,7 +1680,7 @@ namespace ts { host: host }; - const fixes = codefix.getFixes(context); + const fixes = codefix.getFixes(context, cancellationToken); if (fixes) { allFixes = allFixes.concat(fixes); } From 1502f75c515d9704989d1e56ef28689e8cfcbb3f Mon Sep 17 00:00:00 2001 From: zhengbli Date: Thu, 3 Nov 2016 01:28:50 -0700 Subject: [PATCH 04/14] make tests pass --- src/services/codefixes/importFixes.ts | 96 ++++++++++++++++++--------- 1 file changed, 66 insertions(+), 30 deletions(-) diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 4484b950d9e81..e3cb359bd6d17 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -13,8 +13,17 @@ namespace ts.codefix { const token = getTokenAtPosition(sourceFile, context.span.start); const name = token.getText(); const allActions: CodeAction[] = []; + let allPotentialModules: Symbol[] = []; - const allPotentialModules = concatenate(checker.getAmbientModules(), map(allSourceFiles, sf => sf.symbol)); + const ambientModules = checker.getAmbientModules(); + if (ambientModules) { + allPotentialModules = ambientModules; + } + for (const otherSourceFile of allSourceFiles) { + if (otherSourceFile !== sourceFile && otherSourceFile.symbol) { + allPotentialModules.push(otherSourceFile.symbol); + } + } for (const moduleSymbol of allPotentialModules) { cancellationToken.throwIfCancellationRequested(); @@ -191,44 +200,71 @@ namespace ts.codefix { }, sourceFile.fileName ); - } - function getModuleSpecifierForNewImport(): string { - const sourceDir = getDirectoryPath(sourceFile.fileName); - - // If the module is from a module file, then there are typically two cases: - // 1. from a source file in your program - // 2. from a declaration file in a node_modules folder: - // 2.1 the node_modules folder is in the sourceDir or above (can be found by the module resolution) - // 2.2 the node_modules folder is in a subfolder of the sourceDir (cannot be found by the module resolution) - // for case 1 and 2.2, we would return the relative file path as the module specifier; - // for case 2.1, we would just use the module name instead. - if (moduleSymbol.valueDeclaration.kind === SyntaxKind.SourceFile) { - const moduleFileName = moduleSymbol.valueDeclaration.getSourceFile().fileName; - - // case 2.1 - if (moduleFileName.indexOf(combinePaths(sourceDir, nodeModulesFolderName)) === 0 || - (moduleFileName.indexOf(sourceDir) < 0 && moduleFileName.indexOf(`${directorySeparator}${nodeModulesFolderName}${directorySeparator}`) >= 0)) { - return moduleSymbol.name; + function getModuleSpecifierForNewImport(): string { + if (moduleSymbol.valueDeclaration.kind !== SyntaxKind.SourceFile) { + return stripQuotes(moduleSymbol.name); } - // case 1 and case 2.2 - const relativePath = getRelativePathToDirectoryOrUrl( - sourceDir, - moduleFileName, + // If the module is from a module file, then there are typically several cases: + // + // 1. from a source file in your program (file path has no node_modules) + // 2. from a file in a node_modules folder: + // 2.1 the node_modules folder is in a subfolder of the sourceDir (cannot be found by the module resolution) + // 2.2 the node_modules folder is in the sourceDir or above (can be found by the module resolution) + // 2.2.1 the module file is the "main" file in package.json (or "index.js" if not specified) + // 2.2.2 the module file is not the "main" file + // + // for case 1 and 2.2, we would return the relative file path as the module specifier; + // for case 2.1, we would just use the module name instead. + const sourceDir = getDirectoryPath(sourceFile.fileName); + const modulePath = (moduleSymbol.valueDeclaration).fileName; + + const i = modulePath.lastIndexOf(nodeModulesFolderName); + + // case 1 and case 2.1: return the relative file path as the module specifier; + if (i === -1 || (modulePath.indexOf(sourceDir) === 0 && modulePath.indexOf(combinePaths(sourceDir, nodeModulesFolderName)) === -1)) { + const relativePath = getRelativePathToDirectoryOrUrl( + sourceDir, + modulePath, /*currentDirectory*/ sourceDir, - createGetCanonicalFileName(useCaseSensitiveFileNames), + createGetCanonicalFileName(useCaseSensitiveFileNames), /*isAbsolutePathAnUrl*/ false - ); + ); + const isRootedOrRelative = isExternalModuleNameRelative(relativePath) || isRootedDiskPath(relativePath); + return removeFileExtension(isRootedOrRelative ? relativePath : combinePaths(".", relativePath)); + } - // Make sure we got back a path that can be a valid module specifier - const isRootedOrRelative = isExternalModuleNameRelative(relativePath) || isRootedDiskPath(relativePath); - return removeFileExtension(isRootedOrRelative ? relativePath : combinePaths(".", relativePath)); + // case 2.2 + // If this is a node module, check to see if the given file is the main export of the module or not. If so, + // it can be referenced by just the module name. + const moduleDir = getDirectoryPath(modulePath); + let nodePackage: any; + try { + nodePackage = JSON.parse(context.host.readFile(combinePaths(moduleDir, "package.json"))); + } + catch (e) { } + + // If no main export is explicitly defined, check for the default (index.js) + const mainExport = (nodePackage && nodePackage.main) || "index.js"; + const mainExportPath = normalizePath(isRootedDiskPath(mainExport) ? mainExport : combinePaths(moduleDir, mainExport)); + const moduleSpecifier = removeFileExtension(modulePath.substring(i + nodeModulesFolderName.length + 1)); + if (areModuleSpecifiersEqual(modulePath, mainExportPath)) { + return getDirectoryPath(moduleSpecifier); + } + else { + return moduleSpecifier; + } } + } - // the module is not from a module file, so just return the module name. - return moduleSymbol.name; + function areModuleSpecifiersEqual(a: string, b: string): boolean { + // Paths to modules can be relative or absolute and may optionally include the file + // extension of the module + a = removeFileExtension(a); + b = removeFileExtension(b); + return comparePaths(a, b, getDirectoryPath(sourceFile.fileName), !useCaseSensitiveFileNames) === Comparison.EqualTo; } } From e72f0c83e4a91e6341647ebcc1d1c5c0159fbe04 Mon Sep 17 00:00:00 2001 From: zhengbli Date: Mon, 7 Nov 2016 23:23:41 -0800 Subject: [PATCH 05/14] Make import specifier for new imports more comprehensive --- src/compiler/checker.ts | 3 +- src/compiler/core.ts | 8 + src/compiler/moduleNameResolver.ts | 2 +- src/compiler/types.ts | 5 +- src/harness/fourslash.ts | 2 +- src/services/codefixes/codeFixProvider.ts | 7 +- src/services/codefixes/importFixes.ts | 442 ++++++++++-------- src/services/completions.ts | 4 +- src/services/services.ts | 5 +- ... => importNameCodeFixNewImportAmbient0.ts} | 0 ... => importNameCodeFixNewImportAmbient1.ts} | 0 ... => importNameCodeFixNewImportAmbient2.ts} | 0 ... => importNameCodeFixNewImportAmbient3.ts} | 0 .../importNameCodeFixNewImportBaseUrl0.ts | 18 + .../importNameCodeFixNewImportDefault0.ts | 11 + ....ts => importNameCodeFixNewImportFile0.ts} | 0 ....ts => importNameCodeFixNewImportFile1.ts} | 0 ....ts => importNameCodeFixNewImportFile2.ts} | 0 ...importNameCodeFixNewImportNodeModules0.ts} | 0 ...importNameCodeFixNewImportNodeModules1.ts} | 0 ...importNameCodeFixNewImportNodeModules2.ts} | 0 .../importNameCodeFixNewImportNodeModules3.ts | 13 + .../importNameCodeFixNewImportPaths0.ts | 21 + .../importNameCodeFixNewImportPaths1.ts | 21 + .../importNameCodeFixNewImportRootDirs0.ts | 22 + .../importNameCodeFixNewImportTypeRoots0.ts | 21 + 26 files changed, 407 insertions(+), 198 deletions(-) rename tests/cases/fourslash/{importNameCodeFixNewImport0.ts => importNameCodeFixNewImportAmbient0.ts} (100%) rename tests/cases/fourslash/{importNameCodeFixNewImport1.ts => importNameCodeFixNewImportAmbient1.ts} (100%) rename tests/cases/fourslash/{importNameCodeFixNewImport2.ts => importNameCodeFixNewImportAmbient2.ts} (100%) rename tests/cases/fourslash/{importNameCodeFixNewImport8.ts => importNameCodeFixNewImportAmbient3.ts} (100%) create mode 100644 tests/cases/fourslash/importNameCodeFixNewImportBaseUrl0.ts create mode 100644 tests/cases/fourslash/importNameCodeFixNewImportDefault0.ts rename tests/cases/fourslash/{importNameCodeFixNewImport3.ts => importNameCodeFixNewImportFile0.ts} (100%) rename tests/cases/fourslash/{importNameCodeFixNewImport4.ts => importNameCodeFixNewImportFile1.ts} (100%) rename tests/cases/fourslash/{importNameCodeFixNewImport5.ts => importNameCodeFixNewImportFile2.ts} (100%) rename tests/cases/fourslash/{importNameCodeFixNewImport6.ts => importNameCodeFixNewImportNodeModules0.ts} (100%) rename tests/cases/fourslash/{importNameCodeFixNewImport7.ts => importNameCodeFixNewImportNodeModules1.ts} (100%) rename tests/cases/fourslash/{importNameCodeFixNewImport9.ts => importNameCodeFixNewImportNodeModules2.ts} (100%) create mode 100644 tests/cases/fourslash/importNameCodeFixNewImportNodeModules3.ts create mode 100644 tests/cases/fourslash/importNameCodeFixNewImportPaths0.ts create mode 100644 tests/cases/fourslash/importNameCodeFixNewImportPaths1.ts create mode 100644 tests/cases/fourslash/importNameCodeFixNewImportRootDirs0.ts create mode 100644 tests/cases/fourslash/importNameCodeFixNewImportTypeRoots0.ts diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 006b87cb542ee..be7c8a862c76a 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -102,7 +102,8 @@ namespace ts { isImplementationOfOverload, getAliasedSymbol: resolveAlias, getEmitResolver, - getExportsOfModule: getExportsOfModuleAsArray, + getExportsOfModuleAsArray, + getExportsOfModule, getAmbientModules, getJsxElementAttributesType, diff --git a/src/compiler/core.ts b/src/compiler/core.ts index dd71877930a62..cbe78bb074dc6 100644 --- a/src/compiler/core.ts +++ b/src/compiler/core.ts @@ -1357,6 +1357,14 @@ namespace ts { getEmitScriptTarget(compilerOptions) >= ScriptTarget.ES2015 ? ModuleKind.ES2015 : ModuleKind.CommonJS; } + export function getEmitModuleResolutionKind(compilerOptions: CompilerOptions) { + let moduleResolution = compilerOptions.moduleResolution; + if (moduleResolution === undefined) { + moduleResolution = getEmitModuleKind(compilerOptions) === ModuleKind.CommonJS ? ModuleResolutionKind.NodeJs : ModuleResolutionKind.Classic; + } + return moduleResolution; + } + /* @internal */ export function hasZeroOrOneAsteriskCharacter(str: string): boolean { let seenAsterisk = false; diff --git a/src/compiler/moduleNameResolver.ts b/src/compiler/moduleNameResolver.ts index 86e0157d84729..afac831160e7e 100644 --- a/src/compiler/moduleNameResolver.ts +++ b/src/compiler/moduleNameResolver.ts @@ -56,7 +56,7 @@ namespace ts { return { resolvedModule: resolved && resolvedModuleFromResolved(resolved, isExternalLibraryImport), failedLookupLocations }; } - function moduleHasNonRelativeName(moduleName: string): boolean { + export function moduleHasNonRelativeName(moduleName: string): boolean { return !(isRootedDiskPath(moduleName) || isExternalModuleNameRelative(moduleName)); } diff --git a/src/compiler/types.ts b/src/compiler/types.ts index e4c92c502a456..635f08bc25d89 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -2290,7 +2290,8 @@ namespace ts { getConstantValue(node: EnumMember | PropertyAccessExpression | ElementAccessExpression): number; isValidPropertyAccess(node: PropertyAccessExpression | QualifiedName, propertyName: string): boolean; getAliasedSymbol(symbol: Symbol): Symbol; - getExportsOfModule(moduleSymbol: Symbol): Symbol[]; + getExportsOfModuleAsArray(moduleSymbol: Symbol): Symbol[]; + getExportsOfModule(moduleSymbol: Symbol): Map | undefined; getJsxElementAttributesType(elementNode: JsxOpeningLikeElement): Type; getJsxIntrinsicTagNames(): Symbol[]; @@ -3093,7 +3094,7 @@ namespace ts { target?: ScriptTarget; traceResolution?: boolean; types?: string[]; - /** Paths used to used to compute primary types search locations */ + /** Paths used to compute primary types search locations */ typeRoots?: string[]; /*@internal*/ version?: boolean; /*@internal*/ watch?: boolean; diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index b008ca653702d..fb17526e982f7 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -2591,7 +2591,7 @@ ${code} resetLocalData(); } - currentFileName = basePath + "/" + value; + currentFileName = ts.isRootedDiskPath(value) ? value : basePath + "/" + value; currentFileOptions[key] = value; } else { diff --git a/src/services/codefixes/codeFixProvider.ts b/src/services/codefixes/codeFixProvider.ts index a3d0766bc64c4..e4489fc2dca94 100644 --- a/src/services/codefixes/codeFixProvider.ts +++ b/src/services/codefixes/codeFixProvider.ts @@ -2,7 +2,7 @@ namespace ts { export interface CodeFix { errorCodes: number[]; - getCodeActions(context: CodeFixContext, cancellationToken?: CancellationToken): CodeAction[] | undefined; + getCodeActions(context: CodeFixContext): CodeAction[] | undefined; } export interface CodeFixContext { @@ -12,6 +12,7 @@ namespace ts { program: Program; newLineCharacter: string; host: LanguageServiceHost; + cancellationToken: CancellationToken; } export namespace codefix { @@ -32,12 +33,12 @@ namespace ts { return Object.keys(codeFixes); } - export function getFixes(context: CodeFixContext, cancellationToken: CancellationToken): CodeAction[] { + export function getFixes(context: CodeFixContext): CodeAction[] { const fixes = codeFixes[context.errorCode]; let allActions: CodeAction[] = []; forEach(fixes, f => { - const actions = f.getCodeActions(context, cancellationToken); + const actions = f.getCodeActions(context); if (actions && actions.length > 0) { allActions = allActions.concat(actions); } diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index e3cb359bd6d17..a38246a1625e4 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -1,10 +1,9 @@ /* @internal */ namespace ts.codefix { - const nodeModulesFolderName = "node_modules"; registerCodeFix({ errorCodes: [Diagnostics.Cannot_find_name_0.code], - getCodeActions: (context: CodeFixContext, cancellationToken: CancellationToken) => { + getCodeActions: (context: CodeFixContext) => { const sourceFile = context.sourceFile; const checker = context.program.getTypeChecker(); const allSourceFiles = context.program.getSourceFiles(); @@ -13,12 +12,8 @@ namespace ts.codefix { const token = getTokenAtPosition(sourceFile, context.span.start); const name = token.getText(); const allActions: CodeAction[] = []; - let allPotentialModules: Symbol[] = []; - const ambientModules = checker.getAmbientModules(); - if (ambientModules) { - allPotentialModules = ambientModules; - } + const allPotentialModules = checker.getAmbientModules(); for (const otherSourceFile of allSourceFiles) { if (otherSourceFile !== sourceFile && otherSourceFile.symbol) { allPotentialModules.push(otherSourceFile.symbol); @@ -26,11 +21,27 @@ namespace ts.codefix { } for (const moduleSymbol of allPotentialModules) { - cancellationToken.throwIfCancellationRequested(); + context.cancellationToken.throwIfCancellationRequested(); + + const moduleExports = checker.getExportsOfModule(moduleSymbol); + if (!moduleExports) { + continue; + } - const exports = checker.getExportsOfModule(moduleSymbol) || []; - for (const exported of exports) { - if (exported.name === name) { + const currentTokenMeaning = getMeaningFromLocation(token); + + // check the default export + const defaultExport = moduleExports["default"]; + if (defaultExport) { + const localSymbol = getLocalSymbolForExportDefault(defaultExport); + if (localSymbol && localSymbol.name === name && checkSymbolHasMeaning(localSymbol, currentTokenMeaning)) { + allActions.push(getCodeActionForImport(moduleSymbol, /*isDefaultExport*/ true)); + } + } + + // check exports with the same name + if (name in moduleExports) { + if (checkSymbolHasMeaning(moduleExports[name], currentTokenMeaning)) { allActions.push(getCodeActionForImport(moduleSymbol)); } } @@ -38,7 +49,12 @@ namespace ts.codefix { return allActions; - function getCodeActionForImport(moduleSymbol: Symbol): CodeAction { + function checkSymbolHasMeaning(symbol: Symbol, meaning: SemanticMeaning) { + const declarations = symbol.getDeclarations(); + return declarations ? some(symbol.declarations, decl => !!(getMeaningFromDeclaration(decl) & meaning)) : false; + } + + function getCodeActionForImport(moduleSymbol: Symbol, isDefaultExport?: boolean): CodeAction { // Check to see if there are already imports being made from this source in the current file const existingDeclaration = forEach(sourceFile.imports, importModuleSpecifier => { const importSymbol = checker.getSymbolAtLocation(importModuleSpecifier); @@ -57,215 +73,269 @@ namespace ts.codefix { function getImportDeclaration(moduleSpecifier: LiteralExpression) { let node: Node = moduleSpecifier; while (node) { - if (node.kind !== SyntaxKind.ImportDeclaration) { - node = node.parent; + if (node.kind === SyntaxKind.ImportDeclaration) { + return node; } - - return node; + if (node.kind === SyntaxKind.ImportEqualsDeclaration) { + return node; + } + node = node.parent; } return undefined; } - function getCodeActionForExistingImport(declaration: ImportDeclaration): CodeAction { - const moduleSpecifier = declaration.moduleSpecifier.getText(); - - // We have to handle all of the different import declaration forms - if (declaration.importClause) { - if (declaration.importClause.namedBindings) { - const namedBindings = declaration.importClause.namedBindings; - if (namedBindings.kind === SyntaxKind.NamespaceImport) { - /** - * Cases: - * import * as ns from "mod" - * import d, * as ns from "mod" - * - * Because there is no import list, we alter the reference to include the - * namespace instead of altering the import declaration. For example, "foo" would - * become "ns.foo" - */ - const ns = (namedBindings).name.getText(); - return createCodeAction( - Diagnostics.Change_0_to_1, - [name, `${ns}.${name}`], - `${ns}.`, - { start: token.getStart(), length: 0 }, - sourceFile.fileName - ); - } - else if (namedBindings.kind === SyntaxKind.NamedImports) { - /** - * Cases: - * import { a, b as x } from "mod" - * import d, { a, b as x } from "mod" - * - * Because there is already an import list, just insert the identifier into it - */ - const textChange = getTextChangeForImportList(namedBindings); - return createCodeAction( - Diagnostics.Add_0_to_existing_import_declaration_from_1, - [name, moduleSpecifier], - textChange.newText, - textChange.span, - sourceFile.fileName - ); + function getCodeActionForExistingImport(declaration: ImportDeclaration | ImportEqualsDeclaration): CodeAction { + let namespacePrefix: string; + let moduleSpecifier: string; + if (declaration.kind === SyntaxKind.ImportDeclaration) { + const namedBindings = declaration.importClause && declaration.importClause.namedBindings; + if (namedBindings && namedBindings.kind === SyntaxKind.NamespaceImport) { + namespacePrefix = (namedBindings).name.getText(); + } + + moduleSpecifier = declaration.moduleSpecifier.getText(); + } + else { + namespacePrefix = declaration.name.getText(); + moduleSpecifier = declaration.moduleReference.getText(); + } + + /** + * Cases: + * import * as ns from "mod" + * import default, * as ns from "mod" + * import ns = require("mod") + * + * Because there is no import list, we alter the reference to include the + * namespace instead of altering the import declaration. For example, "foo" would + * become "ns.foo" + */ + if (namespacePrefix) { + return createCodeAction( + Diagnostics.Change_0_to_1, + [name, `${namespacePrefix}.${name}`], + `${namespacePrefix}.`, + { start: token.getStart(), length: 0 }, + sourceFile.fileName + ); + } + return getCodeActionForNewImport(moduleSpecifier, declaration.getEnd()); + } + + function getCodeActionForNewImport(moduleSpecifier?: string, insertPos?: number): CodeAction { + // if not specified an insert position, try to insert after any existing imports + if (!insertPos) { + let lastModuleSpecifierEnd = -1; + for (const moduleSpecifier of sourceFile.imports) { + const end = moduleSpecifier.getEnd(); + if (!lastModuleSpecifierEnd || end > lastModuleSpecifierEnd) { + lastModuleSpecifierEnd = end; } } - else if (declaration.importClause.name) { - /** - * Case: import d from "mod" - * - * Add a list of imports after the default import - */ - return createCodeAction( - Diagnostics.Add_0_to_existing_import_declaration_from_1, - [name, moduleSpecifier], - `, { ${name} }`, - { start: declaration.importClause.name.getEnd(), length: 0 }, - sourceFile.fileName - ); + insertPos = lastModuleSpecifierEnd > 0 ? lastModuleSpecifierEnd + 1 : sourceFile.getStart(); + } + + const getCanonicalFileName = createGetCanonicalFileName(useCaseSensitiveFileNames); + moduleSpecifier = stripQuotes(moduleSpecifier || getModuleSpecifierForNewImport()); + const prefixNewLine = insertPos === sourceFile.getStart() ? "" : context.newLineCharacter; + const importStatementText = isDefaultExport + ? `import ${name} from "${moduleSpecifier}"` + : `import { ${name} } from "${moduleSpecifier}"`; + + return createCodeAction( + Diagnostics.Import_0_from_1, + [name, `"${moduleSpecifier}"`], + `${prefixNewLine}${importStatementText};`, + { start: insertPos, length: 0 }, + sourceFile.fileName + ); + + function getModuleSpecifierForNewImport() { + const fileName = normalizeFileName(sourceFile.fileName); + const moduleFileName = normalizeFileName(moduleSymbol.valueDeclaration.getSourceFile().fileName); + const sourceDirectory = getDirectoryPath(fileName); + const options = context.program.getCompilerOptions(); + + return tryGetModuleNameFromAmbientModule() || + tryGetModuleNameFromExistingUses() || + tryGetModuleNameFromBaseUrl() || + tryGetModuleNameFromRootDirs() || + tryGetModuleNameFromTypeRoots() || + tryGetModuleNameAsNodeModule() || + removeFileExtension(getRelativePath(moduleFileName, sourceDirectory)); + + function normalizeFileName(fileName: string) { + return getCanonicalFileName(normalizeSlashes(fileName)); } - function getTextChangeForImportList(importList: NamedImports): TextChange { - if (importList.elements.length === 0) { - const start = importList.getStart(); - return { - newText: `{ ${name} }`, - span: { start, length: importList.getEnd() - start } - }; + function tryGetModuleNameFromAmbientModule(): string { + if (moduleSymbol.valueDeclaration.kind !== SyntaxKind.SourceFile) { + return moduleSymbol.name; } + } - // Insert after the last element - const insertPoint = importList.elements[importList.elements.length - 1].getEnd(); - - // If the import list has one import per line, preserve that. Otherwise, insert on same line as last element - let oneImportPerLine: boolean; + function tryGetModuleNameFromExistingUses(): string { + for (const file of context.program.getSourceFiles()) { + if (file === sourceFile || !file.resolvedModules) { + continue; + } + + for (const moduleName in file.resolvedModules) { + if (!moduleHasNonRelativeName(moduleName)) { + continue; + } + + const resolvedModule = file.resolvedModules[moduleName]; + if (resolvedModule && resolvedModule.resolvedFileName && normalizeFileName(resolvedModule.resolvedFileName) === moduleFileName) { + return moduleName; + } + } + } + } - if (importList.elements.length === 1) { - /** - * If there is only one symbol being imported, still check to see if it's set up for multi-line imports like this: - * import { - * foo - * } from "./module"; - */ - const startLine = getLineOfLocalPosition(sourceFile, importList.getStart()); - const endLine = getLineOfLocalPosition(sourceFile, importList.getEnd()); + function tryGetModuleNameFromBaseUrl() { + if (!options.baseUrl) { + return undefined; + } - oneImportPerLine = endLine - startLine >= 2; + let relativeName = tryRemoveParentDirectoryName(moduleFileName, options.baseUrl); + if (!relativeName) { + return undefined; } - else { - const startLine = getLineOfLocalPosition(sourceFile, importList.elements[0].getStart()); - const endLine = getLineOfLocalPosition(sourceFile, insertPoint); - oneImportPerLine = endLine - startLine >= importList.elements.length - 1; + relativeName = removeFileExtension(relativeName); + + if (options.paths) { + // TODO: handle longest match support + for (const key in options.paths) { + for (const pattern of options.paths[key]) { + const indexOfStar = pattern.indexOf("*"); + if (indexOfStar === 0 && pattern.length === 1) { + continue; + } + else if (indexOfStar !== -1) { + const prefix = pattern.substr(0, indexOfStar); + const suffix = pattern.substr(indexOfStar + 1); + if (relativeName.length >= prefix.length + suffix.length && + startsWith(relativeName, prefix) && + endsWith(relativeName, suffix)) { + const matchedStar = relativeName.substr(prefix.length, relativeName.length - suffix.length); + return key.replace("\*", matchedStar); + } + } + else if (pattern === relativeName) { + return key; + } + } + } } - return { - newText: oneImportPerLine ? `, ${context.newLineCharacter}${name}` : `,${name}`, - span: { start: insertPoint, length: 0 } - }; + return relativeName; } - } + function tryGetModuleNameFromRootDirs() { + if (options.rootDirs) { + const normalizedTargetPath = getPathRelativeToRootDirs(moduleFileName, options.rootDirs); + const normalizedSourcePath = getPathRelativeToRootDirs(sourceDirectory, options.rootDirs); + if (normalizedTargetPath !== undefined) { + const relativePath = normalizedSourcePath !== undefined ? getRelativePath(normalizedTargetPath, normalizedSourcePath) : normalizedTargetPath; + return removeFileExtension(relativePath); + } + } + return undefined; + } - return createCodeAction( - Diagnostics.Add_0_to_existing_import_declaration_from_1, - [name, moduleSpecifier], - `{ ${name} } from `, - { start: declaration.moduleSpecifier.getStart(), length: 0 }, - sourceFile.fileName - ); - } + function tryGetModuleNameFromTypeRoots() { + const typesRoots = getEffectiveTypeRoots(options, context.host); + for (const typeRoot of typesRoots) { + if (startsWith(moduleFileName, typeRoot)) { + let relativeFileName = moduleFileName.substring(typeRoot.length + 1); + relativeFileName = removeFileExtension(relativeFileName); - function getCodeActionForNewImport(): CodeAction { - // Try to insert after any existing imports - let lastModuleSpecifierEnd = -1; - for (const moduleSpecifier of sourceFile.imports) { - const end = moduleSpecifier.getEnd(); - if (!lastModuleSpecifierEnd || end > lastModuleSpecifierEnd) { - lastModuleSpecifierEnd = end; + if (endsWith(relativeFileName, "/index")) { + relativeFileName = relativeFileName.substr(0, relativeFileName.length - 6/* "/index".length */); + } + + return relativeFileName; + } + } } - } - const moduleSpecifier = getModuleSpecifierForNewImport(); - let newText = `import { ${name} } from "${moduleSpecifier}";`; - newText = lastModuleSpecifierEnd ? context.newLineCharacter + newText : newText + context.newLineCharacter; + function tryGetModuleNameAsNodeModule() { + if (getEmitModuleResolutionKind(options) !== ModuleResolutionKind.NodeJs) { + // nothing to do here + return undefined; + } - return createCodeAction( - Diagnostics.Import_0_from_1, - [name, `"${moduleSpecifier}"`], - newText, - { - start: lastModuleSpecifierEnd >= 0 ? lastModuleSpecifierEnd + 1 : sourceFile.getStart(), - length: 0 - }, - sourceFile.fileName - ); + const indexOfNodeModules = moduleFileName.indexOf("node_modules"); + if (indexOfNodeModules < 0) { + return undefined; + } - function getModuleSpecifierForNewImport(): string { - if (moduleSymbol.valueDeclaration.kind !== SyntaxKind.SourceFile) { - return stripQuotes(moduleSymbol.name); - } + let relativeFileName: string; + if (sourceDirectory.indexOf(moduleFileName.substring(0, indexOfNodeModules - 1)) === 0) { + // if node_modules folder is in this folder or any of its parent folder, no need to keep it. + relativeFileName = moduleFileName.substring(indexOfNodeModules + 13 /* "node_modules\".length */); + } + else { + relativeFileName = getRelativePath(moduleFileName, sourceDirectory); + } + + relativeFileName = removeFileExtension(relativeFileName); - // If the module is from a module file, then there are typically several cases: - // - // 1. from a source file in your program (file path has no node_modules) - // 2. from a file in a node_modules folder: - // 2.1 the node_modules folder is in a subfolder of the sourceDir (cannot be found by the module resolution) - // 2.2 the node_modules folder is in the sourceDir or above (can be found by the module resolution) - // 2.2.1 the module file is the "main" file in package.json (or "index.js" if not specified) - // 2.2.2 the module file is not the "main" file - // - // for case 1 and 2.2, we would return the relative file path as the module specifier; - // for case 2.1, we would just use the module name instead. - const sourceDir = getDirectoryPath(sourceFile.fileName); - const modulePath = (moduleSymbol.valueDeclaration).fileName; - - const i = modulePath.lastIndexOf(nodeModulesFolderName); - - // case 1 and case 2.1: return the relative file path as the module specifier; - if (i === -1 || (modulePath.indexOf(sourceDir) === 0 && modulePath.indexOf(combinePaths(sourceDir, nodeModulesFolderName)) === -1)) { - const relativePath = getRelativePathToDirectoryOrUrl( - sourceDir, - modulePath, - /*currentDirectory*/ sourceDir, - createGetCanonicalFileName(useCaseSensitiveFileNames), - /*isAbsolutePathAnUrl*/ false - ); - const isRootedOrRelative = isExternalModuleNameRelative(relativePath) || isRootedDiskPath(relativePath); - return removeFileExtension(isRootedOrRelative ? relativePath : combinePaths(".", relativePath)); + if(startsWith(relativeFileName, "@types/")) { + relativeFileName = relativeFileName.substr(7 /*"@types/.length"*/); + } + + if (endsWith(relativeFileName, "/index")) { + relativeFileName = getDirectoryPath(relativeFileName); + } + else { + try { + const moduleDirectory = getDirectoryPath(moduleFileName); + const packageJsonContent = JSON.parse(context.host.readFile(combinePaths(moduleDirectory, "package.json"))); + if (packageJsonContent && packageJsonContent.main) { + const mainExportFile = isRootedDiskPath(packageJsonContent.main) + ? normalizeFileName(packageJsonContent.main) + : getCanonicalFileName(normalizePath(combinePaths(moduleDirectory, packageJsonContent.main))); + if (removeFileExtension(mainExportFile) === removeFileExtension(moduleFileName)) { + relativeFileName = getDirectoryPath(relativeFileName); + } + } + } + catch(e) { } + } + + return relativeFileName; } + } - // case 2.2 - // If this is a node module, check to see if the given file is the main export of the module or not. If so, - // it can be referenced by just the module name. - const moduleDir = getDirectoryPath(modulePath); - let nodePackage: any; - try { - nodePackage = JSON.parse(context.host.readFile(combinePaths(moduleDir, "package.json"))); + function getPathRelativeToRootDirs(fileName: string, rootDirs: string[]) { + for (const rootDir of rootDirs) { + const relativeName = tryRemoveParentDirectoryName(fileName, rootDir); + if (relativeName !== undefined) { + return relativeName; + } } - catch (e) { } + return undefined; + } - // If no main export is explicitly defined, check for the default (index.js) - const mainExport = (nodePackage && nodePackage.main) || "index.js"; - const mainExportPath = normalizePath(isRootedDiskPath(mainExport) ? mainExport : combinePaths(moduleDir, mainExport)); - const moduleSpecifier = removeFileExtension(modulePath.substring(i + nodeModulesFolderName.length + 1)); + function getRelativePath(path: string, directoryPath: string) { + const relativePath = getRelativePathToDirectoryOrUrl(directoryPath, path, directoryPath, getCanonicalFileName, false); + return moduleHasNonRelativeName(relativePath) ? "./" + relativePath : relativePath; + } - if (areModuleSpecifiersEqual(modulePath, mainExportPath)) { - return getDirectoryPath(moduleSpecifier); - } - else { - return moduleSpecifier; + function tryRemoveParentDirectoryName(path: string, parentDirectory: string) { + const index = path.indexOf(parentDirectory); + if (index === 0) { + return endsWith(parentDirectory, directorySeparator) + ? path.substring(parentDirectory.length) + : path.substring(parentDirectory.length + 1); } + return undefined; } } - function areModuleSpecifiersEqual(a: string, b: string): boolean { - // Paths to modules can be relative or absolute and may optionally include the file - // extension of the module - a = removeFileExtension(a); - b = removeFileExtension(b); - return comparePaths(a, b, getDirectoryPath(sourceFile.fileName), !useCaseSensitiveFileNames) === Comparison.EqualTo; - } } function createCodeAction(description: DiagnosticMessage, diagnosticArgs: string[], newText: string, span: TextSpan, fileName: string): CodeAction { diff --git a/src/services/completions.ts b/src/services/completions.ts index b710aa8cd78bb..9705f07e1a12b 100644 --- a/src/services/completions.ts +++ b/src/services/completions.ts @@ -975,7 +975,7 @@ namespace ts.Completions { if (symbol && symbol.flags & SymbolFlags.HasExports) { // Extract module or enum members - const exportedSymbols = typeChecker.getExportsOfModule(symbol); + const exportedSymbols = typeChecker.getExportsOfModuleAsArray(symbol); forEach(exportedSymbols, symbol => { if (typeChecker.isValidPropertyAccess((node.parent), symbol.name)) { symbols.push(symbol); @@ -1320,7 +1320,7 @@ namespace ts.Completions { let exports: Symbol[]; const moduleSpecifierSymbol = typeChecker.getSymbolAtLocation(importOrExportDeclaration.moduleSpecifier); if (moduleSpecifierSymbol) { - exports = typeChecker.getExportsOfModule(moduleSpecifierSymbol); + exports = typeChecker.getExportsOfModuleAsArray(moduleSpecifierSymbol); } symbols = exports ? filterNamedImportOrExportCompletionItems(exports, namedImportsOrExports.elements) : emptyArray; diff --git a/src/services/services.ts b/src/services/services.ts index 5afed5460f3aa..8e8d034ef0ccb 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -1679,10 +1679,11 @@ namespace ts { span: span, program: program, newLineCharacter: newLineChar, - host: host + host: host, + cancellationToken: cancellationToken }; - const fixes = codefix.getFixes(context, cancellationToken); + const fixes = codefix.getFixes(context); if (fixes) { allFixes = allFixes.concat(fixes); } diff --git a/tests/cases/fourslash/importNameCodeFixNewImport0.ts b/tests/cases/fourslash/importNameCodeFixNewImportAmbient0.ts similarity index 100% rename from tests/cases/fourslash/importNameCodeFixNewImport0.ts rename to tests/cases/fourslash/importNameCodeFixNewImportAmbient0.ts diff --git a/tests/cases/fourslash/importNameCodeFixNewImport1.ts b/tests/cases/fourslash/importNameCodeFixNewImportAmbient1.ts similarity index 100% rename from tests/cases/fourslash/importNameCodeFixNewImport1.ts rename to tests/cases/fourslash/importNameCodeFixNewImportAmbient1.ts diff --git a/tests/cases/fourslash/importNameCodeFixNewImport2.ts b/tests/cases/fourslash/importNameCodeFixNewImportAmbient2.ts similarity index 100% rename from tests/cases/fourslash/importNameCodeFixNewImport2.ts rename to tests/cases/fourslash/importNameCodeFixNewImportAmbient2.ts diff --git a/tests/cases/fourslash/importNameCodeFixNewImport8.ts b/tests/cases/fourslash/importNameCodeFixNewImportAmbient3.ts similarity index 100% rename from tests/cases/fourslash/importNameCodeFixNewImport8.ts rename to tests/cases/fourslash/importNameCodeFixNewImportAmbient3.ts diff --git a/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl0.ts b/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl0.ts new file mode 100644 index 0000000000000..72e7174ba3b48 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl0.ts @@ -0,0 +1,18 @@ +/// + +//// [|f1/*0*/();|] + +// @Filename: tsconfig.json +//// { +//// "compilerOptions": { +//// "baseUrl": "./a" +//// } +//// } + +// @Filename: a/b.ts +//// export function f1() { }; + +verify.codeFixAtPosition( +`import { f1 } from "b"; +f1();` +); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportDefault0.ts b/tests/cases/fourslash/importNameCodeFixNewImportDefault0.ts new file mode 100644 index 0000000000000..1f2cde9f571b0 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImportDefault0.ts @@ -0,0 +1,11 @@ +/// + +//// [|f1/*0*/();|] + +// @Filename: module.ts +//// export default function f1() { }; + +verify.codeFixAtPosition( +`import f1 from "./module"; +f1();` +); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImport3.ts b/tests/cases/fourslash/importNameCodeFixNewImportFile0.ts similarity index 100% rename from tests/cases/fourslash/importNameCodeFixNewImport3.ts rename to tests/cases/fourslash/importNameCodeFixNewImportFile0.ts diff --git a/tests/cases/fourslash/importNameCodeFixNewImport4.ts b/tests/cases/fourslash/importNameCodeFixNewImportFile1.ts similarity index 100% rename from tests/cases/fourslash/importNameCodeFixNewImport4.ts rename to tests/cases/fourslash/importNameCodeFixNewImportFile1.ts diff --git a/tests/cases/fourslash/importNameCodeFixNewImport5.ts b/tests/cases/fourslash/importNameCodeFixNewImportFile2.ts similarity index 100% rename from tests/cases/fourslash/importNameCodeFixNewImport5.ts rename to tests/cases/fourslash/importNameCodeFixNewImportFile2.ts diff --git a/tests/cases/fourslash/importNameCodeFixNewImport6.ts b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules0.ts similarity index 100% rename from tests/cases/fourslash/importNameCodeFixNewImport6.ts rename to tests/cases/fourslash/importNameCodeFixNewImportNodeModules0.ts diff --git a/tests/cases/fourslash/importNameCodeFixNewImport7.ts b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules1.ts similarity index 100% rename from tests/cases/fourslash/importNameCodeFixNewImport7.ts rename to tests/cases/fourslash/importNameCodeFixNewImportNodeModules1.ts diff --git a/tests/cases/fourslash/importNameCodeFixNewImport9.ts b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules2.ts similarity index 100% rename from tests/cases/fourslash/importNameCodeFixNewImport9.ts rename to tests/cases/fourslash/importNameCodeFixNewImportNodeModules2.ts diff --git a/tests/cases/fourslash/importNameCodeFixNewImportNodeModules3.ts b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules3.ts new file mode 100644 index 0000000000000..c3e019eae60a1 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules3.ts @@ -0,0 +1,13 @@ +/// + +// @Filename: /a.ts +//// [|f1/*0*/();|] + +// @Filename: /node_modules/@types/random/index.d.ts +//// export var v1 = 5; +//// export function f1(); + +verify.codeFixAtPosition( +`import { f1 } from "random"; +f1();` +); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportPaths0.ts b/tests/cases/fourslash/importNameCodeFixNewImportPaths0.ts new file mode 100644 index 0000000000000..92c3236bd14c3 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImportPaths0.ts @@ -0,0 +1,21 @@ +/// + +//// [|foo/*0*/();|] + +// @Filename: folder_a/f2.ts +//// export function foo() {}; + +// @Filename: tsconfig.json +//// { +//// "compilerOptions": { +//// "baseUrl": ".", +//// "paths": { +//// "a": [ "folder_a/f2" ] +//// } +//// } +//// } + +verify.codeFixAtPosition( + `import { foo } from "a"; +foo();` +); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportPaths1.ts b/tests/cases/fourslash/importNameCodeFixNewImportPaths1.ts new file mode 100644 index 0000000000000..5734765121727 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImportPaths1.ts @@ -0,0 +1,21 @@ +/// + +//// [|foo/*0*/();|] + +// @Filename: folder_b/f2.ts +//// export function foo() {}; + +// @Filename: tsconfig.json +//// { +//// "compilerOptions": { +//// "baseUrl": ".", +//// "paths": { +//// "b/*": [ "folder_b/*" ] +//// } +//// } +//// } + +verify.codeFixAtPosition( + `import { foo } from "b/f2"; +foo();` +); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportRootDirs0.ts b/tests/cases/fourslash/importNameCodeFixNewImportRootDirs0.ts new file mode 100644 index 0000000000000..e5ce140168b03 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImportRootDirs0.ts @@ -0,0 +1,22 @@ +/// + +// @Filename: a/f1.ts +//// [|foo/*0*/();|] + +// @Filename: b/c/f2.ts +//// export function foo() {}; + +// @Filename: tsconfig.json +//// { +//// "compilerOptions": { +//// "rootDirs": [ +//// "a", +//// "b/c" +//// ] +//// } +//// } + +verify.codeFixAtPosition( + `import { foo } from "./f2"; +foo();` +); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportTypeRoots0.ts b/tests/cases/fourslash/importNameCodeFixNewImportTypeRoots0.ts new file mode 100644 index 0000000000000..07e62fa49cb53 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixNewImportTypeRoots0.ts @@ -0,0 +1,21 @@ +/// + +// @Filename: a/f1.ts +//// [|foo/*0*/();|] + +// @Filename: types/random/index.ts +//// export function foo() {}; + +// @Filename: tsconfig.json +//// { +//// "compilerOptions": { +//// "typeRoots": [ +//// "./types" +//// ] +//// } +//// } + +verify.codeFixAtPosition( + `import { foo } from "random"; +foo();` +); \ No newline at end of file From d273308c3e3d481bd2370410532b27208205a5e8 Mon Sep 17 00:00:00 2001 From: zhengbli Date: Tue, 8 Nov 2016 01:26:49 -0800 Subject: [PATCH 06/14] Fix existing import cases --- src/services/codefixes/importFixes.ts | 60 ++++++++++++++++++- .../importNameCodeFixExistingImport4.ts | 8 ++- .../importNameCodeFixExistingImport5.ts | 8 ++- 3 files changed, 67 insertions(+), 9 deletions(-) diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index a38246a1625e4..a0ae0ef44595e 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -119,7 +119,61 @@ namespace ts.codefix { sourceFile.fileName ); } + /** + * If the existing import declaration already has a named import list, just + * insert the identifier into that list. + */ + else if (declaration.kind === SyntaxKind.ImportDeclaration && + declaration.importClause && + declaration.importClause.namedBindings && + declaration.importClause.namedBindings.kind === SyntaxKind.NamedImports) { + const textChange = getTextChangeForImportList(declaration.importClause.namedBindings); + return createCodeAction( + Diagnostics.Add_0_to_existing_import_declaration_from_1, + [name, moduleSpecifier], + textChange.newText, + textChange.span, + sourceFile.fileName + ); + } return getCodeActionForNewImport(moduleSpecifier, declaration.getEnd()); + + function getTextChangeForImportList(importList: NamedImports): TextChange { + if (importList.elements.length === 0) { + const start = importList.getStart(); + return { + newText: `{ ${name} }`, + span: { start, length: importList.getEnd() - start } + }; + } + + // Insert after the last element + const insertPoint = importList.elements[importList.elements.length - 1].getEnd(); + + // If the import list has one import per line, preserve that. Otherwise, insert on same line as last element + let oneImportPerLine: boolean; + if (importList.elements.length === 1) { + /** + * If there is only one symbol being imported, still check to see if it's set up for multi-line imports like this: + * import { + * foo + * } from "./module"; + */ + const startLine = getLineOfLocalPosition(sourceFile, importList.getStart()); + const endLine = getLineOfLocalPosition(sourceFile, importList.getEnd()); + oneImportPerLine = endLine - startLine >= 2; + } + else { + const startLine = getLineOfLocalPosition(sourceFile, importList.elements[0].getStart()); + const endLine = getLineOfLocalPosition(sourceFile, insertPoint); + oneImportPerLine = endLine - startLine >= importList.elements.length - 1; + } + + return { + newText: `,${oneImportPerLine ? context.newLineCharacter : ""}${name}`, + span: { start: insertPoint, length: 0 } + }; + } } function getCodeActionForNewImport(moduleSpecifier?: string, insertPos?: number): CodeAction { @@ -283,7 +337,7 @@ namespace ts.codefix { relativeFileName = removeFileExtension(relativeFileName); - if(startsWith(relativeFileName, "@types/")) { + if (startsWith(relativeFileName, "@types/")) { relativeFileName = relativeFileName.substr(7 /*"@types/.length"*/); } @@ -295,7 +349,7 @@ namespace ts.codefix { const moduleDirectory = getDirectoryPath(moduleFileName); const packageJsonContent = JSON.parse(context.host.readFile(combinePaths(moduleDirectory, "package.json"))); if (packageJsonContent && packageJsonContent.main) { - const mainExportFile = isRootedDiskPath(packageJsonContent.main) + const mainExportFile = isRootedDiskPath(packageJsonContent.main) ? normalizeFileName(packageJsonContent.main) : getCanonicalFileName(normalizePath(combinePaths(moduleDirectory, packageJsonContent.main))); if (removeFileExtension(mainExportFile) === removeFileExtension(moduleFileName)) { @@ -303,7 +357,7 @@ namespace ts.codefix { } } } - catch(e) { } + catch (e) { } } return relativeFileName; diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport4.ts b/tests/cases/fourslash/importNameCodeFixExistingImport4.ts index a56cab1337d78..a23ecff1fe48e 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport4.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport4.ts @@ -1,11 +1,13 @@ /// -//// [|import d from "./module"|]; -//// f1/*0*/(); +//// [|import d from "./module"; +//// f1/*0*/();|] // @Filename: module.ts //// export function f1() {} //// export var v1 = 5; //// export default var d1 = 6; -verify.codeFixAtPosition(`import d, { f1 } from "./module"`); \ No newline at end of file +verify.codeFixAtPosition(`import d from "./module"; +import { f1 } from "./module"; +f1();`); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport5.ts b/tests/cases/fourslash/importNameCodeFixExistingImport5.ts index 4fef16e7dc16d..ae62c99446154 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport5.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport5.ts @@ -1,10 +1,12 @@ /// -//// [|import "./module";|] -//// f1/*0*/(); +//// [|import "./module"; +//// f1/*0*/();|] // @Filename: module.ts //// export function f1() {} //// export var v1 = 5; -verify.codeFixAtPosition(`import { f1 } from "./module";`); \ No newline at end of file +verify.codeFixAtPosition(`import "./module"; +import { f1 } from "./module"; +f1();`); \ No newline at end of file From eb892d38a87a507ad009a0bd1f96eae38a8f63da Mon Sep 17 00:00:00 2001 From: zhengbli Date: Wed, 9 Nov 2016 12:50:29 -0800 Subject: [PATCH 07/14] refactor --- src/services/codefixes/importFixes.ts | 90 ++++++++++++--------------- 1 file changed, 40 insertions(+), 50 deletions(-) diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index a0ae0ef44595e..71beb8d6430b0 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -20,6 +20,7 @@ namespace ts.codefix { } } + const currentTokenMeaning = getMeaningFromLocation(token); for (const moduleSymbol of allPotentialModules) { context.cancellationToken.throwIfCancellationRequested(); @@ -28,8 +29,6 @@ namespace ts.codefix { continue; } - const currentTokenMeaning = getMeaningFromLocation(token); - // check the default export const defaultExport = moduleExports["default"]; if (defaultExport) { @@ -85,58 +84,30 @@ namespace ts.codefix { } function getCodeActionForExistingImport(declaration: ImportDeclaration | ImportEqualsDeclaration): CodeAction { - let namespacePrefix: string; - let moduleSpecifier: string; if (declaration.kind === SyntaxKind.ImportDeclaration) { const namedBindings = declaration.importClause && declaration.importClause.namedBindings; - if (namedBindings && namedBindings.kind === SyntaxKind.NamespaceImport) { - namespacePrefix = (namedBindings).name.getText(); + if (namedBindings) { + if (namedBindings.kind === SyntaxKind.NamespaceImport) { + return getCodeActionForNamespaceImport(namedBindings.name.getText()); + } + /** + * If the existing import declaration already has a named import list, just + * insert the identifier into that list. + */ + const textChange = getTextChangeForImportList(namedBindings); + return createCodeAction( + Diagnostics.Add_0_to_existing_import_declaration_from_1, + [name, declaration.moduleSpecifier.getText()], + textChange.newText, + textChange.span, + sourceFile.fileName + ); } - moduleSpecifier = declaration.moduleSpecifier.getText(); - } - else { - namespacePrefix = declaration.name.getText(); - moduleSpecifier = declaration.moduleReference.getText(); + // insert a new import statement in a new lines + return getCodeActionForNewImport(declaration.moduleSpecifier.getText(), declaration.getEnd()); } - - /** - * Cases: - * import * as ns from "mod" - * import default, * as ns from "mod" - * import ns = require("mod") - * - * Because there is no import list, we alter the reference to include the - * namespace instead of altering the import declaration. For example, "foo" would - * become "ns.foo" - */ - if (namespacePrefix) { - return createCodeAction( - Diagnostics.Change_0_to_1, - [name, `${namespacePrefix}.${name}`], - `${namespacePrefix}.`, - { start: token.getStart(), length: 0 }, - sourceFile.fileName - ); - } - /** - * If the existing import declaration already has a named import list, just - * insert the identifier into that list. - */ - else if (declaration.kind === SyntaxKind.ImportDeclaration && - declaration.importClause && - declaration.importClause.namedBindings && - declaration.importClause.namedBindings.kind === SyntaxKind.NamedImports) { - const textChange = getTextChangeForImportList(declaration.importClause.namedBindings); - return createCodeAction( - Diagnostics.Add_0_to_existing_import_declaration_from_1, - [name, moduleSpecifier], - textChange.newText, - textChange.span, - sourceFile.fileName - ); - } - return getCodeActionForNewImport(moduleSpecifier, declaration.getEnd()); + return getCodeActionForNamespaceImport(declaration.name.getText()); function getTextChangeForImportList(importList: NamedImports): TextChange { if (importList.elements.length === 0) { @@ -174,6 +145,26 @@ namespace ts.codefix { span: { start: insertPoint, length: 0 } }; } + + function getCodeActionForNamespaceImport(namespacePrefix: string): CodeAction { + /** + * Cases: + * import * as ns from "mod" + * import default, * as ns from "mod" + * import ns = require("mod") + * + * Because there is no import list, we alter the reference to include the + * namespace instead of altering the import declaration. For example, "foo" would + * become "ns.foo" + */ + return createCodeAction( + Diagnostics.Change_0_to_1, + [name, `${namespacePrefix}.${name}`], + `${namespacePrefix}.`, + { start: token.getStart(), length: 0 }, + sourceFile.fileName + ); + } } function getCodeActionForNewImport(moduleSpecifier?: string, insertPos?: number): CodeAction { @@ -260,7 +251,6 @@ namespace ts.codefix { relativeName = removeFileExtension(relativeName); if (options.paths) { - // TODO: handle longest match support for (const key in options.paths) { for (const pattern of options.paths[key]) { const indexOfStar = pattern.indexOf("*"); From 6d024dc86b885b17da35ed56be67a6a5d7c44aa0 Mon Sep 17 00:00:00 2001 From: zhengbli Date: Wed, 9 Nov 2016 23:11:12 -0800 Subject: [PATCH 08/14] Fix multiple import statement case --- src/services/codefixes/importFixes.ts | 186 ++++++++++++++++---------- 1 file changed, 118 insertions(+), 68 deletions(-) diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 71beb8d6430b0..401456e3abd49 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -1,6 +1,19 @@ /* @internal */ namespace ts.codefix { + // The order of the kinds also determines their priority. + // the ones comes first should be presenteed to the user first too. + enum ImportCodeActionKind { + CodeChange, + InsertingIntoExistingImport, + NewImportWithNonRelativeModuleSpecifier, + NewImportWithRelativeModuleSpecifier, + } + + interface ImportCodeAction extends CodeAction { + kind: ImportCodeActionKind + } + registerCodeFix({ errorCodes: [Diagnostics.Cannot_find_name_0.code], getCodeActions: (context: CodeFixContext) => { @@ -11,7 +24,7 @@ namespace ts.codefix { const token = getTokenAtPosition(sourceFile, context.span.start); const name = token.getText(); - const allActions: CodeAction[] = []; + const allActions: ImportCodeAction[] = []; const allPotentialModules = checker.getAmbientModules(); for (const otherSourceFile of allSourceFiles) { @@ -34,18 +47,20 @@ namespace ts.codefix { if (defaultExport) { const localSymbol = getLocalSymbolForExportDefault(defaultExport); if (localSymbol && localSymbol.name === name && checkSymbolHasMeaning(localSymbol, currentTokenMeaning)) { - allActions.push(getCodeActionForImport(moduleSymbol, /*isDefaultExport*/ true)); + addRange(allActions, getCodeActionForImport(moduleSymbol, /*isDefaultExport*/ true)); } } // check exports with the same name if (name in moduleExports) { if (checkSymbolHasMeaning(moduleExports[name], currentTokenMeaning)) { - allActions.push(getCodeActionForImport(moduleSymbol)); + addRange(allActions, getCodeActionForImport(moduleSymbol)); } } } + //sort the code actions + return allActions; function checkSymbolHasMeaning(symbol: Symbol, meaning: SemanticMeaning) { @@ -53,20 +68,21 @@ namespace ts.codefix { return declarations ? some(symbol.declarations, decl => !!(getMeaningFromDeclaration(decl) & meaning)) : false; } - function getCodeActionForImport(moduleSymbol: Symbol, isDefaultExport?: boolean): CodeAction { + function getCodeActionForImport(moduleSymbol: Symbol, isDefaultExport?: boolean): ImportCodeAction[] { // Check to see if there are already imports being made from this source in the current file - const existingDeclaration = forEach(sourceFile.imports, importModuleSpecifier => { + const existingDeclarations: (ImportDeclaration | ImportEqualsDeclaration)[] = []; + for (const importModuleSpecifier of sourceFile.imports) { const importSymbol = checker.getSymbolAtLocation(importModuleSpecifier); if (importSymbol === moduleSymbol) { - return getImportDeclaration(importModuleSpecifier); + existingDeclarations.push(getImportDeclaration(importModuleSpecifier)); } - }); + } - if (existingDeclaration) { - return getCodeActionForExistingImport(existingDeclaration); + if (existingDeclarations.length > 0) { + return getCodeActionForExistingImport(existingDeclarations); } else { - return getCodeActionForNewImport(); + return [getCodeActionForNewImport()]; } function getImportDeclaration(moduleSpecifier: LiteralExpression) { @@ -83,37 +99,76 @@ namespace ts.codefix { return undefined; } - function getCodeActionForExistingImport(declaration: ImportDeclaration | ImportEqualsDeclaration): CodeAction { - if (declaration.kind === SyntaxKind.ImportDeclaration) { - const namedBindings = declaration.importClause && declaration.importClause.namedBindings; - if (namedBindings) { - if (namedBindings.kind === SyntaxKind.NamespaceImport) { - return getCodeActionForNamespaceImport(namedBindings.name.getText()); + function getCodeActionForExistingImport(declarations: (ImportDeclaration | ImportEqualsDeclaration)[]): ImportCodeAction[] { + const actions: ImportCodeAction[] = []; + + // It is possible that multiple import statements with the same specifier exist in the file. + // e.g. + // // File 1 + // import * as ns from "foo"; + // import defaultFunction from "foo"; + // import { member1, member2 } from "foo"; + // + // member3 // <-- cusor here + // + // in this case we should provie 2 actions: + // 1. change "member3" to "ns.member3" + // 2. add "member3" to the third import statement's import list + // and it is up to the user to decide which one fits best. + let namespaceImport: ImportDeclaration | ImportEqualsDeclaration; + let namedImportWithImportList: ImportDeclaration; + let namedImportWithoutImportList: ImportDeclaration; + for (const declaration of declarations) { + if (declaration.kind === SyntaxKind.ImportDeclaration) { + const namedBindings = declaration.importClause && declaration.importClause.namedBindings; + if (namedBindings) { + if (namedBindings.kind === SyntaxKind.NamespaceImport) { + namespaceImport = declaration; + } + else { + namedImportWithImportList = declaration; + } } - /** - * If the existing import declaration already has a named import list, just - * insert the identifier into that list. - */ - const textChange = getTextChangeForImportList(namedBindings); - return createCodeAction( - Diagnostics.Add_0_to_existing_import_declaration_from_1, - [name, declaration.moduleSpecifier.getText()], - textChange.newText, - textChange.span, - sourceFile.fileName - ); + else { + namedImportWithoutImportList = declaration; + } + } + else { + namespaceImport = declaration; } + } + if (namespaceImport) { + actions.push(getCodeActionForNamespaceImport(namespaceImport)); + } + + if (namedImportWithImportList) { + /** + * If the existing import declaration already has a named import list, just + * insert the identifier into that list. + */ + const textChange = getTextChangeForImportList(namedImportWithImportList.importClause.namedBindings); + actions.push(createCodeAction( + Diagnostics.Add_0_to_existing_import_declaration_from_1, + [name, namedImportWithImportList.moduleSpecifier.getText()], + textChange.newText, + textChange.span, + sourceFile.fileName, + ImportCodeActionKind.InsertingIntoExistingImport + )); + } + else if (namedImportWithoutImportList) { // insert a new import statement in a new lines - return getCodeActionForNewImport(declaration.moduleSpecifier.getText(), declaration.getEnd()); + actions.push(getCodeActionForNewImport(namedImportWithoutImportList.moduleSpecifier.getText(), namedImportWithoutImportList.getEnd())); } - return getCodeActionForNamespaceImport(declaration.name.getText()); + return actions; function getTextChangeForImportList(importList: NamedImports): TextChange { + const newImportText = isDefaultImport ? `default as ${name}` : name; if (importList.elements.length === 0) { const start = importList.getStart(); return { - newText: `{ ${name} }`, + newText: `{ ${newImportText} }`, span: { start, length: importList.getEnd() - start } }; } @@ -141,12 +196,20 @@ namespace ts.codefix { } return { - newText: `,${oneImportPerLine ? context.newLineCharacter : ""}${name}`, + newText: `,${oneImportPerLine ? context.newLineCharacter : ""}${newImportText}`, span: { start: insertPoint, length: 0 } }; } - function getCodeActionForNamespaceImport(namespacePrefix: string): CodeAction { + function getCodeActionForNamespaceImport(declaration: ImportDeclaration | ImportEqualsDeclaration): ImportCodeAction { + let namespacePrefix: string; + if (declaration.kind === SyntaxKind.ImportDeclaration) { + namespacePrefix = (declaration.importClause.namedBindings).name.getText(); + } + else { + namespacePrefix = declaration.name.getText(); + } + /** * Cases: * import * as ns from "mod" @@ -162,12 +225,13 @@ namespace ts.codefix { [name, `${namespacePrefix}.${name}`], `${namespacePrefix}.`, { start: token.getStart(), length: 0 }, - sourceFile.fileName + sourceFile.fileName, + ImportCodeActionKind.CodeChange ); } } - function getCodeActionForNewImport(moduleSpecifier?: string, insertPos?: number): CodeAction { + function getCodeActionForNewImport(moduleSpecifier?: string, insertPos?: number): ImportCodeAction { // if not specified an insert position, try to insert after any existing imports if (!insertPos) { let lastModuleSpecifierEnd = -1; @@ -182,17 +246,20 @@ namespace ts.codefix { const getCanonicalFileName = createGetCanonicalFileName(useCaseSensitiveFileNames); moduleSpecifier = stripQuotes(moduleSpecifier || getModuleSpecifierForNewImport()); - const prefixNewLine = insertPos === sourceFile.getStart() ? "" : context.newLineCharacter; const importStatementText = isDefaultExport ? `import ${name} from "${moduleSpecifier}"` : `import { ${name} } from "${moduleSpecifier}"`; + const newText = insertPos === sourceFile.getStart() + ? `${importStatementText};${context.newLineCharacter}` + : `${context.newLineCharacter}${importStatementText};`; return createCodeAction( Diagnostics.Import_0_from_1, [name, `"${moduleSpecifier}"`], - `${prefixNewLine}${importStatementText};`, + newText, { start: insertPos, length: 0 }, - sourceFile.fileName + sourceFile.fileName, + moduleHasNonRelativeName(moduleSpecifier) ? ImportCodeActionKind.NewImportWithNonRelativeModuleSpecifier : ImportCodeActionKind.NewImportWithRelativeModuleSpecifier ); function getModuleSpecifierForNewImport() { @@ -202,7 +269,6 @@ namespace ts.codefix { const options = context.program.getCompilerOptions(); return tryGetModuleNameFromAmbientModule() || - tryGetModuleNameFromExistingUses() || tryGetModuleNameFromBaseUrl() || tryGetModuleNameFromRootDirs() || tryGetModuleNameFromTypeRoots() || @@ -219,25 +285,6 @@ namespace ts.codefix { } } - function tryGetModuleNameFromExistingUses(): string { - for (const file of context.program.getSourceFiles()) { - if (file === sourceFile || !file.resolvedModules) { - continue; - } - - for (const moduleName in file.resolvedModules) { - if (!moduleHasNonRelativeName(moduleName)) { - continue; - } - - const resolvedModule = file.resolvedModules[moduleName]; - if (resolvedModule && resolvedModule.resolvedFileName && normalizeFileName(resolvedModule.resolvedFileName) === moduleFileName) { - return moduleName; - } - } - } - } - function tryGetModuleNameFromBaseUrl() { if (!options.baseUrl) { return undefined; @@ -291,16 +338,18 @@ namespace ts.codefix { function tryGetModuleNameFromTypeRoots() { const typesRoots = getEffectiveTypeRoots(options, context.host); - for (const typeRoot of typesRoots) { - if (startsWith(moduleFileName, typeRoot)) { - let relativeFileName = moduleFileName.substring(typeRoot.length + 1); - relativeFileName = removeFileExtension(relativeFileName); + if (typesRoots) { + for (const typeRoot of typesRoots) { + if (startsWith(moduleFileName, typeRoot)) { + let relativeFileName = moduleFileName.substring(typeRoot.length + 1); + relativeFileName = removeFileExtension(relativeFileName); + + if (endsWith(relativeFileName, "/index")) { + relativeFileName = relativeFileName.substr(0, relativeFileName.length - 6/* "/index".length */); + } - if (endsWith(relativeFileName, "/index")) { - relativeFileName = relativeFileName.substr(0, relativeFileName.length - 6/* "/index".length */); + return relativeFileName; } - - return relativeFileName; } } } @@ -382,10 +431,11 @@ namespace ts.codefix { } - function createCodeAction(description: DiagnosticMessage, diagnosticArgs: string[], newText: string, span: TextSpan, fileName: string): CodeAction { + function createCodeAction(description: DiagnosticMessage, diagnosticArgs: string[], newText: string, span: TextSpan, fileName: string, kind: ImportCodeActionKind): ImportCodeAction { return { description: formatMessage.apply(undefined, [undefined, description].concat(diagnosticArgs)), - changes: [{ fileName, textChanges: [{ newText, span }] }] + changes: [{ fileName, textChanges: [{ newText, span }] }], + kind }; } } From 26cd24aa201b85a18e9f6db284475c4579975f76 Mon Sep 17 00:00:00 2001 From: zhengbli Date: Fri, 11 Nov 2016 00:23:58 -0800 Subject: [PATCH 09/14] add multiple code fixes and code action filtering and polishing --- src/compiler/checker.ts | 12 +- src/compiler/types.ts | 5 +- src/harness/fourslash.ts | 34 ++- src/services/codefixes/importFixes.ts | 246 ++++++++++-------- src/services/completions.ts | 4 +- tests/cases/fourslash/fourslash.ts | 2 +- .../importNameCodeFixExistingImport0.ts | 2 +- .../importNameCodeFixExistingImport1.ts | 2 +- .../importNameCodeFixExistingImport10.ts | 12 +- .../importNameCodeFixExistingImport11.ts | 12 +- .../importNameCodeFixExistingImport12.ts | 2 +- .../importNameCodeFixExistingImport2.ts | 12 +- .../importNameCodeFixExistingImport3.ts | 12 +- .../importNameCodeFixExistingImport4.ts | 7 +- .../importNameCodeFixExistingImport5.ts | 4 +- .../importNameCodeFixExistingImport6.ts | 2 +- .../importNameCodeFixExistingImport7.ts | 2 +- .../importNameCodeFixExistingImport8.ts | 2 +- .../importNameCodeFixExistingImport9.ts | 10 +- .../importNameCodeFixNewImportAmbient0.ts | 5 +- .../importNameCodeFixNewImportAmbient1.ts | 4 +- .../importNameCodeFixNewImportAmbient2.ts | 17 +- .../importNameCodeFixNewImportAmbient3.ts | 7 +- .../importNameCodeFixNewImportBaseUrl0.ts | 5 +- .../importNameCodeFixNewImportDefault0.ts | 5 +- .../importNameCodeFixNewImportFile0.ts | 5 +- .../importNameCodeFixNewImportFile1.ts | 5 +- .../importNameCodeFixNewImportFile2.ts | 5 +- .../importNameCodeFixNewImportNodeModules0.ts | 5 +- .../importNameCodeFixNewImportNodeModules1.ts | 5 +- .../importNameCodeFixNewImportNodeModules2.ts | 5 +- .../importNameCodeFixNewImportNodeModules3.ts | 5 +- .../importNameCodeFixNewImportPaths0.ts | 7 +- .../importNameCodeFixNewImportPaths1.ts | 7 +- .../importNameCodeFixNewImportRootDirs0.ts | 7 +- .../importNameCodeFixNewImportTypeRoots0.ts | 7 +- tests/cases/fourslash/server/codefix.ts | 4 +- tests/cases/fourslash/superFix1.ts | 2 +- tests/cases/fourslash/superFix2.ts | 2 +- 39 files changed, 286 insertions(+), 212 deletions(-) diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts index 8add0b4be2324..8fca1b893cdfd 100644 --- a/src/compiler/checker.ts +++ b/src/compiler/checker.ts @@ -102,13 +102,12 @@ namespace ts { isImplementationOfOverload, getAliasedSymbol: resolveAlias, getEmitResolver, - getExportsOfModuleAsArray, - getExportsOfModule, + getExportsOfModule: getExportsOfModuleAsArray, getAmbientModules, - getJsxElementAttributesType, getJsxIntrinsicTagNames, isOptionalParameter, + tryGetMemberInModuleExports, tryFindAmbientModuleWithoutAugmentations: moduleName => { // we deliberately exclude augmentations // since we are only interested in declarations of the module itself @@ -1465,6 +1464,13 @@ namespace ts { return symbolsToArray(getExportsOfModule(moduleSymbol)); } + function tryGetMemberInModuleExports(memberName: string, moduleSymbol: Symbol): Symbol | undefined { + const symbolTable = getExportsOfModule(moduleSymbol); + if (symbolTable) { + return symbolTable[memberName]; + } + } + function getExportsOfSymbol(symbol: Symbol): SymbolTable { return symbol.flags & SymbolFlags.Module ? getExportsOfModule(symbol) : symbol.exports || emptySymbols; } diff --git a/src/compiler/types.ts b/src/compiler/types.ts index cb372a82c97ec..48b0e5e513446 100644 --- a/src/compiler/types.ts +++ b/src/compiler/types.ts @@ -2291,14 +2291,15 @@ namespace ts { getConstantValue(node: EnumMember | PropertyAccessExpression | ElementAccessExpression): number; isValidPropertyAccess(node: PropertyAccessExpression | QualifiedName, propertyName: string): boolean; getAliasedSymbol(symbol: Symbol): Symbol; - getExportsOfModuleAsArray(moduleSymbol: Symbol): Symbol[]; - getExportsOfModule(moduleSymbol: Symbol): Map | undefined; + getExportsOfModule(moduleSymbol: Symbol): Symbol[]; getJsxElementAttributesType(elementNode: JsxOpeningLikeElement): Type; getJsxIntrinsicTagNames(): Symbol[]; isOptionalParameter(node: ParameterDeclaration): boolean; getAmbientModules(): Symbol[]; + tryGetMemberInModuleExports(memberName: string, moduleSymbol: Symbol): Symbol | undefined; + /* @internal */ tryFindAmbientModuleWithoutAugmentations(moduleName: string): Symbol; // Should not be called directly. Should only be accessed through the Program instance. diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index b0384951ab7a2..94e50c8927933 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -2018,27 +2018,31 @@ namespace FourSlash { return this.languageService.getCodeFixesAtPosition(fileName, diagnostic.start, diagnostic.length, [diagnostic.code]); } - public verifyCodeFixAtPosition(expectedText: string, errorCode?: number) { + public verifyCodeFixAtPosition(expectedTextArray: string[], errorCode?: number) { const ranges = this.getRanges(); if (ranges.length == 0) { this.raiseError("At least one range should be specified in the testfile."); } - const actual = this.getCodeFixes(errorCode); + const codeFixes = this.getCodeFixes(errorCode); - if (!actual || actual.length == 0) { + if (!codeFixes || codeFixes.length == 0) { this.raiseError("No codefixes returned."); } - if (actual.length > 1) { - this.raiseError("More than 1 codefix returned."); + const actualTextArray: string[] = []; + const scriptInfo = this.languageServiceAdapterHost.getScriptInfo(codeFixes[0].changes[0].fileName); + const originalContent = scriptInfo.content; + for (const codeFix of codeFixes) { + this.applyEdits(codeFix.changes[0].fileName, codeFix.changes[0].textChanges, /*isFormattingEdit*/ false); + actualTextArray.push(this.normalizeNewlines(this.rangeText(ranges[0]))); + scriptInfo.updateContent(originalContent); } - - this.applyEdits(actual[0].changes[0].fileName, actual[0].changes[0].textChanges, /*isFormattingEdit*/ false); - const actualText = this.rangeText(ranges[0]); - - if (this.removeWhitespace(actualText) !== this.removeWhitespace(expectedText)) { - this.raiseError(`Actual text doesn't match expected text. Actual: '${actualText}' Expected: '${expectedText}'`); + const sortedExpectedArray = ts.map(expectedTextArray, str => this.normalizeNewlines(str)).sort(); + const sortedActualArray = actualTextArray.sort(); + if (!ts.arrayIsEqualTo(sortedExpectedArray, sortedActualArray)) { + this.raiseError( + `Actual text array doesn't match expected text array. \nActual: \n'${sortedActualArray.join('\n\n')}'\n---\nExpected: \n'${sortedExpectedArray.join('\n\n')}'`); } } @@ -2075,6 +2079,10 @@ namespace FourSlash { }); } + private normalizeNewlines(str: string) { + return str.replace(/\r?\n/g, "\n"); + } + public verifyBraceCompletionAtPosition(negative: boolean, openingBrace: string) { const openBraceMap = ts.createMap({ @@ -3291,8 +3299,8 @@ namespace FourSlashInterface { this.DocCommentTemplate(/*expectedText*/ undefined, /*expectedOffset*/ undefined, /*empty*/ true); } - public codeFixAtPosition(expectedText: string, errorCode?: number): void { - this.state.verifyCodeFixAtPosition(expectedText, errorCode); + public codeFixAtPosition(expectedTextArray: string[], errorCode?: number): void { + this.state.verifyCodeFixAtPosition(expectedTextArray, errorCode); } public navigationBar(json: any) { diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 401456e3abd49..ae45c86932858 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -1,17 +1,10 @@ /* @internal */ namespace ts.codefix { - // The order of the kinds also determines their priority. - // the ones comes first should be presenteed to the user first too. - enum ImportCodeActionKind { - CodeChange, - InsertingIntoExistingImport, - NewImportWithNonRelativeModuleSpecifier, - NewImportWithRelativeModuleSpecifier, - } - + type ImportCodeActionKind = "CodeChange" | "InsertingIntoExistingImport" | "NewImport"; interface ImportCodeAction extends CodeAction { - kind: ImportCodeActionKind + kind: ImportCodeActionKind, + moduleSpecifier?: string } registerCodeFix({ @@ -37,13 +30,8 @@ namespace ts.codefix { for (const moduleSymbol of allPotentialModules) { context.cancellationToken.throwIfCancellationRequested(); - const moduleExports = checker.getExportsOfModule(moduleSymbol); - if (!moduleExports) { - continue; - } - // check the default export - const defaultExport = moduleExports["default"]; + const defaultExport = checker.tryGetMemberInModuleExports("default", moduleSymbol); if (defaultExport) { const localSymbol = getLocalSymbolForExportDefault(defaultExport); if (localSymbol && localSymbol.name === name && checkSymbolHasMeaning(localSymbol, currentTokenMeaning)) { @@ -52,23 +40,38 @@ namespace ts.codefix { } // check exports with the same name - if (name in moduleExports) { - if (checkSymbolHasMeaning(moduleExports[name], currentTokenMeaning)) { - addRange(allActions, getCodeActionForImport(moduleSymbol)); - } + const exportWithIdenticalName = checker.tryGetMemberInModuleExports(name, moduleSymbol); + if (exportWithIdenticalName && checkSymbolHasMeaning(exportWithIdenticalName, currentTokenMeaning)) { + addRange(allActions, getCodeActionForImport(moduleSymbol)); } } //sort the code actions + const confirmedActions: ImportCodeAction[] = []; + for (const action of allActions) { + switch (action.kind) { + case "CodeChange": + case "InsertingIntoExistingImport": + confirmedActions.push(action); + break; + case "NewImport": + if (action.moduleSpecifier && + some(allActions, otherAction => action.moduleSpecifier !== otherAction.moduleSpecifier && action.moduleSpecifier.indexOf(otherAction.moduleSpecifier) === 0)) { + // there is another action with a shorter module specifier. Use that instead. + continue; + } + confirmedActions.push(action); + } + } - return allActions; + return confirmedActions; function checkSymbolHasMeaning(symbol: Symbol, meaning: SemanticMeaning) { const declarations = symbol.getDeclarations(); return declarations ? some(symbol.declarations, decl => !!(getMeaningFromDeclaration(decl) & meaning)) : false; } - function getCodeActionForImport(moduleSymbol: Symbol, isDefaultExport?: boolean): ImportCodeAction[] { + function getCodeActionForImport(moduleSymbol: Symbol, isDefault?: boolean): ImportCodeAction[] { // Check to see if there are already imports being made from this source in the current file const existingDeclarations: (ImportDeclaration | ImportEqualsDeclaration)[] = []; for (const importModuleSpecifier of sourceFile.imports) { @@ -79,7 +82,8 @@ namespace ts.codefix { } if (existingDeclarations.length > 0) { - return getCodeActionForExistingImport(existingDeclarations); + // With an existing import statement, there are more than one actions the user can do. + return getCodeActionsForExistingImport(existingDeclarations); } else { return [getCodeActionForNewImport()]; @@ -99,72 +103,94 @@ namespace ts.codefix { return undefined; } - function getCodeActionForExistingImport(declarations: (ImportDeclaration | ImportEqualsDeclaration)[]): ImportCodeAction[] { + function getCodeActionsForExistingImport(declarations: (ImportDeclaration | ImportEqualsDeclaration)[]): ImportCodeAction[] { const actions: ImportCodeAction[] = []; // It is possible that multiple import statements with the same specifier exist in the file. // e.g. - // // File 1 + // // import * as ns from "foo"; - // import defaultFunction from "foo"; // import { member1, member2 } from "foo"; - // - // member3 // <-- cusor here + // + // member3/**/ <-- cusor here // // in this case we should provie 2 actions: // 1. change "member3" to "ns.member3" - // 2. add "member3" to the third import statement's import list + // 2. add "member3" to the second import statement's import list // and it is up to the user to decide which one fits best. - let namespaceImport: ImportDeclaration | ImportEqualsDeclaration; - let namedImportWithImportList: ImportDeclaration; - let namedImportWithoutImportList: ImportDeclaration; + let namespaceImportDeclaration: ImportDeclaration | ImportEqualsDeclaration; + let namedImportDeclaration: ImportDeclaration; + let existingModuleSpecifier: string; for (const declaration of declarations) { if (declaration.kind === SyntaxKind.ImportDeclaration) { const namedBindings = declaration.importClause && declaration.importClause.namedBindings; - if (namedBindings) { - if (namedBindings.kind === SyntaxKind.NamespaceImport) { - namespaceImport = declaration; - } - else { - namedImportWithImportList = declaration; - } + if (namedBindings && namedBindings.kind === SyntaxKind.NamespaceImport) { + // case: + // import * as ns from "foo" + namespaceImportDeclaration = declaration; } else { - namedImportWithoutImportList = declaration; + // cases: + // import default from "foo" + // import { bar } from "foo" or combination with the first one + // import "foo" + namedImportDeclaration = declaration; } + existingModuleSpecifier = declaration.moduleSpecifier.getText(); } else { - namespaceImport = declaration; + // case: + // import foo = require("foo") + namespaceImportDeclaration = declaration; + existingModuleSpecifier = declaration.name.getText(); } } - if (namespaceImport) { - actions.push(getCodeActionForNamespaceImport(namespaceImport)); + if (namespaceImportDeclaration) { + actions.push(getCodeActionForNamespaceImport(namespaceImportDeclaration)); } - if (namedImportWithImportList) { + if (namedImportDeclaration && namedImportDeclaration.importClause && + (namedImportDeclaration.importClause.name || namedImportDeclaration.importClause.namedBindings)) { /** * If the existing import declaration already has a named import list, just * insert the identifier into that list. */ - const textChange = getTextChangeForImportList(namedImportWithImportList.importClause.namedBindings); + const textChange = getTextChangeForImportClause(namedImportDeclaration.importClause); + const moduleSpecifierWithoutQuotes = stripQuotes(namedImportDeclaration.moduleSpecifier.getText()); actions.push(createCodeAction( Diagnostics.Add_0_to_existing_import_declaration_from_1, - [name, namedImportWithImportList.moduleSpecifier.getText()], + [name, moduleSpecifierWithoutQuotes], textChange.newText, textChange.span, sourceFile.fileName, - ImportCodeActionKind.InsertingIntoExistingImport + "InsertingIntoExistingImport", + moduleSpecifierWithoutQuotes )); } - else if (namedImportWithoutImportList) { - // insert a new import statement in a new lines - actions.push(getCodeActionForNewImport(namedImportWithoutImportList.moduleSpecifier.getText(), namedImportWithoutImportList.getEnd())); + else { + // we need to create a new import statement, but the existing module specifier can be reused. + actions.push(getCodeActionForNewImport(existingModuleSpecifier)); } return actions; - function getTextChangeForImportList(importList: NamedImports): TextChange { - const newImportText = isDefaultImport ? `default as ${name}` : name; + function getTextChangeForImportClause(importClause: ImportClause): TextChange { + const newImportText = isDefault ? `default as ${name}` : name; + const importList = importClause.namedBindings; + // case 1: + // original text: import default from "module" + // change to: import default, { name } from "module" + if (!importList && importClause.name) { + const start = importClause.name.getEnd(); + return { + newText: `, { ${newImportText} }`, + span: { start, length: 0 } + }; + } + + // case 2: + // original text: import {} from "module" + // change to: import { name } from "module" if (importList.elements.length === 0) { const start = importList.getStart(); return { @@ -173,9 +199,10 @@ namespace ts.codefix { }; } - // Insert after the last element + // case 3: + // original text: import { foo, bar } from "module" + // change to: import { foo, bar, name } from "module" const insertPoint = importList.elements[importList.elements.length - 1].getEnd(); - // If the import list has one import per line, preserve that. Otherwise, insert on same line as last element let oneImportPerLine: boolean; if (importList.elements.length === 1) { @@ -196,7 +223,7 @@ namespace ts.codefix { } return { - newText: `,${oneImportPerLine ? context.newLineCharacter : ""}${newImportText}`, + newText: `,${oneImportPerLine ? context.newLineCharacter : " "}${newImportText}`, span: { start: insertPoint, length: 0 } }; } @@ -226,45 +253,48 @@ namespace ts.codefix { `${namespacePrefix}.`, { start: token.getStart(), length: 0 }, sourceFile.fileName, - ImportCodeActionKind.CodeChange + "CodeChange" ); } } - function getCodeActionForNewImport(moduleSpecifier?: string, insertPos?: number): ImportCodeAction { - // if not specified an insert position, try to insert after any existing imports - if (!insertPos) { - let lastModuleSpecifierEnd = -1; - for (const moduleSpecifier of sourceFile.imports) { - const end = moduleSpecifier.getEnd(); - if (!lastModuleSpecifierEnd || end > lastModuleSpecifierEnd) { - lastModuleSpecifierEnd = end; - } + function getCodeActionForNewImport(moduleSpecifier?: string): ImportCodeAction { + // insert after any existing imports + let lastModuleSpecifierEnd = -1; + for (const moduleSpecifier of sourceFile.imports) { + const end = moduleSpecifier.getEnd(); + if (!lastModuleSpecifierEnd || end > lastModuleSpecifierEnd) { + lastModuleSpecifierEnd = end; } - insertPos = lastModuleSpecifierEnd > 0 ? lastModuleSpecifierEnd + 1 : sourceFile.getStart(); } + const insertPos = lastModuleSpecifierEnd > 0 ? lastModuleSpecifierEnd + 1 : sourceFile.getStart(); const getCanonicalFileName = createGetCanonicalFileName(useCaseSensitiveFileNames); - moduleSpecifier = stripQuotes(moduleSpecifier || getModuleSpecifierForNewImport()); - const importStatementText = isDefaultExport - ? `import ${name} from "${moduleSpecifier}"` - : `import { ${name} } from "${moduleSpecifier}"`; - const newText = insertPos === sourceFile.getStart() - ? `${importStatementText};${context.newLineCharacter}` + const moduleSpecifierWithoutQuotes = stripQuotes(moduleSpecifier || getModuleSpecifierForNewImport()); + const importStatementText = isDefault + ? `import ${name} from "${moduleSpecifierWithoutQuotes}"` + : `import { ${name} } from "${moduleSpecifierWithoutQuotes}"`; + + // if this file doesn't have any import statements, insert an import statement and then insert a new line + // between the only import statement and user code. Otherwise just insert the statement because chances + // are there are already a new line seperating code and import statements. + const newText = insertPos === sourceFile.getStart() + ? `${importStatementText};${context.newLineCharacter}${context.newLineCharacter}` : `${context.newLineCharacter}${importStatementText};`; return createCodeAction( Diagnostics.Import_0_from_1, - [name, `"${moduleSpecifier}"`], + [name, `"${moduleSpecifierWithoutQuotes}"`], newText, { start: insertPos, length: 0 }, sourceFile.fileName, - moduleHasNonRelativeName(moduleSpecifier) ? ImportCodeActionKind.NewImportWithNonRelativeModuleSpecifier : ImportCodeActionKind.NewImportWithRelativeModuleSpecifier + "NewImport", + moduleSpecifierWithoutQuotes ); function getModuleSpecifierForNewImport() { - const fileName = normalizeFileName(sourceFile.fileName); - const moduleFileName = normalizeFileName(moduleSymbol.valueDeclaration.getSourceFile().fileName); + const fileName = sourceFile.path; + const moduleFileName = moduleSymbol.valueDeclaration.getSourceFile().path; const sourceDirectory = getDirectoryPath(fileName); const options = context.program.getCompilerOptions(); @@ -275,10 +305,6 @@ namespace ts.codefix { tryGetModuleNameAsNodeModule() || removeFileExtension(getRelativePath(moduleFileName, sourceDirectory)); - function normalizeFileName(fileName: string) { - return getCanonicalFileName(normalizeSlashes(fileName)); - } - function tryGetModuleNameFromAmbientModule(): string { if (moduleSymbol.valueDeclaration.kind !== SyntaxKind.SourceFile) { return moduleSymbol.name; @@ -290,12 +316,13 @@ namespace ts.codefix { return undefined; } - let relativeName = tryRemoveParentDirectoryName(moduleFileName, options.baseUrl); + const normalizedBaseUrl = toPath(options.baseUrl, getDirectoryPath(options.baseUrl), getCanonicalFileName); + let relativeName = tryRemoveParentDirectoryName(moduleFileName, normalizedBaseUrl); if (!relativeName) { return undefined; } - relativeName = removeFileExtension(relativeName); + relativeName = removeExtensionAndIndexPostFix(relativeName); if (options.paths) { for (const key in options.paths) { @@ -326,8 +353,9 @@ namespace ts.codefix { function tryGetModuleNameFromRootDirs() { if (options.rootDirs) { - const normalizedTargetPath = getPathRelativeToRootDirs(moduleFileName, options.rootDirs); - const normalizedSourcePath = getPathRelativeToRootDirs(sourceDirectory, options.rootDirs); + const normalizedRootDirs = map(options.rootDirs, rootDir => toPath(rootDir, /*basePath*/ undefined, getCanonicalFileName)); + const normalizedTargetPath = getPathRelativeToRootDirs(moduleFileName, normalizedRootDirs); + const normalizedSourcePath = getPathRelativeToRootDirs(sourceDirectory, normalizedRootDirs); if (normalizedTargetPath !== undefined) { const relativePath = normalizedSourcePath !== undefined ? getRelativePath(normalizedTargetPath, normalizedSourcePath) : normalizedTargetPath; return removeFileExtension(relativePath); @@ -337,18 +365,13 @@ namespace ts.codefix { } function tryGetModuleNameFromTypeRoots() { - const typesRoots = getEffectiveTypeRoots(options, context.host); - if (typesRoots) { - for (const typeRoot of typesRoots) { + const typeRoots = getEffectiveTypeRoots(options, context.host); + if (typeRoots) { + const normalizedTypeRoots = map(typeRoots, typeRoot => toPath(typeRoot, /*basePath*/ undefined, getCanonicalFileName)); + for (const typeRoot of normalizedTypeRoots) { if (startsWith(moduleFileName, typeRoot)) { let relativeFileName = moduleFileName.substring(typeRoot.length + 1); - relativeFileName = removeFileExtension(relativeFileName); - - if (endsWith(relativeFileName, "/index")) { - relativeFileName = relativeFileName.substr(0, relativeFileName.length - 6/* "/index".length */); - } - - return relativeFileName; + return removeExtensionAndIndexPostFix(relativeFileName); } } } @@ -375,11 +398,6 @@ namespace ts.codefix { } relativeFileName = removeFileExtension(relativeFileName); - - if (startsWith(relativeFileName, "@types/")) { - relativeFileName = relativeFileName.substr(7 /*"@types/.length"*/); - } - if (endsWith(relativeFileName, "/index")) { relativeFileName = getDirectoryPath(relativeFileName); } @@ -388,9 +406,7 @@ namespace ts.codefix { const moduleDirectory = getDirectoryPath(moduleFileName); const packageJsonContent = JSON.parse(context.host.readFile(combinePaths(moduleDirectory, "package.json"))); if (packageJsonContent && packageJsonContent.main) { - const mainExportFile = isRootedDiskPath(packageJsonContent.main) - ? normalizeFileName(packageJsonContent.main) - : getCanonicalFileName(normalizePath(combinePaths(moduleDirectory, packageJsonContent.main))); + const mainExportFile = toPath(packageJsonContent.main, moduleDirectory, getCanonicalFileName); if (removeFileExtension(mainExportFile) === removeFileExtension(moduleFileName)) { relativeFileName = getDirectoryPath(relativeFileName); } @@ -403,9 +419,9 @@ namespace ts.codefix { } } - function getPathRelativeToRootDirs(fileName: string, rootDirs: string[]) { + function getPathRelativeToRootDirs(path: Path, rootDirs: Path[]) { for (const rootDir of rootDirs) { - const relativeName = tryRemoveParentDirectoryName(fileName, rootDir); + const relativeName = tryRemoveParentDirectoryName(path, rootDir); if (relativeName !== undefined) { return relativeName; } @@ -413,12 +429,20 @@ namespace ts.codefix { return undefined; } + function removeExtensionAndIndexPostFix(fileName: string) { + fileName = removeFileExtension(fileName); + if (endsWith(fileName, "/index")) { + fileName = fileName.substr(0, fileName.length - 6/* "/index".length */); + } + return fileName; + } + function getRelativePath(path: string, directoryPath: string) { const relativePath = getRelativePathToDirectoryOrUrl(directoryPath, path, directoryPath, getCanonicalFileName, false); return moduleHasNonRelativeName(relativePath) ? "./" + relativePath : relativePath; } - function tryRemoveParentDirectoryName(path: string, parentDirectory: string) { + function tryRemoveParentDirectoryName(path: Path, parentDirectory: Path) { const index = path.indexOf(parentDirectory); if (index === 0) { return endsWith(parentDirectory, directorySeparator) @@ -431,11 +455,19 @@ namespace ts.codefix { } - function createCodeAction(description: DiagnosticMessage, diagnosticArgs: string[], newText: string, span: TextSpan, fileName: string, kind: ImportCodeActionKind): ImportCodeAction { + function createCodeAction( + description: DiagnosticMessage, + diagnosticArgs: string[], + newText: string, + span: TextSpan, + fileName: string, + kind: ImportCodeActionKind, + moduleSpecifier?: string): ImportCodeAction { return { description: formatMessage.apply(undefined, [undefined, description].concat(diagnosticArgs)), changes: [{ fileName, textChanges: [{ newText, span }] }], - kind + kind, + moduleSpecifier }; } } diff --git a/src/services/completions.ts b/src/services/completions.ts index 9705f07e1a12b..b710aa8cd78bb 100644 --- a/src/services/completions.ts +++ b/src/services/completions.ts @@ -975,7 +975,7 @@ namespace ts.Completions { if (symbol && symbol.flags & SymbolFlags.HasExports) { // Extract module or enum members - const exportedSymbols = typeChecker.getExportsOfModuleAsArray(symbol); + const exportedSymbols = typeChecker.getExportsOfModule(symbol); forEach(exportedSymbols, symbol => { if (typeChecker.isValidPropertyAccess((node.parent), symbol.name)) { symbols.push(symbol); @@ -1320,7 +1320,7 @@ namespace ts.Completions { let exports: Symbol[]; const moduleSpecifierSymbol = typeChecker.getSymbolAtLocation(importOrExportDeclaration.moduleSpecifier); if (moduleSpecifierSymbol) { - exports = typeChecker.getExportsOfModuleAsArray(moduleSpecifierSymbol); + exports = typeChecker.getExportsOfModule(moduleSpecifierSymbol); } symbols = exports ? filterNamedImportOrExportCompletionItems(exports, namedImportsOrExports.elements) : emptyArray; diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts index 0a72d29529586..446277c029832 100644 --- a/tests/cases/fourslash/fourslash.ts +++ b/tests/cases/fourslash/fourslash.ts @@ -209,7 +209,7 @@ declare namespace FourSlashInterface { noMatchingBracePositionInCurrentFile(bracePosition: number): void; DocCommentTemplate(expectedText: string, expectedOffset: number, empty?: boolean): void; noDocCommentTemplate(): void; - codeFixAtPosition(expectedText: string, errorCode?: number): void; + codeFixAtPosition(expectedTextArray: string[], errorCode?: number): void; navigationBar(json: any): void; navigationTree(json: any): void; diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport0.ts b/tests/cases/fourslash/importNameCodeFixExistingImport0.ts index 42f8737468f33..f532af86d3d46 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport0.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport0.ts @@ -7,4 +7,4 @@ //// export function f1() {} //// export var v1 = 5; -verify.codeFixAtPosition(`{ v1, f1 }`); \ No newline at end of file +verify.codeFixAtPosition([`{ v1, f1 }`]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport1.ts b/tests/cases/fourslash/importNameCodeFixExistingImport1.ts index cc376025a736f..0f72631e2cc03 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport1.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport1.ts @@ -8,4 +8,4 @@ //// export var v1 = 5; //// export default var d1 = 6; -verify.codeFixAtPosition(`{ v1, f1 }`); \ No newline at end of file +verify.codeFixAtPosition([`{ v1, f1 }`]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport10.ts b/tests/cases/fourslash/importNameCodeFixExistingImport10.ts index e78b279e178da..a1aa80eb7118b 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport10.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport10.ts @@ -1,8 +1,8 @@ /// //// import [|{ -//// v1, -//// v2 +//// v1, +//// v2 //// }|] from "./module"; //// f1/*0*/(); @@ -12,10 +12,10 @@ //// export var v2 = 5; //// export var v3 = 5; -verify.codeFixAtPosition( +verify.codeFixAtPosition([ `{ v1, v2, - f1 - }` -); \ No newline at end of file +f1 +}` +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport11.ts b/tests/cases/fourslash/importNameCodeFixExistingImport11.ts index 0030ad9f3b320..b0119dd00ee10 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport11.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport11.ts @@ -1,10 +1,10 @@ /// -//// import [|{ +////import [|{ //// v1, v2, //// v3 -//// }|] from "./module"; -//// f1/*0*/(); +////}|] from "./module"; +////f1/*0*/(); // @Filename: module.ts //// export function f1() {} @@ -12,9 +12,9 @@ //// export var v2 = 5; //// export var v3 = 5; -verify.codeFixAtPosition( +verify.codeFixAtPosition([ `{ v1, v2, v3, f1 - }` -); \ No newline at end of file +}` +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport12.ts b/tests/cases/fourslash/importNameCodeFixExistingImport12.ts index d5d2feeb8c17c..04c66cb055bf6 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport12.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport12.ts @@ -9,4 +9,4 @@ //// export var v2 = 5; //// export var v3 = 5; -verify.codeFixAtPosition(`{ f1 }`); \ No newline at end of file +verify.codeFixAtPosition([`{ f1 }`]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport2.ts b/tests/cases/fourslash/importNameCodeFixExistingImport2.ts index 179d80a7ecfe1..eccd6c9601f90 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport2.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport2.ts @@ -1,10 +1,16 @@ /// -//// import * as ns from "./module"; -//// [|f1|]/*0*/(); +//// [|import * as ns from "./module"; +//// f1/*0*/();|] // @Filename: module.ts //// export function f1() {} //// export var v1 = 5; -verify.codeFixAtPosition(`ns.f1`); \ No newline at end of file +verify.codeFixAtPosition([ +`import * as ns from "./module"; +import { f1 } from "./module"; +f1();`, +`import * as ns from "./module"; +ns.f1();` +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport3.ts b/tests/cases/fourslash/importNameCodeFixExistingImport3.ts index 34773cac9d563..3cd073c42159d 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport3.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport3.ts @@ -1,11 +1,17 @@ /// -//// import d, * as ns from "./module"; -//// [|f1|]/*0*/(); +//// [|import d, * as ns from "./module"; +//// f1/*0*/();|] // @Filename: module.ts //// export function f1() {} //// export var v1 = 5; //// export default var d1 = 6; -verify.codeFixAtPosition(`ns.f1`); \ No newline at end of file +verify.codeFixAtPosition([ +`import d, * as ns from "./module"; +ns.f1();`, +`import d, * as ns from "./module"; +import { f1 } from "./module"; +f1();`, +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport4.ts b/tests/cases/fourslash/importNameCodeFixExistingImport4.ts index a23ecff1fe48e..c7a169ae76106 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport4.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport4.ts @@ -8,6 +8,7 @@ //// export var v1 = 5; //// export default var d1 = 6; -verify.codeFixAtPosition(`import d from "./module"; -import { f1 } from "./module"; -f1();`); \ No newline at end of file +verify.codeFixAtPosition([ +`import d, { f1 } from "./module"; +f1();` +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport5.ts b/tests/cases/fourslash/importNameCodeFixExistingImport5.ts index ae62c99446154..60c12fad6981d 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport5.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport5.ts @@ -7,6 +7,6 @@ //// export function f1() {} //// export var v1 = 5; -verify.codeFixAtPosition(`import "./module"; +verify.codeFixAtPosition([`import "./module"; import { f1 } from "./module"; -f1();`); \ No newline at end of file +f1();`]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport6.ts b/tests/cases/fourslash/importNameCodeFixExistingImport6.ts index 141a8cac9468a..61b809c76ef35 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport6.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport6.ts @@ -10,4 +10,4 @@ //// export var v1 = 5; //// export function f1(); -verify.codeFixAtPosition(`{ v1, f1 }`); \ No newline at end of file +verify.codeFixAtPosition([`{ v1, f1 }`]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport7.ts b/tests/cases/fourslash/importNameCodeFixExistingImport7.ts index 56a172ecfe0c1..d98743189cd6a 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport7.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport7.ts @@ -7,4 +7,4 @@ //// export var v1 = 5; //// export function f1(); -verify.codeFixAtPosition(`{ v1, f1 }`); \ No newline at end of file +verify.codeFixAtPosition([`{ v1, f1 }`]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport8.ts b/tests/cases/fourslash/importNameCodeFixExistingImport8.ts index 971e90e7056b2..0a59eadbb0014 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport8.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport8.ts @@ -9,4 +9,4 @@ //// export var v2 = 5; //// export var v3 = 5; -verify.codeFixAtPosition(`{v1, v2, v3, f1,}`); \ No newline at end of file +verify.codeFixAtPosition([`{v1, v2, v3, f1,}`]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport9.ts b/tests/cases/fourslash/importNameCodeFixExistingImport9.ts index 6d0508e9f4fbb..0e4210620cc1c 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport9.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport9.ts @@ -1,7 +1,7 @@ /// //// import [|{ -//// v1 +//// v1 //// }|] from "./module"; //// f1/*0*/(); @@ -9,9 +9,9 @@ //// export function f1() {} //// export var v1 = 5; -verify.codeFixAtPosition( +verify.codeFixAtPosition([ `{ v1, - f1 - }` -); \ No newline at end of file +f1 +}` +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportAmbient0.ts b/tests/cases/fourslash/importNameCodeFixNewImportAmbient0.ts index 1b0d4b3ce1904..ecbcc2889635d 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportAmbient0.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportAmbient0.ts @@ -8,7 +8,8 @@ //// export var v1; //// } -verify.codeFixAtPosition( +verify.codeFixAtPosition([ `import { f1 } from "ambient-module"; + f1();` -); \ No newline at end of file +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportAmbient1.ts b/tests/cases/fourslash/importNameCodeFixNewImportAmbient1.ts index 469ac353f0e7a..acba2ebfaa17d 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportAmbient1.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportAmbient1.ts @@ -21,8 +21,8 @@ //// export var v3; //// } -verify.codeFixAtPosition( +verify.codeFixAtPosition([ `import * as ns from "yet-another-ambient-module"; import { v1 } from "ambient-module"; var x = v1 + 5;` -); \ No newline at end of file +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportAmbient2.ts b/tests/cases/fourslash/importNameCodeFixNewImportAmbient2.ts index a809ba014c26e..f830c8b86543e 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportAmbient2.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportAmbient2.ts @@ -1,9 +1,9 @@ /// -//// [|/* -//// * I'm a license or something -//// */ -//// f1/*0*/();|] +////[|/* +//// * I'm a license or something +//// */ +////f1/*0*/();|] // @Filename: ambientModule.ts //// declare module "ambient-module" { @@ -11,10 +11,11 @@ //// export var v1; //// } -verify.codeFixAtPosition( +verify.codeFixAtPosition([ `/* -* I'm a license or something -*/ + * I'm a license or something + */ import { f1 } from "ambient-module"; + f1();` -); \ No newline at end of file +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportAmbient3.ts b/tests/cases/fourslash/importNameCodeFixNewImportAmbient3.ts index 5ccafa9141188..072b80f6eec28 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportAmbient3.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportAmbient3.ts @@ -22,9 +22,8 @@ //// export var v3; //// } -verify.codeFixAtPosition( -` -import * as ns from "yet-another-ambient-module"; +verify.codeFixAtPosition([ +`import * as ns from "yet-another-ambient-module"; import { v1 } from "ambient-module"; var x = v1 + 5;` -); \ No newline at end of file +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl0.ts b/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl0.ts index 72e7174ba3b48..4fb316ab7fb6c 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl0.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl0.ts @@ -12,7 +12,8 @@ // @Filename: a/b.ts //// export function f1() { }; -verify.codeFixAtPosition( +verify.codeFixAtPosition([ `import { f1 } from "b"; + f1();` -); \ No newline at end of file +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportDefault0.ts b/tests/cases/fourslash/importNameCodeFixNewImportDefault0.ts index 1f2cde9f571b0..acab10cd36b85 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportDefault0.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportDefault0.ts @@ -5,7 +5,8 @@ // @Filename: module.ts //// export default function f1() { }; -verify.codeFixAtPosition( +verify.codeFixAtPosition([ `import f1 from "./module"; + f1();` -); \ No newline at end of file +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportFile0.ts b/tests/cases/fourslash/importNameCodeFixNewImportFile0.ts index 3e8dcc1ae0818..a456d3a2b7486 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportFile0.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportFile0.ts @@ -6,7 +6,8 @@ //// export function f1() {} //// export var v1 = 5; -verify.codeFixAtPosition( +verify.codeFixAtPosition([ `import { f1 } from "./module"; + f1();` -); \ No newline at end of file +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportFile1.ts b/tests/cases/fourslash/importNameCodeFixNewImportFile1.ts index 20ec7ff793fd9..722c005478fbb 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportFile1.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportFile1.ts @@ -10,8 +10,9 @@ // @Filename: tripleSlashReference.ts //// var x = 5;/*dummy*/ -verify.codeFixAtPosition( +verify.codeFixAtPosition([ `/// import { f1 } from "./module"; + f1();` -); \ No newline at end of file +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportFile2.ts b/tests/cases/fourslash/importNameCodeFixNewImportFile2.ts index d41594b1355c7..c5f335394f677 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportFile2.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportFile2.ts @@ -6,7 +6,8 @@ //// export var v1 = 5; //// export function f1(); -verify.codeFixAtPosition( +verify.codeFixAtPosition([ `import { f1 } from "../../other_dir/module"; + f1();` -); \ No newline at end of file +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportNodeModules0.ts b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules0.ts index 7a99663657b6f..e604e796cb825 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportNodeModules0.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules0.ts @@ -12,7 +12,8 @@ // @Filename: ../node_modules/fake-module/package.json //// {} -verify.codeFixAtPosition( +verify.codeFixAtPosition([ `import { f1 } from "fake-module"; + f1();` -); +]); diff --git a/tests/cases/fourslash/importNameCodeFixNewImportNodeModules1.ts b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules1.ts index 88dead5339fd0..08a215a712a6d 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportNodeModules1.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules1.ts @@ -9,7 +9,8 @@ //// export var v1 = 5; //// export function f1(); -verify.codeFixAtPosition( +verify.codeFixAtPosition([ `import { f1 } from "fake-module/nested"; + f1();` -); \ No newline at end of file +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportNodeModules2.ts b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules2.ts index 5629d5ad188e5..4e76ee7a416e2 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportNodeModules2.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules2.ts @@ -18,7 +18,8 @@ // @Filename: ../node_modules/fake-module/package.json //// { "main":"./notindex.js", "typings":"./notindex.d.ts" } -verify.codeFixAtPosition( +verify.codeFixAtPosition([ `import { f1 } from "fake-module"; + f1();` -); \ No newline at end of file +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportNodeModules3.ts b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules3.ts index c3e019eae60a1..aed21283be0b4 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportNodeModules3.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules3.ts @@ -7,7 +7,8 @@ //// export var v1 = 5; //// export function f1(); -verify.codeFixAtPosition( +verify.codeFixAtPosition([ `import { f1 } from "random"; + f1();` -); \ No newline at end of file +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportPaths0.ts b/tests/cases/fourslash/importNameCodeFixNewImportPaths0.ts index 92c3236bd14c3..66d279222cebb 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportPaths0.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportPaths0.ts @@ -15,7 +15,8 @@ //// } //// } -verify.codeFixAtPosition( - `import { foo } from "a"; +verify.codeFixAtPosition([ +`import { foo } from "a"; + foo();` -); \ No newline at end of file +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportPaths1.ts b/tests/cases/fourslash/importNameCodeFixNewImportPaths1.ts index 5734765121727..eb525966b25c5 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportPaths1.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportPaths1.ts @@ -15,7 +15,8 @@ //// } //// } -verify.codeFixAtPosition( - `import { foo } from "b/f2"; +verify.codeFixAtPosition([ +`import { foo } from "b/f2"; + foo();` -); \ No newline at end of file +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportRootDirs0.ts b/tests/cases/fourslash/importNameCodeFixNewImportRootDirs0.ts index e5ce140168b03..9e6405fe0857b 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportRootDirs0.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportRootDirs0.ts @@ -16,7 +16,8 @@ //// } //// } -verify.codeFixAtPosition( - `import { foo } from "./f2"; +verify.codeFixAtPosition([ +`import { foo } from "./f2"; + foo();` -); \ No newline at end of file +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportTypeRoots0.ts b/tests/cases/fourslash/importNameCodeFixNewImportTypeRoots0.ts index 07e62fa49cb53..50f223a45313f 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportTypeRoots0.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportTypeRoots0.ts @@ -15,7 +15,8 @@ //// } //// } -verify.codeFixAtPosition( - `import { foo } from "random"; +verify.codeFixAtPosition([ +`import { foo } from "random"; + foo();` -); \ No newline at end of file +]); \ No newline at end of file diff --git a/tests/cases/fourslash/server/codefix.ts b/tests/cases/fourslash/server/codefix.ts index 7fbe2cb4fd7bd..0032c66c11138 100644 --- a/tests/cases/fourslash/server/codefix.ts +++ b/tests/cases/fourslash/server/codefix.ts @@ -1,4 +1,4 @@ -/// +/// ////class Base{ ////} @@ -7,4 +7,4 @@ //// } ////} -verify.codeFixAtPosition('super();'); +verify.codeFixAtPosition(['super();']); diff --git a/tests/cases/fourslash/superFix1.ts b/tests/cases/fourslash/superFix1.ts index 7fbe2cb4fd7bd..68bb66dfe0aef 100644 --- a/tests/cases/fourslash/superFix1.ts +++ b/tests/cases/fourslash/superFix1.ts @@ -7,4 +7,4 @@ //// } ////} -verify.codeFixAtPosition('super();'); +verify.codeFixAtPosition(['super();']); diff --git a/tests/cases/fourslash/superFix2.ts b/tests/cases/fourslash/superFix2.ts index 880b5d43167c4..d7b4cf8c5d2bc 100644 --- a/tests/cases/fourslash/superFix2.ts +++ b/tests/cases/fourslash/superFix2.ts @@ -10,4 +10,4 @@ //// } ////} -verify.codeFixAtPosition("super(); this.a = 12;"); \ No newline at end of file +verify.codeFixAtPosition(["super(); this.a = 12;"]); \ No newline at end of file From a282fc012fbd17f5000db269b5a1c100bf65cc10 Mon Sep 17 00:00:00 2001 From: zhengbli Date: Fri, 11 Nov 2016 14:13:02 -0800 Subject: [PATCH 10/14] not using the generic verify method for import fixes. --- src/harness/fourslash.ts | 36 ++++++++++++++++--- src/services/codefixes/importFixes.ts | 29 ++++++--------- tests/cases/fourslash/fourslash.ts | 3 +- .../importNameCodeFixExistingImport0.ts | 2 +- .../importNameCodeFixExistingImport1.ts | 2 +- .../importNameCodeFixExistingImport10.ts | 2 +- .../importNameCodeFixExistingImport11.ts | 2 +- .../importNameCodeFixExistingImport12.ts | 2 +- .../importNameCodeFixExistingImport2.ts | 2 +- .../importNameCodeFixExistingImport3.ts | 2 +- .../importNameCodeFixExistingImport4.ts | 2 +- .../importNameCodeFixExistingImport5.ts | 2 +- .../importNameCodeFixExistingImport6.ts | 2 +- .../importNameCodeFixExistingImport7.ts | 2 +- .../importNameCodeFixExistingImport8.ts | 2 +- .../importNameCodeFixExistingImport9.ts | 2 +- .../importNameCodeFixNewImportAmbient0.ts | 2 +- .../importNameCodeFixNewImportAmbient1.ts | 2 +- .../importNameCodeFixNewImportAmbient2.ts | 2 +- .../importNameCodeFixNewImportAmbient3.ts | 2 +- .../importNameCodeFixNewImportBaseUrl0.ts | 2 +- .../importNameCodeFixNewImportDefault0.ts | 2 +- .../importNameCodeFixNewImportFile0.ts | 2 +- .../importNameCodeFixNewImportFile1.ts | 2 +- .../importNameCodeFixNewImportFile2.ts | 2 +- .../importNameCodeFixNewImportNodeModules0.ts | 2 +- .../importNameCodeFixNewImportNodeModules1.ts | 2 +- .../importNameCodeFixNewImportNodeModules2.ts | 2 +- .../importNameCodeFixNewImportNodeModules3.ts | 2 +- .../importNameCodeFixNewImportPaths0.ts | 2 +- .../importNameCodeFixNewImportPaths1.ts | 2 +- .../importNameCodeFixNewImportRootDirs0.ts | 2 +- .../importNameCodeFixNewImportTypeRoots0.ts | 2 +- tests/cases/fourslash/server/codefix.ts | 2 +- tests/cases/fourslash/superFix1.ts | 2 +- tests/cases/fourslash/superFix2.ts | 2 +- 36 files changed, 77 insertions(+), 57 deletions(-) diff --git a/src/harness/fourslash.ts b/src/harness/fourslash.ts index 94e50c8927933..078f4dd9b302b 100644 --- a/src/harness/fourslash.ts +++ b/src/harness/fourslash.ts @@ -2018,7 +2018,31 @@ namespace FourSlash { return this.languageService.getCodeFixesAtPosition(fileName, diagnostic.start, diagnostic.length, [diagnostic.code]); } - public verifyCodeFixAtPosition(expectedTextArray: string[], errorCode?: number) { + public verifyCodeFixAtPosition(expectedText: string, errorCode?: number) { + const ranges = this.getRanges(); + if (ranges.length == 0) { + this.raiseError("At least one range should be specified in the testfile."); + } + + const actual = this.getCodeFixes(errorCode); + + if (!actual || actual.length == 0) { + this.raiseError("No codefixes returned."); + } + + if (actual.length > 1) { + this.raiseError("More than 1 codefix returned."); + } + + this.applyEdits(actual[0].changes[0].fileName, actual[0].changes[0].textChanges, /*isFormattingEdit*/ false); + const actualText = this.rangeText(ranges[0]); + + if (this.removeWhitespace(actualText) !== this.removeWhitespace(expectedText)) { + this.raiseError(`Actual text doesn't match expected text. Actual: '${actualText}' Expected: '${expectedText}'`); + } + } + + public verifyImportFixAtPosition(expectedTextArray: string[], errorCode?: number) { const ranges = this.getRanges(); if (ranges.length == 0) { this.raiseError("At least one range should be specified in the testfile."); @@ -2042,7 +2066,7 @@ namespace FourSlash { const sortedActualArray = actualTextArray.sort(); if (!ts.arrayIsEqualTo(sortedExpectedArray, sortedActualArray)) { this.raiseError( - `Actual text array doesn't match expected text array. \nActual: \n'${sortedActualArray.join('\n\n')}'\n---\nExpected: \n'${sortedExpectedArray.join('\n\n')}'`); + `Actual text array doesn't match expected text array. \nActual: \n"${sortedActualArray.join("\n\n")}"\n---\nExpected: \n'${sortedExpectedArray.join("\n\n")}'`); } } @@ -3299,8 +3323,12 @@ namespace FourSlashInterface { this.DocCommentTemplate(/*expectedText*/ undefined, /*expectedOffset*/ undefined, /*empty*/ true); } - public codeFixAtPosition(expectedTextArray: string[], errorCode?: number): void { - this.state.verifyCodeFixAtPosition(expectedTextArray, errorCode); + public codeFixAtPosition(expectedText: string, errorCode?: number): void { + this.state.verifyCodeFixAtPosition(expectedText, errorCode); + } + + public importFixAtPosition(expectedTextArray: string[], errorCode?: number): void { + this.state.verifyImportFixAtPosition(expectedTextArray, errorCode); } public navigationBar(json: any) { diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index ae45c86932858..b6aeabfcdab14 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -21,7 +21,7 @@ namespace ts.codefix { const allPotentialModules = checker.getAmbientModules(); for (const otherSourceFile of allSourceFiles) { - if (otherSourceFile !== sourceFile && otherSourceFile.symbol) { + if (otherSourceFile !== sourceFile && isExternalOrCommonJsModule(otherSourceFile) && otherSourceFile.symbol) { allPotentialModules.push(otherSourceFile.symbol); } } @@ -203,24 +203,15 @@ namespace ts.codefix { // original text: import { foo, bar } from "module" // change to: import { foo, bar, name } from "module" const insertPoint = importList.elements[importList.elements.length - 1].getEnd(); - // If the import list has one import per line, preserve that. Otherwise, insert on same line as last element - let oneImportPerLine: boolean; - if (importList.elements.length === 1) { - /** - * If there is only one symbol being imported, still check to see if it's set up for multi-line imports like this: - * import { - * foo - * } from "./module"; - */ - const startLine = getLineOfLocalPosition(sourceFile, importList.getStart()); - const endLine = getLineOfLocalPosition(sourceFile, importList.getEnd()); - oneImportPerLine = endLine - startLine >= 2; - } - else { - const startLine = getLineOfLocalPosition(sourceFile, importList.elements[0].getStart()); - const endLine = getLineOfLocalPosition(sourceFile, insertPoint); - oneImportPerLine = endLine - startLine >= importList.elements.length - 1; - } + /** + * If the import list has one import per line, preserve that. Otherwise, insert on same line as last element + * import { + * foo + * } from "./module"; + */ + const startLine = getLineOfLocalPosition(sourceFile, importList.getStart()); + const endLine = getLineOfLocalPosition(sourceFile, importList.getEnd()); + const oneImportPerLine = endLine - startLine > importList.elements.length; return { newText: `,${oneImportPerLine ? context.newLineCharacter : " "}${newImportText}`, diff --git a/tests/cases/fourslash/fourslash.ts b/tests/cases/fourslash/fourslash.ts index 446277c029832..00cb5a9b982a7 100644 --- a/tests/cases/fourslash/fourslash.ts +++ b/tests/cases/fourslash/fourslash.ts @@ -209,7 +209,8 @@ declare namespace FourSlashInterface { noMatchingBracePositionInCurrentFile(bracePosition: number): void; DocCommentTemplate(expectedText: string, expectedOffset: number, empty?: boolean): void; noDocCommentTemplate(): void; - codeFixAtPosition(expectedTextArray: string[], errorCode?: number): void; + codeFixAtPosition(expectedText: string, errorCode?: number): void; + importFixAtPosition(expectedTextArray: string[], errorCode?: number): void; navigationBar(json: any): void; navigationTree(json: any): void; diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport0.ts b/tests/cases/fourslash/importNameCodeFixExistingImport0.ts index f532af86d3d46..5e5be220688c7 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport0.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport0.ts @@ -7,4 +7,4 @@ //// export function f1() {} //// export var v1 = 5; -verify.codeFixAtPosition([`{ v1, f1 }`]); \ No newline at end of file +verify.importFixAtPosition([`{ v1, f1 }`]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport1.ts b/tests/cases/fourslash/importNameCodeFixExistingImport1.ts index 0f72631e2cc03..9571d0fcf57cc 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport1.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport1.ts @@ -8,4 +8,4 @@ //// export var v1 = 5; //// export default var d1 = 6; -verify.codeFixAtPosition([`{ v1, f1 }`]); \ No newline at end of file +verify.importFixAtPosition([`{ v1, f1 }`]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport10.ts b/tests/cases/fourslash/importNameCodeFixExistingImport10.ts index a1aa80eb7118b..25246e7012328 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport10.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport10.ts @@ -12,7 +12,7 @@ //// export var v2 = 5; //// export var v3 = 5; -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `{ v1, v2, diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport11.ts b/tests/cases/fourslash/importNameCodeFixExistingImport11.ts index b0119dd00ee10..304ffb896dfb5 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport11.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport11.ts @@ -12,7 +12,7 @@ //// export var v2 = 5; //// export var v3 = 5; -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `{ v1, v2, v3, f1 diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport12.ts b/tests/cases/fourslash/importNameCodeFixExistingImport12.ts index 04c66cb055bf6..e00dee504c5f2 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport12.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport12.ts @@ -9,4 +9,4 @@ //// export var v2 = 5; //// export var v3 = 5; -verify.codeFixAtPosition([`{ f1 }`]); \ No newline at end of file +verify.importFixAtPosition([`{ f1 }`]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport2.ts b/tests/cases/fourslash/importNameCodeFixExistingImport2.ts index eccd6c9601f90..6a92976f4ef68 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport2.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport2.ts @@ -7,7 +7,7 @@ //// export function f1() {} //// export var v1 = 5; -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `import * as ns from "./module"; import { f1 } from "./module"; f1();`, diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport3.ts b/tests/cases/fourslash/importNameCodeFixExistingImport3.ts index 3cd073c42159d..8d64a933473f7 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport3.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport3.ts @@ -8,7 +8,7 @@ //// export var v1 = 5; //// export default var d1 = 6; -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `import d, * as ns from "./module"; ns.f1();`, `import d, * as ns from "./module"; diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport4.ts b/tests/cases/fourslash/importNameCodeFixExistingImport4.ts index c7a169ae76106..d93cb73664e4c 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport4.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport4.ts @@ -8,7 +8,7 @@ //// export var v1 = 5; //// export default var d1 = 6; -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `import d, { f1 } from "./module"; f1();` ]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport5.ts b/tests/cases/fourslash/importNameCodeFixExistingImport5.ts index 60c12fad6981d..ed9297124d936 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport5.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport5.ts @@ -7,6 +7,6 @@ //// export function f1() {} //// export var v1 = 5; -verify.codeFixAtPosition([`import "./module"; +verify.importFixAtPosition([`import "./module"; import { f1 } from "./module"; f1();`]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport6.ts b/tests/cases/fourslash/importNameCodeFixExistingImport6.ts index 61b809c76ef35..7ae157a51ceb9 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport6.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport6.ts @@ -10,4 +10,4 @@ //// export var v1 = 5; //// export function f1(); -verify.codeFixAtPosition([`{ v1, f1 }`]); \ No newline at end of file +verify.importFixAtPosition([`{ v1, f1 }`]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport7.ts b/tests/cases/fourslash/importNameCodeFixExistingImport7.ts index d98743189cd6a..249929eabc7a0 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport7.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport7.ts @@ -7,4 +7,4 @@ //// export var v1 = 5; //// export function f1(); -verify.codeFixAtPosition([`{ v1, f1 }`]); \ No newline at end of file +verify.importFixAtPosition([`{ v1, f1 }`]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport8.ts b/tests/cases/fourslash/importNameCodeFixExistingImport8.ts index 0a59eadbb0014..da7beaa0a4769 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport8.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport8.ts @@ -9,4 +9,4 @@ //// export var v2 = 5; //// export var v3 = 5; -verify.codeFixAtPosition([`{v1, v2, v3, f1,}`]); \ No newline at end of file +verify.importFixAtPosition([`{v1, v2, v3, f1,}`]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport9.ts b/tests/cases/fourslash/importNameCodeFixExistingImport9.ts index 0e4210620cc1c..05d179274548f 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport9.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport9.ts @@ -9,7 +9,7 @@ //// export function f1() {} //// export var v1 = 5; -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `{ v1, f1 diff --git a/tests/cases/fourslash/importNameCodeFixNewImportAmbient0.ts b/tests/cases/fourslash/importNameCodeFixNewImportAmbient0.ts index ecbcc2889635d..1d7b5bc3e7f33 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportAmbient0.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportAmbient0.ts @@ -8,7 +8,7 @@ //// export var v1; //// } -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `import { f1 } from "ambient-module"; f1();` diff --git a/tests/cases/fourslash/importNameCodeFixNewImportAmbient1.ts b/tests/cases/fourslash/importNameCodeFixNewImportAmbient1.ts index acba2ebfaa17d..60504c8971165 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportAmbient1.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportAmbient1.ts @@ -21,7 +21,7 @@ //// export var v3; //// } -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `import * as ns from "yet-another-ambient-module"; import { v1 } from "ambient-module"; var x = v1 + 5;` diff --git a/tests/cases/fourslash/importNameCodeFixNewImportAmbient2.ts b/tests/cases/fourslash/importNameCodeFixNewImportAmbient2.ts index f830c8b86543e..999da4bffbbde 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportAmbient2.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportAmbient2.ts @@ -11,7 +11,7 @@ //// export var v1; //// } -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `/* * I'm a license or something */ diff --git a/tests/cases/fourslash/importNameCodeFixNewImportAmbient3.ts b/tests/cases/fourslash/importNameCodeFixNewImportAmbient3.ts index 072b80f6eec28..0c4fc89508412 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportAmbient3.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportAmbient3.ts @@ -22,7 +22,7 @@ //// export var v3; //// } -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `import * as ns from "yet-another-ambient-module"; import { v1 } from "ambient-module"; var x = v1 + 5;` diff --git a/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl0.ts b/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl0.ts index 4fb316ab7fb6c..e15c2cf4399d8 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl0.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportBaseUrl0.ts @@ -12,7 +12,7 @@ // @Filename: a/b.ts //// export function f1() { }; -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `import { f1 } from "b"; f1();` diff --git a/tests/cases/fourslash/importNameCodeFixNewImportDefault0.ts b/tests/cases/fourslash/importNameCodeFixNewImportDefault0.ts index acab10cd36b85..3efe023e922b9 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportDefault0.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportDefault0.ts @@ -5,7 +5,7 @@ // @Filename: module.ts //// export default function f1() { }; -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `import f1 from "./module"; f1();` diff --git a/tests/cases/fourslash/importNameCodeFixNewImportFile0.ts b/tests/cases/fourslash/importNameCodeFixNewImportFile0.ts index a456d3a2b7486..2372110437a22 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportFile0.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportFile0.ts @@ -6,7 +6,7 @@ //// export function f1() {} //// export var v1 = 5; -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `import { f1 } from "./module"; f1();` diff --git a/tests/cases/fourslash/importNameCodeFixNewImportFile1.ts b/tests/cases/fourslash/importNameCodeFixNewImportFile1.ts index 722c005478fbb..0223d96e0186c 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportFile1.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportFile1.ts @@ -10,7 +10,7 @@ // @Filename: tripleSlashReference.ts //// var x = 5;/*dummy*/ -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `/// import { f1 } from "./module"; diff --git a/tests/cases/fourslash/importNameCodeFixNewImportFile2.ts b/tests/cases/fourslash/importNameCodeFixNewImportFile2.ts index c5f335394f677..ca9330e9846a5 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportFile2.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportFile2.ts @@ -6,7 +6,7 @@ //// export var v1 = 5; //// export function f1(); -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `import { f1 } from "../../other_dir/module"; f1();` diff --git a/tests/cases/fourslash/importNameCodeFixNewImportNodeModules0.ts b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules0.ts index e604e796cb825..6013f865ddf15 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportNodeModules0.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules0.ts @@ -12,7 +12,7 @@ // @Filename: ../node_modules/fake-module/package.json //// {} -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `import { f1 } from "fake-module"; f1();` diff --git a/tests/cases/fourslash/importNameCodeFixNewImportNodeModules1.ts b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules1.ts index 08a215a712a6d..6bffe41b27a04 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportNodeModules1.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules1.ts @@ -9,7 +9,7 @@ //// export var v1 = 5; //// export function f1(); -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `import { f1 } from "fake-module/nested"; f1();` diff --git a/tests/cases/fourslash/importNameCodeFixNewImportNodeModules2.ts b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules2.ts index 4e76ee7a416e2..ff48fbe182cca 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportNodeModules2.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules2.ts @@ -18,7 +18,7 @@ // @Filename: ../node_modules/fake-module/package.json //// { "main":"./notindex.js", "typings":"./notindex.d.ts" } -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `import { f1 } from "fake-module"; f1();` diff --git a/tests/cases/fourslash/importNameCodeFixNewImportNodeModules3.ts b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules3.ts index aed21283be0b4..b1143cb4b41c9 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportNodeModules3.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportNodeModules3.ts @@ -7,7 +7,7 @@ //// export var v1 = 5; //// export function f1(); -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `import { f1 } from "random"; f1();` diff --git a/tests/cases/fourslash/importNameCodeFixNewImportPaths0.ts b/tests/cases/fourslash/importNameCodeFixNewImportPaths0.ts index 66d279222cebb..93cd6f92ef595 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportPaths0.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportPaths0.ts @@ -15,7 +15,7 @@ //// } //// } -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `import { foo } from "a"; foo();` diff --git a/tests/cases/fourslash/importNameCodeFixNewImportPaths1.ts b/tests/cases/fourslash/importNameCodeFixNewImportPaths1.ts index eb525966b25c5..bb0f1e6705a09 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportPaths1.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportPaths1.ts @@ -15,7 +15,7 @@ //// } //// } -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `import { foo } from "b/f2"; foo();` diff --git a/tests/cases/fourslash/importNameCodeFixNewImportRootDirs0.ts b/tests/cases/fourslash/importNameCodeFixNewImportRootDirs0.ts index 9e6405fe0857b..ae8ef03ccaccc 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportRootDirs0.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportRootDirs0.ts @@ -16,7 +16,7 @@ //// } //// } -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `import { foo } from "./f2"; foo();` diff --git a/tests/cases/fourslash/importNameCodeFixNewImportTypeRoots0.ts b/tests/cases/fourslash/importNameCodeFixNewImportTypeRoots0.ts index 50f223a45313f..a6eb6a9075998 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportTypeRoots0.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportTypeRoots0.ts @@ -15,7 +15,7 @@ //// } //// } -verify.codeFixAtPosition([ +verify.importFixAtPosition([ `import { foo } from "random"; foo();` diff --git a/tests/cases/fourslash/server/codefix.ts b/tests/cases/fourslash/server/codefix.ts index 0032c66c11138..9d0f42c9e4c6d 100644 --- a/tests/cases/fourslash/server/codefix.ts +++ b/tests/cases/fourslash/server/codefix.ts @@ -7,4 +7,4 @@ //// } ////} -verify.codeFixAtPosition(['super();']); +verify.codeFixAtPosition('super();'); diff --git a/tests/cases/fourslash/superFix1.ts b/tests/cases/fourslash/superFix1.ts index 68bb66dfe0aef..7fbe2cb4fd7bd 100644 --- a/tests/cases/fourslash/superFix1.ts +++ b/tests/cases/fourslash/superFix1.ts @@ -7,4 +7,4 @@ //// } ////} -verify.codeFixAtPosition(['super();']); +verify.codeFixAtPosition('super();'); diff --git a/tests/cases/fourslash/superFix2.ts b/tests/cases/fourslash/superFix2.ts index d7b4cf8c5d2bc..880b5d43167c4 100644 --- a/tests/cases/fourslash/superFix2.ts +++ b/tests/cases/fourslash/superFix2.ts @@ -10,4 +10,4 @@ //// } ////} -verify.codeFixAtPosition(["super(); this.a = 12;"]); \ No newline at end of file +verify.codeFixAtPosition("super(); this.a = 12;"); \ No newline at end of file From d83c2928c7c9e273bb80f66f32df1d053e8d4dcd Mon Sep 17 00:00:00 2001 From: zhengbli Date: Mon, 14 Nov 2016 12:18:45 -0800 Subject: [PATCH 11/14] Correct insert position for new imports --- src/services/codefixes/importFixes.ts | 12 ++++++++++-- src/services/services.ts | 9 +++++++++ src/services/types.ts | 1 + .../importNameCodeFixExistingImport3.ts | 7 ++++--- .../importNameCodeFixExistingImportEquals0.ts | 18 ++++++++++++++++++ .../importNameCodeFixNewImportAmbient3.ts | 7 ++++--- 6 files changed, 46 insertions(+), 8 deletions(-) create mode 100644 tests/cases/fourslash/importNameCodeFixExistingImportEquals0.ts diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index f34e8c01850ba..d5703952753ef 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -142,7 +142,7 @@ namespace ts.codefix { // case: // import foo = require("foo") namespaceImportDeclaration = declaration; - existingModuleSpecifier = declaration.name.getText(); + existingModuleSpecifier = getModuleSpecifierFromImportEqualsDeclaration(declaration); } } @@ -174,6 +174,13 @@ namespace ts.codefix { } return actions; + function getModuleSpecifierFromImportEqualsDeclaration(declaration: ImportEqualsDeclaration) { + if (declaration.moduleReference && declaration.moduleReference.kind === SyntaxKind.ExternalModuleReference) { + return declaration.moduleReference.expression.getText(); + } + return declaration.moduleReference.getText(); + } + function getTextChangeForImportClause(importClause: ImportClause): TextChange { const newImportText = isDefault ? `default as ${name}` : name; const importList = importClause.namedBindings; @@ -227,6 +234,7 @@ namespace ts.codefix { else { namespacePrefix = declaration.name.getText(); } + namespacePrefix = stripQuotes(namespacePrefix); /** * Cases: @@ -258,7 +266,7 @@ namespace ts.codefix { lastModuleSpecifierEnd = end; } } - const insertPos = lastModuleSpecifierEnd > 0 ? lastModuleSpecifierEnd + 1 : sourceFile.getStart(); + const insertPos = lastModuleSpecifierEnd > 0 ? sourceFile.getLineEndOfPosition(lastModuleSpecifierEnd) : sourceFile.getStart(); const getCanonicalFileName = createGetCanonicalFileName(useCaseSensitiveFileNames); const moduleSpecifierWithoutQuotes = stripQuotes(moduleSpecifier || getModuleSpecifierForNewImport()); diff --git a/src/services/services.ts b/src/services/services.ts index f4f5ae3d05215..0d4f1ccec2684 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -492,6 +492,15 @@ namespace ts { return ts.getPositionOfLineAndCharacter(this, line, character); } + public getLineEndOfPosition(pos: number): number { + const { line } = this.getLineAndCharacterOfPosition(pos); + const lineStarts = this.getLineStarts(); + if (line >= lineStarts.length) { + return this.getEnd(); + } + return lineStarts[line + 1] - 1; + } + public getNamedDeclarations(): Map { if (!this.namedDeclarations) { this.namedDeclarations = this.computeNamedDeclarations(); diff --git a/src/services/types.ts b/src/services/types.ts index 4e04df3fc7caf..6a0e6e886b583 100644 --- a/src/services/types.ts +++ b/src/services/types.ts @@ -53,6 +53,7 @@ namespace ts { /* @internal */ getNamedDeclarations(): Map; getLineAndCharacterOfPosition(pos: number): LineAndCharacter; + getLineEndOfPosition(pos: number): number; getLineStarts(): number[]; getPositionOfLineAndCharacter(line: number, character: number): number; update(newText: string, textChangeRange: TextChangeRange): SourceFile; diff --git a/tests/cases/fourslash/importNameCodeFixExistingImport3.ts b/tests/cases/fourslash/importNameCodeFixExistingImport3.ts index 8d64a933473f7..bc00e8d420a07 100644 --- a/tests/cases/fourslash/importNameCodeFixExistingImport3.ts +++ b/tests/cases/fourslash/importNameCodeFixExistingImport3.ts @@ -1,6 +1,6 @@ /// -//// [|import d, * as ns from "./module"; +//// [|import d, * as ns from "./module" ; //// f1/*0*/();|] // @Filename: module.ts @@ -8,10 +8,11 @@ //// export var v1 = 5; //// export default var d1 = 6; +// Test with some extra spaces before the semicolon verify.importFixAtPosition([ -`import d, * as ns from "./module"; +`import d, * as ns from "./module" ; ns.f1();`, -`import d, * as ns from "./module"; +`import d, * as ns from "./module" ; import { f1 } from "./module"; f1();`, ]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixExistingImportEquals0.ts b/tests/cases/fourslash/importNameCodeFixExistingImportEquals0.ts new file mode 100644 index 0000000000000..f431e6356d126 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixExistingImportEquals0.ts @@ -0,0 +1,18 @@ +/// + +//// [|import ns = require("ambient-module"); +//// var x = v1/*0*/ + 5;|] + +// @Filename: ambientModule.ts +//// declare module "ambient-module" { +//// export function f1(); +//// export var v1; +//// } + +verify.importFixAtPosition([ +`import ns = require("ambient-module"); +var x = ns.v1 + 5;`, +`import ns = require("ambient-module"); +import { v1 } from "ambient-module"; +var x = v1 + 5;`, +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixNewImportAmbient3.ts b/tests/cases/fourslash/importNameCodeFixNewImportAmbient3.ts index 0c4fc89508412..648293cce2ef0 100644 --- a/tests/cases/fourslash/importNameCodeFixNewImportAmbient3.ts +++ b/tests/cases/fourslash/importNameCodeFixNewImportAmbient3.ts @@ -1,8 +1,8 @@ /// //// let a = "I am a non-trivial statement that appears before imports"; -//// import d from "other-ambient-module"; -//// [|import * as ns from "yet-another-ambient-module"; +//// import d from "other-ambient-module" +//// [|import * as ns from "yet-another-ambient-module" //// var x = v1/*0*/ + 5;|] // @Filename: ambientModule.ts @@ -22,8 +22,9 @@ //// export var v3; //// } +// test cases when there are no semicolons at the line end verify.importFixAtPosition([ -`import * as ns from "yet-another-ambient-module"; +`import * as ns from "yet-another-ambient-module" import { v1 } from "ambient-module"; var x = v1 + 5;` ]); \ No newline at end of file From aa2891c9bb7a03851dd31b0160f9f49db6c11ff8 Mon Sep 17 00:00:00 2001 From: zhengbli Date: Wed, 16 Nov 2016 01:35:27 -0800 Subject: [PATCH 12/14] improve the code action filtering logic --- src/services/codefixes/importFixes.ts | 151 +++++++++++++++--- .../importNameCodeFixOptionalImport0.ts | 20 +++ .../importNameCodeFixOptionalImport1.ts | 20 +++ 3 files changed, 165 insertions(+), 26 deletions(-) create mode 100644 tests/cases/fourslash/importNameCodeFixOptionalImport0.ts create mode 100644 tests/cases/fourslash/importNameCodeFixOptionalImport1.ts diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index d5703952753ef..262b5feaca49e 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -7,6 +7,110 @@ namespace ts.codefix { moduleSpecifier?: string } + enum ModuleSpecifierComparison { + Better, + Equal, + Worse + } + + class ImportCodeActionMap { + private symbolIdToActionMap = createMap(); + + addAction(symbolId: number, newAction: ImportCodeAction) { + if (!newAction) { + return; + } + + if (!this.symbolIdToActionMap[symbolId]) { + this.symbolIdToActionMap[symbolId] = [newAction]; + return; + } + + if (newAction.kind === "CodeChange") { + this.symbolIdToActionMap[symbolId].push(newAction); + return; + } + + const updatedNewImports: ImportCodeAction[] = []; + for (const existingAction of this.symbolIdToActionMap[symbolId]) { + if (existingAction.kind === "CodeChange") { + // only import actions should compare + updatedNewImports.push(existingAction); + continue; + } + + switch (this.compareModuleSpecifiers(existingAction.moduleSpecifier, newAction.moduleSpecifier)) { + case ModuleSpecifierComparison.Better: + // the new one is not worth considering if it is a new improt. + // However if it is instead a insertion into existing import, the user might want to use + // the module specifier even it is worse by our standards. So keep it. + if (newAction.kind === "NewImport") { + return; + } + case ModuleSpecifierComparison.Equal: + // the current one is safe. But it is still possible that the new one is worse + // than another existing one. For example, you may have new imports from "./foo/bar" + // and "bar", when the new one is "bar/bar2" and the current one is "./foo/bar". The new + // one and the current one are not comparable (one relative path and one absolute path), + // but the new one is worse than the other one, so should not add to the list. + updatedNewImports.push(existingAction); + break; + case ModuleSpecifierComparison.Worse: + // the existing one is worse, remove from the list. + continue; + } + } + // if we reach here, it means the new one is better or equal to all of the existing ones. + updatedNewImports.push(newAction); + this.symbolIdToActionMap[symbolId] = updatedNewImports; + } + + addActions(symbolId: number, newActions: ImportCodeAction[]) { + for (const newAction of newActions) { + this.addAction(symbolId, newAction); + } + } + + getAllActions() { + let result: ImportCodeAction[] = []; + for (const symbolId in this.symbolIdToActionMap) { + result = concatenate(result, this.symbolIdToActionMap[symbolId]); + } + return result; + } + + private compareModuleSpecifiers(moduleSpecifier1: string, moduleSpecifier2: string): ModuleSpecifierComparison { + if (moduleSpecifier1 === moduleSpecifier2) { + return ModuleSpecifierComparison.Equal; + } + + // if moduleSpecifier1 (ms1) is a substring of ms2, then it is better + if (moduleSpecifier2.indexOf(moduleSpecifier1) === 0) { + return ModuleSpecifierComparison.Better; + } + + if (moduleSpecifier1.indexOf(moduleSpecifier2) === 0) { + return ModuleSpecifierComparison.Worse; + } + + // if both are relative paths, and ms1 has fewer levels, then it is better + if (isExternalModuleNameRelative(moduleSpecifier1) && isExternalModuleNameRelative(moduleSpecifier2)) { + const regex = new RegExp(directorySeparator, "g"); + const moduleSpecifier1LevelCount = (moduleSpecifier1.match(regex) || []).length; + const moduleSpecifier2LevelCount = (moduleSpecifier2.match(regex) || []).length; + + return moduleSpecifier1LevelCount < moduleSpecifier2LevelCount + ? ModuleSpecifierComparison.Better + : moduleSpecifier1LevelCount === moduleSpecifier2LevelCount + ? ModuleSpecifierComparison.Equal + : ModuleSpecifierComparison.Worse; + } + + // the equal cases include when the two specifiers are not comparable. + return ModuleSpecifierComparison.Equal; + } + } + registerCodeFix({ errorCodes: [Diagnostics.Cannot_find_name_0.code], getCodeActions: (context: CodeFixContext) => { @@ -17,7 +121,7 @@ namespace ts.codefix { const token = getTokenAtPosition(sourceFile, context.span.start); const name = token.getText(); - const allActions: ImportCodeAction[] = []; + const symbolIdActionMap = new ImportCodeActionMap(); const allPotentialModules = checker.getAmbientModules(); for (const otherSourceFile of allSourceFiles) { @@ -35,37 +139,29 @@ namespace ts.codefix { if (defaultExport) { const localSymbol = getLocalSymbolForExportDefault(defaultExport); if (localSymbol && localSymbol.name === name && checkSymbolHasMeaning(localSymbol, currentTokenMeaning)) { - addRange(allActions, getCodeActionForImport(moduleSymbol, /*isDefaultExport*/ true)); + // check if this symbol is already used + const symbolId = getUniqueSymbolId(localSymbol); + symbolIdActionMap.addActions(symbolId, getCodeActionForImport(moduleSymbol, /*isDefaultExport*/ true)); } } // check exports with the same name - const exportWithIdenticalName = checker.tryGetMemberInModuleExports(name, moduleSymbol); - if (exportWithIdenticalName && checkSymbolHasMeaning(exportWithIdenticalName, currentTokenMeaning)) { - addRange(allActions, getCodeActionForImport(moduleSymbol)); + const exportSymbolWithIdenticalName = checker.tryGetMemberInModuleExports(name, moduleSymbol); + if (exportSymbolWithIdenticalName && checkSymbolHasMeaning(exportSymbolWithIdenticalName, currentTokenMeaning)) { + const symbolId = getUniqueSymbolId(exportSymbolWithIdenticalName); + symbolIdActionMap.addActions(symbolId, getCodeActionForImport(moduleSymbol)); } } - //sort the code actions - const confirmedActions: ImportCodeAction[] = []; - for (const action of allActions) { - switch (action.kind) { - case "CodeChange": - case "InsertingIntoExistingImport": - confirmedActions.push(action); - break; - case "NewImport": - if (action.moduleSpecifier && - some(allActions, otherAction => action.moduleSpecifier !== otherAction.moduleSpecifier && action.moduleSpecifier.indexOf(otherAction.moduleSpecifier) === 0)) { - // there is another action with a shorter module specifier. Use that instead. - continue; - } - confirmedActions.push(action); + return symbolIdActionMap.getAllActions(); + + function getUniqueSymbolId(symbol: Symbol) { + if (symbol.flags & SymbolFlags.Alias) { + return getSymbolId(checker.getAliasedSymbol(symbol)); } + return getSymbolId(symbol); } - return confirmedActions; - function checkSymbolHasMeaning(symbol: Symbol, meaning: SemanticMeaning) { const declarations = symbol.getDeclarations(); return declarations ? some(symbol.declarations, decl => !!(getMeaningFromDeclaration(decl) & meaning)) : false; @@ -404,10 +500,13 @@ namespace ts.codefix { try { const moduleDirectory = getDirectoryPath(moduleFileName); const packageJsonContent = JSON.parse(context.host.readFile(combinePaths(moduleDirectory, "package.json"))); - if (packageJsonContent && packageJsonContent.main) { - const mainExportFile = toPath(packageJsonContent.main, moduleDirectory, getCanonicalFileName); - if (removeFileExtension(mainExportFile) === removeFileExtension(moduleFileName)) { - relativeFileName = getDirectoryPath(relativeFileName); + if (packageJsonContent) { + const mainFile = packageJsonContent.main || packageJsonContent.typings; + if (mainFile) { + const mainExportFile = toPath(mainFile, moduleDirectory, getCanonicalFileName); + if (removeFileExtension(mainExportFile) === removeFileExtension(moduleFileName)) { + relativeFileName = getDirectoryPath(relativeFileName); + } } } } diff --git a/tests/cases/fourslash/importNameCodeFixOptionalImport0.ts b/tests/cases/fourslash/importNameCodeFixOptionalImport0.ts new file mode 100644 index 0000000000000..30b482a94d792 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixOptionalImport0.ts @@ -0,0 +1,20 @@ +/// + +// @Filename: a/f1.ts +//// [|import * as ns from "./foo"; +//// foo/*0*/();|] + +// @Filename: a/foo/bar.ts +//// export function foo() {}; + +// @Filename: a/foo.ts +//// export { foo } from "./foo/bar"; + +verify.importFixAtPosition([ +`import * as ns from "./foo"; +import { foo } from "./foo"; +foo();`, + +`import * as ns from "./foo"; +ns.foo();`, +]); \ No newline at end of file diff --git a/tests/cases/fourslash/importNameCodeFixOptionalImport1.ts b/tests/cases/fourslash/importNameCodeFixOptionalImport1.ts new file mode 100644 index 0000000000000..343b0692260c6 --- /dev/null +++ b/tests/cases/fourslash/importNameCodeFixOptionalImport1.ts @@ -0,0 +1,20 @@ +/// + +// @Filename: a/f1.ts +//// [|foo/*0*/();|] + +// @Filename: a/node_modules/bar/index.ts +//// export function foo() {}; + +// @Filename: a/foo.ts +//// export { foo } from "bar"; + +verify.importFixAtPosition([ +`import { foo } from "./foo"; + +foo();`, + +`import { foo } from "bar"; + +foo();`, +]); \ No newline at end of file From 055d20f8ce11d8e18f1d98a0701a43e3f5db7394 Mon Sep 17 00:00:00 2001 From: zhengbli Date: Wed, 16 Nov 2016 11:53:13 -0800 Subject: [PATCH 13/14] Fix line ending issue --- src/services/services.ts | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/services/services.ts b/src/services/services.ts index 36d0741d435c9..ef50f4b5a2f3a 100644 --- a/src/services/services.ts +++ b/src/services/services.ts @@ -495,10 +495,18 @@ namespace ts { public getLineEndOfPosition(pos: number): number { const { line } = this.getLineAndCharacterOfPosition(pos); const lineStarts = this.getLineStarts(); - if (line >= lineStarts.length) { - return this.getEnd(); + + let lastCharPos: number; + if (line + 1 >= lineStarts.length) { + lastCharPos = this.getEnd(); + } + if (!lastCharPos) { + lastCharPos = lineStarts[line + 1] - 1; } - return lineStarts[line + 1] - 1; + + const fullText = this.getFullText(); + // if the new line is "\r\n", we should return the last non-new-line-character position + return fullText[lastCharPos] === "\n" && fullText[lastCharPos - 1] === "\r" ? lastCharPos - 1 : lastCharPos; } public getNamedDeclarations(): Map { From 11ea6b3ff1e174492b52b631ffc7b41d54c18572 Mon Sep 17 00:00:00 2001 From: zhengbli Date: Wed, 16 Nov 2016 14:03:07 -0800 Subject: [PATCH 14/14] cache where we can --- src/services/codefixes/importFixes.ts | 76 +++++++++++++++++---------- 1 file changed, 47 insertions(+), 29 deletions(-) diff --git a/src/services/codefixes/importFixes.ts b/src/services/codefixes/importFixes.ts index 262b5feaca49e..4adda19689db9 100644 --- a/src/services/codefixes/importFixes.ts +++ b/src/services/codefixes/importFixes.ts @@ -123,6 +123,10 @@ namespace ts.codefix { const name = token.getText(); const symbolIdActionMap = new ImportCodeActionMap(); + // this is a module id -> module import declaration map + const cachedImportDeclarations = createMap<(ImportDeclaration | ImportEqualsDeclaration)[]>(); + let cachedNewImportInsertPosition: number; + const allPotentialModules = checker.getAmbientModules(); for (const otherSourceFile of allSourceFiles) { if (otherSourceFile !== sourceFile && isExternalOrCommonJsModule(otherSourceFile)) { @@ -155,20 +159,13 @@ namespace ts.codefix { return symbolIdActionMap.getAllActions(); - function getUniqueSymbolId(symbol: Symbol) { - if (symbol.flags & SymbolFlags.Alias) { - return getSymbolId(checker.getAliasedSymbol(symbol)); - } - return getSymbolId(symbol); - } + function getImportDeclarations(moduleSymbol: Symbol) { + const moduleSymbolId = getUniqueSymbolId(moduleSymbol); - function checkSymbolHasMeaning(symbol: Symbol, meaning: SemanticMeaning) { - const declarations = symbol.getDeclarations(); - return declarations ? some(symbol.declarations, decl => !!(getMeaningFromDeclaration(decl) & meaning)) : false; - } + if (cachedImportDeclarations[moduleSymbolId]) { + return cachedImportDeclarations[moduleSymbolId]; + } - function getCodeActionForImport(moduleSymbol: Symbol, isDefault?: boolean): ImportCodeAction[] { - // Check to see if there are already imports being made from this source in the current file const existingDeclarations: (ImportDeclaration | ImportEqualsDeclaration)[] = []; for (const importModuleSpecifier of sourceFile.imports) { const importSymbol = checker.getSymbolAtLocation(importModuleSpecifier); @@ -176,14 +173,8 @@ namespace ts.codefix { existingDeclarations.push(getImportDeclaration(importModuleSpecifier)); } } - - if (existingDeclarations.length > 0) { - // With an existing import statement, there are more than one actions the user can do. - return getCodeActionsForExistingImport(existingDeclarations); - } - else { - return [getCodeActionForNewImport()]; - } + cachedImportDeclarations[moduleSymbolId] = existingDeclarations; + return existingDeclarations; function getImportDeclaration(moduleSpecifier: LiteralExpression) { let node: Node = moduleSpecifier; @@ -198,6 +189,31 @@ namespace ts.codefix { } return undefined; } + } + + function getUniqueSymbolId(symbol: Symbol) { + if (symbol.flags & SymbolFlags.Alias) { + return getSymbolId(checker.getAliasedSymbol(symbol)); + } + return getSymbolId(symbol); + } + + function checkSymbolHasMeaning(symbol: Symbol, meaning: SemanticMeaning) { + const declarations = symbol.getDeclarations(); + return declarations ? some(symbol.declarations, decl => !!(getMeaningFromDeclaration(decl) & meaning)) : false; + } + + function getCodeActionForImport(moduleSymbol: Symbol, isDefault?: boolean): ImportCodeAction[] { + const existingDeclarations = getImportDeclarations(moduleSymbol); + if (existingDeclarations.length > 0) { + // With an existing import statement, there are more than one actions the user can do. + return getCodeActionsForExistingImport(existingDeclarations); + } + else { + return [getCodeActionForNewImport()]; + } + + function getCodeActionsForExistingImport(declarations: (ImportDeclaration | ImportEqualsDeclaration)[]): ImportCodeAction[] { const actions: ImportCodeAction[] = []; @@ -354,15 +370,17 @@ namespace ts.codefix { } function getCodeActionForNewImport(moduleSpecifier?: string): ImportCodeAction { - // insert after any existing imports - let lastModuleSpecifierEnd = -1; - for (const moduleSpecifier of sourceFile.imports) { - const end = moduleSpecifier.getEnd(); - if (!lastModuleSpecifierEnd || end > lastModuleSpecifierEnd) { - lastModuleSpecifierEnd = end; + if (!cachedNewImportInsertPosition) { + // insert after any existing imports + let lastModuleSpecifierEnd = -1; + for (const moduleSpecifier of sourceFile.imports) { + const end = moduleSpecifier.getEnd(); + if (!lastModuleSpecifierEnd || end > lastModuleSpecifierEnd) { + lastModuleSpecifierEnd = end; + } } + cachedNewImportInsertPosition = lastModuleSpecifierEnd > 0 ? sourceFile.getLineEndOfPosition(lastModuleSpecifierEnd) : sourceFile.getStart(); } - const insertPos = lastModuleSpecifierEnd > 0 ? sourceFile.getLineEndOfPosition(lastModuleSpecifierEnd) : sourceFile.getStart(); const getCanonicalFileName = createGetCanonicalFileName(useCaseSensitiveFileNames); const moduleSpecifierWithoutQuotes = stripQuotes(moduleSpecifier || getModuleSpecifierForNewImport()); @@ -373,7 +391,7 @@ namespace ts.codefix { // if this file doesn't have any import statements, insert an import statement and then insert a new line // between the only import statement and user code. Otherwise just insert the statement because chances // are there are already a new line seperating code and import statements. - const newText = insertPos === sourceFile.getStart() + const newText = cachedNewImportInsertPosition === sourceFile.getStart() ? `${importStatementText};${context.newLineCharacter}${context.newLineCharacter}` : `${context.newLineCharacter}${importStatementText};`; @@ -381,7 +399,7 @@ namespace ts.codefix { Diagnostics.Import_0_from_1, [name, `"${moduleSpecifierWithoutQuotes}"`], newText, - { start: insertPos, length: 0 }, + { start: cachedNewImportInsertPosition, length: 0 }, sourceFile.fileName, "NewImport", moduleSpecifierWithoutQuotes