diff --git a/CHANGELOG.md b/CHANGELOG.md index a88cc01aa..0af90c8d8 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/docs/rules/no-cycle.md b/docs/rules/no-cycle.md new file mode 100644 index 000000000..aa28fda0a --- /dev/null +++ b/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 diff --git a/import.sublime-project b/import.sublime-project index d2ff5f73a..3b7feb346 100644 --- a/import.sublime-project +++ b/import.sublime-project @@ -16,8 +16,12 @@ }, "eslint_d": { + "disable": true, "chdir": "${project}" } + }, + "paths": { + "osx": ["${project}/node_modules/.bin"] } } } diff --git a/src/ExportMap.js b/src/ExportMap.js index aefec2ba3..11354a37e 100644 --- a/src/ExportMap.js +++ b/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,14 +406,14 @@ 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) @@ -401,7 +421,7 @@ ExportMap.parse = function (path, content, context) { return } - if (n.type === 'ExportNamedDeclaration'){ + if (n.type === 'ExportNamedDeclaration') { // capture declaration if (n.declaration != null) { switch (n.declaration.type) { diff --git a/src/index.js b/src/index.js index 6c5e11252..2d6352b83 100644 --- a/src/index.js +++ b/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'), diff --git a/src/rules/no-cycle.js b/src/rules/no-cycle.js new file mode 100644 index 000000000..b1bf7b3e9 --- /dev/null +++ b/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 === '') 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('=>') +} 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..f93ed00db --- /dev/null +++ b/tests/files/cycles/depth-three-indirect.js @@ -0,0 +1,5 @@ +import './depth-two' + +export function bar() { + return "side effects???" +} 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/files/export-all.js b/tests/files/export-all.js index 8839f6bb9..cfe060b7b 100644 --- a/tests/files/export-all.js +++ b/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' diff --git a/tests/src/rules/named.js b/tests/src/rules/named.js index 2364c74ed..8bd78f6eb 100644 --- a/tests/src/rules/named.js +++ b/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'`], + }), + ], +}) diff --git a/tests/src/rules/no-cycle.js b/tests/src/rules/no-cycle.js new file mode 100644 index 000000000..ae45ba36e --- /dev/null +++ b/tests/src/rules/no-cycle.js @@ -0,0 +1,89 @@ +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: '', + }), + test({ + code: 'import { foo } from "./depth-two"', + options: [{ maxDepth: 1 }], + }), + ], + invalid: [ + test({ + code: 'import { foo } from "./depth-one"', + errors: [error(`Dependency cycle detected.`)], + }), + test({ + code: 'import { foo } from "./depth-one"', + options: [{ maxDepth: 1 }], + errors: [error(`Dependency cycle detected.`)], + }), + test({ + code: 'const { foo } = require("./depth-one")', + errors: [error(`Dependency cycle detected.`)], + options: [{ commonjs: true }], + }), + test({ + code: 'require(["./depth-one"], d1 => {})', + errors: [error(`Dependency cycle detected.`)], + options: [{ amd: true }], + }), + test({ + code: 'define(["./depth-one"], d1 => {})', + errors: [error(`Dependency cycle detected.`)], + options: [{ amd: true }], + }), + test({ + code: 'import { foo } from "./depth-two"', + errors: [error(`Dependency cycle via ./depth-one:1`)], + }), + test({ + code: 'import { foo } from "./depth-two"', + options: [{ maxDepth: 2 }], + errors: [error(`Dependency cycle via ./depth-one:1`)], + }), + test({ + code: 'const { foo } = require("./depth-two")', + errors: [error(`Dependency cycle via ./depth-one:1`)], + options: [{ commonjs: true }], + }), + 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/CHANGELOG.md b/utils/CHANGELOG.md index cc9bc051b..5aaa3cc76 100644 --- a/utils/CHANGELOG.md +++ b/utils/CHANGELOG.md @@ -4,7 +4,11 @@ This project adheres to [Semantic Versioning](http://semver.org/). This change log adheres to standards from [Keep a CHANGELOG](http://keepachangelog.com). ## Unreleased - +### Changed +- `parse`: attach node locations by default. +- `moduleVisitor`: visitor now gets the full `import` statement node as a second + argument, so rules may report against the full statement / `require` call instead + of only the string literal node. ## v2.1.1 - 2017-06-22 diff --git a/utils/moduleVisitor.js b/utils/moduleVisitor.js index 4248317b6..7bb980e45 100644 --- a/utils/moduleVisitor.js +++ b/utils/moduleVisitor.js @@ -1,4 +1,4 @@ -"use strict" +'use strict' exports.__esModule = true /** @@ -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, element) } } @@ -90,9 +90,6 @@ exports.default = function visitModules(visitor, options) { /** * make an options schema for the module visitor, optionally * adding extra fields. - - * @param {[type]} additionalProperties [description] - * @return {[type]} [description] */ function makeOptionsSchema(additionalProperties) { const base = { 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