Snapshot env when adding to history

[ja: this seems a little bit safer?]

fish-rust/src/env/environment.rs

@@ -116,16 +116,6 @@ pub struct EnvDyn {
     inner: Box<dyn Environment + Send + Sync>,
 }
 
-pub trait AsEnvironment {
-    fn as_environment(&self) -> &(dyn Environment + Send + Sync);
-}
-
-impl AsEnvironment for EnvDyn {
-    fn as_environment(&self) -> &(dyn Environment + Send + Sync) {
-        &*self.inner
-    }
-}
-
 impl EnvDyn {
     // Exposed for testing.
     pub fn new(inner: Box<dyn Environment + Send + Sync>) -> Self {
@@ -402,12 +392,6 @@ fn get_pwd_slash(&self) -> WString {
 // TODO Remove Pin?
 pub type EnvStackRef = Pin<Arc<EnvStack>>;
 
-impl AsEnvironment for EnvStackRef {
-    fn as_environment(&self) -> &(dyn Environment + Send + Sync) {
-        Pin::get_ref(Pin::as_ref(self))
-    }
-}
-
 // A variable stack that only represents globals.
 // Do not push or pop from this.
 lazy_static! {

fish-rust/src/history.rs

@@ -46,7 +46,7 @@
         str2wcstring, unescape_string, valid_var_name, wcs2zstring, write_loop, CancelChecker,
         UnescapeStringStyle,
     },
-    env::{AsEnvironment, EnvMode, EnvStack, EnvStackRefFFI, Environment},
+    env::{EnvMode, EnvStack, EnvStackRefFFI, Environment},
     expand::{expand_one, ExpandFlags},
     fallback::fish_mkstemp_cloexec,
     fds::{wopen_cloexec, AutoCloseFd},
@@ -1688,7 +1688,7 @@ pub fn remove_ephemeral_items(&self) {
     pub fn add_pending_with_file_detection(
         self: Arc<Self>,
         s: &wstr,
-        vars: impl AsEnvironment + Send + Sync + 'static,
+        vars: &EnvStack,
         persist_mode: PersistenceMode, /*=disk*/
     ) {
         // We use empty items as sentinels to indicate the end of history.
@@ -1745,10 +1745,10 @@ pub fn add_pending_with_file_detection(
             // Don't hold the lock while we perform this file detection.
             imp.add(item, /*pending=*/ true, /*do_save=*/ true);
             drop(imp);
+            let vars_snapshot = vars.snapshot();
             iothread_perform(move || {
                 // Don't hold the lock while we perform this file detection.
-                let validated_paths =
-                    expand_and_detect_paths(potential_paths, vars.as_environment());
+                let validated_paths = expand_and_detect_paths(potential_paths, &vars_snapshot);
                 let mut imp = self.imp();
                 imp.set_valid_file_paths(validated_paths, identifier);
                 imp.enable_automatic_saving();

fish-rust/src/reader.rs

@@ -4714,7 +4714,7 @@ fn add_to_history(&mut self) {
             };
             self.history.clone().add_pending_with_file_detection(
                 &text,
-                self.parser().variables.clone(),
+                &self.parser().variables,
                 mode,
             );
         }

fish-rust/src/tests/history.rs

@@ -1,7 +1,7 @@
 use crate::common::{
     cstr2wcstring, is_windows_subsystem_for_linux, str2wcstring, wcs2osstring, wcs2string,
 };
-use crate::env::{EnvDyn, Environment};
+use crate::env::{EnvDyn, EnvMode, EnvStack, Environment};
 use crate::fds::{wopen_cloexec, AutoCloseFd};
 use crate::ffi_tests::add_test;
 use crate::history::{self, History, HistoryItem, HistorySearch, PathList, SearchDirection};
@@ -453,57 +453,56 @@ fn test_history_races_pound_on_history(item_count: usize, idx: usize) {
     let filename = L!("testfile");
     std::fs::write(wcs2osstring(&(tmpdir.clone() + &filename[..])), []).unwrap();
 
-    let mut test_vars = TestEnvironment::default();
-    test_vars.vars.insert(L!("PWD").to_owned(), tmpdir.clone());
-    test_vars.vars.insert(L!("HOME").to_owned(), tmpdir.clone());
-    let vars = || EnvDyn::new(Box::new(test_vars.clone()) as Box<dyn Environment + Send + Sync>);
+    let test_vars = EnvStack::new();
+    test_vars.set_one(L!("PWD"), EnvMode::GLOBAL, tmpdir.clone());
+    test_vars.set_one(L!("HOME"), EnvMode::GLOBAL, tmpdir.clone());
 
     let history = History::with_name(L!("path_detection"));
     history.clear();
     assert_eq!(history.size(), 0);
     history.clone().add_pending_with_file_detection(
         L!("cmd0 not/a/valid/path"),
-        vars(),
+        &test_vars,
         history::PersistenceMode::Disk,
     );
     history.clone().add_pending_with_file_detection(
         &(L!("cmd1 ").to_owned() + filename),
-        vars(),
+        &test_vars,
         history::PersistenceMode::Disk,
     );
     history.clone().add_pending_with_file_detection(
         &(L!("cmd2 ").to_owned() + &tmpdir[..] + L!("/") + filename),
-        vars(),
+        &test_vars,
         history::PersistenceMode::Disk,
     );
     history.clone().add_pending_with_file_detection(
         &(L!("cmd3  $HOME/").to_owned() + filename),
-        vars(),
+        &test_vars,
         history::PersistenceMode::Disk,
     );
     history.clone().add_pending_with_file_detection(
         L!("cmd4  $HOME/notafile"),
-        vars(),
+        &test_vars,
         history::PersistenceMode::Disk,
     );
     history.clone().add_pending_with_file_detection(
         &(L!("cmd5  ~/").to_owned() + filename),
-        vars(),
+        &test_vars,
         history::PersistenceMode::Disk,
     );
     history.clone().add_pending_with_file_detection(
         L!("cmd6  ~/notafile"),
-        vars(),
+        &test_vars,
         history::PersistenceMode::Disk,
     );
     history.clone().add_pending_with_file_detection(
         L!("cmd7  ~/*f*"),
-        vars(),
+        &test_vars,
         history::PersistenceMode::Disk,
     );
     history.clone().add_pending_with_file_detection(
         L!("cmd8  ~/*zzz*"),
-        vars(),
+        &test_vars,
         history::PersistenceMode::Disk,
     );
     history.resolve_pending();