Re-implement eval as a regular builtin

I did not realize builtins could safely call into the parser and inject
jobs during execution. This is much cleaner than hacking around the
required shape of a plain_statement.

CMakeLists.txt

@@ -77,7 +77,7 @@ SET(FISH_SRCS
     src/builtin_random.cpp src/builtin_read.cpp src/builtin_realpath.cpp
     src/builtin_return.cpp src/builtin_set.cpp src/builtin_set_color.cpp
     src/builtin_source.cpp src/builtin_status.cpp src/builtin_string.cpp
-    src/builtin_test.cpp src/builtin_ulimit.cpp src/builtin_wait.cpp
+    src/builtin_test.cpp src/builtin_ulimit.cpp src/builtin_wait.cpp src/builtin_eval.cpp
     src/color.cpp src/common.cpp src/complete.cpp src/env.cpp src/env_dispatch.cpp
     src/env_universal_common.cpp src/event.cpp src/exec.cpp src/expand.cpp
     src/fallback.cpp src/fish_version.cpp src/function.cpp src/highlight.cpp

share/functions/eval.fish

@@ -1,12 +0,0 @@
-# This empty function is required as while `eval` is implemented internally as
-# a decorator, unlike other decorators such as `command` or `builtin`, it is
-# *not* stripped from the statement by the parser before execution to internal
-# quirks in how statements are handled; the presence of this function allows
-# constructs such as command substitution to be used in the head of an
-# evaluated statement.
-#
-# The function defined below is only executed if a bare `eval` is executed (with
-# no arguments), in all other cases it is bypassed entirely.
-
-function eval
-end

src/builtin.cpp

@@ -41,6 +41,7 @@
 #include "builtin_disown.h"
 #include "builtin_echo.h"
 #include "builtin_emit.h"
+#include "builtin_eval.h"
 #include "builtin_exit.h"
 #include "builtin_fg.h"
 #include "builtin_functions.h"
@@ -381,6 +382,7 @@ static const builtin_data_t builtin_datas[] = {
     {L"else", &builtin_generic, N_(L"Evaluate block if condition is false")},
     {L"emit", &builtin_emit, N_(L"Emit an event")},
     {L"end", &builtin_generic, N_(L"End a block of commands")},
+    {L"eval", &builtin_eval, N_(L"Evaluate a string as a statement")},
     {L"exec", &builtin_generic, N_(L"Run command in current process")},
     {L"exit", &builtin_exit, N_(L"Exit the shell")},
     {L"false", &builtin_false, N_(L"Return an unsuccessful result")},

src/builtin_eval.cpp

@@ -0,0 +1,45 @@
+// Functions for executing the jobs builtin.
+#include "config.h"  // IWYU pragma: keep
+
+#include <errno.h>
+#include <stddef.h>
+#ifdef HAVE__PROC_SELF_STAT
+#include <sys/time.h>
+#endif
+
+#include "builtin.h"
+#include "common.h"
+#include "fallback.h"  // IWYU pragma: keep
+#include "io.h"
+#include "parser.h"
+#include "proc.h"
+#include "wgetopt.h"
+#include "wutil.h"  // IWYU pragma: keep
+
+class parser_t;
+
+/// The jobs builtin. Used for printing running jobs. Defined in builtin_jobs.c.
+int builtin_eval(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
+    wchar_t *cmd = argv[0];
+    int argc = builtin_count_args(argv);
+
+    wcstring new_cmd;
+    for (size_t i = 1; i < argc; ++i) {
+        new_cmd += L' ';
+        new_cmd += argv[i];
+    }
+
+    // debug(1, "new_cmd: %ls", new_cmd.c_str());
+
+    auto status = proc_get_last_status();
+    if (argc > 1) {
+        if (parser.eval(new_cmd.c_str(), *streams.io_chain, block_type_t::TOP) != 0) {
+            return STATUS_CMD_ERROR;
+        }
+        status = proc_get_last_status();
+    } else {
+        status = STATUS_CMD_OK;
+    }
+
+    return status;
+}

src/builtin_eval.h

@@ -0,0 +1,9 @@
+// Prototypes for executing builtin_eval function.
+#ifndef FISH_BUILTIN_EVAL_H
+#define FISH_BUILTIN_EVAL_H
+
+class parser_t;
+struct io_streams_t;
+
+int builtin_eval(parser_t &parser, io_streams_t &streams, wchar_t **argv);
+#endif

src/complete.cpp

@@ -1465,14 +1465,6 @@ void completer_t::perform() {
                 use_abbr = false;
                 break;
             }
-            case parse_statement_decoration_eval: {
-                use_command = true;
-                use_function = true;
-                use_builtin = true;
-                use_implicit_cd = true;
-                use_abbr = true;
-                break;
-            }
         }
 
         if (cmd_node.location_in_or_at_end_of_source_range(pos)) {

src/exec.cpp

@@ -982,29 +982,6 @@ static bool exec_process_in_job(parser_t &parser, process_t *p, std::shared_ptr<
                 "Aborting.");
             break;
         }
-
-        case process_type_t::eval: {
-            // int eval(const wcstring &cmd, const io_chain_t &io, enum block_type_t block_type);
-            bool has_args = false;
-            wcstring new_cmd;
-            for (const wchar_t * const* arg = p->get_argv() + 1; *arg != nullptr; ++arg) {
-                has_args = true;
-                new_cmd += L' ';
-                new_cmd += *arg;
-            }
-
-            // `eval` is not supposed to error or do anything at all if no arguments are provided,
-            // or if it is used to execute a function that wouldn't have changed the status code
-            // (e.g. an empty function) if it were executed normally.
-            j->processes[0]->completed = true;
-            p->status = proc_status_t::from_exit_code(cached_status);
-
-            if (has_args) {
-                parser.eval(new_cmd.c_str(), process_net_io_chain, block_type_t::TOP);
-                p->status = proc_status_t::from_exit_code(proc_get_last_status());
-            }
-            break;
-        }
     }
     return true;
 }

src/parse_constants.h

@@ -104,7 +104,6 @@ enum parse_keyword_t {
     parse_keyword_command,
     parse_keyword_else,
     parse_keyword_end,
-    parse_keyword_eval,
     parse_keyword_exclam,
     parse_keyword_exec,
     parse_keyword_for,
@@ -126,7 +125,6 @@ const enum_map<parse_keyword_t> keyword_enum_map[] = {{parse_keyword_exclam, L"!
                                                       {parse_keyword_command, L"command"},
                                                       {parse_keyword_else, L"else"},
                                                       {parse_keyword_end, L"end"},
-                                                      {parse_keyword_eval, L"eval"},
                                                       {parse_keyword_exec, L"exec"},
                                                       {parse_keyword_for, L"for"},
                                                       {parse_keyword_function, L"function"},
@@ -147,7 +145,6 @@ enum parse_statement_decoration_t {
     parse_statement_decoration_command,
     parse_statement_decoration_builtin,
     parse_statement_decoration_exec,
-    parse_statement_decoration_eval,
 };
 
 // Boolean statement types, stored in node tag.

src/parse_execution.cpp

@@ -161,9 +161,6 @@ process_type_t parse_execution_context_t::process_type_for_command(
         case parse_statement_decoration_builtin:
             process_type = process_type_t::builtin;
             break;
-        case parse_statement_decoration_eval:
-            process_type = process_type_t::eval;
-            break;
         case parse_statement_decoration_none:
             if (function_exists(cmd)) {
                 process_type = process_type_t::function;

src/parse_grammar.h

@@ -334,16 +334,7 @@ DEF_ALT(decorated_statement) {
     using cmds = seq<keyword<parse_keyword_command>, plain_statement>;
     using builtins = seq<keyword<parse_keyword_builtin>, plain_statement>;
     using execs = seq<keyword<parse_keyword_exec>, plain_statement>;
-    // Ideally, `evals` should be defined as `seq<keyword<parse_keyword_eval>,
-    // arguments_or_redirections_list`, but other parts of the code have the logic hard coded to
-    // search for a process at the head of a statement, and bug out if we do that.
-    // We also can't define `evals` as a `seq<keyword<parse_keyword_eval>, plain_statement>` because
-    // `expand.cpp` hard-codes its "command substitution at the head of a statement is not allowed"
-    // check without any way of telling it to perform the substitution anyway. Our solution is to
-    // create an empty function called `eval` that never actually gets executed and convert a
-    // decorated statement `eval ...` into a plain statement `eval ...`
-    using evals = seq<plain_statement>;
-    ALT_BODY(decorated_statement, plains, cmds, builtins, execs, evals);
+    ALT_BODY(decorated_statement, plains, cmds, builtins, execs);
 };
 
 DEF(plain_statement)

src/parse_productions.cpp

@@ -314,10 +314,6 @@ RESOLVE(decorated_statement) {
             *out_tag = parse_statement_decoration_exec;
             return production_for<execs>();
         }
-        case parse_keyword_eval: {
-            *out_tag = parse_statement_decoration_eval;
-            return production_for<evals>();
-        }
         default: {
             *out_tag = parse_statement_decoration_none;
             return production_for<plains>();

src/proc.h

@@ -36,8 +36,6 @@ enum class process_type_t {
     block_node,
     /// The exec builtin.
     exec,
-    /// A literal evaluation
-    eval,
 };
 
 enum class job_control_t {