Skip to content

Commit

Permalink
fix(linter/import): import/no-duplicates handles namespace imports …
Browse files Browse the repository at this point in the history 
…correctly (#6694)

Closes #6660

- Value (i.e. not `import type`) namespace imports are now grouped correctly
- When grouping, consider _all_ import entries, not just the first one. Fixes a false negative when inlined `type` imports are used, e.g. `import { a, type b } from 'foo'`
  • Loading branch information
@DonIsaac
DonIsaac committed Oct 20, 2024
1 parent 23f88b3 commit a5de230
Show file tree
Hide file tree
Showing 2 changed files with 57 additions and 33 deletions.
70 changes: 43 additions & 27 deletions crates/oxc_linter/src/rules/import/no_duplicates.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use oxc_diagnostics::{LabeledSpan, OxcDiagnostic};
use oxc_macros::declare_oxc_lint;
use oxc_span::Span;
use oxc_syntax::module_record::{ImportImportName, RequestedModule};
use rustc_hash::FxHashMap;

use crate::{context::LintContext, rule::Rule};

Expand Down Expand Up @@ -55,11 +56,20 @@ declare_oxc_lint!(
/// ```javascript
/// import { foo } from './module';
/// import { bar } from './module';
///
/// import a from './module';
/// import { b } from './module';
/// ```
///
/// Examples of **correct** code for this rule:
/// ```javascript
/// ```typescript
/// import { foo, bar } from './module';
///
/// import * as a from 'foo'; // separate statements for namespace imports
/// import { b } from 'foo';
///
/// import { c } from 'foo'; // separate type imports, unless
/// import type { d } from 'foo'; // `preferInline` is true
/// ```
NoDuplicates,
suspicious
Expand Down Expand Up @@ -91,33 +101,39 @@ impl Rule for NoDuplicates {
.chunk_by(|r| r.0.clone());

for (_path, group) in &groups {
let has_type_import = module_record.import_entries.iter().any(|entry| entry.is_type);
// When prefer_inline is false, 0 is value, 1 is type named, 2 is type namespace and 3 is type default
// When prefer_inline is true, 0 is value and type named, 2 is type namespace and 3 is type default
let import_entries_maps = group
let requested_modules = group
.into_iter()
.flat_map(|(_path, requested_modules)| requested_modules)
.filter(|requested_module| requested_module.is_import())
.into_group_map_by(|requested_module| {
// We should early return if there is no type import
if !has_type_import {
return 0;
};
for entry in &module_record.import_entries {
if entry.module_request.span() != requested_module.span() {
continue;
.filter(|requested_module| requested_module.is_import());
// When prefer_inline is false, 0 is value, 1 is type named, 2 is type namespace and 3 is type default
// When prefer_inline is true, 0 is value and type named, 2 is type // namespace and 3 is type default
let mut import_entries_maps: FxHashMap<i8, Vec<&RequestedModule>> =
FxHashMap::default();
for requested_module in requested_modules {
let imports = module_record
.import_entries
.iter()
.filter(|entry| entry.module_request.span() == requested_module.span())
.collect::<Vec<_>>();
if imports.is_empty() {
import_entries_maps.entry(0).or_default().push(requested_module);
}
for imports in imports {
let key = if imports.is_type {
match imports.import_name {
ImportImportName::Name(_) => i8::from(!self.prefer_inline),
ImportImportName::NamespaceObject => 2,
ImportImportName::Default(_) => 3,
}

if entry.is_type {
return match entry.import_name {
ImportImportName::Name(_) => i8::from(!self.prefer_inline),
ImportImportName::NamespaceObject => 2,
ImportImportName::Default(_) => 3,
};
} else {
match imports.import_name {
ImportImportName::NamespaceObject => 2,
_ => 0,
}
}
0
});
};
import_entries_maps.entry(key).or_default().push(requested_module);
}
}

for i in 0..4 {
check_duplicates(ctx, import_entries_maps.get(&i));
Expand Down Expand Up @@ -147,13 +163,13 @@ fn test() {
(r#"import "./malformed.js""#, None),
(r"import { x } from './foo'; import { y } from './bar'", None),
(r#"import foo from "234artaf"; import { shoop } from "234q25ad""#, None),
// r#"import { x } from './foo'; import type { y } from './foo'"#,
(r"import { x } from './foo'; import type { y } from './foo'", None),
// TODO: considerQueryString
// r#"import x from './bar?optionX'; import y from './bar?optionY';"#,
(r"import x from './foo'; import y from './bar';", None),
// TODO: separate namespace
// (r"import * as ns from './foo'; import { y } from './foo'", None),
// (r"import { y } from './foo'; import * as ns from './foo'", None),
(r"import * as ns from './foo'; import { y } from './foo'", None),
(r"import { y } from './foo'; import * as ns from './foo'", None),
// TypeScript
(r"import type { x } from './foo'; import y from './foo'", None),
(r"import type x from './foo'; import type y from './bar'", None),
Expand Down
20 changes: 14 additions & 6 deletions crates/oxc_linter/src/snapshots/no_duplicates.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ source: crates/oxc_linter/src/tester.rs
⚠ eslint-plugin-import(no-duplicates): Module './foo' is imported more than once in this file
╭─[index.ts:1:8]
1 │ import './foo'; import def, {x} from './foo'
· ───┬─── ───────
· ───┬─── ────────
· ╰── It is first imported here
╰────
help: Merge these imports into a single import statement
Expand Down Expand Up @@ -249,7 +249,7 @@ source: crates/oxc_linter/src/tester.rs
⚠ eslint-plugin-import(no-duplicates): Module './foo' is imported more than once in this file
╭─[index.ts:1:17]
1 │ import {x} from './foo'; import def, {y} from './foo'
· ───┬─── ───────
· ───┬─── ────────
· ╰── It is first imported here
╰────
help: Merge these imports into a single import statement
Expand All @@ -263,17 +263,17 @@ source: crates/oxc_linter/src/tester.rs
help: Merge these imports into a single import statement

⚠ eslint-plugin-import(no-duplicates): Module './foo' is imported more than once in this file
╭─[index.ts:1:21]
╭─[index.ts:1:46]
1 │ import * as ns from './foo'; import {x} from './foo'; import {y} from './foo'
· ───┬─── ─────── ───────
· ╰── It is first imported here
· ───┬─── ───────
· ╰── It is first imported here
╰────
help: Merge these imports into a single import statement

⚠ eslint-plugin-import(no-duplicates): Module './foo' is imported more than once in this file
╭─[index.ts:1:17]
1 │ import {x} from './foo'; import * as ns from './foo'; import {y} from './foo'; import './foo'
· ───┬─── ─────── ─────── ───────
· ───┬─── ─────── ───────
· ╰── It is first imported here
╰────
help: Merge these imports into a single import statement
Expand Down Expand Up @@ -540,6 +540,14 @@ source: crates/oxc_linter/src/tester.rs
╰────
help: Merge these imports into a single import statement

⚠ eslint-plugin-import(no-duplicates): Module './foo' is imported more than once in this file
╭─[index.ts:1:38]
1 │ import {AValue, type x, BValue} from './foo'; import {type y} from './foo'
· ───┬────
· ╰── It is first imported here
╰────
help: Merge these imports into a single import statement

⚠ eslint-plugin-import(no-duplicates): Module './foo' is imported more than once in this file
╭─[index.ts:1:38]
1 │ import {AValue, type x, BValue} from './foo'; import {type y} from './foo'
Expand Down

0 comments on commit a5de230

Please sign in to comment.