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): [no-circular-imports] add new rule #8965

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 47 additions & 0 deletions packages/eslint-plugin/docs/rules/no-circular-import.mdx
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
---
description: 'Disallow the use of import module that result in circular imports.'
---

import Tabs from '@theme/Tabs';
import TabItem from '@theme/TabItem';

> 🛑 This file is source code, not the primary documentation location! 🛑
>
> See **https://typescript-eslint.io/rules/no-circular-import** for documentation.
This rule disallows the use of import module that result in circular imports except for the type-only imports.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add explanation about what benefit it has over import/no-cycle (i.e. uses the TS resolver and is faster)


## Examples

<Tabs>
<TabItem value="❌ Incorrect">

```ts skipValidation
// foo.ts
import { bar } from './bar';
export const foo = 1;

// bar.ts
import { foo } from './foo';
export const bar = 1;
```

</TabItem>
<TabItem value="✅ Correct">

```ts
// foo.ts
import type { bar } from './bar';
export type baz = number;

// bar.ts
import { type baz } from './baz';
export type bar = number;
```

</TabItem>
</Tabs>

## When Not To Use It

## Related To
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/configs/all.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export = {
'@typescript-eslint/no-base-to-string': 'error',
'@typescript-eslint/no-confusing-non-null-assertion': 'error',
'@typescript-eslint/no-confusing-void-expression': 'error',
'@typescript-eslint/no-circular-import': 'error',
'no-dupe-class-members': 'off',
'@typescript-eslint/no-dupe-class-members': 'error',
'@typescript-eslint/no-duplicate-enum-values': 'error',
Expand Down
1 change: 1 addition & 0 deletions packages/eslint-plugin/src/configs/disable-type-checked.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export = {
'@typescript-eslint/naming-convention': 'off',
'@typescript-eslint/no-array-delete': 'off',
'@typescript-eslint/no-base-to-string': 'off',
'@typescript-eslint/no-circular-import': 'off',
'@typescript-eslint/no-confusing-void-expression': 'off',
'@typescript-eslint/no-duplicate-type-constituents': 'off',
'@typescript-eslint/no-floating-promises': 'off',
Expand Down
2 changes: 2 additions & 0 deletions packages/eslint-plugin/src/rules/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import namingConvention from './naming-convention';
import noArrayConstructor from './no-array-constructor';
import noArrayDelete from './no-array-delete';
import noBaseToString from './no-base-to-string';
import noCircularImport from './no-circular-import';
import confusingNonNullAssertionLikeNotEqual from './no-confusing-non-null-assertion';
import noConfusingVoidExpression from './no-confusing-void-expression';
import noDupeClassMembers from './no-dupe-class-members';
Expand Down Expand Up @@ -184,6 +185,7 @@ export default {
'no-array-constructor': noArrayConstructor,
'no-array-delete': noArrayDelete,
'no-base-to-string': noBaseToString,
'no-circular-import': noCircularImport,
'no-confusing-non-null-assertion': confusingNonNullAssertionLikeNotEqual,
'no-confusing-void-expression': noConfusingVoidExpression,
'no-dupe-class-members': noDupeClassMembers,
Expand Down
181 changes: 181 additions & 0 deletions packages/eslint-plugin/src/rules/no-circular-import.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,181 @@
import * as ts from 'typescript';

import { createRule, getParserServices } from '../util';

type Options = [];
type MessageIds = 'noCircularImport';

interface Edge {
filename: string;
specifier: string;
}

class Graph {
private graph = new Map<
string,
{
filename: string;
specifier: string;
}[]
>();

addEdge(start: string, edge: Edge): void {
if (this.graph.has(start)) {
this.graph.get(start)?.push(edge);
} else {
this.graph.set(start, [edge]);
}
}

hasEdge(name: string): boolean {
return this.graph.has(name);
}

getEdges(name: string): Edge[] {
return this.graph.get(name) ?? [];
}
Comment on lines +30 to +36
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't understand this graph at a glance. I think it's supposed to be an adjancency list indexed by the file names, and the specifiers are extra data for each edge right? This should be added as comments.

These functions' names are not apparent. I think they should be called hasNode and getOutEdges.

}
// imports “a.ts” and is imported from “b.ts”, resulting in a circular reference.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Docs] Not really a place for this, since we have the full docs page.

Suggested change
// imports “a.ts” and is imported from “b.ts”, resulting in a circular reference.

export default createRule<Options, MessageIds>({
name: 'no-circular-import',
meta: {
docs: {
description:
'Disallow the use of import module that result in circular imports',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
'Disallow the use of import module that result in circular imports',
'Disallow a module from importing another module that transitively imports itself.',

import/no-cycle uses the description "Forbid a module from importing a module with a dependency path back to itself.". I think that's better.

requiresTypeChecking: true,
},
messages: {
noCircularImport: 'Circular import dcetected via {{paths}}.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
noCircularImport: 'Circular import dcetected via {{paths}}.',
noCircularImport: 'Circular import detected via {{paths}}.',

},
schema: [],
type: 'suggestion',
},
defaultOptions: [],
create(context) {
const services = getParserServices(context);
const graph = new Graph();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Performance] This is what we've been afraid of 😬. Creating a new graph on each file becomes expensive in larger projects. From #224 (comment):

We can rely on the fact that TS has already analysed every file and knows the dependency graph of the project.
So instead of manually building it - we can just ask TS for the information.

Having to manually recreate the graph on demand for each file is not what I was hoping we'd have to do here. I don't see any TypeScript APIs that can give the full dependency graph on-demand.

I tried out this rule locally with a basic config and it increased the lint time from ~35-36 seconds to ~39-40 seconds on my M1 Mac. So, not immediately terrible... but noticeable, and will get exponentially worse in larger projects.

Testing local eslint.config.mjs
import tseslint from 'typescript-eslint';
import deprecationPlugin from 'eslint-plugin-deprecation';
import tseslintInternalPlugin from '@typescript-eslint/eslint-plugin-internal';
import eslintCommentsPlugin from 'eslint-plugin-eslint-comments';
import eslintPluginPlugin from 'eslint-plugin-eslint-plugin';
import importPlugin from 'eslint-plugin-import';
import jestPlugin from 'eslint-plugin-jest';
import jsdocPlugin from 'eslint-plugin-jsdoc';
import jsxA11yPlugin from 'eslint-plugin-jsx-a11y';
import reactPlugin from 'eslint-plugin-react';
import reactHooksPlugin from 'eslint-plugin-react-hooks';
import simpleImportSortPlugin from 'eslint-plugin-simple-import-sort';
import unicornPlugin from 'eslint-plugin-unicorn';

export default tseslint.config(
  {
    ignores: [
      '**/__snapshots__/**',
      '**/.docusaurus/**',
      '**/.nx/**',
      '**/build/**',
      '**/coverage/**',
      '**/dist/**',
      '**/fixtures/**',
      '**/jest.config.js',
      '**/node_modules/**',
      'packages/rule-tester/tests/eslint-base',
      'packages/types/src/generated/**/*.ts',
      'packages/website/src/vendor',
    ],
  },
  ...tseslint.configs.recommended,
  {
    languageOptions: {
      parserOptions: {
        EXPERIMENTAL_useProjectService: {
          allowDefaultProjectForFiles: ['./*.js', './*/*.js', './*/*/*.js'],
          defaultProject: 'tsconfig.json',
        },
      },
    },
    plugins: {
      ['@typescript-eslint']: tseslint.plugin,
      ['@typescript-eslint/internal']: tseslintInternalPlugin,
      ['deprecation']: deprecationPlugin,
      ['eslint-comments']: eslintCommentsPlugin,
      ['eslint-plugin']: eslintPluginPlugin,
      ['import']: importPlugin,
      ['jest']: jestPlugin,
      ['jsdoc']: jsdocPlugin,
      ['jsx-a11y']: jsxA11yPlugin,
      ['react-hooks']: reactHooksPlugin,
      ['react']: reactPlugin,
      ['simple-import-sort']: simpleImportSortPlugin,
      ['unicorn']: unicornPlugin,
    },
    rules: {
      '@typescript-eslint/ban-ts-comment': 'off',
      '@typescript-eslint/ban-types': 'off',
      '@typescript-eslint/no-array-constructor': 'off',
      '@typescript-eslint/no-duplicate-enum-values': 'off',
      '@typescript-eslint/no-explicit-any': 'off',
      '@typescript-eslint/no-extra-non-null-assertion': 'off',
      '@typescript-eslint/no-loss-of-precision': 'off',
      '@typescript-eslint/no-misused-new': 'off',
      '@typescript-eslint/no-namespace': 'off',
      '@typescript-eslint/no-non-null-asserted-optional-chain': 'off',
      '@typescript-eslint/no-this-alias': 'off',
      '@typescript-eslint/no-unnecessary-type-constraint': 'off',
      '@typescript-eslint/no-unsafe-declaration-merging': 'off',
      '@typescript-eslint/no-unused-vars': 'off',
      '@typescript-eslint/no-var-requires': 'off',
      '@typescript-eslint/prefer-as-const': 'off',
      '@typescript-eslint/triple-slash-reference': 'off',
      'no-async-promise-executor': 'off',
      'no-case-declarations': 'off',
      'no-constant-condition': 'off',
      'no-empty': 'off',
      'no-fallthrough': 'off',
      'no-prototype-builtins': 'off',
      'no-undef': 'off',
      'no-unsafe-finally': 'off',

      '@typescript-eslint/no-circular-import': 'error',
    },
  },
);

I question whether this really makes sense to include in our plugin with this "create a graph for each file" architecture. Similar to how #371 was closed in favor of external tooling like Knip, maybe this just doesn't belong in a lint rule?


function resolveSpecifier(
containingFile: string,
specifier: string,
): ts.ResolvedModuleWithFailedLookupLocations {
return ts.resolveModuleName(
specifier,
containingFile,
services.program.getCompilerOptions(),
ts.sys,
);
}

function isTypeOnlyImport(node: ts.ImportDeclaration): boolean {
return (
node.importClause?.isTypeOnly ||
(!!node.importClause?.namedBindings &&
ts.isNamedImports(node.importClause.namedBindings) &&
node.importClause.namedBindings.elements.every(
elem => elem.isTypeOnly,
))
);
}

function addEdgesRecursively(
graph: Graph,
containingFile: string,
importDeclaration: ts.ImportDeclaration,
): void {
if (graph.hasEdge(containingFile)) {
return;
}

if (isTypeOnlyImport(importDeclaration)) {
return;
}

if (!ts.isStringLiteral(importDeclaration.moduleSpecifier)) {
return;
}

const specifier = importDeclaration.moduleSpecifier.text;

const resolved = resolveSpecifier(containingFile, specifier);

if (
!resolved.resolvedModule ||
resolved.resolvedModule.isExternalLibraryImport
) {
Comment on lines +102 to +105
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Style] Nit:

Suggested change
if (
!resolved.resolvedModule ||
resolved.resolvedModule.isExternalLibraryImport
) {
if (resolved.resolvedModule?.isExternalLibraryImport !== false) {

return;
}

const resolvedFile = resolved.resolvedModule.resolvedFileName;

graph.addEdge(containingFile, {
filename: resolvedFile,
specifier,
});

const sourceCode = services.program.getSourceFile(resolvedFile);
sourceCode?.statements.forEach(statement => {
if (ts.isImportDeclaration(statement)) {
addEdgesRecursively(graph, resolvedFile, statement);
}
});
}

function detectCycleWorker(
start: string,
graph: Graph,
filename: string,
visited: Set<string>,
paths: string[],
): string[] {
visited.add(filename);

for (const edge of graph.getEdges(filename)) {
if (visited.has(edge.filename)) {
if (edge.filename === start) {
return paths.concat(edge.specifier);
}
return [];
}
const detected = detectCycleWorker(
start,
graph,
edge.filename,
visited,
paths.concat(edge.specifier),
);
if (detected.length) {
return detected;
}
}
return [];
}

function detectCycle(graph: Graph, filename: string): string[] {
const visited = new Set<string>();
return detectCycleWorker(filename, graph, filename, visited, []);
}

return {
ImportDeclaration(node): void {
const tsNode = services.esTreeNodeToTSNodeMap.get(node);
const containingFile = tsNode.parent.getSourceFile().fileName;
addEdgesRecursively(graph, containingFile, tsNode);

const cycle = detectCycle(graph, containingFile);
if (cycle.length > 1) {
context.report({
messageId: 'noCircularImport',
node,
data: {
paths:
cycle.length === 2
? cycle[0]
: `${cycle[0]} ... ${cycle[cycle.length - 2]}`,
},
});
}
},
};
},
});

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import { entry } from './entry';
export const one = entry;
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import { two } from './depth-two';
export const three = two;
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import { one } from './depth-one';
export const two = one;
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const entry = 1;
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import { b } from './isolated-circular-b';
export const a = 1;
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import { a } from './isolated-circular-a';
export const b = 1;
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export default {};
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import type { entry } from './entry';
import { type entry as EntryType } from './entry';
export type TypeOnly = typeof entry | typeof EntryType;
3 changes: 2 additions & 1 deletion packages/eslint-plugin/tests/fixtures/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
"file.ts",
"consistent-type-exports.ts",
"mixed-enums-decl.ts",
"react.tsx"
"react.tsx",
"no-circular-import"
]
}