From c816b84814214f7504a0d89a5cd3b08c595bfb50 Mon Sep 17 00:00:00 2001 From: Yosuke Ota Date: Thu, 26 Nov 2020 05:21:11 +0900 Subject: [PATCH] fix(eslint-plugin): [consistent-type-imports] crash when using both default and namespace in one import (#2778) --- .../src/rules/consistent-type-imports.ts | 86 +++- .../rules/consistent-type-imports.test.ts | 408 ++++++++++++------ 2 files changed, 341 insertions(+), 153 deletions(-) diff --git a/packages/eslint-plugin/src/rules/consistent-type-imports.ts b/packages/eslint-plugin/src/rules/consistent-type-imports.ts index 830c7b6669a..c4cb6586a00 100644 --- a/packages/eslint-plugin/src/rules/consistent-type-imports.ts +++ b/packages/eslint-plugin/src/rules/consistent-type-imports.ts @@ -252,15 +252,16 @@ export default util.createRule({ ? node.specifiers[0] : null; const namespaceSpecifier: TSESTree.ImportNamespaceSpecifier | null = - node.specifiers[0].type === AST_NODE_TYPES.ImportNamespaceSpecifier - ? node.specifiers[0] - : null; + node.specifiers.find( + (specifier): specifier is TSESTree.ImportNamespaceSpecifier => + specifier.type === AST_NODE_TYPES.ImportNamespaceSpecifier, + ) ?? null; const namedSpecifiers: TSESTree.ImportSpecifier[] = node.specifiers.filter( (specifier): specifier is TSESTree.ImportSpecifier => specifier.type === AST_NODE_TYPES.ImportSpecifier, ); - if (namespaceSpecifier) { + if (namespaceSpecifier && !defaultSpecifier) { // e.g. // import * as types from 'foo' yield* fixToTypeImportByInsertType(fixer, node, false); @@ -268,7 +269,8 @@ export default util.createRule({ } else if (defaultSpecifier) { if ( report.typeSpecifiers.includes(defaultSpecifier) && - namedSpecifiers.length === 0 + namedSpecifiers.length === 0 && + !namespaceSpecifier ) { // e.g. // import Type from 'foo' @@ -279,7 +281,8 @@ export default util.createRule({ if ( namedSpecifiers.every(specifier => report.typeSpecifiers.includes(specifier), - ) + ) && + !namespaceSpecifier ) { // e.g. // import {Type1, Type2} from 'foo' @@ -336,11 +339,40 @@ export default util.createRule({ } } + const fixesRemoveTypeNamespaceSpecifier: TSESLint.RuleFix[] = []; + if ( + namespaceSpecifier && + report.typeSpecifiers.includes(namespaceSpecifier) + ) { + // e.g. + // import Foo, * as Type from 'foo' + // import DefType, * as Type from 'foo' + // import DefType, * as Type from 'foo' + const commaToken = util.nullThrows( + sourceCode.getTokenBefore(namespaceSpecifier, util.isCommaToken), + util.NullThrowsReasons.MissingToken(',', node.type), + ); + + // import Def, * as Ns from 'foo' + // ^^^^^^^^^ remove + fixesRemoveTypeNamespaceSpecifier.push( + fixer.removeRange([commaToken.range[0], namespaceSpecifier.range[1]]), + ); + + // import type * as Ns from 'foo' + // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ insert + yield fixer.insertTextBefore( + node, + `import type ${sourceCode.getText( + namespaceSpecifier, + )} from ${sourceCode.getText(node.source)};\n`, + ); + } if ( defaultSpecifier && report.typeSpecifiers.includes(defaultSpecifier) ) { - if (typeNamedSpecifiers.length === namedSpecifiers.length) { + if (report.typeSpecifiers.length === node.specifiers.length) { const importToken = util.nullThrows( sourceCode.getFirstToken(node, isImportToken), util.NullThrowsReasons.MissingToken('import', node.type), @@ -349,20 +381,36 @@ export default util.createRule({ // ^^^^ insert yield fixer.insertTextAfter(importToken, ' type'); } else { + const commaToken = util.nullThrows( + sourceCode.getTokenAfter(defaultSpecifier, util.isCommaToken), + util.NullThrowsReasons.MissingToken(',', defaultSpecifier.type), + ); + // import Type , {...} from 'foo' + // ^^^^^ pick + const defaultText = sourceCode.text + .slice(defaultSpecifier.range[0], commaToken.range[0]) + .trim(); yield fixer.insertTextBefore( node, - `import type ${sourceCode.getText( - defaultSpecifier, - )} from ${sourceCode.getText(node.source)};\n`, + `import type ${defaultText} from ${sourceCode.getText( + node.source, + )};\n`, + ); + const afterToken = util.nullThrows( + sourceCode.getTokenAfter(commaToken, { includeComments: true }), + util.NullThrowsReasons.MissingToken('any token', node.type), ); // import Type , {...} from 'foo' - // ^^^^^^ remove - yield fixer.remove(defaultSpecifier); - yield fixer.remove(sourceCode.getTokenAfter(defaultSpecifier)!); + // ^^^^^^^ remove + yield fixer.removeRange([ + defaultSpecifier.range[0], + afterToken.range[0], + ]); } } yield* fixesNamedSpecifiers.removeTypeNamedSpecifiers; + yield* fixesRemoveTypeNamespaceSpecifier; yield* afterFixes; @@ -376,6 +424,12 @@ export default util.createRule({ typeNamedSpecifiersText: string; removeTypeNamedSpecifiers: TSESLint.RuleFix[]; } { + if (allNamedSpecifiers.length === 0) { + return { + typeNamedSpecifiersText: '', + removeTypeNamedSpecifiers: [], + }; + } const typeNamedSpecifiersTexts: string[] = []; const removeTypeNamedSpecifiers: TSESLint.RuleFix[] = []; if (typeNamedSpecifiers.length === allNamedSpecifiers.length) { @@ -564,7 +618,11 @@ export default util.createRule({ ), util.NullThrowsReasons.MissingToken('type', node.type), ); - return fixer.remove(typeToken); + const afterToken = util.nullThrows( + sourceCode.getTokenAfter(typeToken, { includeComments: true }), + util.NullThrowsReasons.MissingToken('any token', node.type), + ); + return fixer.removeRange([typeToken.range[0], afterToken.range[0]]); } }, }); 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 88d6db374a0..8818225a024 100644 --- a/packages/eslint-plugin/tests/rules/consistent-type-imports.test.ts +++ b/packages/eslint-plugin/tests/rules/consistent-type-imports.test.ts @@ -225,6 +225,11 @@ ruleTester.run('consistent-type-imports', rule, { jsxFragmentName: 'Fragment', }, }, + ` + import Default, * as Rest from 'module'; + const a: typeof Default = Default; + const b: typeof Rest = Rest; + `, ], invalid: [ { @@ -362,11 +367,11 @@ ruleTester.run('consistent-type-imports', rule, { ], }, { - code: noFormat` + code: ` import * as A from 'foo'; let foo: A.Foo; `, - output: noFormat` + output: ` import type * as A from 'foo'; let foo: A.Foo; `, @@ -381,22 +386,21 @@ ruleTester.run('consistent-type-imports', rule, { { // default and named code: ` - import A, { B } from 'foo'; - let foo: A; - let bar: B; +import A, { B } from 'foo'; +let foo: A; +let bar: B; `, - // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting - output: noFormat` - import type { B } from 'foo'; + output: ` +import type { B } from 'foo'; import type A from 'foo'; - let foo: A; - let bar: B; +let foo: A; +let bar: B; `, errors: [ { messageId: 'typeOverValue', line: 2, - column: 9, + column: 1, }, ], }, @@ -405,7 +409,7 @@ import type A from 'foo'; import A, {} from 'foo'; let foo: A; `, - output: noFormat` + output: ` import type A from 'foo'; let foo: A; `, @@ -419,110 +423,105 @@ import type A from 'foo'; }, { code: ` - import { A, B } from 'foo'; - const foo: A = B(); +import { A, B } from 'foo'; +const foo: A = B(); `, - // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting output: noFormat` - import type { A} from 'foo'; +import type { A} from 'foo'; import { B } from 'foo'; - const foo: A = B(); +const foo: A = B(); `, errors: [ { messageId: 'aImportIsOnlyTypes', data: { typeImports: '"A"' }, line: 2, - column: 9, + column: 1, }, ], }, { code: ` - import { A, B, C } from 'foo'; - const foo: A = B(); - let bar: C; +import { A, B, C } from 'foo'; +const foo: A = B(); +let bar: C; `, - // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting - output: noFormat` - import type { A, C } from 'foo'; + output: ` +import type { A, C } from 'foo'; import { B } from 'foo'; - const foo: A = B(); - let bar: C; +const foo: A = B(); +let bar: C; `, errors: [ { messageId: 'someImportsAreOnlyTypes', data: { typeImports: '"A" and "C"' }, line: 2, - column: 9, + column: 1, }, ], }, { code: ` - import { A, B, C, D } from 'foo'; - const foo: A = B(); - type T = { bar: C; baz: D }; +import { A, B, C, D } from 'foo'; +const foo: A = B(); +type T = { bar: C; baz: D }; `, - // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting - output: noFormat` - import type { A, C, D } from 'foo'; + output: ` +import type { A, C, D } from 'foo'; import { B } from 'foo'; - const foo: A = B(); - type T = { bar: C; baz: D }; +const foo: A = B(); +type T = { bar: C; baz: D }; `, errors: [ { messageId: 'someImportsAreOnlyTypes', data: { typeImports: '"A", "C" and "D"' }, line: 2, - column: 9, + column: 1, }, ], }, { code: ` - import A, { B, C, D } from 'foo'; - B(); - type T = { foo: A; bar: C; baz: D }; +import A, { B, C, D } from 'foo'; +B(); +type T = { foo: A; bar: C; baz: D }; `, - // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting - output: noFormat` - import type { C, D } from 'foo'; + output: ` +import type { C, D } from 'foo'; import type A from 'foo'; -import { B } from 'foo'; - B(); - type T = { foo: A; bar: C; baz: D }; +import { B } from 'foo'; +B(); +type T = { foo: A; bar: C; baz: D }; `, errors: [ { messageId: 'someImportsAreOnlyTypes', data: { typeImports: '"A", "C" and "D"' }, line: 2, - column: 9, + column: 1, }, ], }, { code: ` - import A, { B } from 'foo'; - B(); - type T = A; +import A, { B } from 'foo'; +B(); +type T = A; `, - // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting - output: noFormat` - import type A from 'foo'; -import { B } from 'foo'; - B(); - type T = A; + output: ` +import type A from 'foo'; +import { B } from 'foo'; +B(); +type T = A; `, errors: [ { messageId: 'aImportIsOnlyTypes', data: { typeImports: '"A"' }, line: 2, - column: 9, + column: 1, }, ], }, @@ -560,198 +559,192 @@ import { B } from 'foo'; }, { code: ` - import A, { /* comment */ B } from 'foo'; - type T = B; +import A, { /* comment */ B } from 'foo'; +type T = B; `, - // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting - output: noFormat` - import type { /* comment */ B } from 'foo'; + output: ` +import type { /* comment */ B } from 'foo'; import A from 'foo'; - type T = B; +type T = B; `, errors: [ { messageId: 'aImportIsOnlyTypes', data: { typeImports: '"B"' }, line: 2, - column: 9, + column: 1, }, ], }, { code: noFormat` - import { A, B, C } from 'foo'; - import { D, E, F, } from 'bar'; - type T = A | D; +import { A, B, C } from 'foo'; +import { D, E, F, } from 'bar'; +type T = A | D; `, - // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting output: noFormat` - import type { A} from 'foo'; +import type { A} from 'foo'; import { B, C } from 'foo'; - import type { D} from 'bar'; +import type { D} from 'bar'; import { E, F, } from 'bar'; - type T = A | D; +type T = A | D; `, errors: [ { messageId: 'aImportIsOnlyTypes', data: { typeImports: '"A"' }, line: 2, - column: 9, + column: 1, }, { messageId: 'aImportIsOnlyTypes', data: { typeImports: '"D"' }, line: 3, - column: 9, + column: 1, }, ], }, { code: noFormat` - import { A, B, C } from 'foo'; - import { D, E, F, } from 'bar'; - type T = B | E; +import { A, B, C } from 'foo'; +import { D, E, F, } from 'bar'; +type T = B | E; `, - // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting output: noFormat` - import type { B} from 'foo'; +import type { B} from 'foo'; import { A, C } from 'foo'; - import type { E} from 'bar'; +import type { E} from 'bar'; import { D, F, } from 'bar'; - type T = B | E; +type T = B | E; `, errors: [ { messageId: 'aImportIsOnlyTypes', data: { typeImports: '"B"' }, line: 2, - column: 9, + column: 1, }, { messageId: 'aImportIsOnlyTypes', data: { typeImports: '"E"' }, line: 3, - column: 9, + column: 1, }, ], }, { code: noFormat` - import { A, B, C } from 'foo'; - import { D, E, F, } from 'bar'; - type T = C | F; +import { A, B, C } from 'foo'; +import { D, E, F, } from 'bar'; +type T = C | F; `, - // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting output: noFormat` - import type { C } from 'foo'; +import type { C } from 'foo'; import { A, B } from 'foo'; - import type { F} from 'bar'; +import type { F} from 'bar'; import { D, E } from 'bar'; - type T = C | F; +type T = C | F; `, errors: [ { messageId: 'aImportIsOnlyTypes', data: { typeImports: '"C"' }, line: 2, - column: 9, + column: 1, }, { messageId: 'aImportIsOnlyTypes', data: { typeImports: '"F"' }, line: 3, - column: 9, + column: 1, }, ], }, { // all type fix cases code: ` - import { Type1, Type2 } from 'named_types'; - import Type from 'default_type'; - import * as Types from 'namespace_type'; - import Default, { Named } from 'default_and_named_type'; - type T = Type1 | Type2 | Type | Types.A | Default | Named; +import { Type1, Type2 } from 'named_types'; +import Type from 'default_type'; +import * as Types from 'namespace_type'; +import Default, { Named } from 'default_and_named_type'; +type T = Type1 | Type2 | Type | Types.A | Default | Named; `, - // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting - output: noFormat` - import type { Type1, Type2 } from 'named_types'; - import type Type from 'default_type'; - import type * as Types from 'namespace_type'; - import type { Named } from 'default_and_named_type'; + output: ` +import type { Type1, Type2 } from 'named_types'; +import type Type from 'default_type'; +import type * as Types from 'namespace_type'; +import type { Named } from 'default_and_named_type'; import type Default from 'default_and_named_type'; - type T = Type1 | Type2 | Type | Types.A | Default | Named; +type T = Type1 | Type2 | Type | Types.A | Default | Named; `, errors: [ { messageId: 'typeOverValue', line: 2, - column: 9, + column: 1, }, { messageId: 'typeOverValue', line: 3, - column: 9, + column: 1, }, { messageId: 'typeOverValue', line: 4, - column: 9, + column: 1, }, { messageId: 'typeOverValue', line: 5, - column: 9, + column: 1, }, ], }, { // some type fix cases code: ` - import { Value1, Type1 } from 'named_import'; - import Type2, { Value2 } from 'default_import'; - import Value3, { Type3 } from 'default_import2'; - import Type4, { Type5, Value4 } from 'default_and_named_import'; - type T = Type1 | Type2 | Type3 | Type4 | Type5; +import { Value1, Type1 } from 'named_import'; +import Type2, { Value2 } from 'default_import'; +import Value3, { Type3 } from 'default_import2'; +import Type4, { Type5, Value4 } from 'default_and_named_import'; +type T = Type1 | Type2 | Type3 | Type4 | Type5; `, - // eslint-disable-next-line @typescript-eslint/internal/plugin-test-formatting output: noFormat` - import type { Type1 } from 'named_import'; +import type { Type1 } from 'named_import'; import { Value1 } from 'named_import'; - import type Type2 from 'default_import'; -import { Value2 } from 'default_import'; - import type { Type3 } from 'default_import2'; +import type Type2 from 'default_import'; +import { Value2 } from 'default_import'; +import type { Type3 } from 'default_import2'; import Value3 from 'default_import2'; - import type { Type5} from 'default_and_named_import'; +import type { Type5} from 'default_and_named_import'; import type Type4 from 'default_and_named_import'; -import { Value4 } from 'default_and_named_import'; - type T = Type1 | Type2 | Type3 | Type4 | Type5; +import { Value4 } from 'default_and_named_import'; +type T = Type1 | Type2 | Type3 | Type4 | Type5; `, errors: [ { messageId: 'aImportIsOnlyTypes', data: { typeImports: '"Type1"' }, line: 2, - column: 9, + column: 1, }, { messageId: 'aImportIsOnlyTypes', data: { typeImports: '"Type2"' }, line: 3, - column: 9, + column: 1, }, { messageId: 'aImportIsOnlyTypes', data: { typeImports: '"Type3"' }, line: 4, - column: 9, + column: 1, }, { messageId: 'someImportsAreOnlyTypes', data: { typeImports: '"Type4" and "Type5"' }, line: 5, - column: 9, + column: 1, }, ], }, @@ -795,8 +788,8 @@ import { Value4 } from 'default_and_named_import'; let foo: Foo; `, options: [{ prefer: 'no-type-imports' }], - output: noFormat` - import Foo from 'foo'; + output: ` + import Foo from 'foo'; let foo: Foo; `, errors: [ @@ -813,8 +806,8 @@ import { Value4 } from 'default_and_named_import'; let foo: Foo; `, options: [{ prefer: 'no-type-imports' }], - output: noFormat` - import { Foo } from 'foo'; + output: ` + import { Foo } from 'foo'; let foo: Foo; `, errors: [ @@ -897,8 +890,8 @@ import { Value4 } from 'default_and_named_import'; type T = typeof Type.foo; `, options: [{ prefer: 'no-type-imports' }], - output: noFormat` - import Type from 'foo'; + output: ` + import Type from 'foo'; type T = typeof Type; type T = typeof Type.foo; @@ -919,8 +912,8 @@ import { Value4 } from 'default_and_named_import'; type T = typeof Type.foo; `, options: [{ prefer: 'no-type-imports' }], - output: noFormat` - import { Type } from 'foo'; + output: ` + import { Type } from 'foo'; type T = typeof Type; type T = typeof Type.foo; @@ -941,8 +934,8 @@ import { Value4 } from 'default_and_named_import'; type T = typeof Type.foo; `, options: [{ prefer: 'no-type-imports' }], - output: noFormat` - import * as Type from 'foo'; + output: ` + import * as Type from 'foo'; type T = typeof Type; type T = typeof Type.foo; @@ -1022,8 +1015,8 @@ import { Value4 } from 'default_and_named_import'; export type { Type }; // is a type-only export `, options: [{ prefer: 'no-type-imports' }], - output: noFormat` - import Type from 'foo'; + output: ` + import Type from 'foo'; export { Type }; // is a type-only export export default Type; // is a type-only export @@ -1046,8 +1039,8 @@ import { Value4 } from 'default_and_named_import'; export type { Type }; // is a type-only export `, options: [{ prefer: 'no-type-imports' }], - output: noFormat` - import { Type } from 'foo'; + output: ` + import { Type } from 'foo'; export { Type }; // is a type-only export export default Type; // is a type-only export @@ -1070,8 +1063,8 @@ import { Value4 } from 'default_and_named_import'; export type { Type }; // is a type-only export `, options: [{ prefer: 'no-type-imports' }], - output: noFormat` - import * as Type from 'foo'; + output: ` + import * as Type from 'foo'; export { Type }; // is a type-only export export default Type; // is a type-only export @@ -1085,5 +1078,142 @@ import { Value4 } from 'default_and_named_import'; }, ], }, + { + // type with comments + code: noFormat` +import type /*comment*/ * as AllType from 'foo'; +import type // comment +DefType from 'foo'; +import type /*comment*/ { Type } from 'foo'; + +type T = { a: AllType; b: DefType; c: Type }; + `, + options: [{ prefer: 'no-type-imports' }], + output: noFormat` +import /*comment*/ * as AllType from 'foo'; +import // comment +DefType from 'foo'; +import /*comment*/ { Type } from 'foo'; + +type T = { a: AllType; b: DefType; c: Type }; + `, + errors: [ + { + messageId: 'valueOverType', + line: 2, + column: 1, + }, + { + messageId: 'valueOverType', + line: 3, + column: 1, + }, + { + messageId: 'valueOverType', + line: 5, + column: 1, + }, + ], + }, + { + // https://github.com/typescript-eslint/typescript-eslint/issues/2775 + code: ` +import Default, * as Rest from 'module'; +const a: Rest.A = ''; + `, + options: [{ prefer: 'type-imports' }], + output: ` +import type * as Rest from 'module'; +import Default from 'module'; +const a: Rest.A = ''; + `, + errors: [ + { + messageId: 'aImportIsOnlyTypes', + line: 2, + column: 1, + }, + ], + }, + { + code: ` +import Default, * as Rest from 'module'; +const a: Default = ''; + `, + options: [{ prefer: 'type-imports' }], + output: ` +import type Default from 'module'; +import * as Rest from 'module'; +const a: Default = ''; + `, + errors: [ + { + messageId: 'aImportIsOnlyTypes', + line: 2, + column: 1, + }, + ], + }, + { + code: ` +import Default, * as Rest from 'module'; +const a: Default = ''; +const b: Rest.A = ''; + `, + options: [{ prefer: 'type-imports' }], + output: ` +import type * as Rest from 'module'; +import type Default from 'module'; +const a: Default = ''; +const b: Rest.A = ''; + `, + errors: [ + { + messageId: 'typeOverValue', + line: 2, + column: 1, + }, + ], + }, + { + // type with comments + code: ` +import Default, /*comment*/ * as Rest from 'module'; +const a: Default = ''; + `, + options: [{ prefer: 'type-imports' }], + output: ` +import type Default from 'module'; +import /*comment*/ * as Rest from 'module'; +const a: Default = ''; + `, + errors: [ + { + messageId: 'aImportIsOnlyTypes', + line: 2, + column: 1, + }, + ], + }, + { + // type with comments + code: noFormat` +import Default /*comment1*/, /*comment2*/ { Data } from 'module'; +const a: Default = ''; + `, + options: [{ prefer: 'type-imports' }], + output: noFormat` +import type Default /*comment1*/ from 'module'; +import /*comment2*/ { Data } from 'module'; +const a: Default = ''; + `, + errors: [ + { + messageId: 'aImportIsOnlyTypes', + line: 2, + column: 1, + }, + ], + }, ], });