From abb8ad29c09354d03771fc404f2341b7ae8a61eb Mon Sep 17 00:00:00 2001 From: piegames Date: Wed, 18 Dec 2024 12:45:56 +0100 Subject: [PATCH] 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 | 7 ++- lix/libcmd/repl.cc | 62 ++++++++----------- lix/libexpr/eval.cc | 11 ++++ lix/libexpr/eval.hh | 18 ++++++ lix/libexpr/nixexpr.hh | 9 +++ lix/libexpr/parser/grammar.hh | 32 +++++++++- lix/libexpr/parser/parser-impl1.inc.cc | 49 ++++++++++++++- lix/libexpr/parser/parser.cc | 37 +++++++++++ .../data/repl_input.test | 50 +++++++++++++++ .../repl_characterization.cc | 5 ++ 10 files changed, 240 insertions(+), 40 deletions(-) create mode 100644 tests/functional/repl_characterization/data/repl_input.test diff --git a/doc/manual/rl-next/repl-improvements.md b/doc/manual/rl-next/repl-improvements.md index 3fb25611d..e2f530d33 100644 --- a/doc/manual/rl-next/repl-improvements.md +++ b/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 diff --git a/lix/libcmd/repl.cc b/lix/libcmd/repl.cc index 383183a24..880fb419e 100644 --- a/lix/libcmd/repl.cc +++ b/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, 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, 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 & 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, ExprReplBindings> NixRepl::parseReplString(std::string s) +{ + return evaluator.parseReplInput(std::move(s), CanonPath::fromCwd(), staticEnv, featureSettings); } diff --git a/lix/libexpr/eval.cc b/lix/libexpr/eval.cc index f4920992b..0e0cc5301 100644 --- a/lix/libexpr/eval.cc +++ b/lix/libexpr/eval.cc @@ -2782,6 +2782,17 @@ Expr & Evaluator::parseExprFromString( return parseExprFromString(std::move(s), basePath, builtins.staticEnv, featureSettings); } +std::variant, ExprReplBindings> +Evaluator::parseReplInput( + std::string s_, + const SourcePath & basePath, + std::shared_ptr & staticEnv, + const FeatureSettings & featureSettings +) +{ + auto s = make_ref(std::move(s_)); + return parse_repl(s->data(), s->size(), Pos::String{.source = s}, basePath, staticEnv, featureSettings); +} Expr & Evaluator::parseStdin() { diff --git a/lix/libexpr/eval.hh b/lix/libexpr/eval.hh index 1634fcd2d..99bd20bc2 100644 --- a/lix/libexpr/eval.hh +++ b/lix/libexpr/eval.hh @@ -599,6 +599,14 @@ public: const FeatureSettings & xpSettings = featureSettings ); + std::variant, ExprReplBindings> + parseReplInput( + std::string s, + const SourcePath & basePath, + std::shared_ptr & staticEnv, + const FeatureSettings & xpSettings = featureSettings + ); + Expr & parseStdin(); /** @@ -615,6 +623,16 @@ private: std::shared_ptr & staticEnv, const FeatureSettings & xpSettings = featureSettings); + std::variant, ExprReplBindings> + parse_repl( + char * text, + size_t length, + Pos::Origin origin, + const SourcePath & basePath, + std::shared_ptr & staticEnv, + const FeatureSettings & xpSettings = featureSettings + ); + public: BindingsBuilder buildBindings(size_t capacity) { diff --git a/lix/libexpr/nixexpr.hh b/lix/libexpr/nixexpr.hh index 780c4ccb8..663f48cb0 100644 --- a/lix/libexpr/nixexpr.hh +++ b/lix/libexpr/nixexpr.hh @@ -248,6 +248,15 @@ struct ExprSet : Expr, ExprAttrs { COMMON_METHODS }; +struct ExprReplBindings { + std::map> symbols; + + void bindVars(Evaluator & es, const std::shared_ptr & env) { + for (auto & [_, e] : symbols) + e->bindVars(es, env); + } +}; + struct ExprList : Expr { std::vector> elems; diff --git a/lix/libexpr/parser/grammar.hh b/lix/libexpr/parser/grammar.hh index 1266e3b53..75fa73f6e 100644 --- a/lix/libexpr/parser/grammar.hh +++ b/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> + _binding::value + // A binding usually must end with a `;`, except when in REPL > {}; -struct bindings : opt, seps>> {}; +struct bindings : opt< + list< + sor< + inherit, + seq>> + >, + seps + > +> {}; + +struct repl_binding : binding {}; + +struct repl_bindings : seq< + list, t::sep>, + /* Optional semicolon at the end for convenience */ + opt> +> {}; struct op { enum class kind { @@ -653,6 +669,16 @@ struct eof : sor> {}; struct root : must {}; +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, + seq + > {}; +}; +struct repl_root : _repl_root, must {}; + template diff --git a/lix/libexpr/parser/parser-impl1.inc.cc b/lix/libexpr/parser/parser-impl1.inc.cc index 2799ece7b..bfa3115d3 100644 --- a/lix/libexpr/parser/parser-impl1.inc.cc +++ b/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 : change_head { } }; -template<> struct BuildAST { +template<> struct BuildAST { static void apply0(BindingState & s, State & ps) { s.value = s->popExprOnly(); } @@ -442,6 +443,52 @@ template<> struct BuildAST : change_head { } }; +struct BindingsStateRepl : ExprState { + std::map> symbols; +}; + +template<> struct BuildAST : change_head { + 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(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, ExprReplBindings>; + +template<> struct BuildAST : change_head { + static void success0(BindingsStateRepl & b, ReplRootState & r, State &) { + r = ExprReplBindings { std::move(b.symbols) }; + } +}; + +template<> struct BuildAST : change_head { + static void success0(ExprState & inner, ReplRootState & outer, State & ps) { + auto [_pos, expr] = inner.finish(ps); + outer = std::move(expr); + } +}; + template<> struct BuildAST { static void apply(const auto & in, ExprState & s, State & ps) { if (in.string_view() == "__curPos") diff --git a/lix/libexpr/parser/parser.cc b/lix/libexpr/parser/parser.cc index c7354c525..4fc43aed1 100644 --- a/lix/libexpr/parser/parser.cc +++ b/lix/libexpr/parser/parser.cc @@ -49,4 +49,41 @@ Expr * Evaluator::parse( } } +std::variant, ExprReplBindings> +Evaluator::parse_repl( + char * text, + size_t length, + Pos::Origin origin, + const SourcePath & basePath, + std::shared_ptr & staticEnv, + const FeatureSettings & featureSettings) +{ + parser::State s = { + symbols, + positions, + basePath, + positions.addOrigin(origin, length), + this->s.exprSymbols, + featureSettings, + }; + + p::string_input inp{std::string_view{text, length}, "input"}; + try { + parser::v1::ReplRootState x; + p::parse(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)] + }); + } +} + } diff --git a/tests/functional/repl_characterization/data/repl_input.test b/tests/functional/repl_characterization/data/repl_input.test new file mode 100644 index 000000000..8e734c0ca --- /dev/null +++ b/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 + | ^ diff --git a/tests/functional/repl_characterization/repl_characterization.cc b/tests/functional/repl_characterization/repl_characterization.cc index 8fde93c4a..dea7b4d98 100644 --- a/tests/functional/repl_characterization/repl_characterization.cc +++ b/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);