Merge branch 'no-cycles'

Author: benmosher

CHANGELOG.md

@@ -4,7 +4,9 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 This change log adheres to standards from [Keep a CHANGELOG](http://keepachangelog.com).
 
 ## [Unreleased]
+### Added
 - Autofixer for [`order`] rule ([#711], thanks [@tihonove])
+- Add [`no-cycle`] rule: reports import cycles.
 
 ## [2.9.0] - 2018-02-21
 ### Added
@@ -443,6 +445,7 @@ for info on changes for earlier releases.
 [`no-self-import`]: ./docs/rules/no-self-import.md
 [`no-default-export`]: ./docs/rules/no-default-export.md
 [`no-useless-path-segments`]: ./docs/rules/no-useless-path-segments.md
+[`no-cycle`]: ./docs/rules/no-cycle.md
 
 [`memo-parser`]: ./memo-parser/README.md
 

docs/rules/no-cycle.md

@@ -0,0 +1,63 @@
+# import/no-cycle
+
+Ensures that there is no resolvable path back to this module via its dependencies.
+
+This includes cycles of depth 1 (imported module imports me) to `Infinity`, if the
+[`maxDepth`](#maxdepth) option is not set.
+
+```js
+// dep-b.js
+import './dep-a.js'
+
+export function b() { /* ... */ }
+
+// dep-a.js
+import { b } from './dep-b.js' // reported: Dependency cycle detected.
+```
+
+This rule does _not_ detect imports that resolve directly to the linted module;
+for that, see [`no-self-import`].
+
+
+## Rule Details
+
+### Options
+
+By default, this rule only detects cycles for ES6 imports, but see the [`no-unresolved` options](./no-unresolved.md#options) as this rule also supports the same `commonjs` and `amd` flags. However, these flags only impact which import types are _linted_; the
+import/export infrastructure only registers `import` statements in dependencies, so
+cycles created by `require` within imported modules may not be detected.
+
+#### `maxDepth`
+
+There is a `maxDepth` option available to prevent full expansion of very deep dependency trees:
+
+```js
+/*eslint import/no-unresolved: [2, { maxDepth: 1 }]*/
+
+// dep-c.js
+import './dep-a.js'
+
+// dep-b.js
+import './dep-c.js'
+
+export function b() { /* ... */ }
+
+// dep-a.js
+import { b } from './dep-b.js' // not reported as the cycle is at depth 2
+```
+
+This is not necessarily recommended, but available as a cost/benefit tradeoff mechanism
+for reducing total project lint time, if needed.
+
+## When Not To Use It
+
+This rule is comparatively computationally expensive. If you are pressed for lint
+time, or don't think you have an issue with dependency cycles, you may not want
+this rule enabled.
+
+## Further Reading
+
+- [Original inspiring issue](https://github.com/benmosher/eslint-plugin-import/issues/941)
+- Rule to detect that module imports itself: [`no-self-import`]
+
+[`no-self-import`]: ./no-self-import.md

import.sublime-project

@@ -16,8 +16,12 @@
             },
             "eslint_d":
             {
+                "disable": true,
                 "chdir": "${project}"
             }
+        },
+        "paths": {
+            "osx": ["${project}/node_modules/.bin"]
         }
     }
 }

src/ExportMap.js

@@ -21,7 +21,16 @@ export default class ExportMap {
     this.namespace = new Map()
     // todo: restructure to key on path, value is resolver + map of names
     this.reexports = new Map()
-    this.dependencies = new Map()
+    /**
+     * star-exports
+     * @type {Set} of () => ExportMap
+     */
+    this.dependencies = new Set()
+    /**
+     * dependencies of this module that are not explicitly re-exported
+     * @type {Map} from path = () => ExportMap
+     */
+    this.imports = new Map()
     this.errors = []
   }
 
@@ -46,7 +55,7 @@ export default class ExportMap {
 
     // default exports must be explicitly re-exported (#328)
     if (name !== 'default') {
-      for (let dep of this.dependencies.values()) {
+      for (let dep of this.dependencies) {
         let innerMap = dep()
 
         // todo: report as unresolved?
@@ -88,7 +97,7 @@ export default class ExportMap {
 
     // default exports must be explicitly re-exported (#328)
     if (name !== 'default') {
-      for (let dep of this.dependencies.values()) {
+      for (let dep of this.dependencies) {
         let innerMap = dep()
         // todo: report as unresolved?
         if (!innerMap) continue
@@ -125,7 +134,7 @@ export default class ExportMap {
 
     // default exports must be explicitly re-exported (#328)
     if (name !== 'default') {
-      for (let dep of this.dependencies.values()) {
+      for (let dep of this.dependencies) {
         let innerMap = dep()
         // todo: report as unresolved?
         if (!innerMap) continue
@@ -373,6 +382,17 @@ ExportMap.parse = function (path, content, context) {
     return object
   }
 
+  function captureDependency(declaration) {
+    if (declaration.source == null) return null
+
+    const p = remotePath(declaration)
+    if (p == null || m.imports.has(p)) return p
+
+    const getter = () => ExportMap.for(p, context)
+    m.imports.set(p, { getter, source: declaration.source })
+    return p
+  }
+
 
   ast.body.forEach(function (n) {
 
@@ -386,22 +406,22 @@ ExportMap.parse = function (path, content, context) {
     }
 
     if (n.type === 'ExportAllDeclaration') {
-      let remoteMap = remotePath(n)
-      if (remoteMap == null) return
-      m.dependencies.set(remoteMap, () => ExportMap.for(remoteMap, context))
+      const p = captureDependency(n)
+      if (p) m.dependencies.add(m.imports.get(p).getter)
       return
     }
 
     // capture namespaces in case of later export
     if (n.type === 'ImportDeclaration') {
+      captureDependency(n)
       let ns
       if (n.specifiers.some(s => s.type === 'ImportNamespaceSpecifier' && (ns = s))) {
         namespaces.set(ns.local.name, n)
       }
       return
     }
 
-    if (n.type === 'ExportNamedDeclaration'){
+    if (n.type === 'ExportNamedDeclaration') {
       // capture declaration
       if (n.declaration != null) {
         switch (n.declaration.type) {

src/index.js

@@ -12,6 +12,7 @@ export const rules = {
   'group-exports': require('./rules/group-exports'),
 
   'no-self-import': require('./rules/no-self-import'),
+  'no-cycle': require('./rules/no-cycle'),
   'no-named-default': require('./rules/no-named-default'),
   'no-named-as-default': require('./rules/no-named-as-default'),
   'no-named-as-default-member': require('./rules/no-named-as-default-member'),

src/rules/no-cycle.js

@@ -0,0 +1,79 @@
+/**
+ * @fileOverview Ensures that no imported module imports the linted module.
+ * @author Ben Mosher
+ */
+
+import Exports from '../ExportMap'
+import moduleVisitor, { makeOptionsSchema } from 'eslint-module-utils/moduleVisitor'
+import docsUrl from '../docsUrl'
+
+// todo: cache cycles / deep relationships for faster repeat evaluation
+module.exports = {
+  meta: {
+    docs: { url: docsUrl('no-cycle') },
+    schema: [makeOptionsSchema({
+      maxDepth:{
+        description: 'maximum dependency depth to traverse',
+        type: 'integer',
+        minimum: 1,
+      },
+    })],
+  },
+
+  create: function (context) {
+    const myPath = context.getFilename()
+    if (myPath === '<text>') return  // can't cycle-check a non-file
+
+    const options = context.options[0] || {}
+    const maxDepth = options.maxDepth || Infinity
+
+    function checkSourceValue(sourceNode, importer) {
+      const imported = Exports.get(sourceNode.value, context)
+
+      if (imported == null) {
+        return  // no-unresolved territory
+      }
+
+      if (imported.path === myPath) {
+        return  // no-self-import territory
+      }
+
+      const untraversed = [{mget: () => imported, route:[]}]
+      const traversed = new Set()
+      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) {
+          if (path === myPath) return true
+          if (traversed.has(path)) continue
+          if (route.length + 1 < maxDepth) {
+            untraversed.push({
+              mget: getter,
+              route: route.concat(source),
+            })
+          }
+        }
+      }
+
+      while (untraversed.length > 0) {
+        const next = untraversed.shift() // bfs!
+        if (detectCycle(next)) {
+          const message = (next.route.length > 0
+            ? `Dependency cycle via ${routeString(next.route)}`
+            : 'Dependency cycle detected.')
+          context.report(importer, message)
+          return
+        }
+      }
+    }
+
+    return moduleVisitor(checkSourceValue, context.options[0])
+  },
+}
+
+function routeString(route) {
+  return route.map(s => `${s.value}:${s.loc.start.line}`).join('=>')
+}

tests/files/cycles/depth-one.js

@@ -0,0 +1,2 @@
+import foo from "./depth-zero"
+export { foo }

tests/files/cycles/depth-three-indirect.js

@@ -0,0 +1,5 @@
+import './depth-two'
+
+export function bar() {
+    return "side effects???"
+}

tests/files/cycles/depth-three-star.js

@@ -0,0 +1,2 @@
+import * as two from "./depth-two"
+export { two }

tests/files/cycles/depth-two.js

@@ -0,0 +1,2 @@
+import { foo } from "./depth-one"
+export { foo }

tests/files/cycles/depth-zero.js

@@ -0,0 +1 @@
+// export function foo() {}

tests/files/export-all.js

@@ -1 +1,2 @@
+import { foo } from './sibling-with-names' // ensure importing exported name doesn't block
 export * from './sibling-with-names'

tests/src/rules/named.js

@@ -329,3 +329,18 @@ if (!CASE_SENSITIVE_FS) {
     ],
   })
 }
+
+// export-all
+ruleTester.run('named (export *)', rule, {
+  valid: [
+    test({
+      code: 'import { foo } from "./export-all"',
+    }),
+  ],
+  invalid: [
+    test({
+      code: 'import { bar } from "./export-all"',
+      errors: [`bar not found in './export-all'`],
+    }),
+  ],
+})