Rewrite dirty files discovery (#45)

## Motivation

Currently, dirty files discovery is not going deep enough into the
graph, thus it often fails to discover all files that have dirty imports
of some depth > 1. In worst case we may only recompile directly changed
files and files directly importing them.

This PR changes this, so all files that have dirty files at some depth
of their imports are marked as dirty.

This should solve a lot of weird issues with verification because it is
very simple to occur such bug and it can result in, for example, script
having and deploying outdated bytecode of contract which does not match
the actual artifact.

## Solution

If we would consider that problem as a graph theory problem, than we are
dealing with oriented graph of `N` nodes representing files and `M`
oriented edges between them representing imports. Some of `N` nodes are
marked as dirty initially and we want to mark as dirty all nodes from
which a path exists to any of the dirty nodes.

Before the fix, the incorrect solution complexity was around `O(M)`, so
I didn't want to increase it too much.

One possible solution without much changes to code could be to run
deeper recursion from each node and try to reach dirty node from it.
However, such solution would have a complexity of `O(N*M)` in worst
case.

The solution I've implemented is working by precomputing `rev_edges` to
store reversed edges, thus simplifying the problem to just finding all
nodes that are reachable from the dirty ones in the reversed graph. The
solution works by running DFS from all nodes that are known to be dirty
in the beginning and keeping track of visited (dirty) nodes, to avoid
visiting the same node twice. The complexity of such approach is `O(M)`
in the worst case because we never use the same edge twice.

---------

Co-authored-by: Enrique <hi@enriqueortiz.dev>

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<PathBuf, bool>,
+        dirty_files: &HashSet<PathBuf>,
     ) -> 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<PathBuf, bool>,
-        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<PathBuf> {
+        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<PathBuf>) {
+        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);
             }
         }
     }

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<Vec<usize>>,
+    /// Reverse of `edges`. That is, `rev_edges[0]` is the set of incoming edges for `nodes[0]`.
+    rev_edges: Vec<Vec<usize>>,
     /// index maps for a solidity file to an index, for fast lookup.
     indices: HashMap<PathBuf, usize>,
     /// 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<Path>) -> 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<Path>) -> 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,

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);
+}