repl: Always use parser, allow trailing ; in assignments

We now properly shell out to the parser instead of hacking stuff
together with a regex. Stuff we get for free by doing this:

- Optional trailing semicolon
- Declaring nested attribute sets
- String identifiers, and future proofing for eventual grammar
improvements to identifiers
- Dynamic attributes

Change-Id: Ibf1ad815e5e27caf162df05ea5ba5b1b4955d9c9

doc/manual/rl-next/repl-improvements.md

@@ -1,13 +1,18 @@
 ---
 synopsis: "REPL improvements"
 issues: []
-cls: [2319, 2320]
+cls: [2319, 2320, 2321]
 category: "Improvements"
 credits: ["piegames"]
 ---
 
 The REPL has seen various minor improvements:
 
+- Variable declarations have been improved, making copy-pasting code from attrsets a lot easier:
+  - Declarations can now optionally end with a semicolon
+  - Multiple declarations can be done within one command, separated by semicolon
+  - The `foo.bar = "baz";` syntax from attrsets is also supported, however without the attrset merging rules and with restrictions on dynamic attrs like in `let` bindings.
+  - Variable names now use the proper Nix grammar rules, instead of a regex that only vaguely matched legal identifiers.
 - Better error messages overall
 - The `:env` command to print currently available variables now also works outside of debug mode
 - Adding variables to the REPL now prints a small message on success

lix/libcmd/repl.cc

@@ -156,6 +156,7 @@ struct NixRepl
     void addAttrsToScope(Value & attrs);
     void addVarToScope(const Symbol name, Value & v);
     Expr & parseString(std::string s);
+    std::variant<std::unique_ptr<Expr>, ExprReplBindings> parseReplString(std::string s);
     void evalString(std::string s, Value & v);
     void loadDebugTraceEnv(const DebugTrace & dt);
 
@@ -469,23 +470,6 @@ StringSet NixRepl::completePrefix(const std::string &prefix)
     return completions;
 }
 
-
-// FIXME: DRY and match or use the parser
-static bool isVarName(std::string_view s)
-{
-    if (s.size() == 0) return false;
-    char c = s[0];
-    if ((c >= '0' && c <= '9') || c == '-' || c == '\'') return false;
-    for (auto & i : s)
-        if (!((i >= 'a' && i <= 'z') ||
-              (i >= 'A' && i <= 'Z') ||
-              (i >= '0' && i <= '9') ||
-              i == '_' || i == '-' || i == '\''))
-            return false;
-    return true;
-}
-
-
 StorePath NixRepl::getDerivationPath(Value & v) {
     auto drvInfo = getDerivation(state, v, false);
     if (!drvInfo)
@@ -870,23 +854,26 @@ ProcessLineResult NixRepl::processLine(std::string line)
         throw Error("unknown command '%1%'", command);
 
     else {
-        size_t p = line.find('=');
-        std::string name;
-        if (p != std::string::npos &&
-            p < line.size() &&
-            line[p + 1] != '=' &&
-            isVarName(name = removeWhitespace(line.substr(0, p))))
-        {
-            Expr & e = parseString(line.substr(p + 1));
-            Value & v(*evaluator.mem.allocValue());
-            v.mkThunk(env, e);
-            addVarToScope(evaluator.symbols.create(name), v);
-        } else {
-            Value v;
-            evalString(line, v);
-            printValue(std::cout, v, 1);
-            std::cout << std::endl;
-        }
+        /* A line is either a regular expression or a `var = expr` assignment */
+        std::variant<std::unique_ptr<Expr>, ExprReplBindings> result = parseReplString(line);
+        std::visit(overloaded {
+            [&](ExprReplBindings & b) {
+                for (auto & [name, e] : b.symbols) {
+                    Value * v = state.ctx.mem.allocValue();
+                    e->eval(state, *env, *v);
+                    (void) e.release(); // NOLINT(bugprone-unused-return-value): leak because of thunk references
+                    addVarToScope(name, *v);
+                }
+            },
+            [&](std::unique_ptr<Expr> & e) {
+                Value v;
+                e->eval(state, *env, v);
+                (void) e.release(); // NOLINT(bugprone-unused-return-value): leak because of thunk references
+                state.forceValue(v, v.determinePos(noPos));
+                printValue(std::cout, v, 1);
+                std::cout << std::endl;
+            }
+        }, result);
     }
 
     return ProcessLineResult::PromptAgain;
@@ -1115,7 +1102,12 @@ Value * NixRepl::bindingsToAttrs()
 
 Expr & NixRepl::parseString(std::string s)
 {
-    return evaluator.parseExprFromString(std::move(s), CanonPath::fromCwd(), staticEnv);
+    return evaluator.parseExprFromString(std::move(s), CanonPath::fromCwd(), staticEnv, featureSettings);
+}
+
+std::variant<std::unique_ptr<Expr>, ExprReplBindings> NixRepl::parseReplString(std::string s)
+{
+    return evaluator.parseReplInput(std::move(s), CanonPath::fromCwd(), staticEnv, featureSettings);
 }
 
 

lix/libexpr/eval.cc

@@ -2782,6 +2782,17 @@ Expr & Evaluator::parseExprFromString(
     return parseExprFromString(std::move(s), basePath, builtins.staticEnv, featureSettings);
 }
 
+std::variant<std::unique_ptr<Expr>, ExprReplBindings>
+Evaluator::parseReplInput(
+    std::string s_,
+    const SourcePath & basePath,
+    std::shared_ptr<StaticEnv> & staticEnv,
+    const FeatureSettings & featureSettings
+)
+{
+    auto s = make_ref<std::string>(std::move(s_));
+    return parse_repl(s->data(), s->size(), Pos::String{.source = s}, basePath, staticEnv, featureSettings);
+}
 
 Expr & Evaluator::parseStdin()
 {

lix/libexpr/eval.hh

@@ -599,6 +599,14 @@ public:
         const FeatureSettings & xpSettings = featureSettings
     );
 
+    std::variant<std::unique_ptr<Expr>, ExprReplBindings>
+    parseReplInput(
+        std::string s,
+        const SourcePath & basePath,
+        std::shared_ptr<StaticEnv> & staticEnv,
+        const FeatureSettings & xpSettings = featureSettings
+    );
+
     Expr & parseStdin();
 
     /**
@@ -615,6 +623,16 @@ private:
         std::shared_ptr<StaticEnv> & staticEnv,
         const FeatureSettings & xpSettings = featureSettings);
 
+    std::variant<std::unique_ptr<Expr>, ExprReplBindings>
+    parse_repl(
+        char * text,
+        size_t length,
+        Pos::Origin origin,
+        const SourcePath & basePath,
+        std::shared_ptr<StaticEnv> & staticEnv,
+        const FeatureSettings & xpSettings = featureSettings
+    );
+
 public:
     BindingsBuilder buildBindings(size_t capacity)
     {

lix/libexpr/nixexpr.hh

@@ -248,6 +248,15 @@ struct ExprSet : Expr, ExprAttrs {
     COMMON_METHODS
 };
 
+struct ExprReplBindings {
+    std::map<Symbol, std::unique_ptr<Expr>> symbols;
+
+    void bindVars(Evaluator & es, const std::shared_ptr<const StaticEnv> & env) {
+        for (auto & [_, e] : symbols)
+            e->bindVars(es, env);
+    }
+};
+
 struct ExprList : Expr
 {
     std::vector<std::unique_ptr<Expr>> elems;

lix/libexpr/parser/grammar.hh

@@ -422,11 +422,27 @@ struct _binding {
 struct binding : _binding, seq<
     _binding::path, seps,
     must<_binding::equal>, seps,
-    _binding::value, seps,
-    must<one<';'>>
+    _binding::value
+    // A binding usually must end with a `;`, except when in REPL
 > {};
 
-struct bindings : opt<list<sor<inherit, binding>, seps>> {};
+struct bindings : opt<
+    list<
+        sor<
+            inherit,
+            seq<binding, seps, must<one<';'>>>
+        >,
+        seps
+    >
+> {};
+
+struct repl_binding : binding {};
+
+struct repl_bindings : seq<
+    list<repl_binding, one<';'>, t::sep>,
+    /* Optional semicolon at the end for convenience */
+    opt<seps, one<';'>>
+> {};
 
 struct op {
     enum class kind {
@@ -653,6 +669,16 @@ struct eof : sor<p::eof, one<0>> {};
 
 struct root : must<seps, expr, seps, eof> {};
 
+struct _repl_root {
+    struct expression : expr {};
+    /* Just a thin wrapper to make the must<> error message declaration less bulky */
+    struct expr_or_binding : sor<
+        seq<expression, seps, eof>,
+        seq<repl_bindings, seps, eof>
+    > {};
+};
+struct repl_root : _repl_root, must<seps, _repl_root::expr_or_binding> {};
+
 
 
 template<typename Rule>

lix/libexpr/parser/parser-impl1.inc.cc

@@ -46,6 +46,7 @@ error_message_for(grammar::v1::seps) = "expecting separators";
 error_message_for(grammar::v1::path::forbid_prefix_triple_slash) = "too many slashes in path";
 error_message_for(grammar::v1::path::forbid_prefix_double_slash_no_interp) = "path has a trailing slash";
 error_message_for(grammar::v1::expr) = "expecting expression";
+error_message_for(grammar::v1::repl_root::expr_or_binding) = "expecting expression or a binding";
 error_message_for(grammar::v1::expr::unary) = "expecting expression";
 error_message_for(grammar::v1::binding::equal) = "expecting '='";
 error_message_for(grammar::v1::expr::lambda::arg) = "expecting identifier";
@@ -430,7 +431,7 @@ template<> struct BuildAST<grammar::v1::inherit> : change_head<InheritState> {
     }
 };
 
-template<> struct BuildAST<grammar::v1::_binding::value> {
+template<> struct BuildAST<grammar::v1::binding::value> {
     static void apply0(BindingState & s, State & ps) {
         s.value = s->popExprOnly();
     }
@@ -442,6 +443,52 @@ template<> struct BuildAST<grammar::v1::binding> : change_head<BindingState> {
     }
 };
 
+struct BindingsStateRepl : ExprState {
+    std::map<Symbol, std::unique_ptr<Expr>> symbols;
+};
+
+template<> struct BuildAST<grammar::v1::repl_binding> : change_head<BindingState> {
+    static void success(const auto & in, BindingState & b, BindingsStateRepl & s, State & ps) {
+        auto path = std::move(b.attrs);
+        AttrName name = std::move(path.front());
+        path.erase(path.begin());
+        if (name.expr)
+            throw ParseError({
+                .msg = HintFmt("dynamic attributes not allowed in REPL"),
+                .pos = ps.positions[ps.at(in)]
+            });
+        Symbol symbol = name.symbol;
+
+        if (auto iter = s.symbols.find(symbol); iter != s.symbols.end())
+            ps.dupAttr({symbol}, iter->second->getPos(), ps.at(in));
+
+        if (path.empty()) {
+            // key = value
+            s.symbols.emplace(symbol, std::move(b.value));
+        } else {
+            // key.stuff = value
+            auto attrs = std::make_unique<ExprSet>(b.value->getPos());
+            ps.addAttr(&*attrs, std::move(path), std::move(b.value), ps.at(in));
+            s.symbols.emplace(symbol, std::move(attrs));
+        }
+    }
+};
+
+using ReplRootState = std::variant<std::unique_ptr<Expr>, ExprReplBindings>;
+
+template<> struct BuildAST<grammar::v1::repl_bindings> : change_head<BindingsStateRepl> {
+    static void success0(BindingsStateRepl & b, ReplRootState & r, State &) {
+        r = ExprReplBindings { std::move(b.symbols) };
+    }
+};
+
+template<> struct BuildAST<grammar::v1::repl_root::expression> : change_head<ExprState> {
+    static void success0(ExprState & inner, ReplRootState & outer, State & ps) {
+        auto [_pos, expr] = inner.finish(ps);
+        outer = std::move(expr);
+    }
+};
+
 template<> struct BuildAST<grammar::v1::expr::id> {
     static void apply(const auto & in, ExprState & s, State & ps) {
         if (in.string_view() == "__curPos")

lix/libexpr/parser/parser.cc

@@ -49,4 +49,41 @@ Expr * Evaluator::parse(
     }
 }
 
+std::variant<std::unique_ptr<Expr>, ExprReplBindings>
+Evaluator::parse_repl(
+    char * text,
+    size_t length,
+    Pos::Origin origin,
+    const SourcePath & basePath,
+    std::shared_ptr<StaticEnv> & staticEnv,
+    const FeatureSettings & featureSettings)
+{
+    parser::State s = {
+        symbols,
+        positions,
+        basePath,
+        positions.addOrigin(origin, length),
+        this->s.exprSymbols,
+        featureSettings,
+    };
+
+    p::string_input<p::tracking_mode::lazy> inp{std::string_view{text, length}, "input"};
+    try {
+        parser::v1::ReplRootState x;
+        p::parse<parser::grammar::v1::repl_root, parser::v1::BuildAST, parser::v1::Control>(inp, x, s);
+
+        std::visit(overloaded {
+            [&] (ExprReplBindings & result) { result.bindVars(*this, staticEnv); },
+            [&] (auto & result) { result->bindVars(*this, staticEnv); }
+        }, x);
+        return x;
+    } catch (p::parse_error & e) { // NOLINT(lix-foreign-exceptions)
+        auto pos = e.positions().back();
+        throw ParseError({
+            .msg = HintFmt("syntax error, %s", e.message()),
+            .pos = positions[s.positions.add(s.origin, pos.byte)]
+        });
+    }
+}
+
 }

tests/functional/repl_characterization/data/repl_input.test

@@ -0,0 +1,50 @@
+@args -v
+Adding variables gives simple user feedback
+
+  Lix $VERSION
+  Type :? for help.
+  nix-repl> foo = 5
+  Added foo.
+
+  nix-repl> foo = 10
+  Updated foo.
+
+Optional semicolon at the end, allow setting multiple variables in one line
+
+  nix-repl> foo = 2;
+  Updated foo.
+
+  nix-repl> foo = 2; bar = 3;
+  Updated foo.
+  Added bar.
+
+String identifiers work
+
+  nix-repl> "silly name" = null
+  Added "silly name".
+
+Attrset syntax works, but without dynamic attrs or merging
+
+  nix-repl> foo.bar = "baz"
+  Updated foo.
+
+  nix-repl> foo
+  { bar = "baz"; }
+
+  nix-repl> foo."this works" = 42
+  Updated foo.
+
+  nix-repl> foo
+  { "this works" = 42; }
+
+  nix-repl> foo.bar = "baz"; foo.more = "error"
+  error: attribute 'foo' already defined at «string»:1:18
+         at «string»:1:12:
+              1| foo.bar = "baz"; foo.more = "error"
+               |            ^
+
+  nix-repl> ${foo} = 10
+  error: dynamic attributes not allowed in REPL
+         at «string»:1:1:
+              1| ${foo} = 10
+               | ^

tests/functional/repl_characterization/repl_characterization.cc

@@ -90,8 +90,12 @@ public:
         }
         session.close();
 
+        // Remove references to the checkout path
         auto replacedOutLog =
             boost::algorithm::replace_all_copy(session.outLog, unitTestData, "$TEST_DATA");
+        // Remove references to the current version
+        replacedOutLog =
+            boost::algorithm::replace_all_copy(replacedOutLog, PACKAGE_VERSION, "$VERSION");
         auto cleanedOutLog = trimOutLog(replacedOutLog);
 
         auto parsedOutLog = cli_literate_parser::parse(
@@ -178,6 +182,7 @@ REPL_TEST(regression_9917);
 REPL_TEST(regression_9918);
 REPL_TEST(regression_l145);
 REPL_TEST(regression_l592);
+REPL_TEST(repl_input);
 REPL_TEST(repl_overlays);
 REPL_TEST(repl_overlays_compose);
 REPL_TEST(repl_overlays_destructure_without_dotdotdot_errors);