From 331caf9418f38646f331a0ee22af4f5725aec77e Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Thu, 10 Feb 2022 18:56:25 +0100 Subject: [PATCH] fix(solc): resolver and remapping auto detection bugs (#893) * fix(solc): support single quote imports * feat: better error message * fix: nfmt * feat: handle additional remappings edge case * fix(solc): treat nested remappings differently depending on src and contracts * fix test * chore(clippy): make clippy happy --- ethers-solc/src/error.rs | 3 + ethers-solc/src/remappings.rs | 226 ++++++++++++++++++++++++++++++---- ethers-solc/src/resolver.rs | 22 ++-- ethers-solc/src/utils.rs | 36 +++++- 4 files changed, 244 insertions(+), 43 deletions(-) diff --git a/ethers-solc/src/error.rs b/ethers-solc/src/error.rs index 2180aa9a..2d6751a5 100644 --- a/ethers-solc/src/error.rs +++ b/ethers-solc/src/error.rs @@ -23,6 +23,9 @@ pub enum SolcError { /// Filesystem IO error #[error(transparent)] Io(#[from] SolcIoError), + /// Failed to resolve a file + #[error("Failed to resolve file: {0}.\n Check configured remappings.")] + Resolve(SolcIoError), #[cfg(feature = "svm")] #[error(transparent)] SvmError(#[from] svm::SolcVmError), diff --git a/ethers-solc/src/remappings.rs b/ethers-solc/src/remappings.rs index 75067db5..5a37267a 100644 --- a/ethers-solc/src/remappings.rs +++ b/ethers-solc/src/remappings.rs @@ -336,10 +336,80 @@ struct Candidate { } impl Candidate { - /// Returns `true` if the source dir of this candidate ends with `/src` or `/contracts` - fn is_candidate_source_a_source_dir(&self) -> bool { - self.source_dir.ends_with(DAPPTOOLS_CONTRACTS_DIR) || - self.source_dir.ends_with(JS_CONTRACTS_DIR) + /// There are several cases where multiple candidates are detected for the same level + /// + /// # Example - Dapptools style + /// + /// Another directory next to a `src` dir: + /// ```text + /// ds-test/ + /// ├── aux/demo.sol + /// └── src/test.sol + /// ``` + /// which effectively ignores the `aux` dir by prioritizing source dirs and keeps + /// `ds-test/=ds-test/src/` + /// + /// + /// # Example - node_modules / commonly onpenzeppelin related + /// + /// The `@openzeppelin` domain can contain several nested dirs in `node_modules/@openzeppelin`. + /// Such as + /// - `node_modules/@openzeppelin/contracts` + /// - `node_modules/@openzeppelin/contracts-upgradeable` + /// + /// Which should be resolved to the top level dir `@openzeppelin` + /// + /// In order to support these cases, we treat the Dapptools case as the outlier, in which case + /// we only keep the candidate that ends with `src` + fn merge_on_same_level( + candidates: &mut Vec, + current_dir: &Path, + current_level: usize, + window_start: PathBuf, + ) { + if let Some(pos) = + candidates.iter().position(|c| c.source_dir.ends_with(DAPPTOOLS_CONTRACTS_DIR)) + { + let c = candidates.remove(pos); + *candidates = vec![c]; + } else { + // merge all candidates on the current level if the current dir is itself a candidate or + // there are multiple nested candidates on the current level like `current/{auth, + // tokens}/contracts/c.sol` + candidates.retain(|c| c.window_level != current_level); + candidates.push(Candidate { + window_start, + source_dir: current_dir.to_path_buf(), + window_level: current_level, + }); + } + } + + /// Returns `true` if the `source_dir` ends with `contracts` or `contracts/src` + /// + /// This is used to detect an edge case in `"@chainlink/contracts"` which layout is + /// + /// ```text + /// contracts/src + /// ├── v0.4 + /// ├── Pointer.sol + /// ├── interfaces + /// ├── AggregatorInterface.sol + /// ├── tests + /// ├── BasicConsumer.sol + /// ├── v0.5 + /// ├── Chainlink.sol + /// ├── v0.6 + /// ├── AccessControlledAggregator.sol + /// ``` + /// + /// And import commonly used is + /// + /// ```solidity + /// import '@chainlink/contracts/src/v0.6/interfaces/AggregatorV3Interface.sol'; + /// ``` + fn source_dir_ends_with_js_source(&self) -> bool { + self.source_dir.ends_with(JS_CONTRACTS_DIR) || self.source_dir.ends_with("contracts/src/") } } @@ -419,28 +489,7 @@ fn find_remapping_candidates( .count() > 1 { - // here we need to check for layouts like - // ```test - // ds-test/ - // ├── aux/demo.sol - // └── src/test.sol - // ``` - // which effectively ignores the `aux` dir by prioritizing source dirs and keeps - // `ds-test/=ds-test/src/` - if let Some(pos) = candidates.iter().position(Candidate::is_candidate_source_a_source_dir) { - let c = candidates.remove(pos); - candidates = vec![c]; - } else { - // merge all candidates on the current level if the current dir is itself a candidate or - // there are multiple nested candidates on the current level like `current/{auth, - // tokens}/contracts/c.sol` - candidates.retain(|c| c.window_level != current_level); - candidates.push(Candidate { - window_start, - source_dir: current_dir.to_path_buf(), - window_level: current_level, - }); - } + Candidate::merge_on_same_level(&mut candidates, current_dir, current_level, window_start); } else { // this handles the case if there is a single nested candidate if let Some(candidate) = candidates.iter_mut().find(|c| c.window_level == current_level) { @@ -448,7 +497,7 @@ fn find_remapping_candidates( // contracts dir for cases like `current/nested/contracts/c.sol` which should point to // `current` let distance = dir_distance(&candidate.window_start, &candidate.source_dir); - if distance > 1 && candidate.source_dir.ends_with(JS_CONTRACTS_DIR) { + if distance > 1 && candidate.source_dir_ends_with_js_source() { candidate.source_dir = window_start; } else if !is_source_dir(&candidate.source_dir) && candidate.source_dir != candidate.window_start @@ -656,6 +705,129 @@ mod tests { pretty_assertions::assert_eq!(remappings, expected); } + #[test] + fn can_resolve_nested_chainlink_remappings() { + let tmp_dir = tempdir("root").unwrap(); + let paths = [ + "@chainlink/contracts/src/v0.6/vendor/Contract.sol", + "@chainlink/contracts/src/v0.8/tests/Contract.sol", + "@chainlink/contracts/src/v0.7/Contract.sol", + "@chainlink/contracts/src/v0.6/Contract.sol", + "@chainlink/contracts/src/v0.5/Contract.sol", + "@chainlink/contracts/src/v0.7/tests/Contract.sol", + "@chainlink/contracts/src/v0.7/interfaces/Contract.sol", + "@chainlink/contracts/src/v0.4/tests/Contract.sol", + "@chainlink/contracts/src/v0.6/tests/Contract.sol", + "@chainlink/contracts/src/v0.5/tests/Contract.sol", + "@chainlink/contracts/src/v0.8/vendor/Contract.sol", + "@chainlink/contracts/src/v0.5/dev/Contract.sol", + "@chainlink/contracts/src/v0.6/examples/Contract.sol", + "@chainlink/contracts/src/v0.5/interfaces/Contract.sol", + "@chainlink/contracts/src/v0.4/interfaces/Contract.sol", + "@chainlink/contracts/src/v0.4/vendor/Contract.sol", + "@chainlink/contracts/src/v0.6/interfaces/Contract.sol", + "@chainlink/contracts/src/v0.7/dev/Contract.sol", + "@chainlink/contracts/src/v0.8/dev/Contract.sol", + "@chainlink/contracts/src/v0.5/vendor/Contract.sol", + "@chainlink/contracts/src/v0.7/vendor/Contract.sol", + "@chainlink/contracts/src/v0.4/Contract.sol", + "@chainlink/contracts/src/v0.8/interfaces/Contract.sol", + "@chainlink/contracts/src/v0.6/dev/Contract.sol", + ]; + mkdir_or_touch(tmp_dir.path(), &paths[..]); + let remappings = Remapping::find_many(tmp_dir.path()); + + let expected = vec![Remapping { + name: "@chainlink/".to_string(), + path: to_str(tmp_dir.path().join("@chainlink")), + }]; + pretty_assertions::assert_eq!(remappings, expected); + } + + #[test] + fn can_resolve_oz_upgradeable_remappings() { + let tmp_dir = tempdir("root").unwrap(); + let paths = [ + "@openzeppelin/contracts-upgradeable/proxy/ERC1967/Contract.sol", + "@openzeppelin/contracts-upgradeable/token/ERC1155/Contract.sol", + "@openzeppelin/contracts/token/ERC777/Contract.sol", + "@openzeppelin/contracts/token/ERC721/presets/Contract.sol", + "@openzeppelin/contracts/interfaces/Contract.sol", + "@openzeppelin/contracts-upgradeable/token/ERC777/presets/Contract.sol", + "@openzeppelin/contracts/token/ERC1155/extensions/Contract.sol", + "@openzeppelin/contracts/proxy/Contract.sol", + "@openzeppelin/contracts/proxy/utils/Contract.sol", + "@openzeppelin/contracts-upgradeable/security/Contract.sol", + "@openzeppelin/contracts-upgradeable/utils/Contract.sol", + "@openzeppelin/contracts/token/ERC20/Contract.sol", + "@openzeppelin/contracts-upgradeable/utils/introspection/Contract.sol", + "@openzeppelin/contracts/metatx/Contract.sol", + "@openzeppelin/contracts/utils/cryptography/Contract.sol", + "@openzeppelin/contracts/token/ERC20/utils/Contract.sol", + "@openzeppelin/contracts-upgradeable/token/ERC20/utils/Contract.sol", + "@openzeppelin/contracts-upgradeable/proxy/Contract.sol", + "@openzeppelin/contracts-upgradeable/token/ERC20/presets/Contract.sol", + "@openzeppelin/contracts-upgradeable/utils/math/Contract.sol", + "@openzeppelin/contracts-upgradeable/utils/escrow/Contract.sol", + "@openzeppelin/contracts/governance/extensions/Contract.sol", + "@openzeppelin/contracts-upgradeable/interfaces/Contract.sol", + "@openzeppelin/contracts/proxy/transparent/Contract.sol", + "@openzeppelin/contracts/utils/structs/Contract.sol", + "@openzeppelin/contracts-upgradeable/access/Contract.sol", + "@openzeppelin/contracts/governance/compatibility/Contract.sol", + "@openzeppelin/contracts/governance/Contract.sol", + "@openzeppelin/contracts-upgradeable/governance/extensions/Contract.sol", + "@openzeppelin/contracts/security/Contract.sol", + "@openzeppelin/contracts-upgradeable/metatx/Contract.sol", + "@openzeppelin/contracts-upgradeable/token/ERC721/utils/Contract.sol", + "@openzeppelin/contracts/token/ERC721/utils/Contract.sol", + "@openzeppelin/contracts-upgradeable/governance/compatibility/Contract.sol", + "@openzeppelin/contracts/token/common/Contract.sol", + "@openzeppelin/contracts/proxy/beacon/Contract.sol", + "@openzeppelin/contracts-upgradeable/token/ERC721/Contract.sol", + "@openzeppelin/contracts-upgradeable/proxy/beacon/Contract.sol", + "@openzeppelin/contracts/token/ERC1155/utils/Contract.sol", + "@openzeppelin/contracts/token/ERC777/presets/Contract.sol", + "@openzeppelin/contracts-upgradeable/token/ERC20/Contract.sol", + "@openzeppelin/contracts-upgradeable/utils/structs/Contract.sol", + "@openzeppelin/contracts/utils/escrow/Contract.sol", + "@openzeppelin/contracts/utils/Contract.sol", + "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/Contract.sol", + "@openzeppelin/contracts/token/ERC721/extensions/Contract.sol", + "@openzeppelin/contracts-upgradeable/token/ERC777/Contract.sol", + "@openzeppelin/contracts/token/ERC1155/presets/Contract.sol", + "@openzeppelin/contracts/token/ERC721/Contract.sol", + "@openzeppelin/contracts/token/ERC1155/Contract.sol", + "@openzeppelin/contracts-upgradeable/governance/Contract.sol", + "@openzeppelin/contracts/token/ERC20/extensions/Contract.sol", + "@openzeppelin/contracts-upgradeable/utils/cryptography/Contract.sol", + "@openzeppelin/contracts-upgradeable/token/ERC1155/presets/Contract.sol", + "@openzeppelin/contracts/access/Contract.sol", + "@openzeppelin/contracts/governance/utils/Contract.sol", + "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/Contract.sol", + "@openzeppelin/contracts-upgradeable/token/common/Contract.sol", + "@openzeppelin/contracts-upgradeable/token/ERC1155/utils/Contract.sol", + "@openzeppelin/contracts/proxy/ERC1967/Contract.sol", + "@openzeppelin/contracts/finance/Contract.sol", + "@openzeppelin/contracts-upgradeable/token/ERC1155/extensions/Contract.sol", + "@openzeppelin/contracts-upgradeable/governance/utils/Contract.sol", + "@openzeppelin/contracts-upgradeable/proxy/utils/Contract.sol", + "@openzeppelin/contracts/token/ERC20/presets/Contract.sol", + "@openzeppelin/contracts/utils/math/Contract.sol", + "@openzeppelin/contracts-upgradeable/token/ERC721/presets/Contract.sol", + "@openzeppelin/contracts-upgradeable/finance/Contract.sol", + "@openzeppelin/contracts/utils/introspection/Contract.sol", + ]; + mkdir_or_touch(tmp_dir.path(), &paths[..]); + let remappings = Remapping::find_many(tmp_dir.path()); + + let expected = vec![Remapping { + name: "@openzeppelin/".to_string(), + path: to_str(tmp_dir.path().join("@openzeppelin")), + }]; + pretty_assertions::assert_eq!(remappings, expected); + } + #[test] fn can_resolve_oz_remappings() { let tmp_dir = tempdir("node_modules").unwrap(); diff --git a/ethers-solc/src/resolver.rs b/ethers-solc/src/resolver.rs index 19367c38..2a295baa 100644 --- a/ethers-solc/src/resolver.rs +++ b/ethers-solc/src/resolver.rs @@ -56,7 +56,7 @@ use regex::Match; use semver::VersionReq; use solang_parser::pt::{Import, Loc, SourceUnitPart}; -use crate::{error::Result, utils, ProjectPathsConfig, Solc, Source, Sources}; +use crate::{error::Result, utils, ProjectPathsConfig, Solc, SolcError, Source, Sources}; /// The underlying edges of the graph which only contains the raw relationship data. /// @@ -210,7 +210,7 @@ impl Graph { let mut unresolved: VecDeque<(PathBuf, Node)> = sources .into_par_iter() .map(|(path, source)| { - let data = parse_data(source.as_ref()); + let data = parse_data(source.as_ref(), &path); (path.clone(), Node { path, source, data }) }) .collect(); @@ -452,7 +452,7 @@ impl Graph { Ok(versioned_nodes) } else { tracing::error!("failed to resolve versions"); - Err(crate::error::SolcError::msg(errors.join("\n"))) + Err(SolcError::msg(errors.join("\n"))) } } @@ -584,8 +584,6 @@ impl VersionedSources { self, allowed_lib_paths: &crate::AllowedLibPaths, ) -> Result> { - use crate::SolcError; - // we take the installer lock here to ensure installation checking is done in sync #[cfg(any(test, feature = "tests"))] let _lock = crate::compile::take_solc_installer_lock(); @@ -720,8 +718,8 @@ impl From for Location { fn read_node(file: impl AsRef) -> Result { let file = file.as_ref(); - let source = Source::read(file)?; - let data = parse_data(source.as_ref()); + let source = Source::read(file).map_err(SolcError::Resolve)?; + let data = parse_data(source.as_ref(), file); Ok(Node { path: file.to_path_buf(), source, data }) } @@ -729,7 +727,7 @@ fn read_node(file: impl AsRef) -> Result { /// /// This will attempt to parse the solidity AST and extract the imports and version pragma. If /// parsing fails, we'll fall back to extract that info via regex -fn parse_data(content: &str) -> SolData { +fn parse_data(content: &str, file: &Path) -> SolData { let mut version = None; let mut imports = Vec::>::new(); match solang_parser::parse(content, 0) { @@ -756,7 +754,8 @@ fn parse_data(content: &str) -> SolData { } Err(err) => { tracing::trace!( - "failed to parse solidity ast: \"{:?}\". Falling back to regex to extract data", + "failed to parse \"{}\" ast: \"{:?}\". Falling back to regex to extract data", + file.display(), err ); version = capture_outer_and_inner(content, &utils::RE_SOL_PRAGMA_VERSION, &["version"]) @@ -764,9 +763,8 @@ fn parse_data(content: &str) -> SolData { .map(|(cap, name)| { SolDataUnit::new(name.as_str().to_owned(), cap.to_owned().into()) }); - imports = capture_outer_and_inner(content, &utils::RE_SOL_IMPORT, &["p1", "p2", "p3"]) - .iter() - .map(|(cap, m)| SolDataUnit::new(PathBuf::from(m.as_str()), cap.to_owned().into())) + imports = utils::find_import_paths(content) + .map(|m| SolDataUnit::new(PathBuf::from(m.as_str()), m.to_owned().into())) .collect(); } }; diff --git a/ethers-solc/src/utils.rs b/ethers-solc/src/utils.rs index ae0da1fa..a5e5978c 100644 --- a/ethers-solc/src/utils.rs +++ b/ethers-solc/src/utils.rs @@ -17,7 +17,7 @@ use walkdir::WalkDir; /// statement with the named groups "path", "id". // Adapted from https://github.com/nomiclabs/hardhat/blob/cced766c65b25d3d0beb39ef847246ac9618bdd9/packages/hardhat-core/src/internal/solidity/parse.ts#L100 pub static RE_SOL_IMPORT: Lazy = Lazy::new(|| { - Regex::new(r#"import\s+(?:(?:"(?P[^;]*)"|'([^;]*)')(?:;|\s+as\s+(?P[^;]*);)|.+from\s+(?:"(?P.*)"|'(?P.*)');)"#).unwrap() + Regex::new(r#"import\s+(?:(?:"(?P[^;]*)"|'(?P[^;]*)')(?:;|\s+as\s+(?P[^;]*);)|.+from\s+(?:"(?P.*)"|'(?P.*)');)"#).unwrap() }); /// A regex that matches the version part of a solidity pragma @@ -37,9 +37,12 @@ pub static RE_SOL_SDPX_LICENSE_IDENTIFIER: Lazy = /// /// See also https://docs.soliditylang.org/en/v0.8.9/grammar.html pub fn find_import_paths(contract: &str) -> impl Iterator { - RE_SOL_IMPORT - .captures_iter(contract) - .filter_map(|cap| cap.name("p1").or_else(|| cap.name("p2")).or_else(|| cap.name("p3"))) + RE_SOL_IMPORT.captures_iter(contract).filter_map(|cap| { + cap.name("p1") + .or_else(|| cap.name("p2")) + .or_else(|| cap.name("p3")) + .or_else(|| cap.name("p4")) + }) } /// Returns the solidity version pragma from the given input: @@ -380,6 +383,31 @@ mod tests { assert_eq!(files, expected); } + #[test] + fn can_find_single_quote_imports() { + let content = r#" +// SPDX-License-Identifier: MIT +pragma solidity 0.8.6; + +import '@openzeppelin/contracts/access/Ownable.sol'; +import '@openzeppelin/contracts/utils/Address.sol'; + +import './../interfaces/IJBDirectory.sol'; +import './../libraries/JBTokens.sol'; + "#; + let imports: Vec<_> = find_import_paths(content).map(|m| m.as_str()).collect(); + + assert_eq!( + imports, + vec![ + "@openzeppelin/contracts/access/Ownable.sol", + "@openzeppelin/contracts/utils/Address.sol", + "./../interfaces/IJBDirectory.sol", + "./../libraries/JBTokens.sol", + ] + ); + } + #[test] fn can_find_import_paths() { let s = r##"//SPDX-License-Identifier: Unlicense