Replace status_cmd with an option

- Using an option makes it much clearer that the check for empty args is
  redundant.
- Also prefer implementing TryFrom only for &str, to not hide the string
  conversion and allocation happening.

fish-rust/src/builtins/status.rs

@@ -1,20 +1,18 @@
 use std::os::unix::prelude::OsStrExt;
 
-use crate::builtins::shared::BUILTIN_ERR_NOT_NUMBER;
 use crate::builtins::shared::{
     builtin_missing_argument, builtin_print_help, builtin_unknown_option, io_streams_t,
     BUILTIN_ERR_ARG_COUNT2, BUILTIN_ERR_COMBO2_EXCLUSIVE, BUILTIN_ERR_INVALID_SUBCMD,
-    STATUS_CMD_ERROR, STATUS_CMD_OK, STATUS_INVALID_ARGS,
+    BUILTIN_ERR_NOT_NUMBER, STATUS_CMD_ERROR, STATUS_CMD_OK, STATUS_INVALID_ARGS,
 };
 use crate::common::{get_executable_path, str2wcstring};
 
-use crate::ffi::get_job_control_mode;
+use crate::ffi::{
-use crate::ffi::get_login;
+    get_job_control_mode, get_login, is_interactive_session, job_control_t, parser_t,
-use crate::ffi::set_job_control_mode;
+    set_job_control_mode, Repin,
-use crate::ffi::{is_interactive_session, Repin};
+};
-use crate::ffi::{job_control_t, parser_t};
 use crate::future_feature_flags::{feature_metadata, feature_test};
-use crate::wchar::{wstr, WString, L};
+use crate::wchar::{wstr, L};
 
 use crate::wchar_ffi::WCharFromFFI;
 
@@ -27,16 +25,17 @@ use nix::errno::Errno;
 use nix::NixPath;
 use num_derive::FromPrimitive;
 use num_traits::FromPrimitive;
+use once_cell::sync::Lazy;
 
 macro_rules! str_enum {
     ($name:ident, $(($val:ident, $str:expr)),* $(,)?) => {
-        impl TryFrom<&wstr> for $name {
+        impl TryFrom<&str> for $name {
             type Error = ();
 
-            fn try_from(s: &wstr) -> Result<Self, Self::Error> {
+            fn try_from(s: &str) -> Result<Self, Self::Error> {
                 // matching on str's let's us avoid having to do binary search and friends outselves,
                 // this is ascii only anyways
-                match s.to_string().as_str() {
+                match s {
                     $($str => Ok(Self::$val)),*,
                     _ => Err(()),
                 }
@@ -44,21 +43,19 @@ macro_rules! str_enum {
         }
 
         impl $name {
-            fn to_wstr(&self) -> WString {
+            fn to_wstr(self) -> &'static wstr {
                 // There can be multiple vals => str mappings, and that's okay
                 #[allow(unreachable_patterns)]
                 match self {
-                    $(Self::$val => WString::from($str)),*,
+                    $(Self::$val => L!($str)),*,
                 }
             }
         }
     }
 }
 
-use once_cell::sync::Lazy;
 use StatusCmd::*;
-#[repr(u32)]
+#[derive(PartialEq, FromPrimitive, Clone, Copy)]
-#[derive(Default, PartialEq, FromPrimitive, Clone)]
 enum StatusCmd {
     STATUS_CURRENT_CMD = 1,
     STATUS_BASENAME,
@@ -80,8 +77,6 @@ enum StatusCmd {
     STATUS_STACK_TRACE,
     STATUS_TEST_FEATURE,
     STATUS_CURRENT_COMMANDLINE,
-    #[default]
-    STATUS_UNDEF,
 }
 
 str_enum!(
@@ -112,15 +107,12 @@ str_enum!(
     (STATUS_STACK_TRACE, "print-stack-trace"),
     (STATUS_STACK_TRACE, "stack-trace"),
     (STATUS_TEST_FEATURE, "test-feature"),
-    // this was a nullptr in C++
-    (STATUS_UNDEF, "undef"),
 );
 
 impl StatusCmd {
-    fn as_char(&self) -> char {
+    fn as_char(self) -> char {
         // TODO: once unwrap is const, make LONG_OPTIONS const
-        let ch: StatusCmd = self.clone();
+        char::from_u32(self as u32).unwrap()
-        char::from_u32(ch as u32).unwrap()
     }
 }
 
@@ -135,7 +127,7 @@ enum TestFeatureRetVal {
 struct StatusCmdOpts {
     level: i32,
     new_job_control_mode: Option<job_control_t>,
-    status_cmd: StatusCmd,
+    status_cmd: Option<StatusCmd>,
     print_help: bool,
 }
 
@@ -144,27 +136,12 @@ impl Default for StatusCmdOpts {
         Self {
             level: 1,
             new_job_control_mode: None,
-            status_cmd: StatusCmd::STATUS_UNDEF,
+            status_cmd: None,
             print_help: false,
         }
     }
 }
 
-impl StatusCmdOpts {
-    fn set_status_cmd(&mut self, cmd: &wstr, sub_cmd: StatusCmd) -> Result<(), WString> {
-        if self.status_cmd != StatusCmd::STATUS_UNDEF {
-            return Err(wgettext_fmt!(
-                BUILTIN_ERR_COMBO2_EXCLUSIVE,
-                cmd,
-                self.status_cmd.to_wstr(),
-                sub_cmd.to_wstr(),
-            ));
-        }
-        self.status_cmd = sub_cmd;
-        Ok(())
-    }
-}
-
 const SHORT_OPTIONS: &wstr = L!(":L:cbilfnhj:t");
 static LONG_OPTIONS: Lazy<[woption; 17]> = Lazy::new(|| {
     use woption_argument_t::*;
@@ -262,59 +239,44 @@ fn parse_cmd_opts(
                     }
                 };
             }
-            'c' => {
+            flag @ ('c' | 'b' | 'i' | 'l' | 'f' | 'n' | 't') => {
-                if let Err(e) = opts.set_status_cmd(cmd, STATUS_IS_COMMAND_SUB) {
+                let subcmd = match flag {
-                    streams.err.append(e);
+                    'c' => STATUS_IS_COMMAND_SUB,
-                    return STATUS_CMD_ERROR;
+                    'b' => STATUS_IS_BLOCK,
-                }
+                    'i' => STATUS_IS_INTERACTIVE,
-            }
+                    'l' => STATUS_IS_LOGIN,
-            'b' => {
+                    'f' => STATUS_FILENAME,
-                if let Err(e) = opts.set_status_cmd(cmd, STATUS_IS_BLOCK) {
+                    'n' => STATUS_LINE_NUMBER,
-                    streams.err.append(e);
+                    't' => STATUS_STACK_TRACE,
-                    return STATUS_CMD_ERROR;
+                    _ => unreachable!(),
-                }
+                };
-            }
+                if let Some(existing) = opts.status_cmd.replace(subcmd) {
-            'i' => {
+                    streams.err.append(wgettext_fmt!(
-                if let Err(e) = opts.set_status_cmd(cmd, STATUS_IS_INTERACTIVE) {
+                        BUILTIN_ERR_COMBO2_EXCLUSIVE,
-                    streams.err.append(e);
+                        cmd,
-                    return STATUS_CMD_ERROR;
+                        existing.to_wstr(),
-                }
+                        subcmd.to_wstr(),
-            }
+                    ));
-            'l' => {
-                if let Err(e) = opts.set_status_cmd(cmd, STATUS_IS_LOGIN) {
-                    streams.err.append(e);
-                    return STATUS_CMD_ERROR;
-                }
-            }
-            'f' => {
-                if let Err(e) = opts.set_status_cmd(cmd, STATUS_FILENAME) {
-                    streams.err.append(e);
-                    return STATUS_CMD_ERROR;
-                }
-            }
-            'n' => {
-                if let Err(e) = opts.set_status_cmd(cmd, STATUS_LINE_NUMBER) {
-                    streams.err.append(e);
                     return STATUS_CMD_ERROR;
                 }
             }
             'j' => {
-                if let Err(e) = opts.set_status_cmd(cmd, STATUS_SET_JOB_CONTROL) {
+                let subcmd = STATUS_SET_JOB_CONTROL;
-                    streams.err.append(e);
+                if let Some(existing) = opts.status_cmd.replace(subcmd) {
+                    streams.err.append(wgettext_fmt!(
+                        BUILTIN_ERR_COMBO2_EXCLUSIVE,
+                        cmd,
+                        existing.to_wstr(),
+                        subcmd.to_wstr(),
+                    ));
                     return STATUS_CMD_ERROR;
                 }
-                let Ok(job_mode) = w.woptarg.unwrap().try_into() else {
+                let Ok(job_mode) = w.woptarg.unwrap().to_string().as_str().try_into() else {
                     streams.err.append(wgettext_fmt!("%ls: Invalid job control mode '%ls'\n", cmd, w.woptarg.unwrap()));
                     return STATUS_CMD_ERROR;
                 };
                 opts.new_job_control_mode = Some(job_mode);
             }
-            't' => {
-                if let Err(e) = opts.set_status_cmd(cmd, STATUS_STACK_TRACE) {
-                    streams.err.append(e);
-                    return STATUS_CMD_ERROR;
-                }
-            }
             'h' => opts.print_help = true,
             ':' => {
                 builtin_missing_argument(parser, streams, cmd, args[w.woptind - 1], false);
@@ -333,8 +295,13 @@ fn parse_cmd_opts(
                     | STATUS_IS_INTERACTIVE_JOB_CTRL
                     | STATUS_IS_NO_JOB_CTRL
                     | STATUS_FISH_PATH => {
-                        if let Err(e) = opts.set_status_cmd(cmd, opt_cmd) {
+                        if let Some(existing) = opts.status_cmd.replace(opt_cmd) {
-                            streams.err.append(e);
+                            streams.err.append(wgettext_fmt!(
+                                BUILTIN_ERR_COMBO2_EXCLUSIVE,
+                                cmd,
+                                existing.to_wstr(),
+                                opt_cmd.to_wstr(),
+                            ));
                             return STATUS_CMD_ERROR;
                         }
                     }
@@ -372,54 +339,53 @@ pub fn status(
     // If a status command hasn't already been specified via a flag check the first word.
     // Note that this can be simplified after we eliminate allowing subcommands as flags.
     if optind < argc {
-        match StatusCmd::try_from(args[optind]) {
+        match StatusCmd::try_from(args[optind].to_string().as_str()) {
-            // TODO: can we replace UNDEF with wrapping in option?
+            Ok(s) => {
-            Ok(STATUS_UNDEF) | Err(_) => {
+                if let Some(existing) = opts.status_cmd.replace(s) {
+                    streams.err.append(wgettext_fmt!(
+                        BUILTIN_ERR_COMBO2_EXCLUSIVE,
+                        cmd,
+                        existing.to_wstr(),
+                        s.to_wstr(),
+                    ));
+                    return STATUS_CMD_ERROR;
+                }
+                optind += 1;
+            }
+            Err(_) => {
                 streams
                     .err
                     .append(wgettext_fmt!(BUILTIN_ERR_INVALID_SUBCMD, cmd, args[1]));
                 return STATUS_INVALID_ARGS;
             }
-            Ok(s) => {
-                if let Err(e) = opts.set_status_cmd(cmd, s) {
-                    streams.err.append(e);
-                    return STATUS_CMD_ERROR;
-                }
-                optind += 1;
-            }
         }
     }
     // Every argument that we haven't consumed already is an argument for a subcommand.
     let args = &args[optind..];
 
-    match opts.status_cmd {
+    let Some(subcmd) = opts.status_cmd else {
-        STATUS_UNDEF => {
+        debug_assert!(args.is_empty(), "passed arguments to nothing");
-            if !args.is_empty() {
+
-                streams.err.append(wgettext_fmt!(
+        if get_login() {
-                    BUILTIN_ERR_ARG_COUNT2,
+            streams.out.append(wgettext!("This is a login shell\n"));
-                    cmd,
+        } else {
-                    opts.status_cmd.to_wstr(),
+            streams.out.append(wgettext!("This is not a login shell\n"));
-                    0,
-                    args.len()
-                ));
-                return STATUS_INVALID_ARGS;
-            }
-            if get_login() {
-                streams.out.append(wgettext!("This is a login shell\n"));
-            } else {
-                streams.out.append(wgettext!("This is not a login shell\n"));
-            }
-            let job_control_mode = match get_job_control_mode() {
-                job_control_t::interactive => wgettext!("Only on interactive jobs"),
-                job_control_t::none => wgettext!("Never"),
-                job_control_t::all => wgettext!("Always"),
-            };
-            streams
-                .out
-                .append(wgettext_fmt!("Job control: %ls\n", job_control_mode));
-            streams.out.append(parser.stack_trace().from_ffi());
         }
-        STATUS_SET_JOB_CONTROL => {
+        let job_control_mode = match get_job_control_mode() {
+            job_control_t::interactive => wgettext!("Only on interactive jobs"),
+            job_control_t::none => wgettext!("Never"),
+            job_control_t::all => wgettext!("Always"),
+        };
+        streams
+            .out
+            .append(wgettext_fmt!("Job control: %ls\n", job_control_mode));
+        streams.out.append(parser.stack_trace().from_ffi());
+
+        return STATUS_CMD_OK;
+    };
+
+    match subcmd {
+        c @ STATUS_SET_JOB_CONTROL => {
             let job_control_mode = match opts.new_job_control_mode {
                 Some(j) => {
                     // Flag form used
@@ -427,7 +393,7 @@ pub fn status(
                         streams.err.append(wgettext_fmt!(
                             BUILTIN_ERR_ARG_COUNT2,
                             cmd,
-                            opts.status_cmd.to_wstr(),
+                            c.to_wstr(),
                             0,
                             args.len()
                         ));
@@ -440,13 +406,13 @@ pub fn status(
                         streams.err.append(wgettext_fmt!(
                             BUILTIN_ERR_ARG_COUNT2,
                             cmd,
-                            opts.status_cmd.to_wstr(),
+                            c.to_wstr(),
                             1,
                             args.len()
                         ));
                         return STATUS_INVALID_ARGS;
                     }
-                    let Ok(new_mode)= args[0].try_into() else {
+                    let Ok(new_mode)= args[0].to_string().as_str().try_into() else {
                         streams.err.append(wgettext_fmt!("%ls: Invalid job control mode '%ls'\n", cmd, args[0]));
                         return STATUS_CMD_ERROR;
                     };
@@ -456,12 +422,12 @@ pub fn status(
             set_job_control_mode(job_control_mode);
         }
         STATUS_FEATURES => print_features(streams),
-        STATUS_TEST_FEATURE => {
+        c @ STATUS_TEST_FEATURE => {
             if args.len() != 1 {
                 streams.err.append(wgettext_fmt!(
                     BUILTIN_ERR_ARG_COUNT2,
                     cmd,
-                    opts.status_cmd.to_wstr(),
+                    c.to_wstr(),
                     1,
                     args.len()
                 ));
@@ -484,7 +450,7 @@ pub fn status(
                 streams.err.append(wgettext_fmt!(
                     BUILTIN_ERR_ARG_COUNT2,
                     cmd,
-                    opts.status_cmd.to_wstr(),
+                    s.to_wstr(),
                     0,
                     args.len()
                 ));
@@ -493,7 +459,7 @@ pub fn status(
             match s {
                 STATUS_BASENAME | STATUS_DIRNAME | STATUS_FILENAME => {
                     let res = parser.current_filename_ffi().from_ffi();
-                    let f = match (res.is_empty(), opts.status_cmd) {
+                    let f = match (res.is_empty(), s) {
                         (false, STATUS_DIRNAME) => wdirname(res),
                         (false, STATUS_BASENAME) => wbasename(res),
                         (true, _) => wgettext!("Standard input").to_owned(),
@@ -622,7 +588,7 @@ pub fn status(
                         streams.out.append1('\n');
                     }
                 }
-                STATUS_UNDEF | STATUS_SET_JOB_CONTROL | STATUS_FEATURES | STATUS_TEST_FEATURE => {
+                STATUS_SET_JOB_CONTROL | STATUS_FEATURES | STATUS_TEST_FEATURE => {
                     unreachable!("")
                 }
             }

fish-rust/src/ffi.rs

@@ -405,11 +405,11 @@ impl core::convert::From<void_ptr> for *const autocxx::c_void {
     }
 }
 
-impl TryFrom<&wstr> for job_control_t {
+impl TryFrom<&str> for job_control_t {
     type Error = ();
 
-    fn try_from(value: &wstr) -> Result<Self, Self::Error> {
+    fn try_from(value: &str) -> Result<Self, Self::Error> {
-        match value.to_string().as_str() {
+        match value {
             "full" => Ok(job_control_t::all),
             "interactive" => Ok(job_control_t::interactive),
             "none" => Ok(job_control_t::none),