Rewrite REPL test parser

- Use a recursive descent parser so that it's easy to extend.
- Add `@args` to enable customizing command-line arguments
- Add `@should-start` to enable `nix repl` tests that error before
  entering the REPL
- Make sure to read all stdout output before comparing. This catches
  some extra output we were tossing out before!

Change-Id: I5522555df4c313024ab15cd10f9f04e7293bda3a

11 files changed, 278 insertions, 100 deletions

diff --git a/tests/functional/repl_characterization/data/basic.ast b/tests/functional/repl_characterization/data/basic.ast
index bdb445c9a..e079af588 100644
--- a/tests/functional/repl_characterization/data/basic.ast
+++ b/tests/functional/repl_characterization/data/basic.ast
@@ -1,16 +1,27 @@
-Commentary "meow meow meow"
-Command "command"
-Output "output output one"
-Output ""
-Output ""
-Output "output output two"
-Commentary "meow meow"
-Command "command two"
-Output "output output output"
-Commentary "commentary"
-Output "output output output"
-Output ""
-Commentary "the blank below should be chomped"
-Command "command three"
-Commentary ""
-Output "meow output"
+Commentary: "meow meow meow"
+Indent: "  "
+Prompt: "nix-repl> "
+Command: "command"
+Indent: "  "
+Output: "output output one"
+Output: ""
+Commentary: ""
+Indent: "  "
+Output: "output output two"
+Commentary: "meow meow"
+Indent: "  "
+Prompt: "nix-repl> "
+Command: "command two"
+Indent: "  "
+Output: "output output output"
+Commentary: "commentary"
+Indent: "  "
+Output: "output output output"
+Output: ""
+Commentary: "the blank below should be chomped"
+Indent: "  "
+Prompt: "nix-repl> "
+Command: "command three"
+Commentary: ""
+Indent: "  "
+Output: "meow output"
diff --git a/tests/functional/repl_characterization/data/basic_tidied.ast b/tests/functional/repl_characterization/data/basic_tidied.ast
index 878065a5c..d41d21a96 100644
--- a/tests/functional/repl_characterization/data/basic_tidied.ast
+++ b/tests/functional/repl_characterization/data/basic_tidied.ast
@@ -1,10 +1,9 @@
-Command "command"
-Output "output output one"
-Output ""
-Output ""
-Output "output output two"
-Command "command two"
-Output "output output output"
-Output "output output output"
-Command "command three"
-Output "meow output"
+Command: "command"
+Output: "output output one"
+Output: ""
+Output: "output output two"
+Command: "command two"
+Output: "output output output"
+Output: "output output output"
+Command: "command three"
+Output: "meow output"
diff --git a/tests/functional/repl_characterization/data/basic_tidied.test b/tests/functional/repl_characterization/data/basic_tidied.test
new file mode 100644
index 000000000..2c56c489b
--- /dev/null
+++ b/tests/functional/repl_characterization/data/basic_tidied.test
@@ -0,0 +1,10 @@
+command
+output output one
+
+output output two
+command two
+output output output
+output output output
+
+command three
+meow output
diff --git a/tests/functional/repl_characterization/data/no_nested_debuggers.test b/tests/functional/repl_characterization/data/no_nested_debuggers.test
index 5e834a68a..0199407f6 100644
--- a/tests/functional/repl_characterization/data/no_nested_debuggers.test
+++ b/tests/functional/repl_characterization/data/no_nested_debuggers.test
@@ -1,3 +1,4 @@
+@args --debugger
 we enter a debugger via builtins.break in the input file.
 
   info: breakpoint reached
@@ -37,3 +38,4 @@ and once again, more breakpoints are ignored.
 
   nix-repl> builtins.break 3
   3
+  error: bar
diff --git a/tests/functional/repl_characterization/data/regression_9917.test b/tests/functional/repl_characterization/data/regression_9917.test
index 44ca951b5..67ad1db6b 100644
--- a/tests/functional/repl_characterization/data/regression_9917.test
+++ b/tests/functional/repl_characterization/data/regression_9917.test
@@ -1,6 +1,7 @@
 https://github.com/NixOS/nix/pull/9917 (Enter debugger more reliably in let expressions and function calls)
 
 This test ensures that continues don't skip opportunities to enter the debugger.
+@args --debugger
   trace: before outer break
   info: breakpoint reached
 
@@ -13,7 +14,7 @@ This test ensures that continues don't skip opportunities to enter the debugger.
   0: error: breakpoint reached
   «none»:0
   1: while calling a function
-  TEST_DATA/regression_9917.nix:3:5
+  $TEST_DATA/regression_9917.nix:3:5
 
        2|   a = builtins.trace "before inner break" (
        3|     builtins.break { msg = "hello"; }
@@ -21,7 +22,7 @@ This test ensures that continues don't skip opportunities to enter the debugger.
        4|   );
 
   2: while calling a function
-  TEST_DATA/regression_9917.nix:2:7
+  $TEST_DATA/regression_9917.nix:2:7
 
        1| let
        2|   a = builtins.trace "before inner break" (
diff --git a/tests/functional/repl_characterization/data/regression_9918.test b/tests/functional/repl_characterization/data/regression_9918.test
index c30c405b6..a85d6d33a 100644
--- a/tests/functional/repl_characterization/data/regression_9918.test
+++ b/tests/functional/repl_characterization/data/regression_9918.test
@@ -1,3 +1,4 @@
+@args --debugger
   error:
          … while evaluating the error message passed to builtin.throw
 
@@ -14,3 +15,18 @@ We expect to be able to see locals like r in the debugger:
 
   Env level 1
   abort baseNameOf break builtins derivation derivationStrict dirOf false fetchGit fetchMercurial fetchTarball fetchTree fromTOML import isNull map null placeholder removeAttrs scopedImport throw toString true
+
+  nix-repl> :quit
+  error:
+         … while evaluating the file '$TEST_DATA/regression_9918.nix':
+
+         … while calling the 'throw' builtin
+           at $TEST_DATA/regression_9918.nix:3:7:
+              2|   r = [];
+              3|   x = builtins.throw r;
+               |       ^
+              4| in
+
+         … while evaluating the error message passed to builtin.throw
+
+         error: cannot coerce a list to a string: [ ]
diff --git a/tests/functional/repl_characterization/data/regression_l145.test b/tests/functional/repl_characterization/data/regression_l145.test
index 2fe04d221..bab347d26 100644
--- a/tests/functional/repl_characterization/data/regression_l145.test
+++ b/tests/functional/repl_characterization/data/regression_l145.test
@@ -1,3 +1,4 @@
+@args --debugger
   info: breakpoint reached
 
 debugger should not crash now, but also not show any with variables
@@ -12,3 +13,14 @@ debugger should not crash now, but also not show any with variables
 
   Env level 2
   abort baseNameOf break builtins derivation derivationStrict dirOf false fetchGit fetchMercurial fetchTarball fetchTree fromTOML import isNull map null placeholder removeAttrs scopedImport throw toString true
+  error:
+         … while evaluating the file '$TEST_DATA/regression_l145.nix':
+
+         … while calling the 'break' builtin
+           at $TEST_DATA/regression_l145.nix:3:7:
+              2| let
+              3|   x = builtins.break 1;
+               |       ^
+              4| in
+
+         error: breakpoint reached
diff --git a/tests/functional/repl_characterization/data/stack_vars.test b/tests/functional/repl_characterization/data/stack_vars.test
index 96ea5fe25..c9296eeaa 100644
--- a/tests/functional/repl_characterization/data/stack_vars.test
+++ b/tests/functional/repl_characterization/data/stack_vars.test
@@ -1,3 +1,4 @@
+@args --debugger
   trace: before outer break
   info: breakpoint reached
 
@@ -24,7 +25,7 @@ If we :st past the frame in the backtrace with the meow in it, the meow should n
   nix-repl> :st 3
 
   3: while calling a function
-  TEST_DATA/stack_vars.nix:5:7
+  $TEST_DATA/stack_vars.nix:5:7
 
        4|   );
        5|   b = builtins.trace "before outer break" (
@@ -58,9 +59,8 @@ If we :st past the frame in the backtrace with the meow in it, the meow should n
   3
 
   nix-repl> :st 3
-
   3: while calling a function
-  TEST_DATA/stack_vars.nix:2:7
+  $TEST_DATA/stack_vars.nix:2:7
 
        1| let
        2|   a = builtins.trace "before inner break" (
@@ -72,3 +72,21 @@ If we :st past the frame in the backtrace with the meow in it, the meow should n
 
   Env level 1
   abort baseNameOf break builtins derivation derivationStrict dirOf false fetchGit fetchMercurial fetchTarball fetchTree fromTOML import isNull map null placeholder removeAttrs scopedImport throw toString true
+
+  nix-repl> :quit
+  error:
+         … while calling the 'trace' builtin
+           at $TEST_DATA/stack_vars.nix:2:7:
+              1| let
+              2|   a = builtins.trace "before inner break" (
+               |       ^
+              3|     let meow' = 3; in builtins.break { msg = "hello"; }
+
+         … while calling the 'break' builtin
+           at $TEST_DATA/stack_vars.nix:3:23:
+              2|   a = builtins.trace "before inner break" (
+              3|     let meow' = 3; in builtins.break { msg = "hello"; }
+               |                       ^
+              4|   );
+
+         error: breakpoint reached
diff --git a/tests/functional/repl_characterization/repl_characterization.cc b/tests/functional/repl_characterization/repl_characterization.cc
index 68a66b2f3..fa5a7ba74 100644
--- a/tests/functional/repl_characterization/repl_characterization.cc
+++ b/tests/functional/repl_characterization/repl_characterization.cc
@@ -1,16 +1,17 @@
 #include <gtest/gtest.h>
 
+#include <boost/algorithm/string/replace.hpp>
+#include <optional>
 #include <string>
 #include <string_view>
-#include <optional>
 #include <unistd.h>
-#include <boost/algorithm/string/replace.hpp>
 
+#include "escape-string.hh"
 #include "test-session.hh"
-#include "util.hh"
 #include "tests/characterization.hh"
 #include "tests/cli-literate-parser.hh"
 #include "tests/terminal-code-eater.hh"
+#include "util.hh"
 
 using namespace std::string_literals;
 
@@ -40,92 +41,149 @@ public:
         return unitTestData + "/" + testStem;
     }
 
-    void runReplTest(std::string_view const & content, std::vector<std::string> extraArgs = {}) const
+    void runReplTest(const std::string content, std::vector<std::string> extraArgs = {}) const
     {
-        auto syntax = CLILiterateParser::parse(std::string(REPL_PROMPT), content);
+        auto parsed = cli_literate_parser::parse(
+            content, cli_literate_parser::Config{.prompt = std::string(REPL_PROMPT), .indent = 2}
+        );
+        parsed.interpolatePwd(unitTestData);
 
         // FIXME: why does this need two --quiets
-        // show-trace is on by default due to test configuration, but is not a standard
-        Strings args{"--quiet", "repl", "--quiet", "--option", "show-trace", "false", "--offline", "--extra-experimental-features", "repl-automation"};
+        // show-trace is on by default due to test configuration, but is not a
+        // standard
+        Strings args{
+            "--quiet",
+            "repl",
+            "--quiet",
+            "--option",
+            "show-trace",
+            "false",
+            "--offline",
+            "--extra-experimental-features",
+            "repl-automation",
+        };
         args.insert(args.end(), extraArgs.begin(), extraArgs.end());
+        args.insert(args.end(), parsed.args.begin(), parsed.args.end());
 
         auto nixBin = canonPath(getEnvNonEmpty("NIX_BIN_DIR").value_or(NIX_BIN_DIR));
 
         auto process = RunningProcess::start(nixBin + "/nix", args);
-        auto session = TestSession{std::string(AUTOMATION_PROMPT), std::move(process)};
-
-        for (auto & bit : syntax) {
-            if (bit.kind != CLILiterateParser::NodeKind::COMMAND) {
-                continue;
-            }
-
-            if (!session.waitForPrompt()) {
-                ASSERT_TRUE(false);
-            }
-            session.runCommand(bit.text);
+        auto session = TestSession(std::string(AUTOMATION_PROMPT), std::move(process));
+
+        for (auto & event : parsed.syntax) {
+            std::visit(
+                overloaded{
+                    [&](const cli_literate_parser::Command & e) {
+                        ASSERT_TRUE(session.waitForPrompt());
+                        if (e.text == ":quit") {
+                            // If we quit the repl explicitly, we won't have a
+                            // prompt when we're done.
+                            parsed.shouldStart = false;
+                        }
+                        session.runCommand(e.text);
+                    },
+                    [&](const auto & e) {},
+                },
+                event
+            );
         }
-        if (!session.waitForPrompt()) {
-            ASSERT_TRUE(false);
+        if (parsed.shouldStart) {
+            ASSERT_TRUE(session.waitForPrompt());
         }
         session.close();
 
-        auto replacedOutLog = boost::algorithm::replace_all_copy(session.outLog, unitTestData, "TEST_DATA");
+        auto replacedOutLog =
+            boost::algorithm::replace_all_copy(session.outLog, unitTestData, "$TEST_DATA");
         auto cleanedOutLog = trimOutLog(replacedOutLog);
 
-        auto parsedOutLog = CLILiterateParser::parse(std::string(AUTOMATION_PROMPT), cleanedOutLog, 0);
+        auto parsedOutLog = cli_literate_parser::parse(
+            std::string(cleanedOutLog),
+            cli_literate_parser::Config{.prompt = std::string(AUTOMATION_PROMPT), .indent = 0}
+        );
 
-        parsedOutLog = CLILiterateParser::tidyOutputForComparison(std::move(parsedOutLog));
-        syntax = CLILiterateParser::tidyOutputForComparison(std::move(syntax));
+        auto expected = parsed.tidyOutputForComparison();
+        auto actual = parsedOutLog.tidyOutputForComparison();
 
-        ASSERT_EQ(parsedOutLog, syntax);
+        ASSERT_EQ(expected, actual);
+    }
+
+    void runReplTestPath(const std::string_view & nameBase, std::vector<std::string> extraArgs)
+    {
+        auto nixPath = goldenMaster(nameBase + ".nix");
+        if (pathExists(nixPath)) {
+            extraArgs.push_back("-f");
+            extraArgs.push_back(nixPath);
+        }
+        readTest(nameBase + ".test", [this, extraArgs](std::string input) {
+            runReplTest(input, extraArgs);
+        });
+    }
+
+    void runReplTestPath(const std::string_view & nameBase)
+    {
+        runReplTestPath(nameBase, {});
+    }
+
+    void runDebuggerTest(const std::string_view & nameBase)
+    {
+        runReplTestPath(nameBase, {"--debugger"});
     }
 };
 
-TEST_F(ReplSessionTest, parses)
+TEST_F(ReplSessionTest, round_trip)
 {
-    writeTest("basic.ast", [this]() {
+    writeTest("basic.test", [this]() {
         const std::string content = readFile(goldenMaster("basic.test"));
-        auto parser = CLILiterateParser{std::string(REPL_PROMPT)};
-        parser.feed(content);
+        auto parsed = cli_literate_parser::parse(
+            content, cli_literate_parser::Config{.prompt = std::string(REPL_PROMPT)}
+        );
 
         std::ostringstream out{};
-        for (auto & bit : parser.syntax()) {
-            out << bit.print() << "\n";
+        for (auto & node : parsed.syntax) {
+            cli_literate_parser::unparseNode(out, node, true);
         }
         return out.str();
     });
+}
 
+TEST_F(ReplSessionTest, tidy)
+{
+    writeTest("basic.ast", [this]() {
+        const std::string content = readFile(goldenMaster("basic.test"));
+        auto parsed = cli_literate_parser::parse(
+            content, cli_literate_parser::Config{.prompt = std::string(REPL_PROMPT)}
+        );
+        std::ostringstream out{};
+        for (auto & node : parsed.syntax) {
+            out << debugNode(node) << "\n";
+        }
+        return out.str();
+    });
     writeTest("basic_tidied.ast", [this]() {
         const std::string content = readFile(goldenMaster("basic.test"));
-        auto syntax = CLILiterateParser::parse(std::string(REPL_PROMPT), content);
-
-        syntax = CLILiterateParser::tidyOutputForComparison(std::move(syntax));
-
+        auto parsed = cli_literate_parser::parse(
+            content, cli_literate_parser::Config{.prompt = std::string(REPL_PROMPT)}
+        );
+        auto tidied = parsed.tidyOutputForComparison();
         std::ostringstream out{};
-        for (auto & bit : syntax) {
-            out << bit.print() << "\n";
+        for (auto & node : tidied) {
+            out << debugNode(node) << "\n";
         }
         return out.str();
     });
 }
 
-TEST_F(ReplSessionTest, repl_basic)
-{
-    readTest("basic_repl.test", [this](std::string input) { runReplTest(input); });
-}
-
-#define DEBUGGER_TEST(name) \
+#define REPL_TEST(name)           \
     TEST_F(ReplSessionTest, name) \
-    { \
-        readTest(#name ".test", [this](std::string input) { \
-            runReplTest(input, {"--debugger", "-f", goldenMaster(#name ".nix")}); \
-        }); \
+    {                             \
+        runReplTestPath(#name);   \
     }
 
-DEBUGGER_TEST(regression_9918);
-DEBUGGER_TEST(regression_9917);
-DEBUGGER_TEST(regression_l145);
-DEBUGGER_TEST(stack_vars);
-DEBUGGER_TEST(no_nested_debuggers);
+REPL_TEST(basic_repl);
+REPL_TEST(no_nested_debuggers);
+REPL_TEST(regression_9917);
+REPL_TEST(regression_9918);
+REPL_TEST(regression_l145);
+REPL_TEST(stack_vars);
 
-};
+}; // namespace nix
diff --git a/tests/functional/repl_characterization/test-session.cc b/tests/functional/repl_characterization/test-session.cc
index 50e27e58c..52179a372 100644
--- a/tests/functional/repl_characterization/test-session.cc
+++ b/tests/functional/repl_characterization/test-session.cc
@@ -1,4 +1,5 @@
 #include <iostream>
+#include <span>
 #include <unistd.h>
 
 #include "test-session.hh"
@@ -21,14 +22,17 @@ RunningProcess RunningProcess::start(std::string executable, Strings args)
 
     // This is separate from runProgram2 because we have different IO requirements
     pid_t pid = startProcess([&]() {
-        if (dup2(procStdout.writeSide.get(), STDOUT_FILENO) == -1)
+        if (dup2(procStdout.writeSide.get(), STDOUT_FILENO) == -1) {
             throw SysError("dupping stdout");
-        if (dup2(procStdin.readSide.get(), STDIN_FILENO) == -1)
+        }
+        if (dup2(procStdin.readSide.get(), STDIN_FILENO) == -1) {
             throw SysError("dupping stdin");
+        }
         procStdin.writeSide.close();
         procStdout.readSide.close();
-        if (dup2(STDOUT_FILENO, STDERR_FILENO) == -1)
+        if (dup2(STDOUT_FILENO, STDERR_FILENO) == -1) {
             throw SysError("dupping stderr");
+        }
         execv(executable.c_str(), stringsToCharPtrs(args).data());
         throw SysError("exec did not happen");
     });
@@ -44,7 +48,8 @@ RunningProcess RunningProcess::start(std::string executable, Strings args)
 }
 
 [[gnu::unused]]
-std::ostream & operator<<(std::ostream & os, ReplOutputParser::State s)
+std::ostream &
+operator<<(std::ostream & os, ReplOutputParser::State s)
 {
     switch (s) {
     case ReplOutputParser::State::Prompt:
@@ -91,8 +96,7 @@ bool ReplOutputParser::feed(char c)
     return false;
 }
 
-/** Waits for the prompt and then returns if a prompt was found */
-bool TestSession::waitForPrompt()
+bool TestSession::readOutThen(ReadOutThenCallback cb)
 {
     std::vector<char> buf(1024);
 
@@ -106,38 +110,67 @@ bool TestSession::waitForPrompt()
             return false;
         }
 
+        switch (cb(std::span(buf.data(), res))) {
+        case ReadOutThenCallbackResult::Stop:
+            return true;
+        case ReadOutThenCallbackResult::Continue:
+            continue;
+        }
+    }
+}
+
+bool TestSession::waitForPrompt()
+{
+    bool notEof = readOutThen([&](std::span<char> s) -> ReadOutThenCallbackResult {
         bool foundPrompt = false;
-        for (ssize_t i = 0; i < res; ++i) {
+
+        for (auto ch : s) {
             // foundPrompt = foundPrompt || outputParser.feed(buf[i]);
             bool wasEaten = true;
-            eater.feed(buf[i], [&](char c) {
+            eater.feed(ch, [&](char c) {
                 wasEaten = false;
-                foundPrompt = outputParser.feed(buf[i]) || foundPrompt;
+                foundPrompt = outputParser.feed(ch) || foundPrompt;
 
                 outLog.push_back(c);
             });
 
             if constexpr (DEBUG_REPL_PARSER) {
-                std::cerr << "raw " << MaybeHexEscapedChar{buf[i]} << (wasEaten ? " [eaten]" : "") << "\n";
+                std::cerr << "raw " << MaybeHexEscapedChar{ch} << (wasEaten ? " [eaten]" : "") << "\n";
             }
         }
 
-        if (foundPrompt) {
-            return true;
+        return foundPrompt ? ReadOutThenCallbackResult::Stop : ReadOutThenCallbackResult::Continue;
+    });
+
+    return notEof;
+}
+
+void TestSession::wait()
+{
+    readOutThen([&](std::span<char> s) {
+        for (auto ch : s) {
+            eater.feed(ch, [&](char c) {
+                outputParser.feed(c);
+                outLog.push_back(c);
+            });
         }
-    }
+        // just keep reading till we hit eof
+        return ReadOutThenCallbackResult::Continue;
+    });
 }
 
 void TestSession::close()
 {
     proc.procStdin.close();
+    wait();
     proc.procStdout.close();
 }
 
 void TestSession::runCommand(std::string command)
 {
-    if constexpr (DEBUG_REPL_PARSER)
+    if constexpr (DEBUG_REPL_PARSER) {
         std::cerr << "runCommand " << command << "\n";
+    }
     command += "\n";
     // We have to feed a newline into the output parser, since Nix might not
     // give us a newline before a prompt in all cases (it might clear line
diff --git a/tests/functional/repl_characterization/test-session.hh b/tests/functional/repl_characterization/test-session.hh
index 19636640b..2552542fb 100644
--- a/tests/functional/repl_characterization/test-session.hh
+++ b/tests/functional/repl_characterization/test-session.hh
@@ -1,7 +1,9 @@
 #pragma once
 ///@file
 
+#include <functional>
 #include <sched.h>
+#include <span>
 #include <string>
 
 #include "util.hh"
@@ -22,8 +24,7 @@ struct RunningProcess
 class ReplOutputParser
 {
 public:
-    ReplOutputParser(std::string prompt)
-        : prompt(prompt)
+    ReplOutputParser(std::string prompt) : prompt(prompt)
     {
         assert(!prompt.empty());
     }
@@ -60,10 +61,27 @@ struct TestSession
     {
     }
 
+    /** Waits for the prompt and then returns if a prompt was found */
     bool waitForPrompt();
 
+    /** Feeds a line of input into the command */
     void runCommand(std::string command);
 
+    /** Closes the session, closing standard input and waiting for standard
+     * output to close, capturing any remaining output. */
     void close();
+
+private:
+    /** Waits until the command closes its output */
+    void wait();
+
+    enum class ReadOutThenCallbackResult { Stop, Continue };
+    using ReadOutThenCallback = std::function<ReadOutThenCallbackResult(std::span<char>)>;
+    /** Reads some chunks of output, calling the callback provided for each
+     * chunk and stopping if it returns Stop.
+     *
+     * @returns false if EOF, true if the callback requested we stop first.
+     * */
+    bool readOutThen(ReadOutThenCallback cb);
 };
 };