Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove duplicate identifiers in duplicate imports #2577

Merged
merged 1 commit into from Feb 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Expand Up @@ -6,6 +6,9 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange

## [Unreleased]

### Fixed
- [`no-duplicates`]: remove duplicate identifiers in duplicate imports ([#2577], thanks [@joe-matsec])

### Changed
- [Docs] [`no-duplicates`]: fix example schema ([#2684], thanks [@simmo])

Expand Down Expand Up @@ -1389,6 +1392,7 @@ for info on changes for earlier releases.
[#2668]: https://github.com/import-js/eslint-plugin-import/issues/2668
[#2666]: https://github.com/import-js/eslint-plugin-import/issues/2666
[#2665]: https://github.com/import-js/eslint-plugin-import/issues/2665
[#2577]: https://github.com/import-js/eslint-plugin-import/issues/2577
[#2444]: https://github.com/import-js/eslint-plugin-import/issues/2444
[#2412]: https://github.com/import-js/eslint-plugin-import/issues/2412
[#2392]: https://github.com/import-js/eslint-plugin-import/issues/2392
Expand Down Expand Up @@ -1703,6 +1707,7 @@ for info on changes for earlier releases.
[@jimbolla]: https://github.com/jimbolla
[@jkimbo]: https://github.com/jkimbo
[@joaovieira]: https://github.com/joaovieira
[@joe-matsec]: https://github.com/joe-matsec
[@johndevedu]: https://github.com/johndevedu
[@johnthagen]: https://github.com/johnthagen
[@jonboiser]: https://github.com/jonboiser
Expand Down
31 changes: 23 additions & 8 deletions src/rules/no-duplicates.js
Expand Up @@ -79,8 +79,7 @@ function getFix(first, rest, sourceCode, context) {

return {
importNode: node,
text: sourceCode.text.slice(openBrace.range[1], closeBrace.range[0]),
hasTrailingComma: isPunctuator(sourceCode.getTokenBefore(closeBrace), ','),
identifiers: sourceCode.text.slice(openBrace.range[1], closeBrace.range[0]).split(','), // Split the text into separate identifiers (retaining any whitespace before or after)
isEmpty: !hasSpecifiers(node),
};
})
Expand Down Expand Up @@ -111,9 +110,15 @@ function getFix(first, rest, sourceCode, context) {
closeBrace != null &&
isPunctuator(sourceCode.getTokenBefore(closeBrace), ',');
const firstIsEmpty = !hasSpecifiers(first);
const firstExistingIdentifiers = firstIsEmpty
? new Set()
: new Set(sourceCode.text.slice(openBrace.range[1], closeBrace.range[0])
.split(',')
.map((x) => x.trim()),
);

const [specifiersText] = specifiers.reduce(
([result, needsComma], specifier) => {
([result, needsComma, existingIdentifiers], specifier) => {
const isTypeSpecifier = specifier.importNode.importKind === 'type';

const preferInline = context.options[0] && context.options[0]['prefer-inline'];
Expand All @@ -122,15 +127,25 @@ function getFix(first, rest, sourceCode, context) {
throw new Error('Your version of TypeScript does not support inline type imports.');
}

const insertText = `${preferInline && isTypeSpecifier ? 'type ' : ''}${specifier.text}`;
// Add *only* the new identifiers that don't already exist, and track any new identifiers so we don't add them again in the next loop
const [specifierText, updatedExistingIdentifiers] = specifier.identifiers.reduce(([text, set], cur) => {
const trimmed = cur.trim(); // Trim whitespace before/after to compare to our set of existing identifiers
const curWithType = trimmed.length > 0 && preferInline && isTypeSpecifier ? `type ${cur}` : cur;
if (existingIdentifiers.has(trimmed)) {
return [text, set];
}
return [text.length > 0 ? `${text},${curWithType}` : curWithType, set.add(trimmed)];
}, ['', existingIdentifiers]);

return [
needsComma && !specifier.isEmpty
? `${result},${insertText}`
: `${result}${insertText}`,
needsComma && !specifier.isEmpty && specifierText.length > 0
? `${result},${specifierText}`
: `${result}${specifierText}`,
specifier.isEmpty ? needsComma : true,
updatedExistingIdentifiers,
];
},
['', !firstHasTrailingComma && !firstIsEmpty],
['', !firstHasTrailingComma && !firstIsEmpty, firstExistingIdentifiers],
);

const fixes = [];
Expand Down
31 changes: 31 additions & 0 deletions tests/src/rules/no-duplicates.js
Expand Up @@ -5,6 +5,7 @@ import jsxConfig from '../../../config/react';
import { RuleTester } from 'eslint';
import eslintPkg from 'eslint/package.json';
import semver from 'semver';
import flatMap from 'array.prototype.flatmap';

const ruleTester = new RuleTester();
const rule = require('rules/no-duplicates');
Expand Down Expand Up @@ -130,6 +131,36 @@ ruleTester.run('no-duplicates', rule, {
errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'],
}),

// These test cases use duplicate import identifiers, which causes a fatal parsing error using ESPREE (default) and TS_OLD.
...flatMap([parsers.BABEL_OLD, parsers.TS_NEW], parser => {
if (!parser) return []; // TS_NEW is not always available
return [
// #2347: duplicate identifiers should be removed
test({
code: "import {a} from './foo'; import { a } from './foo'",
output: "import {a} from './foo'; ",
errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'],
parser,
}),

// #2347: duplicate identifiers should be removed
test({
code: "import {a,b} from './foo'; import { b, c } from './foo'; import {b,c,d} from './foo'",
output: "import {a,b, c ,d} from './foo'; ",
errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'],
parser,
}),

// #2347: duplicate identifiers should be removed, but not if they are adjacent to comments
test({
code: "import {a} from './foo'; import { a/*,b*/ } from './foo'",
output: "import {a, a/*,b*/ } from './foo'; ",
errors: ['\'./foo\' imported multiple times.', '\'./foo\' imported multiple times.'],
parser,
}),
];
}),

test({
code: "import {x} from './foo'; import {} from './foo'; import {/*c*/} from './foo'; import {y} from './foo'",
output: "import {x/*c*/,y} from './foo'; ",
Expand Down