no-cycle: real rule! first draft, perf is likely atrocious

src/ExportMap.js

@@ -390,7 +390,7 @@ ExportMap.parse = function (path, content, context) {
     if (m.imports.has(p)) return
 
     const getter = () => ExportMap.for(p, context)
-    m.imports.set(p, getter)
+    m.imports.set(p, { getter, source: declaration.source })
     return getter
   }
 
@@ -423,7 +423,6 @@ ExportMap.parse = function (path, content, context) {
     }
 
     if (n.type === 'ExportNamedDeclaration') {
-      captureDependency(n)
       // capture declaration
       if (n.declaration != null) {
         switch (n.declaration.type) {

src/rules/no-cycle.js

@@ -10,48 +10,51 @@ import docsUrl from '../docsUrl'
 // todo: cache cycles / deep relationships for faster repeat evaluation
 module.exports = {
   meta: {
-    docs: {
-      url: docsUrl('no-cycle'),
-    },
-
+    docs: { url: docsUrl('no-cycle') },
     schema: [optionsSchema],
   },
 
   create: function (context) {
-
     const myPath = context.getFilename()
+    if (myPath === '<text>') return  // can't cycle-check a non-file
 
-    function checkSourceValue(source) {
-      const imported = Exports.get(source.value, context)
+    function checkSourceValue(sourceNode, importer) {
+      const imported = Exports.get(sourceNode.value, context)
 
-      if (imported === undefined) {
-        return // no-unresolved territory
+      if (imported == null) {
+        return  // no-unresolved territory
       }
 
       if (imported.path === myPath) {
-        // todo: report direct self import?
-        return
+        return  // no-self-import territory
       }
 
-      const untraversed = [imported]
+      const untraversed = [{imported, route:[]}]
       const traversed = new Set()
-      function detectCycle(m) {
+      function detectCycle({imported: m, route}) {
         if (traversed.has(m.path)) return
         traversed.add(m.path)
 
-        for (let [path, getter] of m.imports) {
-          if (path === context) return true
+        for (let [path, { getter, source }] of m.imports) {
+          if (path === myPath) return true
           if (traversed.has(path)) continue
-          untraversed.push(getter())
+          const deeper = getter()
+          if (deeper != null) {
+            untraversed.push({
+              imported: deeper,
+              route: route.concat(source),
+            })
+          }
         }
       }
 
       while (untraversed.length > 0) {
-        if (detectCycle(untraversed.pop())) {
-          // todo: report
-
-          // todo: cache
-
+        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
         }
       }
@@ -60,3 +63,7 @@ module.exports = {
     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/src/rules/no-cycle.js

@@ -0,0 +1,55 @@
+import { test as _test, testFilePath } from '../utils'
+
+import { RuleTester } from 'eslint'
+
+const ruleTester = new RuleTester()
+    , rule = require('rules/no-cycle')
+
+const error = message => ({ ruleId: 'no-cycle', message })
+
+const test = def => _test(Object.assign(def, {
+  filename: testFilePath("./cycles/depth-zero.js")
+}))
+
+// describe.only("no-cycle", () => {
+ruleTester.run('no-cycle', rule, {
+  valid: [
+    // this rule doesn't care if the cycle length is 0
+    test({ code: 'import foo from "./foo.js"'}),
+
+    test({ code: 'import _ from "lodash"' }),
+    test({ code: 'import foo from "@scope/foo"' }),
+    test({ code: 'var _ = require("lodash")' }),
+    test({ code: 'var find = require("lodash.find")' }),
+    test({ code: 'var foo = require("./foo")' }),
+    test({ code: 'var foo = require("../foo")' }),
+    test({ code: 'var foo = require("foo")' }),
+    test({ code: 'var foo = require("./")' }),
+    test({ code: 'var foo = require("@scope/foo")' }),
+    test({ code: 'var bar = require("./bar/index")' }),
+    test({ code: 'var bar = require("./bar")' }),
+    test({
+      code: 'var bar = require("./bar")',
+      filename: '<text>',
+    }),
+  ],
+  invalid: [
+    test({
+      code: 'import { foo } from "./depth-one"',
+      errors: [error("Dependency cycle detected.")]
+    }),
+    test({
+      code: 'import { foo } from "./depth-two"',
+      errors: [error("Dependency cycle via ./depth-one:1")]
+    }),
+    test({
+      code: 'import { two } from "./depth-three-star"',
+      errors: [error("Dependency cycle via ./depth-two:1=>./depth-one:1")]
+    }),
+    test({
+      code: 'import { bar } from "./depth-three-indirect"',
+      errors: [error("Dependency cycle via ./depth-two:1=>./depth-one:1")]
+    }),
+  ],
+})
+// })

utils/moduleVisitor.js

@@ -19,19 +19,19 @@ exports.default = function visitModules(visitor, options) {
     ignoreRegExps = options.ignore.map(p => new RegExp(p))
   }
 
-  function checkSourceValue(source) {
+  function checkSourceValue(source, importer) {
     if (source == null) return //?
 
     // handle ignore
     if (ignoreRegExps.some(re => re.test(source.value))) return
 
     // fire visitor
-    visitor(source)
+    visitor(source, importer)
   }
 
   // for import-y declarations
   function checkSource(node) {
-    checkSourceValue(node.source)
+    checkSourceValue(node.source, node)
   }
 
   // for CommonJS `require` calls
@@ -45,7 +45,7 @@ exports.default = function visitModules(visitor, options) {
     if (modulePath.type !== 'Literal') return
     if (typeof modulePath.value !== 'string') return
 
-    checkSourceValue(modulePath)
+    checkSourceValue(modulePath, call)
   }
 
   function checkAMD(call) {
@@ -64,7 +64,7 @@ exports.default = function visitModules(visitor, options) {
       if (element.value === 'require' ||
           element.value === 'exports') continue // magic modules: http://git.io/vByan
 
-      checkSourceValue(element)
+      checkSourceValue(element, call)
     }
   }
 

utils/parse.js

@@ -23,6 +23,9 @@ exports.default = function parse(path, content, context) {
   parserOptions.comment = true
   parserOptions.attachComment = true
 
+  // attach node locations
+  parserOptions.loc = true
+
   // provide the `filePath` like eslint itself does, in `parserOptions`
   // https://github.com/eslint/eslint/blob/3ec436ee/lib/linter.js#L637
   parserOptions.filePath = path