From 0f15bda45fa00f7b0b69305cb4161d2dc4379f1b Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Mon, 20 May 2019 13:31:44 -0700 Subject: [PATCH 1/3] Add failing test --- .../unittests/services/organizeImports.ts | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/testRunner/unittests/services/organizeImports.ts b/src/testRunner/unittests/services/organizeImports.ts index 8116b1e268a..9216a7adf88 100644 --- a/src/testRunner/unittests/services/organizeImports.ts +++ b/src/testRunner/unittests/services/organizeImports.ts @@ -343,6 +343,19 @@ import { } from "lib"; }, libFile); + testOrganizeImports("Unused_false_positive_module_augmentation", + { + path: "/test.d.ts", + content: ` +import { Caseless } from 'caseless'; + +declare module 'caseless' { + interface Caseless { + test(name: KeyType): boolean; + } +}` + }); + testOrganizeImports("Unused_false_positive_shorthand_assignment", { path: "/test.ts", From 6faeee449d0395529ff7cddc37cf00b988f3c6bf Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Mon, 20 May 2019 14:49:28 -0700 Subject: [PATCH 2/3] =?UTF-8?q?Don=E2=80=99t=20remove=20imports=20that=20a?= =?UTF-8?q?re=20used=20for=20module=20augmentation,=20just=20remove=20thei?= =?UTF-8?q?r=20import=20clauses?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/services/organizeImports.ts | 18 ++++++++++++++- .../unittests/services/organizeImports.ts | 4 +++- ...used_false_positive_module_augmentation.ts | 22 +++++++++++++++++++ 3 files changed, 42 insertions(+), 2 deletions(-) create mode 100644 tests/baselines/reference/organizeImports/Unused_false_positive_module_augmentation.ts diff --git a/src/services/organizeImports.ts b/src/services/organizeImports.ts index 5bfa0c74ab1..b6be92fbf9c 100644 --- a/src/services/organizeImports.ts +++ b/src/services/organizeImports.ts @@ -89,7 +89,7 @@ namespace ts.OrganizeImports { const usedImports: ImportDeclaration[] = []; for (const importDecl of oldImports) { - const {importClause} = importDecl; + const { importClause, moduleSpecifier } = importDecl; if (!importClause) { // Imports without import clauses are assumed to be included for their side effects and are not removed. @@ -125,6 +125,14 @@ namespace ts.OrganizeImports { if (name || namedBindings) { usedImports.push(updateImportDeclarationAndClause(importDecl, name, namedBindings)); } + // If a module is imported to be augmented, keep the import declaration, but without an import clause + else if (hasModuleDeclarationMatchingSpecifier(sourceFile, moduleSpecifier)) { + usedImports.push(createImportDeclaration( + importDecl.decorators, + importDecl.modifiers, + /*importClause*/ undefined, + moduleSpecifier)); + } } return usedImports; @@ -135,6 +143,14 @@ namespace ts.OrganizeImports { } } + function hasModuleDeclarationMatchingSpecifier(sourceFile: SourceFile, moduleSpecifier: Expression) { + const moduleSpecifierText = isStringLiteral(moduleSpecifier) && moduleSpecifier.text; + return isString(moduleSpecifierText) && some(sourceFile.statements, statement => + isModuleDeclaration(statement) + && isStringLiteral(statement.name) + && statement.name.text === moduleSpecifierText); + } + function getExternalModuleName(specifier: Expression) { return specifier !== undefined && isStringLiteralLike(specifier) ? specifier.text diff --git a/src/testRunner/unittests/services/organizeImports.ts b/src/testRunner/unittests/services/organizeImports.ts index 9216a7adf88..8003e1867cd 100644 --- a/src/testRunner/unittests/services/organizeImports.ts +++ b/src/testRunner/unittests/services/organizeImports.ts @@ -1,5 +1,5 @@ namespace ts { - describe("unittests:: services:: Organize imports", () => { + describe("unittests:: services:: organizeImports", () => { describe("Sort imports", () => { it("Sort - non-relative vs non-relative", () => { assertSortsBefore( @@ -347,8 +347,10 @@ import { } from "lib"; { path: "/test.d.ts", content: ` +import foo from 'foo'; import { Caseless } from 'caseless'; +declare module 'foo' {} declare module 'caseless' { interface Caseless { test(name: KeyType): boolean; diff --git a/tests/baselines/reference/organizeImports/Unused_false_positive_module_augmentation.ts b/tests/baselines/reference/organizeImports/Unused_false_positive_module_augmentation.ts new file mode 100644 index 00000000000..4eaf184d493 --- /dev/null +++ b/tests/baselines/reference/organizeImports/Unused_false_positive_module_augmentation.ts @@ -0,0 +1,22 @@ +// ==ORIGINAL== + +import foo from 'foo'; +import { Caseless } from 'caseless'; + +declare module 'foo' {} +declare module 'caseless' { + interface Caseless { + test(name: KeyType): boolean; + } +} +// ==ORGANIZED== + +import 'caseless'; +import 'foo'; + +declare module 'foo' {} +declare module 'caseless' { + interface Caseless { + test(name: KeyType): boolean; + } +} \ No newline at end of file From 60d0bb9b199e76974cdc65647663267e9fcf75b1 Mon Sep 17 00:00:00 2001 From: Andrew Branch Date: Wed, 22 May 2019 11:37:35 -0700 Subject: [PATCH 3/3] =?UTF-8?q?Don=E2=80=99t=20touch=20imports=20used=20fo?= =?UTF-8?q?r=20module=20augmentation=20in=20non-declaration=20files=20sinc?= =?UTF-8?q?e=20it=20could=20change=20JS=20emit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/services/organizeImports.ts | 28 ++++++++++++------- .../unittests/services/organizeImports.ts | 15 ++++++++++ ...le_augmentation_in_non_declaration_file.ts | 22 +++++++++++++++ 3 files changed, 55 insertions(+), 10 deletions(-) create mode 100644 tests/baselines/reference/organizeImports/Unused_preserve_imports_for_module_augmentation_in_non_declaration_file.ts diff --git a/src/services/organizeImports.ts b/src/services/organizeImports.ts index b6be92fbf9c..81c9108749f 100644 --- a/src/services/organizeImports.ts +++ b/src/services/organizeImports.ts @@ -125,13 +125,22 @@ namespace ts.OrganizeImports { if (name || namedBindings) { usedImports.push(updateImportDeclarationAndClause(importDecl, name, namedBindings)); } - // If a module is imported to be augmented, keep the import declaration, but without an import clause + // If a module is imported to be augmented, it’s used else if (hasModuleDeclarationMatchingSpecifier(sourceFile, moduleSpecifier)) { - usedImports.push(createImportDeclaration( - importDecl.decorators, - importDecl.modifiers, - /*importClause*/ undefined, - moduleSpecifier)); + // If we’re in a declaration file, it’s safe to remove the import clause from it + if (sourceFile.isDeclarationFile) { + usedImports.push(createImportDeclaration( + importDecl.decorators, + importDecl.modifiers, + /*importClause*/ undefined, + moduleSpecifier)); + } + // If we’re not in a declaration file, we can’t remove the import clause even though + // the imported symbols are unused, because removing them makes it look like the import + // declaration has side effects, which will cause it to be preserved in the JS emit. + else { + usedImports.push(importDecl); + } } } @@ -145,10 +154,9 @@ namespace ts.OrganizeImports { function hasModuleDeclarationMatchingSpecifier(sourceFile: SourceFile, moduleSpecifier: Expression) { const moduleSpecifierText = isStringLiteral(moduleSpecifier) && moduleSpecifier.text; - return isString(moduleSpecifierText) && some(sourceFile.statements, statement => - isModuleDeclaration(statement) - && isStringLiteral(statement.name) - && statement.name.text === moduleSpecifierText); + return isString(moduleSpecifierText) && some(sourceFile.moduleAugmentations, moduleName => + isStringLiteral(moduleName) + && moduleName.text === moduleSpecifierText); } function getExternalModuleName(specifier: Expression) { diff --git a/src/testRunner/unittests/services/organizeImports.ts b/src/testRunner/unittests/services/organizeImports.ts index 8003e1867cd..c119aadb34e 100644 --- a/src/testRunner/unittests/services/organizeImports.ts +++ b/src/testRunner/unittests/services/organizeImports.ts @@ -350,6 +350,21 @@ import { } from "lib"; import foo from 'foo'; import { Caseless } from 'caseless'; +declare module 'foo' {} +declare module 'caseless' { + interface Caseless { + test(name: KeyType): boolean; + } +}` + }); + + testOrganizeImports("Unused_preserve_imports_for_module_augmentation_in_non_declaration_file", + { + path: "/test.ts", + content: ` +import foo from 'foo'; +import { Caseless } from 'caseless'; + declare module 'foo' {} declare module 'caseless' { interface Caseless { diff --git a/tests/baselines/reference/organizeImports/Unused_preserve_imports_for_module_augmentation_in_non_declaration_file.ts b/tests/baselines/reference/organizeImports/Unused_preserve_imports_for_module_augmentation_in_non_declaration_file.ts new file mode 100644 index 00000000000..6d28f89903a --- /dev/null +++ b/tests/baselines/reference/organizeImports/Unused_preserve_imports_for_module_augmentation_in_non_declaration_file.ts @@ -0,0 +1,22 @@ +// ==ORIGINAL== + +import foo from 'foo'; +import { Caseless } from 'caseless'; + +declare module 'foo' {} +declare module 'caseless' { + interface Caseless { + test(name: KeyType): boolean; + } +} +// ==ORGANIZED== + +import { Caseless } from 'caseless'; +import foo from 'foo'; + +declare module 'foo' {} +declare module 'caseless' { + interface Caseless { + test(name: KeyType): boolean; + } +} \ No newline at end of file