From e817073f8e5dee2d9e3801561d436cd5c412890f Mon Sep 17 00:00:00 2001 From: Matthias Seitz Date: Mon, 1 Aug 2022 18:46:15 +0200 Subject: [PATCH] refactor(solc): improve error message for unresolved imports (#1545) * refactor(solc): improve error message for unresolved imports * chore: rustfmt --- ethers-solc/src/report/mod.rs | 52 ++++++++++++++++++++++++++------- ethers-solc/src/resolver/mod.rs | 22 ++++++++++---- 2 files changed, 58 insertions(+), 16 deletions(-) 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::>().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::>() + .join("\n "); + format!( + "Unable to resolve imports:\n {}\nwith remappings:\n {}", + info, + remappings.iter().map(|r| r.to_string()).collect::>().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::())) } + + #[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, + /// 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 { + 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::>(), + &paths.remappings, + ); + } + let edges = GraphEdges { edges, rev_indices: index.iter().map(|(k, v)| (*v, k.clone())).collect(),