Repository: gakonst/ethers-rs
Pull request #1545 (2022-08-01 13:45:33)

refactor(solc): improve error message for unresolved imports
<!--
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
Closes https://github.com/foundry-rs/foundry/issues/2526
<!--
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
improve output:
* only include remappings once
* include file paths where occurred in
<!--
Summarize the solution and provide any necessary context needed to understand
the code change.
-->

## PR Checklist

- [x] Added Tests
- [ ] Added Documentation
- [ ] Updated the changelog



Patch:
diff --git a/ethers-contract/ethers-contract-abigen/src/contract/methods.rs b/ethers-contract/ethers-contract-abigen/src/contract/methods.rs
index 3894802b..11b1641a 100644
--- a/ethers-contract/ethers-contract-abigen/src/contract/methods.rs
+++ b/ethers-contract/ethers-contract-abigen/src/contract/methods.rs
@@ -281,13 +281,9 @@ impl Context {
         util::ident(&format!("{}Calls", self.contract_ident))
     }
 
-    /// Expands to the `name : type` pairs of the function's parameters
-    fn expand_params(
-        &self,
-        fun: &Function,
-        params: &[Param],
-    ) -> Result<Vec<(TokenStream, TokenStream)>> {
-        params
+    /// Expands to the `name : type` pairs of the function's inputs
+    fn expand_input_params(&self, fun: &Function) -> Result<Vec<(TokenStream, TokenStream)>> {
+        fun.inputs
             .iter()
             .enumerate()
             .map(|(idx, param)| {
@@ -298,14 +294,17 @@ impl Context {
             .collect()
     }
 
-    /// Expands to the `name : type` pairs of the function's inputs
-    fn expand_input_params(&self, fun: &Function) -> Result<Vec<(TokenStream, TokenStream)>> {
-        self.expand_params(fun, &fun.inputs)
-    }
-
     /// Expands to the `name : type` pairs of the function's outputs
     fn expand_output_params(&self, fun: &Function) -> Result<Vec<(TokenStream, TokenStream)>> {
-        self.expand_params(fun, &fun.outputs)
+        fun.outputs
+            .iter()
+            .enumerate()
+            .map(|(idx, param)| {
+                let name = util::expand_input_name(idx, &param.name);
+                let ty = self.expand_output_param_type(fun, param, &param.kind)?;
+                Ok((name, ty))
+            })
+            .collect()
     }
 
     /// Expands to the return type of a function
diff --git a/ethers-solc/src/report/mod.rs b/ethers-solc/src/report/mod.rs
index 4c7a572d..f664afca 100644
--- a/ethers-solc/src/report/mod.rs
+++ b/ethers-solc/src/report/mod.rs
@@ -134,8 +134,9 @@ pub trait Reporter: 'static + std::fmt::Debug {
     /// Invoked after a [`Solc`] installation failed
     fn on_solc_installation_error(&self, _version: &Version, _error: &str) {}
 
-    /// Invoked if the import couldn't be resolved with these remappings
-    fn on_unresolved_import(&self, _import: &Path, _remappings: &[Remapping]) {}
+    /// Invoked if imports couldn't be resolved with the given remappings, where `imports` is the
+    /// list of all import paths and the file they occurred in: `(import stmt, file)`
+    fn on_unresolved_imports(&self, _imports: &[(&Path, &Path)], _remappings: &[Remapping]) {}
 
     /// If `self` is the same type as the provided `TypeId`, returns an untyped
     /// [`NonNull`] pointer to that type. Otherwise, returns `None`.
@@ -213,8 +214,8 @@ pub(crate) fn solc_installation_error(version: &Version, error: &str) {
     get_default(|r| r.reporter.on_solc_installation_error(version, error));
 }
 
-pub(crate) fn unresolved_import(import: &Path, remappings: &[Remapping]) {
-    get_default(|r| r.reporter.on_unresolved_import(import, remappings));
+pub(crate) fn unresolved_imports(imports: &[(&Path, &Path)], remappings: &[Remapping]) {
+    get_default(|r| r.reporter.on_unresolved_imports(imports, remappings));
 }
 
 fn get_global() -> Option<&'static Report> {
@@ -388,15 +389,28 @@ impl Reporter for BasicStdoutReporter {
         eprintln!("Failed to install solc {}: {}", version, error);
     }
 
-    fn on_unresolved_import(&self, import: &Path, remappings: &[Remapping]) {
-        println!(
-            "Unable to resolve import: \"{}\" with remappings:\n        {}",
-            import.display(),
-            remappings.iter().map(|r| r.to_string()).collect::<Vec<_>>().join("\n        ")
-        );
+    fn on_unresolved_imports(&self, imports: &[(&Path, &Path)], remappings: &[Remapping]) {
+        if imports.is_empty() {
+            return
+        }
+        println!("{}", format_unresolved_imports(imports, remappings))
     }
 }
 
+/// Creates a meaningful message for all unresolved imports
+pub fn format_unresolved_imports(imports: &[(&Path, &Path)], remappings: &[Remapping]) -> String {
+    let info = imports
+        .iter()
+        .map(|(import, file)| format!("\"{}\" in \"{}\"", import.display(), file.display()))
+        .collect::<Vec<_>>()
+        .join("\n      ");
+    format!(
+        "Unable to resolve imports:\n      {}\nwith remappings:\n      {}",
+        info,
+        remappings.iter().map(|r| r.to_string()).collect::<Vec<_>>().join("\n      ")
+    )
+}
+
 /// Returned if setting the global reporter fails.
 #[derive(Debug)]
 pub struct SetGlobalReporterError {
@@ -473,6 +487,7 @@ fn set_global_reporter(report: Report) -> Result<(), SetGlobalReporterError> {
 #[cfg(test)]
 mod tests {
     use super::*;
+    use std::str::FromStr;
 
     #[test]
     fn scoped_reporter_works() {
@@ -502,4 +517,21 @@ mod tests {
 
         get_default(|reporter| assert!(reporter.is::<BasicStdoutReporter>()))
     }
+
+    #[test]
+    fn test_unresolved_message() {
+        let unresolved = vec![(Path::new("./src/Import.sol"), Path::new("src/File.col"))];
+
+        let remappings = vec![Remapping::from_str("oz=a/b/c/d").unwrap()];
+
+        assert_eq!(
+            format_unresolved_imports(&unresolved, &remappings).trim(),
+            r#"
+Unable to resolve imports:
+      "./src/Import.sol" in "src/File.col"
+with remappings:
+      oz=a/b/c/d/"#
+                .trim()
+        )
+    }
 }
diff --git a/ethers-solc/src/resolver/mod.rs b/ethers-solc/src/resolver/mod.rs
index 571e3602..374f678e 100644
--- a/ethers-solc/src/resolver/mod.rs
+++ b/ethers-solc/src/resolver/mod.rs
@@ -88,8 +88,8 @@ pub struct GraphEdges {
     /// Combined with the `indices` this way we can determine if a file was original added to the
     /// graph as input or was added as resolved import, see [`Self::is_input_file()`]
     num_input_files: usize,
-    /// tracks all imports that we failed to resolve
-    unresolved_imports: HashSet<PathBuf>,
+    /// tracks all imports that we failed to resolve for a file
+    unresolved_imports: HashSet<(PathBuf, PathBuf)>,
 }
 
 impl GraphEdges {
@@ -114,7 +114,7 @@ impl GraphEdges {
     }
 
     /// Returns all imports that we failed to resolve
-    pub fn unresolved_imports(&self) -> &HashSet<PathBuf> {
+    pub fn unresolved_imports(&self) -> &HashSet<(PathBuf, PathBuf)> {
         &self.unresolved_imports
     }
 
@@ -354,9 +354,7 @@ impl Graph {
                         add_node(&mut unresolved, &mut index, &mut resolved_imports, import)?;
                     }
                     Err(err) => {
-                        if unresolved_imports.insert(import_path.to_path_buf()) {
-                            crate::report::unresolved_import(import_path, &paths.remappings);
-                        }
+                        unresolved_imports.insert((import_path.to_path_buf(), node.path.clone()));
                         tracing::trace!(
                             "failed to resolve import component \"{:?}\" for {:?}",
                             err,
@@ -369,6 +367,18 @@ impl Graph {
             nodes.push(node);
             edges.push(resolved_imports);
         }
+
+        if !unresolved_imports.is_empty() {
+            // notify on all unresolved imports
+            crate::report::unresolved_imports(
+                &unresolved_imports
+                    .iter()
+                    .map(|(i, f)| (i.as_path(), f.as_path()))
+                    .collect::<Vec<_>>(),
+                &paths.remappings,
+            );
+        }
+
         let edges = GraphEdges {
             edges,
             rev_indices: index.iter().map(|(k, v)| (*v, k.clone())).collect(),


Test patch:
diff --git a/ethers-contract/tests/it/abigen.rs b/ethers-contract/tests/it/abigen.rs
index fd661ad0..9ca7e9a9 100644
--- a/ethers-contract/tests/it/abigen.rs
+++ b/ethers-contract/tests/it/abigen.rs
@@ -659,3 +659,10 @@ fn can_generate_to_string_overload() {
 fn can_generate_large_event() {
     abigen!(NewSale, "ethers-contract/tests/solidity-contracts/sale.json");
 }
+
+#[test]
+fn can_generate_large_output_struct() {
+    abigen!(LargeOutputStruct, "ethers-contract/tests/solidity-contracts/LargeStruct.json");
+
+    let r = GetByIdReturn(Info::default());
+}
diff --git a/ethers-contract/tests/solidity-contracts/LargeStruct.json b/ethers-contract/tests/solidity-contracts/LargeStruct.json
new file mode 100644
index 00000000..04e4531b
--- /dev/null
+++ b/ethers-contract/tests/solidity-contracts/LargeStruct.json
@@ -0,0 +1,68 @@
+[
+  {
+    "inputs": [
+      {
+        "internalType": "bytes32",
+        "name": "id",
+        "type": "bytes32"
+      }
+    ],
+    "name": "getById",
+    "outputs": [
+      {
+        "components": [
+          {
+            "internalType": "uint128",
+            "name": "x",
+            "type": "uint128"
+          },
+          {
+            "internalType": "int24",
+            "name": "y",
+            "type": "int24"
+          },
+          {
+            "internalType": "int24",
+            "name": "z",
+            "type": "int24"
+          },
+          {
+            "internalType": "uint256",
+            "name": "a",
+            "type": "uint256"
+          },
+          {
+            "internalType": "int256",
+            "name": "b",
+            "type": "int256"
+          },
+          {
+            "internalType": "int256",
+            "name": "c",
+            "type": "int256"
+          },
+          {
+            "internalType": "int256",
+            "name": "d",
+            "type": "int256"
+          },
+          {
+            "internalType": "uint256",
+            "name": "e",
+            "type": "uint256"
+          },
+          {
+            "internalType": "uint256",
+            "name": "f",
+            "type": "uint256"
+          }
+        ],
+        "internalType": "struct Many.Info",
+        "name": "",
+        "type": "tuple"
+      }
+    ],
+    "stateMutability": "view",
+    "type": "function"
+  }
+]
\ No newline at end of file