Make eval a decorator

`eval` has always been implemented as a function, which was always a bit
of a hack that caused some issues such as triggering the creation of a
new scope. This turns `eval` into a decorator.

The scoping issues with eval prevented it from being usable to actually
implement other shell components in fish script, such as the problems
described in #4442, which should now no longer be the case.

Closes #4443.

share/functions/eval.fish

@@ -1,71 +1,3 @@
-function eval -S -d "Evaluate parameters as a command"
-    # keep a copy of the previous $status and use restore_status
-    # to preserve the status in case the block that is evaluated
-    # does not modify the status itself.
-    set -l status_copy $status
-    function __fish_restore_status
-        return $argv[1]
-    end
+function eval
 
-    if not set -q argv[2]
-        # like most builtins, we only check for -h/--help
-        # if we only have a single argument
-        switch "$argv[1]"
-            case -h --help
-                __fish_print_help eval
-                return 0
-        end
-    end
-
-    if not string length -q -- $argv
-        # If the argument is empty, eval should return 0 for compatibility with other shells.
-        # See #5692.
-        return 0
-    end
-
-    # If we are in an interactive shell, eval should enable full
-    # job control since it should behave like the real code was
-    # executed.  If we don't do this, commands that expect to be
-    # used interactively, like less, wont work using eval.
-
-    set -l mode
-    if status --is-interactive-job-control
-        set mode interactive
-    else
-        if status --is-full-job-control
-            set mode full
-        else
-            set mode none
-        end
-    end
-    if status --is-interactive
-        status --job-control full
-    end
-    __fish_restore_status $status_copy
-
-    # To eval 'foo', we construct a block "begin ; foo; end <&3 3<&-"
-    # Note the redirections are also within the quotes.
-    #
-    # We then pipe this to 'source 3<&0’.
-    #
-    # You might expect that the dup2(3, stdin) should overwrite stdin,
-    # and therefore prevent 'source' from reading the piped-in block. This doesn't happen
-    # because when you pipe to a builtin, we don't overwrite stdin with the read end
-    # of the block; instead we set a separate fd in a variable 'builtin_stdin', which is
-    # what it reads from. So builtins are magic in that, in pipes, their stdin
-    # is not fd 0.
-    #
-    # ‘source’ does not apply the redirections to itself. Instead it saves them and passes
-    # them as block-level redirections to parser.eval(). Ultimately the eval’d code sees
-    # the following redirections (in the following order):
-    #    dup2 0 -> 3
-    #    dup2 pipe -> 0
-    #    dup2 3 -> 0
-    # where the pipe is the pipe we get from piping ‘echo’ to ‘source’. Thus the redirection
-    # effectively makes stdin fd0, instead of the thing that was piped to ‘source’
-    echo "begin; $argv "\n" ;end <&3 3<&-" | source 3<&0
-    set -l res $status
-
-    status --job-control $mode
-    return $res
 end

src/complete.cpp

@@ -1469,8 +1469,8 @@ void completer_t::perform() {
                 use_command = true;
                 use_function = true;
                 use_builtin = true;
-                use_implicit_cd = false;
-                use_abbr = false;
+                use_implicit_cd = true;
+                use_abbr = true;
                 break;
             }
         }

src/exec.cpp

@@ -6,6 +6,7 @@
 
 #include <errno.h>
 #include <fcntl.h>
+#include <numeric>
 #ifdef HAVE_SIGINFO_H
 #include <siginfo.h>
 #endif
@@ -903,6 +904,7 @@ static bool exec_process_in_job(parser_t &parser, process_t *p, std::shared_ptr<
 
     // The IO chain for this process.
     io_chain_t process_net_io_chain = j->block_io_chain();
+    auto cached_status = proc_get_last_status();
 
     if (pipes.write.valid()) {
         process_net_io_chain.push_back(std::make_shared<io_pipe_t>(
@@ -944,7 +946,6 @@ static bool exec_process_in_job(parser_t &parser, process_t *p, std::shared_ptr<
     // Execute the process.
     p->check_generations_before_launch();
     switch (p->type) {
-        case process_type_t::eval: /* so long as `eval` is a function */
         case process_type_t::function:
         case process_type_t::block_node: {
             // Allow buffering unless this is a deferred run. If deferred, then processes after us
@@ -982,6 +983,29 @@ 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_execution.cpp

@@ -151,22 +151,30 @@ process_type_t parse_execution_context_t::process_type_for_command(
     // etc).
     enum parse_statement_decoration_t decoration = get_decoration(statement);
 
-    if (decoration == parse_statement_decoration_exec) {
-        // Always exec.
-        process_type = process_type_t::exec;
-    } else if (decoration == parse_statement_decoration_command) {
-        // Always a command.
-        process_type = process_type_t::external;
-    } else if (decoration == parse_statement_decoration_builtin) {
-        // What happens if this builtin is not valid?
-        process_type = process_type_t::builtin;
-    } else if (function_exists(cmd)) {
-        process_type = process_type_t::function;
-    } else if (builtin_exists(cmd)) {
-        process_type = process_type_t::builtin;
-    } else {
-        process_type = process_type_t::external;
+    switch (decoration) {
+        case parse_statement_decoration_exec:
+            process_type = process_type_t::exec;
+            break;
+        case parse_statement_decoration_command:
+            process_type = process_type_t::external;
+            break;
+        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;
+            } else if (builtin_exists(cmd)) {
+                process_type = process_type_t::builtin;
+            } else {
+                process_type = process_type_t::external;
+            }
+            break;
     }
+
     return process_type;
 }
 

src/parse_grammar.h

@@ -334,9 +334,16 @@ 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>;
-    // using evals = seq<keyword<parse_keyword_eval>, plain_statement>;
-    using evals = single<plain_statement>; /* so long as `eval` is a function */
-    ALT_BODY(decorated_statement, plains, cmds, builtins, execs);
+    // 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);
 };
 
 DEF(plain_statement)

src/proc.cpp

@@ -477,6 +477,9 @@ static bool process_clean_after_marking(bool allow_interactive) {
     static std::vector<shared_ptr<job_t>> erase_list;
     const bool only_one_job = jobs().size() == 1;
     for (const auto &j : jobs()) {
+        if (!j->is_constructed()) {
+            continue;
+        }
         // If we are reaping only jobs who do not need status messages sent to the console, do not
         // consider reaping jobs that need status messages.
         if ((!j->get_flag(job_flag_t::SKIP_NOTIFICATION)) && (!interactive) &&