diff --git a/src/ExportMap.js b/src/ExportMap.js index 1a35d6923..4325de5e0 100644 --- a/src/ExportMap.js +++ b/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) { diff --git a/src/rules/no-cycle.js b/src/rules/no-cycle.js index 394c94e2e..c9148b7fa 100644 --- a/src/rules/no-cycle.js +++ b/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 === '') 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('=>') +} diff --git a/tests/files/cycles/depth-one.js b/tests/files/cycles/depth-one.js new file mode 100644 index 000000000..748f65f84 --- /dev/null +++ b/tests/files/cycles/depth-one.js @@ -0,0 +1,2 @@ +import foo from "./depth-zero" +export { foo } diff --git a/tests/files/cycles/depth-three-indirect.js b/tests/files/cycles/depth-three-indirect.js new file mode 100644 index 000000000..814562f44 --- /dev/null +++ b/tests/files/cycles/depth-three-indirect.js @@ -0,0 +1,5 @@ +import './depth-two' + +export function bar() { + return "side effects???" +} \ No newline at end of file diff --git a/tests/files/cycles/depth-three-star.js b/tests/files/cycles/depth-three-star.js new file mode 100644 index 000000000..3f680bcb0 --- /dev/null +++ b/tests/files/cycles/depth-three-star.js @@ -0,0 +1,2 @@ +import * as two from "./depth-two" +export { two } diff --git a/tests/files/cycles/depth-two.js b/tests/files/cycles/depth-two.js new file mode 100644 index 000000000..3f38d78a5 --- /dev/null +++ b/tests/files/cycles/depth-two.js @@ -0,0 +1,2 @@ +import { foo } from "./depth-one" +export { foo } diff --git a/tests/files/cycles/depth-zero.js b/tests/files/cycles/depth-zero.js new file mode 100644 index 000000000..c9f23e9ca --- /dev/null +++ b/tests/files/cycles/depth-zero.js @@ -0,0 +1 @@ +// export function foo() {} diff --git a/tests/src/rules/no-cycle.js b/tests/src/rules/no-cycle.js new file mode 100644 index 000000000..b05a64100 --- /dev/null +++ b/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: '', + }), + ], + 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")] + }), + ], +}) +// }) diff --git a/utils/moduleVisitor.js b/utils/moduleVisitor.js index 4248317b6..6d087a163 100644 --- a/utils/moduleVisitor.js +++ b/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) } } diff --git a/utils/parse.js b/utils/parse.js index b921f8778..5bafdba49 100644 --- a/utils/parse.js +++ b/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