From fbc109a50cfcb8d1472b2d0b08a123c84c40aa33 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Mon, 16 Sep 2024 22:02:00 +0300 Subject: [PATCH 1/5] feat: better error messages for incompatible versions --- crates/compilers/src/resolver/mod.rs | 98 ++++++++++++++++++++-------- crates/core/src/error.rs | 2 +- 2 files changed, 70 insertions(+), 30 deletions(-) diff --git a/crates/compilers/src/resolver/mod.rs b/crates/compilers/src/resolver/mod.rs index 01e3e520..da62eff0 100644 --- a/crates/compilers/src/resolver/mod.rs +++ b/crates/compilers/src/resolver/mod.rs @@ -547,6 +547,7 @@ impl> Graph { fn format_imports_list( &self, idx: usize, + imports: impl IntoIterator, f: &mut W, ) -> std::result::Result<(), std::fmt::Error> { let node = self.node(idx); @@ -555,7 +556,7 @@ impl> Graph { write!(f, "{req}")?; } write!(f, " imports:")?; - for dep in self.node_ids(idx).skip(1) { + for dep in imports { let dep = self.node(dep); write!(f, "\n {} ", utils::source_name(&dep.path, &self.root).display())?; if let Some(req) = dep.data.version_req() { @@ -566,18 +567,74 @@ impl> Graph { Ok(()) } - /// Filters incompatible versions from the `candidates`. - fn retain_compatible_versions(&self, idx: usize, candidates: &mut Vec<&CompilerVersion>) { - let nodes: HashSet<_> = self.node_ids(idx).collect(); - for node in nodes { - if let Some(req) = &self.node(node).data.version_req() { + /// Filters incompatible versions from the `candidates`. It iterates over node imports and in + /// case if there is no compatible version it returns the latest seen node id. + fn retain_compatible_versions( + &self, + idx: usize, + candidates: &mut Vec<&CompilerVersion>, + offline: bool, + ) -> Result<(), String> { + let mut all_versions = candidates.clone(); + + let nodes: Vec<_> = self.node_ids(idx).collect(); + let mut failed_node = None; + for node in nodes.iter() { + if let Some(req) = self.node(*node).data.version_req() { candidates.retain(|v| req.matches(v.as_ref())); + + if candidates.is_empty() { + failed_node = Some(*node); + break; + } + } + } + + let Some(failed_node_idx) = failed_node else { + // everything is fine + return Ok(()); + }; + + // This now keeps data for the node which were the last one before we had no candidates + // left. It means that there + let failed_node = self.node(failed_node_idx); + + if let Err(version_err) = failed_node.check_available_version(&all_versions, offline) { + // check if the version is even valid + let f = utils::source_name(&failed_node.path, &self.root).display(); + return Err( + format!("Encountered invalid solc version in {f}: {version_err}").to_string() + ); + } else { + // if the node requirement makes sense, it means that there is at least one node + // which requirement conflicts with it + + // retain only versions compatible with the `failed_node` + if let Some(req) = failed_node.data.version_req() { + all_versions.retain(|v| req.matches(v.as_ref())); } - if candidates.is_empty() { - // nothing to filter anymore - return; + + // iterate over all the nodes once again and find the one incompatible + for node in &nodes { + if self.node(*node).check_available_version(&all_versions, offline).is_err() { + let mut msg = String::new(); + + // avoid formatting root node as import + let imports = if *node == idx { + vec![failed_node_idx] + } else { + vec![*node, failed_node_idx] + }; + + self.format_imports_list(idx, imports, &mut msg).unwrap(); + return Err(format!("Found incompatible versions:\n{msg}")); + } } } + + let mut msg = String::new(); + self.format_imports_list(idx, nodes[1..].to_vec(), &mut msg).unwrap(); + Err(format!("Found incompatible versions:\n{msg}")); } fn input_nodes_by_language(&self) -> HashMap> { @@ -619,8 +676,6 @@ impl> Graph { // exit on first error, instead gather all the errors and return a bundled // error message instead let mut errors = Vec::new(); - // we also don't want duplicate error diagnostic - let mut erroneous_nodes = HashSet::with_capacity(self.edges.num_input_files); // the sorted list of all versions let all_versions = if offline { @@ -649,23 +704,8 @@ impl> Graph { let mut candidates = all_versions.iter().collect::>(); // remove all incompatible versions from the candidates list by checking the node // and all its imports - self.retain_compatible_versions(idx, &mut candidates); - - if candidates.is_empty() && !erroneous_nodes.contains(&idx) { - // check if the version is even valid - let node = self.node(idx); - if let Err(version_err) = node.check_available_version(&all_versions, offline) { - let f = utils::source_name(&node.path, &self.root).display(); - errors.push(format!( - "Encountered invalid solc version in {f}: {version_err}" - )); - } else { - let mut msg = String::new(); - self.format_imports_list(idx, &mut msg).unwrap(); - errors.push(format!("Found incompatible versions:\n{msg}")); - } - - erroneous_nodes.insert(idx); + if let Err(err) = self.retain_compatible_versions(idx, &mut candidates, offline) { + errors.push(err); } else { // found viable candidates, pick the most recent version that's already // installed @@ -878,7 +918,7 @@ impl Node { /// 0.8.20, if the highest available version is `0.8.19` fn check_available_version( &self, - all_versions: &[CompilerVersion], + all_versions: &[&CompilerVersion], offline: bool, ) -> std::result::Result<(), SourceVersionError> { let Some(req) = self.data.version_req() else { return Ok(()) }; diff --git a/crates/core/src/error.rs b/crates/core/src/error.rs index aa9e5221..d58b0f44 100644 --- a/crates/core/src/error.rs +++ b/crates/core/src/error.rs @@ -5,7 +5,7 @@ use std::{ }; use thiserror::Error; -pub type Result = std::result::Result; +pub type Result = std::result::Result; #[allow(unused_macros)] #[macro_export] From a801cd57907aa36d2044d37d50b0d30ba90e0ec3 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Mon, 16 Sep 2024 22:30:09 +0300 Subject: [PATCH 2/5] fix --- crates/compilers/src/resolver/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/compilers/src/resolver/mod.rs b/crates/compilers/src/resolver/mod.rs index da62eff0..4fca86e7 100644 --- a/crates/compilers/src/resolver/mod.rs +++ b/crates/compilers/src/resolver/mod.rs @@ -634,7 +634,7 @@ impl> Graph { let mut msg = String::new(); self.format_imports_list(idx, nodes[1..].to_vec(), &mut msg).unwrap(); - Err(format!("Found incompatible versions:\n{msg}")); + Err(format!("Found incompatible versions:\n{msg}")) } fn input_nodes_by_language(&self) -> HashMap> { From af6625556705a9126cc2224acca09f08f0f9faa8 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Mon, 16 Sep 2024 23:20:49 +0300 Subject: [PATCH 3/5] update formatting --- crates/compilers/src/resolver/mod.rs | 47 ++++++++++++++-------------- 1 file changed, 23 insertions(+), 24 deletions(-) diff --git a/crates/compilers/src/resolver/mod.rs b/crates/compilers/src/resolver/mod.rs index 4fca86e7..86055bf5 100644 --- a/crates/compilers/src/resolver/mod.rs +++ b/crates/compilers/src/resolver/mod.rs @@ -64,6 +64,7 @@ use std::{ io, path::{Path, PathBuf}, }; +use yansi::{Color, Paint}; pub mod parse; mod tree; @@ -547,21 +548,25 @@ impl> Graph { fn format_imports_list( &self, idx: usize, - imports: impl IntoIterator, + incompatible: HashSet, f: &mut W, ) -> std::result::Result<(), std::fmt::Error> { - let node = self.node(idx); - write!(f, "{} ", utils::source_name(&node.path, &self.root).display())?; - if let Some(req) = node.data.version_req() { - write!(f, "{req}")?; - } - write!(f, " imports:")?; - for dep in imports { - let dep = self.node(dep); - write!(f, "\n {} ", utils::source_name(&dep.path, &self.root).display())?; - if let Some(req) = dep.data.version_req() { - write!(f, "{req}")?; + let format_node = |idx, f: &mut W| { + let node = self.node(idx); + let color = if incompatible.contains(&idx) { Color::Red } else { Color::White }; + + let mut line = utils::source_name(&node.path, &self.root).display().to_string(); + if let Some(req) = node.data.version_req() { + line.push_str(&format!(" {req}")); } + + write!(f, "{} ", line.paint(color)) + }; + format_node(idx, f)?; + write!(f, " imports:")?; + for dep in self.node_ids(idx).skip(1) { + write!(f, "\n ")?; + format_node(dep, f)?; } Ok(()) @@ -617,23 +622,17 @@ impl> Graph { // iterate over all the nodes once again and find the one incompatible for node in &nodes { if self.node(*node).check_available_version(&all_versions, offline).is_err() { - let mut msg = String::new(); - - // avoid formatting root node as import - let imports = if *node == idx { - vec![failed_node_idx] - } else { - vec![*node, failed_node_idx] - }; + let mut msg = "Found incompatible versions:\n".white().to_string(); - self.format_imports_list(idx, imports, &mut msg).unwrap(); - return Err(format!("Found incompatible versions:\n{msg}")); + self.format_imports_list(idx, [*node, failed_node_idx].into(), &mut msg) + .unwrap(); + return Err(msg); } } } - let mut msg = String::new(); - self.format_imports_list(idx, nodes[1..].to_vec(), &mut msg).unwrap(); + let mut msg = "Found incompatible versions:\n".white().to_string(); + self.format_imports_list(idx, nodes.into_iter().collect(), &mut msg).unwrap(); Err(format!("Found incompatible versions:\n{msg}")) } From 814a4d9098e4a7e4d1aa14d44cb6b42caa1ca72b Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Tue, 17 Sep 2024 15:08:37 +0300 Subject: [PATCH 4/5] fix error --- crates/compilers/src/resolver/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/compilers/src/resolver/mod.rs b/crates/compilers/src/resolver/mod.rs index 86055bf5..96315781 100644 --- a/crates/compilers/src/resolver/mod.rs +++ b/crates/compilers/src/resolver/mod.rs @@ -633,7 +633,7 @@ impl> Graph { let mut msg = "Found incompatible versions:\n".white().to_string(); self.format_imports_list(idx, nodes.into_iter().collect(), &mut msg).unwrap(); - Err(format!("Found incompatible versions:\n{msg}")) + Err(msg) } fn input_nodes_by_language(&self) -> HashMap> { From 488a4cdd8999e43d6063a50885b4b646b7c7e183 Mon Sep 17 00:00:00 2001 From: Arsenii Kulikov Date: Tue, 17 Sep 2024 15:18:18 +0300 Subject: [PATCH 5/5] add test --- Cargo.toml | 2 ++ crates/compilers/Cargo.toml | 1 + crates/compilers/src/resolver/mod.rs | 28 +++++++++++++++++++++++- test-data/incompatible-pragmas/src/A.sol | 3 +++ test-data/incompatible-pragmas/src/B.sol | 1 + test-data/incompatible-pragmas/src/C.sol | 1 + 6 files changed, 35 insertions(+), 1 deletion(-) create mode 100644 test-data/incompatible-pragmas/src/A.sol create mode 100644 test-data/incompatible-pragmas/src/B.sol create mode 100644 test-data/incompatible-pragmas/src/C.sol diff --git a/Cargo.toml b/Cargo.toml index e4936c13..a3f75976 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -62,3 +62,5 @@ yansi = "1.0" # async futures-util = "0.3" tokio = { version = "1.35", features = ["rt-multi-thread"] } + +snapbox = "0.6.9" diff --git a/crates/compilers/Cargo.toml b/crates/compilers/Cargo.toml index b4220549..ecda8dc5 100644 --- a/crates/compilers/Cargo.toml +++ b/crates/compilers/Cargo.toml @@ -61,6 +61,7 @@ fd-lock = "4.0.0" tokio = { version = "1.35", features = ["rt-multi-thread", "macros"] } reqwest = "0.12" tempfile = "3.9" +snapbox.workspace = true foundry-compilers-core = { workspace = true, features = ["test-utils"] } [features] diff --git a/crates/compilers/src/resolver/mod.rs b/crates/compilers/src/resolver/mod.rs index 96315781..cc3a135e 100644 --- a/crates/compilers/src/resolver/mod.rs +++ b/crates/compilers/src/resolver/mod.rs @@ -560,7 +560,7 @@ impl> Graph { line.push_str(&format!(" {req}")); } - write!(f, "{} ", line.paint(color)) + write!(f, "{}", line.paint(color)) }; format_node(idx, f)?; write!(f, " imports:")?; @@ -1052,6 +1052,32 @@ src/Dapp.t.sol >=0.6.6 ); } + #[test] + #[cfg(feature = "svm-solc")] + fn test_print_unresolved() { + let root = + Path::new(env!("CARGO_MANIFEST_DIR")).join("../../test-data/incompatible-pragmas"); + let paths = ProjectPathsConfig::dapptools(&root).unwrap(); + let graph = Graph::::resolve(&paths).unwrap(); + let Err(SolcError::Message(err)) = graph.get_input_node_versions( + false, + &Default::default(), + &crate::solc::SolcCompiler::AutoDetect, + ) else { + panic!("expected error"); + }; + + snapbox::assert_data_eq!( + err, + snapbox::str![[r#" +Found incompatible versions: +src/A.sol =0.8.25 imports: + src/B.sol + src/C.sol =0.7.0 +"#]] + ); + } + #[cfg(target_os = "linux")] #[test] fn can_read_different_case() { diff --git a/test-data/incompatible-pragmas/src/A.sol b/test-data/incompatible-pragmas/src/A.sol new file mode 100644 index 00000000..9dec8d7f --- /dev/null +++ b/test-data/incompatible-pragmas/src/A.sol @@ -0,0 +1,3 @@ +pragma solidity 0.8.25; + +import "./B.sol"; \ No newline at end of file diff --git a/test-data/incompatible-pragmas/src/B.sol b/test-data/incompatible-pragmas/src/B.sol new file mode 100644 index 00000000..2b86db8f --- /dev/null +++ b/test-data/incompatible-pragmas/src/B.sol @@ -0,0 +1 @@ +import "./C.sol"; \ No newline at end of file diff --git a/test-data/incompatible-pragmas/src/C.sol b/test-data/incompatible-pragmas/src/C.sol new file mode 100644 index 00000000..c6f01253 --- /dev/null +++ b/test-data/incompatible-pragmas/src/C.sol @@ -0,0 +1 @@ +pragma solidity 0.7.0; \ No newline at end of file