From f77dc2451ef2c87003e7815d2666cff191f2093b Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Mon, 19 Jun 2023 12:03:46 -0700 Subject: [PATCH] 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 | 36 ++++++++-------- fish-rust/src/env/env_ffi.rs | 2 +- fish-rust/src/env/mod.rs | 2 +- fish-rust/src/ffi.rs | 36 +++++++--------- fish-rust/src/termsize.rs | 69 ++++++++++++++---------------- src/env.cpp | 4 +- src/env.h | 4 ++ src/parser.h | 3 -- 8 files changed, 73 insertions(+), 83 deletions(-) diff --git a/fish-rust/src/builtins/argparse.rs b/fish-rust/src/builtins/argparse.rs index ee6174e69..d18dac750 100644 --- a/fish-rust/src/builtins/argparse.rs +++ b/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; } diff --git a/fish-rust/src/env/env_ffi.rs b/fish-rust/src/env/env_ffi.rs index 6f6f2973e..9f68478cb 100644 --- a/fish-rust/src/env/env_ffi.rs +++ b/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 { diff --git a/fish-rust/src/env/mod.rs b/fish-rust/src/env/mod.rs index f901bad9e..09297271d 100644 --- a/fish-rust/src/env/mod.rs +++ b/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::*; diff --git a/fish-rust/src/ffi.rs b/fish-rust/src/ffi.rs index f5b089b48..55aa83a79 100644 --- a/fish-rust/src/ffi.rs +++ b/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 { - 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, U: AsRef>( - &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 { @@ -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. + let envref = self.get_impl_ffi(); + assert!(!envref.is_null()); + let env: &Box = 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 { diff --git a/fish-rust/src/termsize.rs b/fish-rust/src/termsize.rs index 164b6966c..8298bde0b 100644 --- a/fish-rust/src/termsize.rs +++ b/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 , 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, 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, 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 = 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> = 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(); diff --git a/src/env.cpp b/src/env.cpp index 0fe0b7428..da0c1cbb1 100644 --- a/src/env.cpp +++ b/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(&env_vars)); + auto termsize = + termsize_initialize_ffi(reinterpret_cast(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")) diff --git a/src/env.h b/src/env.h index 404e6f280..54f5c7c15 100644 --- a/src/env.h +++ b/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 *, or in Rust terms, a *const Box. + const void *get_impl_ffi() const { return &impl_; } + private: env_stack_t(rust::Box imp); diff --git a/src/parser.h b/src/parser.h index 87a5d80d8..695d70c78 100644 --- a/src/parser.h +++ b/src/parser.h @@ -393,9 +393,6 @@ class parser_t : public std::enable_shared_from_this { 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.