diff --git a/src/commands/check_internal/checks.rs b/src/commands/check_internal/checks.rs index 01bb2569..709895f4 100644 --- a/src/commands/check_internal/checks.rs +++ b/src/commands/check_internal/checks.rs @@ -17,7 +17,11 @@ 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 import_module_config.utility { return Ok(()); @@ -170,11 +174,20 @@ 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); @@ -182,10 +195,12 @@ pub(super) fn check_layers( 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 +209,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 +239,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, interface_config: Vec, + layers: Vec, #[case] file_mod_path: &str, #[case] import_mod_path: &str, #[case] expected_result: bool, @@ -246,7 +269,7 @@ mod tests { import_mod_path, &module_tree, file_module.clone(), - &[], + &layers, RootModuleTreatment::Allow, &interface_checker, true, @@ -260,6 +283,7 @@ mod tests { module_tree: ModuleTree, module_config: Vec, interface_config: Vec, + layers: Vec, ) { let file_module = module_tree.find_nearest("domain_one").unwrap(); let interface_checker = Some( @@ -272,7 +296,7 @@ mod tests { "domain_one.subdomain", &module_tree, file_module.clone(), - &[], + &layers, RootModuleTreatment::Allow, &interface_checker, true, @@ -282,24 +306,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, #[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 +355,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 +365,7 @@ 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()); - } - - #[rstest] - fn test_layer_violation_in_check_import(module_tree: ModuleTree, layers: Vec) { - let file_module = module_tree.find_nearest("domain_three").unwrap(); // bottom layer - - let result = check_import( - "domain_one", // trying to import from top layer - &module_tree, - file_module, - &layers, - RootModuleTreatment::Allow, - &None, - true, - ); - - assert!(matches!( - result, - Err(ImportCheckError::LayerViolation { - source_layer, - invalid_layer, - .. - }) if source_layer == "bottom" && invalid_layer == "top" - )); + let result = check_layers(&layers, &source_config, &target_config); + assert!(matches!(result, LayerCheckResult::LayerNotSpecified)); } } diff --git a/src/commands/check_internal/diagnostics.rs b/src/commands/check_internal/diagnostics.rs index f7828073..2d531575 100644 --- a/src/commands/check_internal/diagnostics.rs +++ b/src/commands/check_internal/diagnostics.rs @@ -121,6 +121,9 @@ 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.")] UnnecessarilyIgnoredImport { import_mod_path: String }, diff --git a/src/tests/check_internal.rs b/src/tests/check_internal.rs index 925e5c46..349dcb0f 100644 --- a/src/tests/check_internal.rs +++ b/src/tests/check_internal.rs @@ -40,8 +40,8 @@ pub mod fixtures { config: Some(ModuleConfig { path: "domain_one".to_string(), depends_on: vec![ + DependencyConfig::from_path("domain_two"), DependencyConfig::from_deprecated_path("domain_one.subdomain"), - DependencyConfig::from_path("domain_three"), ], strict: false, layer: Some("top".to_string()), @@ -54,7 +54,7 @@ pub mod fixtures { full_path: "domain_one.subdomain".to_string(), config: Some(ModuleConfig::new_with_layer( "domain_one.subdomain", - "middle", + "top", )), children: HashMap::new(), }), @@ -66,36 +66,44 @@ pub mod fixtures { Arc::new(ModuleNode { is_end_of_path: true, full_path: "domain_two".to_string(), + config: Some(ModuleConfig::new_with_layer("domain_two", "top")), + children: HashMap::new(), + }), + ), + ( + "service_one".to_string(), + Arc::new(ModuleNode { + is_end_of_path: true, + full_path: "service_one".to_string(), config: Some(ModuleConfig { - path: "domain_two".to_string(), - depends_on: vec![DependencyConfig::from_path("domain_one")], + path: "service_one".to_string(), + depends_on: vec![DependencyConfig::from_path( + "service_one.internal", + )], strict: false, - layer: Some("top".to_string()), + layer: Some("middle".to_string()), ..Default::default() }), children: HashMap::from([( - "subdomain".to_string(), + "internal".to_string(), Arc::new(ModuleNode { is_end_of_path: true, - full_path: "domain_two.subdomain".to_string(), - config: Some(ModuleConfig { - path: "domain_two".to_string(), - depends_on: vec![DependencyConfig::from_path("domain_one")], - strict: false, - layer: Some("top".to_string()), - ..Default::default() - }), + full_path: "service_one.internal".to_string(), + config: Some(ModuleConfig::new_with_layer( + "service_one.internal", + "middle", + )), children: HashMap::new(), }), )]), }), ), ( - "domain_three".to_string(), + "data_one".to_string(), Arc::new(ModuleNode { is_end_of_path: true, - full_path: "domain_three".to_string(), - config: Some(ModuleConfig::new_with_layer("domain_three", "bottom")), + full_path: "data_one".to_string(), + config: Some(ModuleConfig::new_with_layer("data_one", "bottom")), children: HashMap::new(), }), ), @@ -110,22 +118,48 @@ pub mod fixtures { ModuleConfig { path: "domain_one".to_string(), depends_on: vec![ + DependencyConfig::from_path("domain_two"), DependencyConfig::from_deprecated_path("domain_one.subdomain"), - DependencyConfig::from_path("domain_three"), ], strict: false, layer: Some("top".to_string()), ..Default::default() }, - ModuleConfig::new_with_layer("domain_one.subdomain", "middle"), + ModuleConfig { + path: "domain_one.subdomain".to_string(), + depends_on: vec![], + strict: false, + layer: Some("top".to_string()), + ..Default::default() + }, ModuleConfig { path: "domain_two".to_string(), - depends_on: vec![DependencyConfig::from_path("domain_one")], + depends_on: vec![], strict: false, layer: Some("top".to_string()), ..Default::default() }, - ModuleConfig::new_with_layer("domain_three", "bottom"), + ModuleConfig { + path: "service_one".to_string(), + depends_on: vec![DependencyConfig::from_path("service_one.internal")], + strict: false, + layer: Some("middle".to_string()), + ..Default::default() + }, + ModuleConfig { + path: "service_one.internal".to_string(), + depends_on: vec![], + strict: false, + layer: Some("middle".to_string()), + ..Default::default() + }, + ModuleConfig { + path: "data_one".to_string(), + depends_on: vec![], + strict: false, + layer: Some("bottom".to_string()), + ..Default::default() + }, ] } diff --git a/tach.toml b/tach.toml index 89934247..720e7931 100644 --- a/tach.toml +++ b/tach.toml @@ -39,28 +39,13 @@ layer = "core" [[modules]] path = "tach.check_external" -depends_on = [ - "tach.extension", -] +depends_on = [] layer = "commands" [[modules]] path = "tach.cli" depends_on = [ "tach", - "tach.cache", - "tach.check_external", - "tach.extension", - "tach.filesystem", - "tach.icons", - "tach.logging", - "tach.mod", - "tach.modularity", - "tach.parsing", - "tach.report", - "tach.show", - "tach.sync", - "tach.test", ] layer = "ui" @@ -126,21 +111,12 @@ layer = "core" [[modules]] path = "tach.mod" -depends_on = [ - "tach.filesystem", - "tach.interactive", - "tach.parsing", -] +depends_on = [] layer = "commands" [[modules]] path = "tach.modularity" -depends_on = [ - "tach.extension", - "tach.filesystem", - "tach.filesystem.git_ops", - "tach.parsing", -] +depends_on = [] layer = "commands" [[modules]] @@ -163,18 +139,12 @@ layer = "core" [[modules]] path = "tach.report" -depends_on = [ - "tach.extension", - "tach.filesystem", -] +depends_on = [] layer = "commands" [[modules]] path = "tach.show" -depends_on = [ - "tach.extension", - "tach.filesystem", -] +depends_on = [] layer = "commands" [[modules]] @@ -186,10 +156,7 @@ layer = "ui" [[modules]] path = "tach.sync" -depends_on = [ - "tach.extension", - "tach.filesystem", -] +depends_on = [] layer = "commands" [[modules]]