Repository: gakonst/ethers-rs
Pull request #998 (2022-03-09 11:39:10)

fix(solc): fix cache and allowed paths bug
<!--
Thank you for your Pull Request. Please provide a description above and review
the requirements below.

Bug fixes and new features should include tests.

Contributors guide: https://github.com/gakonst/ethers-rs/blob/master/CONTRIBUTING.md

The contributors guide includes instructions for running rustfmt and building the
documentation.
-->

## Motivation
This fixes several issues that were discovered on https://github.com/maple-labs/loan/releases/tag/v3.0.0-beta.0

- don't emit anything (cache/artifacts) if there are errors in the project output
- an erroneous condition when checking allowed-paths
- when filtering `dirty` files, also include their imports in the CompilerInput so that solc doesn't need to look them up
<!--
Explain the context and why you're making that change. What is the problem
you're trying to solve? In some cases there is not a problem and this can be
thought of as being the motivation for your change.
-->

## Solution

<!--
Summarize the solution and provide any necessary context needed to understand
the code change.
-->

## PR Checklist

- [x] Added Tests
- [x] Added Documentation
- [ ] Updated the changelog



Patch:
diff --git a/Cargo.lock b/Cargo.lock
index e0cdb064..117aecf3 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -2882,9 +2882,9 @@ dependencies = [
 
 [[package]]
 name = "regex"
-version = "1.5.4"
+version = "1.5.5"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "d07a8629359eb56f1e2fb1652bb04212c072a87ba68546a04065d525673ac461"
+checksum = "1a11647b6b25ff05a515cb92c365cec08801e83423a235b51e231e1808747286"
 dependencies = [
  "aho-corasick",
  "memchr",
diff --git a/ethers-solc/Cargo.toml b/ethers-solc/Cargo.toml
index c860fdb5..571cae0b 100644
--- a/ethers-solc/Cargo.toml
+++ b/ethers-solc/Cargo.toml
@@ -22,7 +22,7 @@ walkdir = "2.3.2"
 tokio = { version = "1.15.0", default-features = false, features = ["process", "io-util", "fs", "time"], optional = true }
 futures-util = { version = "^0.3", optional = true }
 once_cell = "1.10.0"
-regex = "1.5.4"
+regex = "1.5.5"
 md-5 = "0.10.1"
 thiserror = "1.0.30"
 hex = "0.4.3"
diff --git a/ethers-solc/src/cache.rs b/ethers-solc/src/cache.rs
index 4ebdf020..faf07465 100644
--- a/ethers-solc/src/cache.rs
+++ b/ethers-solc/src/cache.rs
@@ -581,7 +581,7 @@ impl<'a, T: ArtifactOutput> ArtifactsCacheInner<'a, T> {
         }
     }
 
-    /// inserts the filtered source with the fiven version
+    /// inserts the filtered source with the given version
     fn insert_filtered_source(&mut self, file: PathBuf, source: Source, version: Version) {
         match self.filtered.entry(file) {
             hash_map::Entry::Occupied(mut entry) => {
@@ -593,35 +593,62 @@ impl<'a, T: ArtifactOutput> ArtifactsCacheInner<'a, T> {
         }
     }
 
-    /// Returns only those sources that
+    /// Returns only dirty sources that:
     ///   - are new
     ///   - were changed
     ///   - their imports were changed
     ///   - their artifact is missing
+    /// This also includes their respective imports
     fn filter(&mut self, sources: Sources, version: &Version) -> Sources {
         self.fill_hashes(&sources);
-        sources
+
+        let mut imports_of_dirty = HashSet::new();
+        // separates all source files that fit the criteria (dirty) from those that don't (clean)
+        let (mut dirty_sources, clean_sources) = sources
             .into_iter()
-            .filter_map(|(file, source)| self.requires_solc(file, source, version))
-            .collect()
+            .map(|(file, source)| self.filter_source(file, source, version))
+            .fold(
+                (Sources::default(), Vec::new()),
+                |(mut dirty_sources, mut clean_sources), source| {
+                    if source.dirty {
+                        // mark all files that are imported by a dirty file
+                        imports_of_dirty.extend(self.edges.all_imported_nodes(source.idx));
+                        dirty_sources.insert(source.file, source.source);
+                    } else {
+                        clean_sources.push(source);
+                    }
+
+                    (dirty_sources, clean_sources)
+                },
+            );
+
+        for clean_source in clean_sources {
+            let FilteredSource { file, source, idx, .. } = clean_source;
+            if imports_of_dirty.contains(&idx) {
+                // file is imported by a dirty file
+                dirty_sources.insert(file, source);
+            } else {
+                self.insert_filtered_source(file, source, version.clone());
+            }
+        }
+
+        // track dirty sources internally
+        for (file, source) in dirty_sources.iter() {
+            self.insert_new_cache_entry(file, source, version.clone());
+        }
+
+        dirty_sources
     }
 
-    /// Returns `Some` if the file _needs_ to be compiled and `None` if the artifact can be reu-used
-    fn requires_solc(
-        &mut self,
-        file: PathBuf,
-        source: Source,
-        version: &Version,
-    ) -> Option<(PathBuf, Source)> {
+    /// Returns the state of the given source file.
+    fn filter_source(&self, file: PathBuf, source: Source, version: &Version) -> FilteredSource {
+        let idx = self.edges.node_id(&file);
         if !self.is_dirty(&file, version) &&
             self.edges.imports(&file).iter().all(|file| !self.is_dirty(file, version))
         {
-            self.insert_filtered_source(file, source, version.clone());
-            None
+            FilteredSource { file, source, idx, dirty: false }
         } else {
-            self.insert_new_cache_entry(&file, &source, version.clone());
-
-            Some((file, source))
+            FilteredSource { file, source, idx, dirty: true }
         }
     }
 
@@ -674,6 +701,14 @@ impl<'a, T: ArtifactOutput> ArtifactsCacheInner<'a, T> {
     }
 }
 
+/// Helper type to represent the state of a source file
+struct FilteredSource {
+    file: PathBuf,
+    source: Source,
+    idx: usize,
+    dirty: bool,
+}
+
 /// Abstraction over configured caching which can be either non-existent or an already loaded cache
 #[allow(clippy::large_enum_variant)]
 #[derive(Debug)]
diff --git a/ethers-solc/src/compile/project.rs b/ethers-solc/src/compile/project.rs
index 6e1a9ac7..91ba7d5c 100644
--- a/ethers-solc/src/compile/project.rs
+++ b/ethers-solc/src/compile/project.rs
@@ -179,7 +179,7 @@ impl<'a, T: ArtifactOutput> ProjectCompiler<'a, T> {
         let Self { edges, project, mut sources } = self;
 
         let mut cache = ArtifactsCache::new(project, edges)?;
-        // retain and compile only dirty sources
+        // retain and compile only dirty sources and all their imports
         sources = sources.filtered(&mut cache);
 
         Ok(PreprocessedState { sources, cache })
@@ -216,18 +216,22 @@ struct CompiledState<'a, T: ArtifactOutput> {
 impl<'a, T: ArtifactOutput> CompiledState<'a, T> {
     /// advance to the next state by handling all artifacts
     ///
-    /// Writes all output contracts to disk if enabled in the `Project`
+    /// Writes all output contracts to disk if enabled in the `Project` and if the build was
+    /// successful
     fn write_artifacts(self) -> Result<ArtifactsState<'a, T>> {
         let CompiledState { output, cache } = self;
 
-        // write all artifacts
-        let compiled_artifacts = if !cache.project().no_artifacts {
+        // write all artifacts via the handler but only if the build succeeded
+        let compiled_artifacts = if cache.project().no_artifacts {
+            cache.project().artifacts_handler().output_to_artifacts(&output.contracts)
+        } else if output.has_error() {
+            tracing::trace!("skip writing cache file due to solc errors: {:?}", output.errors);
+            cache.project().artifacts_handler().output_to_artifacts(&output.contracts)
+        } else {
             cache
                 .project()
                 .artifacts_handler()
                 .on_output(&output.contracts, &cache.project().paths)?
-        } else {
-            cache.project().artifacts_handler().output_to_artifacts(&output.contracts)
         };
 
         Ok(ArtifactsState { output, cache, compiled_artifacts })
diff --git a/ethers-solc/src/lib.rs b/ethers-solc/src/lib.rs
index afc6821b..f6cdef08 100644
--- a/ethers-solc/src/lib.rs
+++ b/ethers-solc/src/lib.rs
@@ -125,9 +125,11 @@ impl<T: ArtifactOutput> Project<T> {
         &self.artifacts
     }
 
-    /// Applies the configured settings to the given `Solc`
+    /// Applies the configured arguments to the given `Solc`
+    ///
+    /// This will set the `--allow-paths` to the paths configured for the `Project`, if any.
     fn configure_solc(&self, mut solc: Solc) -> Solc {
-        if self.allowed_lib_paths.0.is_empty() {
+        if solc.args.is_empty() && !self.allowed_lib_paths.0.is_empty() {
             solc = solc.arg("--allow-paths").arg(self.allowed_lib_paths.to_string());
         }
         solc
diff --git a/ethers-solc/src/resolver/mod.rs b/ethers-solc/src/resolver/mod.rs
index a2ba28a1..19610f1a 100644
--- a/ethers-solc/src/resolver/mod.rs
+++ b/ethers-solc/src/resolver/mod.rs
@@ -91,6 +91,11 @@ impl GraphEdges {
         &self.edges[from]
     }
 
+    /// Returns an iterator that yields all imports of a node and all their imports
+    pub fn all_imported_nodes(&self, from: usize) -> impl Iterator<Item = usize> + '_ {
+        NodesIter::new(from, self).skip(1)
+    }
+
     /// Returns all files imported by the given file
     pub fn imports(&self, file: impl AsRef<Path>) -> HashSet<&PathBuf> {
         if let Some(start) = self.indices.get(file.as_ref()).copied() {
@@ -100,6 +105,11 @@ impl GraphEdges {
         }
     }
 
+    /// Returns the id of the given file
+    pub fn node_id(&self, file: impl AsRef<Path>) -> usize {
+        self.indices[file.as_ref()]
+    }
+
     /// Returns true if the `file` was originally included when the graph was first created and not
     /// added when all `imports` were resolved
     pub fn is_input_file(&self, file: impl AsRef<Path>) -> bool {
diff --git a/ethers-solc/src/resolver/tree.rs b/ethers-solc/src/resolver/tree.rs
index 230036b6..9716613d 100644
--- a/ethers-solc/src/resolver/tree.rs
+++ b/ethers-solc/src/resolver/tree.rs
@@ -157,11 +157,6 @@ fn print_imports(
         return Ok(())
     }
 
-    for continues in &**levels_continue {
-        let c = if *continues { symbols.down } else { " " };
-        write!(out, "{}   ", c)?;
-    }
-
     let mut iter = imports.iter().peekable();
 
     while let Some(import) = iter.next() {


Test patch:
diff --git a/ethers-solc/tests/project.rs b/ethers-solc/tests/project.rs
index 2cdbe6d3..3129da7c 100644
--- a/ethers-solc/tests/project.rs
+++ b/ethers-solc/tests/project.rs
@@ -613,3 +613,43 @@ contract LinkTest {
     let s = serde_json::to_string(&bytecode).unwrap();
     assert_eq!(bytecode.clone(), serde_json::from_str(&s).unwrap());
 }
+
+#[test]
+fn can_recompile_with_changes() {
+    let mut tmp = TempProject::dapptools().unwrap();
+    tmp.project_mut().allowed_lib_paths = vec![tmp.root().join("modules")].into();
+
+    let content = r#"
+    pragma solidity ^0.8.10;
+    import "../modules/B.sol";
+    contract A {}
+   "#;
+    tmp.add_source("A", content).unwrap();
+
+    tmp.add_contract(
+        "modules/B",
+        r#"
+    pragma solidity ^0.8.10;
+    contract B {}
+   "#,
+    )
+    .unwrap();
+
+    let compiled = tmp.compile().unwrap();
+    assert!(!compiled.has_compiler_errors());
+    assert!(compiled.find("A").is_some());
+    assert!(compiled.find("B").is_some());
+
+    let compiled = tmp.compile().unwrap();
+    assert!(compiled.find("A").is_some());
+    assert!(compiled.find("B").is_some());
+    assert!(compiled.is_unchanged());
+
+    // modify A.sol
+    tmp.add_source("A", format!("{}\n", content)).unwrap();
+    let compiled = tmp.compile().unwrap();
+    assert!(!compiled.has_compiler_errors());
+    assert!(!compiled.is_unchanged());
+    assert!(compiled.find("A").is_some());
+    assert!(compiled.find("B").is_some());
+}