From 0a7746603eda58c4b368e977e87d0aa4db397f5b Mon Sep 17 00:00:00 2001 From: pennae Date: Wed, 19 Jan 2022 13:39:42 +0100 Subject: [PATCH 01/13] remove ExprIndStr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit it can be replaced with StringToken if we add another bit if information to StringToken, namely whether this string should take part in indentation scanning or not. since all escaping terminates indentation scanning we need to set this bit only for the non-escaped IND_STRING rule. this improves performance by about 1%. before nix search --no-eval-cache --offline ../nixpkgs hello Time (mean ± σ): 8.880 s ± 0.048 s [User: 6.809 s, System: 1.643 s] Range (min … max): 8.781 s … 8.993 s 20 runs nix eval -f ../nixpkgs/pkgs/development/haskell-modules/hackage-packages.nix Time (mean ± σ): 375.0 ms ± 2.2 ms [User: 339.8 ms, System: 35.2 ms] Range (min … max): 371.5 ms … 379.3 ms 20 runs nix eval --raw --impure --expr 'with import {}; system' Time (mean ± σ): 2.831 s ± 0.040 s [User: 2.536 s, System: 0.225 s] Range (min … max): 2.769 s … 2.912 s 20 runs after nix search --no-eval-cache --offline ../nixpkgs hello Time (mean ± σ): 8.832 s ± 0.048 s [User: 6.757 s, System: 1.657 s] Range (min … max): 8.743 s … 8.921 s 20 runs nix eval -f ../nixpkgs/pkgs/development/haskell-modules/hackage-packages.nix Time (mean ± σ): 367.4 ms ± 3.2 ms [User: 332.7 ms, System: 34.7 ms] Range (min … max): 364.6 ms … 374.6 ms 20 runs nix eval --raw --impure --expr 'with import {}; system' Time (mean ± σ): 2.810 s ± 0.030 s [User: 2.517 s, System: 0.225 s] Range (min … max): 2.742 s … 2.854 s 20 runs --- src/libexpr/lexer.l | 18 ++++----- src/libexpr/nixexpr.hh | 7 ---- src/libexpr/parser.y | 86 +++++++++++++++++++++++------------------- 3 files changed, 57 insertions(+), 54 deletions(-) diff --git a/src/libexpr/lexer.l b/src/libexpr/lexer.l index a0e7a1877..e276b0467 100644 --- a/src/libexpr/lexer.l +++ b/src/libexpr/lexer.l @@ -66,7 +66,7 @@ static void adjustLoc(YYLTYPE * loc, const char * s, size_t len) // we make use of the fact that the parser receives a private copy of the input // string and can munge around in it. -static Expr * unescapeStr(SymbolTable & symbols, char * s, size_t length) +static StringToken unescapeStr(SymbolTable & symbols, char * s, size_t length) { char * result = s; char * t = s; @@ -89,7 +89,7 @@ static Expr * unescapeStr(SymbolTable & symbols, char * s, size_t length) else *t = c; t++; } - return new ExprString(symbols.create({result, size_t(t - result)})); + return {result, size_t(t - result)}; } @@ -176,7 +176,7 @@ or { return OR_KW; } /* It is impossible to match strings ending with '$' with one regex because trailing contexts are only valid at the end of a rule. (A sane but undocumented limitation.) */ - yylval->e = unescapeStr(data->symbols, yytext, yyleng); + yylval->str = unescapeStr(data->symbols, yytext, yyleng); return STR; } \$\{ { PUSH_STATE(DEFAULT); return DOLLAR_CURLY; } @@ -191,26 +191,26 @@ or { return OR_KW; } \'\'(\ *\n)? { PUSH_STATE(IND_STRING); return IND_STRING_OPEN; } ([^\$\']|\$[^\{\']|\'[^\'\$])+ { - yylval->e = new ExprIndStr(yytext); + yylval->str = {yytext, (size_t) yyleng, true}; return IND_STR; } \'\'\$ | \$ { - yylval->e = new ExprIndStr("$"); + yylval->str = {"$", 1}; return IND_STR; } \'\'\' { - yylval->e = new ExprIndStr("''"); + yylval->str = {"''", 2}; return IND_STR; } \'\'\\{ANY} { - yylval->e = unescapeStr(data->symbols, yytext + 2, yyleng - 2); + yylval->str = unescapeStr(data->symbols, yytext + 2, yyleng - 2); return IND_STR; } \$\{ { PUSH_STATE(DEFAULT); return DOLLAR_CURLY; } \'\' { POP_STATE(); return IND_STRING_CLOSE; } \' { - yylval->e = new ExprIndStr("'"); + yylval->str = {"'", 1}; return IND_STR; } @@ -264,7 +264,7 @@ or { return OR_KW; } PUSH_STATE(INPATH_SLASH); else PUSH_STATE(INPATH); - yylval->e = new ExprString(data->symbols.create(string(yytext))); + yylval->str = {yytext, (size_t) yyleng}; return STR; } {ANY} | diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 0a60057e5..ae11560ea 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -117,13 +117,6 @@ struct ExprString : Expr Value * maybeThunk(EvalState & state, Env & env); }; -/* Temporary class used during parsing of indented strings. */ -struct ExprIndStr : Expr -{ - string s; - ExprIndStr(const string & s) : s(s) { }; -}; - struct ExprPath : Expr { string s; diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index a3e713937..38b218156 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -16,6 +16,8 @@ #ifndef BISON_HEADER #define BISON_HEADER +#include + #include "util.hh" #include "nixexpr.hh" @@ -41,6 +43,15 @@ namespace nix { } +// using C a struct allows us to avoid having to define the special +// members that using string_view here would implicitly delete. +struct StringToken { + const char * p; + size_t l; + bool hasIndentation; + operator std::string_view() const { return {p, l}; } +}; + #define YY_DECL int yylex \ (YYSTYPE * yylval_param, YYLTYPE * yylloc_param, yyscan_t yyscanner, nix::ParseData * data) @@ -152,7 +163,8 @@ static void addFormal(const Pos & pos, Formals * formals, const Formal & formal) } -static Expr * stripIndentation(const Pos & pos, SymbolTable & symbols, vector > & es) +static Expr * stripIndentation(const Pos & pos, SymbolTable & symbols, + vector > > & es) { if (es.empty()) return new ExprString(symbols.create("")); @@ -163,20 +175,20 @@ static Expr * stripIndentation(const Pos & pos, SymbolTable & symbols, vector(i); - if (!e) { - /* Anti-quotations end the current start-of-line whitespace. */ + auto * str = std::get_if(&i); + if (!str || !str->hasIndentation) { + /* Anti-quotations and escaped characters end the current start-of-line whitespace. */ if (atStartOfLine) { atStartOfLine = false; if (curIndent < minIndent) minIndent = curIndent; } continue; } - for (size_t j = 0; j < e->s.size(); ++j) { + for (size_t j = 0; j < str->l; ++j) { if (atStartOfLine) { - if (e->s[j] == ' ') + if (str->p[j] == ' ') curIndent++; - else if (e->s[j] == '\n') { + else if (str->p[j] == '\n') { /* Empty line, doesn't influence minimum indentation. */ curIndent = 0; @@ -184,7 +196,7 @@ static Expr * stripIndentation(const Pos & pos, SymbolTable & symbols, vectors[j] == '\n') { + } else if (str->p[j] == '\n') { atStartOfLine = true; curIndent = 0; } @@ -196,33 +208,31 @@ static Expr * stripIndentation(const Pos & pos, SymbolTable & symbols, vector >::iterator i = es.begin(); i != es.end(); ++i, --n) { - ExprIndStr * e = dynamic_cast(i->second); - if (!e) { - atStartOfLine = false; - curDropped = 0; - es2->push_back(*i); - continue; - } - + auto i = es.begin(); + const auto trimExpr = [&] (Expr * e) { + atStartOfLine = false; + curDropped = 0; + es2->emplace_back(i->first, e); + }; + const auto trimString = [&] (const StringToken & t) { string s2; - for (size_t j = 0; j < e->s.size(); ++j) { + for (size_t j = 0; j < t.l; ++j) { if (atStartOfLine) { - if (e->s[j] == ' ') { + if (t.p[j] == ' ') { if (curDropped++ >= minIndent) - s2 += e->s[j]; + s2 += t.p[j]; } - else if (e->s[j] == '\n') { + else if (t.p[j] == '\n') { curDropped = 0; - s2 += e->s[j]; + s2 += t.p[j]; } else { atStartOfLine = false; curDropped = 0; - s2 += e->s[j]; + s2 += t.p[j]; } } else { - s2 += e->s[j]; - if (e->s[j] == '\n') atStartOfLine = true; + s2 += t.p[j]; + if (t.p[j] == '\n') atStartOfLine = true; } } @@ -235,6 +245,9 @@ static Expr * stripIndentation(const Pos & pos, SymbolTable & symbols, vectoremplace_back(i->first, new ExprString(symbols.create(s2))); + }; + for (; i != es.end(); ++i, --n) { + std::visit(overloaded { trimExpr, trimString }, i->second); } /* If this is a single string, then don't do a concatenation. */ @@ -273,18 +286,13 @@ void yyerror(YYLTYPE * loc, yyscan_t scanner, ParseData * data, const char * err nix::Formal * formal; nix::NixInt n; nix::NixFloat nf; - // using C a struct allows us to avoid having to define the special - // members that using string_view here would implicitly delete. - struct StringToken { - const char * p; - size_t l; - operator std::string_view() const { return {p, l}; } - }; StringToken id; // !!! -> Symbol StringToken path; StringToken uri; + StringToken str; std::vector * attrNames; std::vector > * string_parts; + std::vector > > * ind_string_parts; } %type start expr expr_function expr_if expr_op @@ -294,11 +302,12 @@ void yyerror(YYLTYPE * loc, yyscan_t scanner, ParseData * data, const char * err %type formals %type formal %type attrs attrpath -%type string_parts_interpolated ind_string_parts +%type string_parts_interpolated +%type ind_string_parts %type path_start string_parts string_attr %type attr %token ID ATTRPATH -%token STR IND_STR +%token STR IND_STR %token INT %token FLOAT %token PATH HPATH SPATH PATH_END @@ -449,18 +458,19 @@ expr_simple ; string_parts - : STR + : STR { $$ = new ExprString(data->symbols.create($1)); } | string_parts_interpolated { $$ = new ExprConcatStrings(CUR_POS, true, $1); } | { $$ = new ExprString(data->symbols.create("")); } ; string_parts_interpolated - : string_parts_interpolated STR { $$ = $1; $1->emplace_back(makeCurPos(@2, data), $2); } + : string_parts_interpolated STR + { $$ = $1; $1->emplace_back(makeCurPos(@2, data), new ExprString(data->symbols.create($2))); } | string_parts_interpolated DOLLAR_CURLY expr '}' { $$ = $1; $1->emplace_back(makeCurPos(@2, data), $3); } | DOLLAR_CURLY expr '}' { $$ = new vector >; $$->emplace_back(makeCurPos(@1, data), $2); } | STR DOLLAR_CURLY expr '}' { $$ = new vector >; - $$->emplace_back(makeCurPos(@1, data), $1); + $$->emplace_back(makeCurPos(@1, data), new ExprString(data->symbols.create($1))); $$->emplace_back(makeCurPos(@2, data), $3); } ; @@ -482,7 +492,7 @@ path_start ind_string_parts : ind_string_parts IND_STR { $$ = $1; $1->emplace_back(makeCurPos(@2, data), $2); } | ind_string_parts DOLLAR_CURLY expr '}' { $$ = $1; $1->emplace_back(makeCurPos(@2, data), $3); } - | { $$ = new vector >; } + | { $$ = new vector > >; } ; binds From 9ac836d1d6d7e3480ad3bc4dbe23286d9052052b Mon Sep 17 00:00:00 2001 From: pennae Date: Wed, 19 Jan 2022 14:31:30 +0100 Subject: [PATCH 02/13] don't use Symbols for strings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit string expressions by and large do not need the benefits a Symbol gives us, instead they pollute the symbol table and cause unnecessary overhead for almost all strings. the one place we can think of that benefits from them (attrpaths with expressions) extracts the benefit in the parser, which we'll have to touch anyway when changing ExprString to hold strings. this gives a sizeable improvement on of 3-5% on all benchmarks we've run. before nix search --no-eval-cache --offline ../nixpkgs hello Time (mean ± σ): 8.844 s ± 0.045 s [User: 6.750 s, System: 1.663 s] Range (min … max): 8.758 s … 8.922 s 20 runs nix eval -f ../nixpkgs/pkgs/development/haskell-modules/hackage-packages.nix Time (mean ± σ): 367.4 ms ± 3.3 ms [User: 332.3 ms, System: 35.2 ms] Range (min … max): 364.0 ms … 375.2 ms 20 runs nix eval --raw --impure --expr 'with import {}; system' Time (mean ± σ): 2.810 s ± 0.030 s [User: 2.517 s, System: 0.225 s] Range (min … max): 2.742 s … 2.854 s 20 runs after nix search --no-eval-cache --offline ../nixpkgs hello Time (mean ± σ): 8.533 s ± 0.068 s [User: 6.485 s, System: 1.642 s] Range (min … max): 8.404 s … 8.657 s 20 runs nix eval -f ../nixpkgs/pkgs/development/haskell-modules/hackage-packages.nix Time (mean ± σ): 347.6 ms ± 3.1 ms [User: 313.1 ms, System: 34.5 ms] Range (min … max): 343.3 ms … 354.6 ms 20 runs nix eval --raw --impure --expr 'with import {}; system' Time (mean ± σ): 2.709 s ± 0.032 s [User: 2.414 s, System: 0.232 s] Range (min … max): 2.655 s … 2.788 s 20 runs --- src/libexpr/nixexpr.hh | 4 ++-- src/libexpr/parser.y | 22 +++++++++++----------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index ae11560ea..a2070ff69 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -110,9 +110,9 @@ struct ExprFloat : Expr struct ExprString : Expr { - Symbol s; + string s; Value v; - ExprString(const Symbol & s) : s(s) { v.mkString(s); }; + ExprString(std::string s) : s(std::move(s)) { v.mkString(this->s.data()); }; COMMON_METHODS Value * maybeThunk(EvalState & state, Env & env); }; diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 38b218156..88bdebcd6 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -166,7 +166,7 @@ static void addFormal(const Pos & pos, Formals * formals, const Formal & formal) static Expr * stripIndentation(const Pos & pos, SymbolTable & symbols, vector > > & es) { - if (es.empty()) return new ExprString(symbols.create("")); + if (es.empty()) return new ExprString(""); /* Figure out the minimum indentation. Note that by design whitespace-only final lines are not taken into account. (So @@ -244,7 +244,7 @@ static Expr * stripIndentation(const Pos & pos, SymbolTable & symbols, s2 = string(s2, 0, p + 1); } - es2->emplace_back(i->first, new ExprString(symbols.create(s2))); + es2->emplace_back(i->first, new ExprString(s2)); }; for (; i != es.end(); ++i, --n) { std::visit(overloaded { trimExpr, trimString }, i->second); @@ -434,7 +434,7 @@ expr_simple $$ = new ExprCall(CUR_POS, new ExprVar(data->symbols.create("__findFile")), {new ExprVar(data->symbols.create("__nixPath")), - new ExprString(data->symbols.create(path))}); + new ExprString(path)}); } | URI { static bool noURLLiterals = settings.isExperimentalFeatureEnabled(Xp::NoUrlLiterals); @@ -443,7 +443,7 @@ expr_simple .msg = hintfmt("URL literals are disabled"), .errPos = CUR_POS }); - $$ = new ExprString(data->symbols.create($1)); + $$ = new ExprString(string($1)); } | '(' expr ')' { $$ = $2; } /* Let expressions `let {..., body = ...}' are just desugared @@ -458,19 +458,19 @@ expr_simple ; string_parts - : STR { $$ = new ExprString(data->symbols.create($1)); } + : STR { $$ = new ExprString(string($1)); } | string_parts_interpolated { $$ = new ExprConcatStrings(CUR_POS, true, $1); } - | { $$ = new ExprString(data->symbols.create("")); } + | { $$ = new ExprString(""); } ; string_parts_interpolated : string_parts_interpolated STR - { $$ = $1; $1->emplace_back(makeCurPos(@2, data), new ExprString(data->symbols.create($2))); } + { $$ = $1; $1->emplace_back(makeCurPos(@2, data), new ExprString(string($2))); } | string_parts_interpolated DOLLAR_CURLY expr '}' { $$ = $1; $1->emplace_back(makeCurPos(@2, data), $3); } | DOLLAR_CURLY expr '}' { $$ = new vector >; $$->emplace_back(makeCurPos(@1, data), $2); } | STR DOLLAR_CURLY expr '}' { $$ = new vector >; - $$->emplace_back(makeCurPos(@1, data), new ExprString(data->symbols.create($1))); + $$->emplace_back(makeCurPos(@1, data), new ExprString(string($1))); $$->emplace_back(makeCurPos(@2, data), $3); } ; @@ -524,7 +524,7 @@ attrs { $$ = $1; ExprString * str = dynamic_cast($2); if (str) { - $$->push_back(AttrName(str->s)); + $$->push_back(AttrName(data->symbols.create(str->s))); delete str; } else throw ParseError({ @@ -541,7 +541,7 @@ attrpath { $$ = $1; ExprString * str = dynamic_cast($3); if (str) { - $$->push_back(AttrName(str->s)); + $$->push_back(AttrName(data->symbols.create(str->s))); delete str; } else $$->push_back(AttrName($3)); @@ -551,7 +551,7 @@ attrpath { $$ = new vector; ExprString *str = dynamic_cast($1); if (str) { - $$->push_back(AttrName(str->s)); + $$->push_back(AttrName(data->symbols.create(str->s))); delete str; } else $$->push_back(AttrName($1)); From 7d4cc5515ce5e4972fd5b474ae18cf95f6ea257e Mon Sep 17 00:00:00 2001 From: pennae Date: Wed, 19 Jan 2022 16:49:02 +0100 Subject: [PATCH 03/13] defer formals duplicate check for incresed efficiency all round MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit if we defer the duplicate argument check for lambda formals we can use more efficient data structures for the formals set, and we can get rid of the duplication of formals names to boot. instead of a list of formals we've seen and a set of names we'll keep a vector instead and run a sort+dupcheck step before moving the parsed formals into a newly created lambda. this improves performance on search and rebuild by ~1%, pure parsing gains more (about 4%). this does reorder lambda arguments in the xml output, but the output is still stable. this shouldn't be a problem since argument order is not semantically important anyway. before nix search --no-eval-cache --offline ../nixpkgs hello Time (mean ± σ): 8.550 s ± 0.060 s [User: 6.470 s, System: 1.664 s] Range (min … max): 8.435 s … 8.666 s 20 runs nix eval -f ../nixpkgs/pkgs/development/haskell-modules/hackage-packages.nix Time (mean ± σ): 346.7 ms ± 2.1 ms [User: 312.4 ms, System: 34.2 ms] Range (min … max): 343.8 ms … 353.4 ms 20 runs nix eval --raw --impure --expr 'with import {}; system' Time (mean ± σ): 2.720 s ± 0.031 s [User: 2.415 s, System: 0.231 s] Range (min … max): 2.662 s … 2.780 s 20 runs after nix search --no-eval-cache --offline ../nixpkgs hello Time (mean ± σ): 8.462 s ± 0.063 s [User: 6.398 s, System: 1.661 s] Range (min … max): 8.339 s … 8.542 s 20 runs nix eval -f ../nixpkgs/pkgs/development/haskell-modules/hackage-packages.nix Time (mean ± σ): 329.1 ms ± 1.4 ms [User: 296.8 ms, System: 32.3 ms] Range (min … max): 326.1 ms … 330.8 ms 20 runs nix eval --raw --impure --expr 'with import {}; system' Time (mean ± σ): 2.687 s ± 0.035 s [User: 2.392 s, System: 0.228 s] Range (min … max): 2.626 s … 2.754 s 20 runs --- src/libexpr/eval.cc | 2 +- src/libexpr/nixexpr.hh | 24 +++++++++---- src/libexpr/parser.y | 61 +++++++++++++++++++++++++------- src/libexpr/value-to-xml.cc | 2 +- src/nix/flake.cc | 4 +-- tests/check.nix | 2 +- tests/lang/eval-okay-xml.exp.xml | 2 +- 7 files changed, 71 insertions(+), 26 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 94ffab175..73ed3a8a1 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1368,7 +1368,7 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value & /* Nope, so show the first unexpected argument to the user. */ for (auto & i : *args[0]->attrs) - if (!lambda.formals->argNames.count(i.name)) + if (!lambda.formals->has(i.name)) throwTypeError(pos, "%1% called with unexpected argument '%2%'", lambda, i.name); abort(); // can't happen } diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index a2070ff69..4e923ac89 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -216,10 +216,25 @@ struct Formal struct Formals { - typedef std::list Formals_; + typedef std::vector Formals_; Formals_ formals; - std::set argNames; // used during parsing bool ellipsis; + + bool has(Symbol arg) const { + auto it = std::lower_bound(formals.begin(), formals.end(), arg, + [] (const Formal & f, const Symbol & sym) { return f.name < sym; }); + return it != formals.end() && it->name == arg; + } + + std::vector lexicographicOrder() const + { + std::vector result(formals.begin(), formals.end()); + std::sort(result.begin(), result.end(), + [] (const Formal & a, const Formal & b) { + return std::string_view(a.name) < std::string_view(b.name); + }); + return result; + } }; struct ExprLambda : Expr @@ -232,11 +247,6 @@ struct ExprLambda : Expr ExprLambda(const Pos & pos, const Symbol & arg, Formals * formals, Expr * body) : pos(pos), arg(arg), formals(formals), body(body) { - if (!arg.empty() && formals && formals->argNames.find(arg) != formals->argNames.end()) - throw ParseError({ - .msg = hintfmt("duplicate formal function argument '%1%'", arg), - .errPos = pos - }); }; void setName(Symbol & name); string showNamePos() const; diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 88bdebcd6..1b0177e4a 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -41,6 +41,11 @@ namespace nix { { }; }; + struct ParserFormals { + std::vector formals; + bool ellipsis = false; + }; + } // using C a struct allows us to avoid having to define the special @@ -151,15 +156,39 @@ static void addAttr(ExprAttrs * attrs, AttrPath & attrPath, } -static void addFormal(const Pos & pos, Formals * formals, const Formal & formal) +static Formals * toFormals(ParseData & data, ParserFormals * formals, + Pos pos = noPos, Symbol arg = {}) { - if (!formals->argNames.insert(formal.name).second) + std::sort(formals->formals.begin(), formals->formals.end(), + [] (const auto & a, const auto & b) { + return std::tie(a.name, a.pos) < std::tie(b.name, b.pos); + }); + + std::optional> duplicate; + for (size_t i = 0; i + 1 < formals->formals.size(); i++) { + if (formals->formals[i].name != formals->formals[i + 1].name) + continue; + std::pair thisDup{formals->formals[i].name, formals->formals[i + 1].pos}; + duplicate = std::min(thisDup, duplicate.value_or(thisDup)); + } + if (duplicate) throw ParseError({ - .msg = hintfmt("duplicate formal function argument '%1%'", - formal.name), + .msg = hintfmt("duplicate formal function argument '%1%'", duplicate->first), + .errPos = duplicate->second + }); + + Formals result; + result.ellipsis = formals->ellipsis; + result.formals = std::move(formals->formals); + + if (arg.set() && result.has(arg)) + throw ParseError({ + .msg = hintfmt("duplicate formal function argument '%1%'", arg), .errPos = pos }); - formals->formals.push_front(formal); + + delete formals; + return new Formals(std::move(result)); } @@ -282,7 +311,7 @@ void yyerror(YYLTYPE * loc, yyscan_t scanner, ParseData * data, const char * err nix::Expr * e; nix::ExprList * list; nix::ExprAttrs * attrs; - nix::Formals * formals; + nix::ParserFormals * formals; nix::Formal * formal; nix::NixInt n; nix::NixFloat nf; @@ -340,11 +369,17 @@ expr_function : ID ':' expr_function { $$ = new ExprLambda(CUR_POS, data->symbols.create($1), 0, $3); } | '{' formals '}' ':' expr_function - { $$ = new ExprLambda(CUR_POS, data->symbols.create(""), $2, $5); } + { $$ = new ExprLambda(CUR_POS, data->symbols.create(""), toFormals(*data, $2), $5); } | '{' formals '}' '@' ID ':' expr_function - { $$ = new ExprLambda(CUR_POS, data->symbols.create($5), $2, $7); } + { + Symbol arg = data->symbols.create($5); + $$ = new ExprLambda(CUR_POS, arg, toFormals(*data, $2, CUR_POS, arg), $7); + } | ID '@' '{' formals '}' ':' expr_function - { $$ = new ExprLambda(CUR_POS, data->symbols.create($1), $4, $7); } + { + Symbol arg = data->symbols.create($1); + $$ = new ExprLambda(CUR_POS, arg, toFormals(*data, $4, CUR_POS, arg), $7); + } | ASSERT expr ';' expr_function { $$ = new ExprAssert(CUR_POS, $2, $4); } | WITH expr ';' expr_function @@ -575,13 +610,13 @@ expr_list formals : formal ',' formals - { $$ = $3; addFormal(CUR_POS, $$, *$1); } + { $$ = $3; $$->formals.push_back(*$1); } | formal - { $$ = new Formals; addFormal(CUR_POS, $$, *$1); $$->ellipsis = false; } + { $$ = new ParserFormals; $$->formals.push_back(*$1); $$->ellipsis = false; } | - { $$ = new Formals; $$->ellipsis = false; } + { $$ = new ParserFormals; $$->ellipsis = false; } | ELLIPSIS - { $$ = new Formals; $$->ellipsis = true; } + { $$ = new ParserFormals; $$->ellipsis = true; } ; formal diff --git a/src/libexpr/value-to-xml.cc b/src/libexpr/value-to-xml.cc index a875f82d7..a9fb60b0e 100644 --- a/src/libexpr/value-to-xml.cc +++ b/src/libexpr/value-to-xml.cc @@ -142,7 +142,7 @@ static void printValueAsXML(EvalState & state, bool strict, bool location, if (!v.lambda.fun->arg.empty()) attrs["name"] = v.lambda.fun->arg; if (v.lambda.fun->formals->ellipsis) attrs["ellipsis"] = "1"; XMLOpenElement _(doc, "attrspat", attrs); - for (auto & i : v.lambda.fun->formals->formals) + for (auto & i : v.lambda.fun->formals->lexicographicOrder()) doc.writeEmptyElement("attr", singletonAttrs("name", i.name)); } else doc.writeEmptyElement("varpat", singletonAttrs("name", v.lambda.fun->arg)); diff --git a/src/nix/flake.cc b/src/nix/flake.cc index 97f4d911c..101d38c6c 100644 --- a/src/nix/flake.cc +++ b/src/nix/flake.cc @@ -476,8 +476,8 @@ struct CmdFlakeCheck : FlakeCommand if (!v.isLambda()) throw Error("bundler must be a function"); if (!v.lambda.fun->formals || - !v.lambda.fun->formals->argNames.count(state->symbols.create("program")) || - !v.lambda.fun->formals->argNames.count(state->symbols.create("system"))) + !v.lambda.fun->formals->has(state->symbols.create("program")) || + !v.lambda.fun->formals->has(state->symbols.create("system"))) throw Error("bundler must take formal arguments 'program' and 'system'"); } catch (Error & e) { e.addTrace(pos, hintfmt("while checking the template '%s'", attrPath)); diff --git a/tests/check.nix b/tests/check.nix index ec455ae2d..ed91ff845 100644 --- a/tests/check.nix +++ b/tests/check.nix @@ -50,6 +50,6 @@ with import ./config.nix; fetchurl = import { url = "file://" + toString ./lang/eval-okay-xml.exp.xml; - sha256 = "0kg4sla7ihm8ijr8cb3117fhl99zrc2bwy1jrngsfmkh8bav4m0v"; + sha256 = "sha256-behBlX+DQK/Pjvkuc8Tx68Jwi4E5v86wDq+ZLaHyhQE="; }; } diff --git a/tests/lang/eval-okay-xml.exp.xml b/tests/lang/eval-okay-xml.exp.xml index 92b75e0b8..20099326c 100644 --- a/tests/lang/eval-okay-xml.exp.xml +++ b/tests/lang/eval-okay-xml.exp.xml @@ -31,9 +31,9 @@ - + From 203ef26974d5edf9e67bce6153407414be42cf91 Mon Sep 17 00:00:00 2001 From: "lincoln auster [they/them]" Date: Tue, 25 Jan 2022 11:59:51 -0700 Subject: [PATCH 04/13] flakes: document nixConfig option Fixes #5988. --- src/nix/flake.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/nix/flake.md b/src/nix/flake.md index a8436bcaa..db6e20d4f 100644 --- a/src/nix/flake.md +++ b/src/nix/flake.md @@ -292,6 +292,12 @@ The following attributes are supported in `flake.nix`: value (e.g. `packages.x86_64-linux` must be an attribute set of derivations built for the `x86_64-linux` platform). +* `nixConfig`: a set of `nix.conf` options to be set when evaluating any + part of a flake. In the interests of security, only a small set of + whitelisted options (currently `bash-prompt`, `brash-prompt-suffix`, + and `flake-registry`) are allowed to be set without confirmation so long as + `accept-flake-config` is not set in the global configuration. + ## Flake inputs The attribute `inputs` specifies the dependencies of a flake, as an From c746a429db0e48e60362b6ea45b5d01551ba46d6 Mon Sep 17 00:00:00 2001 From: lincoln auster Date: Tue, 25 Jan 2022 14:55:49 -0700 Subject: [PATCH 05/13] fix typo Co-authored-by: Cole Helbling --- src/nix/flake.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/nix/flake.md b/src/nix/flake.md index db6e20d4f..accddd436 100644 --- a/src/nix/flake.md +++ b/src/nix/flake.md @@ -294,7 +294,7 @@ The following attributes are supported in `flake.nix`: * `nixConfig`: a set of `nix.conf` options to be set when evaluating any part of a flake. In the interests of security, only a small set of - whitelisted options (currently `bash-prompt`, `brash-prompt-suffix`, + whitelisted options (currently `bash-prompt`, `bash-prompt-suffix`, and `flake-registry`) are allowed to be set without confirmation so long as `accept-flake-config` is not set in the global configuration. From 0d7fae6a574ec1b6758a7e6d8e639145c1c465a9 Mon Sep 17 00:00:00 2001 From: pennae Date: Fri, 21 Jan 2022 17:55:51 +0100 Subject: [PATCH 06/13] convert a for more utilities to string_view --- src/libexpr/eval.hh | 4 ++-- src/libexpr/parser.y | 2 +- src/libstore/optimise-store.cc | 9 +++++---- src/libstore/parsed-derivations.cc | 2 +- src/libutil/util.cc | 20 ++++++++++--------- src/libutil/util.hh | 10 +++++++--- src/nix/develop.cc | 6 ++++-- .../resolve-system-dependencies.cc | 2 +- 8 files changed, 32 insertions(+), 23 deletions(-) diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index c59203aa5..67bdd4de4 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -309,8 +309,8 @@ private: friend struct ExprAttrs; friend struct ExprLet; - Expr * parse(char * text, size_t length, FileOrigin origin, const Path & path, - const Path & basePath, StaticEnv & staticEnv); + Expr * parse(char * text, size_t length, FileOrigin origin, const PathView path, + const PathView basePath, StaticEnv & staticEnv); public: diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 7a8e93c12..dd76fd66f 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -598,7 +598,7 @@ namespace nix { Expr * EvalState::parse(char * text, size_t length, FileOrigin origin, - const Path & path, const Path & basePath, StaticEnv & staticEnv) + const PathView path, const PathView basePath, StaticEnv & staticEnv) { yyscan_t scanner; ParseData data(*this); diff --git a/src/libstore/optimise-store.cc b/src/libstore/optimise-store.cc index 1833c954e..13cb142f8 100644 --- a/src/libstore/optimise-store.cc +++ b/src/libstore/optimise-store.cc @@ -26,7 +26,7 @@ static void makeWritable(const Path & path) struct MakeReadOnly { Path path; - MakeReadOnly(const Path & path) : path(path) { } + MakeReadOnly(const PathView path) : path(path) { } ~MakeReadOnly() { try { @@ -205,12 +205,13 @@ void LocalStore::optimisePath_(Activity * act, OptimiseStats & stats, /* Make the containing directory writable, but only if it's not the store itself (we don't want or need to mess with its permissions). */ - bool mustToggle = dirOf(path) != realStoreDir.get(); - if (mustToggle) makeWritable(dirOf(path)); + const Path dirOfPath(dirOf(path)); + bool mustToggle = dirOfPath != realStoreDir.get(); + if (mustToggle) makeWritable(dirOfPath); /* When we're done, make the directory read-only again and reset its timestamp back to 0. */ - MakeReadOnly makeReadOnly(mustToggle ? dirOf(path) : ""); + MakeReadOnly makeReadOnly(mustToggle ? dirOfPath : ""); Path tempLink = (format("%1%/.tmp-link-%2%-%3%") % realStoreDir % getpid() % random()).str(); diff --git a/src/libstore/parsed-derivations.cc b/src/libstore/parsed-derivations.cc index caddba9b1..8c65053e4 100644 --- a/src/libstore/parsed-derivations.cc +++ b/src/libstore/parsed-derivations.cc @@ -170,7 +170,7 @@ std::string writeStructuredAttrsShell(const nlohmann::json & json) auto handleSimpleType = [](const nlohmann::json & value) -> std::optional { if (value.is_string()) - return shellEscape(value); + return shellEscape(value.get()); if (value.is_number()) { auto f = value.get(); diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 1f1f2c861..692bcb180 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -81,7 +81,7 @@ void replaceEnv(std::map newEnv) } -Path absPath(Path path, std::optional dir, bool resolveSymlinks) +Path absPath(Path path, std::optional dir, bool resolveSymlinks) { if (path[0] != '/') { if (!dir) { @@ -95,12 +95,12 @@ Path absPath(Path path, std::optional dir, bool resolveSymlinks) if (!getcwd(buf, sizeof(buf))) #endif throw SysError("cannot get cwd"); - dir = buf; + path = concatStrings(buf, "/", path); #ifdef __GNU__ free(buf); #endif - } - path = *dir + "/" + path; + } else + path = concatStrings(*dir, "/", path); } return canonPath(path, resolveSymlinks); } @@ -172,7 +172,7 @@ Path canonPath(PathView path, bool resolveSymlinks) } -Path dirOf(const Path & path) +Path dirOf(const PathView path) { Path::size_type pos = path.rfind('/'); if (pos == string::npos) @@ -1344,9 +1344,11 @@ std::string toLower(const std::string & s) } -std::string shellEscape(const std::string & s) +std::string shellEscape(const std::string_view s) { - std::string r = "'"; + std::string r; + r.reserve(s.size() + 2); + r += "'"; for (auto & i : s) if (i == '\'') r += "'\\''"; else r += i; r += '\''; @@ -1751,7 +1753,7 @@ void bind(int fd, const std::string & path) if (path.size() + 1 >= sizeof(addr.sun_path)) { Pid pid = startProcess([&]() { - auto dir = dirOf(path); + Path dir = dirOf(path); if (chdir(dir.c_str()) == -1) throw SysError("chdir to '%s' failed", dir); std::string base(baseNameOf(path)); @@ -1780,7 +1782,7 @@ void connect(int fd, const std::string & path) if (path.size() + 1 >= sizeof(addr.sun_path)) { Pid pid = startProcess([&]() { - auto dir = dirOf(path); + Path dir = dirOf(path); if (chdir(dir.c_str()) == -1) throw SysError("chdir to '%s' failed", dir); std::string base(baseNameOf(path)); diff --git a/src/libutil/util.hh b/src/libutil/util.hh index 369c44f78..579a42785 100644 --- a/src/libutil/util.hh +++ b/src/libutil/util.hh @@ -49,7 +49,7 @@ void clearEnv(); specified directory, or the current directory otherwise. The path is also canonicalised. */ Path absPath(Path path, - std::optional dir = {}, + std::optional dir = {}, bool resolveSymlinks = false); /* Canonicalise a path by removing all `.' or `..' components and @@ -62,7 +62,7 @@ Path canonPath(PathView path, bool resolveSymlinks = false); everything before the final `/'. If the path is the root or an immediate child thereof (e.g., `/foo'), this means `/' is returned.*/ -Path dirOf(const Path & path); +Path dirOf(const PathView path); /* Return the base name of the given canonical path, i.e., everything following the final `/' (trailing slashes are removed). */ @@ -148,6 +148,9 @@ Path getDataDir(); /* Create a directory and all its parents, if necessary. Returns the list of created directories, in order of creation. */ Paths createDirs(const Path & path); +inline Paths createDirs(PathView path) { + return createDirs(Path(path)); +} /* Create a symlink. */ void createSymlink(const Path & target, const Path & link, @@ -187,6 +190,7 @@ public: void cancel(); void reset(const Path & p, bool recursive = true); operator Path() const { return path; } + operator PathView() const { return path; } }; @@ -491,7 +495,7 @@ std::string toLower(const std::string & s); /* Escape a string as a shell word. */ -std::string shellEscape(const std::string & s); +std::string shellEscape(const std::string_view s); /* Exception handling in destructors: print an error message, then diff --git a/src/nix/develop.cc b/src/nix/develop.cc index a8ca1cac2..42e13436a 100644 --- a/src/nix/develop.cc +++ b/src/nix/develop.cc @@ -472,9 +472,11 @@ struct CmdDevelop : Common, MixEnvironment else { script = "[ -n \"$PS1\" ] && [ -e ~/.bashrc ] && source ~/.bashrc;\n" + script; if (developSettings.bashPrompt != "") - script += fmt("[ -n \"$PS1\" ] && PS1=%s;\n", shellEscape(developSettings.bashPrompt)); + script += fmt("[ -n \"$PS1\" ] && PS1=%s;\n", + shellEscape(developSettings.bashPrompt.get())); if (developSettings.bashPromptSuffix != "") - script += fmt("[ -n \"$PS1\" ] && PS1+=%s;\n", shellEscape(developSettings.bashPromptSuffix)); + script += fmt("[ -n \"$PS1\" ] && PS1+=%s;\n", + shellEscape(developSettings.bashPromptSuffix.get())); } writeFull(rcFileFd.get(), script); diff --git a/src/resolve-system-dependencies/resolve-system-dependencies.cc b/src/resolve-system-dependencies/resolve-system-dependencies.cc index 27cf53a45..98c969437 100644 --- a/src/resolve-system-dependencies/resolve-system-dependencies.cc +++ b/src/resolve-system-dependencies/resolve-system-dependencies.cc @@ -107,7 +107,7 @@ Path resolveSymlink(const Path & path) auto target = readLink(path); return hasPrefix(target, "/") ? target - : dirOf(path) + "/" + target; + : concatStrings(dirOf(path), "/", target); } std::set resolveTree(const Path & path, PathSet & deps) From 41d70a2fc8d243d8c83ecc1c9ba648b625957437 Mon Sep 17 00:00:00 2001 From: pennae Date: Fri, 21 Jan 2022 14:44:00 +0100 Subject: [PATCH 07/13] return string_views from forceString* once a string has been forced we already have dynamic storage allocated for it, so we can easily reuse that storage instead of copying. --- src/libexpr/attr-path.cc | 2 +- src/libexpr/eval.cc | 12 ++-- src/libexpr/eval.hh | 10 ++-- src/libexpr/flake/flake.cc | 6 +- src/libexpr/get-drvs.cc | 2 +- src/libexpr/json-to-value.cc | 2 +- src/libexpr/json-to-value.hh | 2 +- src/libexpr/parser.y | 10 ++-- src/libexpr/primops.cc | 81 +++++++++++++++------------ src/libexpr/primops/context.cc | 2 +- src/libexpr/primops/fetchMercurial.cc | 8 +-- src/libexpr/primops/fromTOML.cc | 2 +- src/libstore/derivations.cc | 4 +- src/libstore/derivations.hh | 2 +- src/libstore/names.cc | 24 ++++---- src/libstore/names.hh | 6 +- src/libutil/hash.cc | 2 +- src/libutil/hash.hh | 2 +- src/nix/prefetch.cc | 2 +- 19 files changed, 94 insertions(+), 87 deletions(-) diff --git a/src/libexpr/attr-path.cc b/src/libexpr/attr-path.cc index c50c6d92b..edef4d9f8 100644 --- a/src/libexpr/attr-path.cc +++ b/src/libexpr/attr-path.cc @@ -121,7 +121,7 @@ Pos findPackageFilename(EvalState & state, Value & v, std::string what) std::string filename(pos, 0, colon); unsigned int lineno; try { - lineno = std::stoi(std::string(pos, colon + 1)); + lineno = std::stoi(std::string(pos, colon + 1, string::npos)); } catch (std::invalid_argument & e) { throw ParseError("cannot parse line number '%s'", pos); } diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index b884b4001..bfc5d3ebf 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1857,7 +1857,7 @@ void EvalState::forceFunction(Value & v, const Pos & pos) } -string EvalState::forceString(Value & v, const Pos & pos) +std::string_view EvalState::forceString(Value & v, const Pos & pos) { forceValue(v, pos); if (v.type() != nString) { @@ -1866,7 +1866,7 @@ string EvalState::forceString(Value & v, const Pos & pos) else throwTypeError("value is %1% while a string was expected", v); } - return string(v.string.s); + return v.string.s; } @@ -1901,17 +1901,17 @@ std::vector> Value::getContext() } -string EvalState::forceString(Value & v, PathSet & context, const Pos & pos) +std::string_view EvalState::forceString(Value & v, PathSet & context, const Pos & pos) { - string s = forceString(v, pos); + auto s = forceString(v, pos); copyContext(v, context); return s; } -string EvalState::forceStringNoCtx(Value & v, const Pos & pos) +std::string_view EvalState::forceStringNoCtx(Value & v, const Pos & pos) { - string s = forceString(v, pos); + auto s = forceString(v, pos); if (v.string.context) { if (pos) throwEvalError(pos, "the string '%1%' is not allowed to refer to a store path (such as '%2%')", diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 67bdd4de4..5b94a96ca 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -201,8 +201,8 @@ public: void resetFileCache(); /* Look up a file in the search path. */ - Path findFile(const string & path); - Path findFile(SearchPath & searchPath, const string & path, const Pos & pos = noPos); + Path findFile(const std::string_view path); + Path findFile(SearchPath & searchPath, const std::string_view path, const Pos & pos = noPos); /* If the specified search path element is a URI, download it. */ std::pair resolveSearchPathElem(const SearchPathElem & elem); @@ -236,9 +236,9 @@ public: inline void forceList(Value & v); inline void forceList(Value & v, const Pos & pos); void forceFunction(Value & v, const Pos & pos); // either lambda or primop - string forceString(Value & v, const Pos & pos = noPos); - string forceString(Value & v, PathSet & context, const Pos & pos = noPos); - string forceStringNoCtx(Value & v, const Pos & pos = noPos); + std::string_view forceString(Value & v, const Pos & pos = noPos); + std::string_view forceString(Value & v, PathSet & context, const Pos & pos = noPos); + std::string_view forceStringNoCtx(Value & v, const Pos & pos = noPos); /* Return true iff the value `v' denotes a derivation (i.e. a set with attribute `type = "derivation"'). */ diff --git a/src/libexpr/flake/flake.cc b/src/libexpr/flake/flake.cc index 0fbe9b960..809f54cc0 100644 --- a/src/libexpr/flake/flake.cc +++ b/src/libexpr/flake/flake.cc @@ -250,7 +250,7 @@ static Flake getFlake( for (auto & setting : *nixConfig->value->attrs) { forceTrivialValue(state, *setting.value, *setting.pos); if (setting.value->type() == nString) - flake.config.settings.insert({setting.name, state.forceStringNoCtx(*setting.value, *setting.pos)}); + flake.config.settings.insert({setting.name, string(state.forceStringNoCtx(*setting.value, *setting.pos))}); else if (setting.value->type() == nPath) { PathSet emptyContext = {}; flake.config.settings.insert({setting.name, state.coerceToString(*setting.pos, *setting.value, emptyContext, false, true, true)}); @@ -265,7 +265,7 @@ static Flake getFlake( if (elem->type() != nString) throw TypeError("list element in flake configuration setting '%s' is %s while a string is expected", setting.name, showType(*setting.value)); - ss.push_back(state.forceStringNoCtx(*elem, *setting.pos)); + ss.emplace_back(state.forceStringNoCtx(*elem, *setting.pos)); } flake.config.settings.insert({setting.name, ss}); } @@ -726,7 +726,7 @@ static void prim_getFlake(EvalState & state, const Pos & pos, Value * * args, Va { state.requireExperimentalFeatureOnEvaluation(Xp::Flakes, "builtins.getFlake", pos); - auto flakeRefS = state.forceStringNoCtx(*args[0], pos); + string flakeRefS(state.forceStringNoCtx(*args[0], pos)); auto flakeRef = parseFlakeRef(flakeRefS, {}, true); if (evalSettings.pureEval && !flakeRef.input.isImmutable()) throw Error("cannot call 'getFlake' on mutable flake reference '%s', at %s (use --impure to override)", flakeRefS, pos); diff --git a/src/libexpr/get-drvs.cc b/src/libexpr/get-drvs.cc index 25fd9b949..2651266b2 100644 --- a/src/libexpr/get-drvs.cc +++ b/src/libexpr/get-drvs.cc @@ -104,7 +104,7 @@ DrvInfo::Outputs DrvInfo::queryOutputs(bool onlyOutputsToInstall) /* For each output... */ for (auto elem : i->value->listItems()) { /* Evaluate the corresponding set. */ - string name = state->forceStringNoCtx(*elem, *i->pos); + string name(state->forceStringNoCtx(*elem, *i->pos)); Bindings::iterator out = attrs->find(state->symbols.create(name)); if (out == attrs->end()) continue; // FIXME: throw error? state->forceAttrs(*out->value); diff --git a/src/libexpr/json-to-value.cc b/src/libexpr/json-to-value.cc index 88716250c..99a475ff9 100644 --- a/src/libexpr/json-to-value.cc +++ b/src/libexpr/json-to-value.cc @@ -163,7 +163,7 @@ public: } }; -void parseJSON(EvalState & state, const string & s_, Value & v) +void parseJSON(EvalState & state, const std::string_view & s_, Value & v) { JSONSax parser(state, v); bool res = json::sax_parse(s_, &parser); diff --git a/src/libexpr/json-to-value.hh b/src/libexpr/json-to-value.hh index 3b0fdae11..84bec4eba 100644 --- a/src/libexpr/json-to-value.hh +++ b/src/libexpr/json-to-value.hh @@ -8,6 +8,6 @@ namespace nix { MakeError(JSONParseError, EvalError); -void parseJSON(EvalState & state, const string & s, Value & v); +void parseJSON(EvalState & state, const std::string_view & s, Value & v); } diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index dd76fd66f..b7910da8c 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -709,24 +709,24 @@ void EvalState::addToSearchPath(const string & s) } -Path EvalState::findFile(const string & path) +Path EvalState::findFile(const std::string_view path) { return findFile(searchPath, path); } -Path EvalState::findFile(SearchPath & searchPath, const string & path, const Pos & pos) +Path EvalState::findFile(SearchPath & searchPath, const std::string_view path, const Pos & pos) { for (auto & i : searchPath) { std::string suffix; if (i.first.empty()) - suffix = "/" + path; + suffix = concatStrings("/", path); else { auto s = i.first.size(); if (path.compare(0, s, i.first) != 0 || (path.size() > s && path[s] != '/')) continue; - suffix = path.size() == s ? "" : "/" + string(path, s); + suffix = path.size() == s ? "" : concatStrings("/", path.substr(s)); } auto r = resolveSearchPathElem(i); if (!r.first) continue; @@ -735,7 +735,7 @@ Path EvalState::findFile(SearchPath & searchPath, const string & path, const Pos } if (hasPrefix(path, "nix/")) - return corepkgsPrefix + path.substr(4); + return concatStrings(corepkgsPrefix, path.substr(4)); throw ThrownError({ .msg = hintfmt(evalSettings.pureEval diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index acee71d19..0454acc3d 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -314,7 +314,7 @@ void prim_importNative(EvalState & state, const Pos & pos, Value * * args, Value { auto path = realisePath(state, pos, *args[0]); - string sym = state.forceStringNoCtx(*args[1], pos); + string sym(state.forceStringNoCtx(*args[1], pos)); void *handle = dlopen(path.c_str(), RTLD_LAZY | RTLD_LOCAL); if (!handle) @@ -825,7 +825,7 @@ static RegisterPrimOp primop_tryEval({ /* Return an environment variable. Use with care. */ static void prim_getEnv(EvalState & state, const Pos & pos, Value * * args, Value & v) { - string name = state.forceStringNoCtx(*args[0], pos); + string name(state.forceStringNoCtx(*args[0], pos)); v.mkString(evalSettings.restrictEval || evalSettings.pureEval ? "" : getEnv(name).value_or("")); } @@ -975,7 +975,7 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * const string & key = i->name; vomit("processing attribute '%1%'", key); - auto handleHashMode = [&](const std::string & s) { + auto handleHashMode = [&](const std::string_view s) { if (s == "recursive") ingestionMethod = FileIngestionMethod::Recursive; else if (s == "flat") ingestionMethod = FileIngestionMethod::Flat; else @@ -1502,7 +1502,7 @@ static void prim_findFile(EvalState & state, const Pos & pos, Value * * args, Va searchPath.emplace_back(prefix, path); } - string path = state.forceStringNoCtx(*args[1], pos); + auto path = state.forceStringNoCtx(*args[1], pos); v.mkPath(state.checkSourcePath(state.findFile(searchPath, path, pos))); } @@ -1516,7 +1516,7 @@ static RegisterPrimOp primop_findFile(RegisterPrimOp::Info { /* Return the cryptographic hash of a file in base-16. */ static void prim_hashFile(EvalState & state, const Pos & pos, Value * * args, Value & v) { - string type = state.forceStringNoCtx(*args[0], pos); + auto type = state.forceStringNoCtx(*args[0], pos); std::optional ht = parseHashType(type); if (!ht) throw Error({ @@ -1723,7 +1723,7 @@ static RegisterPrimOp primop_toJSON({ /* Parse a JSON string to a value. */ static void prim_fromJSON(EvalState & state, const Pos & pos, Value * * args, Value & v) { - string s = state.forceStringNoCtx(*args[0], pos); + auto s = state.forceStringNoCtx(*args[0], pos); try { parseJSON(state, s, v); } catch (JSONParseError &e) { @@ -1752,8 +1752,8 @@ static RegisterPrimOp primop_fromJSON({ static void prim_toFile(EvalState & state, const Pos & pos, Value * * args, Value & v) { PathSet context; - string name = state.forceStringNoCtx(*args[0], pos); - string contents = state.forceString(*args[1], context, pos); + string name(state.forceStringNoCtx(*args[0], pos)); + string contents(state.forceString(*args[1], context, pos)); StorePathSet refs; @@ -2153,7 +2153,7 @@ static RegisterPrimOp primop_attrValues({ /* Dynamic version of the `.' operator. */ void prim_getAttr(EvalState & state, const Pos & pos, Value * * args, Value & v) { - string attr = state.forceStringNoCtx(*args[0], pos); + auto attr = state.forceStringNoCtx(*args[0], pos); state.forceAttrs(*args[1], pos); Bindings::iterator i = getAttr( state, @@ -2183,7 +2183,7 @@ static RegisterPrimOp primop_getAttr({ /* Return position information of the specified attribute. */ static void prim_unsafeGetAttrPos(EvalState & state, const Pos & pos, Value * * args, Value & v) { - string attr = state.forceStringNoCtx(*args[0], pos); + auto attr = state.forceStringNoCtx(*args[0], pos); state.forceAttrs(*args[1], pos); Bindings::iterator i = args[1]->attrs->find(state.symbols.create(attr)); if (i == args[1]->attrs->end()) @@ -2201,7 +2201,7 @@ static RegisterPrimOp primop_unsafeGetAttrPos(RegisterPrimOp::Info { /* Dynamic version of the `?' operator. */ static void prim_hasAttr(EvalState & state, const Pos & pos, Value * * args, Value & v) { - string attr = state.forceStringNoCtx(*args[0], pos); + auto attr = state.forceStringNoCtx(*args[0], pos); state.forceAttrs(*args[1], pos); v.mkBool(args[1]->attrs->find(state.symbols.create(attr)) != args[1]->attrs->end()); } @@ -2300,7 +2300,7 @@ static void prim_listToAttrs(EvalState & state, const Pos & pos, Value * * args, pos ); - string name = state.forceStringNoCtx(*j->value, *j->pos); + auto name = state.forceStringNoCtx(*j->value, *j->pos); Symbol sym = state.symbols.create(name); if (seen.insert(sym).second) { @@ -3032,7 +3032,7 @@ static void prim_groupBy(EvalState & state, const Pos & pos, Value * * args, Val for (auto vElem : args[1]->listItems()) { Value res; state.callFunction(*args[0], *vElem, res, pos); - string name = state.forceStringNoCtx(res, pos); + auto name = state.forceStringNoCtx(res, pos); Symbol sym = state.symbols.create(name); auto vector = attrs.try_emplace(sym, ValueVector()).first; vector->second.push_back(vElem); @@ -3376,7 +3376,7 @@ static RegisterPrimOp primop_stringLength({ /* Return the cryptographic hash of a string in base-16. */ static void prim_hashString(EvalState & state, const Pos & pos, Value * * args, Value & v) { - string type = state.forceStringNoCtx(*args[0], pos); + auto type = state.forceStringNoCtx(*args[0], pos); std::optional ht = parseHashType(type); if (!ht) throw Error({ @@ -3385,7 +3385,7 @@ static void prim_hashString(EvalState & state, const Pos & pos, Value * * args, }); PathSet context; // discarded - string s = state.forceString(*args[1], context, pos); + auto s = state.forceString(*args[1], context, pos); v.mkString(hashString(*ht, s).to_string(Base16, false)); } @@ -3403,7 +3403,18 @@ static RegisterPrimOp primop_hashString({ struct RegexCache { - std::unordered_map cache; + // TODO use C++20 transparent comparison when available + std::unordered_map cache; + std::list keys; + + std::regex get(std::string_view re) + { + auto it = cache.find(re); + if (it != cache.end()) + return it->second; + keys.emplace_back(re); + return cache.emplace(keys.back(), std::regex(keys.back(), std::regex::extended)).first->second; + } }; std::shared_ptr makeRegexCache() @@ -3417,15 +3428,13 @@ void prim_match(EvalState & state, const Pos & pos, Value * * args, Value & v) try { - auto regex = state.regexCache->cache.find(re); - if (regex == state.regexCache->cache.end()) - regex = state.regexCache->cache.emplace(re, std::regex(re, std::regex::extended)).first; + auto regex = state.regexCache->get(re); PathSet context; - const std::string str = state.forceString(*args[1], context, pos); + const auto str = state.forceString(*args[1], context, pos); - std::smatch match; - if (!std::regex_match(str, match, regex->second)) { + std::cmatch match; + if (!std::regex_match(str.begin(), str.end(), match, regex)) { v.mkNull(); return; } @@ -3500,15 +3509,13 @@ void prim_split(EvalState & state, const Pos & pos, Value * * args, Value & v) try { - auto regex = state.regexCache->cache.find(re); - if (regex == state.regexCache->cache.end()) - regex = state.regexCache->cache.emplace(re, std::regex(re, std::regex::extended)).first; + auto regex = state.regexCache->get(re); PathSet context; - const std::string str = state.forceString(*args[1], context, pos); + const auto str = state.forceString(*args[1], context, pos); - auto begin = std::sregex_iterator(str.begin(), str.end(), regex->second); - auto end = std::sregex_iterator(); + auto begin = std::cregex_iterator(str.begin(), str.end(), regex); + auto end = std::cregex_iterator(); // Any matches results are surrounded by non-matching results. const size_t len = std::distance(begin, end); @@ -3520,9 +3527,9 @@ void prim_split(EvalState & state, const Pos & pos, Value * * args, Value & v) return; } - for (std::sregex_iterator i = begin; i != end; ++i) { + for (auto i = begin; i != end; ++i) { assert(idx <= 2 * len + 1 - 3); - std::smatch match = *i; + auto match = *i; // Add a string for non-matched characters. (v.listElems()[idx++] = state.allocValue())->mkString(match.prefix().str()); @@ -3643,14 +3650,14 @@ static void prim_replaceStrings(EvalState & state, const Pos & pos, Value * * ar vector from; from.reserve(args[0]->listSize()); for (auto elem : args[0]->listItems()) - from.push_back(state.forceString(*elem, pos)); + from.emplace_back(state.forceString(*elem, pos)); vector> to; to.reserve(args[1]->listSize()); for (auto elem : args[1]->listItems()) { PathSet ctx; auto s = state.forceString(*elem, ctx, pos); - to.push_back(std::make_pair(std::move(s), std::move(ctx))); + to.emplace_back(s, std::move(ctx)); } PathSet context; @@ -3712,7 +3719,7 @@ static RegisterPrimOp primop_replaceStrings({ static void prim_parseDrvName(EvalState & state, const Pos & pos, Value * * args, Value & v) { - string name = state.forceStringNoCtx(*args[0], pos); + auto name = state.forceStringNoCtx(*args[0], pos); DrvName parsed(name); auto attrs = state.buildBindings(2); attrs.alloc(state.sName).mkString(parsed.name); @@ -3736,8 +3743,8 @@ static RegisterPrimOp primop_parseDrvName({ static void prim_compareVersions(EvalState & state, const Pos & pos, Value * * args, Value & v) { - string version1 = state.forceStringNoCtx(*args[0], pos); - string version2 = state.forceStringNoCtx(*args[1], pos); + auto version1 = state.forceStringNoCtx(*args[0], pos); + auto version2 = state.forceStringNoCtx(*args[1], pos); v.mkInt(compareVersions(version1, version2)); } @@ -3756,14 +3763,14 @@ static RegisterPrimOp primop_compareVersions({ static void prim_splitVersion(EvalState & state, const Pos & pos, Value * * args, Value & v) { - string version = state.forceStringNoCtx(*args[0], pos); + auto version = state.forceStringNoCtx(*args[0], pos); auto iter = version.cbegin(); Strings components; while (iter != version.cend()) { auto component = nextComponent(iter, version.cend()); if (component.empty()) break; - components.emplace_back(std::move(component)); + components.emplace_back(component); } state.mkList(v, components.size()); for (const auto & [n, component] : enumerate(components)) diff --git a/src/libexpr/primops/context.cc b/src/libexpr/primops/context.cc index a239c06da..cd7eeb588 100644 --- a/src/libexpr/primops/context.cc +++ b/src/libexpr/primops/context.cc @@ -180,7 +180,7 @@ static void prim_appendContext(EvalState & state, const Pos & pos, Value * * arg } for (auto elem : iter->value->listItems()) { auto name = state.forceStringNoCtx(*elem, *iter->pos); - context.insert("!" + name + "!" + string(i.name)); + context.insert(concatStrings("!", name, "!", i.name)); } } } diff --git a/src/libexpr/primops/fetchMercurial.cc b/src/libexpr/primops/fetchMercurial.cc index 42214c207..f808e2da5 100644 --- a/src/libexpr/primops/fetchMercurial.cc +++ b/src/libexpr/primops/fetchMercurial.cc @@ -12,7 +12,7 @@ static void prim_fetchMercurial(EvalState & state, const Pos & pos, Value * * ar std::string url; std::optional rev; std::optional ref; - std::string name = "source"; + std::string_view name = "source"; PathSet context; state.forceValue(*args[0], pos); @@ -22,14 +22,14 @@ static void prim_fetchMercurial(EvalState & state, const Pos & pos, Value * * ar state.forceAttrs(*args[0], pos); for (auto & attr : *args[0]->attrs) { - string n(attr.name); + std::string_view n(attr.name); if (n == "url") url = state.coerceToString(*attr.pos, *attr.value, context, false, false); else if (n == "rev") { // Ugly: unlike fetchGit, here the "rev" attribute can // be both a revision or a branch/tag name. auto value = state.forceStringNoCtx(*attr.value, *attr.pos); - if (std::regex_match(value, revRegex)) + if (std::regex_match(value.begin(), value.end(), revRegex)) rev = Hash::parseAny(value, htSHA1); else ref = value; @@ -62,7 +62,7 @@ static void prim_fetchMercurial(EvalState & state, const Pos & pos, Value * * ar fetchers::Attrs attrs; attrs.insert_or_assign("type", "hg"); attrs.insert_or_assign("url", url.find("://") != std::string::npos ? url : "file://" + url); - attrs.insert_or_assign("name", name); + attrs.insert_or_assign("name", string(name)); if (ref) attrs.insert_or_assign("ref", *ref); if (rev) attrs.insert_or_assign("rev", rev->gitRev()); auto input = fetchers::Input::fromAttrs(std::move(attrs)); diff --git a/src/libexpr/primops/fromTOML.cc b/src/libexpr/primops/fromTOML.cc index 80c7e0b82..c0e858b61 100644 --- a/src/libexpr/primops/fromTOML.cc +++ b/src/libexpr/primops/fromTOML.cc @@ -9,7 +9,7 @@ static void prim_fromTOML(EvalState & state, const Pos & pos, Value * * args, Va { auto toml = state.forceStringNoCtx(*args[0], pos); - std::istringstream tomlStream(toml); + std::istringstream tomlStream(string{toml}); std::function visit; diff --git a/src/libstore/derivations.cc b/src/libstore/derivations.cc index 3e3d50144..40af6a775 100644 --- a/src/libstore/derivations.cc +++ b/src/libstore/derivations.cc @@ -699,10 +699,10 @@ void writeDerivation(Sink & out, const Store & store, const BasicDerivation & dr } -std::string hashPlaceholder(const std::string & outputName) +std::string hashPlaceholder(const std::string_view outputName) { // FIXME: memoize? - return "/" + hashString(htSHA256, "nix-output:" + outputName).to_string(Base32, false); + return "/" + hashString(htSHA256, concatStrings("nix-output:", outputName)).to_string(Base32, false); } std::string downstreamPlaceholder(const Store & store, const StorePath & drvPath, std::string_view outputName) diff --git a/src/libstore/derivations.hh b/src/libstore/derivations.hh index b1cb68194..a644cec60 100644 --- a/src/libstore/derivations.hh +++ b/src/libstore/derivations.hh @@ -236,7 +236,7 @@ void writeDerivation(Sink & out, const Store & store, const BasicDerivation & dr It is used as a placeholder to allow derivations to refer to their own outputs without needing to use the hash of a derivation in itself, making the hash near-impossible to calculate. */ -std::string hashPlaceholder(const std::string & outputName); +std::string hashPlaceholder(const std::string_view outputName); /* This creates an opaque and almost certainly unique string deterministically from a derivation path and output name. diff --git a/src/libstore/names.cc b/src/libstore/names.cc index 54c95055d..277aabf0f 100644 --- a/src/libstore/names.cc +++ b/src/libstore/names.cc @@ -56,8 +56,8 @@ bool DrvName::matches(const DrvName & n) } -string nextComponent(string::const_iterator & p, - const string::const_iterator end) +std::string_view nextComponent(std::string_view::const_iterator & p, + const std::string_view::const_iterator end) { /* Skip any dots and dashes (component separators). */ while (p != end && (*p == '.' || *p == '-')) ++p; @@ -67,18 +67,18 @@ string nextComponent(string::const_iterator & p, /* If the first character is a digit, consume the longest sequence of digits. Otherwise, consume the longest sequence of non-digit, non-separator characters. */ - string s; + auto s = p; if (isdigit(*p)) - while (p != end && isdigit(*p)) s += *p++; + while (p != end && isdigit(*p)) p++; else while (p != end && (!isdigit(*p) && *p != '.' && *p != '-')) - s += *p++; + p++; - return s; + return {s, size_t(p - s)}; } -static bool componentsLT(const string & c1, const string & c2) +static bool componentsLT(const std::string_view c1, const std::string_view c2) { auto n1 = string2Int(c1); auto n2 = string2Int(c2); @@ -94,14 +94,14 @@ static bool componentsLT(const string & c1, const string & c2) } -int compareVersions(const string & v1, const string & v2) +int compareVersions(const std::string_view v1, const std::string_view v2) { - string::const_iterator p1 = v1.begin(); - string::const_iterator p2 = v2.begin(); + auto p1 = v1.begin(); + auto p2 = v2.begin(); while (p1 != v1.end() || p2 != v2.end()) { - string c1 = nextComponent(p1, v1.end()); - string c2 = nextComponent(p2, v2.end()); + auto c1 = nextComponent(p1, v1.end()); + auto c2 = nextComponent(p2, v2.end()); if (componentsLT(c1, c2)) return -1; else if (componentsLT(c2, c1)) return 1; } diff --git a/src/libstore/names.hh b/src/libstore/names.hh index 3f861bc44..6f01fe2a1 100644 --- a/src/libstore/names.hh +++ b/src/libstore/names.hh @@ -27,9 +27,9 @@ private: typedef list DrvNames; -string nextComponent(string::const_iterator & p, - const string::const_iterator end); -int compareVersions(const string & v1, const string & v2); +std::string_view nextComponent(std::string_view::const_iterator & p, + const std::string_view::const_iterator end); +int compareVersions(const std::string_view v1, const std::string_view v2); DrvNames drvNamesFromArgs(const Strings & opArgs); } diff --git a/src/libutil/hash.cc b/src/libutil/hash.cc index 4df8b4ecb..6ed00d43c 100644 --- a/src/libutil/hash.cc +++ b/src/libutil/hash.cc @@ -259,7 +259,7 @@ Hash::Hash(std::string_view rest, HashType type, bool isSRI) throw BadHash("hash '%s' has wrong length for hash type '%s'", rest, printHashType(this->type)); } -Hash newHashAllowEmpty(std::string hashStr, std::optional ht) +Hash newHashAllowEmpty(std::string_view hashStr, std::optional ht) { if (hashStr.empty()) { if (!ht) diff --git a/src/libutil/hash.hh b/src/libutil/hash.hh index 1b626dd85..dff46542f 100644 --- a/src/libutil/hash.hh +++ b/src/libutil/hash.hh @@ -107,7 +107,7 @@ public: }; /* Helper that defaults empty hashes to the 0 hash. */ -Hash newHashAllowEmpty(std::string hashStr, std::optional ht); +Hash newHashAllowEmpty(std::string_view hashStr, std::optional ht); /* Print a hash in base-16 if it's MD5, or base-32 otherwise. */ string printHash16or32(const Hash & hash); diff --git a/src/nix/prefetch.cc b/src/nix/prefetch.cc index 768d37595..669be0709 100644 --- a/src/nix/prefetch.cc +++ b/src/nix/prefetch.cc @@ -38,7 +38,7 @@ string resolveMirrorUrl(EvalState & state, string url) if (mirrorList->value->listSize() < 1) throw Error("mirror URL '%s' did not expand to anything", url); - auto mirror = state.forceString(*mirrorList->value->listElems()[0]); + string mirror(state.forceString(*mirrorList->value->listElems()[0])); return mirror + (hasSuffix(mirror, "/") ? "" : "/") + string(s, p + 1); } From d439dceb3bc47f10a6f1f5b8cf4a5b17adc80071 Mon Sep 17 00:00:00 2001 From: pennae Date: Fri, 21 Jan 2022 16:20:54 +0100 Subject: [PATCH 08/13] optionally return string_view from coerceToString we'll retain the old coerceToString interface that returns a string, but callers that don't need the returned value to outlive the Value it came from can save copies by using the new interface instead. for values that weren't stringy we'll pass a new buffer argument that'll be used for storage and shouldn't be inspected. --- src/libexpr/eval.cc | 48 +++++++++++---------- src/libexpr/eval.hh | 3 +- src/libexpr/flake/flake.cc | 4 +- src/libexpr/primops.cc | 38 +++++++++-------- src/libexpr/primops/context.cc | 7 ++-- src/libexpr/primops/fetchMercurial.cc | 4 +- src/libexpr/primops/fetchTree.cc | 4 +- src/libutil/types.hh | 60 +++++++++++++++++++++++++++ src/nix/eval.cc | 2 +- src/nix/repl.cc | 2 +- 10 files changed, 121 insertions(+), 51 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index bfc5d3ebf..3332dd703 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1,5 +1,6 @@ #include "eval.hh" #include "hash.hh" +#include "types.hh" #include "util.hh" #include "store-api.hh" #include "derivations.hh" @@ -1694,7 +1695,7 @@ void EvalState::concatLists(Value & v, size_t nrLists, Value * * lists, const Po void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v) { PathSet context; - std::vector s; + std::vector s; size_t sSize = 0; NixInt n = 0; NixFloat nf = 0; @@ -1705,7 +1706,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v) const auto str = [&] { std::string result; result.reserve(sSize); - for (const auto & part : s) result += part; + for (const auto & part : s) result += *part; return result; }; /* c_str() is not str().c_str() because we want to create a string @@ -1715,15 +1716,18 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v) char * result = allocString(sSize + 1); char * tmp = result; for (const auto & part : s) { - memcpy(tmp, part.c_str(), part.size()); - tmp += part.size(); + memcpy(tmp, part->data(), part->size()); + tmp += part->size(); } *tmp = 0; return result; }; + Value values[es->size()]; + Value * vTmpP = values; + for (auto & [i_pos, i] : *es) { - Value vTmp; + Value & vTmp = *vTmpP++; i->eval(state, env, vTmp); /* If the first element is a path, then the result will also @@ -1756,9 +1760,9 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v) /* skip canonization of first path, which would only be not canonized in the first place if it's coming from a ./${foo} type path */ - s.emplace_back( - state.coerceToString(i_pos, vTmp, context, false, firstType == nString, !first)); - sSize += s.back().size(); + auto part = state.coerceToString(i_pos, vTmp, context, false, firstType == nString, !first); + sSize += part->size(); + s.emplace_back(std::move(part)); } first = false; @@ -1942,34 +1946,35 @@ std::optional EvalState::tryAttrsToString(const Pos & pos, Value & v, if (i != v.attrs->end()) { Value v1; callFunction(*i->value, v, v1, pos); - return coerceToString(pos, v1, context, coerceMore, copyToStore); + return coerceToString(pos, v1, context, coerceMore, copyToStore).toOwned(); } return {}; } -string EvalState::coerceToString(const Pos & pos, Value & v, PathSet & context, +BackedStringView EvalState::coerceToString(const Pos & pos, Value & v, PathSet & context, bool coerceMore, bool copyToStore, bool canonicalizePath) { forceValue(v, pos); - string s; - if (v.type() == nString) { copyContext(v, context); - return v.string.s; + return std::string_view(v.string.s); } if (v.type() == nPath) { - Path path(canonicalizePath ? canonPath(v.path) : v.path); - return copyToStore ? copyPathToStore(context, path) : path; + BackedStringView path(PathView(v.path)); + if (canonicalizePath) + path = canonPath(*path); + if (copyToStore) + path = copyPathToStore(context, std::move(path).toOwned()); + return path; } if (v.type() == nAttrs) { auto maybeString = tryAttrsToString(pos, v, context, coerceMore, copyToStore); - if (maybeString) { - return *maybeString; - } + if (maybeString) + return std::move(*maybeString); auto i = v.attrs->find(sOutPath); if (i == v.attrs->end()) throwTypeError(pos, "cannot coerce a set to a string"); return coerceToString(pos, *i->value, context, coerceMore, copyToStore); @@ -1991,14 +1996,13 @@ string EvalState::coerceToString(const Pos & pos, Value & v, PathSet & context, if (v.isList()) { string result; for (auto [n, v2] : enumerate(v.listItems())) { - result += coerceToString(pos, *v2, - context, coerceMore, copyToStore); + result += *coerceToString(pos, *v2, context, coerceMore, copyToStore); if (n < v.listSize() - 1 /* !!! not quite correct */ && (!v2->isList() || v2->listSize() != 0)) result += " "; } - return result; + return std::move(result); } } @@ -2032,7 +2036,7 @@ string EvalState::copyPathToStore(PathSet & context, const Path & path) Path EvalState::coerceToPath(const Pos & pos, Value & v, PathSet & context) { - string path = coerceToString(pos, v, context, false, false); + string path = coerceToString(pos, v, context, false, false).toOwned(); if (path == "" || path[0] != '/') throwEvalError(pos, "string '%1%' doesn't represent an absolute path", path); return path; diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh index 5b94a96ca..04acc5728 100644 --- a/src/libexpr/eval.hh +++ b/src/libexpr/eval.hh @@ -1,6 +1,7 @@ #pragma once #include "attr-set.hh" +#include "types.hh" #include "value.hh" #include "nixexpr.hh" #include "symbol-table.hh" @@ -251,7 +252,7 @@ public: string. If `coerceMore' is set, also converts nulls, integers, booleans and lists to a string. If `copyToStore' is set, referenced paths are copied to the Nix store as a side effect. */ - string coerceToString(const Pos & pos, Value & v, PathSet & context, + BackedStringView coerceToString(const Pos & pos, Value & v, PathSet & context, bool coerceMore = false, bool copyToStore = true, bool canonicalizePath = true); diff --git a/src/libexpr/flake/flake.cc b/src/libexpr/flake/flake.cc index 809f54cc0..27ba3288b 100644 --- a/src/libexpr/flake/flake.cc +++ b/src/libexpr/flake/flake.cc @@ -253,7 +253,9 @@ static Flake getFlake( flake.config.settings.insert({setting.name, string(state.forceStringNoCtx(*setting.value, *setting.pos))}); else if (setting.value->type() == nPath) { PathSet emptyContext = {}; - flake.config.settings.insert({setting.name, state.coerceToString(*setting.pos, *setting.value, emptyContext, false, true, true)}); + flake.config.settings.emplace( + setting.name, + state.coerceToString(*setting.pos, *setting.value, emptyContext, false, true, true) .toOwned()); } else if (setting.value->type() == nInt) flake.config.settings.insert({setting.name, state.forceInt(*setting.value, *setting.pos)}); diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index 0454acc3d..b07133d5e 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -350,10 +350,11 @@ void prim_exec(EvalState & state, const Pos & pos, Value * * args, Value & v) }); } PathSet context; - auto program = state.coerceToString(pos, *elems[0], context, false, false); + auto program = state.coerceToString(pos, *elems[0], context, false, false).toOwned(); Strings commandArgs; - for (unsigned int i = 1; i < args[0]->listSize(); ++i) - commandArgs.emplace_back(state.coerceToString(pos, *elems[i], context, false, false)); + for (unsigned int i = 1; i < args[0]->listSize(); ++i) { + commandArgs.push_back(state.coerceToString(pos, *elems[i], context, false, false).toOwned()); + } try { auto _ = state.realiseContext(context); // FIXME: Handle CA derivations } catch (InvalidPathError & e) { @@ -706,7 +707,7 @@ static RegisterPrimOp primop_abort({ .fun = [](EvalState & state, const Pos & pos, Value * * args, Value & v) { PathSet context; - string s = state.coerceToString(pos, *args[0], context); + string s = state.coerceToString(pos, *args[0], context).toOwned(); throw Abort("evaluation aborted with the following error message: '%1%'", s); } }); @@ -724,7 +725,7 @@ static RegisterPrimOp primop_throw({ .fun = [](EvalState & state, const Pos & pos, Value * * args, Value & v) { PathSet context; - string s = state.coerceToString(pos, *args[0], context); + string s = state.coerceToString(pos, *args[0], context).toOwned(); throw ThrownError(s); } }); @@ -736,7 +737,7 @@ static void prim_addErrorContext(EvalState & state, const Pos & pos, Value * * a v = *args[1]; } catch (Error & e) { PathSet context; - e.addTrace(std::nullopt, state.coerceToString(pos, *args[0], context)); + e.addTrace(std::nullopt, state.coerceToString(pos, *args[0], context).toOwned()); throw; } } @@ -1030,7 +1031,7 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * else if (i->name == state.sArgs) { state.forceList(*i->value, pos); for (auto elem : i->value->listItems()) { - string s = state.coerceToString(posDrvName, *elem, context, true); + string s = state.coerceToString(posDrvName, *elem, context, true).toOwned(); drv.args.push_back(s); } } @@ -1066,7 +1067,7 @@ static void prim_derivationStrict(EvalState & state, const Pos & pos, Value * * } } else { - auto s = state.coerceToString(*i->pos, *i->value, context, true); + auto s = state.coerceToString(*i->pos, *i->value, context, true).toOwned(); drv.env.emplace(key, s); if (i->name == state.sBuilder) drv.builder = std::move(s); else if (i->name == state.sSystem) drv.platform = std::move(s); @@ -1399,7 +1400,7 @@ static RegisterPrimOp primop_pathExists({ static void prim_baseNameOf(EvalState & state, const Pos & pos, Value * * args, Value & v) { PathSet context; - v.mkString(baseNameOf(state.coerceToString(pos, *args[0], context, false, false)), context); + v.mkString(baseNameOf(*state.coerceToString(pos, *args[0], context, false, false)), context); } static RegisterPrimOp primop_baseNameOf({ @@ -1419,7 +1420,8 @@ static RegisterPrimOp primop_baseNameOf({ static void prim_dirOf(EvalState & state, const Pos & pos, Value * * args, Value & v) { PathSet context; - Path dir = dirOf(state.coerceToString(pos, *args[0], context, false, false)); + auto path = state.coerceToString(pos, *args[0], context, false, false); + auto dir = dirOf(*path); if (args[0]->type() == nPath) v.mkPath(dir); else v.mkString(dir, context); } @@ -1486,7 +1488,7 @@ static void prim_findFile(EvalState & state, const Pos & pos, Value * * args, Va ); PathSet context; - string path = state.coerceToString(pos, *i->value, context, false, false); + string path = state.coerceToString(pos, *i->value, context, false, false).toOwned(); try { auto rewrites = state.realiseContext(context); @@ -3288,8 +3290,8 @@ static RegisterPrimOp primop_lessThan({ static void prim_toString(EvalState & state, const Pos & pos, Value * * args, Value & v) { PathSet context; - string s = state.coerceToString(pos, *args[0], context, true, false); - v.mkString(s, context); + auto s = state.coerceToString(pos, *args[0], context, true, false); + v.mkString(*s, context); } static RegisterPrimOp primop_toString({ @@ -3325,7 +3327,7 @@ static void prim_substring(EvalState & state, const Pos & pos, Value * * args, V int start = state.forceInt(*args[0], pos); int len = state.forceInt(*args[1], pos); PathSet context; - string s = state.coerceToString(pos, *args[2], context); + auto s = state.coerceToString(pos, *args[2], context); if (start < 0) throw EvalError({ @@ -3333,7 +3335,7 @@ static void prim_substring(EvalState & state, const Pos & pos, Value * * args, V .errPos = pos }); - v.mkString((unsigned int) start >= s.size() ? "" : string(s, start, len), context); + v.mkString((unsigned int) start >= s->size() ? "" : s->substr(start, len), context); } static RegisterPrimOp primop_substring({ @@ -3359,8 +3361,8 @@ static RegisterPrimOp primop_substring({ static void prim_stringLength(EvalState & state, const Pos & pos, Value * * args, Value & v) { PathSet context; - string s = state.coerceToString(pos, *args[0], context); - v.mkInt(s.size()); + auto s = state.coerceToString(pos, *args[0], context); + v.mkInt(s->size()); } static RegisterPrimOp primop_stringLength({ @@ -3620,7 +3622,7 @@ static void prim_concatStringsSep(EvalState & state, const Pos & pos, Value * * for (auto elem : args[1]->listItems()) { if (first) first = false; else res += sep; - res += state.coerceToString(pos, *elem, context); + res += *state.coerceToString(pos, *elem, context); } v.mkString(res, context); diff --git a/src/libexpr/primops/context.cc b/src/libexpr/primops/context.cc index cd7eeb588..654251c23 100644 --- a/src/libexpr/primops/context.cc +++ b/src/libexpr/primops/context.cc @@ -7,7 +7,8 @@ namespace nix { static void prim_unsafeDiscardStringContext(EvalState & state, const Pos & pos, Value * * args, Value & v) { PathSet context; - v.mkString(state.coerceToString(pos, *args[0], context)); + auto s = state.coerceToString(pos, *args[0], context); + v.mkString(*s); } static RegisterPrimOp primop_unsafeDiscardStringContext("__unsafeDiscardStringContext", 1, prim_unsafeDiscardStringContext); @@ -32,13 +33,13 @@ static RegisterPrimOp primop_hasContext("__hasContext", 1, prim_hasContext); static void prim_unsafeDiscardOutputDependency(EvalState & state, const Pos & pos, Value * * args, Value & v) { PathSet context; - string s = state.coerceToString(pos, *args[0], context); + auto s = state.coerceToString(pos, *args[0], context); PathSet context2; for (auto & p : context) context2.insert(p.at(0) == '=' ? string(p, 1) : p); - v.mkString(s, context2); + v.mkString(*s, context2); } static RegisterPrimOp primop_unsafeDiscardOutputDependency("__unsafeDiscardOutputDependency", 1, prim_unsafeDiscardOutputDependency); diff --git a/src/libexpr/primops/fetchMercurial.cc b/src/libexpr/primops/fetchMercurial.cc index f808e2da5..c4e1a7bf0 100644 --- a/src/libexpr/primops/fetchMercurial.cc +++ b/src/libexpr/primops/fetchMercurial.cc @@ -24,7 +24,7 @@ static void prim_fetchMercurial(EvalState & state, const Pos & pos, Value * * ar for (auto & attr : *args[0]->attrs) { std::string_view n(attr.name); if (n == "url") - url = state.coerceToString(*attr.pos, *attr.value, context, false, false); + url = state.coerceToString(*attr.pos, *attr.value, context, false, false).toOwned(); else if (n == "rev") { // Ugly: unlike fetchGit, here the "rev" attribute can // be both a revision or a branch/tag name. @@ -50,7 +50,7 @@ static void prim_fetchMercurial(EvalState & state, const Pos & pos, Value * * ar }); } else - url = state.coerceToString(pos, *args[0], context, false, false); + url = state.coerceToString(pos, *args[0], context, false, false).toOwned(); // FIXME: git externals probably can be used to bypass the URI // whitelist. Ah well. diff --git a/src/libexpr/primops/fetchTree.cc b/src/libexpr/primops/fetchTree.cc index 6647bd35c..d09e2d9e1 100644 --- a/src/libexpr/primops/fetchTree.cc +++ b/src/libexpr/primops/fetchTree.cc @@ -125,7 +125,7 @@ static void fetchTree( if (attr.name == state.sType) continue; state.forceValue(*attr.value, *attr.pos); if (attr.value->type() == nPath || attr.value->type() == nString) { - auto s = state.coerceToString(*attr.pos, *attr.value, context, false, false); + auto s = state.coerceToString(*attr.pos, *attr.value, context, false, false).toOwned(); attrs.emplace(attr.name, attr.name == "url" ? type == "git" @@ -151,7 +151,7 @@ static void fetchTree( input = fetchers::Input::fromAttrs(std::move(attrs)); } else { - auto url = state.coerceToString(pos, *args[0], context, false, false); + auto url = state.coerceToString(pos, *args[0], context, false, false).toOwned(); if (type == "git") { fetchers::Attrs attrs; diff --git a/src/libutil/types.hh b/src/libutil/types.hh index 8f72c926f..e3aca20c9 100644 --- a/src/libutil/types.hh +++ b/src/libutil/types.hh @@ -6,6 +6,7 @@ #include #include #include +#include #include namespace nix { @@ -47,4 +48,63 @@ struct Explicit { } }; + +/* This wants to be a little bit like rust's Cow type. + Some parts of the evaluator benefit greatly from being able to reuse + existing allocations for strings, but have to be able to also use + newly allocated storage for values. + + We do not define implicit conversions, even with ref qualifiers, + since those can easily become ambiguous to the reader and can degrade + into copying behaviour we want to avoid. */ +class BackedStringView { +private: + std::variant data; + + /* Needed to introduce a temporary since operator-> must return + a pointer. Without this we'd need to store the view object + even when we already own a string. */ + class Ptr { + private: + std::string_view view; + public: + Ptr(std::string_view view): view(view) {} + const std::string_view * operator->() const { return &view; } + }; + +public: + BackedStringView(std::string && s): data(std::move(s)) {} + BackedStringView(std::string_view sv): data(sv) {} + template + BackedStringView(const char (& lit)[N]): data(std::string_view(lit)) {} + + BackedStringView(const BackedStringView &) = delete; + BackedStringView & operator=(const BackedStringView &) = delete; + + /* We only want move operations defined since the sole purpose of + this type is to avoid copies. */ + BackedStringView(BackedStringView && other) = default; + BackedStringView & operator=(BackedStringView && other) = default; + + bool isOwned() const + { + return std::holds_alternative(data); + } + + std::string toOwned() && + { + return isOwned() + ? std::move(std::get(data)) + : std::string(std::get(data)); + } + + std::string_view operator*() const + { + return isOwned() + ? std::get(data) + : std::get(data); + } + Ptr operator->() const { return Ptr(**this); } +}; + } diff --git a/src/nix/eval.cc b/src/nix/eval.cc index c7517cf79..c0435461f 100644 --- a/src/nix/eval.cc +++ b/src/nix/eval.cc @@ -107,7 +107,7 @@ struct CmdEval : MixJSON, InstallableCommand else if (raw) { stopProgressBar(); - std::cout << state->coerceToString(noPos, *v, context); + std::cout << *state->coerceToString(noPos, *v, context); } else if (json) { diff --git a/src/nix/repl.cc b/src/nix/repl.cc index be9f96836..2d983e98e 100644 --- a/src/nix/repl.cc +++ b/src/nix/repl.cc @@ -463,7 +463,7 @@ bool NixRepl::processLine(string line) if (v.type() == nPath || v.type() == nString) { PathSet context; auto filename = state->coerceToString(noPos, v, context); - pos.file = state->symbols.create(filename); + pos.file = state->symbols.create(*filename); } else if (v.isLambda()) { pos = v.lambda.fun->pos; } else { From a0357abda743496b6731fe1ff63c6be49e03f56f Mon Sep 17 00:00:00 2001 From: Will Dietz Date: Sat, 29 Jan 2022 16:23:35 -0600 Subject: [PATCH 09/13] canonPath: fix missing slash when resolving links Fixes #6017 --- src/libutil/util.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libutil/util.cc b/src/libutil/util.cc index 1f1f2c861..4cd46c84c 100644 --- a/src/libutil/util.cc +++ b/src/libutil/util.cc @@ -147,7 +147,7 @@ Path canonPath(PathView path, bool resolveSymlinks) path = {}; } else { s += path.substr(0, slash); - path = path.substr(slash + 1); + path = path.substr(slash); } /* If s points to a symlink, resolve it and continue from there */ From 85b1427662979c6f4fe765839ff7a742216f70f9 Mon Sep 17 00:00:00 2001 From: Thomas Koch Date: Sun, 30 Jan 2022 10:51:39 +0200 Subject: [PATCH 10/13] fix spelling mistakes reported by Debian's lintian tool --- src/libexpr/primops.cc | 4 ++-- src/libstore/globals.hh | 2 +- src/nix/develop.md | 2 +- src/nix/nix.md | 2 +- src/nix/realisation/info.md | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc index b53425510..ee0ffcd05 100644 --- a/src/libexpr/primops.cc +++ b/src/libexpr/primops.cc @@ -1269,7 +1269,7 @@ static RegisterPrimOp primop_derivationStrict(RegisterPrimOp::Info { substituted by the corresponding output path at build time. For example, 'placeholder "out"' returns the string /1rz4g4znpzjwh1xymhjpm42vipw92pr73vdgl6xs1hycac8kf2n9. At build - time, any occurence of this string in an derivation attribute will + time, any occurrence of this string in an derivation attribute will be replaced with the concrete path in the Nix store of the output ‘out’. */ static void prim_placeholder(EvalState & state, const Pos & pos, Value * * args, Value & v) @@ -2279,7 +2279,7 @@ static RegisterPrimOp primop_removeAttrs({ /* Builds a set from a list specifying (name, value) pairs. To be precise, a list [{name = "name1"; value = value1;} ... {name = "nameN"; value = valueN;}] is transformed to {name1 = value1; - ... nameN = valueN;}. In case of duplicate occurences of the same + ... nameN = valueN;}. In case of duplicate occurrences of the same name, the first takes precedence. */ static void prim_listToAttrs(EvalState & state, const Pos & pos, Value * * args, Value & v) { diff --git a/src/libstore/globals.hh b/src/libstore/globals.hh index f65893b10..893c95bd6 100644 --- a/src/libstore/globals.hh +++ b/src/libstore/globals.hh @@ -970,7 +970,7 @@ public: Setting commitLockFileSummary{ this, "", "commit-lockfile-summary", R"( - The commit summary to use when commiting changed flake lock files. If + The commit summary to use when committing changed flake lock files. If empty, the summary is generated based on the action performed. )"}; }; diff --git a/src/nix/develop.md b/src/nix/develop.md index 1f214966a..3e7e339d5 100644 --- a/src/nix/develop.md +++ b/src/nix/develop.md @@ -55,7 +55,7 @@ R""( # nix develop /tmp/my-build-env ``` -* Replace all occurences of the store path corresponding to +* Replace all occurrences of the store path corresponding to `glibc.dev` with a writable directory: ```console diff --git a/src/nix/nix.md b/src/nix/nix.md index 2f54c5e2c..1dc59362d 100644 --- a/src/nix/nix.md +++ b/src/nix/nix.md @@ -76,7 +76,7 @@ the Nix store. Here are the recognised types of installables: Note that the search will only include files indexed by git. In particular, files which are matched by `.gitignore` or have never been `git add`-ed will not be - available in the flake. If this is undesireable, specify `path:` explicitly; + available in the flake. If this is undesirable, specify `path:` explicitly; For example, if `/foo/bar` is a git repository with the following structure: ``` diff --git a/src/nix/realisation/info.md b/src/nix/realisation/info.md index 852240f44..8aa986516 100644 --- a/src/nix/realisation/info.md +++ b/src/nix/realisation/info.md @@ -1,7 +1,7 @@ R"MdBoundary( # Description -Display some informations about the given realisation +Display some information about the given realisation # Examples From 43509cc69d24d3b8def894a2d47338b7873dc0f4 Mon Sep 17 00:00:00 2001 From: Thomas Koch Date: Sun, 30 Jan 2022 20:59:58 +0200 Subject: [PATCH 11/13] use LOWDOWN_LIBS variable fixes: #5931 --- Makefile.config.in | 1 + src/libcmd/local.mk | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Makefile.config.in b/Makefile.config.in index c8c4446b4..3505f337e 100644 --- a/Makefile.config.in +++ b/Makefile.config.in @@ -16,6 +16,7 @@ LDFLAGS = @LDFLAGS@ LIBARCHIVE_LIBS = @LIBARCHIVE_LIBS@ LIBBROTLI_LIBS = @LIBBROTLI_LIBS@ LIBCURL_LIBS = @LIBCURL_LIBS@ +LOWDOWN_LIBS = @LOWDOWN_LIBS@ OPENSSL_LIBS = @OPENSSL_LIBS@ LIBSECCOMP_LIBS = @LIBSECCOMP_LIBS@ PACKAGE_NAME = @PACKAGE_NAME@ diff --git a/src/libcmd/local.mk b/src/libcmd/local.mk index 8b0662753..7a2f83cc7 100644 --- a/src/libcmd/local.mk +++ b/src/libcmd/local.mk @@ -8,7 +8,7 @@ libcmd_SOURCES := $(wildcard $(d)/*.cc) libcmd_CXXFLAGS += -I src/libutil -I src/libstore -I src/libexpr -I src/libmain -I src/libfetchers -libcmd_LDFLAGS += -llowdown -pthread +libcmd_LDFLAGS += $(LOWDOWN_LIBS) -pthread libcmd_LIBS = libstore libutil libexpr libmain libfetchers From 73d5f38a47e7c3dea62994cfb8f962976194f767 Mon Sep 17 00:00:00 2001 From: Eelco Dolstra Date: Tue, 1 Feb 2022 10:44:19 +0100 Subject: [PATCH 12/13] Require lowdown 0.9.0 Fixes #6021. --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 01d674879..8a01c33ec 100644 --- a/configure.ac +++ b/configure.ac @@ -272,7 +272,7 @@ AC_ARG_ENABLE(doc-gen, AS_HELP_STRING([--disable-doc-gen],[disable documentation AC_SUBST(doc_generate) # Look for lowdown library. -PKG_CHECK_MODULES([LOWDOWN], [lowdown >= 0.8.0], [CXXFLAGS="$LOWDOWN_CFLAGS $CXXFLAGS"]) +PKG_CHECK_MODULES([LOWDOWN], [lowdown >= 0.9.0], [CXXFLAGS="$LOWDOWN_CFLAGS $CXXFLAGS"]) # Setuid installations. AC_CHECK_FUNCS([setresuid setreuid lchown]) From 169ea0b83fe09b473766e22337dfd488c96ff2be Mon Sep 17 00:00:00 2001 From: Alexander Bantyev Date: Wed, 2 Feb 2022 21:41:45 +0300 Subject: [PATCH 13/13] Flake follows: resolve all follows to absolute It's not possible in general to know in computeLocks, relative to which path the follows was intended to be. So, we always resolve follows to their absolute states when we encounter them (which can either be in parseFlakeInput or computeLocks' fake input population). Fixes https://github.com/NixOS/nix/issues/6013 Fixes https://github.com/NixOS/nix/issues/5609 Fixes https://github.com/NixOS/nix/issues/5697 (again) --- src/libexpr/flake/flake.cc | 87 +++++++++++++++----------------------- tests/flakes.sh | 6 ++- 2 files changed, 38 insertions(+), 55 deletions(-) diff --git a/src/libexpr/flake/flake.cc b/src/libexpr/flake/flake.cc index 0fbe9b960..22ff9b153 100644 --- a/src/libexpr/flake/flake.cc +++ b/src/libexpr/flake/flake.cc @@ -89,11 +89,11 @@ static void expectType(EvalState & state, ValueType type, static std::map parseFlakeInputs( EvalState & state, Value * value, const Pos & pos, - const std::optional & baseDir); + const std::optional & baseDir, InputPath lockRootPath); static FlakeInput parseFlakeInput(EvalState & state, const std::string & inputName, Value * value, const Pos & pos, - const std::optional & baseDir) + const std::optional & baseDir, InputPath lockRootPath) { expectType(state, nAttrs, *value, pos); @@ -117,10 +117,12 @@ static FlakeInput parseFlakeInput(EvalState & state, expectType(state, nBool, *attr.value, *attr.pos); input.isFlake = attr.value->boolean; } else if (attr.name == sInputs) { - input.overrides = parseFlakeInputs(state, attr.value, *attr.pos, baseDir); + input.overrides = parseFlakeInputs(state, attr.value, *attr.pos, baseDir, lockRootPath); } else if (attr.name == sFollows) { expectType(state, nString, *attr.value, *attr.pos); - input.follows = parseInputPath(attr.value->string.s); + auto follows(parseInputPath(attr.value->string.s)); + follows.insert(follows.begin(), lockRootPath.begin(), lockRootPath.end()); + input.follows = follows; } else { switch (attr.value->type()) { case nString: @@ -166,7 +168,7 @@ static FlakeInput parseFlakeInput(EvalState & state, static std::map parseFlakeInputs( EvalState & state, Value * value, const Pos & pos, - const std::optional & baseDir) + const std::optional & baseDir, InputPath lockRootPath) { std::map inputs; @@ -178,7 +180,8 @@ static std::map parseFlakeInputs( inputAttr.name, inputAttr.value, *inputAttr.pos, - baseDir)); + baseDir, + lockRootPath)); } return inputs; @@ -188,7 +191,8 @@ static Flake getFlake( EvalState & state, const FlakeRef & originalRef, bool allowLookup, - FlakeCache & flakeCache) + FlakeCache & flakeCache, + InputPath lockRootPath) { auto [sourceInfo, resolvedRef, lockedRef] = fetchOrSubstituteTree( state, originalRef, allowLookup, flakeCache); @@ -223,7 +227,7 @@ static Flake getFlake( auto sInputs = state.symbols.create("inputs"); if (auto inputs = vInfo.attrs->get(sInputs)) - flake.inputs = parseFlakeInputs(state, inputs->value, *inputs->pos, flakeDir); + flake.inputs = parseFlakeInputs(state, inputs->value, *inputs->pos, flakeDir, lockRootPath); auto sOutputs = state.symbols.create("outputs"); @@ -287,6 +291,11 @@ static Flake getFlake( return flake; } +Flake getFlake(EvalState & state, const FlakeRef & originalRef, bool allowLookup, FlakeCache & flakeCache) +{ + return getFlake(state, originalRef, allowLookup, flakeCache, {}); +} + Flake getFlake(EvalState & state, const FlakeRef & originalRef, bool allowLookup) { FlakeCache flakeCache; @@ -332,22 +341,12 @@ LockedFlake lockFlake( std::vector parents; - struct LockParent { - /* The path to this parent. */ - InputPath path; - - /* Whether we are currently inside a top-level lockfile - (inputs absolute) or subordinate lockfile (inputs - relative). */ - bool absolute; - }; - std::function node, const InputPath & inputPathPrefix, std::shared_ptr oldNode, - const LockParent & parent, + const InputPath & lockRootPath, const Path & parentPath, bool trustLock)> computeLocks; @@ -357,7 +356,7 @@ LockedFlake lockFlake( std::shared_ptr node, const InputPath & inputPathPrefix, std::shared_ptr oldNode, - const LockParent & parent, + const InputPath & lockRootPath, const Path & parentPath, bool trustLock) { @@ -402,17 +401,7 @@ LockedFlake lockFlake( if (input.follows) { InputPath target; - if (parent.absolute && !hasOverride) { - target = *input.follows; - } else { - if (hasOverride) { - target = inputPathPrefix; - target.pop_back(); - } else - target = parent.path; - - for (auto & i : *input.follows) target.push_back(i); - } + target.insert(target.end(), input.follows->begin(), input.follows->end()); debug("input '%s' follows '%s'", inputPathS, printInputPath(target)); node->inputs.insert_or_assign(id, target); @@ -485,23 +474,25 @@ LockedFlake lockFlake( break; } } + auto absoluteFollows(lockRootPath); + absoluteFollows.insert(absoluteFollows.end(), follows->begin(), follows->end()); fakeInputs.emplace(i.first, FlakeInput { - .follows = *follows, + .follows = absoluteFollows, }); } } } - LockParent newParent { - .path = inputPath, - .absolute = true - }; - + auto localPath(parentPath); + // If this input is a path, recurse it down. + // This allows us to resolve path inputs relative to the current flake. + if ((*input.ref).input.getType() == "path") + localPath = absPath(*input.ref->input.getSourcePath(), parentPath); computeLocks( mustRefetch - ? getFlake(state, oldLock->lockedRef, false, flakeCache).inputs + ? getFlake(state, oldLock->lockedRef, false, flakeCache, inputPath).inputs : fakeInputs, - childNode, inputPath, oldLock, newParent, parentPath, !mustRefetch); + childNode, inputPath, oldLock, lockRootPath, parentPath, !mustRefetch); } else { /* We need to create a new lock file entry. So fetch @@ -520,7 +511,7 @@ LockedFlake lockFlake( if (localRef.input.getType() == "path") localPath = absPath(*input.ref->input.getSourcePath(), parentPath); - auto inputFlake = getFlake(state, localRef, useRegistries, flakeCache); + auto inputFlake = getFlake(state, localRef, useRegistries, flakeCache, inputPath); /* Note: in case of an --override-input, we use the *original* ref (input2.ref) for the @@ -541,13 +532,6 @@ LockedFlake lockFlake( parents.push_back(*input.ref); Finally cleanup([&]() { parents.pop_back(); }); - // Follows paths from existing inputs in the top-level lockfile are absolute, - // whereas paths in subordinate lockfiles are relative to those lockfiles. - LockParent newParent { - .path = inputPath, - .absolute = oldLock ? true : false - }; - /* Recursively process the inputs of this flake. Also, unless we already have this flake in the top-level lock file, use this flake's @@ -558,7 +542,7 @@ LockedFlake lockFlake( ? std::dynamic_pointer_cast(oldLock) : LockFile::read( inputFlake.sourceInfo->actualPath + "/" + inputFlake.lockedRef.subdir + "/flake.lock").root, - newParent, localPath, false); + oldLock ? lockRootPath : inputPath, localPath, false); } else { @@ -576,17 +560,12 @@ LockedFlake lockFlake( } }; - LockParent parent { - .path = {}, - .absolute = true - }; - // Bring in the current ref for relative path resolution if we have it auto parentPath = canonPath(flake.sourceInfo->actualPath + "/" + flake.lockedRef.subdir, true); computeLocks( flake.inputs, newLockFile.root, {}, - lockFlags.recreateLockFile ? nullptr : oldLockFile.root, parent, parentPath, false); + lockFlags.recreateLockFile ? nullptr : oldLockFile.root, {}, parentPath, false); for (auto & i : lockFlags.inputOverrides) if (!overridesUsed.count(i.first)) diff --git a/tests/flakes.sh b/tests/flakes.sh index 20e6d09c1..db178967f 100644 --- a/tests/flakes.sh +++ b/tests/flakes.sh @@ -730,6 +730,7 @@ cat > $flakeFollowsB/flake.nix < $flakeFollowsC/flake.nix < $flakeFollowsE/flake.nix <