From 326244f742072b71b3813a9b9664682570fd5bff Mon Sep 17 00:00:00 2001 From: cherryblossom <31467609+cherryblossom000@users.noreply.github.com> Date: Wed, 13 Jan 2021 10:39:57 +1100 Subject: [PATCH] [Fix] `no-unused-modules`: make type imports mark a module as used (fixes #1924) --- CHANGELOG.md | 4 + src/ExportMap.js | 112 +++++++++--------- src/rules/no-cycle.js | 42 ++++--- src/rules/no-unused-modules.js | 34 +++--- .../typescript/file-ts-f-import-type.ts | 1 + .../no-unused-modules/typescript/file-ts-f.ts | 1 + .../typescript/file-ts-g-used-as-type.ts | 1 + .../no-unused-modules/typescript/file-ts-g.ts | 1 + tests/src/rules/no-unused-modules.js | 32 ++++- 9 files changed, 138 insertions(+), 90 deletions(-) create mode 100644 tests/files/no-unused-modules/typescript/file-ts-f-import-type.ts create mode 100644 tests/files/no-unused-modules/typescript/file-ts-f.ts create mode 100644 tests/files/no-unused-modules/typescript/file-ts-g-used-as-type.ts create mode 100644 tests/files/no-unused-modules/typescript/file-ts-g.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c5da1d455..1687ebee0d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel - [`no-webpack-loader-syntax`]/TypeScript: avoid crash on missing name ([#1947], thanks @leonardodino) - [`no-extraneous-dependencies`]: Add package.json cache ([#1948], thanks @fa93hws) - [`prefer-default-export`]: handle empty array destructuring ([#1965], thanks @ljharb) +- [`no-unused-modules`]: make type imports mark a module as used (fixes #1924) ([#1974], thanks [@cherryblossom000]) ## [2.22.1] - 2020-09-27 ### Fixed @@ -739,6 +740,8 @@ for info on changes for earlier releases. [`memo-parser`]: ./memo-parser/README.md +[#1974]: https://github.com/benmosher/eslint-plugin-import/pull/1974 +[#1924]: https://github.com/benmosher/eslint-plugin-import/issues/1924 [#1965]: https://github.com/benmosher/eslint-plugin-import/issues/1965 [#1948]: https://github.com/benmosher/eslint-plugin-import/pull/1948 [#1947]: https://github.com/benmosher/eslint-plugin-import/pull/1947 @@ -1289,3 +1292,4 @@ for info on changes for earlier releases. [@tomprats]: https://github.com/tomprats [@straub]: https://github.com/straub [@andreubotella]: https://github.com/andreubotella +[@cherryblossom000]: https://github.com/cherryblossom000 diff --git a/src/ExportMap.js b/src/ExportMap.js index 837546aeb4..3504d9dfb2 100644 --- a/src/ExportMap.js +++ b/src/ExportMap.js @@ -68,8 +68,8 @@ export default class ExportMap { // default exports must be explicitly re-exported (#328) if (name !== 'default') { - for (let dep of this.dependencies) { - let innerMap = dep() + for (const dep of this.dependencies) { + const innerMap = dep() // todo: report as unresolved? if (!innerMap) continue @@ -83,8 +83,8 @@ export default class ExportMap { /** * ensure that imported name fully resolves. - * @param {[type]} name [description] - * @return {Boolean} [description] + * @param {string} name + * @return {{ found: boolean, path: ExportMap[] }} */ hasDeep(name) { if (this.namespace.has(name)) return { found: true, path: [this] } @@ -110,8 +110,8 @@ export default class ExportMap { // default exports must be explicitly re-exported (#328) if (name !== 'default') { - for (let dep of this.dependencies) { - let innerMap = dep() + for (const dep of this.dependencies) { + const innerMap = dep() if (innerMap == null) return { found: true, path: [this] } // todo: report as unresolved? if (!innerMap) continue @@ -119,7 +119,7 @@ export default class ExportMap { // safeguard against cycles if (innerMap.path === this.path) continue - let innerValue = innerMap.hasDeep(name) + const innerValue = innerMap.hasDeep(name) if (innerValue.found) { innerValue.path.unshift(this) return innerValue @@ -148,15 +148,15 @@ export default class ExportMap { // default exports must be explicitly re-exported (#328) if (name !== 'default') { - for (let dep of this.dependencies) { - let innerMap = dep() + for (const dep of this.dependencies) { + const innerMap = dep() // todo: report as unresolved? if (!innerMap) continue // safeguard against cycles if (innerMap.path === this.path) continue - let innerValue = innerMap.get(name) + const innerValue = innerMap.get(name) if (innerValue !== undefined) return innerValue } } @@ -218,7 +218,7 @@ function captureDoc(source, docStyleParsers, ...nodes) { if (!leadingComments || leadingComments.length === 0) return false - for (let name in docStyleParsers) { + for (const name in docStyleParsers) { const doc = docStyleParsers[name](leadingComments) if (doc) { metadata.doc = doc @@ -241,8 +241,8 @@ const availableDocStyleParsers = { /** * parse JSDoc from leading comments - * @param {...[type]} comments [description] - * @return {{doc: object}} + * @param {object[]} comments + * @return {{ doc: object }} */ function captureJsDoc(comments) { let doc @@ -286,6 +286,8 @@ function captureTomDoc(comments) { } } +const supportedImportTypes = new Set(['ImportDefaultSpecifier', 'ImportNamespaceSpecifier']) + ExportMap.get = function (source, context) { const path = resolve(source, context) if (path == null) return null @@ -347,10 +349,11 @@ ExportMap.for = function (context) { ExportMap.parse = function (path, content, context) { - var m = new ExportMap(path) + const m = new ExportMap(path) + let ast try { - var ast = parse(path, content, context) + ast = parse(path, content, context) } catch (err) { log('parse error:', path, err) m.errors.push(err) @@ -409,43 +412,27 @@ ExportMap.parse = function (path, content, context) { return object } - function captureDependency(declaration) { - if (declaration.source == null) return null - if (declaration.importKind === 'type') return null // skip Flow type imports - const importedSpecifiers = new Set() - const supportedTypes = new Set(['ImportDefaultSpecifier', 'ImportNamespaceSpecifier']) - let hasImportedType = false - if (declaration.specifiers) { - declaration.specifiers.forEach(specifier => { - const isType = specifier.importKind === 'type' - hasImportedType = hasImportedType || isType - - if (supportedTypes.has(specifier.type) && !isType) { - importedSpecifiers.add(specifier.type) - } - if (specifier.type === 'ImportSpecifier' && !isType) { - importedSpecifiers.add(specifier.imported.name) - } - }) - } - - // only Flow types were imported - if (hasImportedType && importedSpecifiers.size === 0) return null + function captureDependency({ source }, isOnlyImportingTypes, importedSpecifiers = new Set()) { + if (source == null) return null - const p = remotePath(declaration.source.value) + const p = remotePath(source.value) if (p == null) return null + + const declarationMetadata = { + // capturing actual node reference holds full AST in memory! + source: { value: source.value, loc: source.loc }, + isOnlyImportingTypes, + importedSpecifiers, + } + const existing = m.imports.get(p) - if (existing != null) return existing.getter + if (existing != null) { + existing.declarations.add(declarationMetadata) + return existing.getter + } const getter = thunkFor(p, context) - m.imports.set(p, { - getter, - source: { // capturing actual node reference holds full AST in memory! - value: declaration.source.value, - loc: declaration.source.loc, - }, - importedSpecifiers, - }) + m.imports.set(p, {getter, declarations: new Set([declarationMetadata])}) return getter } @@ -471,7 +458,7 @@ ExportMap.parse = function (path, content, context) { } } - ast.body.forEach(function (n) { + ast.body.forEach((n) => { if (n.type === 'ExportDefaultDeclaration') { const exportMeta = captureDoc(source, docStyleParsers, n) if (n.declaration.type === 'Identifier') { @@ -482,14 +469,30 @@ ExportMap.parse = function (path, content, context) { } if (n.type === 'ExportAllDeclaration') { - const getter = captureDependency(n) + const getter = captureDependency(n, n.exportKind === 'type') if (getter) m.dependencies.add(getter) return } // capture namespaces in case of later export if (n.type === 'ImportDeclaration') { - captureDependency(n) + // import type { Foo } (TS and Flow) + const declarationIsType = n.importKind === 'type' + let isOnlyImportingTypes = declarationIsType + const importedSpecifiers = new Set() + n.specifiers.forEach(specifier => { + if (supportedImportTypes.has(specifier.type)) { + importedSpecifiers.add(specifier.type) + } + if (specifier.type === 'ImportSpecifier') { + importedSpecifiers.add(specifier.imported.name) + } + + // import { type Foo } (Flow) + if (!declarationIsType) isOnlyImportingTypes = specifier.importKind === 'type' + }) + captureDependency(n, isOnlyImportingTypes, importedSpecifiers) + let ns if (n.specifiers.some(s => s.type === 'ImportNamespaceSpecifier' && (ns = s))) { namespaces.set(ns.local.name, n.source.value) @@ -575,9 +578,12 @@ ExportMap.parse = function (path, content, context) { 'TSAbstractClassDeclaration', 'TSModuleDeclaration', ] - const exportedDecls = ast.body.filter(({ type, id, declarations }) => includes(declTypes, type) && ( - (id && id.name === exportedName) || (declarations && declarations.find((d) => d.id.name === exportedName)) - )) + const exportedDecls = ast.body.filter(({ type, id, declarations }) => + includes(declTypes, type) && ( + (id && id.name === exportedName) || + (declarations && declarations.find(d => d.id.name === exportedName)) + ) + ) if (exportedDecls.length === 0) { // Export is not referencing any local declaration, must be re-exporting m.namespace.set('default', captureDoc(source, docStyleParsers, n)) diff --git a/src/rules/no-cycle.js b/src/rules/no-cycle.js index 2ad381e91a..97489fc040 100644 --- a/src/rules/no-cycle.js +++ b/src/rules/no-cycle.js @@ -48,37 +48,49 @@ module.exports = { return // ignore external modules } - const imported = Exports.get(sourceNode.value, context) - - if (importer.importKind === 'type') { - return // no Flow import resolution + if ( + importer.type === 'ImportDeclaration' && ( + // import type { Foo } (TS and Flow) + importer.importKind === 'type' || + // import { type Foo } (Flow) + importer.specifiers.every(({ importKind }) => importKind === 'type') + ) + ) { + return // ignore type imports } + const imported = Exports.get(sourceNode.value, context) + if (imported == null) { - return // no-unresolved territory + return // no-unresolved territory } if (imported.path === myPath) { - return // no-self-import territory + return // no-self-import territory } - const untraversed = [{mget: () => imported, route:[]}] + const untraversed = [{ mget: () => imported, route: [] }] const traversed = new Set() - function detectCycle({mget, route}) { + function detectCycle({ mget, route }) { const m = mget() if (m == null) return if (traversed.has(m.path)) return traversed.add(m.path) - for (let [path, { getter, source }] of m.imports) { + for (const [path, { getter, declarations }] of m.imports) { if (path === myPath) return true if (traversed.has(path)) continue - if (ignoreModule(source.value)) continue - if (route.length + 1 < maxDepth) { - untraversed.push({ - mget: getter, - route: route.concat(source), - }) + for (const { source, isOnlyImportingTypes } of declarations) { + if (ignoreModule(source.value)) continue + // Ignore only type imports + if (isOnlyImportingTypes) continue + + if (route.length + 1 < maxDepth) { + untraversed.push({ + mget: getter, + route: route.concat(source), + }) + } } } } diff --git a/src/rules/no-unused-modules.js b/src/rules/no-unused-modules.js index 9f38ac8e1c..9d3d213b6c 100644 --- a/src/rules/no-unused-modules.js +++ b/src/rules/no-unused-modules.js @@ -114,7 +114,7 @@ const importList = new Map() * keys are the paths to the modules containing the exports, while the * lower-level Map keys are the specific identifiers or special AST node names * being exported. The leaf-level metadata object at the moment only contains a - * `whereUsed` propoerty, which contains a Set of paths to modules that import + * `whereUsed` property, which contains a Set of paths to modules that import * the name. * * For example, if we have a file named bar.js containing the following exports: @@ -130,7 +130,7 @@ const importList = new Map() * * Map { 'bar.js' => Map { 'o2' => { whereUsed: Set { 'foo.js' } } } } * - * @type {Map>} + * @type {Map }>>} */ const exportList = new Map() @@ -153,7 +153,7 @@ const resolveFiles = (src, ignoreExports, context) => { const srcFileList = listFilesToProcess(src, extensions) // prepare list of ignored files - const ignoredFilesList = listFilesToProcess(ignoreExports, extensions) + const ignoredFilesList = listFilesToProcess(ignoreExports, extensions) ignoredFilesList.forEach(({ filename }) => ignoredFiles.add(filename)) // prepare list of source files, don't consider files from node_modules @@ -173,7 +173,7 @@ const prepareImportsAndExports = (srcFiles, context) => { const imports = new Map() const currentExports = Exports.get(file, context) if (currentExports) { - const { dependencies, reexports, imports: localImportList, namespace } = currentExports + const { dependencies, reexports, imports: localImportList, namespace } = currentExports // dependencies === export * from const currentExportAll = new Set() @@ -193,7 +193,7 @@ const prepareImportsAndExports = (srcFiles, context) => { } else { exports.set(key, { whereUsed: new Set() }) } - const reexport = value.getImport() + const reexport = value.getImport() if (!reexport) { return } @@ -216,12 +216,10 @@ const prepareImportsAndExports = (srcFiles, context) => { if (isNodeModule(key)) { return } - let localImport = imports.get(key) - if (typeof localImport !== 'undefined') { - localImport = new Set([...localImport, ...value.importedSpecifiers]) - } else { - localImport = value.importedSpecifiers - } + const localImport = imports.get(key) || new Set() + value.declarations.forEach(({ importedSpecifiers }) => + importedSpecifiers.forEach(specifier => localImport.add(specifier)) + ) imports.set(key, localImport) }) importList.set(file, imports) @@ -329,8 +327,8 @@ const fileIsInPkg = file => { const checkPkgFieldString = pkgField => { if (join(basePath, pkgField) === file) { - return true - } + return true + } } const checkPkgFieldObject = pkgField => { @@ -507,7 +505,7 @@ module.exports = { } } - exports = exportList.get(file) + const exports = exportList.get(file) // special case: export * from const exportAll = exports.get(EXPORT_ALL_DECLARATION) @@ -580,7 +578,7 @@ module.exports = { } }) } - forEachDeclarationIdentifier(declaration, (name) => { + forEachDeclarationIdentifier(declaration, name => { newExportIdentifiers.add(name) }) } @@ -666,7 +664,7 @@ module.exports = { resolvedPath = resolve(astNode.source.raw.replace(/('|")/g, ''), context) astNode.specifiers.forEach(specifier => { const name = specifier.local.name - if (specifier.local.name === DEFAULT) { + if (name === DEFAULT) { newDefaultImports.add(resolvedPath) } else { newImports.set(name, resolvedPath) @@ -892,9 +890,9 @@ module.exports = { }, 'ExportNamedDeclaration': node => { node.specifiers.forEach(specifier => { - checkUsage(node, specifier.exported.name) + checkUsage(node, specifier.exported.name) }) - forEachDeclarationIdentifier(node.declaration, (name) => { + forEachDeclarationIdentifier(node.declaration, name => { checkUsage(node, name) }) }, diff --git a/tests/files/no-unused-modules/typescript/file-ts-f-import-type.ts b/tests/files/no-unused-modules/typescript/file-ts-f-import-type.ts new file mode 100644 index 0000000000..dd82043774 --- /dev/null +++ b/tests/files/no-unused-modules/typescript/file-ts-f-import-type.ts @@ -0,0 +1 @@ +import type {g} from './file-ts-g-used-as-type' diff --git a/tests/files/no-unused-modules/typescript/file-ts-f.ts b/tests/files/no-unused-modules/typescript/file-ts-f.ts new file mode 100644 index 0000000000..f3a1ca7ab4 --- /dev/null +++ b/tests/files/no-unused-modules/typescript/file-ts-f.ts @@ -0,0 +1 @@ +import {g} from './file-ts-g'; diff --git a/tests/files/no-unused-modules/typescript/file-ts-g-used-as-type.ts b/tests/files/no-unused-modules/typescript/file-ts-g-used-as-type.ts new file mode 100644 index 0000000000..fe5318fbe7 --- /dev/null +++ b/tests/files/no-unused-modules/typescript/file-ts-g-used-as-type.ts @@ -0,0 +1 @@ +export interface g {} diff --git a/tests/files/no-unused-modules/typescript/file-ts-g.ts b/tests/files/no-unused-modules/typescript/file-ts-g.ts new file mode 100644 index 0000000000..fe5318fbe7 --- /dev/null +++ b/tests/files/no-unused-modules/typescript/file-ts-g.ts @@ -0,0 +1 @@ +export interface g {} diff --git a/tests/src/rules/no-unused-modules.js b/tests/src/rules/no-unused-modules.js index 1efa88d55a..57ac385ec4 100644 --- a/tests/src/rules/no-unused-modules.js +++ b/tests/src/rules/no-unused-modules.js @@ -100,7 +100,7 @@ ruleTester.run('no-unused-modules', rule, { }) -// tests for exports +// tests for exports ruleTester.run('no-unused-modules', rule, { valid: [ @@ -196,7 +196,7 @@ ruleTester.run('no-unused-modules', rule, { ], }) -// // test for export from +// test for export from ruleTester.run('no-unused-modules', rule, { valid: [ test({ options: unusedExportsOptions, @@ -744,7 +744,7 @@ describe('Avoid errors if re-export all from umd compiled library', () => { }) }) -context('TypeScript', function () { +context('TypeScript', () => { getTSParsers().forEach((parser) => { typescriptRuleTester.run('no-unused-modules', rule, { valid: [ @@ -827,6 +827,31 @@ context('TypeScript', function () { parser: parser, filename: testFilePath('./no-unused-modules/typescript/file-ts-e-used-as-type.ts'), }), + // Should also be valid when the exporting files are linted before the importing ones + test({ + options: unusedExportsTypescriptOptions, + code: `export interface g {}`, + parser, + filename: testFilePath('./no-unused-modules/typescript/file-ts-g.ts'), + }), + test({ + options: unusedExportsTypescriptOptions, + code: `import {g} from './file-ts-g';`, + parser, + filename: testFilePath('./no-unused-modules/typescript/file-ts-f.ts'), + }), + test({ + options: unusedExportsTypescriptOptions, + code: `export interface g {}`, + parser, + filename: testFilePath('./no-unused-modules/typescript/file-ts-g-used-as-type.ts'), + }), + test({ + options: unusedExportsTypescriptOptions, + code: `import type {g} from './file-ts-g';`, + parser, + filename: testFilePath('./no-unused-modules/typescript/file-ts-f-import-type.ts'), + }), ], invalid: [ test({ @@ -940,4 +965,3 @@ describe('ignore flow types', () => { invalid: [], }) }) -