fix(remappings): properly filter only autodetected remappings, do not sort unnecesarily

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::<Vec<Remapping>>("remappings").unwrap_or_default());
-        figment.merge(("remappings", remappings.remappings)).merge(args)
+        figment.merge(("remappings", remappings.into_inner())).merge(args)
     }
 }
 

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,

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

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<Remapping>,
+    remappings: Vec<Remapping>,
 }
 
 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<Remapping> {
+        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::<Error, _>(|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<Vec<_>, _> =
                 remappings_from_newline(&content).collect();
-            new_remappings
+            user_remappings
                 .extend(remappings_from_file.map_err::<Error, _>(|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