From 56911f205a4e4a54aaad0e3128b69ddd14c863b8 Mon Sep 17 00:00:00 2001 From: William Cheung Date: Sat, 6 Jul 2024 14:43:45 -0400 Subject: [PATCH] Fix inconsistent trailing slash in remappings (#49) Fix #47. One corresponding test is added. ## Fix strategy - Remove the code that blindly add trailing `/` when serializing `Remapping` and `RelativeRemapping`. - Preserve the trailing `/` in the `strip_prefix` method of `Remapping`. ## Rationale There are roughly two sources where `Remapping` comes from: - Generated in `find_many` function. - Loaded from configuration like `foundry.toml`. When generating `Remapping` in `find_many` function, the algorithm makes sure that the `name` and `path` of every remapping are valid folders in the file system. Then, trailing `/`s are added to both `name` and `path` in the `impl From for Remapping` implementation. https://github.com/Troublor/compilers/blob/93c5f46a7dd0c0d387dd94c8ea756812e2907d92/src/remappings.rs#L360-L365 When loading `Remapping` from user-provided configurations, the `name` and `path` of remappings should be set as it is, and do not add any trailing `/`s. Either way, the field `name` and `path` of `Remapping` contains intended values and should not be changed in the serialization (the implementation for `Display` trait). PS: About coding style, I tried to use `TempProject` to write the test case but it seems the `project.compile()` method **does not respect** the `remapping` I give it via `project.with_settings(settings_with_remapping)`. The remapping just does not take effect in the compilation. Not sure whether it's I don't know how to use it or `TempProject` has some bugs. --- crates/artifacts/solc/src/remappings.rs | 10 +++--- crates/compilers/src/report/mod.rs | 2 +- crates/compilers/tests/project.rs | 43 ++++++++++++++++++++++--- 3 files changed, 45 insertions(+), 10 deletions(-) diff --git a/crates/artifacts/solc/src/remappings.rs b/crates/artifacts/solc/src/remappings.rs index e6633bcb..8a5bcc4a 100644 --- a/crates/artifacts/solc/src/remappings.rs +++ b/crates/artifacts/solc/src/remappings.rs @@ -143,16 +143,18 @@ impl fmt::Display for Remapping { } s.push(':'); } + let name = + if !self.name.ends_with('/') { format!("{}/", self.name) } else { self.name.clone() }; s.push_str(&{ #[cfg(target_os = "windows")] { // ensure we have `/` slashes on windows use path_slash::PathExt; - format!("{}={}", self.name, std::path::Path::new(&self.path).to_slash_lossy()) + format!("{}={}", name, std::path::Path::new(&self.path).to_slash_lossy()) } #[cfg(not(target_os = "windows"))] { - format!("{}={}", self.name, self.path) + format!("{}={}", name, self.path) } }); @@ -1161,7 +1163,7 @@ mod tests { path: "a/b/c/d".to_string(), } ); - assert_eq!(remapping.to_string(), "context:oz=a/b/c/d/".to_string()); + assert_eq!(remapping.to_string(), "context:oz/=a/b/c/d/".to_string()); let remapping = "context:foo=C:/bar/src/"; let remapping = Remapping::from_str(remapping).unwrap(); @@ -1185,7 +1187,7 @@ mod tests { remapping, Remapping { context: None, name: "oz".to_string(), path: "a/b/c/d/".to_string() } ); - assert_eq!(remapping.to_string(), "oz=a/b/c/d/".to_string()); + assert_eq!(remapping.to_string(), "oz/=a/b/c/d/".to_string()); } #[test] diff --git a/crates/compilers/src/report/mod.rs b/crates/compilers/src/report/mod.rs index 7ac1538e..f54edf1c 100644 --- a/crates/compilers/src/report/mod.rs +++ b/crates/compilers/src/report/mod.rs @@ -492,7 +492,7 @@ mod tests { Unable to resolve imports: "./src/Import.sol" in "src/File.col" with remappings: - oz=a/b/c/d/"# + oz/=a/b/c/d/"# .trim() ) } diff --git a/crates/compilers/tests/project.rs b/crates/compilers/tests/project.rs index e97b1282..c8b25ff7 100644 --- a/crates/compilers/tests/project.rs +++ b/crates/compilers/tests/project.rs @@ -716,7 +716,7 @@ fn can_flatten_on_solang_failure() { r#"// SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.10; -library Lib {} +library Lib {} "#, ) .unwrap(); @@ -748,7 +748,7 @@ pragma solidity ^0.8.10; // src/Lib.sol -library Lib {} +library Lib {} // src/Contract.sol @@ -2980,17 +2980,17 @@ fn can_parse_notice() { /** * @notice hello - */ + */ constructor(string memory _greeting) public { greeting = _greeting; } - + /** * @notice hello */ function xyz() public { } - + /// @notice hello function abc() public { } @@ -3986,3 +3986,36 @@ fn test_can_compile_multi() { assert!(compiled.find(&root.join("src/Counter.vy"), "Counter").is_some()); compiled.assert_success(); } + +// This is a reproduction of https://github.com/foundry-rs/compilers/issues/47 +#[cfg(feature = "svm-solc")] +#[test] +fn remapping_trailing_slash_issue47() { + use std::sync::Arc; + + use foundry_compilers_artifacts::{EvmVersion, Source, Sources}; + + let mut sources = Sources::new(); + sources.insert( + PathBuf::from("./C.sol"), + Source { + content: Arc::new(r#"import "@project/D.sol"; contract C {}"#.to_string()), + kind: Default::default(), + }, + ); + sources.insert( + PathBuf::from("./D.sol"), + Source { content: Arc::new(r#"contract D {}"#.to_string()), kind: Default::default() }, + ); + + let mut settings = Settings { evm_version: Some(EvmVersion::Byzantium), ..Default::default() }; + settings.remappings.push(Remapping { + context: None, + name: "@project".into(), + path: ".".into(), + }); + let input = SolcInput { language: SolcLanguage::Solidity, sources, settings }; + let compiler = Solc::find_or_install(&Version::new(0, 6, 8)).unwrap(); + let output = compiler.compile_exact(&input).unwrap(); + assert!(!output.has_error()); +}