Process shell commands from bindings like regular char events

A long standing issue is that bindings cannot mix special input functions
and shell commands. For example,

    bind x end-of-line "commandline -i x"

silently does nothing. Instead we have to do lift everything to shell commands

    bind x "commandline -f end-of-line; commandline -i x"

for no good reason.

Additionally, there is a weird ordering difference between special input
functions and shell commands. Special input functions are pushed into the
the queue wherease shell commands are executed immediately.

This weird ordering means that the above "bind x" still doesn't work as
expected, because "commandline -i" is processed before "end-of-line".

Finally, this is implemented via weird hack to allow recursive use of a
mutable reference to the reader state.

Fix all of this by processing shell commands the same as both special input
functions and regular chars. Hopefully this doesn't break anything.

Fixes #8186

src/builtins/commandline.rs

@@ -11,9 +11,7 @@ use crate::parse_util::{
     parse_util_token_extent,
 };
 use crate::proc::is_interactive_session;
-use crate::reader::{
-    commandline_get_state, commandline_set_buffer, reader_handle_command, reader_queue_ch,
-};
+use crate::reader::{commandline_get_state, commandline_set_buffer, reader_queue_ch};
 use crate::tokenizer::TOK_ACCEPT_UNFINISHED;
 use crate::tokenizer::{TokenType, Tokenizer};
 use crate::wchar::prelude::*;
@@ -332,18 +330,8 @@ pub fn commandline(parser: &Parser, streams: &mut IoStreams, args: &mut [&wstr])
                 }
             }
 
-            // HACK: Execute these right here and now so they can affect any insertions/changes
-            // made via bindings. The correct solution is to change all `commandline`
-            // insert/replace operations into readline functions with associated data, so that
-            // all queued `commandline` operations - including buffer modifications - are
-            // executed in order
-            match cmd {
-                rl::BeginUndoGroup | rl::EndUndoGroup => reader_handle_command(cmd),
-                _ => {
-                    // Inserts the readline function at the back of the queue.
-                    reader_queue_ch(CharEvent::from_readline(cmd));
-                }
-            }
+            // Inserts the readline function at the back of the queue.
+            reader_queue_ch(CharEvent::from_readline(cmd));
         }
 
         return STATUS_CMD_OK;

src/input.rs

@@ -248,7 +248,7 @@ fn input_get_bind_mode(vars: &dyn Environment) -> WString {
 }
 
 /// Set the current bind mode.
-fn input_set_bind_mode(parser: &Parser, bm: &wstr) {
+pub fn input_set_bind_mode(parser: &Parser, bm: &wstr) {
     // Only set this if it differs to not execute variable handlers all the time.
     // modes may not be empty - empty is a sentinel value meaning to not change the mode
     assert!(!bm.is_empty());
@@ -488,64 +488,28 @@ impl Inputter {
         self.event_storage.clear();
     }
 
-    /// Perform the action of the specified binding. allow_commands controls whether fish commands
-    /// should be executed, or should be deferred until later.
-    fn mapping_execute(
-        &mut self,
-        m: &InputMapping,
-        command_handler: &mut Option<&mut CommandHandler>,
-    ) {
-        // has_functions: there are functions that need to be put on the input queue
-        // has_commands: there are shell commands that need to be evaluated
-        let mut has_commands = false;
-        let mut has_functions = false;
-        for cmd in &m.commands {
-            if input_function_get_code(cmd).is_some() {
-                has_functions = true;
-            } else {
-                has_commands = true;
-            }
-            if has_functions && has_commands {
-                break;
-            }
-        }
-
-        // !has_functions && !has_commands: only set bind mode
-        if !has_commands && !has_functions {
-            if let Some(sets_mode) = m.sets_mode.as_ref() {
-                input_set_bind_mode(&self.parser, sets_mode);
-            }
-            return;
+    /// Perform the action of the specified binding.
+    fn mapping_execute(&mut self, m: &InputMapping) {
+        // Missing bind mode indicates to not reset the mode (#2871)
+        if let Some(sets_mode) = m.sets_mode.as_ref() {
+            self.push_front(CharEvent::from_set_mode(sets_mode.clone()));
         }
-
-        if has_commands && command_handler.is_none() {
-            // We don't want to run commands yet. Put the characters back and return check_exit.
-            self.insert_front(m.seq.chars().map(CharEvent::from_char));
-            self.push_front(CharEvent::from_check_exit());
-            return; // skip the input_set_bind_mode
-        } else if has_functions && !has_commands {
-            // Functions are added at the head of the input queue.
-            for cmd in m.commands.iter().rev() {
-                let code = input_function_get_code(cmd).unwrap();
-                self.function_push_args(code);
-                self.push_front(CharEvent::from_readline_seq(code, m.seq.clone()));
-            }
-        } else if has_commands && !has_functions {
-            // Execute all commands.
-            //
-            // FIXME(snnw): if commands add stuff to input queue (e.g. commandline -f execute), we won't
-            // see that until all other commands have also been run.
-            let command_handler = command_handler.as_mut().unwrap();
-            command_handler(&m.commands);
-            self.push_front(CharEvent::from_check_exit());
-        } else {
-            // Invalid binding, mixed commands and functions.  We would need to execute these one by
-            // one.
+        let has_command = m
+            .commands
+            .iter()
+            .any(|cmd| input_function_get_code(cmd).is_none());
+        if has_command {
             self.push_front(CharEvent::from_check_exit());
         }
-        // Missing bind mode indicates to not reset the mode (#2871)
-        if let Some(sets_mode) = m.sets_mode.as_ref() {
-            input_set_bind_mode(&self.parser, sets_mode);
+        for cmd in m.commands.iter().rev() {
+            let evt = match input_function_get_code(cmd) {
+                Some(code) => {
+                    self.function_push_args(code);
+                    CharEvent::from_readline_seq(code, m.seq.clone())
+                }
+                None => CharEvent::from_command(cmd.clone()),
+            };
+            self.push_front(evt);
         }
     }
 
@@ -667,7 +631,7 @@ impl EventQueuePeeker<'_> {
     fn char_sequence_interrupted(&self) -> bool {
         self.peeked
             .iter()
-            .any(|evt| evt.is_readline() || evt.is_check_exit())
+            .any(|evt| evt.is_readline_or_command() || evt.is_check_exit())
     }
 
     /// Reset our index back to 0.
@@ -809,10 +773,7 @@ impl Inputter {
         }
     }
 
-    fn mapping_execute_matching_or_generic(
-        &mut self,
-        command_handler: &mut Option<&mut CommandHandler>,
-    ) {
+    fn mapping_execute_matching_or_generic(&mut self) {
         let vars = self.parser.vars_ref();
         let mut peeker = EventQueuePeeker::new(self);
         // Check for mouse-tracking CSI before mappings to prevent the generic mapping handler from
@@ -837,7 +798,7 @@ impl Inputter {
         // Check for ordinary mappings.
         if let Some(mapping) = Self::find_mapping(&*vars, &mut peeker) {
             peeker.consume();
-            self.mapping_execute(&mapping, command_handler);
+            self.mapping_execute(&mapping);
             return;
         }
         peeker.restart();
@@ -868,7 +829,7 @@ impl Inputter {
         let evt_to_return: CharEvent;
         loop {
             let evt = self.readch();
-            if evt.is_readline() {
+            if evt.is_readline_or_command() {
                 saved_events.push(evt);
             } else {
                 evt_to_return = evt;
@@ -891,11 +852,7 @@ impl Inputter {
     /// to be an escape sequence for a special character (such as an arrow key), and readch attempts
     /// to parse it. If no more input follows after the escape key, it is assumed to be an actual
     /// escape key press, and is returned as such.
-    ///
-    /// \p command_handler is used to run commands. If empty (in the std::function sense), when a
-    /// character is encountered that would invoke a fish command, it is unread and
-    /// char_event_type_t::check_exit is returned. Note the handler is not stored.
-    pub fn read_char(&mut self, mut command_handler: Option<&mut CommandHandler>) -> CharEvent {
+    pub fn read_char(&mut self) -> CharEvent {
         // Clear the interrupted flag.
         reader_reset_interrupted();
 
@@ -933,6 +890,9 @@ impl Inputter {
                         return evt;
                     }
                 },
+                CharEventType::Command(_) | CharEventType::SetMode(_) => {
+                    return evt;
+                }
                 CharEventType::Eof => {
                     // If we have EOF, we need to immediately quit.
                     // There's no need to go through the input functions.
@@ -944,10 +904,7 @@ impl Inputter {
                 }
                 CharEventType::Char(_) => {
                     self.push_front(evt);
-                    self.mapping_execute_matching_or_generic(&mut command_handler);
-                    // Regarding allow_commands, we're in a loop, but if a fish command is executed,
-                    // check_exit is unread, so the next pass through the loop we'll break out and return
-                    // it.
+                    self.mapping_execute_matching_or_generic();
                 }
             }
         }

src/input_common.rs

@@ -116,14 +116,20 @@ pub enum ReadlineCmd {
 }
 
 /// Represents an event on the character input stream.
-#[derive(Debug, Copy, Clone)]
+#[derive(Debug, Clone)]
 pub enum CharEventType {
     /// A character was entered.
     Char(char),
 
     /// A readline event.
     Readline(ReadlineCmd),
 
+    /// A shell command.
+    Command(WString),
+
+    /// A request to change the input mapping mode.
+    SetMode(WString),
+
     /// end-of-file was reached.
     Eof,
 
@@ -162,6 +168,13 @@ impl CharEvent {
         matches!(self.evt, CharEventType::Readline(_))
     }
 
+    pub fn is_readline_or_command(&self) -> bool {
+        matches!(
+            self.evt,
+            CharEventType::Readline(_) | CharEventType::Command(_) | CharEventType::SetMode(_)
+        )
+    }
+
     pub fn get_char(&self) -> char {
         let CharEventType::Char(c) = self.evt else {
             panic!("Not a char type");
@@ -184,6 +197,20 @@ impl CharEvent {
         c
     }
 
+    pub fn get_command(&self) -> Option<&wstr> {
+        match &self.evt {
+            CharEventType::Command(c) => Some(&c),
+            _ => None,
+        }
+    }
+
+    pub fn get_mode(&self) -> Option<&wstr> {
+        match &self.evt {
+            CharEventType::SetMode(m) => Some(&m),
+            _ => None,
+        }
+    }
+
     pub fn from_char(c: char) -> CharEvent {
         CharEvent {
             evt: CharEventType::Char(c),
@@ -204,6 +231,22 @@ impl CharEvent {
         }
     }
 
+    pub fn from_command(cmd: WString) -> CharEvent {
+        CharEvent {
+            evt: CharEventType::Command(cmd),
+            input_style: CharInputStyle::Normal,
+            seq: WString::new(),
+        }
+    }
+
+    pub fn from_set_mode(mode: WString) -> CharEvent {
+        CharEvent {
+            evt: CharEventType::SetMode(mode),
+            input_style: CharInputStyle::Normal,
+            seq: WString::new(),
+        }
+    }
+
     pub fn from_check_exit() -> CharEvent {
         CharEvent {
             evt: CharEventType::CheckExit,
@@ -587,7 +630,7 @@ pub trait InputEventQueuer {
     fn drop_leading_readline_events(&mut self) {
         let queue = self.get_queue_mut();
         while let Some(evt) = queue.front() {
-            if evt.is_readline() {
+            if evt.is_readline_or_command() {
                 queue.pop_front();
             } else {
                 break;

src/reader.rs

@@ -67,8 +67,8 @@ use crate::history::{
     history_session_id, in_private_mode, History, HistorySearch, PersistenceMode, SearchDirection,
     SearchType,
 };
-use crate::input::init_input;
 use crate::input::Inputter;
+use crate::input::{init_input, input_set_bind_mode};
 use crate::input_common::{CharEvent, CharInputStyle, ReadlineCmd};
 use crate::io::IoChain;
 use crate::kill::{kill_add, kill_replace, kill_yank, kill_yank_rotate};
@@ -1800,8 +1800,8 @@ impl ReaderData {
                 continue;
             }
             assert!(
-                event_needing_handling.is_char() || event_needing_handling.is_readline(),
-                "Should have a char or readline"
+                event_needing_handling.is_char() || event_needing_handling.is_readline_or_command(),
+                "Should have a char, readline or command"
             );
 
             if !matches!(rls.last_cmd, Some(rl::Yank | rl::YankPop)) {
@@ -1837,6 +1837,10 @@ impl ReaderData {
                 }
 
                 rls.last_cmd = Some(readline_cmd);
+            } else if let Some(command) = event_needing_handling.get_command() {
+                zelf.run_input_command_scripts(command);
+            } else if let Some(mode) = event_needing_handling.get_mode() {
+                input_set_bind_mode(zelf.parser(), mode);
             } else {
                 // Ordinary char.
                 let c = event_needing_handling.get_char();
@@ -1909,13 +1913,11 @@ impl ReaderData {
     }
 
     /// Run a sequence of commands from an input binding.
-    fn run_input_command_scripts(&mut self, cmds: &[WString]) {
+    fn run_input_command_scripts(&mut self, cmd: &wstr) {
         let last_statuses = self.parser().vars().get_last_statuses();
-        for cmd in cmds {
-            self.update_commandline_state();
-            self.parser().eval(cmd, &IoChain::new());
-            self.apply_commandline_state_changes();
-        }
+        self.update_commandline_state();
+        self.parser().eval(cmd, &IoChain::new());
+        self.apply_commandline_state_changes();
         self.parser().set_last_statuses(last_statuses);
 
         // Restore tty to shell modes.
@@ -1953,22 +1955,8 @@ impl ReaderData {
         let mut last_exec_count = self.exec_count();
         let mut accumulated_chars = WString::new();
 
-        let mut command_handler = {
-            // TODO Remove this hack.
-            let zelf = self as *mut Self;
-            move |cmds: &[WString]| {
-                // Safety: this is a pinned pointer.
-                let zelf = unsafe { &mut *zelf };
-                zelf.run_input_command_scripts(cmds);
-            }
-        };
-
         while accumulated_chars.len() < limit {
-            let evt = {
-                let allow_commands = accumulated_chars.is_empty();
-                self.inputter
-                    .read_char(allow_commands.then_some(&mut command_handler))
-            };
+            let evt = self.inputter.read_char();
             if !evt.is_char() || !poll_fd_readable(self.conf.inputfd) {
                 event_needing_handling = Some(evt);
                 break;

src/tests/input.rs

@@ -44,7 +44,7 @@ fn test_input() {
     }
 
     // Now test.
-    let evt = input.read_char(None);
+    let evt = input.read_char();
     if !evt.is_readline() {
         panic!("Event is not a readline");
     } else if evt.get_readline() != ReadlineCmd::DownLine {

tests/pexpects/undo.py

@@ -19,16 +19,8 @@
 expect_str("echo word")
 expect_str("echo word")  # Not sure why we get this twice.
 
-# FIXME why does this only undo one character? It undoes the entire word when run interactively.
 send("Undo")
-expect_str("echo wor")
+expect_str("echo")
 
-send("Undo")
-expect_str("echo ")
-
-send("Redo")
-expect_str("echo wor")
-
-# FIXME see above.
 send("Redo")
 expect_str("echo word")