diff --git a/backends/src/rust/mod.rs b/backends/src/rust/mod.rs index 13404ad7..900c427d 100644 --- a/backends/src/rust/mod.rs +++ b/backends/src/rust/mod.rs @@ -3,7 +3,7 @@ use super::{ WalkDirResult, }; use anyhow::Result; -use cargo_metadata::Package; +use cargo_metadata::{Metadata, Package}; use necessist_core::{ framework::{SpanTestMaps, TestSet}, LightContext, SourceFile, Span, ToInternalSpan, __Rewriter as Rewriter, @@ -31,6 +31,9 @@ use visitor::{collect_local_functions, visit}; #[derive(Debug)] pub struct Rust { + source_file_fs_module_path_cache: BTreeMap>, + source_file_package_cache: BTreeMap, + directory_metadata_cache: BTreeMap, source_file_flags_cache: BTreeMap>, } @@ -45,6 +48,9 @@ impl Rust { pub fn new() -> Self { Self { + source_file_fs_module_path_cache: BTreeMap::new(), + source_file_package_cache: BTreeMap::new(), + directory_metadata_cache: BTreeMap::new(), source_file_flags_cache: BTreeMap::new(), } } @@ -59,13 +65,22 @@ pub struct Test<'ast> { impl<'ast> Test<'ast> { fn new( storage: &RefCell, + source_file_fs_module_path_cache: &mut BTreeMap>, + source_file_package_cache: &mut BTreeMap, + directory_metadata_map: &mut BTreeMap, source_file: &SourceFile, item_fn: &'ast syn::ItemFn, ) -> Option { // smoelius: If the module path cannot be determined, return `None` to prevent the // `GenericVisitor` from walking the test. let test_name = item_fn.sig.ident.to_string(); - let result = storage.borrow_mut().test_path(source_file, &test_name); + let result = storage.borrow_mut().test_path( + source_file_fs_module_path_cache, + source_file_package_cache, + directory_metadata_map, + source_file, + &test_name, + ); let test_path = match result { Ok(test_path) => test_path, Err(error) => { @@ -658,15 +673,15 @@ impl Rust { } #[cfg_attr(dylint_lib = "general", allow(non_local_effect_before_error_return))] - fn cached_source_file_flags( - &mut self, - source_file_package_map: &mut BTreeMap, - source_file: &Path, - ) -> Result<&Vec> { + fn cached_source_file_flags(&mut self, source_file: &Path) -> Result<&Vec> { self.source_file_flags_cache .entry(source_file.to_path_buf()) .or_try_insert_with(|| { - let package = cached_source_file_package(source_file_package_map, source_file)?; + let package = cached_source_file_package( + &mut self.source_file_package_cache, + &mut self.directory_metadata_cache, + source_file, + )?; let mut flags = vec![ "--manifest-path".to_owned(), diff --git a/backends/src/rust/storage.rs b/backends/src/rust/storage.rs index a4cdc661..76768d2a 100644 --- a/backends/src/rust/storage.rs +++ b/backends/src/rust/storage.rs @@ -1,6 +1,6 @@ use super::TryInsert; use anyhow::{anyhow, Error, Result}; -use cargo_metadata::{MetadataCommand, Package}; +use cargo_metadata::{Metadata, MetadataCommand, Package}; use necessist_core::{util, SourceFile}; use std::{ collections::BTreeMap, @@ -12,8 +12,6 @@ use syn::{File, Ident}; /// Structures needed during parsing but not after. pub struct Storage<'ast> { pub module_path: Vec<&'ast Ident>, - pub source_file_fs_module_path_cache: BTreeMap>, - pub source_file_package_cache: BTreeMap, pub tests_needing_warnings: BTreeMap>, pub error: Option, } @@ -22,17 +20,23 @@ impl<'ast> Storage<'ast> { pub fn new(_file: &'ast File) -> Self { Self { module_path: Vec::new(), - source_file_fs_module_path_cache: BTreeMap::new(), - source_file_package_cache: BTreeMap::new(), tests_needing_warnings: BTreeMap::new(), error: None, } } - pub fn test_path(&mut self, source_file: &SourceFile, name: &str) -> Result> { + pub fn test_path( + &mut self, + source_file_fs_module_path_map: &mut BTreeMap>, + source_file_package_map: &mut BTreeMap, + directory_metadata_map: &mut BTreeMap, + source_file: &SourceFile, + name: &str, + ) -> Result> { let mut test_path = cached_source_file_fs_module_path( - &mut self.source_file_fs_module_path_cache, - &mut self.source_file_package_cache, + source_file_fs_module_path_map, + source_file_package_map, + directory_metadata_map, source_file, ) .cloned()?; @@ -46,12 +50,17 @@ impl<'ast> Storage<'ast> { pub(super) fn cached_source_file_fs_module_path<'a>( source_file_fs_module_path_map: &'a mut BTreeMap>, source_file_package_map: &mut BTreeMap, + directory_metadata_map: &mut BTreeMap, source_file: &Path, ) -> Result<&'a Vec> { source_file_fs_module_path_map .entry(source_file.to_path_buf()) .or_try_insert_with(|| { - let package = cached_source_file_package(source_file_package_map, source_file)?; + let package = cached_source_file_package( + source_file_package_map, + directory_metadata_map, + source_file, + )?; let manifest_dir = package .manifest_path @@ -88,6 +97,7 @@ pub(super) fn cached_source_file_fs_module_path<'a>( #[cfg_attr(dylint_lib = "general", allow(non_local_effect_before_error_return))] pub(super) fn cached_source_file_package<'a>( source_file_package_map: &'a mut BTreeMap, + directory_metadata_map: &mut BTreeMap, source_file: &Path, ) -> Result<&'a Package> { source_file_package_map @@ -97,14 +107,11 @@ pub(super) fn cached_source_file_package<'a>( .parent() .ok_or_else(|| anyhow!("Failed to get parent directory"))?; - let metadata = MetadataCommand::new() - .current_dir(parent) - .no_deps() - .exec()?; + let metadata = cached_directory_metadata(directory_metadata_map, parent)?; // smoelius: Use the package whose manifest directory is nearest to the source file. let mut package_near: Option = None; - for package_curr in metadata.packages { + for package_curr in &metadata.packages { let manifest_dir = package_curr .manifest_path .parent() @@ -116,10 +123,10 @@ pub(super) fn cached_source_file_package<'a>( if package_prev.manifest_path.components().count() < package_curr.manifest_path.components().count() { - package_near = Some(package_curr); + package_near = Some(package_curr.clone()); } } else { - package_near = Some(package_curr); + package_near = Some(package_curr.clone()); } } @@ -128,6 +135,22 @@ pub(super) fn cached_source_file_package<'a>( .map(|value| value as &_) } +fn cached_directory_metadata<'a>( + directory_metadata_map: &'a mut BTreeMap, + path: &Path, +) -> Result<&'a Metadata> { + directory_metadata_map + .entry(path.to_path_buf()) + .or_try_insert_with(|| { + MetadataCommand::new() + .current_dir(path) + .no_deps() + .exec() + .map_err(Into::into) + }) + .map(|value| value as &_) +} + fn fs_module_path(path: &Path) -> Result> { let Some(path_parent) = path.parent() else { return Ok(Vec::new()); diff --git a/backends/src/rust/try_insert.rs b/backends/src/rust/try_insert.rs index 16965334..86a75fdf 100644 --- a/backends/src/rust/try_insert.rs +++ b/backends/src/rust/try_insert.rs @@ -23,3 +23,29 @@ impl<'a, K: Ord, V, E> TryInsert<'a, V, E> for Entry<'a, K, V> { } } } + +#[cfg(test)] +mod test { + use super::*; + use anyhow::Result; + use std::{collections::BTreeMap, path::PathBuf}; + + #[test] + fn or_try_insert_with() { + let mut map = BTreeMap::new(); + let path_buf = PathBuf::from("/"); + let _: &mut bool = map + .entry(path_buf.clone()) + .or_try_insert_with(|| -> Result { Ok(true) }) + .unwrap(); + + // smoelius: Ensure `path_buf` is in `map`. + assert!(map.contains_key(&path_buf)); + + // smoelius: Ensure a second call to `or_try_insert_with` does not invoke the closure. + let _: &mut bool = map + .entry(path_buf) + .or_try_insert_with(|| -> Result { panic!() }) + .unwrap(); + } +} diff --git a/backends/src/rust/visitor.rs b/backends/src/rust/visitor.rs index 80346b6c..1136aa18 100644 --- a/backends/src/rust/visitor.rs +++ b/backends/src/rust/visitor.rs @@ -66,10 +66,10 @@ pub(super) fn visit<'ast>( if let Some(error) = storage.borrow_mut().error.take() { return Err(error); } - let _: &Vec = visitor.generic_visitor.backend.cached_source_file_flags( - &mut storage.borrow_mut().source_file_package_cache, - &visitor.generic_visitor.source_file, - )?; + let _: &Vec = visitor + .generic_visitor + .backend + .cached_source_file_flags(&visitor.generic_visitor.source_file)?; visitor.generic_visitor.results() } @@ -134,7 +134,17 @@ impl<'ast> Visit<'ast> for Visitor<'_, '_, '_, 'ast, '_> { assert!(self.test_ident.is_none()); self.test_ident = Some(ident); - if let Some(test) = Test::new(self.storage, &self.generic_visitor.source_file, item) { + if let Some(test) = Test::new( + self.storage, + &mut self + .generic_visitor + .backend + .source_file_fs_module_path_cache, + &mut self.generic_visitor.backend.source_file_package_cache, + &mut self.generic_visitor.backend.directory_metadata_cache, + &self.generic_visitor.source_file, + item, + ) { let walk = self.generic_visitor.visit_test(self.storage, test); if walk {