Skip to content

Commit

Permalink
fix(linter): import/no-cycle ignore type-only imports (#2924)
Browse files Browse the repository at this point in the history
A fix for the work done in: #2905

The existing code checks if _all_ import entries in a ModuleRecord are
type-only, when determining if the ModuleRecord should be skipped when
checking for cycles. This is incorrect.

To demonstrate the problem: Running the `no-cycles` rule, with
`ignoreTypes` enabled, on the following example code will cause a cycle
to be reported between Module A and Module B

```typescript
// Module A
import type { Bar } from './b';
import { Baz } from './c'

// Module B
import type { Foo } from './a';

// Module C
export const Baz = 'baz';
```

The reason this is happening is because the import to Module C in Module
A is causing the `was_imported_as_type` variable to evaluate to `false`,
since the Module C import is not type-only.

What we actually want to do is skip visiting Module B entirely, by
checking if its import request from Module A is type-only.

This PR fixes the logic so that only the import entries for the next
ModuleRecord are considered when determining `was_imported_as_type`.
  • Loading branch information
JohnDaly committed Apr 9, 2024
1 parent 990eda6 commit 5abbb0c
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 4 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// @ts-ignore
import { foo } from './ts-value';

// @ts-ignore
import type { foo as FooType } from '../depth-zero';
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export const foo = 'foo';
17 changes: 13 additions & 4 deletions crates/oxc_linter/src/rules/import/no_cycle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,10 +147,15 @@ impl NoCycle {

for module_record_ref in &module_record.loaded_modules {
let resolved_absolute_path = &module_record_ref.resolved_absolute_path;
let was_imported_as_type =
&module_record_ref.import_entries.iter().all(|entry| entry.is_type);
if self.ignore_types && *was_imported_as_type {
continue;
if self.ignore_types {
let was_imported_as_type = &module_record
.import_entries
.iter()
.filter(|entry| entry.module_request.name() == module_record_ref.key())
.all(|entry| entry.is_type);
if *was_imported_as_type {
continue;
}
}
if !state.traversed.insert(resolved_absolute_path.clone()) {
continue;
Expand Down Expand Up @@ -229,6 +234,10 @@ fn test() {
r#"import { foo } from "./typescript/ts-types-depth-two";"#,
Some(json!([{"ignoreTypes":true}])),
),
(
r#"import { foo } from "./typescript/ts-depth-type-and-value-imports";"#,
Some(json!([{"ignoreTypes":true}])),
),
// Flow not supported
// (r#"import { bar } from "./flow-types""#, None),
// (r#"import { bar } from "./flow-types-only-importing-type""#, None),
Expand Down

0 comments on commit 5abbb0c

Please sign in to comment.