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

…ixes #1924)
import-js · Jan 16, 2021 · 326244f · 326244f
1 parent 929bade
commit 326244f
Show file tree

Hide file tree

Showing 9 changed files with 138 additions and 90 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
@@ -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,16 +110,16 @@ 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
 
         // 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
@@ -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),
+              })
+            }
           }
         }
       }