Shell: Make the parser read consecutive sequences without recursing

This fixes (the easy) part of #4976.

Userland/Libraries/LibGUI/ShellSyntaxHighlighter.cpp

@@ -380,21 +380,19 @@ private:
     }
     virtual void visit(const AST::Sequence* node) override
     {
-        {
+        for (auto& entry : node->entries()) {
             ScopedValueRollback first_in_command { m_is_first_in_command };
-            node->left()->visit(*this);
+            entry.visit(*this);
-        }
-        {
-            ScopedValueRollback first_in_command { m_is_first_in_command };
-            node->right()->visit(*this);
         }
 
-        auto& span = span_for_node(node);
+        for (auto& position : node->separator_positions()) {
-        span.range.set_start({ node->separator_position().start_line.line_number, node->separator_position().start_line.line_column });
+            auto& span = span_for_node(node);
-        set_offset_range_end(span.range, node->separator_position().end_line);
+            span.range.set_start({ position.start_line.line_number, position.start_line.line_column });
-        span.attributes.color = m_palette.syntax_punctuation();
+            set_offset_range_end(span.range, position.end_line);
-        span.attributes.bold = true;
+            span.attributes.color = m_palette.syntax_punctuation();
-        span.is_skippable = true;
+            span.attributes.bold = true;
+            span.is_skippable = true;
+        }
     }
     virtual void visit(const AST::Subshell* node) override
     {

Userland/Shell/AST.cpp

@@ -2302,32 +2302,37 @@ ReadWriteRedirection::~ReadWriteRedirection()
 void Sequence::dump(int level) const
 {
     Node::dump(level);
-    m_left->dump(level + 1);
+    for (auto& entry : m_entries)
-    m_right->dump(level + 1);
+        entry.dump(level + 1);
 }
 
 RefPtr<Value> Sequence::run(RefPtr<Shell> shell)
 {
-    auto left = m_left->to_lazy_evaluated_commands(shell);
+    Vector<Command> all_commands;
-    // This could happen if a comment is next to a command.
+    Command* last_command_in_sequence = nullptr;
-    if (left.size() == 1) {
+    for (auto& entry : m_entries) {
-        auto& command = left.first();
+        if (!last_command_in_sequence) {
-        if (command.argv.is_empty() && command.redirections.is_empty() && command.next_chain.is_empty())
+            auto commands = entry.to_lazy_evaluated_commands(shell);
-            return m_right->run(shell);
+            all_commands.append(move(commands));
+            last_command_in_sequence = &all_commands.last();
+            continue;
+        }
+
+        if (last_command_in_sequence->should_wait) {
+            last_command_in_sequence->next_chain.append(NodeWithAction { entry, NodeWithAction::Sequence });
+        } else {
+            all_commands.append(entry.to_lazy_evaluated_commands(shell));
+            last_command_in_sequence = &all_commands.last();
+        }
     }
 
-    if (left.last().should_wait)
+    return create<CommandSequenceValue>(move(all_commands));
-        left.last().next_chain.append(NodeWithAction { *m_right, NodeWithAction::Sequence });
-    else
-        left.append(m_right->to_lazy_evaluated_commands(shell));
-
-    return create<CommandSequenceValue>(move(left));
 }
 
 void Sequence::highlight_in_editor(Line::Editor& editor, Shell& shell, HighlightMetadata metadata)
 {
-    m_left->highlight_in_editor(editor, shell, metadata);
+    for (auto& entry : m_entries)
-    m_right->highlight_in_editor(editor, shell, metadata);
+        entry.highlight_in_editor(editor, shell, metadata);
 }
 
 HitTestResult Sequence::hit_test_position(size_t offset)
@@ -2335,29 +2340,29 @@ HitTestResult Sequence::hit_test_position(size_t offset)
     if (!position().contains(offset))
         return {};
 
-    auto result = m_left->hit_test_position(offset);
+    for (auto& entry : m_entries) {
-    if (result.matching_node) {
+        auto result = entry.hit_test_position(offset);
-        if (!result.closest_command_node)
+        if (result.matching_node) {
-            result.closest_command_node = m_right;
+            if (!result.closest_command_node)
-        return result;
+                result.closest_command_node = entry;
+            return result;
+        }
     }
 
-    result = m_right->hit_test_position(offset);
+    return {};
-    if (!result.closest_command_node)
-        result.closest_command_node = m_right;
-    return result;
 }
 
-Sequence::Sequence(Position position, NonnullRefPtr<Node> left, NonnullRefPtr<Node> right, Position separator_position)
+Sequence::Sequence(Position position, NonnullRefPtrVector<Node> entries, Vector<Position> separator_positions)
     : Node(move(position))
-    , m_left(move(left))
+    , m_entries(move(entries))
-    , m_right(move(right))
+    , m_separator_positions(separator_positions)
-    , m_separator_position(separator_position)
 {
-    if (m_left->is_syntax_error())
+    for (auto& entry : m_entries) {
-        set_is_syntax_error(m_left->syntax_error_node());
+        if (entry.is_syntax_error()) {
-    else if (m_right->is_syntax_error())
+            set_is_syntax_error(entry.syntax_error_node());
-        set_is_syntax_error(m_right->syntax_error_node());
+            break;
+        }
+    }
 }
 
 Sequence::~Sequence()

Userland/Shell/AST.h

@@ -1139,14 +1139,13 @@ private:
 
 class Sequence final : public Node {
 public:
-    Sequence(Position, NonnullRefPtr<Node>, NonnullRefPtr<Node>, Position separator_position);
+    Sequence(Position, NonnullRefPtrVector<Node>, Vector<Position> separator_positions);
     virtual ~Sequence();
     virtual void visit(NodeVisitor& visitor) override { visitor.visit(this); }
 
-    const NonnullRefPtr<Node>& left() const { return m_left; }
+    const NonnullRefPtrVector<Node>& entries() const { return m_entries; }
-    const NonnullRefPtr<Node>& right() const { return m_right; }
 
-    const Position& separator_position() const { return m_separator_position; }
+    const Vector<Position>& separator_positions() const { return m_separator_positions; }
 
 private:
     NODE(Sequence);
@@ -1157,9 +1156,8 @@ private:
     virtual bool is_list() const override { return true; }
     virtual bool should_override_execution_in_current_process() const override { return true; }
 
-    NonnullRefPtr<Node> m_left;
+    NonnullRefPtrVector<Node> m_entries;
-    NonnullRefPtr<Node> m_right;
+    Vector<Position> m_separator_positions;
-    Position m_separator_position;
 };
 
 class Subshell final : public Node {

Userland/Shell/Formatter.cpp

@@ -631,10 +631,17 @@ void Formatter::visit(const AST::Sequence* node)
     test_and_update_output_cursor(node);
 
     TemporaryChange<const AST::Node*> parent { m_parent_node, node };
-    node->left()->visit(*this);
-    insert_separator();
 
-    node->right()->visit(*this);
+    bool first = true;
+    for (auto& entry : node->entries()) {
+        if (first)
+            first = false;
+        else
+            insert_separator();
+
+        entry.visit(*this);
+    }
+
     visited(node);
 }
 

Userland/Shell/NodeVisitor.cpp

@@ -186,8 +186,8 @@ void NodeVisitor::visit(const AST::ReadWriteRedirection* node)
 
 void NodeVisitor::visit(const AST::Sequence* node)
 {
-    node->left()->visit(*this);
+    for (auto& entry : node->entries())
-    node->right()->visit(*this);
+        entry.visit(*this);
 }
 
 void NodeVisitor::visit(const AST::Subshell* node)

Userland/Shell/Parser.cpp

@@ -111,6 +111,11 @@ NonnullRefPtr<A> Parser::create(Args... args)
     return make<ScopedOffset>(m_rule_start_offsets, m_rule_start_lines, m_offset, m_line.line_number, m_line.line_column);
 }
 
+Parser::Offset Parser::current_position()
+{
+    return Offset { m_offset, { m_line.line_number, m_line.line_column } };
+}
+
 static constexpr bool is_whitespace(char c)
 {
     return c == ' ' || c == '\t';
@@ -180,58 +185,70 @@ RefPtr<AST::Node> Parser::parse_toplevel()
 {
     auto rule_start = push_start();
 
-    if (auto sequence = parse_sequence())
+    SequenceParseResult result;
-        return create<AST::Execute>(sequence.release_nonnull());
+    NonnullRefPtrVector<AST::Node> sequence;
+    Vector<AST::Position> positions;
+    do {
+        result = parse_sequence();
+        if (result.entries.is_empty())
+            break;
 
-    return nullptr;
+        sequence.append(move(result.entries));
+        positions.append(move(result.separator_positions));
+    } while (result.decision == ShouldReadMoreSequences::Yes);
+
+    if (sequence.is_empty())
+        return nullptr;
+
+    return create<AST::Execute>(
+        create<AST::Sequence>(move(sequence), move(positions)));
 }
 
-RefPtr<AST::Node> Parser::parse_sequence()
+Parser::SequenceParseResult Parser::parse_sequence()
 {
     consume_while(is_any_of(" \t\n;")); // ignore whitespaces or terminators without effect.
 
+    NonnullRefPtrVector<AST::Node> left;
+
     auto rule_start = push_start();
-    auto var_decls = parse_variable_decls();
+    {
+        auto var_decls = parse_variable_decls();
+        if (var_decls)
+            left.append(var_decls.release_nonnull());
+    }
 
     auto pos_before_seps = save_offset();
 
     switch (peek()) {
     case '}':
-        return var_decls;
+        return { move(left), {}, ShouldReadMoreSequences::No };
     case ';':
     case '\n': {
-        if (!var_decls)
+        if (left.is_empty())
             break;
 
         consume_while(is_any_of("\n;"));
-
         auto pos_after_seps = save_offset();
+        AST::Position separator_position { pos_before_seps.offset, pos_after_seps.offset, pos_before_seps.line, pos_after_seps.line };
 
-        auto rest = parse_sequence();
+        return { move(left), { move(separator_position) }, ShouldReadMoreSequences::Yes };
-        if (rest)
-            return create<AST::Sequence>(
-                var_decls.release_nonnull(),
-                rest.release_nonnull(),
-                AST::Position { pos_before_seps.offset, pos_after_seps.offset, pos_before_seps.line, pos_after_seps.line });
-        return var_decls;
     }
     default:
         break;
     }
 
-    auto first = parse_function_decl();
+    auto first_entry = parse_function_decl();
 
-    if (!first)
+    Vector<AST::Position> separator_positions;
-        first = parse_or_logical_sequence();
 
-    if (!first)
+    if (!first_entry)
-        return var_decls;
+        first_entry = parse_or_logical_sequence();
 
-    if (var_decls)
+    if (!first_entry)
-        first = create<AST::Sequence>(
+        return { move(left), {}, ShouldReadMoreSequences::No };
-            var_decls.release_nonnull(),
+
-            first.release_nonnull(),
+    left.append(first_entry.release_nonnull());
-            AST::Position { pos_before_seps.offset, pos_before_seps.offset, pos_before_seps.line, pos_before_seps.line });
+    separator_positions.empend(pos_before_seps.offset, pos_before_seps.offset, pos_before_seps.line, pos_before_seps.line);
 
     consume_while(is_whitespace);
 
@@ -241,29 +258,19 @@ RefPtr<AST::Node> Parser::parse_sequence()
     case '\n': {
         consume_while(is_any_of("\n;"));
         auto pos_after_seps = save_offset();
-
+        separator_positions.empend(pos_before_seps.offset, pos_after_seps.offset, pos_before_seps.line, pos_after_seps.line);
-        if (auto expr = parse_sequence()) {
+        return { move(left), move(separator_positions), ShouldReadMoreSequences::Yes };
-            return create<AST::Sequence>(
-                first.release_nonnull(),
-                expr.release_nonnull(),
-                AST::Position { pos_before_seps.offset, pos_after_seps.offset, pos_before_seps.line, pos_after_seps.line }); // Sequence
-        }
-        return first;
     }
     case '&': {
         consume();
         auto pos_after_seps = save_offset();
-        auto bg = create<AST::Background>(first.release_nonnull()); // Execute Background
+        auto bg = create<AST::Background>(left.take_last()); // Execute Background
-        if (auto rest = parse_sequence())
+        left.append(move(bg));
-            return create<AST::Sequence>(
+        separator_positions.empend(pos_before_seps.offset, pos_after_seps.offset, pos_before_seps.line, pos_after_seps.line);
-                move(bg),
+        return { move(left), move(separator_positions), ShouldReadMoreSequences::Yes };
-                rest.release_nonnull(),
-                AST::Position { pos_before_seps.offset, pos_after_seps.offset, pos_before_seps.line, pos_before_seps.line }); // Sequence Background Sequence
-
-        return bg;
     }
     default:
-        return first;
+        return { move(left), move(separator_positions), ShouldReadMoreSequences::No };
     }
 }
 

Userland/Shell/Parser.h

@@ -55,9 +55,19 @@ public:
     SavedOffset save_offset() const;
 
 private:
+    enum class ShouldReadMoreSequences {
+        Yes,
+        No,
+    };
+    struct SequenceParseResult {
+        NonnullRefPtrVector<AST::Node> entries;
+        Vector<AST::Position, 1> separator_positions;
+        ShouldReadMoreSequences decision;
+    };
+
     constexpr static size_t max_allowed_nested_rule_depth = 2048;
     RefPtr<AST::Node> parse_toplevel();
-    RefPtr<AST::Node> parse_sequence();
+    SequenceParseResult parse_sequence();
     RefPtr<AST::Node> parse_function_decl();
     RefPtr<AST::Node> parse_and_logical_sequence();
     RefPtr<AST::Node> parse_or_logical_sequence();
@@ -108,6 +118,10 @@ private:
 
     StringView consume_while(Function<bool(char)>);
 
+    struct Offset {
+        size_t offset;
+        AST::Position::Line line;
+    };
     struct ScopedOffset {
         ScopedOffset(Vector<size_t>& offsets, Vector<AST::Position::Line>& lines, size_t offset, size_t lineno, size_t linecol)
             : offsets(offsets)
@@ -136,6 +150,7 @@ private:
     void restore_to(const ScopedOffset& offset) { restore_to(offset.offset, offset.line); }
 
     OwnPtr<ScopedOffset> push_start();
+    Offset current_position();
 
     StringView m_input;
     size_t m_offset { 0 };