diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ca2345676..6e1fcd225b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel ### Added - [`group-exports`]: make aggregate module exports valid ([#1472], thanks [@atikenny]) +- [`no-namespace`]: Make rule fixable ([#1401], thanks [@TrevorBurnham]) ### Added - support `parseForESLint` from custom parser ([#1435], thanks [@JounQin]) @@ -617,6 +618,7 @@ for info on changes for earlier releases. [#1412]: https://github.com/benmosher/eslint-plugin-import/pull/1412 [#1409]: https://github.com/benmosher/eslint-plugin-import/pull/1409 [#1404]: https://github.com/benmosher/eslint-plugin-import/pull/1404 +[#1401]: https://github.com/benmosher/eslint-plugin-import/pull/1401 [#1393]: https://github.com/benmosher/eslint-plugin-import/pull/1393 [#1389]: https://github.com/benmosher/eslint-plugin-import/pull/1389 [#1377]: https://github.com/benmosher/eslint-plugin-import/pull/1377 @@ -989,3 +991,4 @@ for info on changes for earlier releases. [@JounQin]: https://github.com/JounQin [@atikenny]: https://github.com/atikenny [@schmidsi]: https://github.com/schmidsi +[@TrevorBurnham]: https://github.com/TrevorBurnham diff --git a/README.md b/README.md index aaa3f4c201..856baa9090 100644 --- a/README.md +++ b/README.md @@ -81,7 +81,7 @@ This plugin intends to support linting of ES2015+ (ES6+) import/export syntax, a * Ensure all imports appear before other statements ([`first`]) * Ensure all exports appear after other statements ([`exports-last`]) * Report repeated import of the same module in multiple places ([`no-duplicates`]) -* Report namespace imports ([`no-namespace`]) +* Forbid namespace (a.k.a. "wildcard" `*`) imports ([`no-namespace`]) * Ensure consistent use of file extension within the import path ([`extensions`]) * Enforce a convention in module import order ([`order`]) * Enforce a newline after import statements ([`newline-after-import`]) diff --git a/docs/rules/no-namespace.md b/docs/rules/no-namespace.md index b308d66210..e0b0f0b967 100644 --- a/docs/rules/no-namespace.md +++ b/docs/rules/no-namespace.md @@ -1,6 +1,9 @@ # import/no-namespace -Reports if namespace import is used. +Enforce a convention of not using namespace (a.k.a. "wildcard" `*`) imports. + ++(fixable) The `--fix` option on the [command line] automatically fixes problems reported by this rule, provided that the namespace object is only used for direct member access, e.g. `namespace.a`. +The `--fix` functionality for this rule requires ESLint 5 or newer. ## Rule Details @@ -12,10 +15,13 @@ import { a, b } from './bar' import defaultExport, { a, b } from './foobar' ``` -...whereas here imports will be reported: +Invalid: ```js import * as foo from 'foo'; +``` + +```js import defaultExport, * as foo from 'foo'; ``` diff --git a/src/rules/no-namespace.js b/src/rules/no-namespace.js index 3dbedca500..a3a6913646 100644 --- a/src/rules/no-namespace.js +++ b/src/rules/no-namespace.js @@ -16,13 +16,143 @@ module.exports = { docs: { url: docsUrl('no-namespace'), }, + fixable: 'code', }, create: function (context) { return { 'ImportNamespaceSpecifier': function (node) { - context.report(node, `Unexpected namespace import.`) + const scopeVariables = context.getScope().variables + const namespaceVariable = scopeVariables.find((variable) => + variable.defs[0].node === node + ) + const namespaceReferences = namespaceVariable.references + const namespaceIdentifiers = namespaceReferences.map(reference => reference.identifier) + const canFix = namespaceIdentifiers.length > 0 && !usesNamespaceAsObject(namespaceIdentifiers) + + context.report({ + node, + message: `Unexpected namespace import.`, + fix: canFix && (fixer => { + const scopeManager = context.getSourceCode().scopeManager + const fixes = [] + + // Pass 1: Collect variable names that are already in scope for each reference we want + // to transform, so that we can be sure that we choose non-conflicting import names + const importNameConflicts = {} + namespaceIdentifiers.forEach((identifier) => { + const parent = identifier.parent + if (parent && parent.type === 'MemberExpression') { + const importName = getMemberPropertyName(parent) + const localConflicts = getVariableNamesInScope(scopeManager, parent) + if (!importNameConflicts[importName]) { + importNameConflicts[importName] = localConflicts + } else { + localConflicts.forEach((c) => importNameConflicts[importName].add(c)) + } + } + }) + + // Choose new names for each import + const importNames = Object.keys(importNameConflicts) + const importLocalNames = generateLocalNames( + importNames, + importNameConflicts, + namespaceVariable.name + ) + + // Replace the ImportNamespaceSpecifier with a list of ImportSpecifiers + const namedImportSpecifiers = importNames.map((importName) => + importName === importLocalNames[importName] + ? importName + : `${importName} as ${importLocalNames[importName]}` + ) + fixes.push(fixer.replaceText(node, `{ ${namedImportSpecifiers.join(', ')} }`)) + + // Pass 2: Replace references to the namespace with references to the named imports + namespaceIdentifiers.forEach((identifier) => { + const parent = identifier.parent + if (parent && parent.type === 'MemberExpression') { + const importName = getMemberPropertyName(parent) + fixes.push(fixer.replaceText(parent, importLocalNames[importName])) + } + }) + + return fixes + }), + }) }, } }, } + +/** + * @param {Identifier[]} namespaceIdentifiers + * @returns {boolean} `true` if the namespace variable is more than just a glorified constant + */ +function usesNamespaceAsObject(namespaceIdentifiers) { + return !namespaceIdentifiers.every((identifier) => { + const parent = identifier.parent + + // `namespace.x` or `namespace['x']` + return ( + parent && parent.type === 'MemberExpression' && + (parent.property.type === 'Identifier' || parent.property.type === 'Literal') + ) + }) +} + +/** + * @param {MemberExpression} memberExpression + * @returns {string} the name of the member in the object expression, e.g. the `x` in `namespace.x` + */ +function getMemberPropertyName(memberExpression) { + return memberExpression.property.type === 'Identifier' + ? memberExpression.property.name + : memberExpression.property.value +} + +/** + * @param {ScopeManager} scopeManager + * @param {ASTNode} node + * @return {Set} + */ +function getVariableNamesInScope(scopeManager, node) { + let currentNode = node + let scope = scopeManager.acquire(currentNode) + while (scope == null) { + currentNode = currentNode.parent + scope = scopeManager.acquire(currentNode, true) + } + return new Set([ + ...scope.variables.map(variable => variable.name), + ...scope.upper.variables.map(variable => variable.name), + ]) +} + +/** + * + * @param {*} names + * @param {*} nameConflicts + * @param {*} namespaceName + */ +function generateLocalNames(names, nameConflicts, namespaceName) { + const localNames = {} + names.forEach((name) => { + let localName + if (!nameConflicts[name].has(name)) { + localName = name + } else if (!nameConflicts[name].has(`${namespaceName}_${name}`)) { + localName = `${namespaceName}_${name}` + } else { + for (let i = 1; i < Infinity; i++) { + if (!nameConflicts[name].has(`${namespaceName}_${name}_${i}`)) { + localName = `${namespaceName}_${name}_${i}` + break + } + } + } + localNames[name] = localName + }) + return localNames +} diff --git a/tests/src/rules/no-namespace.js b/tests/src/rules/no-namespace.js index d9ef3423c2..a7cb4dd21f 100644 --- a/tests/src/rules/no-namespace.js +++ b/tests/src/rules/no-namespace.js @@ -1,44 +1,113 @@ import { RuleTester } from 'eslint' +import eslintPkg from 'eslint/package.json' +import semver from 'semver' +import { test } from '../utils' const ERROR_MESSAGE = 'Unexpected namespace import.' const ruleTester = new RuleTester() +// --fix functionality requires ESLint 5+ +const FIX_TESTS = semver.satisfies(eslintPkg.version, '>5.0.0') ? [ + test({ + code: ` + import * as foo from './foo'; + florp(foo.bar); + florp(foo['baz']); + `.trim(), + output: ` + import { bar, baz } from './foo'; + florp(bar); + florp(baz); + `.trim(), + errors: [ { + line: 1, + column: 8, + message: ERROR_MESSAGE, + }], + }), + test({ + code: ` + import * as foo from './foo'; + const bar = 'name conflict'; + const baz = 'name conflict'; + const foo_baz = 'name conflict'; + florp(foo.bar); + florp(foo['baz']); + `.trim(), + output: ` + import { bar as foo_bar, baz as foo_baz_1 } from './foo'; + const bar = 'name conflict'; + const baz = 'name conflict'; + const foo_baz = 'name conflict'; + florp(foo_bar); + florp(foo_baz_1); + `.trim(), + errors: [ { + line: 1, + column: 8, + message: ERROR_MESSAGE, + }], + }), + test({ + code: ` + import * as foo from './foo'; + function func(arg) { + florp(foo.func); + florp(foo['arg']); + } + `.trim(), + output: ` + import { func as foo_func, arg as foo_arg } from './foo'; + function func(arg) { + florp(foo_func); + florp(foo_arg); + } + `.trim(), + errors: [ { + line: 1, + column: 8, + message: ERROR_MESSAGE, + }], + }), +] : [] + ruleTester.run('no-namespace', require('rules/no-namespace'), { valid: [ - { code: "import { a, b } from 'foo';", parserOptions: { ecmaVersion: 2015, sourceType: 'module' } }, - { code: "import { a, b } from './foo';", parserOptions: { ecmaVersion: 2015, sourceType: 'module' } }, - { code: "import bar from 'bar';", parserOptions: { ecmaVersion: 2015, sourceType: 'module' } }, - { code: "import bar from './bar';", parserOptions: { ecmaVersion: 2015, sourceType: 'module' } }, + { code: 'import { a, b } from \'foo\';', parserOptions: { ecmaVersion: 2015, sourceType: 'module' } }, + { code: 'import { a, b } from \'./foo\';', parserOptions: { ecmaVersion: 2015, sourceType: 'module' } }, + { code: 'import bar from \'bar\';', parserOptions: { ecmaVersion: 2015, sourceType: 'module' } }, + { code: 'import bar from \'./bar\';', parserOptions: { ecmaVersion: 2015, sourceType: 'module' } }, ], invalid: [ - { - code: "import * as foo from 'foo';", + test({ + code: 'import * as foo from \'foo\';', + output: 'import * as foo from \'foo\';', errors: [ { line: 1, column: 8, message: ERROR_MESSAGE, } ], - parserOptions: { ecmaVersion: 2015, sourceType: 'module' }, - }, - { - code: "import defaultExport, * as foo from 'foo';", + }), + test({ + code: 'import defaultExport, * as foo from \'foo\';', + output: 'import defaultExport, * as foo from \'foo\';', errors: [ { line: 1, column: 23, message: ERROR_MESSAGE, } ], - parserOptions: { ecmaVersion: 2015, sourceType: 'module' }, - }, - { - code: "import * as foo from './foo';", + }), + test({ + code: 'import * as foo from \'./foo\';', + output: 'import * as foo from \'./foo\';', errors: [ { line: 1, column: 8, message: ERROR_MESSAGE, } ], - parserOptions: { ecmaVersion: 2015, sourceType: 'module' }, - }, + }), + ...FIX_TESTS, ], })