From 75dcdf164d206c5530ba7cc095c4599ec90abe35 Mon Sep 17 00:00:00 2001 From: Scott Newcomer Date: Mon, 7 Nov 2022 17:49:40 -0600 Subject: [PATCH] feat(eslint-plugin): [consistent-type-imports] support fixing to inline types (#5050) * feat(eslint-plugin): [consistent-type-imports] support fixing to inline types * first pass fix to simpler fixer * cleanup pass 2 * add test for default import * add some docs * add moar test cases * add failing test * use aImportIsOnlyTypes instead of inlineTypes message * wip * fix for default import inline * fix another case with as * simplify by using existing typeOverValue func * add some commentse * add another test for two imports from same source * better doc with link to 4.5 * fix lint * cleanup comments * Update packages/eslint-plugin/docs/rules/consistent-type-imports.md Co-authored-by: Josh Goldberg * Update packages/eslint-plugin/docs/rules/consistent-type-imports.md Co-authored-by: Josh Goldberg * Update packages/eslint-plugin/docs/rules/consistent-type-imports.md Co-authored-by: Josh Goldberg * Update packages/eslint-plugin/src/rules/consistent-type-imports.ts Co-authored-by: Josh Goldberg * Update packages/eslint-plugin/src/rules/consistent-type-imports.ts Co-authored-by: Josh Goldberg * fix changelog formatting * Update packages/eslint-plugin/docs/rules/consistent-type-imports.md Co-authored-by: Josh Goldberg * try single quotest * cleanup not valid comment anymore * Update packages/eslint-plugin/tests/rules/consistent-type-imports.test.ts Co-authored-by: Josh Goldberg * Update packages/eslint-plugin/tests/rules/consistent-type-imports.test.ts Co-authored-by: Josh Goldberg * rm comment * improve documentation with feedback * add tabs Co-authored-by: Josh Goldberg Co-authored-by: Josh Goldberg --- .../docs/rules/consistent-type-imports.md | 40 +- .../src/rules/consistent-type-imports.ts | 169 +++++++-- .../rules/consistent-type-imports.test.ts | 359 ++++++++++++++++++ 3 files changed, 536 insertions(+), 32 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/consistent-type-imports.md b/packages/eslint-plugin/docs/rules/consistent-type-imports.md index 745930d0d54..4ea1f755994 100644 --- a/packages/eslint-plugin/docs/rules/consistent-type-imports.md +++ b/packages/eslint-plugin/docs/rules/consistent-type-imports.md @@ -15,7 +15,7 @@ This allows transpilers to drop imports without knowing the types of the depende This option defines the expected import kind for type-only imports. Valid values for `prefer` are: -- `type-imports` will enforce that you always use `import type Foo from '...'` except referenced by metadata of decorators. It is default. +- `type-imports` will enforce that you always use `import type Foo from '...'` except referenced by metadata of decorators. It is the default. - `no-type-imports` will enforce that you always use `import Foo from '...'`. Examples of **correct** code with `{prefer: 'type-imports'}`, and **incorrect** code with `{prefer: 'no-type-imports'}`. @@ -36,6 +36,44 @@ type T = Foo; const x: Bar = 1; ``` +### `fixStyle` + +This option defines the expected type modifier to be added when an import is detected as used only in the type position. Valid values for `fixStyle` are: + +- `separate-type-imports` will add the type keyword after the import keyword `import type { A } from '...'`. It is the default. +- `inline-type-imports` will inline the type keyword `import { type A } from '...'` and is only available in TypeScript 4.5 and onwards. See [documentation here](https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-5.html#type-modifiers-on-import-names 'TypeScript 4.5 documentation on type modifiers and import names'). + + + +#### ❌ Incorrect + +```ts +import { Foo } from 'Foo'; +import Bar from 'Bar'; +type T = Foo; +const x: Bar = 1; +``` + +#### ✅ With `separate-type-imports` + +```ts +import type { Foo } from 'Foo'; +import type Bar from 'Bar'; +type T = Foo; +const x: Bar = 1; +``` + +#### ✅ With `inline-type-imports` + +```ts +import { type Foo } from 'Foo'; +import type Bar from 'Bar'; +type T = Foo; +const x: Bar = 1; +``` + + + ### `disallowTypeAnnotations` If `true`, type imports in type annotations (`import()`) are not allowed. diff --git a/packages/eslint-plugin/src/rules/consistent-type-imports.ts b/packages/eslint-plugin/src/rules/consistent-type-imports.ts index 1817018fe7b..a812116f2f8 100644 --- a/packages/eslint-plugin/src/rules/consistent-type-imports.ts +++ b/packages/eslint-plugin/src/rules/consistent-type-imports.ts @@ -4,11 +4,13 @@ import { AST_NODE_TYPES, AST_TOKEN_TYPES } from '@typescript-eslint/utils'; import * as util from '../util'; type Prefer = 'type-imports' | 'no-type-imports'; +type FixStyle = 'separate-type-imports' | 'inline-type-imports'; type Options = [ { prefer?: Prefer; disallowTypeAnnotations?: boolean; + fixStyle?: FixStyle; }, ]; @@ -19,6 +21,8 @@ interface SourceImports { typeOnlyNamedImport: TSESTree.ImportDeclaration | null; // ImportDeclaration for value-only import only with named imports. valueOnlyNamedImport: TSESTree.ImportDeclaration | null; + // ImportDeclaration for value-only import only with default imports and/or named imports. + valueImport: TSESTree.ImportDeclaration | null; } interface ReportValueImport { node: TSESTree.ImportDeclaration; @@ -79,6 +83,9 @@ export default util.createRule({ disallowTypeAnnotations: { type: 'boolean', }, + fixStyle: { + enum: ['separate-type-imports', 'inline-type-imports'], + }, }, additionalProperties: false, }, @@ -90,12 +97,14 @@ export default util.createRule({ { prefer: 'type-imports', disallowTypeAnnotations: true, + fixStyle: 'separate-type-imports', }, ], create(context, [option]) { const prefer = option.prefer ?? 'type-imports'; const disallowTypeAnnotations = option.disallowTypeAnnotations !== false; + const fixStyle = option.fixStyle ?? 'separate-type-imports'; const sourceCode = context.getSourceCode(); const sourceImportsMap: { [key: string]: SourceImports } = {}; @@ -106,13 +115,15 @@ export default util.createRule({ // prefer type imports ImportDeclaration(node): void { const source = node.source.value; + // sourceImports is the object containing all the specifics for a particular import source, type or value const sourceImports = sourceImportsMap[source] ?? (sourceImportsMap[source] = { source, - reportValueImports: [], - typeOnlyNamedImport: null, - valueOnlyNamedImport: null, + reportValueImports: [], // if there is a mismatch where type importKind but value specifiers + typeOnlyNamedImport: null, // if only type imports + valueOnlyNamedImport: null, // if only value imports with named specifiers + valueImport: null, // if only value imports }); if (node.importKind === 'type') { if ( @@ -122,6 +133,7 @@ export default util.createRule({ specifier.type === AST_NODE_TYPES.ImportSpecifier, ) ) { + // definitely import type { TypeX } sourceImports.typeOnlyNamedImport = node; } } else { @@ -133,6 +145,15 @@ export default util.createRule({ ) ) { sourceImports.valueOnlyNamedImport = node; + sourceImports.valueImport = node; + } else if ( + !sourceImports.valueImport && + node.specifiers.some( + specifier => + specifier.type === AST_NODE_TYPES.ImportDefaultSpecifier, + ) + ) { + sourceImports.valueImport = node; } } @@ -243,14 +264,15 @@ export default util.createRule({ 'Program:exit'(): void { for (const sourceImports of Object.values(sourceImportsMap)) { if (sourceImports.reportValueImports.length === 0) { + // nothing to fix. value specifiers and type specifiers are correctly written continue; } for (const report of sourceImports.reportValueImports) { if ( report.valueSpecifiers.length === 0 && - report.unusedSpecifiers.length === 0 + report.unusedSpecifiers.length === 0 && + report.node.importKind !== 'type' ) { - // import is all type-only, convert the entire import to `import type` context.report({ node: report.node, messageId: 'typeOverValue', @@ -265,12 +287,13 @@ export default util.createRule({ } else { const isTypeImport = report.node.importKind === 'type'; - // we have a mixed type/value import, so we need to split them out into multiple exports + // we have a mixed type/value import or just value imports, so we need to split them out into multiple imports if separate-type-imports is configured const importNames = ( isTypeImport - ? report.valueSpecifiers + ? report.valueSpecifiers // import type { A } from 'roo'; // WHERE A is used in value position : report.typeSpecifiers - ).map(specifier => `"${specifier.local.name}"`); + ) // import { A, B } from 'roo'; // WHERE A is used in type position and B is in value position + .map(specifier => `"${specifier.local.name}"`); const message = ((): { messageId: MessageIds; @@ -294,12 +317,12 @@ export default util.createRule({ if (isTypeImport) { return { messageId: 'someImportsInDecoMeta', - data: { typeImports }, + data: { typeImports }, // typeImports are all the value specifiers that are in the type position }; } else { return { messageId: 'someImportsAreOnlyTypes', - data: { typeImports }, + data: { typeImports }, // typeImports are all the type specifiers in the value position }; } } @@ -310,12 +333,14 @@ export default util.createRule({ ...message, *fix(fixer) { if (isTypeImport) { + // take all the valueSpecifiers and put them on a new line yield* fixToValueImportDeclaration( fixer, report, sourceImports, ); } else { + // take all the typeSpecifiers and put them on a new line yield* fixToTypeImportDeclaration( fixer, report, @@ -396,12 +421,12 @@ export default util.createRule({ } /** - * Returns information for fixing named specifiers. + * Returns information for fixing named specifiers, type or value */ function getFixesNamedSpecifiers( fixer: TSESLint.RuleFixer, node: TSESTree.ImportDeclaration, - typeNamedSpecifiers: TSESTree.ImportSpecifier[], + subsetNamedSpecifiers: TSESTree.ImportSpecifier[], allNamedSpecifiers: TSESTree.ImportSpecifier[], ): { typeNamedSpecifiersText: string; @@ -415,12 +440,12 @@ export default util.createRule({ } const typeNamedSpecifiersTexts: string[] = []; const removeTypeNamedSpecifiers: TSESLint.RuleFix[] = []; - if (typeNamedSpecifiers.length === allNamedSpecifiers.length) { + if (subsetNamedSpecifiers.length === allNamedSpecifiers.length) { // import Foo, {Type1, Type2} from 'foo' // import DefType, {Type1, Type2} from 'foo' const openingBraceToken = util.nullThrows( sourceCode.getTokenBefore( - typeNamedSpecifiers[0], + subsetNamedSpecifiers[0], util.isOpeningBraceToken, ), util.NullThrowsReasons.MissingToken('{', node.type), @@ -451,20 +476,20 @@ export default util.createRule({ ), ); } else { - const typeNamedSpecifierGroups: TSESTree.ImportSpecifier[][] = []; + const namedSpecifierGroups: TSESTree.ImportSpecifier[][] = []; let group: TSESTree.ImportSpecifier[] = []; for (const namedSpecifier of allNamedSpecifiers) { - if (typeNamedSpecifiers.includes(namedSpecifier)) { + if (subsetNamedSpecifiers.includes(namedSpecifier)) { group.push(namedSpecifier); } else if (group.length) { - typeNamedSpecifierGroups.push(group); + namedSpecifierGroups.push(group); group = []; } } if (group.length) { - typeNamedSpecifierGroups.push(group); + namedSpecifierGroups.push(group); } - for (const namedSpecifiers of typeNamedSpecifierGroups) { + for (const namedSpecifiers of namedSpecifierGroups) { const { removeRange, textRange } = getNamedSpecifierRanges( namedSpecifiers, allNamedSpecifiers, @@ -541,7 +566,53 @@ export default util.createRule({ if (!util.isCommaToken(before) && !util.isOpeningBraceToken(before)) { insertText = `,${insertText}`; } - return fixer.insertTextBefore(closingBraceToken, `${insertText}`); + return fixer.insertTextBefore(closingBraceToken, insertText); + } + + /** + * insert type keyword to named import node. + * e.g. + * import ADefault, { Already, type Type1, type Type2 } from 'foo' + * ^^^^ insert + */ + function* fixInsertTypeKeywordInNamedSpecifierList( + fixer: TSESLint.RuleFixer, + typeSpecifiers: TSESTree.ImportSpecifier[], + ): IterableIterator { + for (const spec of typeSpecifiers) { + const insertText = sourceCode.text.slice(...spec.range); + yield fixer.replaceTextRange(spec.range, `type ${insertText}`); + } + } + + function* fixInlineTypeImportDeclaration( + fixer: TSESLint.RuleFixer, + report: ReportValueImport, + sourceImports: SourceImports, + ): IterableIterator { + const { node } = report; + // For a value import, will only add an inline type to named specifiers + const { namedSpecifiers } = classifySpecifier(node); + const typeNamedSpecifiers = namedSpecifiers.filter(specifier => + report.typeSpecifiers.includes(specifier), + ); + + if (sourceImports.valueImport) { + // add import named type specifiers to its value import + // import ValueA, { type A } + // ^^^^ insert + const { namedSpecifiers: valueImportNamedSpecifiers } = + classifySpecifier(sourceImports.valueImport); + if ( + sourceImports.valueOnlyNamedImport || + valueImportNamedSpecifiers.length + ) { + yield* fixInsertTypeKeywordInNamedSpecifierList( + fixer, + typeNamedSpecifiers, + ); + } + } } function* fixToTypeImportDeclaration( @@ -567,13 +638,31 @@ export default util.createRule({ // import Type from 'foo' yield* fixInsertTypeSpecifierForImportDeclaration(fixer, node, true); return; + } else if ( + fixStyle === 'inline-type-imports' && + !report.typeSpecifiers.includes(defaultSpecifier) && + namedSpecifiers.length > 0 && + !namespaceSpecifier + ) { + // if there is a default specifier but it isn't a type specifier, then just add the inline type modifier to the named specifiers + // import AValue, {BValue, Type1, Type2} from 'foo' + yield* fixInlineTypeImportDeclaration(fixer, report, sourceImports); + return; } - } else { + } else if (!namespaceSpecifier) { if ( + fixStyle === 'inline-type-imports' && + namedSpecifiers.some(specifier => + report.typeSpecifiers.includes(specifier), + ) + ) { + // import {AValue, Type1, Type2} from 'foo' + yield* fixInlineTypeImportDeclaration(fixer, report, sourceImports); + return; + } else if ( namedSpecifiers.every(specifier => report.typeSpecifiers.includes(specifier), - ) && - !namespaceSpecifier + ) ) { // import {Type1, Type2} from 'foo' yield* fixInsertTypeSpecifierForImportDeclaration(fixer, node, false); @@ -606,12 +695,25 @@ export default util.createRule({ afterFixes.push(insertTypeNamedSpecifiers); } } else { - yield fixer.insertTextBefore( - node, - `import type {${ - fixesNamedSpecifiers.typeNamedSpecifiersText - }} from ${sourceCode.getText(node.source)};\n`, - ); + // The import is both default and named. Insert named on new line because can't mix default type import and named type imports + if (fixStyle === 'inline-type-imports') { + yield fixer.insertTextBefore( + node, + `import {${typeNamedSpecifiers + .map(spec => { + const insertText = sourceCode.text.slice(...spec.range); + return `type ${insertText}`; + }) + .join(', ')}} from ${sourceCode.getText(node.source)};\n`, + ); + } else { + yield fixer.insertTextBefore( + node, + `import type {${ + fixesNamedSpecifiers.typeNamedSpecifiersText + }} from ${sourceCode.getText(node.source)};\n`, + ); + } } } @@ -736,8 +838,6 @@ export default util.createRule({ closingBraceToken.range[1], ); if (node.specifiers.length > 1) { - // import type Foo from 'foo' - // import type {...} from 'foo' // <- insert yield fixer.insertTextAfter( node, `\nimport type${specifiersText} from ${sourceCode.getText( @@ -794,6 +894,9 @@ export default util.createRule({ } } + // we have some valueSpecifiers intermixed in types that need to be put on their own line + // import type { Type1, A } from 'foo' + // import type { A } from 'foo' const valueNamedSpecifiers = namedSpecifiers.filter(specifier => report.valueSpecifiers.includes(specifier), ); @@ -819,6 +922,8 @@ export default util.createRule({ afterFixes.push(insertTypeNamedSpecifiers); } } else { + // some are types. + // Add new value import and later remove those value specifiers from import type yield fixer.insertTextBefore( node, `import {${ @@ -862,6 +967,8 @@ export default util.createRule({ fixer: TSESLint.RuleFixer, node: TSESTree.ImportSpecifier, ): IterableIterator { + // import { type Foo } from 'foo' + // ^^^^ remove const typeToken = util.nullThrows( sourceCode.getFirstToken(node, isTypeToken), util.NullThrowsReasons.MissingToken('type', node.type), diff --git a/packages/eslint-plugin/tests/rules/consistent-type-imports.test.ts b/packages/eslint-plugin/tests/rules/consistent-type-imports.test.ts index b1389b26e6b..ee01b29c1b0 100644 --- a/packages/eslint-plugin/tests/rules/consistent-type-imports.test.ts +++ b/packages/eslint-plugin/tests/rules/consistent-type-imports.test.ts @@ -118,11 +118,80 @@ ruleTester.run('consistent-type-imports', rule, { `, options: [{ prefer: 'no-type-imports' }], }, + ` + import { type A } from 'foo'; + type T = A; + `, ` import { type A, B } from 'foo'; type T = A; const b = B; `, + ` + import { type A, type B } from 'foo'; + type T = A; + type Z = B; + `, + ` + import { B } from 'foo'; + import { type A } from 'foo'; + type T = A; + const b = B; + `, + { + code: ` + import { B, type A } from 'foo'; + type T = A; + const b = B; + `, + options: [{ fixStyle: 'inline-type-imports' }], + }, + { + code: ` + import { B } from 'foo'; + import type A from 'baz'; + type T = A; + const b = B; + `, + options: [{ fixStyle: 'inline-type-imports' }], + }, + { + code: ` + import { type B } from 'foo'; + import type { A } from 'foo'; + type T = A; + const b = B; + `, + options: [{ fixStyle: 'inline-type-imports' }], + }, + { + code: ` + import { B, type C } from 'foo'; + import type A from 'baz'; + type T = A; + type Z = C; + const b = B; + `, + options: [{ prefer: 'type-imports', fixStyle: 'inline-type-imports' }], + }, + { + code: ` + import { B } from 'foo'; + import type { A } from 'foo'; + type T = A; + const b = B; + `, + options: [{ prefer: 'type-imports', fixStyle: 'inline-type-imports' }], + }, + { + code: ` + import { B } from 'foo'; + import { A } from 'foo'; + type T = A; + const b = B; + `, + options: [{ prefer: 'no-type-imports', fixStyle: 'inline-type-imports' }], + }, // exports ` import Type from 'foo'; @@ -512,6 +581,24 @@ export type Y = { }, ], }, + { + code: ` + import Foo from 'foo'; + let foo: Foo; + `, + output: ` + import type Foo from 'foo'; + let foo: Foo; + `, + options: [{ prefer: 'type-imports', fixStyle: 'inline-type-imports' }], + errors: [ + { + messageId: 'typeOverValue', + line: 2, + column: 9, + }, + ], + }, { code: ` import { A, B } from 'foo'; @@ -1833,5 +1920,277 @@ const b = B; }, ], }, + + // inline-type-imports + { + code: ` + import { A, B } from 'foo'; + let foo: A; + let bar: B; + `, + output: ` + import { type A, type B } from 'foo'; + let foo: A; + let bar: B; + `, + options: [{ prefer: 'type-imports', fixStyle: 'inline-type-imports' }], + errors: [ + { + messageId: 'typeOverValue', + line: 2, + column: 9, + }, + ], + }, + { + code: ` + import { A, B } from 'foo'; + + let foo: A; + B(); + `, + output: ` + import { type A, B } from 'foo'; + + let foo: A; + B(); + `, + options: [{ prefer: 'type-imports', fixStyle: 'inline-type-imports' }], + errors: [ + { + messageId: 'aImportIsOnlyTypes', + line: 2, + column: 9, + }, + ], + }, + { + code: ` + import { A, B } from 'foo'; + type T = A; + B(); + `, + output: ` + import { type A, B } from 'foo'; + type T = A; + B(); + `, + options: [{ prefer: 'type-imports', fixStyle: 'inline-type-imports' }], + errors: [ + { + messageId: 'aImportIsOnlyTypes', + line: 2, + column: 9, + }, + ], + }, + { + code: ` + import { A } from 'foo'; + import { B } from 'foo'; + type T = A; + type U = B; + `, + output: ` + import { type A } from 'foo'; + import { type B } from 'foo'; + type T = A; + type U = B; + `, + options: [{ prefer: 'type-imports', fixStyle: 'inline-type-imports' }], + errors: [ + { + messageId: 'typeOverValue', + line: 2, + column: 9, + }, + { + messageId: 'typeOverValue', + line: 3, + column: 9, + }, + ], + }, + { + code: ` + import { A } from 'foo'; + import B from 'foo'; + type T = A; + type U = B; + `, + output: ` + import { type A } from 'foo'; + import type B from 'foo'; + type T = A; + type U = B; + `, + options: [{ prefer: 'type-imports', fixStyle: 'inline-type-imports' }], + errors: [ + { + messageId: 'typeOverValue', + line: 2, + column: 9, + }, + { + messageId: 'typeOverValue', + line: 3, + column: 9, + }, + ], + }, + { + code: ` +import A, { B, C } from 'foo'; +type T = B; +type U = C; +A(); + `, + output: ` +import A, { type B, type C } from 'foo'; +type T = B; +type U = C; +A(); + `, + options: [{ prefer: 'type-imports', fixStyle: 'inline-type-imports' }], + errors: [ + { + messageId: 'someImportsAreOnlyTypes', + line: 2, + column: 1, + }, + ], + }, + { + code: ` +import A, { B, C } from 'foo'; +type T = B; +type U = C; +type V = A; + `, + output: ` +import {type B, type C} from 'foo'; +import type A from 'foo'; +type T = B; +type U = C; +type V = A; + `, + options: [{ prefer: 'type-imports', fixStyle: 'inline-type-imports' }], + errors: [ + { + messageId: 'typeOverValue', + line: 2, + column: 1, + }, + ], + }, + { + code: ` +import A, { B, C as D } from 'foo'; +type T = B; +type U = D; +type V = A; + `, + output: ` +import {type B, type C as D} from 'foo'; +import type A from 'foo'; +type T = B; +type U = D; +type V = A; + `, + options: [{ prefer: 'type-imports', fixStyle: 'inline-type-imports' }], + errors: [ + { + messageId: 'typeOverValue', + line: 2, + column: 1, + }, + ], + }, + { + code: ` + import { /* comment */ A, B } from 'foo'; + type T = A; + `, + output: ` + import { /* comment */ type A, B } from 'foo'; + type T = A; + `, + options: [{ prefer: 'type-imports', fixStyle: 'inline-type-imports' }], + errors: [ + { + messageId: 'aImportIsOnlyTypes', + line: 2, + column: 9, + }, + ], + }, + { + code: ` + import { B, /* comment */ A } from 'foo'; + type T = A; + `, + output: ` + import { B, /* comment */ type A } from 'foo'; + type T = A; + `, + options: [{ prefer: 'type-imports', fixStyle: 'inline-type-imports' }], + errors: [ + { + messageId: 'aImportIsOnlyTypes', + line: 2, + column: 9, + }, + ], + }, + { + code: ` +import { A, B, C } from 'foo'; +import type { D } from 'deez'; + +const foo: A = B(); +let bar: C; +let baz: D; + `, + output: ` +import { type A, B, type C } from 'foo'; +import type { D } from 'deez'; + +const foo: A = B(); +let bar: C; +let baz: D; + `, + options: [{ prefer: 'type-imports', fixStyle: 'inline-type-imports' }], + errors: [ + { + messageId: 'someImportsAreOnlyTypes', + line: 2, + column: 1, + }, + ], + }, + { + code: ` +import { A, B, type C } from 'foo'; +import type { D } from 'deez'; +const foo: A = B(); +let bar: C; +let baz: D; + `, + output: ` +import { type A, B, type C } from 'foo'; +import type { D } from 'deez'; +const foo: A = B(); +let bar: C; +let baz: D; + `, + options: [{ prefer: 'type-imports', fixStyle: 'inline-type-imports' }], + errors: [ + { + messageId: 'aImportIsOnlyTypes', + line: 2, + column: 1, + }, + ], + }, ], });