Reimplement prevd-or-backward-word and nextd-or-forward-word in C++

[andmis]

Description

Reimplement prevd-or-backward-word and nextd-or-forward-word in C++.

(Working on #6695.)

TODOs:

• Changes to fish usage are reflected in user documentation/manpages.
• Tests have been added for regressions fixed
• User-visible changes noted in CHANGELOG.rst

[andmis]

As suggested by @faho in #6695.

I considered adding an int exec_subshell(const wcstring &cmd, parser_t &parser, outputter_t &outp, bool apply_exit_status); overload to dedup the code I added in reader.cpp but I'm not really familiar with the codebase yet -- let me know if that is a reasonable thing to do.

[andmis]

The failing test is line 21 of tmux-complete.fish, in a macOS environment. The test passes on my machine (also macOS), and running the commands in the test file manually seems to work fine on my machine (in a tmux, etc.), and I am having trouble seeing how my code is even getting executed in that test. Hmm, am I missing something?

Edit: a test failed on an earlier CI/CD run. It looks from reading the test like it could be racy -- we are asking for autocompletions and reading the prompt to see if the prompt is correct.

[krobelus]

Hmm yeah the tmux tests can fail in resource-constrained environments (since they rely on sleeps).
Long-term we should probably write our own synchronous mock terminal, and port most of these tests.
I'll try to monitor how often these tests fail.

[andmis]

Again, the failing test seems racy/perfy and unrelated -- also passes on my machine:

69/227 Test  #64: history_races ............................***Failed    0.92 sec
Proceeding with target execution
warning: Locking the history file took too long (0.436 seconds).
Testing low-level functionality
Testing history race conditions
Error: Item dropped from history: 2 228
Error: Item dropped from history: 0 152
Error: Item dropped from history: 0 102
Error: Item dropped from history: 3 165
Error: Item dropped from history: 1 117
Error: Item dropped from history: 0 77
Error: Expected 1025 items, but instead got 1019 items
Encountered 7 errors in low-level tests

(Edit: I just did a quick manual test and as far as I can tell, the switch blocks with the logic for forward_word/backward_word and friends in reader.cpp are not even executed during the test suite.)

[krobelus]

LGTM, I only fixed the formatting, see below.

BTW fish also knows special input functions "and" and "or" so in theory this could be
bind x nextd or backward-word but mixing functions (nextd) and special input functions is currently not supported (there's not even an error message, we should probably fix that).

not sure about the term, function seems indeed suboptimal, because they are more like builtins.

$ git range-diff origin/pr-8538@{2}...pr-8538
1:  0d5a1e8ac ! 1:  971a0acca Implement nextd-or-forward-word and prevd-or-backward-word in C++
    @@ src/reader.cpp: void reader_data_t::handle_readline_command(readline_cmd_t c, re
     +        case rl::backward_bigword:
     +        case rl::prevd_or_backward_word: {
     +            if (c == rl::prevd_or_backward_word && command_line.empty()) {
    -+              auto last_statuses = parser().get_last_statuses();
    -+              (void)parser().eval(L"prevd", io_chain_t{});
    -+              parser().set_last_statuses(std::move(last_statuses));
    -+              force_exec_prompt_and_repaint = true;
    -+              inputter.queue_char(readline_cmd_t::repaint);
    -+              break;
    ++                auto last_statuses = parser().get_last_statuses();
    ++                (void)parser().eval(L"prevd", io_chain_t{});
    ++                parser().set_last_statuses(std::move(last_statuses));
    ++                force_exec_prompt_and_repaint = true;
    ++                inputter.queue_char(readline_cmd_t::repaint);
    ++                break;
     +            }
     +
                  auto move_style =
    @@ src/reader.cpp: void reader_data_t::handle_readline_command(readline_cmd_t c, re
     +        case rl::forward_bigword:
     +        case rl::nextd_or_forward_word: {
     +            if (c == rl::nextd_or_forward_word && command_line.empty()) {
    -+              auto last_statuses = parser().get_last_statuses();
    -+              (void)parser().eval(L"nextd", io_chain_t{});
    -+              parser().set_last_statuses(std::move(last_statuses));
    -+              force_exec_prompt_and_repaint = true;
    -+              inputter.queue_char(readline_cmd_t::repaint);
    -+              break;
    ++                auto last_statuses = parser().get_last_statuses();
    ++                (void)parser().eval(L"nextd", io_chain_t{});
    ++                parser().set_last_statuses(std::move(last_statuses));
    ++                force_exec_prompt_and_repaint = true;
    ++                inputter.queue_char(readline_cmd_t::repaint);
    ++                break;
     +            }
     +
                  auto move_style =
2:  d049ade86 = 2:  b7eb0f75d Remove delete-or-exit function, which has a C++ implementation
3:  daea29378 = 3:  969b59c6d Fix delete-or-exit doc wording for local consistency
4:  635da71c0 = 4:  24f9533aa Add documentation for nextd-or-forward-word and prevd-or-backward-word readline functions
5:  66544c872 = 5:  7fb287735 Fix word usage complimenting -> complementing
6:  f218ae18e = 6:  e0bb43c95 Fix underline width
7:  105bb9afc ! 7:  ee04bc857 Add CHANGELOG entry noting special input functions no longer available as fish functions
    @@ CHANGELOG.rst: Deprecations and removed features
      - ``set --query`` now returns a falsy status of 255 if given no variable names. This means ``if set -q $foo`` will not enter the if-block if ``$foo`` is empty or unset. To restore the previous behavior you would use something like ``if not set -q foo; or set -q $foo``. We do not expect anyone to have used this on purpose, any places this happens are almost certainly buggy (:issue:`8214`).
      - Mac OS X 10.9 is no longer supported. The minimum Mac version is now 10.10 "Yosemite."
      - ``_`` is now a reserved keyword (:issue:`8342`).
    -+- New special input functions ``nextd-or-forward-word`` and ``prevd-or-backward-word`` replace fish functions having the same names. The special input function ``delete-or-exit`` is no longer available as fish function.
    ++- New special input functions ``nextd-or-forward-word`` and ``prevd-or-backward-word`` replace fish functions of the same names. The special input function ``delete-or-exit`` is no longer available as fish function (:issue:`8538`).
      
      Scripting improvements
      ----------------------

[andmis]

Oops, sorry about the indent and thanks for fixing it.

[faho]

We should re-add the delete-or-exit function (and keep it around for a while) because it is the default for control-d.

That means upgrading to 3.4.0 would trigger errors if you tried closing 3.3.1 fishes with ctrl+d.

cc @zanchey I think we might also want to add a notice that fish should be restarted after an upgrade?

[andmis]

There is a C++ special input function delete-or-exit also present in 3.3.1 -- why wouldn't the ctrl-d binding just invoke the C++ function?

[andmis]

I just tried it and on my 3.3.1, delete-or-exit doesn't seem to be called. The name is overloaded (there is a special input function and a regular function). It looks like bind prefers to take the special input functions in this case (when there is an overload):

fish-shell/src/input.cpp

Line 396 in a7b4fc3

if (input_function_get_code(cmd)) {

[faho]

There is a C++ special input function delete-or-exit also present in 3.3.1

Aaaah, okay. Looks like this was added in 3.1 already - d415350.

That should be okay, then.