Implement trigger-on for abbreviations

trigger-on enables abbreviations to trigger only on "entry" (anything
which closes a token, like space) or only on "exec" (typically enter key).

src/abbrs.cpp

@@ -18,21 +18,11 @@ bool abbreviation_t::matches_position(abbrs_position_t position) const {
     return this->position == abbrs_position_t::anywhere || this->position == position;
 }
 
-bool abbreviation_t::matches_phase(abbrs_phase_t phase) const {
-    switch (phase) {
-        case abbrs_phase_t::noisy:
-            return !this->is_quiet;
-        case abbrs_phase_t::quiet:
-            return this->is_quiet;
-        case abbrs_phase_t::any:
-            return true;
-    }
-    DIE("Unreachable");
-}
+bool abbreviation_t::matches_phases(abbrs_phases_t p) const { return bool(this->phases & p); }
 
 bool abbreviation_t::matches(const wcstring &token, abbrs_position_t position,
-                             abbrs_phase_t phase) const {
-    if (!this->matches_position(position) || !this->matches_phase(phase)) {
+                             abbrs_phases_t phases) const {
+    if (!this->matches_position(position) || !this->matches_phases(phases)) {
         return false;
     }
     if (this->is_regex()) {
@@ -48,12 +38,12 @@ acquired_lock<abbrs_set_t> abbrs_get_set() {
 }
 
 abbrs_replacer_list_t abbrs_set_t::match(const wcstring &token, abbrs_position_t position,
-                                         abbrs_phase_t phase) const {
+                                         abbrs_phases_t phases) const {
     abbrs_replacer_list_t result{};
     // Later abbreviations take precedence so walk backwards.
     for (auto it = abbrs_.rbegin(); it != abbrs_.rend(); ++it) {
         const abbreviation_t &abbr = *it;
-        if (abbr.matches(token, position, phase)) {
+        if (abbr.matches(token, position, phases)) {
             result.push_back(abbrs_replacer_t{abbr.replacement, abbr.replacement_is_function,
                                               abbr.set_cursor_indicator});
         }
@@ -62,9 +52,9 @@ abbrs_replacer_list_t abbrs_set_t::match(const wcstring &token, abbrs_position_t
 }
 
 bool abbrs_set_t::has_match(const wcstring &token, abbrs_position_t position,
-                            abbrs_phase_t phase) const {
+                            abbrs_phases_t phases) const {
     for (const auto &abbr : abbrs_) {
-        if (abbr.matches(token, position, phase)) {
+        if (abbr.matches(token, position, phases)) {
             return true;
         }
     }

src/abbrs.h

@@ -20,11 +20,24 @@ enum class abbrs_position_t : uint8_t {
 };
 
 /// Describes a phase of expansion.
-enum class abbrs_phase_t : uint8_t {
-    noisy,  // expand noisy abbreviations, which visibly replace their tokens.
-    quiet,  // expand quiet abbreviations, which do not visibly replace their tokens.
-    any,    // phase is ignored - useful for syntax highlighting.
+enum abbrs_phase_t : uint8_t {
+    // Expands "on space" immediately after the user types it.
+    abbrs_phase_entry = 1 << 0,
+
+    // Expands "on enter" before submitting the command to be executed.
+    abbrs_phase_exec = 1 << 1,
+
+    // Expands "quietly" after submitting the command. This is incompatible with the other
+    // phases.
+    abbrs_phase_quiet = 1 << 2,
+
+    // Default set of phases.
+    abbrs_phases_default = abbrs_phase_entry | abbrs_phase_exec,
+
+    // All phases.
+    abbrs_phases_all = abbrs_phase_entry | abbrs_phase_exec | abbrs_phase_quiet,
 };
+using abbrs_phases_t = uint8_t;
 
 struct abbreviation_t {
     // Abbreviation name. This is unique within the abbreviation set.
@@ -55,15 +68,14 @@ struct abbreviation_t {
     /// Mark if we came from a universal variable.
     bool from_universal{};
 
-    /// Whether this abbrevation is quiet. Noisy abbreviations visibly replace their tokens in the
-    /// command line any history, quiet ones do not (unless expansion results in a syntax error).
-    bool is_quiet{false};
+    /// Set of phases in which this abbreviation expands.
+    abbrs_phases_t phases{abbrs_phases_default};
 
     // \return true if this is a regex abbreviation.
     bool is_regex() const { return this->regex.has_value(); }
 
-    // \return true if we match a token at a given position in a given phase.
-    bool matches(const wcstring &token, abbrs_position_t position, abbrs_phase_t phase) const;
+    // \return true if we match a token at a given position in a given set of phases.
+    bool matches(const wcstring &token, abbrs_position_t position, abbrs_phases_t phases) const;
 
     // Construct from a name, a key which matches a token, a replacement token, a position, and
     // whether we are derived from a universal variable.
@@ -77,8 +89,8 @@ struct abbreviation_t {
     // \return if we expand in a given position.
     bool matches_position(abbrs_position_t position) const;
 
-    // \return if we expand in a given phase.
-    bool matches_phase(abbrs_phase_t phase) const;
+    // \return if we expand in a given set of phases.
+    bool matches_phases(abbrs_phases_t p) const;
 };
 
 /// The result of an abbreviation expansion.
@@ -118,10 +130,10 @@ class abbrs_set_t {
     /// \return the list of replacers for an input token, in priority order.
     /// The \p position is given to describe where the token was found.
     abbrs_replacer_list_t match(const wcstring &token, abbrs_position_t position,
-                                abbrs_phase_t phase) const;
+                                abbrs_phases_t phases) const;
 
     /// \return whether we would have at least one replacer for a given token.
-    bool has_match(const wcstring &token, abbrs_position_t position, abbrs_phase_t phase) const;
+    bool has_match(const wcstring &token, abbrs_position_t position, abbrs_phases_t phases) const;
 
     /// Add an abbreviation. Any abbreviation with the same name is replaced.
     void add(abbreviation_t &&abbr);
@@ -158,8 +170,8 @@ acquired_lock<abbrs_set_t> abbrs_get_set();
 /// \return the list of replacers for an input token, in priority order, using the global set.
 /// The \p position is given to describe where the token was found.
 inline abbrs_replacer_list_t abbrs_match(const wcstring &token, abbrs_position_t position,
-                                         abbrs_phase_t phase) {
-    return abbrs_get_set()->match(token, position, phase);
+                                         abbrs_phases_t phases) {
+    return abbrs_get_set()->match(token, position, phases);
 }
 
 #endif

src/builtins/abbr.cpp

@@ -41,13 +41,14 @@ struct abbr_options_t {
     bool function{};
     maybe_t<wcstring> regex_pattern;
     maybe_t<abbrs_position_t> position{};
+    maybe_t<abbrs_phases_t> phases{};
     maybe_t<wcstring> set_cursor_indicator{};
 
-    bool quiet{};
-
     wcstring_list_t args;
 
     bool validate(io_streams_t &streams) {
+        const bool quiet = (phases.value_or(0) & abbrs_phase_quiet);
+
         // Duplicate options?
         wcstring_list_t cmds;
         if (add) cmds.push_back(L"add");
@@ -83,6 +84,9 @@ struct abbr_options_t {
         if (!add && quiet) {
             streams.err.append_format(_(L"%ls: --quiet option requires --add\n"), CMD);
             return false;
+        } else if (!add && phases.has_value()) {
+            streams.err.append_format(_(L"%ls: --trigger-on option requires --add\n"), CMD);
+            return false;
         }
         if (!add && set_cursor_indicator.has_value()) {
             streams.err.append_format(_(L"%ls: --set-cursor option requires --add\n"), CMD);
@@ -96,6 +100,11 @@ struct abbr_options_t {
             streams.err.append_format(_(L"%ls: --set-cursor argument cannot be empty\n"), CMD);
             return false;
         }
+        if (quiet && (*phases & ~abbrs_phase_quiet) != 0) {
+            streams.err.append_format(_(L"%ls: Cannot use --quiet with --trigger-on\n"), CMD);
+            return false;
+        }
+
         return true;
     }
 };
@@ -120,8 +129,16 @@ static int abbr_show(const abbr_options_t &, io_streams_t &streams) {
             comps.push_back(L"--set-cursor");
             comps.push_back(escape_string(*abbr.set_cursor_indicator));
         }
-        if (abbr.is_quiet) {
-            comps.push_back(L"--quiet");
+        if (abbr.phases != abbrs_phases_default) {
+            if (abbr.phases & abbrs_phase_entry) {
+                comps.push_back(L"--trigger-on entry");
+            }
+            if (abbr.phases & abbrs_phase_exec) {
+                comps.push_back(L"--trigger-on exec");
+            }
+            if (abbr.phases & abbrs_phase_quiet) {
+                comps.push_back(L"--quiet");
+            }
         }
         if (abbr.replacement_is_function) {
             comps.push_back(L"--function");
@@ -262,7 +279,7 @@ static int abbr_add(const abbr_options_t &opts, io_streams_t &streams) {
     abbr.regex = std::move(regex);
     abbr.replacement_is_function = opts.function;
     abbr.set_cursor_indicator = opts.set_cursor_indicator;
-    abbr.is_quiet = opts.quiet;
+    abbr.phases = opts.phases.value_or(abbrs_phases_default);
     abbrs_get_set()->add(std::move(abbr));
     return STATUS_CMD_OK;
 }
@@ -300,6 +317,7 @@ maybe_t<int> builtin_abbr(parser_t &parser, io_streams_t &streams, const wchar_t
     static const struct woption long_options[] = {{L"add", no_argument, 'a'},
                                                   {L"position", required_argument, 'p'},
                                                   {L"regex", required_argument, REGEX_SHORT},
+                                                  {L"trigger-on", required_argument, 't'},
                                                   {L"quiet", no_argument, QUIET_SHORT},
                                                   {L"set-cursor", required_argument, 'C'},
                                                   {L"function", no_argument, 'f'},
@@ -344,8 +362,8 @@ maybe_t<int> builtin_abbr(parser_t &parser, io_streams_t &streams, const wchar_t
                 } else if (!wcscmp(w.woptarg, L"anywhere")) {
                     opts.position = abbrs_position_t::anywhere;
                 } else {
-                    streams.err.append_format(_(L"%ls: Invalid position '%ls'\nPosition must be "
-                                                L"one of: command, anywhere.\n"),
+                    streams.err.append_format(_(L"%ls: Invalid position '%ls'\n"
+                                                L"Position must be one of: command, anywhere.\n"),
                                               CMD, w.woptarg);
                     return STATUS_INVALID_ARGS;
                 }
@@ -360,8 +378,23 @@ maybe_t<int> builtin_abbr(parser_t &parser, io_streams_t &streams, const wchar_t
                 opts.regex_pattern = w.woptarg;
                 break;
             }
+            case 't': {
+                abbrs_phases_t phases = opts.phases.value_or(0);
+                if (!wcscmp(w.woptarg, L"entry")) {
+                    phases |= abbrs_phase_entry;
+                } else if (!wcscmp(w.woptarg, L"exec")) {
+                    phases |= abbrs_phase_exec;
+                } else {
+                    streams.err.append_format(_(L"%ls: Invalid --trigger-on '%ls'\n"
+                                                L"Must be one of: entry, exec.\n"),
+                                              CMD, w.woptarg);
+                    return STATUS_INVALID_ARGS;
+                }
+                opts.phases = phases;
+                break;
+            }
             case QUIET_SHORT: {
-                opts.quiet = true;
+                opts.phases = opts.phases.value_or(0) | abbrs_phase_quiet;
                 break;
             }
             case 'C': {

src/fish_tests.cpp

@@ -2481,7 +2481,7 @@ static void test_abbreviations() {
 
     // Helper to expand an abbreviation, enforcing we have no more than one result.
     auto abbr_expand_1 = [](const wcstring &token, abbrs_position_t pos) -> maybe_t<wcstring> {
-        auto result = abbrs_match(token, pos, abbrs_phase_t::any);
+        auto result = abbrs_match(token, pos, abbrs_phases_all);
         if (result.size() > 1) {
             err(L"abbreviation expansion for %ls returned more than 1 result", token.c_str());
         }
@@ -2507,7 +2507,7 @@ static void test_abbreviations() {
     auto expand_abbreviation_in_command = [](const wcstring &cmdline,
                                              size_t cursor_pos) -> maybe_t<wcstring> {
         if (auto replacement = reader_expand_abbreviation_at_cursor(
-                cmdline, cursor_pos, abbrs_phase_t::noisy, parser_t::principal_parser())) {
+                cmdline, cursor_pos, abbrs_phases_default, parser_t::principal_parser())) {
             wcstring cmdline_expanded = cmdline;
             std::vector<highlight_spec_t> colors{cmdline_expanded.size()};
             apply_edit(&cmdline_expanded, &colors, edit_t{replacement->range, replacement->text});

src/highlight.cpp

@@ -1336,7 +1336,7 @@ static bool command_is_valid(const wcstring &cmd, enum statement_decoration_t de
 
     // Abbreviations
     if (!is_valid && abbreviation_ok)
-        is_valid = abbrs_get_set()->has_match(cmd, abbrs_position_t::command, abbrs_phase_t::any);
+        is_valid = abbrs_get_set()->has_match(cmd, abbrs_position_t::command, abbrs_phases_default);
 
     // Regular commands
     if (!is_valid && command_ok) is_valid = path_get_path(cmd, vars).has_value();

src/reader.cpp

@@ -792,8 +792,9 @@ class reader_data_t : public std::enable_shared_from_this<reader_data_t> {
     /// Do what we need to do whenever our pager selection changes.
     void pager_selection_changed();
 
-    /// Expand abbreviations at the current cursor position, minus backtrack_amt.
-    bool expand_abbreviation_at_cursor(size_t cursor_backtrack, abbrs_phase_t phase);
+    /// Expand abbreviations in the given phases at the current cursor position, minus
+    /// cursor_backtrack.
+    bool expand_abbreviation_at_cursor(size_t cursor_backtrack, abbrs_phases_t phases);
 
     /// \return true if the command line has changed and repainting is needed. If \p colors is not
     /// null, then also return true if the colors have changed.
@@ -1453,7 +1454,7 @@ static std::vector<positioned_token_t> extract_tokens(const wcstring &str) {
 /// cursor. \return the replacement. This does NOT inspect the current reader data.
 maybe_t<abbrs_replacement_t> reader_expand_abbreviation_at_cursor(const wcstring &cmdline,
                                                                   size_t cursor_pos,
-                                                                  abbrs_phase_t phase,
+                                                                  abbrs_phases_t phases,
                                                                   parser_t &parser) {
     // Find the token containing the cursor. Usually users edit from the end, so walk backwards.
     const auto tokens = extract_tokens(cmdline);
@@ -1468,7 +1469,7 @@ maybe_t<abbrs_replacement_t> reader_expand_abbreviation_at_cursor(const wcstring
         iter->is_cmd ? abbrs_position_t::command : abbrs_position_t::anywhere;
 
     wcstring token_str = cmdline.substr(range.start, range.length);
-    auto replacers = abbrs_match(token_str, position, phase);
+    auto replacers = abbrs_match(token_str, position, phases);
     for (const auto &replacer : replacers) {
         if (auto replacement = expand_replacer(range, token_str, replacer, parser)) {
             return replacement;
@@ -1480,7 +1481,7 @@ maybe_t<abbrs_replacement_t> reader_expand_abbreviation_at_cursor(const wcstring
 /// Expand abbreviations at the current cursor position, minus the given cursor backtrack. This may
 /// change the command line but does NOT repaint it. This is to allow the caller to coalesce
 /// repaints.
-bool reader_data_t::expand_abbreviation_at_cursor(size_t cursor_backtrack, abbrs_phase_t phase) {
+bool reader_data_t::expand_abbreviation_at_cursor(size_t cursor_backtrack, abbrs_phases_t phases) {
     bool result = false;
     editable_line_t *el = active_edit_line();
 
@@ -1488,7 +1489,7 @@ bool reader_data_t::expand_abbreviation_at_cursor(size_t cursor_backtrack, abbrs
         // Try expanding abbreviations.
         this->update_commandline_state();
         size_t cursor_pos = el->position() - std::min(el->position(), cursor_backtrack);
-        if (auto replacement = reader_expand_abbreviation_at_cursor(el->text(), cursor_pos, phase,
+        if (auto replacement = reader_expand_abbreviation_at_cursor(el->text(), cursor_pos, phases,
                                                                     this->parser())) {
             push_edit(el, edit_t{replacement->range, std::move(replacement->text)});
             update_buff_pos(el, replacement->cursor);
@@ -1517,7 +1518,7 @@ static bool expand_quiet_abbreviations(wcstring *inout_str, parser_t &parser) {
 
         abbrs_position_t position =
             pt.is_cmd ? abbrs_position_t::command : abbrs_position_t::anywhere;
-        auto replacers = abbrs_match(token, position, abbrs_phase_t::quiet);
+        auto replacers = abbrs_match(token, position, abbrs_phase_quiet);
         for (const auto &replacer : replacers) {
             const auto replacement = expand_replacer(orig_range, token, replacer, parser);
             if (replacement && replacement->text != token) {
@@ -4230,7 +4231,7 @@ void reader_data_t::handle_readline_command(readline_cmd_t c, readline_loop_stat
         }
 
         case rl::expand_abbr: {
-            if (expand_abbreviation_at_cursor(1, abbrs_phase_t::noisy)) {
+            if (expand_abbreviation_at_cursor(1, abbrs_phase_entry)) {
                 inputter.function_set_status(true);
             } else {
                 inputter.function_set_status(false);
@@ -4328,9 +4329,9 @@ parser_test_error_bits_t reader_data_t::expand_for_execute(wcstring *to_exec) {
         if (test_res & PARSER_TEST_ERROR) return test_res;
     }
 
-    // Noisy abbreviations at the cursor.
+    // Exec abbreviations at the cursor.
     // Note we want to expand abbreviations even if incomplete.
-    if (expand_abbreviation_at_cursor(0, abbrs_phase_t::noisy)) {
+    if (expand_abbreviation_at_cursor(0, abbrs_phase_exec)) {
         // Trigger syntax highlighting as we are likely about to execute this command.
         this->super_highlight_me_plenty();
         if (conf.syntax_check_ok) {

src/reader.h

@@ -267,11 +267,11 @@ wcstring combine_command_and_autosuggestion(const wcstring &cmdline,
 /// Expand at most one abbreviation at the given cursor position, updating the position if the
 /// abbreviation wants to move the cursor. Use the parser to run any abbreviations which want
 /// function calls. \return none if no abbreviations were expanded, otherwise the resulting edit.
-enum class abbrs_phase_t : uint8_t;
+using abbrs_phases_t = uint8_t;
 struct abbrs_replacement_t;
 maybe_t<abbrs_replacement_t> reader_expand_abbreviation_at_cursor(const wcstring &cmdline,
                                                                   size_t cursor_pos,
-                                                                  abbrs_phase_t phase,
+                                                                  abbrs_phases_t phases,
                                                                   parser_t &parser);
 
 /// Apply a completion string. Exposed for testing only.

tests/checks/abbr.fish

@@ -168,8 +168,23 @@ abbr --add bogus --position never stuff
 abbr --add bogus --position anywhere --position command stuff
 # CHECKERR: abbr: Cannot specify multiple positions
 
+abbr --add --trigger-on derp zzxjoanw stuff
+# CHECKERR: abbr: Invalid --trigger-on 'derp'
+# CHECKERR: Must be one of: entry, exec.
+
+abbr --add --trigger-on entry --quiet zzxjoanw stuff
+# CHECKERR: abbr: Cannot use --quiet with --trigger-on
+
+abbr --add --quiet --trigger-on exec zzxjoanw stuff
+# CHECKERR: abbr: Cannot use --quiet with --trigger-on
+
 abbr --add quiet-abbr --quiet foo1
 abbr --add loud-abbr foo2
 abbr --show
 # CHECK: abbr -a -- quiet-abbr --quiet foo1
 # CHECK: abbr -a -- loud-abbr foo2
+
+abbr --add loud-abbr foo2
+abbr --show
+# CHECK: abbr -a -- quiet-abbr --quiet foo1
+# CHECK: abbr -a -- loud-abbr foo2

tests/pexpects/abbrs.py

@@ -160,3 +160,20 @@ sendline(r"""abbr LLL --position anywhere --set-cursor !HERE! '!HERE! | less'"""
 expect_prompt()
 send(r"""echo LLL derp?""")
 expect_str(r"echo derp | less ")
+
+# Test trigger-on.
+sendline(r"""abbr --erase (abbr --list) """)
+expect_prompt()
+sendline(r"""abbr --add entry-only --position anywhere --trigger-on entry 'worked1'""")
+expect_prompt()
+sendline(r"""abbr --add exec-only --position anywhere --trigger-on exec 'worked2'""")
+expect_prompt()
+sendline(r"echo entry-only")  # should not trigger, no space
+expect_prompt(r"entry-only")
+sendline(r"echo entry-only ")  # should trigger, there is a space
+expect_prompt(r"worked1")
+
+sendline(r"echo exec-only")  # should trigger, no space
+expect_prompt(r"worked2")
+sendline(r"echo exec-only ")  # should not trigger, there is a space
+expect_prompt(r"exec-only")