feat(eslint-plugin): add `consistent-type-imports` rule #2367

ota-meshi · 2020-08-07T01:17:40Z

This PR adds the consistent-type-imports rule.

typescript-eslint · 2020-08-07T01:17:46Z

Thanks for the PR, @ota-meshi!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

codecov · 2020-08-07T01:27:54Z

Codecov Report

Merging #2367 into v4 will increase coverage by 0.07%.
The diff coverage is 96.91%.

@@            Coverage Diff             @@
##               v4    #2367      +/-   ##
==========================================
+ Coverage   92.86%   92.93%   +0.07%     
==========================================
  Files         286      287       +1     
  Lines        9064     9226     +162     
  Branches     2517     2564      +47     
==========================================
+ Hits         8417     8574     +157     
- Misses        319      320       +1     
- Partials      328      332       +4

Flag	Coverage Δ
#unittest	`92.93% <96.91%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/eslint-plugin/src/configs/all.ts	`100.00% <ø> (ø)`
...eslint-plugin/src/rules/consistent-type-imports.ts	`96.91% <96.91%> (ø)`

bradzacher

Thanks for working on this!
This is a great start. A few comments on the docs and the implementation.

bradzacher · 2020-08-08T22:53:59Z

packages/eslint-plugin/docs/rules/consistent-type-imports.md

+
+## When Not To Use It
+
+If you specifically want to use both import kinds for stylistic reasons, you can disable this rule.


Suggested change

If you specifically want to use both import kinds for stylistic reasons, you can disable this rule.

- If you are not using TypeScript 3.8 (or greater), then you will not be able to use this rule, as type-only imports are not allowed.

- If you specifically want to use both import kinds for stylistic reasons, you can disable this rule.

bradzacher · 2020-08-08T22:54:20Z

packages/eslint-plugin/docs/rules/consistent-type-imports.md

+```ts
+import type { Foo } from './foo';
+let foo: Foo;
+```
+
+```ts
+import { Foo } from './foo';
+let foo: Foo;
+```
+
+```ts
+let foo: import('foo').Foo;
+```


I think remove these examples here, and we can provide all the examples under the specific options

packages/eslint-plugin/docs/rules/consistent-type-imports.md

bradzacher · 2020-08-08T23:08:28Z

packages/eslint-plugin/src/rules/consistent-type-imports.ts

+          }
+        : {
+            // prefer no type imports
+            'ImportDeclaration[importKind=type]'(


styling nitpick

Suggested change

'ImportDeclaration[importKind=type]'(

'ImportDeclaration[importKind = "type"]'(

bradzacher · 2020-08-08T23:16:00Z

packages/eslint-plugin/src/rules/consistent-type-imports.ts

+                  const importToken = sourceCode.getFirstToken(node)!;
+                  return fixer.removeRange([
+                    importToken.range[1],
+                    sourceCode.getTokenAfter(importToken)!.range[1],


You can use the second argument to getFirstToken to ensure you fetch a type keyword.
You can also use our nullThrows util to throw with a nice error message if it's not found.

Suggested change

const importToken = sourceCode.getFirstToken(node)!;

return fixer.removeRange([

importToken.range[1],

sourceCode.getTokenAfter(importToken)!.range[1],

const importToken = util.nullThrows(

sourceCode.getFirstToken(

node,

token =>

token.type === AST_TOKEN_TYPES.Keyword && token.value === 'type',

),

util.NullThrowsReasons.MissingToken('type', node.type),

);

return fixer.removeRange([

importToken.range[1],

sourceCode.getTokenAfter(importToken)!.range[1],

bradzacher · 2020-08-08T23:43:20Z

packages/eslint-plugin/tests/rules/consistent-type-imports.test.ts

+        let bar: B;
+      `,
+      output: `
+        import type A, { B } from 'foo';


this is invalid syntax - a type-only import can specify a default OR named import, not both

I didn't know that. Thank you for teaching me!

bradzacher · 2020-08-09T00:05:31Z

packages/eslint-plugin/src/rules/consistent-type-imports.ts

+        ? {
+            // prefer type imports
+            'ImportDeclaration[importKind=value]'(
+              node: TSESTree.ImportDeclaration,
+            ): void {
+              let used = false;
+              for (const specifier of node.specifiers) {
+                const id = specifier.local;
+                const variable = context
+                  .getScope()
+                  .variables.find(v => v.name === id.name)!;
+                for (const ref of variable.references) {
+                  if (ref.identifier !== id) {
+                    referenceIdToDeclMap.set(ref.identifier, node);
+                    used = true;
+                  }
+                }
+              }
+              if (used) {
+                allValueImports.push(node);
+              }
+            },
+            'TSTypeReference Identifier'(node: TSESTree.Identifier): void {
+              // Remove type reference ids
+              referenceIdToDeclMap.delete(node);
+            },
+            'Program:exit'(): void {
+              const usedAsValueImports = new Set(referenceIdToDeclMap.values());
+              for (const valueImport of allValueImports) {
+                if (usedAsValueImports.has(valueImport)) {
+                  continue;
+                }
+                context.report({
+                  node: valueImport,
+                  messageId: 'typeOverValue',
+                  fix(fixer) {
+                    // import type Foo from 'foo'
+                    //       ^^^^^ insert
+                    const importToken = sourceCode.getFirstToken(valueImport)!;
+                    return fixer.insertTextAfter(importToken, ' type');
+                  },
+                });
+              }
+            },
+          }


This is a great start! But there are a few problems I can see:

this code will incorrectly convert import A, { B } from 'foo' to import type A, { B } from 'foo'

type-only imports must only have a default OR named imports, not both.

this code explicitly ignores the case where you have a mixture of type and value imports

the TSTypeReference Identifier selector is inaccurate, as it does not handle shadowing:
import Type from 'foo'; type T<Type> = Type; // this "Type" shadows the imported "Type", and will false positive

You should be able to lean much harder on the scope analyser here - as I built the scope analyser with some helpers to make it easier to inspect references.

I had a quick play, below is some code to help you. The difficult bit now will writing a resilient fixer:

{ 'ImportDeclaration[importKind = "value"]'( node: TSESTree.ImportDeclaration, ): void { const variables = context.getDeclaredVariables(node); const typeVariables: TSESLint.Scope.Variable[] = []; const valueVariables: TSESLint.Scope.Variable[] = []; for (const variable of variables) { const onlyHasTypeReferences = variable.references.every(ref => { if (ref.isTypeReference) { return true; } if (ref.isValueReference) { // `type T = typeof foo` will create a value reference because "foo" must be a value type // however this value reference is safe to use with type-only imports let parent = ref.identifier.parent; while (parent) { if (parent.type === AST_NODE_TYPES.TSTypeQuery) { return true; } // TSTypeQuery must have a TSESTree.EntityName as its child, so we can filter here and break early if (parent.type !== AST_NODE_TYPES.TSQualifiedName) { break; } parent = parent.parent; } } return false; }); if (onlyHasTypeReferences) { typeVariables.push(variable); } else { valueVariables.push(variable); } } if (valueVariables.length === 0) { // import is all type-only, convert the entire import to `import type` // EXAMPLES: // import { Type1, Type2 } from 'foo'; // should be fixed to: // import type { Type1, Type2 } from 'foo'; // import Type from 'foo'; // should be fixed to: // import type Type from 'foo'; // import * as Type from 'foo'; // should be fixed to: // import type * as Type from 'foo'; // !!! NOTE: must take special care in this case: // import Default, { Named } from 'foo'; // should be fixed to: // import type Default from 'foo'; // import type { Named } from 'foo'; // context.report({ message: 'All imports in the declaration are only used as types (or some error like that)' }) } else { // we have a mixed type/value import, so we need to split them out into multiple exports // EXAMPLES: // import { Value, Type } from 'foo'; // should be fixed to: // import type { Type } from 'foo'; // import { Value } from 'foo'; // import Type, { Value } from 'foo'; // should be fixed to: // import type Type from 'foo'; // import { Value } from 'foo'; // import Value, { Type } from 'foo'; // should be fixed to: // import type { Type } from 'foo'; // import Value from 'foo'; // !!! NOTE: must take special care in this case: // import Type1, { Type2, Value } from 'foo'; // should be fixed to: // import type Type1 from 'foo'; // import type { Type2 } from 'foo'; // import { Value } from 'foo'; // context.report({ message: 'Imports "A", "B" and "C" are only used as types (or some error like that)' }) } }, }

bradzacher · 2020-08-09T00:09:20Z

packages/eslint-plugin/tests/rules/consistent-type-imports.test.ts

+  },
+});
+
+ruleTester.run('consistent-type-imports', rule, {


some extra test cases that you'll want to test (the `typeof` query)

import Type from 'foo'; type T = typeof Type; type T = typeof Type.foo;

import type Type from 'foo'; type T = typeof Type; type T = typeof Type.foo;

import { Type } from 'foo'; type T = typeof Type; type T = typeof Type.foo;

import type { Type } from 'foo'; type T = typeof Type; type T = typeof Type.foo;

import * as Type from 'foo'; type T = typeof Type; type T = typeof Type.foo;

import type * as Type from 'foo'; type T = typeof Type; type T = typeof Type.foo;

some extra test cases that you'll want to test (exports)

import Type from 'foo'; export { Type }; // is a value export export default Type; // is a value export export type { Type }; // is a type-only export

import type Type from 'foo'; export { Type }; // is a type-only export export default Type; // is a type-only export export type { Type }; // is a type-only export

import { Type } from 'foo'; export { Type }; // is a value export export default Type; // is a value export export type { Type }; // is a type-only export

import type { Type } from 'foo'; export { Type }; // is a type-only export export default Type; // is a type-only export export type { Type }; // is a type-only export

import * as Type from 'foo'; export { Type }; // is a value export export default Type; // is a value export export type { Type }; // is a type-only export

import type * as Type from 'foo'; export { Type }; // is a type-only export export default Type; // is a type-only export export type { Type }; // is a type-only export

Plus the examples I listed in the comments in the suggestion here

ota-meshi · 2020-08-10T09:35:40Z

@bradzacher Thank you for your review!

I have made your requested changes to this PR. So please check again it.

This change made the auto-fix complicated. I think it should be simplified if other rules can support it. Let me know if you know a good way.

ota-meshi · 2020-08-11T03:34:51Z

@bradzacher
I am making a mistake in the base branch that should be the basis for this PR?
I saw the following comment and I thought I should fork from the v4 branch.
#2200 (comment)

I misunderstood. Forget what I said earlier.

bradzacher

This LGTM!
Thanks for your work here - the fixer is a complex piece of work so you've done a great job 😄

tilgovi · 2020-08-17T19:57:30Z

Has adding this to the recommended config been discussed?

bradzacher · 2020-08-17T19:59:31Z

This rule requires a certain (recent) version of TS, so it can't and won't be added to recommended until our minimum version range matches.

tilgovi · 2020-08-17T20:01:58Z

Thank you. That makes sense.

bradzacher added the enhancement: new plugin rule New rule request for eslint-plugin label Aug 7, 2020

bradzacher added this to the 4.0.0 milestone Aug 7, 2020

ota-meshi force-pushed the consistent-type-import branch from 2eaf38b to 182bf9a Compare August 7, 2020 01:25

bradzacher requested changes Aug 9, 2020

View reviewed changes

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Aug 9, 2020

bradzacher removed this from the 4.0.0 milestone Aug 9, 2020

bradzacher force-pushed the v4 branch from 5ab473c to e925538 Compare August 9, 2020 23:59

ota-meshi force-pushed the consistent-type-import branch from c0e572a to d4215cb Compare August 10, 2020 09:23

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Aug 10, 2020

bradzacher force-pushed the v4 branch from cf29440 to 12b9c8a Compare August 10, 2020 23:55

ota-meshi added 2 commits August 11, 2020 12:44

feat(eslint-plugin): add consistent-type-imports rule

fe60024

feat(eslint-plugin): make the requested changes

bb913f0

ota-meshi force-pushed the consistent-type-import branch from d4215cb to bb913f0 Compare August 11, 2020 03:44

bradzacher approved these changes Aug 16, 2020

View reviewed changes

bradzacher merged commit ba9295b into typescript-eslint:v4 Aug 16, 2020

bradzacher linked an issue Aug 16, 2020 that may be closed by this pull request

Proposal for new rule - consistent style for type-only imports #2200

Closed

ota-meshi deleted the consistent-type-import branch August 16, 2020 21:30

bradzacher pushed a commit that referenced this pull request Aug 19, 2020

feat(eslint-plugin): add consistent-type-imports rule (#2367)

b736705

bradzacher pushed a commit that referenced this pull request Aug 29, 2020

feat(eslint-plugin): add consistent-type-imports rule (#2367)

73d605d

bradzacher pushed a commit that referenced this pull request Aug 29, 2020

feat(eslint-plugin): add consistent-type-imports rule (#2367)

58b1c2d

github-actions bot locked as resolved and limited conversation to collaborators Sep 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): add `consistent-type-imports` rule #2367

feat(eslint-plugin): add `consistent-type-imports` rule #2367

ota-meshi commented Aug 7, 2020

typescript-eslint bot commented Aug 7, 2020

codecov bot commented Aug 7, 2020 •

edited

bradzacher left a comment

bradzacher Aug 8, 2020

bradzacher Aug 8, 2020

bradzacher Aug 8, 2020

bradzacher Aug 8, 2020

bradzacher Aug 8, 2020

ota-meshi Aug 10, 2020

bradzacher Aug 9, 2020

bradzacher Aug 9, 2020 •

edited

ota-meshi commented Aug 10, 2020

ota-meshi commented Aug 11, 2020 •

edited

bradzacher left a comment

tilgovi commented Aug 17, 2020

bradzacher commented Aug 17, 2020

tilgovi commented Aug 17, 2020


		## When Not To Use It

		If you specifically want to use both import kinds for stylistic reasons, you can disable this rule.

	If you specifically want to use both import kinds for stylistic reasons, you can disable this rule.
	- If you are not using TypeScript 3.8 (or greater), then you will not be able to use this rule, as type-only imports are not allowed.
	- If you specifically want to use both import kinds for stylistic reasons, you can disable this rule.

	'ImportDeclaration[importKind=type]'(
	'ImportDeclaration[importKind = "type"]'(

feat(eslint-plugin): add consistent-type-imports rule #2367

feat(eslint-plugin): add consistent-type-imports rule #2367

Conversation

ota-meshi commented Aug 7, 2020

typescript-eslint bot commented Aug 7, 2020

codecov bot commented Aug 7, 2020 • edited

Codecov Report

bradzacher left a comment

Choose a reason for hiding this comment

bradzacher Aug 8, 2020

Choose a reason for hiding this comment

bradzacher Aug 8, 2020

Choose a reason for hiding this comment

bradzacher Aug 8, 2020

Choose a reason for hiding this comment

bradzacher Aug 8, 2020

Choose a reason for hiding this comment

bradzacher Aug 8, 2020

Choose a reason for hiding this comment

ota-meshi Aug 10, 2020

Choose a reason for hiding this comment

bradzacher Aug 9, 2020

Choose a reason for hiding this comment

bradzacher Aug 9, 2020 • edited

Choose a reason for hiding this comment

ota-meshi commented Aug 10, 2020

ota-meshi commented Aug 11, 2020 • edited

bradzacher left a comment

Choose a reason for hiding this comment

tilgovi commented Aug 17, 2020

bradzacher commented Aug 17, 2020

tilgovi commented Aug 17, 2020

feat(eslint-plugin): add `consistent-type-imports` rule #2367

feat(eslint-plugin): add `consistent-type-imports` rule #2367

codecov bot commented Aug 7, 2020 •

edited

bradzacher Aug 9, 2020 •

edited

ota-meshi commented Aug 11, 2020 •

edited