Evaluate tnode_t instead of parse_node_t

This concerns block nodes with redirections, like
begin ... end | grep ...
Prior to this fix, we passed in a pointer to the node. Switch to passing
in the tnode and parsed source ref. This improves type safety and better
aligns with the function-node plans.

src/exec.cpp

@@ -306,13 +306,14 @@ static bool io_transmogrify(const io_chain_t &in_chain, io_chain_t *out_chain,
 /// clean up morphed redirections.
 ///
 /// \param def the code to evaluate, or the empty string if none
-/// \param node_offset the offset of the node to evalute, or NODE_OFFSET_INVALID
+/// \param statement the statement node to evaluate, or empty
 /// \param block_type the type of block to push on evaluation
 /// \param ios the io redirections to be performed on this block
-static void internal_exec_helper(parser_t &parser, const wcstring &def, node_offset_t node_offset,
+static void internal_exec_helper(parser_t &parser, const wcstring &def,
+                                 tnode_t<grammar::statement> statement,
                                  enum block_type_t block_type, const io_chain_t &ios) {
-    // If we have a valid node offset, then we must not have a string to execute.
-    assert(node_offset == NODE_OFFSET_INVALID || def.empty());
+    // If we have a valid statement node, then we must not have a string to execute.
+    assert(!statement || def.empty());
 
     io_chain_t morphed_chain;
     std::vector<int> opened_fds;
@@ -324,10 +325,10 @@ static void internal_exec_helper(parser_t &parser, const wcstring &def, node_off
         return;
     }
 
-    if (node_offset == NODE_OFFSET_INVALID) {
+    if (!statement) {
         parser.eval(def, morphed_chain, block_type);
     } else {
-        parser.eval_block_node(node_offset, morphed_chain, block_type);
+        parser.eval_node(statement, morphed_chain, block_type);
     }
 
     morphed_chain.clear();
@@ -810,8 +811,7 @@ void exec_job(parser_t &parser, job_t *j) {
                 verify_buffer_output();
 
                 if (!exec_error) {
-                    internal_exec_helper(parser, def, NODE_OFFSET_INVALID, TOP,
-                                         process_net_io_chain);
+                    internal_exec_helper(parser, def, {}, TOP, process_net_io_chain);
                 }
 
                 parser.allow_function();

src/parse_execution.cpp

@@ -975,25 +975,27 @@ parse_execution_result_t parse_execution_context_t::populate_boolean_process(
 }
 
 template <typename Type>
-parse_execution_result_t parse_execution_context_t::populate_block_process(job_t *job,
-                                                                           process_t *proc,
-                                                                           tnode_t<Type> node) {
+parse_execution_result_t parse_execution_context_t::populate_block_process(
+    job_t *job, process_t *proc, tnode_t<g::statement> statement,
+    tnode_t<Type> specific_statement) {
     // We handle block statements by creating INTERNAL_BLOCK_NODE, that will bounce back to us when
     // it's time to execute them.
     UNUSED(job);
     static_assert(Type::token == symbol_block_statement || Type::token == symbol_if_statement ||
                       Type::token == symbol_switch_statement,
                   "Invalid block process");
+    assert(statement && "statement missing");
+    assert(specific_statement && "specific_statement missing");
 
     // The set of IO redirections that we construct for the process.
     // TODO: fix this ugly find_child.
-    auto arguments = node.template find_child<g::arguments_or_redirections_list>();
+    auto arguments = specific_statement.template find_child<g::arguments_or_redirections_list>();
     io_chain_t process_io_chain;
     bool errored = !this->determine_io_chain(arguments, &process_io_chain);
     if (errored) return parse_execution_errored;
 
     proc->type = INTERNAL_BLOCK_NODE;
-    proc->internal_block_node = this->get_offset(node);
+    proc->internal_block_node = statement;
     proc->set_io_chain(process_io_chain);
     return parse_execution_success;
 }
@@ -1012,15 +1014,15 @@ parse_execution_result_t parse_execution_context_t::populate_job_process(
         }
         case symbol_block_statement:
             result = this->populate_block_process(
-                job, proc, tnode_t<g::block_statement>(&tree(), &specific_statement));
+                job, proc, statement, tnode_t<g::block_statement>(&tree(), &specific_statement));
             break;
         case symbol_if_statement:
             result = this->populate_block_process(
-                job, proc, tnode_t<g::if_statement>(&tree(), &specific_statement));
+                job, proc, statement, tnode_t<g::if_statement>(&tree(), &specific_statement));
             break;
         case symbol_switch_statement:
             result = this->populate_block_process(
-                job, proc, tnode_t<g::switch_statement>(&tree(), &specific_statement));
+                job, proc, statement, tnode_t<g::switch_statement>(&tree(), &specific_statement));
             break;
         case symbol_decorated_statement: {
             // Get the plain statement. It will pull out the decoration itself.
@@ -1127,7 +1129,7 @@ parse_execution_result_t parse_execution_context_t::run_1_job(tnode_t<g::job> jo
     }
 
     // When we encounter a block construct (e.g. while loop) in the general case, we create a "block
-    // process" that has a pointer to its source. This allows us to handle block-level redirections.
+    // process" containing its node. This allows us to handle block-level redirections.
     // However, if there are no redirections, then we can just jump into the block directly, which
     // is significantly faster.
     if (job_is_simple_block(job_node)) {
@@ -1257,63 +1259,48 @@ parse_execution_result_t parse_execution_context_t::run_job_list(tnode_t<Type> j
     return result;
 }
 
-parse_execution_result_t parse_execution_context_t::eval_node_at_offset(
-    node_offset_t offset, const block_t *associated_block, const io_chain_t &io) {
-    // Don't ever expect to have an empty tree if this is called.
-    assert(!tree().empty());  //!OCLINT(multiple unary operator)
-    assert(offset < tree().size());
-
+parse_execution_result_t parse_execution_context_t::eval_node(tnode_t<g::statement> statement,
+                                                              const block_t *associated_block,
+                                                              const io_chain_t &io) {
+    assert(statement && "Empty node in eval_node");
+    assert(statement.matches_node_tree(tree()) && "statement has unexpected tree");
     // Apply this block IO for the duration of this function.
     scoped_push<io_chain_t> block_io_push(&block_io, io);
-
-    const parse_node_t &node = tree().at(offset);
-
-    // Currently, we only expect to execute the top level job list, or a block node. Assert that.
-    assert(node.type == symbol_job_list || specific_statement_type_is_redirectable_block(node));
-
     enum parse_execution_result_t status = parse_execution_success;
-    switch (node.type) {
-        case symbol_job_list: {
-            // We should only get a job list if it's the very first node. This is because this is
-            // the entry point for both top-level execution (the first node) and INTERNAL_BLOCK_NODE
-            // execution (which does block statements, but never job lists).
-            assert(offset == 0);
-            tnode_t<g::job_list> job_list{&tree(), &node};
-            wcstring func_name;
-            auto infinite_recursive_node =
-                this->infinite_recursive_statement_in_job_list(job_list, &func_name);
-            if (infinite_recursive_node) {
-                // We have an infinite recursion.
-                this->report_error(infinite_recursive_node, INFINITE_FUNC_RECURSION_ERR_MSG,
-                                   func_name.c_str());
-                status = parse_execution_errored;
-            } else {
-                // No infinite recursion.
-                status = this->run_job_list(job_list, associated_block);
-            }
-            break;
-        }
-        case symbol_block_statement: {
-            status = this->run_block_statement({&tree(), &node});
-            break;
-        }
-        case symbol_if_statement: {
-            status = this->run_if_statement({&tree(), &node});
-            break;
-        }
-        case symbol_switch_statement: {
-            status = this->run_switch_statement({&tree(), &node});
-            break;
-        }
-        default: {
-            // In principle, we could support other node types. However we never expect to be passed
-            // them - see above.
-            debug(0, "Unexpected node %ls found in %s", node.describe().c_str(), __FUNCTION__);
-            PARSER_DIE();
-            break;
-        }
+    if (auto block = statement.try_get_child<g::block_statement, 0>()) {
+        status = this->run_block_statement(block);
+    } else if (auto ifstat = statement.try_get_child<g::if_statement, 0>()) {
+        status = this->run_if_statement(ifstat);
+    } else if (auto switchstat = statement.try_get_child<g::switch_statement, 0>()) {
+        status = this->run_switch_statement(switchstat);
+    } else {
+        debug(0, "Unexpected node %ls found in %s", statement.node()->describe().c_str(),
+              __FUNCTION__);
+        abort();
     }
+    return status;
+}
 
+parse_execution_result_t parse_execution_context_t::eval_node(tnode_t<g::job_list> job_list,
+                                                              const block_t *associated_block,
+                                                              const io_chain_t &io) {
+    // Apply this block IO for the duration of this function.
+    assert(job_list && "Empty node in eval_node");
+    assert(job_list.matches_node_tree(tree()) && "job_list has unexpected tree");
+    scoped_push<io_chain_t> block_io_push(&block_io, io);
+    enum parse_execution_result_t status = parse_execution_success;
+    wcstring func_name;
+    auto infinite_recursive_node =
+        this->infinite_recursive_statement_in_job_list(job_list, &func_name);
+    if (infinite_recursive_node) {
+        // We have an infinite recursion.
+        this->report_error(infinite_recursive_node, INFINITE_FUNC_RECURSION_ERR_MSG,
+                           func_name.c_str());
+        status = parse_execution_errored;
+    } else {
+        // No infinite recursion.
+        status = this->run_job_list(job_list, associated_block);
+    }
     return status;
 }
 

src/parse_execution.h

@@ -91,7 +91,8 @@ class parse_execution_context_t {
 
     template <typename Type>
     parse_execution_result_t populate_block_process(job_t *job, process_t *proc,
-                                                    tnode_t<Type> statement_node);
+                                                    tnode_t<grammar::statement> statement,
+                                                    tnode_t<Type> specific_statement);
 
     // These encapsulate the actual logic of various (block) statements.
     parse_execution_result_t run_block_statement(tnode_t<grammar::block_statement> statement);
@@ -146,11 +147,12 @@ class parse_execution_context_t {
     /// Return the parse tree.
     const parse_node_tree_t &tree() const { return pstree->tree; }
 
-    /// Start executing at the given node offset. Returns 0 if there was no error, 1 if there was an
+    /// Start executing at the given node. Returns 0 if there was no error, 1 if there was an
     /// error.
-    parse_execution_result_t eval_node_at_offset(node_offset_t offset,
-                                                 const block_t *associated_block,
-                                                 const io_chain_t &io);
+    parse_execution_result_t eval_node(tnode_t<grammar::statement> statement,
+                                       const block_t *associated_block, const io_chain_t &io);
+    parse_execution_result_t eval_node(tnode_t<grammar::job_list> job_list,
+                                       const block_t *associated_block, const io_chain_t &io);
 };
 
 #endif

src/parser.cpp

@@ -670,7 +670,8 @@ int parser_t::eval(parsed_source_ref_t ps, const io_chain_t &io, enum block_type
     const parse_execution_context_t *ctx = execution_contexts.back().get();
 
     // Execute the first node.
-    this->eval_block_node(0, io, block_type);
+    tnode_t<grammar::job_list> start{&ps->tree, &ps->tree.front()};
+    this->eval_node(start, io, block_type);
 
     // Clean up the execution context stack.
     assert(!execution_contexts.empty() && execution_contexts.back().get() == ctx);
@@ -679,12 +680,11 @@ int parser_t::eval(parsed_source_ref_t ps, const io_chain_t &io, enum block_type
     return 0;
 }
 
-int parser_t::eval_block_node(node_offset_t node_idx, const io_chain_t &io,
-                              enum block_type_t block_type) {
-    // Paranoia. It's a little frightening that we're given only a node_idx and we interpret this in
-    // the topmost execution context's tree. What happens if two trees were to be interleaved?
-    // Fortunately that cannot happen (yet); in the future we probably want some sort of reference
-    // counted trees.
+template <typename T>
+int parser_t::eval_node(tnode_t<T> node, const io_chain_t &io, enum block_type_t block_type) {
+    static_assert(
+        std::is_same<T, grammar::statement>::value || std::is_same<T, grammar::job_list>::value,
+        "Unexpected node type");
     parse_execution_context_t *ctx = execution_contexts.back().get();
     assert(ctx != NULL);
 
@@ -711,13 +711,17 @@ int parser_t::eval_block_node(node_offset_t node_idx, const io_chain_t &io,
 
     // Start it up
     scope_block_t *scope_block = this->push_block<scope_block_t>(block_type);
-    int result = ctx->eval_node_at_offset(node_idx, scope_block, io);
+    int result = ctx->eval_node(node, scope_block, io);
     this->pop_block(scope_block);
 
     job_reap(0);  // reap again
     return result;
 }
 
+// Explicit instantiations. TODO: use overloads instead?
+template int parser_t::eval_node(tnode_t<grammar::statement>, const io_chain_t &io, enum block_type_t block_type);
+template int parser_t::eval_node(tnode_t<grammar::job_list>, const io_chain_t &io, enum block_type_t block_type);
+
 bool parser_t::detect_errors_in_argument_list(const wcstring &arg_list_src, wcstring *out,
                                               const wchar_t *prefix) {
     bool errored = false;

src/parser.h

@@ -251,8 +251,10 @@ class parser_t {
     /// Evaluate the parsed source ps.
     int eval(parsed_source_ref_t ps, const io_chain_t &io, enum block_type_t block_type);
 
-    /// Evaluates a block node at the given node offset in the topmost execution context.
-    int eval_block_node(node_offset_t node_idx, const io_chain_t &io, enum block_type_t block_type);
+    /// Evaluates a node in the topmost execution context.
+    /// The node type must be grammar::statement or grammar::job_list.
+    template <typename T>
+    int eval_node(tnode_t<T> node, const io_chain_t &io, enum block_type_t block_type);
 
     /// Evaluate line as a list of parameters, i.e. tokenize it and perform parameter expansion and
     /// cmdsubst execution on the tokens. The output is inserted into output. Errors are ignored.

src/proc.cpp

@@ -344,25 +344,7 @@ static void handle_child_status(pid_t pid, int status) {
     return;
 }
 
-process_t::process_t()
-    : is_first_in_job(),
-      is_last_in_job(),
-      type(),  // gets set later
-      internal_block_node(NODE_OFFSET_INVALID),
-      pid(0),
-      pipe_write_fd(0),
-      pipe_read_fd(0),
-      completed(0),
-      stopped(0),
-      status(0),
-      count_help_magic(0)
-#ifdef HAVE__PROC_SELF_STAT
-      ,
-      last_time(),
-      last_jiffies(0)
-#endif
-{
-}
+process_t::process_t() {}
 
 /// The constructor sets the pgid to -2 as a sentinel value
 /// 0 should not be used; although it is not a valid PGID in userspace,

src/proc.h

@@ -18,6 +18,7 @@
 #include "common.h"
 #include "io.h"
 #include "parse_tree.h"
+#include "tnode.h"
 
 /// Types of processes.
 enum process_type_t {
@@ -73,15 +74,16 @@ class process_t {
     process_t();
 
     // Note whether we are the first and/or last in the job
-    bool is_first_in_job;
-    bool is_last_in_job;
+    bool is_first_in_job{false};
+    bool is_last_in_job{false};
 
     /// Type of process. Can be one of \c EXTERNAL, \c INTERNAL_BUILTIN, \c INTERNAL_FUNCTION, \c
     /// INTERNAL_EXEC.
-    enum process_type_t type;
+    enum process_type_t type { EXTERNAL };
 
-    /// For internal block processes only, the node offset of the block.
-    node_offset_t internal_block_node;
+    /// For internal block processes only, the node offset of the statement.
+    /// This is always either block, ifs, or switchs, never boolean or decorated.
+    tnode_t<grammar::statement> internal_block_node{};
 
     /// Sets argv.
     void set_argv(const wcstring_list_t &argv) { argv_array.set(argv); }
@@ -111,24 +113,24 @@ class process_t {
     /// Actual command to pass to exec in case of EXTERNAL or INTERNAL_EXEC.
     wcstring actual_cmd;
     /// Process ID
-    pid_t pid;
+    pid_t pid{0};
     /// File descriptor that pipe output should bind to.
-    int pipe_write_fd;
+    int pipe_write_fd{0};
     /// File descriptor that the _next_ process pipe input should bind to.
-    int pipe_read_fd;
+    int pipe_read_fd{0};
     /// True if process has completed.
-    volatile int completed;
+    volatile int completed{false};
     /// True if process has stopped.
-    volatile int stopped;
+    volatile int stopped{false};
     /// Reported status value.
-    volatile int status;
+    volatile int status{0};
     /// Special flag to tell the evaluation function for count to print the help information.
-    int count_help_magic;
+    int count_help_magic{0};
 #ifdef HAVE__PROC_SELF_STAT
     /// Last time of cpu time check.
-    struct timeval last_time;
+    struct timeval last_time {};
     /// Number of jiffies spent in process at last cpu time check.
-    unsigned long last_jiffies;
+    unsigned long last_jiffies{0};
 #endif
 };
 

src/tnode.h

@@ -88,6 +88,9 @@ class tnode_t {
 
     bool operator!=(const tnode_t &rhs) const { return !(*this == rhs); }
 
+    // Helper to return whether the given tree is the same as ours.
+    bool matches_node_tree(const parse_node_tree_t &t) const { return &t == tree; }
+
     bool has_source() const { return nodeptr && nodeptr->has_source(); }
 
     // return the tag, or 0 if missing.