Clean up the return type of parser_t::eval

parser_t::eval indicates whether there was a parse error. It can be
easily confused with the status of the execution. Use a real type to
make it more clear.

src/builtin_eval.cpp

@@ -27,8 +27,8 @@ int builtin_eval(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
     const auto cached_exec_count = parser.libdata().exec_count;
     int status = STATUS_CMD_OK;
     if (argc > 1) {
-        if (parser.eval(std::move(new_cmd), *streams.io_chain, block_type_t::TOP) != 0) {
-            // This indicates a parse error; nothing actually got executed.
+        if (parser.eval(std::move(new_cmd), *streams.io_chain, block_type_t::TOP) !=
+            eval_result_t::ok) {
             status = STATUS_CMD_ERROR;
         } else if (cached_exec_count == parser.libdata().exec_count) {
             // Issue #5692, in particular, to catch `eval ""`, `eval "begin; end;"`, etc.

src/exec.cpp

@@ -1183,7 +1183,7 @@ static int exec_subshell_internal(const wcstring &cmd, parser_t &parser, wcstrin
     // be null.
     std::shared_ptr<io_buffer_t> buffer;
     if (auto bufferfill = io_bufferfill_t::create(fd_set_t{}, ld.read_limit)) {
-        if (parser.eval(cmd, io_chain_t{bufferfill}, SUBST) == 0) {
+        if (parser.eval(cmd, io_chain_t{bufferfill}, SUBST) == eval_result_t::ok) {
             subcommand_statuses = parser.get_last_statuses();
         }
         buffer = io_bufferfill_t::finish(std::move(bufferfill));

src/fish.cpp

@@ -254,7 +254,8 @@ int run_command_list(std::vector<std::string> *cmds, const io_chain_t &io) {
 
     for (const auto &cmd : *cmds) {
         const wcstring cmd_wcs = str2wcstring(cmd);
-        res = parser.eval(cmd_wcs, io, TOP);
+        eval_result_t eval_res = parser.eval(cmd_wcs, io, TOP);
+        res = (eval_res == eval_result_t::ok ? 0 : 1);
     }
 
     return res;

src/fish_tests.cpp

@@ -5202,13 +5202,11 @@ static void test_illegal_command_exit_code() {
         {L"abc?def", STATUS_UNMATCHED_WILDCARD},
     };
 
-    int res = 0;
-    UNUSED(res);
     const io_chain_t empty_ios;
     parser_t &parser = parser_t::principal_parser();
 
     for (const auto &test : tests) {
-        res = parser.eval(test.txt, empty_ios, TOP);
+        parser.eval(test.txt, empty_ios, TOP);
 
         int exit_status = parser.get_last_status();
         if (exit_status != test.result) {

src/parser.cpp

@@ -616,23 +616,25 @@ profile_item_t *parser_t::create_profile_item() {
     return result;
 }
 
-int parser_t::eval(wcstring cmd, const io_chain_t &io, enum block_type_t block_type) {
+eval_result_t parser_t::eval(const wcstring &cmd, const io_chain_t &io,
+                             enum block_type_t block_type) {
     // Parse the source into a tree, if we can.
     parse_error_list_t error_list;
-    parsed_source_ref_t ps = parse_source(cmd, parse_flag_none, &error_list);
-    if (!ps) {
+    if (parsed_source_ref_t ps = parse_source(cmd, parse_flag_none, &error_list)) {
+        return this->eval(ps, io, block_type);
+    } else {
         // Get a backtrace. This includes the message.
         wcstring backtrace_and_desc;
         this->get_backtrace(cmd, error_list, backtrace_and_desc);
 
         // Print it.
         std::fwprintf(stderr, L"%ls\n", backtrace_and_desc.c_str());
-        return 1;
+        return eval_result_t::error;
     }
-    return this->eval(ps, io, block_type);
 }
 
-int parser_t::eval(parsed_source_ref_t ps, const io_chain_t &io, enum block_type_t block_type) {
+eval_result_t parser_t::eval(parsed_source_ref_t ps, const io_chain_t &io,
+                             enum block_type_t block_type) {
     assert(block_type == TOP || block_type == SUBST);
     if (!ps->tree.empty()) {
         job_lineage_t lineage;
@@ -641,12 +643,12 @@ int parser_t::eval(parsed_source_ref_t ps, const io_chain_t &io, enum block_type
         tnode_t<grammar::job_list> start{&ps->tree, &ps->tree.front()};
         return this->eval_node(ps, start, block_type, std::move(lineage));
     }
-    return 0;
+    return eval_result_t::ok;
 }
 
 template <typename T>
-int parser_t::eval_node(parsed_source_ref_t ps, tnode_t<T> node, block_type_t block_type,
-                        job_lineage_t lineage) {
+eval_result_t parser_t::eval_node(parsed_source_ref_t ps, tnode_t<T> node, block_type_t block_type,
+                                  job_lineage_t lineage) {
     static_assert(
         std::is_same<T, grammar::statement>::value || std::is_same<T, grammar::job_list>::value,
         "Unexpected node type");
@@ -655,17 +657,13 @@ int parser_t::eval_node(parsed_source_ref_t ps, tnode_t<T> node, block_type_t bl
     // not empty, we are still in the process of cancelling; refuse to evaluate anything.
     if (this->cancellation_requested) {
         if (!block_stack.empty()) {
-            return 1;
+            return eval_result_t::cancelled;
         }
         this->cancellation_requested = false;
     }
 
     // Only certain blocks are allowed.
-    if ((block_type != TOP) && (block_type != SUBST)) {
-        debug(1, INVALID_SCOPE_ERR_MSG, parser_t::get_block_desc(block_type));
-        bugreport();
-        return 1;
-    }
+    assert((block_type == TOP || block_type == SUBST) && "Invalid block type");
 
     job_reap(*this, false);  // not sure why we reap jobs here
 
@@ -676,19 +674,28 @@ int parser_t::eval_node(parsed_source_ref_t ps, tnode_t<T> node, block_type_t bl
     using exc_ctx_ref_t = std::unique_ptr<parse_execution_context_t>;
     scoped_push<exc_ctx_ref_t> exc(
         &execution_context, make_unique<parse_execution_context_t>(ps, this, std::move(lineage)));
-    int result = execution_context->eval_node(node, scope_block);
+    parse_execution_result_t res = execution_context->eval_node(node, scope_block);
     exc.restore();
     this->pop_block(scope_block);
 
     job_reap(*this, false);  // reap again
-    return result;
+    switch (res) {
+        case parse_execution_success:
+            return eval_result_t::ok;
+        case parse_execution_errored:
+            return eval_result_t::error;
+        case parse_execution_cancelled:
+            return eval_result_t::cancelled;
+        case parse_execution_skipped:
+            DIE("skipped should not be returned from run functions");
+    }
 }
 
 // Explicit instantiations. TODO: use overloads instead?
-template int parser_t::eval_node(parsed_source_ref_t, tnode_t<grammar::statement>,
-                                 enum block_type_t, job_lineage_t lineage);
-template int parser_t::eval_node(parsed_source_ref_t, tnode_t<grammar::job_list>, enum block_type_t,
-                                 job_lineage_t lineage);
+template eval_result_t parser_t::eval_node(parsed_source_ref_t, tnode_t<grammar::statement>,
+                                           enum block_type_t, job_lineage_t lineage);
+template eval_result_t parser_t::eval_node(parsed_source_ref_t, tnode_t<grammar::job_list>,
+                                           enum block_type_t, job_lineage_t lineage);
 
 void parser_t::get_backtrace(const wcstring &src, const parse_error_list_t &errors,
                              wcstring &output) const {

src/parser.h

@@ -53,6 +53,18 @@ enum class loop_status_t {
     continues,  /// current loop block should be skipped
 };
 
+/// Result of the source code form of eval.
+enum class eval_result_t {
+    /// eval was able to evaluate the source or tree.
+    ok,
+
+    /// Evaluation was cancelled, e.g. because of a signal.
+    cancelled,
+
+    /// Parse or execution error (but not simply a failed command).
+    error,
+};
+
 /// block_t represents a block of commands.
 class block_t {
     /// Construct from a block type.
@@ -258,18 +270,18 @@ class parser_t : public std::enable_shared_from_this<parser_t> {
     /// \param io io redirections to perform on all started jobs
     /// \param block_type The type of block to push on the block stack
     ///
-    /// \return 0 on success, 1 on a parse error.
-    int eval(wcstring cmd, const io_chain_t &io, enum block_type_t block_type);
+    /// \return the eval result,
+    eval_result_t eval(const wcstring &cmd, const io_chain_t &io, enum block_type_t block_type);
 
     /// Evaluate the parsed source ps.
-    /// \return 0 on success, 1 on a parse error.
-    int eval(parsed_source_ref_t ps, const io_chain_t &io, enum block_type_t block_type);
+    /// Because the source has been parsed, a syntax error is impossible.
+    eval_result_t eval(parsed_source_ref_t ps, const io_chain_t &io, enum block_type_t block_type);
 
     /// Evaluates a node.
     /// The node type must be grammar::statement or grammar::job_list.
     template <typename T>
-    int eval_node(parsed_source_ref_t ps, tnode_t<T> node, block_type_t block_type,
-                  job_lineage_t lineage);
+    eval_result_t eval_node(parsed_source_ref_t ps, tnode_t<T> node, block_type_t block_type,
+                            job_lineage_t lineage);
 
     /// Evaluate line as a list of parameters, i.e. tokenize it and perform parameter expansion and
     /// cmdsubst execution on the tokens. Errors are ignored. If a parser is provided, it is used