From 0c42c2374622a5e544f1ab7adcd4d02a7bde8a20 Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Wed, 9 Mar 2022 19:52:40 +0100 Subject: [PATCH] fix(solc): fix cache and allowed paths bug (#998) * fix: don't emit on error * fix: allowed-paths condition * fix(solc): cache and allowed paths bug --- ethers-solc/src/cache.rs | 69 ++++++++++++++++++++++-------- ethers-solc/src/compile/project.rs | 16 ++++--- ethers-solc/src/lib.rs | 6 ++- ethers-solc/src/resolver/mod.rs | 10 +++++ ethers-solc/tests/project.rs | 40 +++++++++++++++++ 5 files changed, 116 insertions(+), 25 deletions(-) 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> { 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 Project { &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 + '_ { + NodesIter::new(from, self).skip(1) + } + /// Returns all files imported by the given file pub fn imports(&self, file: impl AsRef) -> 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) -> 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) -> bool { 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()); +}