Skip to content

Commit

Permalink
[Fix] no-unused-modules: make type imports mark a module as used (f…
Browse files Browse the repository at this point in the history
…ixes #1924)
  • Loading branch information
cherryblossom000 authored and ljharb committed Jan 12, 2021
1 parent 00b079e commit 462b016
Show file tree
Hide file tree
Showing 9 changed files with 103 additions and 57 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ This change log adheres to standards from [Keep a CHANGELOG](http://keepachangel
- [`no-webpack-loader-syntax`]/TypeScript: avoid crash on missing name ([#1947], thanks @leonardodino)
- [`no-extraneous-dependencies`]: Add package.json cache ([#1948], thanks @fa93hws)
- [`prefer-default-export`]: handle empty array destructuring ([#1965], thanks @ljharb)
- [`no-unused-modules`]: make type imports mark a module as used (fixes #1924) ([#1974], thanks [@cherryblossom000])

## [2.22.1] - 2020-09-27
### Fixed
Expand Down Expand Up @@ -739,6 +740,8 @@ for info on changes for earlier releases.

[`memo-parser`]: ./memo-parser/README.md

[#1974]: https://github.com/benmosher/eslint-plugin-import/pull/1974
[#1924]: https://github.com/benmosher/eslint-plugin-import/issues/1924
[#1965]: https://github.com/benmosher/eslint-plugin-import/issues/1965
[#1948]: https://github.com/benmosher/eslint-plugin-import/pull/1948
[#1947]: https://github.com/benmosher/eslint-plugin-import/pull/1947
Expand Down Expand Up @@ -1289,3 +1292,4 @@ for info on changes for earlier releases.
[@tomprats]: https://github.com/tomprats
[@straub]: https://github.com/straub
[@andreubotella]: https://github.com/andreubotella
[@cherryblossom000]: https://github.com/cherryblossom000
80 changes: 42 additions & 38 deletions src/ExportMap.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,8 @@ export default class ExportMap {

/**
* ensure that imported name fully resolves.
* @param {[type]} name [description]
* @return {Boolean} [description]
* @param {string} name
* @return {{ found: boolean, path: ExportMap[] }}
*/
hasDeep(name) {
if (this.namespace.has(name)) return { found: true, path: [this] };
Expand Down Expand Up @@ -241,8 +241,8 @@ const availableDocStyleParsers = {

/**
* parse JSDoc from leading comments
* @param {...[type]} comments [description]
* @return {{doc: object}}
* @param {object[]} comments
* @return {{ doc: object }}
*/
function captureJsDoc(comments) {
let doc;
Expand Down Expand Up @@ -286,6 +286,8 @@ function captureTomDoc(comments) {
}
}

const supportedImportTypes = new Set(['ImportDefaultSpecifier', 'ImportNamespaceSpecifier']);

ExportMap.get = function (source, context) {
const path = resolve(source, context);
if (path == null) return null;
Expand Down Expand Up @@ -410,43 +412,27 @@ ExportMap.parse = function (path, content, context) {
return object;
}

function captureDependency(declaration) {
if (declaration.source == null) return null;
if (declaration.importKind === 'type') return null; // skip Flow type imports
const importedSpecifiers = new Set();
const supportedTypes = new Set(['ImportDefaultSpecifier', 'ImportNamespaceSpecifier']);
let hasImportedType = false;
if (declaration.specifiers) {
declaration.specifiers.forEach(specifier => {
const isType = specifier.importKind === 'type';
hasImportedType = hasImportedType || isType;

if (supportedTypes.has(specifier.type) && !isType) {
importedSpecifiers.add(specifier.type);
}
if (specifier.type === 'ImportSpecifier' && !isType) {
importedSpecifiers.add(specifier.imported.name);
}
});
}

// only Flow types were imported
if (hasImportedType && importedSpecifiers.size === 0) return null;
function captureDependency({ source }, isOnlyImportingTypes, importedSpecifiers = new Set()) {
if (source == null) return null;

const p = remotePath(declaration.source.value);
const p = remotePath(source.value);
if (p == null) return null;

const declarationMetadata = {
// capturing actual node reference holds full AST in memory!
source: { value: source.value, loc: source.loc },
isOnlyImportingTypes,
importedSpecifiers,
};

const existing = m.imports.get(p);
if (existing != null) return existing.getter;
if (existing != null) {
existing.declarations.add(declarationMetadata);
return existing.getter;
}

const getter = thunkFor(p, context);
m.imports.set(p, {
getter,
source: { // capturing actual node reference holds full AST in memory!
value: declaration.source.value,
loc: declaration.source.loc,
},
importedSpecifiers,
});
m.imports.set(p, { getter, declarations: new Set([declarationMetadata]) });
return getter;
}

Expand Down Expand Up @@ -483,14 +469,32 @@ ExportMap.parse = function (path, content, context) {
}

if (n.type === 'ExportAllDeclaration') {
const getter = captureDependency(n);
const getter = captureDependency(n, n.exportKind === 'type');
if (getter) m.dependencies.add(getter);
return;
}

// capture namespaces in case of later export
if (n.type === 'ImportDeclaration') {
captureDependency(n);
// import type { Foo } (TS and Flow)
const declarationIsType = n.importKind === 'type';
let isOnlyImportingTypes = declarationIsType;
const importedSpecifiers = new Set();
n.specifiers.forEach(specifier => {
if (supportedImportTypes.has(specifier.type)) {
importedSpecifiers.add(specifier.type);
}
if (specifier.type === 'ImportSpecifier') {
importedSpecifiers.add(specifier.imported.name);
}

// import { type Foo } (Flow)
if (!declarationIsType) {
isOnlyImportingTypes = specifier.importKind === 'type';
}
});
captureDependency(n, isOnlyImportingTypes, importedSpecifiers);

const ns = n.specifiers.find(s => s.type === 'ImportNamespaceSpecifier');
if (ns) {
namespaces.set(ns.local.name, n.source.value);
Expand Down
34 changes: 23 additions & 11 deletions src/rules/no-cycle.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,19 @@ module.exports = {
return; // ignore external modules
}

const imported = Exports.get(sourceNode.value, context);

if (importer.importKind === 'type') {
return; // no Flow import resolution
if (
importer.type === 'ImportDeclaration' && (
// import type { Foo } (TS and Flow)
importer.importKind === 'type' ||
// import { type Foo } (Flow)
importer.specifiers.every(({ importKind }) => importKind === 'type')
)
) {
return; // ignore type imports
}

const imported = Exports.get(sourceNode.value, context);

if (imported == null) {
return; // no-unresolved territory
}
Expand All @@ -70,15 +77,20 @@ module.exports = {
if (traversed.has(m.path)) return;
traversed.add(m.path);

for (const [path, { getter, source }] of m.imports) {
for (const [path, { getter, declarations }] of m.imports) {
if (path === myPath) return true;
if (traversed.has(path)) continue;
if (ignoreModule(source.value)) continue;
if (route.length + 1 < maxDepth) {
untraversed.push({
mget: getter,
route: route.concat(source),
});
for (const { source, isOnlyImportingTypes } of declarations) {
if (ignoreModule(source.value)) continue;
// Ignore only type imports
if (isOnlyImportingTypes) continue;

if (route.length + 1 < maxDepth) {
untraversed.push({
mget: getter,
route: route.concat(source),
});
}
}
}
}
Expand Down
12 changes: 5 additions & 7 deletions src/rules/no-unused-modules.js
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ const importList = new Map();
* keys are the paths to the modules containing the exports, while the
* lower-level Map keys are the specific identifiers or special AST node names
* being exported. The leaf-level metadata object at the moment only contains a
* `whereUsed` propoerty, which contains a Set of paths to modules that import
* `whereUsed` property, which contains a Set of paths to modules that import
* the name.
*
* For example, if we have a file named bar.js containing the following exports:
Expand Down Expand Up @@ -216,12 +216,10 @@ const prepareImportsAndExports = (srcFiles, context) => {
if (isNodeModule(key)) {
return;
}
let localImport = imports.get(key);
if (typeof localImport !== 'undefined') {
localImport = new Set([...localImport, ...value.importedSpecifiers]);
} else {
localImport = value.importedSpecifiers;
}
const localImport = imports.get(key) || new Set();
value.declarations.forEach(({ importedSpecifiers }) =>
importedSpecifiers.forEach(specifier => localImport.add(specifier))
);
imports.set(key, localImport);
});
importList.set(file, imports);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import type {g} from './file-ts-g-used-as-type'
1 change: 1 addition & 0 deletions tests/files/no-unused-modules/typescript/file-ts-f.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
import {g} from './file-ts-g';
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export interface g {}
1 change: 1 addition & 0 deletions tests/files/no-unused-modules/typescript/file-ts-g.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export interface g {}
26 changes: 25 additions & 1 deletion tests/src/rules/no-unused-modules.js
Original file line number Diff line number Diff line change
Expand Up @@ -827,6 +827,31 @@ context('TypeScript', function () {
parser: parser,
filename: testFilePath('./no-unused-modules/typescript/file-ts-e-used-as-type.ts'),
}),
// Should also be valid when the exporting files are linted before the importing ones
test({
options: unusedExportsTypescriptOptions,
code: `export interface g {}`,
parser,
filename: testFilePath('./no-unused-modules/typescript/file-ts-g.ts'),
}),
test({
options: unusedExportsTypescriptOptions,
code: `import {g} from './file-ts-g';`,
parser,
filename: testFilePath('./no-unused-modules/typescript/file-ts-f.ts'),
}),
test({
options: unusedExportsTypescriptOptions,
code: `export interface g {};`,
parser,
filename: testFilePath('./no-unused-modules/typescript/file-ts-g-used-as-type.ts'),
}),
test({
options: unusedExportsTypescriptOptions,
code: `import type {g} from './file-ts-g';`,
parser,
filename: testFilePath('./no-unused-modules/typescript/file-ts-f-import-type.ts'),
}),
],
invalid: [
test({
Expand Down Expand Up @@ -940,4 +965,3 @@ describe('ignore flow types', () => {
invalid: [],
});
});

0 comments on commit 462b016

Please sign in to comment.