diff --git a/cli/src/cmd/forge/build/core.rs b/cli/src/cmd/forge/build/core.rs index b557f978415c1..bc10d21f8a811 100644 --- a/cli/src/cmd/forge/build/core.rs +++ b/cli/src/cmd/forge/build/core.rs @@ -153,7 +153,7 @@ impl<'a> From<&'a CoreBuildArgs> for Figment { let mut remappings = Remappings::new_with_remappings(args.project_paths.get_remappings()); remappings .extend(figment.extract_inner::>("remappings").unwrap_or_default()); - figment.merge(("remappings", remappings.remappings)).merge(args) + figment.merge(("remappings", remappings.into_inner())).merge(args) } } diff --git a/cli/tests/it/config.rs b/cli/tests/it/config.rs index 9f08b565ec4c1..b1bdb03d9ab63 100644 --- a/cli/tests/it/config.rs +++ b/cli/tests/it/config.rs @@ -198,7 +198,8 @@ forgetest_init!( assert_eq!(config.remappings.len(), 3); pretty_eq!( format!("other-key/={}/", prj.root().join("lib/other").to_slash_lossy()), - Remapping::from(config.remappings[2].clone()).to_string() + // As CLI has the higher priority, it'll be found at the first slot. + Remapping::from(config.remappings[0].clone()).to_string() ); std::env::remove_var("DAPP_REMAPPINGS"); @@ -210,6 +211,62 @@ forgetest_init!( } ); +forgetest_init!( + #[serial_test::serial] + can_parse_remappings_correctly, + |prj: TestProject, mut cmd: TestCommand| { + cmd.set_current_dir(prj.root()); + let foundry_toml = prj.root().join(Config::FILE_NAME); + assert!(foundry_toml.exists()); + + let profile = Config::load_with_root(prj.root()); + // ensure that the auto-generated internal remapping for forge-std's ds-test exists + assert_eq!(profile.remappings.len(), 2); + pretty_eq!("ds-test/=lib/forge-std/lib/ds-test/src/", profile.remappings[0].to_string()); + + // ensure remappings contain test + pretty_eq!("ds-test/=lib/forge-std/lib/ds-test/src/", profile.remappings[0].to_string()); + // the loaded config has resolved, absolute paths + pretty_eq!( + "ds-test/=lib/forge-std/lib/ds-test/src/", + Remapping::from(profile.remappings[0].clone()).to_string() + ); + + cmd.arg("config"); + let expected = profile.to_string_pretty().unwrap(); + pretty_eq!(expected.trim().to_string(), cmd.stdout().trim().to_string()); + + let install = |cmd: &mut TestCommand, dep: &str| { + cmd.forge_fuse().args(["install", dep, "--no-commit"]); + cmd.assert_non_empty_stdout(); + }; + + install(&mut cmd, "transmissions11/solmate"); + let profile = Config::load_with_root(prj.root()); + // remappings work + let remappings_txt = prj.create_file( + "remappings.txt", + "solmate/=lib/solmate/src/\nsolmate-contracts/=lib/solmate/src/", + ); + let config = forge_utils::load_config_with_root(Some(prj.root().into())); + pretty_eq!( + format!("solmate/={}", prj.root().join("lib/solmate/src/").to_slash_lossy()), + Remapping::from(config.remappings[0].clone()).to_string() + ); + // As this is an user-generated remapping, it is not removed, even if it points to the same + // location. + pretty_eq!( + format!("solmate-contracts/={}", prj.root().join("lib/solmate/src/").to_slash_lossy()), + Remapping::from(config.remappings[1].clone()).to_string() + ); + pretty_err(&remappings_txt, fs::remove_file(&remappings_txt)); + + cmd.set_cmd(prj.forge_bin()).args(["config", "--basic"]); + let expected = profile.into_basic().to_string_pretty().unwrap(); + pretty_eq!(expected.trim().to_string(), cmd.stdout().trim().to_string()); + } +); + forgetest_init!( #[serial_test::serial] can_detect_config_vals, diff --git a/config/src/lib.rs b/config/src/lib.rs index bb1e1d5ab1611..350ec5a609aa7 100644 --- a/config/src/lib.rs +++ b/config/src/lib.rs @@ -574,9 +574,6 @@ impl Config { r.path.path = r.path.path.to_slash_lossy().into_owned().into(); }); } - // remove any potential duplicates - self.remappings.sort_unstable(); - self.remappings.dedup(); } /// Returns the directory in which dependencies should be installed diff --git a/config/src/providers/remappings.rs b/config/src/providers/remappings.rs index b2bf3d91efda1..f6c84eb1fce4e 100644 --- a/config/src/providers/remappings.rs +++ b/config/src/providers/remappings.rs @@ -6,7 +6,7 @@ use figment::{ }; use std::{ borrow::Cow, - collections::{btree_map::Entry, BTreeMap}, + collections::{btree_map::Entry, BTreeMap, HashSet}, fs, path::{Path, PathBuf}, }; @@ -16,7 +16,7 @@ use tracing::trace; #[derive(Debug, Clone, Default)] pub struct Remappings { /// Remappings. - pub remappings: Vec, + remappings: Vec, } impl Remappings { @@ -30,10 +30,23 @@ impl Remappings { Self { remappings } } + /// Consumes the wrapper and returns the inner remappings vector. + pub fn into_inner(self) -> Vec { + let mut tmp = HashSet::new(); + let remappings = + self.remappings.iter().filter(|r| tmp.insert(r.name.clone())).cloned().collect(); + remappings + } + /// Push an element ot the remappings vector, but only if it's not already present. pub fn push(&mut self, remapping: Remapping) { if !self.remappings.iter().any(|existing| { - existing.name.contains(&remapping.name) && existing.context == remapping.context + // What we're doing here is filtering for ambiguous paths. For example, if we have + // @prb/math/=node_modules/@prb/math/src/ as existing, and + // @prb/=node_modules/@prb/ as the one being checked, + // we want to keep the already existing one, which is the first one. This way we avoid + // having to deal with ambiguous paths which is unwanted when autodetecting remappings. + existing.name.starts_with(&remapping.name) && existing.context == remapping.context }) { self.remappings.push(remapping) } @@ -102,13 +115,15 @@ impl<'a> RemappingsProvider<'a> { } } - let mut new_remappings = Remappings::new(); + // Let's first just extend the remappings with the ones that were passed in, + // without any filtering. + let mut user_remappings = Vec::new(); - // check env var + // check env vars if let Some(env_remappings) = remappings_from_env_var("DAPP_REMAPPINGS") .or_else(|| remappings_from_env_var("FOUNDRY_REMAPPINGS")) { - new_remappings + user_remappings .extend(env_remappings.map_err::(|err| err.to_string().into())?); } @@ -118,11 +133,14 @@ impl<'a> RemappingsProvider<'a> { let content = fs::read_to_string(remappings_file).map_err(|err| err.to_string())?; let remappings_from_file: Result, _> = remappings_from_newline(&content).collect(); - new_remappings + user_remappings .extend(remappings_from_file.map_err::(|err| err.to_string().into())?); } - new_remappings.extend(remappings); + user_remappings.extend(remappings); + // Let's now use the wrapper to conditionally extend the remappings with the autodetected + // ones. We want to avoid duplicates, and the wrapper will handle this for us. + let mut all_remappings = Remappings::new_with_remappings(user_remappings); // scan all library dirs and autodetect remappings // todo: if a lib specifies contexts for remappings manually, we need to figure out how to @@ -151,7 +169,7 @@ impl<'a> RemappingsProvider<'a> { insert_closest(&mut lib_remappings, r.context, r.name, r.path.into()); } - new_remappings.extend( + all_remappings.extend( lib_remappings .into_iter() .flat_map(|(context, remappings)| { @@ -165,7 +183,7 @@ impl<'a> RemappingsProvider<'a> { ); } - Ok(new_remappings.remappings) + Ok(all_remappings.into_inner()) } /// Returns all remappings declared in foundry.toml files of libraries