From 29d23e303dfa927d29108d68e93c92e1755c3c61 Mon Sep 17 00:00:00 2001 From: Samuel Moelius Date: Sun, 3 Nov 2024 19:22:26 -0500 Subject: [PATCH 1/3] Add `or_try_insert_with` test --- backends/src/rust/try_insert.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) 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(); + } +} From aee606fe31671f055bacf53355d2d58ddbc97d28 Mon Sep 17 00:00:00 2001 From: Samuel Moelius Date: Sun, 3 Nov 2024 19:23:38 -0500 Subject: [PATCH 2/3] Cache directory metadata in the Rust backend --- backends/src/rust/mod.rs | 16 +++++++++--- backends/src/rust/storage.rs | 47 ++++++++++++++++++++++++++++-------- backends/src/rust/visitor.rs | 7 +++++- 3 files changed, 56 insertions(+), 14 deletions(-) diff --git a/backends/src/rust/mod.rs b/backends/src/rust/mod.rs index 13404ad7..40d49edf 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,7 @@ use visitor::{collect_local_functions, visit}; #[derive(Debug)] pub struct Rust { + directory_metadata_cache: BTreeMap, source_file_flags_cache: BTreeMap>, } @@ -45,6 +46,7 @@ impl Rust { pub fn new() -> Self { Self { + directory_metadata_cache: BTreeMap::new(), source_file_flags_cache: BTreeMap::new(), } } @@ -59,13 +61,17 @@ pub struct Test<'ast> { impl<'ast> Test<'ast> { fn new( storage: &RefCell, + 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(directory_metadata_map, source_file, &test_name); let test_path = match result { Ok(test_path) => test_path, Err(error) => { @@ -666,7 +672,11 @@ impl Rust { 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( + source_file_package_map, + &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..93bd109c 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,6 +12,8 @@ use syn::{File, Ident}; /// Structures needed during parsing but not after. pub struct Storage<'ast> { pub module_path: Vec<&'ast Ident>, + // smoelius: I think putting the next two maps in `Storage` may be a bug. A `Storage` lasts + // only as long as one source file. pub source_file_fs_module_path_cache: BTreeMap>, pub source_file_package_cache: BTreeMap, pub tests_needing_warnings: BTreeMap>, @@ -29,10 +31,16 @@ impl<'ast> Storage<'ast> { } } - pub fn test_path(&mut self, source_file: &SourceFile, name: &str) -> Result> { + pub fn test_path( + &mut self, + 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, + directory_metadata_map, source_file, ) .cloned()?; @@ -46,12 +54,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 +101,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 +111,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 +127,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 +139,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/visitor.rs b/backends/src/rust/visitor.rs index 80346b6c..8b682b36 100644 --- a/backends/src/rust/visitor.rs +++ b/backends/src/rust/visitor.rs @@ -134,7 +134,12 @@ 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.directory_metadata_cache, + &self.generic_visitor.source_file, + item, + ) { let walk = self.generic_visitor.visit_test(self.storage, test); if walk { From 01608c3f23dd5b960c1975b85858c7331d958a8c Mon Sep 17 00:00:00 2001 From: Samuel Moelius Date: Mon, 4 Nov 2024 18:02:25 -0500 Subject: [PATCH 3/3] Move two caches out of `Storage` in Rust backend --- backends/src/rust/mod.rs | 25 +++++++++++++++---------- backends/src/rust/storage.rs | 12 ++++-------- backends/src/rust/visitor.rs | 13 +++++++++---- 3 files changed, 28 insertions(+), 22 deletions(-) diff --git a/backends/src/rust/mod.rs b/backends/src/rust/mod.rs index 40d49edf..900c427d 100644 --- a/backends/src/rust/mod.rs +++ b/backends/src/rust/mod.rs @@ -31,6 +31,8 @@ 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>, } @@ -46,6 +48,8 @@ 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(), } @@ -61,6 +65,8 @@ 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, @@ -68,10 +74,13 @@ impl<'ast> Test<'ast> { // 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(directory_metadata_map, 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) => { @@ -664,16 +673,12 @@ 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, + &mut self.source_file_package_cache, &mut self.directory_metadata_cache, source_file, )?; diff --git a/backends/src/rust/storage.rs b/backends/src/rust/storage.rs index 93bd109c..76768d2a 100644 --- a/backends/src/rust/storage.rs +++ b/backends/src/rust/storage.rs @@ -12,10 +12,6 @@ use syn::{File, Ident}; /// Structures needed during parsing but not after. pub struct Storage<'ast> { pub module_path: Vec<&'ast Ident>, - // smoelius: I think putting the next two maps in `Storage` may be a bug. A `Storage` lasts - // only as long as one source file. - pub source_file_fs_module_path_cache: BTreeMap>, - pub source_file_package_cache: BTreeMap, pub tests_needing_warnings: BTreeMap>, pub error: Option, } @@ -24,8 +20,6 @@ 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, } @@ -33,13 +27,15 @@ impl<'ast> Storage<'ast> { 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, ) diff --git a/backends/src/rust/visitor.rs b/backends/src/rust/visitor.rs index 8b682b36..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() } @@ -136,6 +136,11 @@ impl<'ast> Visit<'ast> for Visitor<'_, '_, '_, 'ast, '_> { 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,