Merge branch 'main' into add-username-to-report-upload

Makefile

@@ -60,8 +60,8 @@ fmt-python: ## Format Python code
 
 
 fmt-rust: ## Format Rust code
-	cargo fmt --all
 	cargo clippy --fix --allow-dirty --allow-staged
+	cargo fmt --all
 
 
 lint-python: ## Lint Python code

src/commands/check_internal/checks.rs

@@ -17,17 +17,25 @@ fn check_dependencies(
     layers: &[String],
 ) -> Result<(), ImportCheckError> {
     // Layer check should take precedence over other dependency checks
-    check_layers(layers, file_module_config, import_module_config)?;
+    match check_layers(layers, file_module_config, import_module_config) {
+        LayerCheckResult::Ok => return Ok(()), // Higher layers can unconditionally import lower layers
+        LayerCheckResult::SameLayer | LayerCheckResult::LayerNotSpecified => (), // We need to do further processing to determine if the dependency is allowed
+        LayerCheckResult::LayerViolation(e) | LayerCheckResult::UnknownLayer(e) => return Err(e),
+    };
+
+    if file_module_config.depends_on.is_none() {
+        return Ok(());
+    }
 
     if import_module_config.utility {
         return Ok(());
     }
+
     let file_nearest_module_path = &file_module_config.path;
     let import_nearest_module_path = &import_module_config.path;
 
     match file_module_config
-        .depends_on
-        .iter()
+        .dependencies_iter()
         .find(|dep| &dep.path == import_nearest_module_path)
     {
         Some(DependencyConfig {
@@ -151,7 +159,7 @@ pub(super) fn check_unused_ignore_directive(
         interface_checker,
         check_dependencies,
     ) {
-        Ok(()) => Err(ImportCheckError::UnusedIgnoreDirective {
+        Ok(()) => Err(ImportCheckError::UnnecessarilyIgnoredImport {
             import_mod_path: directive_ignored_import.import.module_path.to_string(),
         }),
         Err(_) => Ok(()),
@@ -170,22 +178,33 @@ pub(super) fn check_missing_ignore_directive_reason(
     }
 }
 
-pub(super) fn check_layers(
+#[derive(Debug)]
+enum LayerCheckResult {
+    Ok,
+    SameLayer,
+    LayerNotSpecified,
+    LayerViolation(ImportCheckError),
+    UnknownLayer(ImportCheckError),
+}
+
+fn check_layers(
     layers: &[String],
     source_module_config: &ModuleConfig,
     target_module_config: &ModuleConfig,
-) -> Result<(), ImportCheckError> {
+) -> LayerCheckResult {
     match (&source_module_config.layer, &target_module_config.layer) {
         (Some(source_layer), Some(target_layer)) => {
             let source_index = layers.iter().position(|layer| layer == source_layer);
             let target_index = layers.iter().position(|layer| layer == target_layer);
 
             match (source_index, target_index) {
                 (Some(source_index), Some(target_index)) => {
-                    if source_index <= target_index {
-                        Ok(())
+                    if source_index == target_index {
+                        LayerCheckResult::SameLayer
+                    } else if source_index < target_index {
+                        LayerCheckResult::Ok
                     } else {
-                        Err(ImportCheckError::LayerViolation {
+                        LayerCheckResult::LayerViolation(ImportCheckError::LayerViolation {
                             import_mod_path: target_module_config.path.clone(),
                             source_module: source_module_config.path.clone(),
                             source_layer: source_layer.clone(),
@@ -194,17 +213,26 @@ pub(super) fn check_layers(
                         })
                     }
                 }
-                // If either index is not found, the layer is unknown -- ignore for now
-                _ => Ok(()),
+                // If either index is not found, the layer is unknown
+                (Some(_), None) => LayerCheckResult::UnknownLayer(ImportCheckError::UnknownLayer {
+                    layer: target_layer.clone(),
+                }),
+                (None, Some(_)) => LayerCheckResult::UnknownLayer(ImportCheckError::UnknownLayer {
+                    layer: source_layer.clone(),
+                }),
+                _ => LayerCheckResult::UnknownLayer(ImportCheckError::UnknownLayer {
+                    layer: source_layer.clone(),
+                }),
             }
         }
-        _ => Ok(()),
+        _ => LayerCheckResult::LayerNotSpecified, // At least one module does not have a layer
     }
 }
 
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::commands::check_internal::diagnostics::ImportCheckError;
     use crate::config::{InterfaceConfig, ModuleConfig};
     use crate::modules::ModuleTree;
     use crate::tests::check_internal::fixtures::{
@@ -215,22 +243,21 @@ mod tests {
     use rstest::rstest;
 
     #[rstest]
-    #[case("domain_one", "domain_one", true)]
-    #[case("domain_one", "domain_one.core", true)]
-    #[case("domain_one", "domain_three", true)]
-    #[case("domain_two", "domain_one", true)]
-    #[case("domain_two", "domain_one.public_fn", true)]
-    #[case("domain_two.subdomain", "domain_one", true)]
-    #[case("domain_two", "domain_one.private_fn", false)]
-    #[case("domain_three", "domain_one", false)]
-    #[case("domain_two", "domain_one.core", false)]
-    #[case("domain_two.subdomain", "domain_one.core", false)]
-    #[case("domain_two", "domain_three", false)]
-    #[case("domain_two", "domain_two.subdomain", false)]
+    #[case("domain_one", "domain_two", true)] // same layer, explicit dependency
+    #[case("domain_one", "domain_one.subdomain", false)] // same layer, parent->child (deprecated)
+    #[case("domain_one.subdomain", "domain_one", false)] // same layer, child->parent not allowed
+    #[case("domain_one", "service_one", true)] // top->middle allowed
+    #[case("domain_one", "data_one", true)] // top->bottom allowed
+    #[case("service_one", "service_one.internal", true)] // same layer, explicit dependency
+    #[case("service_one", "data_one", true)] // middle->bottom allowed
+    #[case("service_one", "domain_one", false)] // middle->top denied
+    #[case("data_one", "service_one", false)] // bottom->middle denied
+    #[case("data_one", "domain_one", false)] // bottom->top denied
     fn test_check_import(
         module_tree: ModuleTree,
         module_config: Vec<ModuleConfig>,
         interface_config: Vec<InterfaceConfig>,
+        layers: Vec<String>,
         #[case] file_mod_path: &str,
         #[case] import_mod_path: &str,
         #[case] expected_result: bool,
@@ -246,7 +273,7 @@ mod tests {
             import_mod_path,
             &module_tree,
             file_module.clone(),
-            &[],
+            &layers,
             RootModuleTreatment::Allow,
             &interface_checker,
             true,
@@ -260,6 +287,7 @@ mod tests {
         module_tree: ModuleTree,
         module_config: Vec<ModuleConfig>,
         interface_config: Vec<InterfaceConfig>,
+        layers: Vec<String>,
     ) {
         let file_module = module_tree.find_nearest("domain_one").unwrap();
         let interface_checker = Some(
@@ -272,7 +300,7 @@ mod tests {
             "domain_one.subdomain",
             &module_tree,
             file_module.clone(),
-            &[],
+            &layers,
             RootModuleTreatment::Allow,
             &interface_checker,
             true,
@@ -282,24 +310,47 @@ mod tests {
     }
 
     #[rstest]
-    #[case("top", "top", true)]
-    #[case("top", "middle", true)]
-    #[case("top", "bottom", true)]
-    #[case("middle", "bottom", true)]
-    #[case("bottom", "top", false)]
-    #[case("middle", "top", false)]
-    #[case("bottom", "middle", false)]
+    #[case("top", "top", LayerCheckResult::SameLayer)]
+    #[case("top", "middle", LayerCheckResult::Ok)]
+    #[case("top", "bottom", LayerCheckResult::Ok)]
+    #[case("middle", "bottom", LayerCheckResult::Ok)]
+    #[case("bottom", "top", LayerCheckResult::LayerViolation(ImportCheckError::LayerViolation {
+        source_layer: "bottom".to_string(),
+        invalid_layer: "top".to_string(),
+        import_mod_path: "".to_string(),
+        source_module: "".to_string(),
+        invalid_module: "".to_string(),
+    }))]
+    #[case("middle", "top", LayerCheckResult::LayerViolation(ImportCheckError::LayerViolation {
+        source_layer: "middle".to_string(),
+        invalid_layer: "top".to_string(),
+        import_mod_path: "".to_string(),
+        source_module: "".to_string(),
+        invalid_module: "".to_string(),
+    }))]
+    #[case("bottom", "middle", LayerCheckResult::LayerViolation(ImportCheckError::LayerViolation {
+        source_layer: "bottom".to_string(),
+        invalid_layer: "middle".to_string(),
+        import_mod_path: "".to_string(),
+        source_module: "".to_string(),
+        invalid_module: "".to_string(),
+    }))]
     fn test_check_layers_hierarchy(
         layers: Vec<String>,
         #[case] source_layer: &str,
         #[case] target_layer: &str,
-        #[case] expected_result: bool,
+        #[case] expected_pattern: LayerCheckResult,
     ) {
         let source_config = ModuleConfig::new_with_layer("source", source_layer);
         let target_config = ModuleConfig::new_with_layer("target", target_layer);
 
         let result = check_layers(&layers, &source_config, &target_config);
-        assert_eq!(result.is_ok(), expected_result);
+        match (result, expected_pattern) {
+            (LayerCheckResult::Ok, LayerCheckResult::Ok) => (),
+            (LayerCheckResult::SameLayer, LayerCheckResult::SameLayer) => (),
+            (LayerCheckResult::LayerViolation(_), LayerCheckResult::LayerViolation(_)) => (),
+            (actual, expected) => panic!("Expected {:?} but got {:?}", expected, actual),
+        }
     }
 
     #[rstest]
@@ -308,7 +359,8 @@ mod tests {
         let source_config = ModuleConfig::new_with_layer("source", "any");
         let target_config = ModuleConfig::new_with_layer("target", "any");
 
-        assert!(check_layers(&layers, &source_config, &target_config).is_ok());
+        let result = check_layers(&layers, &source_config, &target_config);
+        assert!(matches!(result, LayerCheckResult::UnknownLayer(_)));
     }
 
     #[rstest]
@@ -317,31 +369,44 @@ mod tests {
         let source_config = ModuleConfig::default();
         let target_config = ModuleConfig::default();
 
-        // When modules don't specify layers, they should be allowed
-        assert!(check_layers(&layers, &source_config, &target_config).is_ok());
+        let result = check_layers(&layers, &source_config, &target_config);
+        assert!(matches!(result, LayerCheckResult::LayerNotSpecified));
     }
 
     #[rstest]
-    fn test_layer_violation_in_check_import(module_tree: ModuleTree, layers: Vec<String>) {
-        let file_module = module_tree.find_nearest("domain_three").unwrap(); // bottom layer
+    #[case("unrestricted", "domain_one", false)] // middle->top denied due to layer check
+    #[case("unrestricted", "service_one", true)] // same layer allowed
+    #[case("unrestricted", "data_one", true)] // middle->bottom allowed
+    fn test_check_import_unrestricted_dependencies(
+        module_tree: ModuleTree,
+        module_config: Vec<ModuleConfig>,
+        interface_config: Vec<InterfaceConfig>,
+        layers: Vec<String>,
+        #[case] file_mod_path: &str,
+        #[case] import_mod_path: &str,
+        #[case] expected_result: bool,
+    ) {
+        let file_module = module_tree.find_nearest(file_mod_path).unwrap();
+        let interface_checker = Some(
+            InterfaceChecker::new(&interface_config)
+                .with_type_check_cache(&module_config, &[PathBuf::from(".")])
+                .unwrap(),
+        );
 
-        let result = check_import(
-            "domain_one", // trying to import from top layer
+        let check_error = check_import(
+            import_mod_path,
             &module_tree,
-            file_module,
+            file_module.clone(),
             &layers,
             RootModuleTreatment::Allow,
-            &None,
+            &interface_checker,
             true,
         );
-
-        assert!(matches!(
-            result,
-            Err(ImportCheckError::LayerViolation {
-                source_layer,
-                invalid_layer,
-                ..
-            }) if source_layer == "bottom" && invalid_layer == "top"
-        ));
+        let result = check_error.is_ok();
+        assert_eq!(
+            result, expected_result,
+            "Expected import from '{}' to '{}' to be {}, but got {}",
+            file_mod_path, import_mod_path, expected_result, result
+        );
     }
 }

src/commands/check_internal/diagnostics.rs

@@ -121,8 +121,14 @@ pub enum ImportCheckError {
         invalid_layer: String,
     },
 
+    #[error("Layer '{layer}' is not defined in the project.")]
+    UnknownLayer { layer: String },
+
     #[error("Import '{import_mod_path}' is unnecessarily ignored by a directive.")]
-    UnusedIgnoreDirective { import_mod_path: String },
+    UnnecessarilyIgnoredImport { import_mod_path: String },
+
+    #[error("Ignore directive is unused.")]
+    UnusedIgnoreDirective(),
 
     #[error("Import '{import_mod_path}' is ignored without providing a reason.")]
     MissingIgnoreDirectiveReason { import_mod_path: String },

src/commands/check_internal/mod.rs

@@ -1,6 +1,7 @@
 pub mod checks;
 use checks::{check_import, check_missing_ignore_directive_reason, check_unused_ignore_directive};
 pub mod diagnostics;
+use diagnostics::ImportCheckError;
 pub use diagnostics::{BoundaryError, CheckDiagnostics};
 pub mod error;
 pub use error::CheckError;
@@ -162,6 +163,29 @@ fn process_file(
             }
         });
 
+    project_imports
+        .unused_ignore_directives()
+        .for_each(
+            |ignore_directive| match project_config.rules.unused_ignore_directives {
+                RuleSetting::Error => {
+                    diagnostics.errors.push(BoundaryError {
+                        file_path: file_path.clone(),
+                        line_number: ignore_directive.line_no,
+                        import_mod_path: ignore_directive.modules.join(", "),
+                        error_info: ImportCheckError::UnusedIgnoreDirective(),
+                    });
+                }
+                RuleSetting::Warn => {
+                    diagnostics.warnings.push(format!(
+                        "Unused ignore directive: '{}' in file '{}'",
+                        ignore_directive.modules.join(","),
+                        file_path.display()
+                    ));
+                }
+                RuleSetting::Off => {}
+            },
+        );
+
     Some(diagnostics)
 }
 

src/commands/sync.rs

@@ -88,7 +88,7 @@ pub fn sync_dependency_constraints(
                 new_modules.push(module.with_no_dependencies());
             }
             // Track deprecations for each module
-            for dependency in module.depends_on.iter() {
+            for dependency in module.dependencies_iter() {
                 if dependency.deprecated {
                     deprecation_map
                         .entry(module.path.clone())

src/commands/test.rs

@@ -104,7 +104,7 @@ impl TachPytestPluginHandler {
 fn build_module_consumer_map(modules: &Vec<ModuleConfig>) -> HashMap<&String, Vec<String>> {
     let mut consumer_map: HashMap<&String, Vec<String>> = HashMap::new();
     for module in modules {
-        for dependency in &module.depends_on {
+        for dependency in module.dependencies_iter() {
             consumer_map
                 .entry(&dependency.path)
                 .or_default()