From 0707270a0502653f2fb9b57975b6ee92f7f6e0ba Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Sun, 21 Aug 2022 00:35:56 +0200 Subject: [PATCH] fix(solc): consider case sensitive conflicting artifact paths (#1625) * fix(solc): consider case sensitive conflicting artifact paths * chore: fmt Co-authored-by: Georgios Konstantopoulos --- ethers-solc/src/artifact_output/files.rs | 60 +++++++++++++ ethers-solc/src/artifact_output/mod.rs | 30 +++---- ethers-solc/src/compile/output/contracts.rs | 21 +++-- ethers-solc/tests/project.rs | 97 +++++++++++++++++++++ 4 files changed, 186 insertions(+), 22 deletions(-) create mode 100644 ethers-solc/src/artifact_output/files.rs diff --git a/ethers-solc/src/artifact_output/files.rs b/ethers-solc/src/artifact_output/files.rs new file mode 100644 index 00000000..12c88ad4 --- /dev/null +++ b/ethers-solc/src/artifact_output/files.rs @@ -0,0 +1,60 @@ +use crate::contracts::VersionedContract; +use std::{ + collections::HashMap, + hash::Hash, + ops::{Deref, DerefMut}, + path::{Path, PathBuf}, +}; + +/// Container type for all contracts mapped to their output file +pub struct MappedArtifactFiles<'a> { + /// Represents the determined output artifact file and the contract(s) that target it + /// + /// This is guaranteed to be `len >= 1` + /// + /// If there is more than 1 contract in the map, this means we have a naming conflict where + /// different contracts target the same output file. This happens if the solidity file and + /// contract name match, but they are in different folders. + pub files: HashMap>>, +} + +impl<'a> MappedArtifactFiles<'a> { + pub fn with_capacity(len: usize) -> Self { + Self { files: HashMap::with_capacity(len) } + } +} + +impl<'a> Deref for MappedArtifactFiles<'a> { + type Target = HashMap>>; + + fn deref(&self) -> &Self::Target { + &self.files + } +} + +impl<'a> DerefMut for MappedArtifactFiles<'a> { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.files + } +} + +/// Represents the targeted path of a contract or multiple contracts +/// +/// To account for case-sensitivity we identify it via lowercase path +#[derive(Debug, Hash, PartialEq, Eq)] +pub struct MappedArtifactFile { + lower_case_path: String, +} + +impl MappedArtifactFile { + pub fn new(path: &Path) -> Self { + Self { lower_case_path: path.to_string_lossy().to_lowercase() } + } +} + +pub struct MappedContract<'a> { + pub file: &'a str, + pub name: &'a str, + pub contract: &'a VersionedContract, + pub artifact_path: PathBuf, +} diff --git a/ethers-solc/src/artifact_output/mod.rs b/ethers-solc/src/artifact_output/mod.rs index a33c6095..37528dc1 100644 --- a/ethers-solc/src/artifact_output/mod.rs +++ b/ethers-solc/src/artifact_output/mod.rs @@ -17,16 +17,20 @@ use semver::Version; use serde::{de::DeserializeOwned, Serialize}; use std::{ borrow::Cow, - collections::{btree_map::BTreeMap, HashMap, HashSet}, + collections::{btree_map::BTreeMap, HashSet}, ffi::OsString, - fmt, fs, io, + fmt, fs, + hash::Hash, + io, ops::Deref, path::{Path, PathBuf}, }; use tracing::{error, trace}; mod configurable; -use crate::contracts::VersionedContract; +pub(crate) mod files; + +use crate::files::MappedContract; pub use configurable::*; /// Represents unique artifact metadata for identifying artifacts on output @@ -120,11 +124,6 @@ impl ArtifactFile { } } -/// Internal helper type alias that maps all files for the contracts -/// `output -> [(file, name, contract)]` -pub(crate) type ArtifactFiles<'a> = - HashMap>; - /// local helper type alias `file name -> (contract name -> Vec<..>)` pub(crate) type ArtifactsMap = FileToContractsMap>>; @@ -808,8 +807,9 @@ pub trait ArtifactOutput { // converting stand-alone artifacts in the next step let mut final_artifact_paths = HashSet::new(); - for (artifact_path, contracts) in artifact_files { - for (idx, (file, name, contract)) in contracts.iter().enumerate() { + for contracts in artifact_files.files.into_values() { + for (idx, mapped_contract) in contracts.iter().enumerate() { + let MappedContract { file, name, contract, artifact_path } = mapped_contract; // track `SourceFile`s that can be mapped to contracts let source_file = sources.find_file_and_version(file, &contract.version); @@ -824,11 +824,11 @@ pub trait ArtifactOutput { // contracts need to adjust the paths properly // we keep the top most conflicting file unchanged - let is_top_most = contracts.iter().enumerate().filter(|(i, _)| *i != idx).all( - |(_, (f, _, _))| { - Path::new(file).components().count() < Path::new(f).components().count() - }, - ); + let is_top_most = + contracts.iter().enumerate().filter(|(i, _)| *i != idx).all(|(_, c)| { + Path::new(file).components().count() < + Path::new(c.file).components().count() + }); if !is_top_most { // we resolve the conflicting by finding a new unique, alternative path artifact_path = Self::conflict_free_output_file( diff --git a/ethers-solc/src/compile/output/contracts.rs b/ethers-solc/src/compile/output/contracts.rs index 48a8453d..58787f8a 100644 --- a/ethers-solc/src/compile/output/contracts.rs +++ b/ethers-solc/src/compile/output/contracts.rs @@ -3,7 +3,8 @@ use crate::{ contract::{CompactContractRef, Contract}, FileToContractsMap, }, - ArtifactFiles, ArtifactOutput, OutputContext, + files::{MappedArtifactFile, MappedArtifactFiles, MappedContract}, + ArtifactOutput, OutputContext, }; use semver::Version; use serde::{Deserialize, Serialize}; @@ -47,8 +48,8 @@ impl VersionedContracts { pub(crate) fn artifact_files( &self, ctx: &OutputContext, - ) -> ArtifactFiles { - let mut output_files = ArtifactFiles::with_capacity(self.len()); + ) -> MappedArtifactFiles { + let mut output_files = MappedArtifactFiles::with_capacity(self.len()); for (file, contracts) in self.iter() { for (name, versioned_contracts) in contracts { for contract in versioned_contracts { @@ -56,7 +57,7 @@ impl VersionedContracts { // we reuse the path, this will make sure that even if there are conflicting // files (files for witch `T::output_file()` would return the same path) we use // consistent output paths - let output = if let Some(existing_artifact) = + let artifact_path = if let Some(existing_artifact) = ctx.existing_artifact(file, name, &contract.version).cloned() { trace!("use existing artifact file {:?}", existing_artifact,); @@ -69,12 +70,18 @@ impl VersionedContracts { trace!( "use artifact file {:?} for contract file {} {}", - output, + artifact_path, file, contract.version ); - let contract = (file.as_str(), name.as_str(), contract); - output_files.entry(output).or_default().push(contract); + let artifact = MappedArtifactFile::new(&artifact_path); + let contract = MappedContract { + file: file.as_str(), + name: name.as_str(), + contract, + artifact_path, + }; + output_files.entry(artifact).or_default().push(contract); } } } diff --git a/ethers-solc/tests/project.rs b/ethers-solc/tests/project.rs index f5a77edb..41fcb6f6 100644 --- a/ethers-solc/tests/project.rs +++ b/ethers-solc/tests/project.rs @@ -2226,6 +2226,103 @@ fn can_handle_conflicting_files_recompile() { assert!(inner_recompiled.get_abi().unwrap().functions.contains_key("baz")); } +// +#[test] +fn can_handle_conflicting_files_case_sensitive_recompile() { + let project = TempProject::::dapptools().unwrap(); + + project + .add_source( + "a", + r#" + pragma solidity ^0.8.10; + + contract A { + function foo() public{} + } + "#, + ) + .unwrap(); + + project + .add_source( + "inner/A", + r#" + pragma solidity ^0.8.10; + + contract A { + function bar() public{} + } + "#, + ) + .unwrap(); + + let compiled = project.compile().unwrap(); + assert!(!compiled.has_compiler_errors()); + + let artifacts = compiled.artifacts().count(); + assert_eq!(artifacts, 2); + + // nothing to compile + let compiled = project.compile().unwrap(); + assert!(compiled.is_unchanged()); + let artifacts = compiled.artifacts().count(); + assert_eq!(artifacts, 2); + + let cache = SolFilesCache::read(project.cache_path()).unwrap(); + + let mut source_files = cache.files.keys().cloned().collect::>(); + source_files.sort_unstable(); + + assert_eq!(source_files, vec![PathBuf::from("src/a.sol"), PathBuf::from("src/inner/A.sol"),]); + + let mut artifacts = + project.artifacts_snapshot().unwrap().artifacts.into_stripped_file_prefixes(project.root()); + artifacts.strip_prefix_all(&project.paths().artifacts); + + assert_eq!(artifacts.len(), 2); + let mut artifact_files = artifacts.artifact_files().map(|f| f.file.clone()).collect::>(); + artifact_files.sort_unstable(); + + let expected_files = vec![PathBuf::from("a.sol/A.json"), PathBuf::from("inner/A.sol/A.json")]; + assert_eq!(artifact_files, expected_files); + + // overwrite conflicting nested file, effectively changing it + project + .add_source( + "inner/A", + r#" + pragma solidity ^0.8.10; + contract A { + function bar() public{} + function baz() public{} + } + "#, + ) + .unwrap(); + + let compiled = project.compile().unwrap(); + assert!(!compiled.has_compiler_errors()); + + let mut recompiled_artifacts = + project.artifacts_snapshot().unwrap().artifacts.into_stripped_file_prefixes(project.root()); + recompiled_artifacts.strip_prefix_all(&project.paths().artifacts); + + assert_eq!(recompiled_artifacts.len(), 2); + let mut artifact_files = + recompiled_artifacts.artifact_files().map(|f| f.file.clone()).collect::>(); + artifact_files.sort_unstable(); + assert_eq!(artifact_files, expected_files); + + // ensure that `a.sol/A.json` is unchanged + let outer = artifacts.find("src/a.sol", "A").unwrap(); + let outer_recompiled = recompiled_artifacts.find("src/a.sol", "A").unwrap(); + assert_eq!(outer, outer_recompiled); + + let inner_recompiled = recompiled_artifacts.find("src/inner/A.sol", "A").unwrap(); + assert!(inner_recompiled.get_abi().unwrap().functions.contains_key("baz")); +} + #[test] fn can_checkout_repo() { let project = TempProject::checkout("transmissions11/solmate").unwrap();