feat: enhance cycle deps error message (#1639)

* feat: enhance cycle deps error message Signed-off-by: he1pa <[email protected]> * fix: fix ut Signed-off-by: he1pa <[email protected]> * fix ut Signed-off-by: he1pa <[email protected]> * remove unwrap and extra magic var __main__ Signed-off-by: he1pa <[email protected]> * use SchemaType::full_ty_str() to calculate schame name in cycle Signed-off-by: he1pa <[email protected]> * use full_ty_str() to calculate schame name in cycle Signed-off-by: he1pa <[email protected]> --------- Signed-off-by: he1pa <[email protected]>
kcl-lang · Sep 14, 2024 · a8242c7 · a8242c7
1 parent 6148b03
commit a8242c7
Show file tree

Hide file tree

Showing 13 changed files with 219 additions and 87 deletions.
diff --git a/kclvm/sema/src/resolver/global.rs b/kclvm/sema/src/resolver/global.rs
@@ -4,8 +4,8 @@ use std::sync::Arc;
 use crate::info::is_private_field;
 use crate::resolver::Resolver;
 use crate::ty::{
-    is_upper_bound, DecoratorTarget, FunctionType, Parameter, SchemaAttr, SchemaIndexSignature,
-    SchemaType, Type, TypeKind, RESERVED_TYPE_IDENTIFIERS,
+    full_ty_str, is_upper_bound, DecoratorTarget, FunctionType, Parameter, SchemaAttr,
+    SchemaIndexSignature, SchemaType, Type, TypeKind, RESERVED_TYPE_IDENTIFIERS,
 };
 use indexmap::IndexMap;
 use kclvm_ast::ast;
@@ -63,7 +63,6 @@ impl<'ctx> Resolver<'ctx> {
                                 false,
                                 true,
                             ),
-
                             _ => continue,
                         };
                         if self.contains_object(name) {
@@ -757,22 +756,44 @@ impl<'ctx> Resolver<'ctx> {
                 });
             }
         }
-        let schema_runtime_ty = kclvm_runtime::schema_runtime_type(name, &self.ctx.pkgpath);
+
+        let schema_full_ty_str = full_ty_str(&self.ctx.pkgpath, name);
+
         if should_add_schema_ref {
             if let Some(ref parent_ty) = parent_ty {
-                let parent_schema_runtime_ty =
-                    kclvm_runtime::schema_runtime_type(&parent_ty.name, &parent_ty.pkgpath);
-                self.ctx
-                    .ty_ctx
-                    .add_dependencies(&schema_runtime_ty, &parent_schema_runtime_ty);
-                if self.ctx.ty_ctx.is_cyclic_from_node(&schema_runtime_ty) {
-                    self.handler.add_compile_error(
-                        &format!(
-                            "There is a circular reference between schema {} and {}",
-                            name, parent_ty.name,
-                        ),
-                        schema_stmt.get_span_pos(),
-                    );
+                let parent_full_ty_str = parent_ty.full_ty_str();
+                self.ctx.ty_ctx.add_dependencies(
+                    &schema_full_ty_str,
+                    &parent_full_ty_str,
+                    schema_stmt.name.get_span_pos(),
+                );
+
+                if self.ctx.ty_ctx.is_cyclic_from_node(&schema_full_ty_str) {
+                    let cycles = self.ctx.ty_ctx.find_cycle_nodes(&schema_full_ty_str);
+                    for cycle in cycles {
+                        let node_names: Vec<String> = cycle
+                            .iter()
+                            .map(|idx| {
+                                if let Some(name) = self.ctx.ty_ctx.dep_graph.node_weight(*idx) {
+                                    name.clone()
+                                } else {
+                                    "".to_string()
+                                }
+                            })
+                            .filter(|name| !name.is_empty())
+                            .collect();
+                        for node in &cycle {
+                            if let Some(range) = self.ctx.ty_ctx.get_node_range(node) {
+                                self.handler.add_compile_error(
+                                    &format!(
+                                        "There is a circular reference between schemas {}",
+                                        node_names.join(", "),
+                                    ),
+                                    range,
+                                );
+                            }
+                        }
+                    }
                 }
             }
         }
@@ -806,6 +827,7 @@ impl<'ctx> Resolver<'ctx> {
             index_signature,
             decorators,
         };
+        let schema_runtime_ty = kclvm_runtime::schema_runtime_type(name, &self.ctx.pkgpath);
         self.ctx
             .schema_mapping
             .insert(schema_runtime_ty, Arc::new(RefCell::new(schema_ty.clone())));
@@ -870,21 +892,41 @@ impl<'ctx> Resolver<'ctx> {
             }
         }
         if should_add_schema_ref {
-            let schema_runtime_ty = kclvm_runtime::schema_runtime_type(name, &self.ctx.pkgpath);
+            let schema_full_ty_str = full_ty_str(&self.ctx.pkgpath, &name);
+
             for parent_ty in &parent_types {
-                let parent_schema_runtime_ty =
-                    kclvm_runtime::schema_runtime_type(&parent_ty.name, &parent_ty.pkgpath);
-                self.ctx
-                    .ty_ctx
-                    .add_dependencies(&schema_runtime_ty, &parent_schema_runtime_ty);
-                if self.ctx.ty_ctx.is_cyclic_from_node(&schema_runtime_ty) {
-                    self.handler.add_compile_error(
-                        &format!(
-                            "There is a circular reference between rule {} and {}",
-                            name, parent_ty.name,
-                        ),
-                        rule_stmt.get_span_pos(),
-                    );
+                let parent_full_ty_str = parent_ty.full_ty_str();
+                self.ctx.ty_ctx.add_dependencies(
+                    &schema_full_ty_str,
+                    &parent_full_ty_str,
+                    rule_stmt.name.get_span_pos(),
+                );
+                if self.ctx.ty_ctx.is_cyclic_from_node(&schema_full_ty_str) {
+                    let cycles = self.ctx.ty_ctx.find_cycle_nodes(&schema_full_ty_str);
+                    for cycle in cycles {
+                        let node_names: Vec<String> = cycle
+                            .iter()
+                            .map(|idx| {
+                                if let Some(name) = self.ctx.ty_ctx.dep_graph.node_weight(*idx) {
+                                    name.clone()
+                                } else {
+                                    "".to_string()
+                                }
+                            })
+                            .filter(|name| !name.is_empty())
+                            .collect();
+                        for node in &cycle {
+                            if let Some(range) = self.ctx.ty_ctx.get_node_range(node) {
+                                self.handler.add_compile_error(
+                                    &format!(
+                                        "There is a circular reference between rules {}",
+                                        node_names.join(", "),
+                                    ),
+                                    range,
+                                );
+                            }
+                        }
+                    }
                 }
             }
         }

diff --git a/kclvm/sema/src/resolver/import.rs b/kclvm/sema/src/resolver/import.rs
@@ -7,7 +7,7 @@ use crate::{
     ty::{Type, TypeKind},
 };
 use indexmap::{IndexMap, IndexSet};
-use kclvm_ast::{ast, MAIN_PKG};
+use kclvm_ast::ast;
 use kclvm_error::*;
 use std::rc::Rc;
 use std::sync::Arc;
@@ -233,22 +233,39 @@ impl<'ctx> Resolver<'ctx> {
                             }
                             let current_pkgpath = self.ctx.pkgpath.clone();
                             let current_filename = self.ctx.filename.clone();
-                            self.ctx
-                                .ty_ctx
-                                .add_dependencies(&self.ctx.pkgpath, &import_stmt.path.node);
+                            self.ctx.ty_ctx.add_dependencies(
+                                &self.ctx.pkgpath,
+                                &import_stmt.path.node,
+                                stmt.get_span_pos(),
+                            );
                             if self.ctx.ty_ctx.is_cyclic_from_node(&self.ctx.pkgpath) {
-                                let pkg_path = if self.ctx.pkgpath == MAIN_PKG {
-                                    self.ctx.filename.clone()
-                                } else {
-                                    self.ctx.pkgpath.clone()
-                                };
-                                self.handler.add_compile_error(
-                                    &format!(
-                                        "There is a circular import reference between module {} and {}",
-                                        pkg_path, import_stmt.path.node,
-                                    ),
-                                    stmt.get_span_pos(),
-                                );
+                                let cycles = self.ctx.ty_ctx.find_cycle_nodes(&self.ctx.pkgpath);
+                                for cycle in cycles {
+                                    let node_names: Vec<String> = cycle
+                                        .iter()
+                                        .map(|idx| {
+                                            if let Some(name) =
+                                                self.ctx.ty_ctx.dep_graph.node_weight(*idx)
+                                            {
+                                                name.clone()
+                                            } else {
+                                                "".to_string()
+                                            }
+                                        })
+                                        .filter(|name| !name.is_empty())
+                                        .collect();
+                                    for node in &cycle {
+                                        if let Some(range) = self.ctx.ty_ctx.get_node_range(node) {
+                                            self.handler.add_compile_error(
+                                                &format!(
+                                                    "There is a circular reference between modules {}",
+                                                    node_names.join(", "),
+                                                ),
+                                                range
+                                            );
+                                        }
+                                    }
+                                }
                             }
                             // Switch pkgpath context
                             if !self.scope_map.contains_key(&import_stmt.path.node) {

diff --git a/kclvm/sema/src/resolver/test_fail_data/cycle_reference/file1.k b/kclvm/sema/src/resolver/test_fail_data/cycle_reference/file1.k
@@ -9,3 +9,12 @@ rule RuleBase(RuleSub):
 
 rule RuleSub(RuleBase):
     True
+
+schema A(B):
+    name: str
+
+schema B(C):
+    name: str
+
+schema C(A):
+    name: str
diff --git a/kclvm/sema/src/resolver/tests.rs b/kclvm/sema/src/resolver/tests.rs
@@ -229,21 +229,25 @@ fn test_resolve_program_cycle_reference_fail() {
     let sess = Arc::new(ParseSession::default());
     let mut program = load_program(
         sess.clone(),
-        &["./src/resolver/test_fail_data/cycle_reference/file1.k"],
+        &["./src/resolver/test_fail_data/cycle_reference/file2.k"],
         None,
         None,
     )
     .unwrap()
     .program;
     let scope = resolve_program(&mut program);
     let err_messages = [
-        "There is a circular import reference between module file1 and file2",
-        // "There is a circular reference between schema SchemaBase and SchemaSub",
-        "There is a circular reference between schema SchemaSub and SchemaBase",
-        // "There is a circular reference between rule RuleBase and RuleSub",
-        "There is a circular reference between rule RuleSub and RuleBase",
-        "Module 'file2' imported but unused",
+        "There is a circular reference between modules file1, file2",
+        "There is a circular reference between modules file1, file2",
+        "There is a circular reference between schemas file1.SchemaBase, file1.SchemaSub",
+        "There is a circular reference between schemas file1.SchemaBase, file1.SchemaSub",
+        "There is a circular reference between rules file1.RuleBase, file1.RuleSub",
+        "There is a circular reference between rules file1.RuleBase, file1.RuleSub",
+        "There is a circular reference between schemas file1.A, file1.B, file1.C",
+        "There is a circular reference between schemas file1.A, file1.B, file1.C",
+        "There is a circular reference between schemas file1.A, file1.B, file1.C",
         "Module 'file1' imported but unused",
+        "Module 'file2' imported but unused",
     ];
     assert_eq!(scope.handler.diagnostics.len(), err_messages.len());
     for (diag, msg) in scope.handler.diagnostics.iter().zip(err_messages.iter()) {

diff --git a/kclvm/sema/src/ty/context.rs b/kclvm/sema/src/ty/context.rs
@@ -2,6 +2,8 @@ use std::collections::HashMap;
 use std::sync::Arc;
 
 use super::{sup, DictType, Type, TypeFlags, TypeKind, TypeRef};
+use kclvm_error::diagnostic::Range;
+use petgraph::algo::kosaraju_scc;
 use petgraph::graph::{DiGraph, NodeIndex};
 use petgraph::visit::{depth_first_search, DfsEvent};
 
@@ -12,6 +14,7 @@ pub struct TypeContext {
     pub dep_graph: DiGraph<String, ()>,
     pub builtin_types: BuiltinTypes,
     node_index_map: HashMap<String, NodeIndex>,
+    node_range_map: HashMap<NodeIndex, Range>,
 }
 
 #[derive(Debug)]
@@ -46,6 +49,7 @@ impl TypeContext {
                 none: Arc::new(Type::NONE),
             },
             node_index_map: HashMap::new(),
+            node_range_map: HashMap::new(),
         }
     }
 
@@ -63,11 +67,32 @@ impl TypeContext {
         .is_err()
     }
 
+    pub fn find_cycle_nodes(&self, node: &String) -> Vec<Vec<NodeIndex>> {
+        let idx = match self.node_index_map.get(node) {
+            Some(idx) => idx,
+            None => return vec![],
+        };
+        let mut res = vec![];
+        let strongly_connected_components: Vec<Vec<NodeIndex>> = kosaraju_scc(&self.dep_graph);
+        for comp in strongly_connected_components {
+            if comp.len() > 1 && comp.contains(idx) {
+                res.push(comp)
+            }
+        }
+        res
+    }
+
+    #[inline]
+    pub fn get_node_range(&self, idx: &NodeIndex) -> Option<Range> {
+        self.node_range_map.get(idx).cloned()
+    }
+
     /// Add dependencies between "from" and "to".
-    pub fn add_dependencies(&mut self, from: &str, to: &str) {
+    pub fn add_dependencies(&mut self, from: &str, to: &str, from_node_range: Range) {
         let from_idx = self.get_or_insert_node_index(from);
         let to_idx = self.get_or_insert_node_index(to);
         self.dep_graph.add_edge(from_idx, to_idx, ());
+        self.node_range_map.insert(from_idx, from_node_range);
     }
 
     /// Get the node index from the node index map or insert it into the dependency graph.

diff --git a/kclvm/sema/src/ty/mod.rs b/kclvm/sema/src/ty/mod.rs
@@ -259,11 +259,7 @@ impl SchemaType {
     }
     /// Get the object type string.
     pub fn full_ty_str(&self) -> String {
-        if self.pkgpath.is_empty() || self.pkgpath == MAIN_PKG {
-            self.name.clone()
-        } else {
-            format!("{}.{}", self.pkgpath, self.name)
-        }
+        full_ty_str(&self.pkgpath, &self.name)
     }
     /// Is `name` a schema member function
     pub fn is_member_functions(&self, name: &str) -> bool {
@@ -375,6 +371,14 @@ impl SchemaType {
     }
 }
 
+pub fn full_ty_str(pkgpath: &str, name: &str) -> String {
+    if pkgpath.is_empty() || pkgpath == MAIN_PKG {
+        name.to_string()
+    } else {
+        format!("{}.{}", pkgpath, name)
+    }
+}
+
 #[derive(Debug, Clone, PartialEq)]
 pub struct SchemaAttr {
     pub is_optional: bool,

diff --git a/test/grammar/import/import_main_file_fail_0/stderr.golden b/test/grammar/import/import_main_file_fail_0/stderr.golden
@@ -1,6 +1,13 @@
+error[E2L23]: CompileError
+ --> ${CWD}/module.k:2:1
+  |
+2 | import main
+  | ^ There is a circular reference between modules module, main
+  |
+
 error[E2L23]: CompileError
  --> ${CWD}/main.k:1:1
   |
 1 | import module
-  | ^ There is a circular import reference between module main and module
+  | ^ There is a circular reference between modules module, main
   |
diff --git a/test/grammar/import/import_main_file_fail_1/stderr.golden b/test/grammar/import/import_main_file_fail_1/stderr.golden
@@ -1,6 +1,3 @@
-error[E2L23]: CompileError
- --> ${CWD}/main.k:1:1
-  |
-1 | import main
-  | ^ There is a circular import reference between module main and main
-  |
+error[E3M38]: error[E1001]: RecursiveLoad
+Could not compiles due to cyclic import statements
+- ${CWD}/main.k
diff --git a/test/grammar/schema/inherit/cycle_inherit_fail_0/stderr.golden b/test/grammar/schema/inherit/cycle_inherit_fail_0/stderr.golden
@@ -1,6 +1,13 @@
+error[E2L23]: CompileError
+ --> ${CWD}/main.k:1:8
+  |
+1 | schema Parent(Son):
+  |        ^ There is a circular reference between schemas Parent, Son
+  |
+
 error[E2L23]: CompileError
  --> ${CWD}/main.k:4:8
   |
 4 | schema Son(Parent):
-  |        ^ There is a circular reference between schema Son and Parent
+  |        ^ There is a circular reference between schemas Parent, Son
   |