[Fix] no-unused-modules: make type imports mark a module as used (f…

…ixes #1924)
import-js · Jan 12, 2021 · 462b016 · 462b016
1 parent 00b079e
commit 462b016
Show file tree

Hide file tree

Showing 9 changed files with 103 additions and 57 deletions.
diff --git 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
@@ -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] };
@@ -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;
@@ -410,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;
   }
 
@@ -483,14 +469,32 @@ 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);
+
       const ns = n.specifiers.find(s => s.type === 'ImportNamespaceSpecifier');
       if (ns) {
         namespaces.set(ns.local.name, n.source.value);

diff --git a/src/rules/no-cycle.js b/src/rules/no-cycle.js
@@ -48,12 +48,19 @@ 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
       }
@@ -70,15 +77,20 @@ module.exports = {
         if (traversed.has(m.path)) return;
         traversed.add(m.path);
 
-        for (const [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
@@ -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:
@@ -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);

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
@@ -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
@@ -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
@@ -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
@@ -0,0 +1 @@
+export interface g {}
diff --git a/tests/src/rules/no-unused-modules.js b/tests/src/rules/no-unused-modules.js
@@ -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: [],
   });
 });
-