Expose Rust EnvStack from parser_t

Prior to this change, parser_t exposed an environment_t, and Rust had to go
through that. But because we have implemented Environment in Rust, it is
better to just expose the native Environment from parser_t. Make that
change and update call sites.

fish-rust/src/builtins/argparse.rs

@@ -6,8 +6,7 @@ use crate::builtins::shared::{
     BUILTIN_ERR_MAX_ARG_COUNT1, BUILTIN_ERR_MIN_ARG_COUNT1, BUILTIN_ERR_UNKNOWN, STATUS_CMD_ERROR,
     STATUS_CMD_OK, STATUS_INVALID_ARGS,
 };
-use crate::env::EnvMode;
-use crate::ffi::env_stack_t;
+use crate::env::{EnvMode, EnvStack};
 use crate::ffi::parser_t;
 use crate::ffi::Repin;
 use crate::wchar::{wstr, WString, L};
@@ -649,25 +648,25 @@ fn validate_arg<'opts>(
         return STATUS_CMD_OK;
     }
 
-    // TODO: in C++ this set directly in the vars (so does not emit events), reimplement that.
-    parser.get_var_stack().pin().push(true);
+    let vars = parser.get_vars();
+    vars.push(true /* new_scope */);
 
     let env_mode = EnvMode::LOCAL | EnvMode::EXPORT;
-    parser.set_var(L!("_argparse_cmd"), &[&opts_name], env_mode);
+    vars.set_one(L!("_argparse_cmd"), env_mode, opts_name.to_owned());
     let flag_name = WString::from(VAR_NAME_PREFIX) + "name";
     if is_long_flag {
-        parser.set_var(flag_name, &[&opt_spec.long_flag], env_mode);
+        vars.set_one(&flag_name, env_mode, opt_spec.long_flag.to_owned());
     } else {
-        parser.set_var(
-            flag_name,
-            &[&wstr::from_char_slice(&[opt_spec.short_flag])],
+        vars.set_one(
+            &flag_name,
             env_mode,
+            WString::from_chars(vec![opt_spec.short_flag]),
         );
     }
-    parser.set_var(
-        WString::from(VAR_NAME_PREFIX) + "value",
-        &[&woptarg],
+    vars.set_one(
+        &(WString::from(VAR_NAME_PREFIX) + "value"),
         env_mode,
+        woptarg.to_owned(),
     );
 
     let mut cmd_output = Vec::new();
@@ -678,7 +677,7 @@ fn validate_arg<'opts>(
         streams.err.append(output);
         streams.err.append1('\n');
     }
-    parser.get_var_stack().pin().pop();
+    vars.pop();
     return retval;
 }
 
@@ -922,7 +921,7 @@ fn check_min_max_args_constraints(
 }
 
 /// Put the result of parsing the supplied args into the caller environment as local vars.
-fn set_argparse_result_vars(vars: &mut env_stack_t, opts: &ArgParseCmdOpts) {
+fn set_argparse_result_vars(vars: &EnvStack, opts: &ArgParseCmdOpts) {
     for opt_spec in opts.options.values() {
         if opt_spec.num_seen == 0 {
             continue;
@@ -931,7 +930,7 @@ fn set_argparse_result_vars(vars: &mut env_stack_t, opts: &ArgParseCmdOpts) {
         if opt_spec.short_flag_valid {
             let mut var_name = WString::from(VAR_NAME_PREFIX);
             var_name.push(opt_spec.short_flag);
-            vars.set_var(&var_name, opt_spec.vals.as_slice(), EnvMode::LOCAL);
+            vars.set(&var_name, EnvMode::LOCAL, opt_spec.vals.clone());
         }
 
         if !opt_spec.long_flag.is_empty() {
@@ -942,11 +941,12 @@ fn set_argparse_result_vars(vars: &mut env_stack_t, opts: &ArgParseCmdOpts) {
                 .chars()
                 .map(|c| if fish_iswalnum(c) { c } else { '_' });
             let var_name_long: WString = VAR_NAME_PREFIX.chars().chain(long_flag).collect();
-            vars.set_var(var_name_long, opt_spec.vals.as_slice(), EnvMode::LOCAL);
+            vars.set(&var_name_long, EnvMode::LOCAL, opt_spec.vals.clone());
         }
     }
 
-    vars.set_var(L!("argv"), opts.args.as_slice(), EnvMode::LOCAL);
+    let args = opts.args.iter().map(|&s| s.to_owned()).collect();
+    vars.set(L!("argv"), EnvMode::LOCAL, args);
 }
 
 /// The argparse builtin. This is explicitly not compatible with the BSD or GNU version of this
@@ -1004,7 +1004,7 @@ pub fn argparse(
         return retval;
     }
 
-    set_argparse_result_vars(parser.get_var_stack(), &opts);
+    set_argparse_result_vars(&parser.get_vars(), &opts);
 
     return retval;
 }

fish-rust/src/env/env_ffi.rs

@@ -220,7 +220,7 @@ impl EnvDynFFI {
 }
 
 /// FFI wrapper around EnvStackRef.
-pub struct EnvStackRefFFI(EnvStackRef);
+pub struct EnvStackRefFFI(pub EnvStackRef);
 
 impl EnvStackRefFFI {
     fn getf(&self, name: &CxxWString, mode: u16) -> *mut EnvVar {

fish-rust/src/env/mod.rs

@@ -4,7 +4,7 @@ mod environment_impl;
 pub mod var;
 
 use crate::common::ToCString;
-pub use env_ffi::EnvStackSetResult;
+pub use env_ffi::{EnvStackRefFFI, EnvStackSetResult};
 pub use environment::*;
 use std::sync::atomic::{AtomicBool, AtomicUsize};
 pub use var::*;

fish-rust/src/ffi.rs

@@ -4,7 +4,7 @@ use crate::wchar_ffi::WCharToFFI;
 use ::std::pin::Pin;
 #[rustfmt::skip]
 use ::std::slice;
-use crate::env::EnvMode;
+use crate::env::{EnvMode, EnvStackRef, EnvStackRefFFI};
 pub use crate::wait_handle::{
     WaitHandleRef, WaitHandleRefFFI, WaitHandleStore, WaitHandleStoreFFI,
 };
@@ -193,26 +193,8 @@ impl parser_t {
         unsafe { job.as_ref() }
     }
 
-    /// Helper to get a variable as a string, using the default flags.
-    pub fn var_as_string(&mut self, name: &wstr) -> Option<WString> {
-        self.pin().vars().unpin().get_as_string(name)
-    }
-
-    pub fn get_var_stack(&mut self) -> &mut env_stack_t {
-        self.pin().vars().unpin()
-    }
-
-    pub fn get_var_stack_env(&mut self) -> &environment_t {
-        self.vars_env_ffi()
-    }
-
-    pub fn set_var<T: AsRef<wstr>, U: AsRef<wstr>>(
-        &mut self,
-        name: T,
-        value: &[U],
-        flags: EnvMode,
-    ) -> libc::c_int {
-        self.get_var_stack().set_var(name, value, flags)
+    pub fn get_vars(&mut self) -> EnvStackRef {
+        self.pin().vars().from_ffi()
     }
 
     pub fn get_func_name(&mut self, level: i32) -> Option<WString> {
@@ -225,6 +207,18 @@ impl parser_t {
 
 unsafe impl Send for env_universal_t {}
 
+impl env_stack_t {
+    /// Access the underlying Rust environment stack.
+    #[allow(clippy::borrowed_box)]
+    pub fn from_ffi(&self) -> EnvStackRef {
+        // Safety: get_impl_ffi returns a pointer to a Box<EnvStackRefFFI>.
+        let envref = self.get_impl_ffi();
+        assert!(!envref.is_null());
+        let env: &Box<EnvStackRefFFI> = unsafe { &*(envref.cast()) };
+        env.0.clone()
+    }
+}
+
 impl environment_t {
     /// Helper to get a variable as a string, using the default flags.
     pub fn get_as_string(&self, name: &wstr) -> Option<WString> {

fish-rust/src/termsize.rs

@@ -1,7 +1,7 @@
 // Support for exposing the terminal size.
 use crate::common::assert_sync;
-use crate::env::{EnvMode, Environment};
-use crate::ffi::{environment_t, parser_t, Repin};
+use crate::env::{EnvMode, EnvStackRefFFI, EnvVar, Environment};
+use crate::ffi::{parser_t, Repin};
 use crate::flog::FLOG;
 use crate::wchar::{WString, L};
 use crate::wchar_ext::ToWString;
@@ -29,7 +29,6 @@ mod termsize_ffi {
         pub fn termsize_last() -> Termsize;
         pub fn termsize_initialize_ffi(vars: *const u8) -> Termsize;
         pub fn termsize_invalidate_tty();
-        pub fn handle_columns_lines_var_change_ffi(vars: *const u8);
         pub fn termsize_update_ffi(parser: *mut u8) -> Termsize;
         pub fn termsize_handle_winch();
     }
@@ -41,10 +40,10 @@ static TTY_TERMSIZE_GEN_COUNT: AtomicU32 = AtomicU32::new(0);
 
 /// Convert an environment variable to an int, or return a default value.
 /// The int must be >0 and <USHRT_MAX (from struct winsize).
-fn var_to_int_or(var: Option<WString>, default: isize) -> isize {
-    if var.is_some() && !var.as_ref().unwrap().is_empty() {
-        #[allow(clippy::unnecessary_unwrap)]
-        if let Ok(proposed) = fish_wcstoi(&var.unwrap()) {
+fn var_to_int_or(var: Option<EnvVar>, default: isize) -> isize {
+    let val: WString = var.map(|v| v.as_string()).unwrap_or_default();
+    if !val.is_empty() {
+        if let Ok(proposed) = fish_wcstoi(&val) {
             if proposed > 0 && proposed <= u16::MAX as i32 {
                 return proposed as isize;
             }
@@ -161,10 +160,10 @@ impl TermsizeContainer {
     /// Initialize our termsize, using the given environment stack.
     /// This will prefer to use COLUMNS and LINES, but will fall back to the tty size reader.
     /// This does not change any variables in the environment.
-    pub fn initialize(&self, vars: &environment_t) -> Termsize {
+    pub fn initialize(&self, vars: &dyn Environment) -> Termsize {
         let new_termsize = Termsize {
-            width: var_to_int_or(vars.get_as_string_flags(L!("COLUMNS"), EnvMode::GLOBAL), -1),
-            height: var_to_int_or(vars.get_as_string_flags(L!("LINES"), EnvMode::GLOBAL), -1),
+            width: var_to_int_or(vars.getf(L!("COLUMNS"), EnvMode::GLOBAL), -1),
+            height: var_to_int_or(vars.getf(L!("LINES"), EnvMode::GLOBAL), -1),
         };
 
         let mut data = self.data.lock().unwrap();
@@ -253,7 +252,7 @@ impl TermsizeContainer {
     }
 
     /// Note that COLUMNS and/or LINES global variables changed.
-    fn handle_columns_lines_var_change_ffi(&self, vars: &environment_t) {
+    fn handle_columns_lines_var_change_ffi(&self, vars: &dyn Environment) {
         // Do nothing if we are the ones setting it.
         if self.setting_env_vars.load(Ordering::Relaxed) {
             return;
@@ -261,11 +260,11 @@ impl TermsizeContainer {
         // Construct a new termsize from COLUMNS and LINES, then set it in our data.
         let new_termsize = Termsize {
             width: var_to_int_or(
-                vars.get_as_string_flags(L!("COLUMNS"), EnvMode::GLOBAL),
+                vars.getf(L!("COLUMNS"), EnvMode::GLOBAL),
                 Termsize::DEFAULT_WIDTH,
             ),
             height: var_to_int_or(
-                vars.get_as_string_flags(L!("LINES"), EnvMode::GLOBAL),
+                vars.getf(L!("LINES"), EnvMode::GLOBAL),
                 Termsize::DEFAULT_HEIGHT,
             ),
         };
@@ -311,18 +310,13 @@ pub fn handle_columns_lines_var_change(vars: &dyn Environment) {
     SHARED_CONTAINER.handle_columns_lines_var_change(vars);
 }
 
-fn handle_columns_lines_var_change_ffi(vars_ptr: *const u8) {
-    assert!(!vars_ptr.is_null());
-    let vars: &environment_t = unsafe { &*(vars_ptr.cast()) };
-    SHARED_CONTAINER.handle_columns_lines_var_change_ffi(vars);
-}
-
 /// Called to initialize the termsize.
-/// The pointer is to an environment_t, but has the wrong type to satisfy cxx.
+/// The pointer is to a Box<EnvStackRefFFI>, but has the wrong type to satisfy cxx.
+#[allow(clippy::borrowed_box)]
 pub fn termsize_initialize_ffi(vars_ptr: *const u8) -> Termsize {
     assert!(!vars_ptr.is_null());
-    let vars: &environment_t = unsafe { &*(vars_ptr as *const environment_t) };
-    SHARED_CONTAINER.initialize(vars)
+    let vars: &Box<EnvStackRefFFI> = unsafe { &*(vars_ptr.cast()) };
+    SHARED_CONTAINER.initialize(&*vars.0)
 }
 
 /// Called to update termsize.
@@ -345,6 +339,7 @@ use crate::ffi_tests::add_test;
 add_test!("test_termsize", || {
     let env_global = EnvMode::GLOBAL;
     let parser: &mut parser_t = unsafe { &mut *parser_t::principal_parser_ffi() };
+    let vars = parser.get_vars();
 
     // Use a static variable so we can pretend we're the kernel exposing a terminal size.
     static STUBBY_TERMSIZE: Mutex<Option<Termsize>> = Mutex::new(None);
@@ -374,33 +369,33 @@ add_test!("test_termsize", || {
     // Ok now we tell it to update.
     ts.updating(parser);
     assert_eq!(ts.last(), Termsize::new(42, 84));
-    assert_eq!(parser.var_as_string(L!("COLUMNS")).unwrap(), "42");
-    assert_eq!(parser.var_as_string(L!("LINES")).unwrap(), "84");
+    assert_eq!(vars.get(L!("COLUMNS")).unwrap().as_string(), "42");
+    assert_eq!(vars.get(L!("LINES")).unwrap().as_string(), "84");
 
     // Wow someone set COLUMNS and LINES to a weird value.
     // Now the tty's termsize doesn't matter.
-    parser.set_var(L!("COLUMNS"), &[L!("75")], env_global);
-    parser.set_var(L!("LINES"), &[L!("150")], env_global);
-    ts.handle_columns_lines_var_change_ffi(parser.get_var_stack_env());
+    vars.set_one(L!("COLUMNS"), env_global, L!("75").to_owned());
+    vars.set_one(L!("LINES"), env_global, L!("150").to_owned());
+    ts.handle_columns_lines_var_change(&*parser.get_vars());
     assert_eq!(ts.last(), Termsize::new(75, 150));
-    assert_eq!(parser.var_as_string(L!("COLUMNS")).unwrap(), "75");
-    assert_eq!(parser.var_as_string(L!("LINES")).unwrap(), "150");
+    assert_eq!(vars.get(L!("COLUMNS")).unwrap().as_string(), "75");
+    assert_eq!(vars.get(L!("LINES")).unwrap().as_string(), "150");
 
-    parser.set_var(L!("COLUMNS"), &[L!("33")], env_global);
-    ts.handle_columns_lines_var_change_ffi(parser.get_var_stack_env());
+    vars.set_one(L!("COLUMNS"), env_global, L!("33").to_owned());
+    ts.handle_columns_lines_var_change(&*parser.get_vars());
     assert_eq!(ts.last(), Termsize::new(33, 150));
 
     // Oh it got SIGWINCH, now the tty matters again.
     TermsizeContainer::handle_winch();
     assert_eq!(ts.last(), Termsize::new(33, 150));
     assert_eq!(ts.updating(parser), stubby_termsize().unwrap());
-    assert_eq!(parser.var_as_string(L!("COLUMNS")).unwrap(), "42");
-    assert_eq!(parser.var_as_string(L!("LINES")).unwrap(), "84");
+    assert_eq!(vars.get(L!("COLUMNS")).unwrap().as_string(), "42");
+    assert_eq!(vars.get(L!("LINES")).unwrap().as_string(), "84");
 
     // Test initialize().
-    parser.set_var(L!("COLUMNS"), &[L!("83")], env_global);
-    parser.set_var(L!("LINES"), &[L!("38")], env_global);
-    ts.initialize(parser.get_var_stack_env());
+    vars.set_one(L!("COLUMNS"), env_global, L!("83").to_owned());
+    vars.set_one(L!("LINES"), env_global, L!("38").to_owned());
+    ts.initialize(&*vars);
     assert_eq!(ts.last(), Termsize::new(83, 38));
 
     // initialize() even beats the tty reader until a sigwinch.
@@ -409,7 +404,7 @@ add_test!("test_termsize", || {
         setting_env_vars: AtomicBool::new(false),
         tty_size_reader: stubby_termsize,
     };
-    ts.initialize(parser.get_var_stack_env());
+    ts.initialize(&*parser.get_vars());
     ts2.updating(parser);
     assert_eq!(ts.last(), Termsize::new(83, 38));
     TermsizeContainer::handle_winch();

src/env.cpp

@@ -353,8 +353,8 @@ void env_init(const struct config_paths_t *paths, bool do_uvars, bool default_pa
     }
 
     // Initialize termsize variables.
-    environment_t &env_vars = vars;
-    auto termsize = termsize_initialize_ffi(reinterpret_cast<const unsigned char *>(&env_vars));
+    auto termsize =
+        termsize_initialize_ffi(reinterpret_cast<const unsigned char *>(vars.get_impl_ffi()));
     if (!vars.get_unless_empty(L"COLUMNS"))
         vars.set_one(L"COLUMNS", ENV_GLOBAL, to_string(termsize.width));
     if (!vars.get_unless_empty(L"LINES"))

src/env.h

@@ -301,6 +301,10 @@ class env_stack_t final : public environment_t {
     // Do not push or pop from this.
     static env_stack_t &globals();
 
+    /// Access the underlying Rust implementation.
+    /// This returns a const rust::Box<EnvStackRef> *, or in Rust terms, a *const Box<EnvStackRef>.
+    const void *get_impl_ffi() const { return &impl_; }
+
    private:
     env_stack_t(rust::Box<EnvStackRef> imp);
 

src/parser.h

@@ -393,9 +393,6 @@ class parser_t : public std::enable_shared_from_this<parser_t> {
     env_stack_t &vars() { return *variables; }
     const env_stack_t &vars() const { return *variables; }
 
-    /// Rust helper - variables as an environment_t.
-    const environment_t &vars_env_ffi() const { return *variables; }
-
     int remove_var_ffi(const wcstring &key, int mode) { return vars().remove(key, mode); }
 
     /// Get the library data.