diff --git a/CHANGELOG.md b/CHANGELOG.md index 9427a79ae6..ef04278d62 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +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 [`no-duplicates`] rule ([#1312], thanks [@lydell]) + ### Fixed - [`order`]: Fix interpreting some external modules being interpreted as internal modules ([#793], [#794] thanks [@ephys]) @@ -512,9 +515,11 @@ for info on changes for earlier releases. [`no-cycle`]: ./docs/rules/no-cycle.md [`dynamic-import-chunkname`]: ./docs/rules/dynamic-import-chunkname.md [`no-named-export`]: ./docs/rules/no-named-export.md +[`no-duplicates`]: ./docs/rules/no-duplicates.md [`memo-parser`]: ./memo-parser/README.md +[#1312]: https://github.com/benmosher/eslint-plugin-import/pull/1312 [#1257]: https://github.com/benmosher/eslint-plugin-import/pull/1257 [#1232]: https://github.com/benmosher/eslint-plugin-import/pull/1232 [#1176]: https://github.com/benmosher/eslint-plugin-import/pull/1176 @@ -809,3 +814,4 @@ for info on changes for earlier releases. [@asapach]: https://github.com/asapach [@sergei-startsev]: https://github.com/sergei-startsev [@ephys]: https://github.com/ephys +[@lydell]: https://github.com/lydell diff --git a/docs/rules/no-duplicates.md b/docs/rules/no-duplicates.md index 580f360119..0641e44186 100644 --- a/docs/rules/no-duplicates.md +++ b/docs/rules/no-duplicates.md @@ -1,6 +1,7 @@ # import/no-duplicates Reports if a resolved path is imported more than once. ++(fixable) The `--fix` option on the [command line] automatically fixes some problems reported by this rule. ESLint core has a similar rule ([`no-duplicate-imports`](http://eslint.org/docs/rules/no-duplicate-imports)), but this version is different in two key ways: diff --git a/src/rules/no-duplicates.js b/src/rules/no-duplicates.js index 4632ea0ec9..33e3357482 100644 --- a/src/rules/no-duplicates.js +++ b/src/rules/no-duplicates.js @@ -2,21 +2,234 @@ import resolve from 'eslint-module-utils/resolve' import docsUrl from '../docsUrl' function checkImports(imported, context) { - for (let [module, nodes] of imported.entries()) { - if (nodes.size > 1) { - for (let node of nodes) { - context.report(node, `'${module}' imported multiple times.`) + for (const [module, nodes] of imported.entries()) { + if (nodes.length > 1) { + const message = `'${module}' imported multiple times.` + const [first, ...rest] = nodes + const sourceCode = context.getSourceCode() + const fix = getFix(first, rest, sourceCode) + + context.report({ + node: first.source, + message, + fix, // Attach the autofix (if any) to the first import. + }) + + for (const node of rest) { + context.report({ + node: node.source, + message, + }) + } + } + } +} + +function getFix(first, rest, sourceCode) { + // Sorry ESLint <= 3 users, no autofix for you. Autofixing duplicate imports + // requires multiple `fixer.whatever()` calls in the `fix`: We both need to + // update the first one, and remove the rest. Support for multiple + // `fixer.whatever()` in a single `fix` was added in ESLint 4.1. + // `sourceCode.getCommentsBefore` was added in 4.0, so that's an easy thing to + // check for. + if (typeof sourceCode.getCommentsBefore !== 'function') { + return undefined + } + + // Adjusting the first import might make it multiline, which could break + // `eslint-disable-next-line` comments and similar, so bail if the first + // import has comments. Also, if the first import is `import * as ns from + // './foo'` there's nothing we can do. + if (hasProblematicComments(first, sourceCode) || hasNamespace(first)) { + return undefined + } + + const defaultImportNames = new Set( + [first, ...rest].map(getDefaultImportName).filter(Boolean) + ) + + // Bail if there are multiple different default import names – it's up to the + // user to choose which one to keep. + if (defaultImportNames.size > 1) { + return undefined + } + + // Leave it to the user to handle comments. Also skip `import * as ns from + // './foo'` imports, since they cannot be merged into another import. + const restWithoutComments = rest.filter(node => !( + hasProblematicComments(node, sourceCode) || + hasNamespace(node) + )) + + const specifiers = restWithoutComments + .map(node => { + const tokens = sourceCode.getTokens(node) + const openBrace = tokens.find(token => isPunctuator(token, '{')) + const closeBrace = tokens.find(token => isPunctuator(token, '}')) + + if (openBrace == null || closeBrace == null) { + return undefined + } + + return { + importNode: node, + text: sourceCode.text.slice(openBrace.range[1], closeBrace.range[0]), + hasTrailingComma: isPunctuator(sourceCode.getTokenBefore(closeBrace), ','), + isEmpty: !hasSpecifiers(node), + } + }) + .filter(Boolean) + + const unnecessaryImports = restWithoutComments.filter(node => + !hasSpecifiers(node) && + !hasNamespace(node) && + !specifiers.some(specifier => specifier.importNode === node) + ) + + const shouldAddDefault = getDefaultImportName(first) == null && defaultImportNames.size === 1 + const shouldAddSpecifiers = specifiers.length > 0 + const shouldRemoveUnnecessary = unnecessaryImports.length > 0 + + if (!(shouldAddDefault || shouldAddSpecifiers || shouldRemoveUnnecessary)) { + return undefined + } + + return fixer => { + const tokens = sourceCode.getTokens(first) + const openBrace = tokens.find(token => isPunctuator(token, '{')) + const closeBrace = tokens.find(token => isPunctuator(token, '}')) + const firstToken = sourceCode.getFirstToken(first) + const [defaultImportName] = defaultImportNames + + const firstHasTrailingComma = + closeBrace != null && + isPunctuator(sourceCode.getTokenBefore(closeBrace), ',') + const firstIsEmpty = !hasSpecifiers(first) + + const [specifiersText] = specifiers.reduce( + ([result, needsComma], specifier) => { + return [ + needsComma && !specifier.isEmpty + ? `${result},${specifier.text}` + : `${result}${specifier.text}`, + specifier.isEmpty ? needsComma : true, + ] + }, + ['', !firstHasTrailingComma && !firstIsEmpty] + ) + + const fixes = [] + + if (shouldAddDefault && openBrace == null && shouldAddSpecifiers) { + // `import './foo'` → `import def, {...} from './foo'` + fixes.push( + fixer.insertTextAfter(firstToken, ` ${defaultImportName}, {${specifiersText}} from`) + ) + } else if (shouldAddDefault && openBrace == null && !shouldAddSpecifiers) { + // `import './foo'` → `import def from './foo'` + fixes.push(fixer.insertTextAfter(firstToken, ` ${defaultImportName} from`)) + } else if (shouldAddDefault && openBrace != null && closeBrace != null) { + // `import {...} from './foo'` → `import def, {...} from './foo'` + fixes.push(fixer.insertTextAfter(firstToken, ` ${defaultImportName},`)) + if (shouldAddSpecifiers) { + // `import def, {...} from './foo'` → `import def, {..., ...} from './foo'` + fixes.push(fixer.insertTextBefore(closeBrace, specifiersText)) } + } else if (!shouldAddDefault && openBrace == null && shouldAddSpecifiers) { + // `import './foo'` → `import {...} from './foo'` + fixes.push(fixer.insertTextAfter(firstToken, ` {${specifiersText}} from`)) + } else if (!shouldAddDefault && openBrace != null && closeBrace != null) { + // `import {...} './foo'` → `import {..., ...} from './foo'` + fixes.push(fixer.insertTextBefore(closeBrace, specifiersText)) } + + // Remove imports whose specifiers have been moved into the first import. + for (const specifier of specifiers) { + fixes.push(fixer.remove(specifier.importNode)) + } + + // Remove imports whose default import has been moved to the first import, + // and side-effect-only imports that are unnecessary due to the first + // import. + for (const node of unnecessaryImports) { + fixes.push(fixer.remove(node)) + } + + return fixes } } +function isPunctuator(node, value) { + return node.type === 'Punctuator' && node.value === value +} + +// Get the name of the default import of `node`, if any. +function getDefaultImportName(node) { + const defaultSpecifier = node.specifiers + .find(specifier => specifier.type === 'ImportDefaultSpecifier') + return defaultSpecifier != null ? defaultSpecifier.local.name : undefined +} + +// Checks whether `node` has a namespace import. +function hasNamespace(node) { + const specifiers = node.specifiers + .filter(specifier => specifier.type === 'ImportNamespaceSpecifier') + return specifiers.length > 0 +} + +// Checks whether `node` has any non-default specifiers. +function hasSpecifiers(node) { + const specifiers = node.specifiers + .filter(specifier => specifier.type === 'ImportSpecifier') + return specifiers.length > 0 +} + +// It's not obvious what the user wants to do with comments associated with +// duplicate imports, so skip imports with comments when autofixing. +function hasProblematicComments(node, sourceCode) { + return ( + hasCommentBefore(node, sourceCode) || + hasCommentAfter(node, sourceCode) || + hasCommentInsideNonSpecifiers(node, sourceCode) + ) +} + +// Checks whether `node` has a comment (that ends) on the previous line or on +// the same line as `node` (starts). +function hasCommentBefore(node, sourceCode) { + return sourceCode.getCommentsBefore(node) + .some(comment => comment.loc.end.line >= node.loc.start.line - 1) +} + +// Checks whether `node` has a comment (that starts) on the same line as `node` +// (ends). +function hasCommentAfter(node, sourceCode) { + return sourceCode.getCommentsAfter(node) + .some(comment => comment.loc.start.line === node.loc.end.line) +} + +// Checks whether `node` has any comments _inside,_ except inside the `{...}` +// part (if any). +function hasCommentInsideNonSpecifiers(node, sourceCode) { + const tokens = sourceCode.getTokens(node) + const openBraceIndex = tokens.findIndex(token => isPunctuator(token, '{')) + const closeBraceIndex = tokens.findIndex(token => isPunctuator(token, '}')) + // Slice away the first token, since we're no looking for comments _before_ + // `node` (only inside). If there's a `{...}` part, look for comments before + // the `{`, but not before the `}` (hence the `+1`s). + const someTokens = openBraceIndex >= 0 && closeBraceIndex >= 0 + ? tokens.slice(1, openBraceIndex + 1).concat(tokens.slice(closeBraceIndex + 1)) + : tokens.slice(1) + return someTokens.some(token => sourceCode.getCommentsBefore(token).length > 0) +} + module.exports = { meta: { type: 'problem', docs: { url: docsUrl('no-duplicates'), }, + fixable: 'code', }, create: function (context) { @@ -29,9 +242,9 @@ module.exports = { const importMap = n.importKind === 'type' ? typesImported : imported if (importMap.has(resolvedPath)) { - importMap.get(resolvedPath).add(n.source) + importMap.get(resolvedPath).push(n) } else { - importMap.set(resolvedPath, new Set([n.source])) + importMap.set(resolvedPath, [n]) } }, diff --git a/tests/src/rules/no-duplicates.js b/tests/src/rules/no-duplicates.js index 82bccdee05..cdd365382c 100644 --- a/tests/src/rules/no-duplicates.js +++ b/tests/src/rules/no-duplicates.js @@ -1,11 +1,15 @@ import * as path from 'path' -import { test } from '../utils' +import { test as testUtil } from '../utils' import { RuleTester } from 'eslint' const ruleTester = new RuleTester() , rule = require('rules/no-duplicates') +const test = process.env.ESLINT_VERSION === '3' || process.env.ESLINT_VERSION === '2' + ? t => testUtil(Object.assign({}, t, {output: t.code})) + : testUtil + ruleTester.run('no-duplicates', rule, { valid: [ test({ code: 'import "./malformed.js"' }), @@ -25,17 +29,20 @@ ruleTester.run('no-duplicates', rule, { invalid: [ test({ code: "import { x } from './foo'; import { y } from './foo'", + output: "import { x , y } from './foo'; ", errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], }), test({ - code: "import { x } from './foo'; import { y } from './foo'; import { z } from './foo'", + code: "import {x} from './foo'; import {y} from './foo'; import { z } from './foo'", + output: "import {x,y, z } from './foo'; ", errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], }), // ensure resolved path results in warnings test({ code: "import { x } from './bar'; import { y } from 'bar';", + output: "import { x , y } from './bar'; ", settings: { 'import/resolve': { paths: [path.join( process.cwd() , 'tests', 'files' @@ -46,6 +53,8 @@ ruleTester.run('no-duplicates', rule, { // #86: duplicate unresolved modules should be flagged test({ code: "import foo from 'non-existent'; import bar from 'non-existent';", + // Autofix bail because of different default import names. + output: "import foo from 'non-existent'; import bar from 'non-existent';", errors: [ "'non-existent' imported multiple times.", "'non-existent' imported multiple times.", @@ -54,8 +63,286 @@ ruleTester.run('no-duplicates', rule, { test({ code: "import type { x } from './foo'; import type { y } from './foo'", + output: "import type { x , y } from './foo'; ", parser: 'babel-eslint', errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], }), + + test({ + code: "import './foo'; import './foo'", + output: "import './foo'; ", + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: "import { x, /* x */ } from './foo'; import {//y\ny//y2\n} from './foo'", + output: "import { x, /* x */ //y\ny//y2\n} from './foo'; ", + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: "import {x} from './foo'; import {} from './foo'", + output: "import {x} from './foo'; ", + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: "import {x} from './foo'; import {} from './foo'; import {/*c*/} from './foo'; import {y} from './foo'", + output: "import {x/*c*/,y} from './foo'; ", + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: "import { } from './foo'; import {x} from './foo'", + output: "import { x} from './foo'; ", + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: "import './foo'; import {x} from './foo'", + output: "import {x} from './foo'; ", + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: "import'./foo'; import {x} from './foo'", + output: "import {x} from'./foo'; ", + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: "import './foo'; import { /*x*/} from './foo'; import {//y\n} from './foo'; import {z} from './foo'", + output: "import { /*x*///y\nz} from './foo'; ", + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: "import './foo'; import def, {x} from './foo'", + output: "import def, {x} from './foo'; ", + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: "import './foo'; import def from './foo'", + output: "import def from './foo'; ", + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: "import {x} from './foo'; import def from './foo'", + output: "import def, {x} from './foo'; ", + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: "import{x} from './foo'; import def from './foo'", + output: "import def,{x} from './foo'; ", + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: "import {x} from './foo'; import def, {y} from './foo'", + output: "import def, {x,y} from './foo'; ", + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: "import * as ns from './foo'; import {y} from './foo'", + // Autofix bail because first import is a namespace import. + output: "import * as ns from './foo'; import {y} from './foo'", + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: "import {x} from './foo'; import * as ns from './foo'; import {y} from './foo'; import './foo'", + // Autofix could merge some imports, but not the namespace import. + output: "import {x,y} from './foo'; import * as ns from './foo'; ", + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: ` + // some-tool-disable-next-line + import {x} from './foo' + import {//y\ny} from './foo' + `, + // Autofix bail because of comment. + output: ` + // some-tool-disable-next-line + import {x} from './foo' + import {//y\ny} from './foo' + `, + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: ` + import {x} from './foo' + // some-tool-disable-next-line + import {y} from './foo' + `, + // Autofix bail because of comment. + output: ` + import {x} from './foo' + // some-tool-disable-next-line + import {y} from './foo' + `, + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: ` + import {x} from './foo' // some-tool-disable-line + import {y} from './foo' + `, + // Autofix bail because of comment. + output: ` + import {x} from './foo' // some-tool-disable-line + import {y} from './foo' + `, + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: ` + import {x} from './foo' + import {y} from './foo' // some-tool-disable-line + `, + // Autofix bail because of comment. + output: ` + import {x} from './foo' + import {y} from './foo' // some-tool-disable-line + `, + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: ` + import {x} from './foo' + /* comment */ import {y} from './foo' + `, + // Autofix bail because of comment. + output: ` + import {x} from './foo' + /* comment */ import {y} from './foo' + `, + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: ` + import {x} from './foo' + import {y} from './foo' /* comment + multiline */ + `, + // Autofix bail because of comment. + output: ` + import {x} from './foo' + import {y} from './foo' /* comment + multiline */ + `, + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: ` + import {x} from './foo' + import {y} from './foo' + // some-tool-disable-next-line + `, + // Not autofix bail. + output: ` + import {x,y} from './foo' + + // some-tool-disable-next-line + `, + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: ` + import {x} from './foo' + // comment + + import {y} from './foo' + `, + // Not autofix bail. + output: ` + import {x,y} from './foo' + // comment + + + `, + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: ` + import {x} from './foo' + import/* comment */{y} from './foo' + `, + // Autofix bail because of comment. + output: ` + import {x} from './foo' + import/* comment */{y} from './foo' + `, + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: ` + import {x} from './foo' + import/* comment */'./foo' + `, + // Autofix bail because of comment. + output: ` + import {x} from './foo' + import/* comment */'./foo' + `, + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: ` + import {x} from './foo' + import{y}/* comment */from './foo' + `, + // Autofix bail because of comment. + output: ` + import {x} from './foo' + import{y}/* comment */from './foo' + `, + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: ` + import {x} from './foo' + import{y}from/* comment */'./foo' + `, + // Autofix bail because of comment. + output: ` + import {x} from './foo' + import{y}from/* comment */'./foo' + `, + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), + + test({ + code: ` + import {x} from + // some-tool-disable-next-line + './foo' + import {y} from './foo' + `, + // Autofix bail because of comment. + output: ` + import {x} from + // some-tool-disable-next-line + './foo' + import {y} from './foo' + `, + errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'], + }), ], })