refactor(linter): improve docs and simplify code of eslint/no-duplicate-imports (#11320)

ulrichstark · web-flow · commit e2f0f0a87e5e · 2025-05-27T12:52:04.000+01:00
diff --git a/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs b/crates/oxc_linter/src/rules/eslint/no_duplicate_imports.rs
@@ -10,21 +10,29 @@ use crate::{
     rule::Rule,
 };
 
-fn no_duplicate_imports_diagnostic(module_name: &str, span: Span, span2: Span) -> OxcDiagnostic {
+fn no_duplicate_imports_diagnostic(
+    module_name: &str,
+    span: Span,
+    previous_span: Span,
+) -> OxcDiagnostic {
     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"),
-            span2.label("Can be merged with this import"),
+            previous_span.label("Can be merged with this import"),
         ])
 }
 
-fn no_duplicate_exports_diagnostic(module_name: &str, span: Span, span2: Span) -> OxcDiagnostic {
+fn no_duplicate_exports_diagnostic(
+    module_name: &str,
+    span: Span,
+    previous_span: Span,
+) -> OxcDiagnostic {
     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"),
-            span2.label("Can be merged with this"),
+            previous_span.label("Can be merged with this"),
         ])
 }
 
@@ -65,21 +73,21 @@ declare_oxc_lint!(
     ///
     /// #### includeExports
     ///
-    /// `{ "includeExports": boolean }`
+    /// `{ type: boolean, default: false }`
     ///
     /// When `true` this rule will also look at exports to see if there is both a re-export of a
     /// module as in `export ... from 'module'` and also a standard import statement for the same
     /// module. This would count as a rule violation because there are in a sense two statements
     /// importing from the same module.
     ///
-    /// Examples of **incorrect** when this rule is set to `true`
+    /// Examples of **incorrect** code when `includeExports` is set to `true`:
     /// ```js
     /// import { merge } from 'module';
     ///
     /// export { find } from 'module'; // re-export which is an import and an export.
     /// ```
     ///
-    /// Examples of **correct** when this rule is set to `true`
+    /// Examples of **correct** code when `includeExports` is set to `true`:
     ///
     /// If re-exporting from an imported module, you should add the imports to the
     /// `import` statement, and export that directly, not use `export ... from`.
@@ -100,7 +108,8 @@ declare_oxc_lint!(
     NoDuplicateImports,
     eslint,
     style,
-    pending);
+    pending
+);
 
 #[derive(Debug, Clone, PartialEq)]
 enum ImportType {
@@ -119,10 +128,10 @@ enum ModuleType {
 
 impl Rule for NoDuplicateImports {
     fn from_configuration(value: serde_json::Value) -> Self {
-        let Some(value) = value.get(0) else { return Self { include_exports: false } };
+        let value = value.get(0);
         Self {
             include_exports: value
-                .get("includeExports")
+                .and_then(|v| v.get("includeExports"))
                 .and_then(serde_json::Value::as_bool)
                 .unwrap_or(false),
         }
@@ -132,62 +141,54 @@ impl Rule for NoDuplicateImports {
         let module_record = ctx.module_record();
         let mut import_map: FxHashMap<&CompactStr, Vec<(ImportType, Span, ModuleType)>> =
             FxHashMap::default();
-        let mut current_span: Option<Span> = None;
+        let mut previous_span: Option<Span> = None;
         let mut side_effect_import_map: FxHashMap<&CompactStr, Vec<Span>> = FxHashMap::default();
 
         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,
             };
 
-            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,
-                        existing.first().unwrap().1,
-                    ));
-                    continue;
+            if previous_span != Some(span) {
+                previous_span = Some(span);
+
+                if let Some(existing) = import_map.get(source) {
+                    if can_merge_imports(&import_type, existing) {
+                        ctx.diagnostic(no_duplicate_imports_diagnostic(
+                            source,
+                            span,
+                            existing.first().unwrap().1,
+                        ));
+                        continue;
+                    }
                 }
             }
 
             import_map.entry(source).or_default().push((import_type, span, ModuleType::Import));
-
-            if !same_statement {
-                current_span = Some(span);
-            }
         }
 
-        for (source, requests) in &module_record.requested_modules {
-            for request in requests {
-                if request.is_import && module_record.import_entries.is_empty() {
-                    side_effect_import_map.entry(source).or_default().push(request.span);
+        if module_record.import_entries.is_empty() {
+            for (source, requests) in &module_record.requested_modules {
+                for request in requests {
+                    if request.is_import {
+                        side_effect_import_map.entry(source).or_default().push(request.span);
+                    }
                 }
             }
-        }
 
-        for (source, spans) in &side_effect_import_map {
-            if spans.len() > 1 {
-                for span in spans {
-                    let i = spans.iter().position(|s| s == span).unwrap();
-                    if i > 0 {
+            for (source, spans) in &side_effect_import_map {
+                let mut spans_iter = spans.iter();
+                if let Some(first_span) = spans_iter.next() {
+                    for following_span in spans_iter {
                         ctx.diagnostic(no_duplicate_imports_diagnostic(
                             source,
-                            *span,
-                            *spans.first().unwrap(),
+                            *following_span,
+                            *first_span,
                         ));
                     }
                 }
@@ -266,7 +267,6 @@ impl Rule for NoDuplicateImports {
                                 span,
                                 existing.first().unwrap().1,
                             ));
-                            continue;
                         }
 
                         continue;
@@ -302,12 +302,7 @@ impl Rule for NoDuplicateImports {
 fn can_merge_imports(
     current_type: &ImportType,
     existing: &[(ImportType, Span, ModuleType)],
-    same_statement: bool,
 ) -> bool {
-    if same_statement {
-        return false;
-    }
-
     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));
@@ -316,46 +311,25 @@ fn can_merge_imports(
     let has_named = named.is_some();
     let has_default = default.is_some();
 
-    if matches!(current_type, ImportType::Named) && has_named {
-        return true;
-    }
-
-    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;
+    match current_type {
+        ImportType::Named => has_named,
+        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;
+                        }
                     }
                 }
             }
-        }
 
-        if has_namespace {
-            return true;
-        }
-        if has_default && !same_statement {
-            return true;
+            has_namespace || has_default
         }
+        ImportType::Default => has_default || has_namespace || has_named,
+        ImportType::SideEffect => !existing.is_empty(),
+        ImportType::AllButDefault => false,
     }
-
-    if matches!(current_type, ImportType::Default) {
-        if has_default {
-            return true;
-        }
-        if has_named && !same_statement {
-            return true;
-        }
-        if has_namespace {
-            return true;
-        }
-    }
-
-    if matches!(current_type, ImportType::SideEffect) && !existing.is_empty() {
-        return true;
-    }
-
-    false
 }
 
 #[test]
