diff --git a/src/cache.rs b/src/cache.rs index 6fc17833..f61a417f 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -683,9 +683,10 @@ impl<'a, T: ArtifactOutput> ArtifactsCacheInner<'a, T> { // separates all source files that fit the criteria (dirty) from those that don't (clean) let mut dirty_sources = BTreeMap::new(); let mut clean_sources = Vec::with_capacity(sources.len()); - let mut memo = HashMap::with_capacity(sources.len()); + let dirty_files = self.get_dirty_files(&sources, version); + for (file, source) in sources { - let source = self.filter_source(file, source, version, &mut memo); + let source = self.filter_source(file, source, &dirty_files); if source.dirty { // mark all files that are imported by a dirty file imports_of_dirty.extend(self.edges.all_imported_nodes(source.idx)); @@ -717,39 +718,42 @@ impl<'a, T: ArtifactOutput> ArtifactsCacheInner<'a, T> { &self, file: PathBuf, source: Source, - version: &Version, - memo: &mut HashMap, + dirty_files: &HashSet, ) -> FilteredSourceInfo { let idx = self.edges.node_id(&file); - let dirty = self.is_dirty(&file, version, memo, true); + let dirty = dirty_files.contains(&file); FilteredSourceInfo { file, source, idx, dirty } } - /// Returns `false` if the corresponding cache entry remained unchanged, otherwise `true`. - #[instrument(level = "trace", skip_all, fields(file = %file.display(), version = %version))] - fn is_dirty( - &self, - file: &Path, - version: &Version, - memo: &mut HashMap, - check_imports: bool, - ) -> bool { - match memo.get(file) { - Some(&dirty) => { - trace!(dirty, "memoized"); - dirty + /// Returns a set of files that are dirty itself or import dirty file directly or indirectly. + fn get_dirty_files(&self, sources: &Sources, version: &Version) -> HashSet { + let mut dirty_files = HashSet::new(); + + // Pre-add all sources that are guaranteed to be dirty + for file in sources.keys() { + if self.is_dirty_impl(file, version) { + dirty_files.insert(file.to_path_buf()); } - None => { - // `check_imports` avoids infinite recursion - let dirty = self.is_dirty_impl(file, version) - || (check_imports - && self - .edges - .imports(file) - .iter() - .any(|file| self.is_dirty(file, version, memo, false))); - memo.insert(file.to_path_buf(), dirty); - dirty + } + + // Perform DFS to find direct/indirect importers of dirty files + for file in dirty_files.clone().iter() { + self.populate_dirty_files(file, &mut dirty_files); + } + + dirty_files + } + + /// Accepts known dirty file and performs DFS over it's importers marking all visited files as + /// dirty. + #[instrument(level = "trace", skip_all, fields(file = %file.display()))] + fn populate_dirty_files(&self, file: &Path, dirty_files: &mut HashSet) { + for file in self.edges.importers(file) { + // If file is marked as dirty we either have already visited it or it was marked as + // dirty initially and will be visited at some point later. + if !dirty_files.contains(file) { + dirty_files.insert(file.to_path_buf()); + self.populate_dirty_files(file, dirty_files); } } } diff --git a/src/resolver/mod.rs b/src/resolver/mod.rs index 9bbec7ba..744c1548 100644 --- a/src/resolver/mod.rs +++ b/src/resolver/mod.rs @@ -72,6 +72,8 @@ pub struct GraphEdges { /// The indices of `edges` correspond to the `nodes`. That is, `edges[0]` /// is the set of outgoing edges for `nodes[0]`. edges: Vec>, + /// Reverse of `edges`. That is, `rev_edges[0]` is the set of incoming edges for `nodes[0]`. + rev_edges: Vec>, /// index maps for a solidity file to an index, for fast lookup. indices: HashMap, /// reverse of `indices` for reverse lookup @@ -146,6 +148,15 @@ impl GraphEdges { } } + /// Returns all files that import the given file + pub fn importers(&self, file: impl AsRef) -> HashSet<&PathBuf> { + if let Some(start) = self.indices.get(file.as_ref()).copied() { + self.rev_edges[start].iter().map(move |idx| &self.rev_indices[&idx]).collect() + } else { + HashSet::new() + } + } + /// Returns the id of the given file pub fn node_id(&self, file: impl AsRef) -> usize { self.indices[file.as_ref()] @@ -343,6 +354,7 @@ impl Graph { // contains the files and their dependencies let mut nodes = Vec::with_capacity(unresolved.len()); let mut edges = Vec::with_capacity(unresolved.len()); + let mut rev_edges = Vec::with_capacity(unresolved.len()); // tracks additional paths that should be used with `--include-path`, these are libraries // that use absolute imports like `import "src/Contract.sol"` @@ -400,6 +412,15 @@ impl Graph { nodes.push(node); edges.push(resolved_imports); + // Will be populated later + rev_edges.push(Vec::new()); + } + + // Build `rev_edges` + for (idx, edges) in edges.iter().enumerate() { + for &edge in edges.iter() { + rev_edges[edge].push(idx); + } } if !unresolved_imports.is_empty() { @@ -415,6 +436,7 @@ impl Graph { let edges = GraphEdges { edges, + rev_edges, rev_indices: index.iter().map(|(k, v)| (*v, k.clone())).collect(), indices: index, num_input_files, diff --git a/tests/project.rs b/tests/project.rs index 277c2f47..1ac9cdbc 100644 --- a/tests/project.rs +++ b/tests/project.rs @@ -2731,3 +2731,55 @@ contract ContractTest { compiled.assert_success(); assert!(compiled.find_first("Contract").is_some()); } + +// This is a repro and a regression test for https://github.com/foundry-rs/compilers/pull/45 +#[test] +fn dirty_files_discovery() { + let project = TempProject::dapptools().unwrap(); + + project + .add_source( + "D.sol", + r" +pragma solidity 0.8.23; +contract D { + function foo() internal pure returns (uint256) { + return 1; + } +} + ", + ) + .unwrap(); + + project + .add_source("A.sol", "pragma solidity ^0.8.10; import './C.sol'; contract A is D {}") + .unwrap(); + project + .add_source("B.sol", "pragma solidity ^0.8.10; import './A.sol'; contract B is D {}") + .unwrap(); + project + .add_source("C.sol", "pragma solidity ^0.8.10; import './D.sol'; contract C is D {}") + .unwrap(); + + project.compile().unwrap(); + + // Change D.sol so it becomes dirty + project + .add_source( + "D.sol", + r" +pragma solidity 0.8.23; +contract D { + function foo() internal pure returns (uint256) { + return 2; + } +} + ", + ) + .unwrap(); + + let output = project.compile().unwrap(); + + // Check that all contracts were recompiled + assert_eq!(output.compiled_artifacts().len(), 4); +}