feat: emit warning for circular dependency (#839)

Co-authored-by: Yunfei <i.heyunfei@gmail.com>
rolldown · Apr 12, 2024 · 6d64a64 · 6d64a64
1 parent 117d2c4
commit 6d64a64
Show file tree

Hide file tree

Showing 17 changed files with 161 additions and 7 deletions.
diff --git a/crates/rolldown/src/stages/link_stage/sort_modules.rs b/crates/rolldown/src/stages/link_stage/sort_modules.rs
@@ -1,6 +1,7 @@
 use std::iter;
 
 use rolldown_common::ModuleId;
+use rolldown_error::BuildError;
 use rustc_hash::{FxHashMap, FxHashSet};
 
 use super::LinkStage;
@@ -95,9 +96,15 @@ impl<'a> LinkStage<'a> {
     }
 
     if !circular_dependencies.is_empty() {
-      let mut cycles = circular_dependencies.into_iter().collect::<Vec<_>>();
-      cycles.sort();
-      // TODO: emit warning
+      let cycles = circular_dependencies.into_iter().collect::<Vec<_>>();
+      for cycle in cycles {
+        let paths = cycle
+          .iter()
+          .filter_map(|id| id.as_normal())
+          .map(|id| self.module_table.normal_modules[id].resource_id.expect_file().to_string())
+          .collect::<Vec<_>>();
+        self.warnings.push(BuildError::circular_dependency(paths).with_severity_warning());
+      }
     }
 
     self.sorted_modules = sorted_modules;

diff --git a/crates/rolldown/tests/esbuild/import_star/export_self_as_namespace_es6/artifacts.snap b/crates/rolldown/tests/esbuild/import_star/export_self_as_namespace_es6/artifacts.snap
@@ -3,6 +3,14 @@ source: crates/rolldown/tests/common/case.rs
 expression: content
 input_file: crates/rolldown/tests/esbuild/import_star/export_self_as_namespace_es6
 ---
+# warnings
+
+## CIRCULAR_DEPENDENCY
+
+```text
+[CIRCULAR_DEPENDENCY] Warning: Circular dependency: tests/esbuild/import_star/export_self_as_namespace_es6/entry.js -> tests/esbuild/import_star/export_self_as_namespace_es6/entry.js.
+
+```
 # Assets
 
 ## entry_js.mjs

diff --git a/crates/rolldown/tests/esbuild/import_star/export_self_es6/artifacts.snap b/crates/rolldown/tests/esbuild/import_star/export_self_es6/artifacts.snap
@@ -3,6 +3,14 @@ source: crates/rolldown/tests/common/case.rs
 expression: content
 input_file: crates/rolldown/tests/esbuild/import_star/export_self_es6
 ---
+# warnings
+
+## CIRCULAR_DEPENDENCY
+
+```text
+[CIRCULAR_DEPENDENCY] Warning: Circular dependency: tests/esbuild/import_star/export_self_es6/entry.js -> tests/esbuild/import_star/export_self_es6/entry.js.
+
+```
 # Assets
 
 ## entry_js.mjs

diff --git a/crates/rolldown/tests/esbuild/import_star/import_export_self_as_namespace_es6/artifacts.snap b/crates/rolldown/tests/esbuild/import_star/import_export_self_as_namespace_es6/artifacts.snap
@@ -3,6 +3,14 @@ source: crates/rolldown/tests/common/case.rs
 expression: content
 input_file: crates/rolldown/tests/esbuild/import_star/import_export_self_as_namespace_es6
 ---
+# warnings
+
+## CIRCULAR_DEPENDENCY
+
+```text
+[CIRCULAR_DEPENDENCY] Warning: Circular dependency: tests/esbuild/import_star/import_export_self_as_namespace_es6/entry.js -> tests/esbuild/import_star/import_export_self_as_namespace_es6/entry.js.
+
+```
 # Assets
 
 ## entry_js.mjs

diff --git a/...n/tests/esbuild/import_star/other_file_export_self_as_namespace_unused_es6/artifacts.snap b/...n/tests/esbuild/import_star/other_file_export_self_as_namespace_unused_es6/artifacts.snap
@@ -3,6 +3,14 @@ source: crates/rolldown/tests/common/case.rs
 expression: content
 input_file: crates/rolldown/tests/esbuild/import_star/other_file_export_self_as_namespace_unused_es6
 ---
+# warnings
+
+## CIRCULAR_DEPENDENCY
+
+```text
+[CIRCULAR_DEPENDENCY] Warning: Circular dependency: tests/esbuild/import_star/other_file_export_self_as_namespace_unused_es6/foo.js -> tests/esbuild/import_star/other_file_export_self_as_namespace_unused_es6/foo.js.
+
+```
 # Assets
 
 ## entry_js.mjs

diff --git a/.../esbuild/import_star/other_file_import_export_self_as_namespace_unused_es6/artifacts.snap b/.../esbuild/import_star/other_file_import_export_self_as_namespace_unused_es6/artifacts.snap
@@ -3,6 +3,14 @@ source: crates/rolldown/tests/common/case.rs
 expression: content
 input_file: crates/rolldown/tests/esbuild/import_star/other_file_import_export_self_as_namespace_unused_es6
 ---
+# warnings
+
+## CIRCULAR_DEPENDENCY
+
+```text
+[CIRCULAR_DEPENDENCY] Warning: Circular dependency: tests/esbuild/import_star/other_file_import_export_self_as_namespace_unused_es6/foo.js -> tests/esbuild/import_star/other_file_import_export_self_as_namespace_unused_es6/foo.js.
+
+```
 # Assets
 
 ## entry_js.mjs

diff --git a/...ests/esbuild/import_star/re_export_other_file_export_self_as_namespace_es6/artifacts.snap b/...ests/esbuild/import_star/re_export_other_file_export_self_as_namespace_es6/artifacts.snap
@@ -3,6 +3,14 @@ source: crates/rolldown/tests/common/case.rs
 expression: content
 input_file: crates/rolldown/tests/esbuild/import_star/re_export_other_file_export_self_as_namespace_es6
 ---
+# warnings
+
+## CIRCULAR_DEPENDENCY
+
+```text
+[CIRCULAR_DEPENDENCY] Warning: Circular dependency: tests/esbuild/import_star/re_export_other_file_export_self_as_namespace_es6/foo.js -> tests/esbuild/import_star/re_export_other_file_export_self_as_namespace_es6/foo.js.
+
+```
 # Assets
 
 ## entry_js.mjs

diff --git a/...build/import_star/re_export_other_file_import_export_self_as_namespace_es6/artifacts.snap b/...build/import_star/re_export_other_file_import_export_self_as_namespace_es6/artifacts.snap
@@ -3,6 +3,14 @@ source: crates/rolldown/tests/common/case.rs
 expression: content
 input_file: crates/rolldown/tests/esbuild/import_star/re_export_other_file_import_export_self_as_namespace_es6
 ---
+# warnings
+
+## CIRCULAR_DEPENDENCY
+
+```text
+[CIRCULAR_DEPENDENCY] Warning: Circular dependency: tests/esbuild/import_star/re_export_other_file_import_export_self_as_namespace_es6/foo.js -> tests/esbuild/import_star/re_export_other_file_import_export_self_as_namespace_es6/foo.js.
+
+```
 # Assets
 
 ## entry_js.mjs

diff --git a/crates/rolldown/tests/fixtures/circular_depency/_config.json b/crates/rolldown/tests/fixtures/circular_depency/_config.json
@@ -0,0 +1,15 @@
+{
+  "config": {
+      "input": [
+          {
+              "name": "entry",
+              "import": "entry.js"
+          },
+          {
+              "name": "main",
+              "import": "main.js"
+          }
+      ]
+  },
+  "expectExecuted": false
+}
diff --git a/crates/rolldown/tests/fixtures/circular_depency/artifacts.snap b/crates/rolldown/tests/fixtures/circular_depency/artifacts.snap
@@ -0,0 +1,36 @@
+---
+source: crates/rolldown/tests/common/case.rs
+expression: content
+input_file: crates/rolldown/tests/fixtures/circular_depency
+---
+# warnings
+
+## CIRCULAR_DEPENDENCY
+
+```text
+[CIRCULAR_DEPENDENCY] Warning: Circular dependency: tests/fixtures/circular_depency/foo.js -> tests/fixtures/circular_depency/main.js -> tests/fixtures/circular_depency/foo.js.
+
+```
+## CIRCULAR_DEPENDENCY
+
+```text
+[CIRCULAR_DEPENDENCY] Warning: Circular dependency: tests/fixtures/circular_depency/entry.js -> tests/fixtures/circular_depency/foo.js -> tests/fixtures/circular_depency/entry.js.
+
+```
+# Assets
+
+## entry.mjs
+
+```js
+import "./main_js.mjs";
+```
+## main.mjs
+
+```js
+import "./main_js.mjs";
+```
+## main_js.mjs
+
+```js
+
+```
diff --git a/crates/rolldown/tests/fixtures/circular_depency/entry.js b/crates/rolldown/tests/fixtures/circular_depency/entry.js
@@ -0,0 +1 @@
+import './foo'
diff --git a/crates/rolldown/tests/fixtures/circular_depency/foo.js b/crates/rolldown/tests/fixtures/circular_depency/foo.js
@@ -0,0 +1,2 @@
+import './main'
+import './entry'
diff --git a/crates/rolldown/tests/fixtures/circular_depency/main.js b/crates/rolldown/tests/fixtures/circular_depency/main.js
@@ -0,0 +1 @@
+import './foo'
diff --git a/crates/rolldown_error/src/build_error/error_constructors.rs b/crates/rolldown_error/src/build_error/error_constructors.rs
@@ -8,9 +8,10 @@ use oxc::span::Span;
 use super::BuildError;
 
 use crate::events::{
-  external_entry::ExternalEntry, forbid_const_assign::ForbidConstAssign,
-  sourcemap_error::SourceMapError, unresolved_entry::UnresolvedEntry,
-  unresolved_import::UnresolvedImport, unsupported_eval::UnsupportedEval, NapiError,
+  circular_dependency::CircularDependency, external_entry::ExternalEntry,
+  forbid_const_assign::ForbidConstAssign, sourcemap_error::SourceMapError,
+  unresolved_entry::UnresolvedEntry, unresolved_import::UnresolvedImport,
+  unsupported_eval::UnsupportedEval, NapiError,
 };
 
 impl BuildError {
@@ -31,6 +32,10 @@ impl BuildError {
     Self::new_inner(SourceMapError { error })
   }
 
+  pub fn circular_dependency(paths: Vec<String>) -> Self {
+    Self::new_inner(CircularDependency { paths })
+  }
+
   // --- Rolldown related
 
   pub fn forbid_const_assign(

diff --git a/crates/rolldown_error/src/event_kind.rs b/crates/rolldown_error/src/event_kind.rs
@@ -6,7 +6,7 @@ pub enum EventKind {
   UnresolvedImport,
   IllegalReassignment,
   Eval,
-
+  CircularDependency,
   // --- These kinds are rolldown specific
   // !! Only add new kind if it's not covered by the kinds from rollup !!
 
@@ -25,6 +25,7 @@ impl Display for EventKind {
       EventKind::IllegalReassignment => write!(f, "ILLEGAL_REASSIGNMENT"),
       EventKind::Eval => write!(f, "EVAL"),
       EventKind::SourcemapError => write!(f, "SOURCEMAP_ERROR"),
+      EventKind::CircularDependency => write!(f, "CIRCULAR_DEPENDENCY"),
 
       // --- Rolldown specific
       EventKind::NapiError => write!(f, "NAPI_ERROR"),

diff --git a/crates/rolldown_error/src/events/circular_dependency.rs b/crates/rolldown_error/src/events/circular_dependency.rs
@@ -0,0 +1,28 @@
+use sugar_path::SugarPath;
+
+use super::BuildEvent;
+use crate::{event_kind::EventKind, PathExt};
+
+#[derive(Debug)]
+pub struct CircularDependency {
+  pub paths: Vec<String>,
+}
+
+impl CircularDependency {
+  fn relative_paths(&self) -> Vec<String> {
+    self.paths.iter().map(|p| p.as_path().relative_display().into_owned()).collect::<Vec<_>>()
+  }
+}
+
+impl BuildEvent for CircularDependency {
+  fn kind(&self) -> EventKind {
+    EventKind::CircularDependency
+  }
+  fn code(&self) -> &'static str {
+    "CIRCULAR_DEPENDENCY"
+  }
+
+  fn message(&self) -> String {
+    format!("Circular dependency: {}.", self.relative_paths().join(" -> "))
+  }
+}
diff --git a/crates/rolldown_error/src/events/mod.rs b/crates/rolldown_error/src/events/mod.rs
@@ -1,6 +1,8 @@
 use std::fmt::Debug;
 
 use crate::{diagnostic::Diagnostic, event_kind::EventKind};
+
+pub mod circular_dependency;
 pub mod external_entry;
 pub mod forbid_const_assign;
 pub mod sourcemap_error;