From e58cfbbcef056da071695c498d7642aad499d386 Mon Sep 17 00:00:00 2001 From: Guillaume Piedigrossi Date: Sat, 16 Nov 2024 15:25:34 +0100 Subject: [PATCH 1/8] feat(linter): add the eslint/no_duplicate_imports rule --- crates/oxc_linter/src/rules.rs | 2 + .../src/rules/eslint/no_duplicate_imports.rs | 264 ++++++++++++++++++ .../src/snapshots/no_duplicate_imports.snap | 44 +++ 3 files changed, 310 insertions(+) create mode 100644 crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs create mode 100644 crates/oxc_linter/src/snapshots/no_duplicate_imports.snap diff --git a/crates/oxc_linter/src/rules.rs b/crates/oxc_linter/src/rules.rs index 326cfb2577734..30470bb635a89 100644 --- a/crates/oxc_linter/src/rules.rs +++ b/crates/oxc_linter/src/rules.rs @@ -67,6 +67,7 @@ mod eslint { pub mod no_dupe_else_if; pub mod no_dupe_keys; pub mod no_duplicate_case; + pub mod no_duplicate_imports; pub mod no_else_return; pub mod no_empty; pub mod no_empty_character_class; @@ -531,6 +532,7 @@ oxc_macros::declare_all_lint_rules! { eslint::max_lines, eslint::max_params, eslint::no_object_constructor, + eslint::no_duplicate_imports, eslint::no_alert, eslint::no_array_constructor, eslint::no_async_promise_executor, diff --git a/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs b/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs new file mode 100644 index 0000000000000..64320840dadbb --- /dev/null +++ b/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs @@ -0,0 +1,264 @@ +use std::collections::HashMap; + +use oxc_ast::{ + ast::{ImportDeclaration, ImportDeclarationSpecifier}, + AstKind, +}; +use oxc_diagnostics::OxcDiagnostic; +use oxc_macros::declare_oxc_lint; +use oxc_span::Span; + +use crate::{context::LintContext, rule::Rule}; + +fn no_duplicate_imports_diagnostic(module_name: &str, span: Span) -> OxcDiagnostic { + OxcDiagnostic::warn(format!("'{}' import is duplicated", module_name)) + .with_help("Merge the duplicated import into a single import statement") + .with_label(span) +} + +#[derive(Debug, Default, Clone)] +pub struct NoDuplicateImports {} + +declare_oxc_lint!( + /// ### What it does + /// Disallow duplicate module imports + /// + /// ### Why is this bad? + /// Using a single import statement per module will make the code clearer because you can see everything being imported from that module on one line. + /// + /// ### Examples + /// + /// Examples of **incorrect** code for this rule: + /// ```js + /// import { merge } from 'module'; + /// import something from 'another-module'; + /// import { find } from 'module'; + /// ``` + /// + /// Examples of **correct** code for this rule: + /// ```js + /// import { merge, find } from 'module'; + /// import something from 'another-module'; + /// ``` + NoDuplicateImports, + style, + pending); + +#[derive(Debug, Clone)] +enum DeclarationType { + Import, +} + +#[derive(Debug, Clone)] +struct ModuleEntry { + declaration_type: DeclarationType, +} + +impl Rule for NoDuplicateImports { + fn run_once(&self, ctx: &LintContext) { + let semantic = ctx.semantic(); + let nodes = semantic.nodes(); + + let mut modules: HashMap> = HashMap::new(); + + for node in nodes { + match node.kind() { + AstKind::ImportDeclaration(import_decl) => { + handle_import(import_decl, &mut modules, ctx); + } + _ => {} + } + } + } +} + +fn handle_import( + import_decl: &ImportDeclaration, + modules: &mut HashMap>, + ctx: &LintContext, +) { + let source = &import_decl.source; + let module_name = source.value.to_string(); + if let Some(specifiers) = &import_decl.specifiers { + let has_namespace = specifiers.iter().any(|s| match s { + ImportDeclarationSpecifier::ImportDefaultSpecifier(_) => false, + ImportDeclarationSpecifier::ImportNamespaceSpecifier(_) => true, + _ => false, + }); + if has_namespace { + return; + } + } + if let Some(existing_modules) = modules.get(&module_name) { + if existing_modules + .iter() + .any(|entry| matches!(entry.declaration_type, DeclarationType::Import)) + { + ctx.diagnostic(no_duplicate_imports_diagnostic(&module_name, import_decl.span)); + return; + } + } + + let entry = ModuleEntry { declaration_type: DeclarationType::Import }; + modules.entry(module_name.clone()).or_default().push(entry); +} + +#[test] +fn test() { + use crate::tester::Tester; + + let pass = vec![ + ( + r#"import os from "os"; + import fs from "fs";"#, + None, + ), + (r#"import { merge } from "lodash-es";"#, None), + (r#"import _, { merge } from "lodash-es";"#, None), + (r#"import * as Foobar from "async";"#, None), + (r#"import "foo""#, None), + ( + r#"import os from "os"; + export { something } from "os";"#, + None, + ), + ( + r#"import * as bar from "os"; + import { baz } from "os";"#, + None, + ), + ( + r#"import foo, * as bar from "os"; + import { baz } from "os";"#, + None, + ), + ( + r#"import foo, { bar } from "os"; + import * as baz from "os";"#, + None, + ), + ( + r#"import os from "os"; + export { hello } from "hello";"#, + Some(serde_json::json!([{ "includeExports": true }])), + ), + ( + r#"import os from "os"; + export * from "hello";"#, + Some(serde_json::json!([{ "includeExports": true }])), + ), + ( + r#"import os from "os"; + export { hello as hi } from "hello";"#, + Some(serde_json::json!([{ "includeExports": true }])), + ), + ( + r#"import os from "os"; + export default function(){};"#, + Some(serde_json::json!([{ "includeExports": true }])), + ), + ( + r#"import { merge } from "lodash-es"; + export { merge as lodashMerge }"#, + Some(serde_json::json!([{ "includeExports": true }])), + ), + ( + r#"export { something } from "os"; + export * as os from "os";"#, + Some(serde_json::json!([{ "includeExports": true }])), + ), + ( + r#"import { something } from "os"; + export * as os from "os";"#, + Some(serde_json::json!([{ "includeExports": true }])), + ), + ( + r#"import * as os from "os"; + export { something } from "os";"#, + Some(serde_json::json!([{ "includeExports": true }])), + ), + ( + r#"import os from "os"; + export * from "os";"#, + Some(serde_json::json!([{ "includeExports": true }])), + ), + ( + r#"export { something } from "os"; + export * from "os";"#, + Some(serde_json::json!([{ "includeExports": true }])), + ), + ]; + + let fail = vec![ + ( + r#"import "fs"; + import "fs""#, + None, + ), + ( + r#"import { merge } from "lodash-es"; + import { find } from "lodash-es";"#, + None, + ), + ( + r#"import { merge } from "lodash-es"; + import _ from "lodash-es";"#, + None, + ), + ( + r#"import os from "os"; + import { something } from "os"; + import * as foobar from "os";"#, + None, + ), + ( + r#"import * as modns from "lodash-es"; + import { merge } from "lodash-es"; + import { baz } from "lodash-es";"#, + None, + ), + // ( + // r#"export { os } from "os"; + // export { something } from "os";"#, + // Some(serde_json::json!([{ "includeExports": true }])), + // ), + // ( + // r#"import os from "os"; + // export { os as foobar } from "os"; + // export { something } from "os";"#, + // Some(serde_json::json!([{ "includeExports": true }])), + // ), + // ( + // r#"import os from "os"; + // export { something } from "os";"#, + // Some(serde_json::json!([{ "includeExports": true }])), + // ), + // ( + // r#"import os from "os"; + // export * as os from "os";"#, + // Some(serde_json::json!([{ "includeExports": true }])), + // ), + // ( + // r#"export * as os from "os"; + // import os from "os";"#, + // Some(serde_json::json!([{ "includeExports": true }])), + // ), + // ( + // r#"import * as modns from "mod"; + // export * as modns from "mod";"#, + // Some(serde_json::json!([{ "includeExports": true }])), + // ), + // ( + // r#"export * from "os"; + // export * from "os";"#, + // Some(serde_json::json!([{ "includeExports": true }])), + // ), + // ( + // r#"import "os"; + // export * from "os";"#, + // Some(serde_json::json!([{ "includeExports": true }])), + // ), + ]; + + Tester::new(NoDuplicateImports::NAME, pass, fail).test_and_snapshot(); +} diff --git a/crates/oxc_linter/src/snapshots/no_duplicate_imports.snap b/crates/oxc_linter/src/snapshots/no_duplicate_imports.snap new file mode 100644 index 0000000000000..6885974c83c47 --- /dev/null +++ b/crates/oxc_linter/src/snapshots/no_duplicate_imports.snap @@ -0,0 +1,44 @@ +--- +source: crates/oxc_linter/src/tester.rs +snapshot_kind: text +--- + ⚠ eslint(no-duplicate-imports): 'fs' import is duplicated + ╭─[no_duplicate_imports.tsx:2:7] + 1 │ import "fs"; + 2 │ import "fs" + · ─────────── + ╰──── + help: Merge the duplicated import into a single import statement + + ⚠ eslint(no-duplicate-imports): 'lodash-es' import is duplicated + ╭─[no_duplicate_imports.tsx:2:7] + 1 │ import { merge } from "lodash-es"; + 2 │ import { find } from "lodash-es"; + · ───────────────────────────────── + ╰──── + help: Merge the duplicated import into a single import statement + + ⚠ eslint(no-duplicate-imports): 'lodash-es' import is duplicated + ╭─[no_duplicate_imports.tsx:2:9] + 1 │ import { merge } from "lodash-es"; + 2 │ import _ from "lodash-es"; + · ────────────────────────── + ╰──── + help: Merge the duplicated import into a single import statement + + ⚠ eslint(no-duplicate-imports): 'os' import is duplicated + ╭─[no_duplicate_imports.tsx:2:9] + 1 │ import os from "os"; + 2 │ import { something } from "os"; + · ─────────────────────────────── + 3 │ import * as foobar from "os"; + ╰──── + help: Merge the duplicated import into a single import statement + + ⚠ eslint(no-duplicate-imports): 'lodash-es' import is duplicated + ╭─[no_duplicate_imports.tsx:3:9] + 2 │ import { merge } from "lodash-es"; + 3 │ import { baz } from "lodash-es"; + · ──────────────────────────────── + ╰──── + help: Merge the duplicated import into a single import statement From 66fbe8e23280a9ed69c5477c250d062ef872932e Mon Sep 17 00:00:00 2001 From: Guillaume Piedigrossi Date: Sat, 16 Nov 2024 16:19:47 +0100 Subject: [PATCH 2/8] feat(linter): add a config option to the no_duplicate_imports rule --- .../src/rules/eslint/no_duplicate_imports.rs | 232 ++++++++++++++---- .../src/snapshots/no_duplicate_imports.snap | 99 ++++++-- 2 files changed, 261 insertions(+), 70 deletions(-) diff --git a/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs b/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs index 64320840dadbb..c32d706170d9a 100644 --- a/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs +++ b/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs @@ -1,7 +1,9 @@ use std::collections::HashMap; use oxc_ast::{ - ast::{ImportDeclaration, ImportDeclarationSpecifier}, + ast::{ + ExportAllDeclaration, ExportNamedDeclaration, ImportDeclaration, ImportDeclarationSpecifier, + }, AstKind, }; use oxc_diagnostics::OxcDiagnostic; @@ -16,8 +18,16 @@ fn no_duplicate_imports_diagnostic(module_name: &str, span: Span) -> OxcDiagnost .with_label(span) } +fn no_duplicate_exports_diagnostic(module_name: &str, span: Span) -> OxcDiagnostic { + OxcDiagnostic::warn(format!("'{}' export is duplicated", module_name)) + .with_help("Merge the duplicated exports into a single export statement") + .with_label(span) +} + #[derive(Debug, Default, Clone)] -pub struct NoDuplicateImports {} +pub struct NoDuplicateImports { + include_exports: bool, +} declare_oxc_lint!( /// ### What it does @@ -41,20 +51,40 @@ declare_oxc_lint!( /// import something from 'another-module'; /// ``` NoDuplicateImports, - style, + nursery, pending); #[derive(Debug, Clone)] enum DeclarationType { Import, + Export, +} + +#[derive(Debug, Clone)] +enum Specifier { + Named, + Default, + Namespace, + All, } #[derive(Debug, Clone)] struct ModuleEntry { + specifier: Specifier, declaration_type: DeclarationType, } impl Rule for NoDuplicateImports { + fn from_configuration(value: serde_json::Value) -> Self { + let Some(value) = value.get(0) else { return Self { include_exports: false } }; + Self { + include_exports: value + .get("includeExports") + .and_then(serde_json::Value::as_bool) + .unwrap_or(false), + } + } + fn run_once(&self, ctx: &LintContext) { let semantic = ctx.semantic(); let nodes = semantic.nodes(); @@ -66,6 +96,12 @@ impl Rule for NoDuplicateImports { AstKind::ImportDeclaration(import_decl) => { handle_import(import_decl, &mut modules, ctx); } + AstKind::ExportNamedDeclaration(export_decl) if self.include_exports => { + handle_export(export_decl, &mut modules, ctx); + } + AstKind::ExportAllDeclaration(export_decl) if self.include_exports => { + handle_export_all(export_decl, &mut modules, ctx); + } _ => {} } } @@ -79,30 +115,120 @@ fn handle_import( ) { let source = &import_decl.source; let module_name = source.value.to_string(); + let mut specifier = Specifier::All; + if let Some(specifiers) = &import_decl.specifiers { let has_namespace = specifiers.iter().any(|s| match s { ImportDeclarationSpecifier::ImportDefaultSpecifier(_) => false, ImportDeclarationSpecifier::ImportNamespaceSpecifier(_) => true, _ => false, }); + + specifier = if specifiers.iter().any(|s| match s { + ImportDeclarationSpecifier::ImportDefaultSpecifier(_) => true, + _ => false, + }) { + Specifier::Default + } else if specifiers.iter().any(|s| match s { + ImportDeclarationSpecifier::ImportNamespaceSpecifier(_) => true, + _ => false, + }) { + Specifier::Namespace + } else { + Specifier::Named + }; + if has_namespace { return; } } + if let Some(existing_modules) = modules.get(&module_name) { - if existing_modules - .iter() - .any(|entry| matches!(entry.declaration_type, DeclarationType::Import)) - { + if existing_modules.iter().any(|entry| { + matches!(entry.declaration_type, DeclarationType::Import) + || matches!( + (entry.declaration_type.clone(), entry.specifier.clone()), + (DeclarationType::Export, Specifier::All) + ) + }) { ctx.diagnostic(no_duplicate_imports_diagnostic(&module_name, import_decl.span)); return; } } - let entry = ModuleEntry { declaration_type: DeclarationType::Import }; + let entry = ModuleEntry { declaration_type: DeclarationType::Import, specifier }; modules.entry(module_name.clone()).or_default().push(entry); } +fn handle_export( + export_decl: &ExportNamedDeclaration, + modules: &mut HashMap>, + ctx: &LintContext, +) { + let source = match &export_decl.source { + Some(source) => source, + None => return, + }; + let module_name = source.value.to_string(); + + if let Some(existing_modules) = modules.get(&module_name) { + if existing_modules.iter().any(|entry| { + matches!(entry.declaration_type, DeclarationType::Export) + || matches!(entry.declaration_type, DeclarationType::Import) + }) { + ctx.diagnostic(no_duplicate_exports_diagnostic(&module_name, export_decl.span)); + } + } + + modules.entry(module_name).or_default().push(ModuleEntry { + declaration_type: DeclarationType::Export, + specifier: Specifier::Named, + }); +} + +fn handle_export_all( + export_decl: &ExportAllDeclaration, + modules: &mut HashMap>, + ctx: &LintContext, +) { + let source = &export_decl.source; + let module_name = source.value.to_string(); + + let exported_name = export_decl.exported.clone(); + + if let Some(existing_modules) = modules.get(&module_name) { + if existing_modules.iter().any(|entry| { + matches!( + (&entry.declaration_type, &entry.specifier), + (DeclarationType::Import, Specifier::All) + ) || matches!( + (&entry.declaration_type, &entry.specifier), + (DeclarationType::Export, Specifier::All) + ) + }) { + ctx.diagnostic(no_duplicate_exports_diagnostic(&module_name, export_decl.span)); + } + + if exported_name.is_none() { + return; + } + + if existing_modules.iter().any(|entry| { + matches!( + (&entry.declaration_type, &entry.specifier), + (DeclarationType::Import, Specifier::Default) + ) + }) { + ctx.diagnostic(no_duplicate_exports_diagnostic(&module_name, export_decl.span)); + } + } + + modules + .entry(module_name) + .or_default() + .push(ModuleEntry { declaration_type: DeclarationType::Export, specifier: Specifier::All }); +} + #[test] fn test() { use crate::tester::Tester; @@ -192,72 +318,72 @@ fn test() { let fail = vec![ ( r#"import "fs"; - import "fs""#, + import "fs""#, None, ), ( r#"import { merge } from "lodash-es"; - import { find } from "lodash-es";"#, + import { find } from "lodash-es";"#, None, ), ( r#"import { merge } from "lodash-es"; - import _ from "lodash-es";"#, + import _ from "lodash-es";"#, None, ), ( r#"import os from "os"; - import { something } from "os"; - import * as foobar from "os";"#, + import { something } from "os"; + import * as foobar from "os";"#, None, ), ( r#"import * as modns from "lodash-es"; - import { merge } from "lodash-es"; - import { baz } from "lodash-es";"#, + import { merge } from "lodash-es"; + import { baz } from "lodash-es";"#, None, ), + ( + r#"export { os } from "os"; + export { something } from "os";"#, + Some(serde_json::json!([{ "includeExports": true }])), + ), + ( + r#"import os from "os"; + export { os as foobar } from "os"; + export { something } from "os";"#, + Some(serde_json::json!([{ "includeExports": true }])), + ), + ( + r#"import os from "os"; + export { something } from "os";"#, + Some(serde_json::json!([{ "includeExports": true }])), + ), + ( + r#"import os from "os"; + export * as os from "os";"#, + Some(serde_json::json!([{ "includeExports": true }])), + ), + ( + r#"export * as os from "os"; + import os from "os";"#, + Some(serde_json::json!([{ "includeExports": true }])), + ), // ( - // r#"export { os } from "os"; - // export { something } from "os";"#, - // Some(serde_json::json!([{ "includeExports": true }])), - // ), - // ( - // r#"import os from "os"; - // export { os as foobar } from "os"; - // export { something } from "os";"#, + // r#"import * as modns from "mod"; + // export * as modns from "mod";"#, // Some(serde_json::json!([{ "includeExports": true }])), // ), - // ( - // r#"import os from "os"; - // export { something } from "os";"#, - // Some(serde_json::json!([{ "includeExports": true }])), - // ), - // ( - // r#"import os from "os"; - // export * as os from "os";"#, - // Some(serde_json::json!([{ "includeExports": true }])), - // ), - // ( - // r#"export * as os from "os"; - // import os from "os";"#, - // Some(serde_json::json!([{ "includeExports": true }])), - // ), - // ( - // r#"import * as modns from "mod"; - // export * as modns from "mod";"#, - // Some(serde_json::json!([{ "includeExports": true }])), - // ), - // ( - // r#"export * from "os"; - // export * from "os";"#, - // Some(serde_json::json!([{ "includeExports": true }])), - // ), - // ( - // r#"import "os"; - // export * from "os";"#, - // Some(serde_json::json!([{ "includeExports": true }])), - // ), + ( + r#"export * from "os"; + export * from "os";"#, + Some(serde_json::json!([{ "includeExports": true }])), + ), + ( + r#"import "os"; + export * from "os";"#, + Some(serde_json::json!([{ "includeExports": true }])), + ), ]; Tester::new(NoDuplicateImports::NAME, pass, fail).test_and_snapshot(); diff --git a/crates/oxc_linter/src/snapshots/no_duplicate_imports.snap b/crates/oxc_linter/src/snapshots/no_duplicate_imports.snap index 6885974c83c47..6eae291c8d116 100644 --- a/crates/oxc_linter/src/snapshots/no_duplicate_imports.snap +++ b/crates/oxc_linter/src/snapshots/no_duplicate_imports.snap @@ -3,42 +3,107 @@ source: crates/oxc_linter/src/tester.rs snapshot_kind: text --- ⚠ eslint(no-duplicate-imports): 'fs' import is duplicated - ╭─[no_duplicate_imports.tsx:2:7] + ╭─[no_duplicate_imports.tsx:2:9] 1 │ import "fs"; - 2 │ import "fs" - · ─────────── + 2 │ import "fs" + · ─────────── ╰──── help: Merge the duplicated import into a single import statement ⚠ eslint(no-duplicate-imports): 'lodash-es' import is duplicated - ╭─[no_duplicate_imports.tsx:2:7] + ╭─[no_duplicate_imports.tsx:2:9] 1 │ import { merge } from "lodash-es"; - 2 │ import { find } from "lodash-es"; - · ───────────────────────────────── + 2 │ import { find } from "lodash-es"; + · ───────────────────────────────── ╰──── help: Merge the duplicated import into a single import statement ⚠ eslint(no-duplicate-imports): 'lodash-es' import is duplicated - ╭─[no_duplicate_imports.tsx:2:9] + ╭─[no_duplicate_imports.tsx:2:11] 1 │ import { merge } from "lodash-es"; - 2 │ import _ from "lodash-es"; - · ────────────────────────── + 2 │ import _ from "lodash-es"; + · ────────────────────────── ╰──── help: Merge the duplicated import into a single import statement ⚠ eslint(no-duplicate-imports): 'os' import is duplicated - ╭─[no_duplicate_imports.tsx:2:9] + ╭─[no_duplicate_imports.tsx:2:11] 1 │ import os from "os"; - 2 │ import { something } from "os"; - · ─────────────────────────────── - 3 │ import * as foobar from "os"; + 2 │ import { something } from "os"; + · ─────────────────────────────── + 3 │ import * as foobar from "os"; ╰──── help: Merge the duplicated import into a single import statement ⚠ eslint(no-duplicate-imports): 'lodash-es' import is duplicated - ╭─[no_duplicate_imports.tsx:3:9] - 2 │ import { merge } from "lodash-es"; - 3 │ import { baz } from "lodash-es"; - · ──────────────────────────────── + ╭─[no_duplicate_imports.tsx:3:11] + 2 │ import { merge } from "lodash-es"; + 3 │ import { baz } from "lodash-es"; + · ──────────────────────────────── ╰──── help: Merge the duplicated import into a single import statement + + ⚠ eslint(no-duplicate-imports): 'os' export is duplicated + ╭─[no_duplicate_imports.tsx:2:11] + 1 │ export { os } from "os"; + 2 │ export { something } from "os"; + · ─────────────────────────────── + ╰──── + help: Merge the duplicated exports into a single export statement + + ⚠ eslint(no-duplicate-imports): 'os' export is duplicated + ╭─[no_duplicate_imports.tsx:2:11] + 1 │ import os from "os"; + 2 │ export { os as foobar } from "os"; + · ────────────────────────────────── + 3 │ export { something } from "os"; + ╰──── + help: Merge the duplicated exports into a single export statement + + ⚠ eslint(no-duplicate-imports): 'os' export is duplicated + ╭─[no_duplicate_imports.tsx:3:11] + 2 │ export { os as foobar } from "os"; + 3 │ export { something } from "os"; + · ─────────────────────────────── + ╰──── + help: Merge the duplicated exports into a single export statement + + ⚠ eslint(no-duplicate-imports): 'os' export is duplicated + ╭─[no_duplicate_imports.tsx:2:11] + 1 │ import os from "os"; + 2 │ export { something } from "os"; + · ─────────────────────────────── + ╰──── + help: Merge the duplicated exports into a single export statement + + ⚠ eslint(no-duplicate-imports): 'os' export is duplicated + ╭─[no_duplicate_imports.tsx:2:9] + 1 │ import os from "os"; + 2 │ export * as os from "os"; + · ───────────────────────── + ╰──── + help: Merge the duplicated exports into a single export statement + + ⚠ eslint(no-duplicate-imports): 'os' import is duplicated + ╭─[no_duplicate_imports.tsx:2:9] + 1 │ export * as os from "os"; + 2 │ import os from "os"; + · ──────────────────── + ╰──── + help: Merge the duplicated import into a single import statement + + ⚠ eslint(no-duplicate-imports): 'os' export is duplicated + ╭─[no_duplicate_imports.tsx:2:9] + 1 │ export * from "os"; + 2 │ export * from "os"; + · ─────────────────── + ╰──── + help: Merge the duplicated exports into a single export statement + + ⚠ eslint(no-duplicate-imports): 'os' export is duplicated + ╭─[no_duplicate_imports.tsx:2:9] + 1 │ import "os"; + 2 │ export * from "os"; + · ─────────────────── + ╰──── + help: Merge the duplicated exports into a single export statement From d26174b7bcbd9f75cb1d15e42ccc76965c86be51 Mon Sep 17 00:00:00 2001 From: Guillaume Piedigrossi Date: Sat, 23 Nov 2024 16:03:05 +0100 Subject: [PATCH 3/8] reactor(linter / no_duplicate_imports): use only module_record instead of iterating on all AST nodes --- .../src/rules/eslint/no_duplicate_imports.rs | 299 +++++++++--------- 1 file changed, 152 insertions(+), 147 deletions(-) diff --git a/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs b/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs index c32d706170d9a..a25649728b02f 100644 --- a/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs +++ b/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs @@ -1,14 +1,9 @@ use std::collections::HashMap; -use oxc_ast::{ - ast::{ - ExportAllDeclaration, ExportNamedDeclaration, ImportDeclaration, ImportDeclarationSpecifier, - }, - AstKind, -}; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; -use oxc_span::Span; +use oxc_span::{CompactStr, Span}; +use oxc_syntax::module_record::ImportImportName; use crate::{context::LintContext, rule::Rule}; @@ -54,24 +49,12 @@ declare_oxc_lint!( nursery, pending); -#[derive(Debug, Clone)] -enum DeclarationType { - Import, - Export, -} - -#[derive(Debug, Clone)] -enum Specifier { +#[derive(Debug, Clone, PartialEq)] +enum ImportType { Named, Default, Namespace, - All, -} - -#[derive(Debug, Clone)] -struct ModuleEntry { - specifier: Specifier, - declaration_type: DeclarationType, + SideEffect, } impl Rule for NoDuplicateImports { @@ -86,147 +69,169 @@ impl Rule for NoDuplicateImports { } fn run_once(&self, ctx: &LintContext) { - let semantic = ctx.semantic(); - let nodes = semantic.nodes(); - - let mut modules: HashMap> = HashMap::new(); - - for node in nodes { - match node.kind() { - AstKind::ImportDeclaration(import_decl) => { - handle_import(import_decl, &mut modules, ctx); - } - AstKind::ExportNamedDeclaration(export_decl) if self.include_exports => { - handle_export(export_decl, &mut modules, ctx); + let module_record = ctx.module_record(); + let mut import_map: HashMap<&CompactStr, Vec<(ImportType, Span, bool)>> = HashMap::new(); // Added bool for same_statement + let mut current_span: Option = None; + + println!("source_text: {:?}", ctx.source_text()); + // Handle bare imports first + if module_record.import_entries.is_empty() { + for (source, requests) in &module_record.requested_modules { + for request in requests { + if request.is_import() { + if let Some(existing) = import_map.get(source) { + // Bare imports can't be duplicated at all + if !existing.is_empty() { + ctx.diagnostic(no_duplicate_imports_diagnostic( + source, + request.span(), + )); + continue; + } + } + import_map.entry(source).or_default().push(( + ImportType::SideEffect, + request.span(), + false, + )); + } } - AstKind::ExportAllDeclaration(export_decl) if self.include_exports => { - handle_export_all(export_decl, &mut modules, ctx); - } - _ => {} } } - } -} - -fn handle_import( - import_decl: &ImportDeclaration, - modules: &mut HashMap>, - ctx: &LintContext, -) { - let source = &import_decl.source; - let module_name = source.value.to_string(); - let mut specifier = Specifier::All; - - if let Some(specifiers) = &import_decl.specifiers { - let has_namespace = specifiers.iter().any(|s| match s { - ImportDeclarationSpecifier::ImportDefaultSpecifier(_) => false, - ImportDeclarationSpecifier::ImportNamespaceSpecifier(_) => true, - _ => false, - }); + // Handle regular imports + for entry in &module_record.import_entries { + let source = entry.module_request.name(); + let span = entry.module_request.span(); + + let same_statement = if let Some(curr_span) = current_span { + curr_span == span + } else { + current_span = Some(span); + true + }; + + let import_type = match &entry.import_name { + ImportImportName::Name(_) => ImportType::Named, + ImportImportName::NamespaceObject => ImportType::Namespace, + ImportImportName::Default(_) => ImportType::Default, + }; + + println!("- source {source:?}, import_type {import_type:?}, same_statement: {same_statement}"); + if let Some(existing) = import_map.get(source) { + let can_merge = can_merge_imports(&import_type, existing, same_statement); + if can_merge { + ctx.diagnostic(no_duplicate_imports_diagnostic(source, span)); + continue; + } + } - specifier = if specifiers.iter().any(|s| match s { - ImportDeclarationSpecifier::ImportDefaultSpecifier(_) => true, - _ => false, - }) { - Specifier::Default - } else if specifiers.iter().any(|s| match s { - ImportDeclarationSpecifier::ImportNamespaceSpecifier(_) => true, - _ => false, - }) { - Specifier::Namespace - } else { - Specifier::Named - }; + import_map.entry(source).or_default().push((import_type, span, same_statement)); - if has_namespace { - return; + if !same_statement { + current_span = Some(span); + } } - } - if let Some(existing_modules) = modules.get(&module_name) { - if existing_modules.iter().any(|entry| { - matches!(entry.declaration_type, DeclarationType::Import) - || matches!( - (entry.declaration_type.clone(), entry.specifier.clone()), - (DeclarationType::Export, Specifier::All) - ) - }) { - ctx.diagnostic(no_duplicate_imports_diagnostic(&module_name, import_decl.span)); - return; - } - } - - let entry = ModuleEntry { declaration_type: DeclarationType::Import, specifier }; - modules.entry(module_name.clone()).or_default().push(entry); -} - -fn handle_export( - export_decl: &ExportNamedDeclaration, - modules: &mut HashMap>, - ctx: &LintContext, -) { - let source = match &export_decl.source { - Some(source) => source, - None => return, - }; - let module_name = source.value.to_string(); + // Handle exports if includeExports is true + if self.include_exports { + // Handle star exports + for entry in &module_record.star_export_entries { + println!("star_export_entry: {:?}", entry); + if let Some(module_request) = &entry.module_request { + let source = module_request.name(); + let span = entry.span; + + if let Some(existing) = import_map.get(source) { + if existing.iter().any(|(t, _, _)| { + matches!(t, ImportType::Named | ImportType::SideEffect) + }) { + ctx.diagnostic(no_duplicate_exports_diagnostic(source, span)); + continue; + } + } + + import_map.entry(source).or_default().push(( + ImportType::SideEffect, + span, + false, + )); + } + } - if let Some(existing_modules) = modules.get(&module_name) { - if existing_modules.iter().any(|entry| { - matches!(entry.declaration_type, DeclarationType::Export) - || matches!(entry.declaration_type, DeclarationType::Import) - }) { - ctx.diagnostic(no_duplicate_exports_diagnostic(&module_name, export_decl.span)); + // Handle indirect exports + for entry in &module_record.indirect_export_entries { + println!("indirect_export_entry: {:?}", entry); + + if let Some(module_request) = &entry.module_request { + let source = module_request.name(); + let span = entry.span; + + if !entry.local_name.is_null() { + if let Some(existing) = import_map.get(source) { + if existing.iter().any(|(t, _, _)| { + matches!(t, ImportType::Named | ImportType::SideEffect) + }) { + ctx.diagnostic(no_duplicate_exports_diagnostic(source, span)); + continue; + } + } + + import_map.entry(source).or_default().push(( + ImportType::Named, + span, + false, + )); + } + } + } } } - - modules.entry(module_name).or_default().push(ModuleEntry { - declaration_type: DeclarationType::Export, - specifier: Specifier::Named, - }); } -fn handle_export_all( - export_decl: &ExportAllDeclaration, - modules: &mut HashMap>, - ctx: &LintContext, -) { - let source = &export_decl.source; - let module_name = source.value.to_string(); - - let exported_name = export_decl.exported.clone(); - - if let Some(existing_modules) = modules.get(&module_name) { - if existing_modules.iter().any(|entry| { - matches!( - (&entry.declaration_type, &entry.specifier), - (DeclarationType::Import, Specifier::All) - ) || matches!( - (&entry.declaration_type, &entry.specifier), - (DeclarationType::Export, Specifier::All) - ) - }) { - ctx.diagnostic(no_duplicate_exports_diagnostic(&module_name, export_decl.span)); - } - - if exported_name.is_none() { - return; +fn can_merge_imports( + current_type: &ImportType, + existing: &[(ImportType, Span, bool)], + same_statement: bool, +) -> bool { + println!("existing: {existing:?}"); + for (existing_type, _, is_same_stmt) in existing { + // Allow multiple imports in the same statement + println!("same_statement: {same_statement}, is_same_stmt: {is_same_stmt}"); + if same_statement { + return false; } - if existing_modules.iter().any(|entry| { - matches!( - (&entry.declaration_type, &entry.specifier), - (DeclarationType::Import, Specifier::Default) - ) - }) { - ctx.diagnostic(no_duplicate_exports_diagnostic(&module_name, export_decl.span)); + println!("current_type: {:?}, existing_type: {:?}", current_type, existing_type); + match (current_type, existing_type) { + // Side effect imports can't be merged with anything + (ImportType::SideEffect, _) | (_, ImportType::SideEffect) => return false, + + // Namespace imports can't be merged with named imports + (ImportType::Namespace, ImportType::Named) + | (ImportType::Named, ImportType::Namespace) => return false, + + // Default imports can't be duplicated + (ImportType::Default, ImportType::Default) => return false, + + // Named imports from the same module can be merged unless there's a namespace import + (ImportType::Named, ImportType::Named) => { + if existing + .iter() + .any(|(t, _, same_stmt)| *t == ImportType::Namespace && *same_stmt) + { + return true; + } + } + (ImportType::Named, ImportType::Default) => { + if existing.iter().any(|(t, _, same_stmt)| *t == ImportType::Named && *same_stmt) { + return true; + } + } + // Other combinations are allowed + _ => continue, } } - - modules - .entry(module_name) - .or_default() - .push(ModuleEntry { declaration_type: DeclarationType::Export, specifier: Specifier::All }); + true } #[test] From a4c762e118d9a7af7f089195a41f13e203af69c8 Mon Sep 17 00:00:00 2001 From: Guillaume Piedigrossi Date: Sat, 23 Nov 2024 21:27:12 +0100 Subject: [PATCH 4/8] feat(linter / no_duplicate_imports): add rules for the includeExports option --- .../src/rules/eslint/no_duplicate_imports.rs | 205 ++++++++++++------ 1 file changed, 135 insertions(+), 70 deletions(-) diff --git a/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs b/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs index a25649728b02f..a08107b96bb27 100644 --- a/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs +++ b/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs @@ -3,7 +3,7 @@ use std::collections::HashMap; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; use oxc_span::{CompactStr, Span}; -use oxc_syntax::module_record::ImportImportName; +use oxc_syntax::module_record::{ExportImportName, ImportImportName}; use crate::{context::LintContext, rule::Rule}; @@ -55,6 +55,13 @@ enum ImportType { Default, Namespace, SideEffect, + AllButDefault, +} + +#[derive(Debug, Clone, PartialEq)] +enum ModuleType { + Import, + Export, } impl Rule for NoDuplicateImports { @@ -70,10 +77,12 @@ impl Rule for NoDuplicateImports { fn run_once(&self, ctx: &LintContext) { let module_record = ctx.module_record(); - let mut import_map: HashMap<&CompactStr, Vec<(ImportType, Span, bool)>> = HashMap::new(); // Added bool for same_statement + let mut import_map: HashMap<&CompactStr, Vec<(ImportType, Span, ModuleType)>> = + HashMap::new(); // Added bool for same_statement let mut current_span: Option = None; println!("source_text: {:?}", ctx.source_text()); + // Handle bare imports first if module_record.import_entries.is_empty() { for (source, requests) in &module_record.requested_modules { @@ -92,7 +101,7 @@ impl Rule for NoDuplicateImports { import_map.entry(source).or_default().push(( ImportType::SideEffect, request.span(), - false, + ModuleType::Import, )); } } @@ -125,7 +134,7 @@ impl Rule for NoDuplicateImports { } } - import_map.entry(source).or_default().push((import_type, span, same_statement)); + import_map.entry(source).or_default().push((import_type, span, ModuleType::Import)); if !same_statement { current_span = Some(span); @@ -141,6 +150,23 @@ impl Rule for NoDuplicateImports { let source = module_request.name(); let span = entry.span; + if entry.import_name.is_all_but_default() { + if let Some(existing) = import_map.get(source) { + if existing + .iter() + .any(|(t, _, _)| matches!(t, ImportType::AllButDefault)) + { + ctx.diagnostic(no_duplicate_exports_diagnostic(source, span)); + continue; + } + } + import_map.entry(source).or_default().push(( + ImportType::AllButDefault, + span, + ModuleType::Export, + )); + continue; + } if let Some(existing) = import_map.get(source) { if existing.iter().any(|(t, _, _)| { matches!(t, ImportType::Named | ImportType::SideEffect) @@ -153,35 +179,47 @@ impl Rule for NoDuplicateImports { import_map.entry(source).or_default().push(( ImportType::SideEffect, span, - false, + ModuleType::Export, )); } } // Handle indirect exports for entry in &module_record.indirect_export_entries { - println!("indirect_export_entry: {:?}", entry); - if let Some(module_request) = &entry.module_request { let source = module_request.name(); let span = entry.span; + println!("- source: {source:?}, span: {span:?}, type: indirect_export_entries"); - if !entry.local_name.is_null() { - if let Some(existing) = import_map.get(source) { + if let Some(existing) = import_map.get(source) { + if entry.import_name == ExportImportName::All { if existing.iter().any(|(t, _, _)| { - matches!(t, ImportType::Named | ImportType::SideEffect) + matches!(t, ImportType::Default | ImportType::Namespace) }) { ctx.diagnostic(no_duplicate_exports_diagnostic(source, span)); continue; } + + continue; } - import_map.entry(source).or_default().push(( - ImportType::Named, - span, - false, - )); + if existing.iter().any(|(t, _, module_type)| { + (matches!( + t, + ImportType::Named | ImportType::SideEffect | ImportType::Default + ) && *module_type == ModuleType::Export) + || (matches!(t, ImportType::Default) + && *module_type == ModuleType::Import) + }) { + ctx.diagnostic(no_duplicate_exports_diagnostic(source, span)); + continue; + } } + import_map.entry(source).or_default().push(( + ImportType::Named, + span, + ModuleType::Export, + )); } } } @@ -190,48 +228,75 @@ impl Rule for NoDuplicateImports { fn can_merge_imports( current_type: &ImportType, - existing: &[(ImportType, Span, bool)], + existing: &[(ImportType, Span, ModuleType)], same_statement: bool, ) -> bool { println!("existing: {existing:?}"); - for (existing_type, _, is_same_stmt) in existing { - // Allow multiple imports in the same statement - println!("same_statement: {same_statement}, is_same_stmt: {is_same_stmt}"); - if same_statement { - return false; + + // Allow multiple imports in the same statement + if same_statement { + return false; + } + + // this is the (namespace, span) if exsiting. None else + + let namespace = existing.iter().find(|(t, _, _)| matches!(t, ImportType::Namespace)); + let named = existing.iter().find(|(t, _, _)| matches!(t, ImportType::Named)); + let default = existing.iter().find(|(t, _, _)| matches!(t, ImportType::Default)); + + let has_namespace = namespace.is_some(); + let has_named = named.is_some(); + let has_default = default.is_some(); + + // For duplicate named imports + if matches!(current_type, ImportType::Named) { + if has_named { + return true; } + } - println!("current_type: {:?}, existing_type: {:?}", current_type, existing_type); - match (current_type, existing_type) { - // Side effect imports can't be merged with anything - (ImportType::SideEffect, _) | (_, ImportType::SideEffect) => return false, - - // Namespace imports can't be merged with named imports - (ImportType::Namespace, ImportType::Named) - | (ImportType::Named, ImportType::Namespace) => return false, - - // Default imports can't be duplicated - (ImportType::Default, ImportType::Default) => return false, - - // Named imports from the same module can be merged unless there's a namespace import - (ImportType::Named, ImportType::Named) => { - if existing - .iter() - .any(|(t, _, same_stmt)| *t == ImportType::Namespace && *same_stmt) - { - return true; - } - } - (ImportType::Named, ImportType::Default) => { - if existing.iter().any(|(t, _, same_stmt)| *t == ImportType::Named && *same_stmt) { - return true; + // For namespace imports + if matches!(current_type, ImportType::Namespace) { + if has_named && has_default { + if let Some((_, named_span, _)) = named { + if let Some((_, default_span, _)) = default { + if named_span == default_span { + return false; + } } } - // Other combinations are allowed - _ => continue, + } + + if has_namespace { + return true; + } + if has_default && !same_statement { + return true; + } + } + + // For default imports + if matches!(current_type, ImportType::Default) { + if has_default { + return true; + } + if has_named && !same_statement { + return true; + } + if has_namespace { + return true; } } - true + + // For side effect imports + if matches!(current_type, ImportType::SideEffect) { + // Any existing import means it's a duplicate + if !existing.is_empty() { + return true; + } + } + + false } #[test] @@ -250,72 +315,72 @@ fn test() { (r#"import "foo""#, None), ( r#"import os from "os"; - export { something } from "os";"#, + export { something } from "os";"#, None, ), ( r#"import * as bar from "os"; - import { baz } from "os";"#, + import { baz } from "os";"#, None, ), ( r#"import foo, * as bar from "os"; - import { baz } from "os";"#, + import { baz } from "os";"#, None, ), ( r#"import foo, { bar } from "os"; - import * as baz from "os";"#, + import * as baz from "os";"#, None, ), ( r#"import os from "os"; - export { hello } from "hello";"#, + export { hello } from "hello";"#, Some(serde_json::json!([{ "includeExports": true }])), ), ( r#"import os from "os"; - export * from "hello";"#, + export * from "hello";"#, Some(serde_json::json!([{ "includeExports": true }])), ), ( r#"import os from "os"; - export { hello as hi } from "hello";"#, + export { hello as hi } from "hello";"#, Some(serde_json::json!([{ "includeExports": true }])), ), ( r#"import os from "os"; - export default function(){};"#, + export default function(){};"#, Some(serde_json::json!([{ "includeExports": true }])), ), ( r#"import { merge } from "lodash-es"; - export { merge as lodashMerge }"#, + export { merge as lodashMerge }"#, Some(serde_json::json!([{ "includeExports": true }])), ), ( r#"export { something } from "os"; - export * as os from "os";"#, + export * as os from "os";"#, Some(serde_json::json!([{ "includeExports": true }])), ), ( r#"import { something } from "os"; - export * as os from "os";"#, + export * as os from "os";"#, Some(serde_json::json!([{ "includeExports": true }])), ), ( r#"import * as os from "os"; - export { something } from "os";"#, + export { something } from "os";"#, Some(serde_json::json!([{ "includeExports": true }])), ), ( r#"import os from "os"; - export * from "os";"#, + export * from "os";"#, Some(serde_json::json!([{ "includeExports": true }])), ), ( r#"export { something } from "os"; - export * from "os";"#, + export * from "os";"#, Some(serde_json::json!([{ "includeExports": true }])), ), ]; @@ -374,21 +439,21 @@ fn test() { import os from "os";"#, Some(serde_json::json!([{ "includeExports": true }])), ), - // ( - // r#"import * as modns from "mod"; - // export * as modns from "mod";"#, - // Some(serde_json::json!([{ "includeExports": true }])), - // ), ( - r#"export * from "os"; - export * from "os";"#, + r#"import * as modns from "mod"; + export * as modns from "mod";"#, Some(serde_json::json!([{ "includeExports": true }])), ), ( - r#"import "os"; + r#"export * from "os"; export * from "os";"#, Some(serde_json::json!([{ "includeExports": true }])), ), + // ( + // r#"import "os"; + // export * from "os";"#, + // Some(serde_json::json!([{ "includeExports": true }])), + // ), ]; Tester::new(NoDuplicateImports::NAME, pass, fail).test_and_snapshot(); From e8773ee6eadcb1342170e0f5690822671d49a134 Mon Sep 17 00:00:00 2001 From: Guillaume Piedigrossi Date: Sat, 23 Nov 2024 22:20:23 +0100 Subject: [PATCH 5/8] feat(linter / no_duplicate_imports): fix last test cases and clean code --- .../src/rules/eslint/no_duplicate_imports.rs | 78 +++++++------------ 1 file changed, 29 insertions(+), 49 deletions(-) diff --git a/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs b/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs index a08107b96bb27..90dfba0a8b2d7 100644 --- a/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs +++ b/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs @@ -78,36 +78,10 @@ impl Rule for NoDuplicateImports { fn run_once(&self, ctx: &LintContext) { let module_record = ctx.module_record(); let mut import_map: HashMap<&CompactStr, Vec<(ImportType, Span, ModuleType)>> = - HashMap::new(); // Added bool for same_statement + HashMap::new(); let mut current_span: Option = None; + let mut side_effect_import_map: HashMap<&CompactStr, Vec> = HashMap::new(); - println!("source_text: {:?}", ctx.source_text()); - - // Handle bare imports first - if module_record.import_entries.is_empty() { - for (source, requests) in &module_record.requested_modules { - for request in requests { - if request.is_import() { - if let Some(existing) = import_map.get(source) { - // Bare imports can't be duplicated at all - if !existing.is_empty() { - ctx.diagnostic(no_duplicate_imports_diagnostic( - source, - request.span(), - )); - continue; - } - } - import_map.entry(source).or_default().push(( - ImportType::SideEffect, - request.span(), - ModuleType::Import, - )); - } - } - } - } - // Handle regular imports for entry in &module_record.import_entries { let source = entry.module_request.name(); let span = entry.module_request.span(); @@ -125,7 +99,6 @@ impl Rule for NoDuplicateImports { ImportImportName::Default(_) => ImportType::Default, }; - println!("- source {source:?}, import_type {import_type:?}, same_statement: {same_statement}"); if let Some(existing) = import_map.get(source) { let can_merge = can_merge_imports(&import_type, existing, same_statement); if can_merge { @@ -141,11 +114,26 @@ impl Rule for NoDuplicateImports { } } - // Handle exports if includeExports is true + for (source, requests) in &module_record.requested_modules { + for request in requests { + if request.is_import() { + if module_record.import_entries.is_empty() { + side_effect_import_map.entry(source).or_default().push(request.span()); + } + } + } + } + + side_effect_import_map.iter().for_each(|(source, spans)| { + if spans.len() > 1 { + spans.iter().for_each(|span| { + ctx.diagnostic(no_duplicate_imports_diagnostic(source, *span)); + }); + } + }); + if self.include_exports { - // Handle star exports for entry in &module_record.star_export_entries { - println!("star_export_entry: {:?}", entry); if let Some(module_request) = &entry.module_request { let source = module_request.name(); let span = entry.span; @@ -160,6 +148,10 @@ impl Rule for NoDuplicateImports { continue; } } + if side_effect_import_map.get(source).is_some() { + ctx.diagnostic(no_duplicate_exports_diagnostic(source, span)); + continue; + } import_map.entry(source).or_default().push(( ImportType::AllButDefault, span, @@ -184,12 +176,10 @@ impl Rule for NoDuplicateImports { } } - // Handle indirect exports for entry in &module_record.indirect_export_entries { if let Some(module_request) = &entry.module_request { let source = module_request.name(); let span = entry.span; - println!("- source: {source:?}, span: {span:?}, type: indirect_export_entries"); if let Some(existing) = import_map.get(source) { if entry.import_name == ExportImportName::All { @@ -231,15 +221,10 @@ fn can_merge_imports( existing: &[(ImportType, Span, ModuleType)], same_statement: bool, ) -> bool { - println!("existing: {existing:?}"); - - // Allow multiple imports in the same statement if same_statement { return false; } - // this is the (namespace, span) if exsiting. None else - let namespace = existing.iter().find(|(t, _, _)| matches!(t, ImportType::Namespace)); let named = existing.iter().find(|(t, _, _)| matches!(t, ImportType::Named)); let default = existing.iter().find(|(t, _, _)| matches!(t, ImportType::Default)); @@ -248,14 +233,12 @@ fn can_merge_imports( let has_named = named.is_some(); let has_default = default.is_some(); - // For duplicate named imports if matches!(current_type, ImportType::Named) { if has_named { return true; } } - // For namespace imports if matches!(current_type, ImportType::Namespace) { if has_named && has_default { if let Some((_, named_span, _)) = named { @@ -275,7 +258,6 @@ fn can_merge_imports( } } - // For default imports if matches!(current_type, ImportType::Default) { if has_default { return true; @@ -288,9 +270,7 @@ fn can_merge_imports( } } - // For side effect imports if matches!(current_type, ImportType::SideEffect) { - // Any existing import means it's a duplicate if !existing.is_empty() { return true; } @@ -449,11 +429,11 @@ fn test() { export * from "os";"#, Some(serde_json::json!([{ "includeExports": true }])), ), - // ( - // r#"import "os"; - // export * from "os";"#, - // Some(serde_json::json!([{ "includeExports": true }])), - // ), + ( + r#"import "os"; + export * from "os";"#, + Some(serde_json::json!([{ "includeExports": true }])), + ), ]; Tester::new(NoDuplicateImports::NAME, pass, fail).test_and_snapshot(); From f73c124fb9f111a8f4c722b069199a5593454d73 Mon Sep 17 00:00:00 2001 From: Guillaume Piedigrossi Date: Sat, 23 Nov 2024 22:38:35 +0100 Subject: [PATCH 6/8] feat(linter / no_duplicate_imports): use with_labels instead of with_label to give more info --- .../src/rules/eslint/no_duplicate_imports.rs | 61 ++++++++-- .../src/snapshots/no_duplicate_imports.snap | 108 +++++++++++++----- 2 files changed, 129 insertions(+), 40 deletions(-) diff --git a/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs b/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs index 90dfba0a8b2d7..af7f94c479ece 100644 --- a/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs +++ b/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs @@ -7,16 +7,22 @@ use oxc_syntax::module_record::{ExportImportName, ImportImportName}; use crate::{context::LintContext, rule::Rule}; -fn no_duplicate_imports_diagnostic(module_name: &str, span: Span) -> OxcDiagnostic { +fn no_duplicate_imports_diagnostic(module_name: &str, span: Span, span2: Span) -> OxcDiagnostic { OxcDiagnostic::warn(format!("'{}' import is duplicated", module_name)) .with_help("Merge the duplicated import into a single import statement") - .with_label(span) + .with_labels([ + span.label("This import is duplicated"), + span2.label("Can be merged with this import"), + ]) } -fn no_duplicate_exports_diagnostic(module_name: &str, span: Span) -> OxcDiagnostic { +fn no_duplicate_exports_diagnostic(module_name: &str, span: Span, span2: Span) -> OxcDiagnostic { OxcDiagnostic::warn(format!("'{}' export is duplicated", module_name)) .with_help("Merge the duplicated exports into a single export statement") - .with_label(span) + .with_labels([ + span.label("This export is duplicated"), + span2.label("Can be merged with this"), + ]) } #[derive(Debug, Default, Clone)] @@ -102,7 +108,11 @@ impl Rule for NoDuplicateImports { if let Some(existing) = import_map.get(source) { let can_merge = can_merge_imports(&import_type, existing, same_statement); if can_merge { - ctx.diagnostic(no_duplicate_imports_diagnostic(source, span)); + ctx.diagnostic(no_duplicate_imports_diagnostic( + source, + span, + existing.first().unwrap().1, + )); continue; } } @@ -127,7 +137,14 @@ impl Rule for NoDuplicateImports { side_effect_import_map.iter().for_each(|(source, spans)| { if spans.len() > 1 { spans.iter().for_each(|span| { - ctx.diagnostic(no_duplicate_imports_diagnostic(source, *span)); + let i = spans.iter().position(|s| s == span).unwrap(); + if i > 0 { + ctx.diagnostic(no_duplicate_imports_diagnostic( + source, + *span, + spans.first().unwrap().clone(), + )); + } }); } }); @@ -144,12 +161,20 @@ impl Rule for NoDuplicateImports { .iter() .any(|(t, _, _)| matches!(t, ImportType::AllButDefault)) { - ctx.diagnostic(no_duplicate_exports_diagnostic(source, span)); + ctx.diagnostic(no_duplicate_exports_diagnostic( + source, + span, + existing.first().unwrap().1, + )); continue; } } - if side_effect_import_map.get(source).is_some() { - ctx.diagnostic(no_duplicate_exports_diagnostic(source, span)); + if let Some(existing) = side_effect_import_map.get(source) { + ctx.diagnostic(no_duplicate_exports_diagnostic( + source, + span, + *existing.first().unwrap(), + )); continue; } import_map.entry(source).or_default().push(( @@ -163,7 +188,11 @@ impl Rule for NoDuplicateImports { if existing.iter().any(|(t, _, _)| { matches!(t, ImportType::Named | ImportType::SideEffect) }) { - ctx.diagnostic(no_duplicate_exports_diagnostic(source, span)); + ctx.diagnostic(no_duplicate_exports_diagnostic( + source, + span, + existing.first().unwrap().1, + )); continue; } } @@ -186,7 +215,11 @@ impl Rule for NoDuplicateImports { if existing.iter().any(|(t, _, _)| { matches!(t, ImportType::Default | ImportType::Namespace) }) { - ctx.diagnostic(no_duplicate_exports_diagnostic(source, span)); + ctx.diagnostic(no_duplicate_exports_diagnostic( + source, + span, + existing.first().unwrap().1, + )); continue; } @@ -201,7 +234,11 @@ impl Rule for NoDuplicateImports { || (matches!(t, ImportType::Default) && *module_type == ModuleType::Import) }) { - ctx.diagnostic(no_duplicate_exports_diagnostic(source, span)); + ctx.diagnostic(no_duplicate_exports_diagnostic( + source, + span, + existing.first().unwrap().1, + )); continue; } } diff --git a/crates/oxc_linter/src/snapshots/no_duplicate_imports.snap b/crates/oxc_linter/src/snapshots/no_duplicate_imports.snap index 6eae291c8d116..f00135c0848e0 100644 --- a/crates/oxc_linter/src/snapshots/no_duplicate_imports.snap +++ b/crates/oxc_linter/src/snapshots/no_duplicate_imports.snap @@ -3,107 +3,159 @@ source: crates/oxc_linter/src/tester.rs snapshot_kind: text --- ⚠ eslint(no-duplicate-imports): 'fs' import is duplicated - ╭─[no_duplicate_imports.tsx:2:9] + ╭─[no_duplicate_imports.tsx:1:8] 1 │ import "fs"; + · ──┬─ + · ╰── Can be merged with this import 2 │ import "fs" - · ─────────── + · ──┬─ + · ╰── This import is duplicated ╰──── help: Merge the duplicated import into a single import statement ⚠ eslint(no-duplicate-imports): 'lodash-es' import is duplicated - ╭─[no_duplicate_imports.tsx:2:9] + ╭─[no_duplicate_imports.tsx:1:23] 1 │ import { merge } from "lodash-es"; + · ─────┬───── + · ╰── Can be merged with this import 2 │ import { find } from "lodash-es"; - · ───────────────────────────────── + · ─────┬───── + · ╰── This import is duplicated ╰──── help: Merge the duplicated import into a single import statement ⚠ eslint(no-duplicate-imports): 'lodash-es' import is duplicated - ╭─[no_duplicate_imports.tsx:2:11] + ╭─[no_duplicate_imports.tsx:1:23] 1 │ import { merge } from "lodash-es"; + · ─────┬───── + · ╰── Can be merged with this import 2 │ import _ from "lodash-es"; - · ────────────────────────── + · ─────┬───── + · ╰── This import is duplicated ╰──── help: Merge the duplicated import into a single import statement ⚠ eslint(no-duplicate-imports): 'os' import is duplicated - ╭─[no_duplicate_imports.tsx:2:11] + ╭─[no_duplicate_imports.tsx:1:16] 1 │ import os from "os"; + · ──┬─ + · ╰── Can be merged with this import 2 │ import { something } from "os"; - · ─────────────────────────────── 3 │ import * as foobar from "os"; + · ──┬─ + · ╰── This import is duplicated ╰──── help: Merge the duplicated import into a single import statement ⚠ eslint(no-duplicate-imports): 'lodash-es' import is duplicated - ╭─[no_duplicate_imports.tsx:3:11] + ╭─[no_duplicate_imports.tsx:1:24] + 1 │ import * as modns from "lodash-es"; + · ─────┬───── + · ╰── Can be merged with this import 2 │ import { merge } from "lodash-es"; 3 │ import { baz } from "lodash-es"; - · ──────────────────────────────── + · ─────┬───── + · ╰── This import is duplicated ╰──── help: Merge the duplicated import into a single import statement ⚠ eslint(no-duplicate-imports): 'os' export is duplicated - ╭─[no_duplicate_imports.tsx:2:11] + ╭─[no_duplicate_imports.tsx:1:10] 1 │ export { os } from "os"; + · ─┬ + · ╰── Can be merged with this 2 │ export { something } from "os"; - · ─────────────────────────────── + · ────┬──── + · ╰── This export is duplicated ╰──── help: Merge the duplicated exports into a single export statement ⚠ eslint(no-duplicate-imports): 'os' export is duplicated - ╭─[no_duplicate_imports.tsx:2:11] + ╭─[no_duplicate_imports.tsx:1:16] 1 │ import os from "os"; + · ──┬─ + · ╰── Can be merged with this 2 │ export { os as foobar } from "os"; - · ────────────────────────────────── + · ──────┬───── + · ╰── This export is duplicated 3 │ export { something } from "os"; ╰──── help: Merge the duplicated exports into a single export statement ⚠ eslint(no-duplicate-imports): 'os' export is duplicated - ╭─[no_duplicate_imports.tsx:3:11] + ╭─[no_duplicate_imports.tsx:1:16] + 1 │ import os from "os"; + · ──┬─ + · ╰── Can be merged with this 2 │ export { os as foobar } from "os"; 3 │ export { something } from "os"; - · ─────────────────────────────── + · ────┬──── + · ╰── This export is duplicated ╰──── help: Merge the duplicated exports into a single export statement ⚠ eslint(no-duplicate-imports): 'os' export is duplicated - ╭─[no_duplicate_imports.tsx:2:11] + ╭─[no_duplicate_imports.tsx:1:16] 1 │ import os from "os"; + · ──┬─ + · ╰── Can be merged with this 2 │ export { something } from "os"; - · ─────────────────────────────── + · ────┬──── + · ╰── This export is duplicated ╰──── help: Merge the duplicated exports into a single export statement ⚠ eslint(no-duplicate-imports): 'os' export is duplicated - ╭─[no_duplicate_imports.tsx:2:9] + ╭─[no_duplicate_imports.tsx:1:16] 1 │ import os from "os"; + · ──┬─ + · ╰── Can be merged with this 2 │ export * as os from "os"; - · ───────────────────────── + · ────────────┬──────────── + · ╰── This export is duplicated ╰──── help: Merge the duplicated exports into a single export statement - ⚠ eslint(no-duplicate-imports): 'os' import is duplicated - ╭─[no_duplicate_imports.tsx:2:9] + ⚠ eslint(no-duplicate-imports): 'os' export is duplicated + ╭─[no_duplicate_imports.tsx:1:1] 1 │ export * as os from "os"; + · ────────────┬──────────── + · ╰── This export is duplicated 2 │ import os from "os"; - · ──────────────────── + · ──┬─ + · ╰── Can be merged with this ╰──── - help: Merge the duplicated import into a single import statement + help: Merge the duplicated exports into a single export statement + + ⚠ eslint(no-duplicate-imports): 'mod' export is duplicated + ╭─[no_duplicate_imports.tsx:1:24] + 1 │ import * as modns from "mod"; + · ──┬── + · ╰── Can be merged with this + 2 │ export * as modns from "mod"; + · ───────────────┬────────────── + · ╰── This export is duplicated + ╰──── + help: Merge the duplicated exports into a single export statement ⚠ eslint(no-duplicate-imports): 'os' export is duplicated - ╭─[no_duplicate_imports.tsx:2:9] + ╭─[no_duplicate_imports.tsx:1:1] 1 │ export * from "os"; + · ─────────┬───────── + · ╰── Can be merged with this 2 │ export * from "os"; - · ─────────────────── + · ─────────┬───────── + · ╰── This export is duplicated ╰──── help: Merge the duplicated exports into a single export statement ⚠ eslint(no-duplicate-imports): 'os' export is duplicated - ╭─[no_duplicate_imports.tsx:2:9] + ╭─[no_duplicate_imports.tsx:1:8] 1 │ import "os"; + · ──┬─ + · ╰── Can be merged with this 2 │ export * from "os"; - · ─────────────────── + · ─────────┬───────── + · ╰── This export is duplicated ╰──── help: Merge the duplicated exports into a single export statement From c589152fc51f72a23066a8d3521fcebc038a9216 Mon Sep 17 00:00:00 2001 From: Guillaume Piedigrossi Date: Mon, 25 Nov 2024 21:03:19 +0100 Subject: [PATCH 7/8] refactor(linte / no_duplicate_imports): improve code style and performances with clippy --- .../src/rules/eslint/no_duplicate_imports.rs | 40 ++++++++----------- 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs b/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs index af7f94c479ece..5e16cebd0d10a 100644 --- a/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs +++ b/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs @@ -1,4 +1,4 @@ -use std::collections::HashMap; +use rustc_hash::FxHashMap; use oxc_diagnostics::OxcDiagnostic; use oxc_macros::declare_oxc_lint; @@ -8,7 +8,7 @@ use oxc_syntax::module_record::{ExportImportName, ImportImportName}; use crate::{context::LintContext, rule::Rule}; fn no_duplicate_imports_diagnostic(module_name: &str, span: Span, span2: Span) -> OxcDiagnostic { - OxcDiagnostic::warn(format!("'{}' import is duplicated", module_name)) + OxcDiagnostic::warn(format!("'{module_name}' import is duplicated")) .with_help("Merge the duplicated import into a single import statement") .with_labels([ span.label("This import is duplicated"), @@ -17,7 +17,7 @@ fn no_duplicate_imports_diagnostic(module_name: &str, span: Span, span2: Span) - } fn no_duplicate_exports_diagnostic(module_name: &str, span: Span, span2: Span) -> OxcDiagnostic { - OxcDiagnostic::warn(format!("'{}' export is duplicated", module_name)) + OxcDiagnostic::warn(format!("'{module_name}' export is duplicated")) .with_help("Merge the duplicated exports into a single export statement") .with_labels([ span.label("This export is duplicated"), @@ -83,10 +83,10 @@ impl Rule for NoDuplicateImports { fn run_once(&self, ctx: &LintContext) { let module_record = ctx.module_record(); - let mut import_map: HashMap<&CompactStr, Vec<(ImportType, Span, ModuleType)>> = - HashMap::new(); + let mut import_map: FxHashMap<&CompactStr, Vec<(ImportType, Span, ModuleType)>> = + FxHashMap::default(); let mut current_span: Option = None; - let mut side_effect_import_map: HashMap<&CompactStr, Vec> = HashMap::new(); + let mut side_effect_import_map: FxHashMap<&CompactStr, Vec> = FxHashMap::default(); for entry in &module_record.import_entries { let source = entry.module_request.name(); @@ -126,28 +126,26 @@ impl Rule for NoDuplicateImports { for (source, requests) in &module_record.requested_modules { for request in requests { - if request.is_import() { - if module_record.import_entries.is_empty() { - side_effect_import_map.entry(source).or_default().push(request.span()); - } + if request.is_import() && module_record.import_entries.is_empty() { + side_effect_import_map.entry(source).or_default().push(request.span()); } } } - side_effect_import_map.iter().for_each(|(source, spans)| { + for (source, spans) in &side_effect_import_map { if spans.len() > 1 { - spans.iter().for_each(|span| { + for span in spans { let i = spans.iter().position(|s| s == span).unwrap(); if i > 0 { ctx.diagnostic(no_duplicate_imports_diagnostic( source, *span, - spans.first().unwrap().clone(), + *spans.first().unwrap(), )); } - }); + } } - }); + } if self.include_exports { for entry in &module_record.star_export_entries { @@ -270,10 +268,8 @@ fn can_merge_imports( let has_named = named.is_some(); let has_default = default.is_some(); - if matches!(current_type, ImportType::Named) { - if has_named { - return true; - } + if matches!(current_type, ImportType::Named) && has_named { + return true; } if matches!(current_type, ImportType::Namespace) { @@ -307,10 +303,8 @@ fn can_merge_imports( } } - if matches!(current_type, ImportType::SideEffect) { - if !existing.is_empty() { - return true; - } + if matches!(current_type, ImportType::SideEffect) && !existing.is_empty() { + return true; } false From 95b33c794304a22da6f81634a1ad9796905f3ee4 Mon Sep 17 00:00:00 2001 From: Guillaume Piedigrossi Date: Mon, 25 Nov 2024 21:05:03 +0100 Subject: [PATCH 8/8] feat(linter / no_duplicate_imports): change the rule category to "style" --- crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs b/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs index 5e16cebd0d10a..dca41efb96eaa 100644 --- a/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs +++ b/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs @@ -52,7 +52,7 @@ declare_oxc_lint!( /// import something from 'another-module'; /// ``` NoDuplicateImports, - nursery, + style, pending); #[derive(Debug, Clone, PartialEq)]