diff --git a/src/packs/checker.rs b/src/packs/checker.rs index bac25933..6a7a7f4d 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -68,7 +68,7 @@ pub(crate) trait CheckerInterface { &self, reference: &Reference, configuration: &Configuration, - ) -> Option; + ) -> anyhow::Result>; fn is_strict_mode_violation( &self, @@ -415,19 +415,24 @@ fn get_all_violations( debug!("Running checkers on resolved references"); - let violations: HashSet = checkers + let violations = checkers .into_par_iter() - .flat_map(|c| { - references - .par_iter() - .flat_map(|r| c.check(r, configuration)) - .collect::>() + .try_fold(HashSet::new, |mut acc, c| { + for reference in &references { + if let Some(violation) = c.check(reference, configuration)? { + acc.insert(violation); + } + } + Ok(acc) }) - .collect(); + .try_reduce(HashSet::new, |mut acc, v| { + acc.extend(v); + Ok(acc) + }); debug!("Finished running checkers"); - Ok(violations) + violations } fn get_checkers( diff --git a/src/packs/checker/architecture.rs b/src/packs/checker/architecture.rs index ad7d5c1e..1a10821d 100644 --- a/src/packs/checker/architecture.rs +++ b/src/packs/checker/architecture.rs @@ -117,7 +117,7 @@ impl CheckerInterface for Checker { &self, reference: &Reference, configuration: &Configuration, - ) -> Option { + ) -> anyhow::Result> { let pack_set = &configuration.pack_set; let referencing_pack = &reference.referencing_pack(pack_set); @@ -125,24 +125,24 @@ impl CheckerInterface for Checker { let relative_defining_file = &reference.relative_defining_file; let referencing_pack_name = &referencing_pack.name; - let defining_pack = &reference.defining_pack(pack_set); + let defining_pack = &reference.defining_pack(pack_set)?; if defining_pack.is_none() { - return None; + return Ok(None); } let defining_pack = defining_pack.unwrap(); if referencing_pack.enforce_architecture().is_false() { - return None; + return Ok(None); } let defining_pack_name = &defining_pack.name; if relative_defining_file.is_none() { - return None; + return Ok(None); } if referencing_pack_name == defining_pack_name { - return None; + return Ok(None); } match (&referencing_pack.layer, &defining_pack.layer) { @@ -152,7 +152,7 @@ impl CheckerInterface for Checker { .can_depend_on(referencing_layer, defining_layer) .unwrap() { - return None; + return Ok(None); } let message = format!( @@ -177,12 +177,12 @@ impl CheckerInterface for Checker { defining_pack_name: defining_pack_name.clone(), }; - Some(Violation { + Ok(Some(Violation { message, identifier, - }) + })) } - _ => None, + _ => Ok(None), } } @@ -262,7 +262,7 @@ mod tests { .unwrap(), ..Configuration::default() }; - assert_eq!(None, checker.check(&reference, &configuration)) + assert_eq!(None, checker.check(&reference, &configuration).unwrap()) } #[test] @@ -330,7 +330,7 @@ mod tests { }; assert_eq!( expected_violation, - checker.check(&reference, &configuration).unwrap() + checker.check(&reference, &configuration).unwrap().unwrap() ) } @@ -387,7 +387,7 @@ mod tests { ..Configuration::default() }; - assert_eq!(None, checker.check(&reference, &configuration)) + assert_eq!(None, checker.check(&reference, &configuration).unwrap()) } struct ArchitectureTestCase { diff --git a/src/packs/checker/dependency.rs b/src/packs/checker/dependency.rs index 865f34c1..dcf48afe 100644 --- a/src/packs/checker/dependency.rs +++ b/src/packs/checker/dependency.rs @@ -109,26 +109,27 @@ impl CheckerInterface for Checker { &self, reference: &Reference, configuration: &Configuration, - ) -> Option { + ) -> anyhow::Result> { let referencing_pack = reference.referencing_pack(&configuration.pack_set); if referencing_pack.enforce_dependencies().is_false() { - return None; + return Ok(None); } let referencing_pack_name = &referencing_pack.name; - let defining_pack = &reference.defining_pack(&configuration.pack_set); + let defining_pack = + &reference.defining_pack(&configuration.pack_set)?; if defining_pack.is_none() { - return None; + return Ok(None); } let defining_pack = defining_pack.unwrap(); let defining_pack_name = &defining_pack.name; if referencing_pack_name == defining_pack_name { - return None; + return Ok(None); } let referencing_pack_dependencies = &referencing_pack.dependencies; @@ -170,13 +171,13 @@ impl CheckerInterface for Checker { defining_pack_name: defining_pack_name.clone(), }; - return Some(Violation { + return Ok(Some(Violation { message, identifier, - }); + })); } - None + Ok(None) } fn is_strict_mode_violation( @@ -224,7 +225,7 @@ mod tests { )), source_location: SourceLocation { line: 3, column: 1 }, }; - assert_eq!(None, checker.check(&reference, &configuration)) + assert_eq!(None, checker.check(&reference, &configuration).unwrap()) } #[test] @@ -251,7 +252,7 @@ mod tests { }; assert_eq!( expected_violation, - checker.check(&reference, &configuration).unwrap() + checker.check(&reference, &configuration).unwrap().unwrap() ) } @@ -267,7 +268,7 @@ mod tests { .unwrap(); let reference = build_foo_reference_bar_reference(); - assert_eq!(checker.check(&reference, &configuration), None) + assert_eq!(checker.check(&reference, &configuration).unwrap(), None) } fn build_foo_reference_bar_reference() -> Reference { diff --git a/src/packs/checker/folder_visibility.rs b/src/packs/checker/folder_visibility.rs index af8d4750..85a01e7a 100644 --- a/src/packs/checker/folder_visibility.rs +++ b/src/packs/checker/folder_visibility.rs @@ -11,16 +11,16 @@ impl CheckerInterface for Checker { &self, reference: &Reference, configuration: &Configuration, - ) -> Option { + ) -> anyhow::Result> { let pack_set = &configuration.pack_set; let referencing_pack = &reference.referencing_pack(pack_set); let relative_defining_file = &reference.relative_defining_file; if relative_defining_file.is_none() { - return None; + return Ok(None); } - let defining_pack = &reference.defining_pack(pack_set); + let defining_pack = &reference.defining_pack(pack_set)?; if defining_pack.is_none() { - return None; + return Ok(None); } let defining_pack = defining_pack.unwrap(); if !folder_visible(referencing_pack, defining_pack).unwrap() { @@ -40,12 +40,12 @@ impl CheckerInterface for Checker { referencing_pack_name: referencing_pack.name.clone(), defining_pack_name: defining_pack.name.clone(), }; - Some(Violation { + Ok(Some(Violation { message, identifier, - }) + })) } else { - None + Ok(None) } } diff --git a/src/packs/checker/privacy.rs b/src/packs/checker/privacy.rs index 4f32b424..1fbf73dd 100644 --- a/src/packs/checker/privacy.rs +++ b/src/packs/checker/privacy.rs @@ -9,37 +9,38 @@ impl CheckerInterface for Checker { &self, reference: &Reference, configuration: &Configuration, - ) -> Option { + ) -> anyhow::Result> { let referencing_pack = &reference.referencing_pack(&configuration.pack_set); let relative_defining_file = &reference.relative_defining_file; let referencing_pack_name = &referencing_pack.name; - let defining_pack = &reference.defining_pack(&configuration.pack_set); + let defining_pack = + &reference.defining_pack(&configuration.pack_set)?; if defining_pack.is_none() { - return None; + return Ok(None); } let defining_pack = defining_pack.unwrap(); if defining_pack.enforce_privacy().is_false() { - return None; + return Ok(None); } if defining_pack .ignored_private_constants .contains(&reference.constant_name) { - return None; + return Ok(None); } let defining_pack_name = &defining_pack.name; if relative_defining_file.is_none() { - return None; + return Ok(None); } if referencing_pack_name == defining_pack_name { - return None; + return Ok(None); } // This is a hack for now – we need to read package.yml file public_paths at some point, @@ -58,7 +59,7 @@ impl CheckerInterface for Checker { // Later we might want to add some sort of validation that a constant can be in the public folder OR in the list of private_constants, // but not both. if is_public { - return None; + return Ok(None); } let private_constants = &defining_pack.private_constants; @@ -75,7 +76,7 @@ impl CheckerInterface for Checker { }); if !constant_is_private && !constant_is_in_private_namespace { - return None; + return Ok(None); } } @@ -108,10 +109,10 @@ impl CheckerInterface for Checker { defining_pack_name: defining_pack_name.clone(), }; - Some(Violation { + Ok(Some(Violation { message, identifier, - }) + })) } fn is_strict_mode_violation( @@ -185,7 +186,7 @@ mod tests { ..Configuration::default() }; - assert_eq!(None, checker.check(&reference, &configuration)) + assert_eq!(None, checker.check(&reference, &configuration).unwrap()) } #[test] @@ -247,7 +248,7 @@ mod tests { assert_eq!( expected_violation, - checker.check(&reference, &configuration).unwrap() + checker.check(&reference, &configuration).unwrap().unwrap() ) } @@ -297,7 +298,7 @@ mod tests { ..Configuration::default() }; - assert_eq!(None, checker.check(&reference, &configuration)) + assert_eq!(None, checker.check(&reference, &configuration).unwrap()) } #[test] @@ -359,7 +360,7 @@ mod tests { assert_eq!( expected_violation, - checker.check(&reference, &configuration).unwrap() + checker.check(&reference, &configuration).unwrap().unwrap() ) } @@ -409,7 +410,7 @@ mod tests { ..Configuration::default() }; - assert_eq!(None, checker.check(&reference, &configuration)) + assert_eq!(None, checker.check(&reference, &configuration).unwrap()) } #[test] @@ -474,7 +475,7 @@ mod tests { assert_eq!( expected_violation, - checker.check(&reference, &configuration).unwrap() + checker.check(&reference, &configuration).unwrap().unwrap() ) } @@ -540,7 +541,7 @@ mod tests { assert_eq!( expected_violation, - checker.check(&reference, &configuration).unwrap() + checker.check(&reference, &configuration).unwrap().unwrap() ) } @@ -593,7 +594,7 @@ mod tests { ..Configuration::default() }; - assert_eq!(None, checker.check(&reference, &configuration)); + assert_eq!(None, checker.check(&reference, &configuration).unwrap()); } #[test] @@ -643,7 +644,7 @@ mod tests { .unwrap(), ..Configuration::default() }; - assert_eq!(None, checker.check(&reference, &configuration)) + assert_eq!(None, checker.check(&reference, &configuration).unwrap()); } #[test] @@ -694,6 +695,6 @@ mod tests { .unwrap(), ..Configuration::default() }; - assert_eq!(None, checker.check(&reference, &configuration)) + assert_eq!(None, checker.check(&reference, &configuration).unwrap()) } } diff --git a/src/packs/checker/reference.rs b/src/packs/checker/reference.rs index 069b10ee..0311a860 100644 --- a/src/packs/checker/reference.rs +++ b/src/packs/checker/reference.rs @@ -1,6 +1,6 @@ use std::path::Path; -use anyhow::bail; +use anyhow::{bail, Context}; use crate::packs::{ constant_resolver::ConstantResolver, pack::Pack, @@ -18,11 +18,19 @@ pub struct Reference { } impl Reference { - pub fn defining_pack<'a>(&self, pack_set: &'a PackSet) -> Option<&'a Pack> { + pub fn defining_pack<'a>( + &self, + pack_set: &'a PackSet, + ) -> anyhow::Result> { if let Some(name) = &self.defining_pack_name { - Some(pack_set.for_pack(name).unwrap_or_else(|_| panic!("Reference#defining_pack_name is {}, but that pack is not found in pack set.", &name))) + Ok(Some(pack_set + .for_pack(name) + .context(format!( + "Reference#defining_pack_name is {}, but that pack is not found in pack set.", + &name + ))?)) } else { - None + Ok(None) } } diff --git a/src/packs/checker/visibility.rs b/src/packs/checker/visibility.rs index c85e0aec..2886bfa3 100644 --- a/src/packs/checker/visibility.rs +++ b/src/packs/checker/visibility.rs @@ -14,20 +14,21 @@ impl CheckerInterface for Checker { &self, reference: &Reference, configuration: &Configuration, - ) -> Option { + ) -> anyhow::Result> { let referencing_pack = &reference.referencing_pack(&configuration.pack_set); let relative_defining_file = &reference.relative_defining_file; let referencing_pack_name = &referencing_pack.name; - let defining_pack = &reference.defining_pack(&configuration.pack_set); + let defining_pack = + &reference.defining_pack(&configuration.pack_set)?; if defining_pack.is_none() { - return None; + return Ok(None); } let defining_pack = defining_pack.unwrap(); if defining_pack.enforce_visibility().is_false() { - return None; + return Ok(None); } if defining_pack @@ -36,17 +37,17 @@ impl CheckerInterface for Checker { .unwrap_or(&HashSet::new()) .contains(referencing_pack_name) { - return None; + return Ok(None); } let defining_pack_name = &defining_pack.name; if relative_defining_file.is_none() { - return None; + return Ok(None); } if referencing_pack_name == defining_pack_name { - return None; + return Ok(None); } let message = format!( @@ -69,10 +70,10 @@ impl CheckerInterface for Checker { defining_pack_name: defining_pack_name.clone(), }; - Some(Violation { + Ok(Some(Violation { message, identifier, - }) + })) } fn is_strict_mode_violation( @@ -146,7 +147,7 @@ mod tests { ..Configuration::default() }; - assert_eq!(None, checker.check(&reference, &configuration)) + assert_eq!(None, checker.check(&reference, &configuration).unwrap()) } #[test] @@ -206,7 +207,7 @@ mod tests { }; assert_eq!( expected_violation, - checker.check(&reference, &configuration).unwrap() + checker.check(&reference, &configuration).unwrap().unwrap() ) } @@ -258,6 +259,6 @@ mod tests { .unwrap(), ..Configuration::default() }; - assert_eq!(None, checker.check(&reference, &configuration)) + assert_eq!(None, checker.check(&reference, &configuration).unwrap()); } }