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

@@ -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),

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`
+    /// There are several cases where multiple candidates are detected for the same level
-    fn is_candidate_source_a_source_dir(&self) -> bool {
+    ///
-        self.source_dir.ends_with(DAPPTOOLS_CONTRACTS_DIR) ||
+    /// # Example - Dapptools style
-            self.source_dir.ends_with(JS_CONTRACTS_DIR)
+    ///
+    /// 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<Candidate>,
+        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
+        Candidate::merge_on_same_level(&mut candidates, current_dir, current_level, window_start);
-        // ```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,
-            });
-        }
     } 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();

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<std::collections::BTreeMap<Solc, (semver::Version, Sources)>> {
-        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<Loc> for Location {
 
 fn read_node(file: impl AsRef<Path>) -> Result<Node> {
     let file = file.as_ref();
-    let source = Source::read(file)?;
+    let source = Source::read(file).map_err(SolcError::Resolve)?;
-    let data = parse_data(source.as_ref());
+    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<Path>) -> Result<Node> {
 ///
 /// 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::<SolDataUnit<PathBuf>>::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"])
+            imports = utils::find_import_paths(content)
-                .iter()
+                .map(|m| SolDataUnit::new(PathBuf::from(m.as_str()), m.to_owned().into()))
-                .map(|(cap, m)| SolDataUnit::new(PathBuf::from(m.as_str()), cap.to_owned().into()))
                 .collect();
         }
     };

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<Regex> = Lazy::new(|| {
-    Regex::new(r#"import\s+(?:(?:"(?P<p1>[^;]*)"|'([^;]*)')(?:;|\s+as\s+(?P<id>[^;]*);)|.+from\s+(?:"(?P<p2>.*)"|'(?P<p3>.*)');)"#).unwrap()
+    Regex::new(r#"import\s+(?:(?:"(?P<p1>[^;]*)"|'(?P<p2>[^;]*)')(?:;|\s+as\s+(?P<id>[^;]*);)|.+from\s+(?:"(?P<p3>.*)"|'(?P<p4>.*)');)"#).unwrap()
 });
 
 /// A regex that matches the version part of a solidity pragma
@@ -37,9 +37,12 @@ pub static RE_SOL_SDPX_LICENSE_IDENTIFIER: Lazy<Regex> =
 ///
 /// See also https://docs.soliditylang.org/en/v0.8.9/grammar.html
 pub fn find_import_paths(contract: &str) -> impl Iterator<Item = Match> {
-    RE_SOL_IMPORT
+    RE_SOL_IMPORT.captures_iter(contract).filter_map(|cap| {
-        .captures_iter(contract)
+        cap.name("p1")
-        .filter_map(|cap| cap.name("p1").or_else(|| cap.name("p2")).or_else(|| cap.name("p3")))
+            .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