From 2c9b2d8928deb20b36443429324edc46fe599866 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Thu, 26 Oct 2017 09:04:09 -0700 Subject: [PATCH 1/2] no-duplicate-imports: Allow duplicate imports from separate ambient module declarations --- src/rules/noDuplicateImportsRule.ts | 39 +++++++++++++++---- .../no-duplicate-imports/test2.d.ts.lint | 8 ++++ .../no-duplicate-imports/test3.d.ts.lint | 11 ++++++ 3 files changed, 51 insertions(+), 7 deletions(-) create mode 100644 test/rules/no-duplicate-imports/test2.d.ts.lint create mode 100644 test/rules/no-duplicate-imports/test3.d.ts.lint diff --git a/src/rules/noDuplicateImportsRule.ts b/src/rules/noDuplicateImportsRule.ts index 25a0e3fba17..fb419ec06f9 100644 --- a/src/rules/noDuplicateImportsRule.ts +++ b/src/rules/noDuplicateImportsRule.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { findImports, ImportKind } from "tsutils"; +import { isLiteralExpression, isModuleBlock, isModuleDeclaration } from "tsutils"; import * as ts from "typescript"; import * as Lint from "../index"; @@ -44,13 +44,38 @@ export class Rule extends Lint.Rules.AbstractRule { } } -function walk(ctx: Lint.WalkContext) { - const seen = new Set(); - for (const {text, parent} of findImports(ctx.sourceFile, ImportKind.ImportDeclaration)) { - if (seen.has(text)) { - ctx.addFailureAtNode(parent!, Rule.FAILURE_STRING(text)); - } else { +function walk(ctx: Lint.WalkContext): void { + walkWorker(ctx, ctx.sourceFile.statements, new Set()); +} + +function walkWorker(ctx: Lint.WalkContext, statements: ReadonlyArray, seen: Set): void { + for (const statement of statements) { + const im = tryGetImportSpecifier(statement); + if (im !== undefined && isLiteralExpression(im)) { + const { text } = im; + if (seen.has(text)) { + ctx.addFailureAtNode(im.parent!, Rule.FAILURE_STRING(text)); + } seen.add(text); } + + if (isModuleDeclaration(statement) && statement.body !== undefined && isModuleBlock(statement.body)) { + // If this is a module augmentation, re-use `seen` since those imports could be moved outside. + // If this is an ambient module, create a fresh `seen` + // because they should have separate imports to avoid becoming augmentations. + walkWorker(ctx, statement.body.statements, ts.isExternalModule(ctx.sourceFile) ? seen : new Set()); + } + } +} + +function tryGetImportSpecifier(statement: ts.Statement): ts.Expression | undefined { + switch (statement.kind) { + case ts.SyntaxKind.ImportDeclaration: + return (statement as ts.ImportDeclaration).moduleSpecifier; + case ts.SyntaxKind.ImportEqualsDeclaration: + const ref = (statement as ts.ImportEqualsDeclaration).moduleReference; + return ref.kind === ts.SyntaxKind.ExternalModuleReference ? ref.expression : undefined; + default: + return undefined; } } diff --git a/test/rules/no-duplicate-imports/test2.d.ts.lint b/test/rules/no-duplicate-imports/test2.d.ts.lint new file mode 100644 index 00000000000..00ce946e7d1 --- /dev/null +++ b/test/rules/no-duplicate-imports/test2.d.ts.lint @@ -0,0 +1,8 @@ +declare module 'a' { + import foo from 'foo'; +} + +declare module 'b' { + // No error -- this is a separate ambient module declaration. + import foo from 'foo'; +} diff --git a/test/rules/no-duplicate-imports/test3.d.ts.lint b/test/rules/no-duplicate-imports/test3.d.ts.lint new file mode 100644 index 00000000000..bd64667a22f --- /dev/null +++ b/test/rules/no-duplicate-imports/test3.d.ts.lint @@ -0,0 +1,11 @@ +export {}; + +declare module 'a' { + import foo from 'foo'; +} + +declare module 'b' { + // Error because these imports could be combined in an outer scope. + import foo from 'foo'; + ~~~~~~~~~~~~~~~~~~~~~~ [Multiple imports from 'foo' can be combined into one.] +} From fc2cc42054dcd439ec47014c5cbdca0398b1c5b3 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Thu, 26 Oct 2017 13:01:33 -0700 Subject: [PATCH 2/2] Suggested fixes --- src/rules/noDuplicateImportsRule.ts | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/src/rules/noDuplicateImportsRule.ts b/src/rules/noDuplicateImportsRule.ts index fb419ec06f9..3677e4da8ef 100644 --- a/src/rules/noDuplicateImportsRule.ts +++ b/src/rules/noDuplicateImportsRule.ts @@ -15,7 +15,7 @@ * limitations under the License. */ -import { isLiteralExpression, isModuleBlock, isModuleDeclaration } from "tsutils"; +import { isImportDeclaration, isLiteralExpression, isModuleDeclaration } from "tsutils"; import * as ts from "typescript"; import * as Lint from "../index"; @@ -50,32 +50,19 @@ function walk(ctx: Lint.WalkContext): void { function walkWorker(ctx: Lint.WalkContext, statements: ReadonlyArray, seen: Set): void { for (const statement of statements) { - const im = tryGetImportSpecifier(statement); - if (im !== undefined && isLiteralExpression(im)) { - const { text } = im; + if (isImportDeclaration(statement) && isLiteralExpression(statement.moduleSpecifier)) { + const { text } = statement.moduleSpecifier; if (seen.has(text)) { - ctx.addFailureAtNode(im.parent!, Rule.FAILURE_STRING(text)); + ctx.addFailureAtNode(statement, Rule.FAILURE_STRING(text)); } seen.add(text); } - if (isModuleDeclaration(statement) && statement.body !== undefined && isModuleBlock(statement.body)) { + if (isModuleDeclaration(statement) && statement.body !== undefined && statement.name.kind === ts.SyntaxKind.StringLiteral) { // If this is a module augmentation, re-use `seen` since those imports could be moved outside. // If this is an ambient module, create a fresh `seen` // because they should have separate imports to avoid becoming augmentations. - walkWorker(ctx, statement.body.statements, ts.isExternalModule(ctx.sourceFile) ? seen : new Set()); + walkWorker(ctx, (statement.body as ts.ModuleBlock).statements, ts.isExternalModule(ctx.sourceFile) ? seen : new Set()); } } } - -function tryGetImportSpecifier(statement: ts.Statement): ts.Expression | undefined { - switch (statement.kind) { - case ts.SyntaxKind.ImportDeclaration: - return (statement as ts.ImportDeclaration).moduleSpecifier; - case ts.SyntaxKind.ImportEqualsDeclaration: - const ref = (statement as ts.ImportEqualsDeclaration).moduleReference; - return ref.kind === ts.SyntaxKind.ExternalModuleReference ? ref.expression : undefined; - default: - return undefined; - } -}