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

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

Merged
merged 2 commits into from Aug 16, 2020

Conversation

@ota-meshi
Copy link
Contributor

@ota-meshi ota-meshi commented Aug 7, 2020

This PR adds the consistent-type-imports rule.

Fixes #2200

@typescript-eslint
Copy link

@typescript-eslint typescript-eslint bot commented Aug 7, 2020

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.

@bradzacher bradzacher added this to the 4.0.0 milestone Aug 7, 2020
@ota-meshi ota-meshi force-pushed the ota-meshi:consistent-type-import branch from 2eaf38b to 182bf9a Aug 7, 2020
@codecov
Copy link

@codecov codecov bot commented Aug 7, 2020

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%> (ø)
Copy link
Member

@bradzacher bradzacher left a comment

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


## When Not To Use It

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

This comment has been minimized.

@bradzacher

bradzacher Aug 9, 2020
Member

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.
```ts
import type { Foo } from './foo';
let foo: Foo;
```

```ts
import { Foo } from './foo';
let foo: Foo;
```

```ts
let foo: import('foo').Foo;
```
Comment on lines 7 to 19

This comment has been minimized.

@bradzacher

bradzacher Aug 9, 2020
Member

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

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

This comment has been minimized.

@bradzacher

bradzacher Aug 9, 2020
Member

styling nitpick

Suggested change
'ImportDeclaration[importKind=type]'(
'ImportDeclaration[importKind = "type"]'(
const importToken = sourceCode.getFirstToken(node)!;
return fixer.removeRange([
importToken.range[1],
sourceCode.getTokenAfter(importToken)!.range[1],
Comment on lines 131 to 134

This comment has been minimized.

@bradzacher

bradzacher Aug 9, 2020
Member

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],
let bar: B;
`,
output: `
import type A, { B } from 'foo';

This comment has been minimized.

@bradzacher

bradzacher Aug 9, 2020
Member

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

This comment has been minimized.

@ota-meshi

ota-meshi Aug 10, 2020
Author Contributor

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

? {
// 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');
},
});
}
},
}
Comment on lines 75 to 215

This comment has been minimized.

@bradzacher

bradzacher Aug 9, 2020
Member

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)' })
    }
  },
}
},
});

ruleTester.run('consistent-type-imports', rule, {

This comment has been minimized.

@bradzacher

bradzacher Aug 9, 2020
Member

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

@bradzacher bradzacher removed this from the 4.0.0 milestone Aug 9, 2020
@bradzacher bradzacher force-pushed the typescript-eslint:v4 branch from 5ab473c to e925538 Aug 9, 2020
@ota-meshi ota-meshi force-pushed the ota-meshi:consistent-type-import branch from c0e572a to d4215cb Aug 10, 2020
@ota-meshi
Copy link
Contributor Author

@ota-meshi ota-meshi commented Aug 10, 2020

@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.

@bradzacher bradzacher force-pushed the typescript-eslint:v4 branch from cf29440 to 12b9c8a Aug 10, 2020
@ota-meshi
Copy link
Contributor Author

@ota-meshi ota-meshi commented Aug 11, 2020

@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.

@ota-meshi ota-meshi force-pushed the ota-meshi:consistent-type-import branch from d4215cb to bb913f0 Aug 11, 2020
Copy link
Member

@bradzacher bradzacher left a comment

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

@bradzacher bradzacher merged commit ba9295b into typescript-eslint:v4 Aug 16, 2020
11 checks passed
11 checks passed
Typecheck
Details
Unit tests Unit tests
Details
Code style and lint
Details
Run integration tests on primary Node.js version
Details
Run unit tests on other Node.js versions (10.x)
Details
Run unit tests on other Node.js versions (14.x)
Details
Publish the latest code as a canary version
Details
Publish the latest code as a v4 prerelease version
Details
Semantic Pull Request ready to be squashed
Details
codecov/patch 96.91% of diff hit (target 90.00%)
Details
codecov/project 92.93% (+0.07%) compared to 12b9c8a
Details
@bradzacher bradzacher linked an issue that may be closed by this pull request Aug 16, 2020
@ota-meshi ota-meshi deleted the ota-meshi:consistent-type-import branch Aug 16, 2020
@tilgovi
Copy link

@tilgovi tilgovi commented Aug 17, 2020

Has adding this to the recommended config been discussed?

@bradzacher
Copy link
Member

@bradzacher bradzacher commented Aug 17, 2020

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
Copy link

@tilgovi tilgovi commented Aug 17, 2020

Thank you. That makes sense.

bradzacher added a commit that referenced this pull request Aug 19, 2020
bradzacher added a commit that referenced this pull request Aug 29, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants