From 8669c02468994887be91072ac58b1ee43380d354 Mon Sep 17 00:00:00 2001 From: pennae Date: Sat, 27 Jan 2024 16:33:34 +0100 Subject: [PATCH 1/9] add test for inherit-from semantics --- tests/functional/lang/eval-okay-inherit-from.err.exp | 2 ++ tests/functional/lang/eval-okay-inherit-from.exp | 1 + tests/functional/lang/eval-okay-inherit-from.nix | 6 ++++++ 3 files changed, 9 insertions(+) create mode 100644 tests/functional/lang/eval-okay-inherit-from.err.exp create mode 100644 tests/functional/lang/eval-okay-inherit-from.exp create mode 100644 tests/functional/lang/eval-okay-inherit-from.nix diff --git a/tests/functional/lang/eval-okay-inherit-from.err.exp b/tests/functional/lang/eval-okay-inherit-from.err.exp new file mode 100644 index 000000000..51881205b --- /dev/null +++ b/tests/functional/lang/eval-okay-inherit-from.err.exp @@ -0,0 +1,2 @@ +trace: used +trace: used diff --git a/tests/functional/lang/eval-okay-inherit-from.exp b/tests/functional/lang/eval-okay-inherit-from.exp new file mode 100644 index 000000000..43bd0e899 --- /dev/null +++ b/tests/functional/lang/eval-okay-inherit-from.exp @@ -0,0 +1 @@ +[ 1 2 { __overrides = { y = { d = [ ]; }; }; c = [ ]; d = 4; x = { c = [ ]; }; y = «repeated»; } ] diff --git a/tests/functional/lang/eval-okay-inherit-from.nix b/tests/functional/lang/eval-okay-inherit-from.nix new file mode 100644 index 000000000..d1fad7d69 --- /dev/null +++ b/tests/functional/lang/eval-okay-inherit-from.nix @@ -0,0 +1,6 @@ +let + inherit (builtins.trace "used" { a = 1; b = 2; }) a b; + x.c = 3; + y.d = 4; +in + [ a b rec { x.c = []; inherit (x) c; inherit (y) d; __overrides.y.d = []; } ] From 73065a400d176b21f518c1f4ece90c31318b218d Mon Sep 17 00:00:00 2001 From: pennae Date: Sat, 27 Jan 2024 16:33:34 +0100 Subject: [PATCH 2/9] add test for inherit expr printing --- tests/functional/lang/parse-okay-inherits.exp | 1 + tests/functional/lang/parse-okay-inherits.nix | 9 +++++++++ 2 files changed, 10 insertions(+) create mode 100644 tests/functional/lang/parse-okay-inherits.exp create mode 100644 tests/functional/lang/parse-okay-inherits.nix diff --git a/tests/functional/lang/parse-okay-inherits.exp b/tests/functional/lang/parse-okay-inherits.exp new file mode 100644 index 000000000..050b54afd --- /dev/null +++ b/tests/functional/lang/parse-okay-inherits.exp @@ -0,0 +1 @@ +(let c = { }; b = 2; in { a = 1; inherit b ; d = (c).d; e = (c).e; f = 3; }) diff --git a/tests/functional/lang/parse-okay-inherits.nix b/tests/functional/lang/parse-okay-inherits.nix new file mode 100644 index 000000000..10596c8ad --- /dev/null +++ b/tests/functional/lang/parse-okay-inherits.nix @@ -0,0 +1,9 @@ +let + c = {}; + b = 2; +in { + a = 1; + inherit b; + inherit (c) d e; + f = 3; +} From c66ee57edc6cac3571bfbf77d0c0ea4d25b4e805 Mon Sep 17 00:00:00 2001 From: pennae Date: Sat, 27 Jan 2024 16:33:34 +0100 Subject: [PATCH 3/9] preserve information about whether/how an attribute was inherited --- src/libexpr/eval.cc | 6 +++--- src/libexpr/nixexpr.cc | 8 ++++---- src/libexpr/nixexpr.hh | 17 ++++++++++++++--- src/libexpr/parser-state.hh | 2 +- src/libexpr/parser.y | 11 +++++++++-- 5 files changed, 31 insertions(+), 13 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index bffbd5f1a..12d7d825f 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1207,11 +1207,11 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v) Displacement displ = 0; for (auto & i : attrs) { Value * vAttr; - if (hasOverrides && !i.second.inherited) { + if (hasOverrides && !i.second.inherited()) { vAttr = state.allocValue(); mkThunk(*vAttr, env2, i.second.e); } else - vAttr = i.second.e->maybeThunk(state, i.second.inherited ? env : env2); + vAttr = i.second.e->maybeThunk(state, i.second.inherited() ? env : env2); env2.values[displ++] = vAttr; v.attrs->push_back(Attr(i.first, vAttr, i.second.pos)); } @@ -1282,7 +1282,7 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v) environment. */ Displacement displ = 0; for (auto & i : attrs->attrs) - env2.values[displ++] = i.second.e->maybeThunk(state, i.second.inherited ? env : env2); + env2.values[displ++] = i.second.e->maybeThunk(state, i.second.inherited() ? env : env2); auto dts = state.debugRepl ? makeDebugTraceStacker( diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index 46737fea6..4c06864fd 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -82,7 +82,7 @@ void ExprAttrs::show(const SymbolTable & symbols, std::ostream & str) const return sa < sb; }); for (auto & i : sorted) { - if (i->second.inherited) + if (i->second.inherited()) str << "inherit " << symbols[i->first] << " " << "; "; else { str << symbols[i->first] << " = "; @@ -153,7 +153,7 @@ void ExprLet::show(const SymbolTable & symbols, std::ostream & str) const { str << "(let "; for (auto & i : attrs->attrs) - if (i.second.inherited) { + if (i.second.inherited()) { str << "inherit " << symbols[i.first] << "; "; } else { @@ -343,7 +343,7 @@ void ExprAttrs::bindVars(EvalState & es, const std::shared_ptr // No need to sort newEnv since attrs is in sorted order. for (auto & i : attrs) - i.second.e->bindVars(es, i.second.inherited ? env : newEnv); + i.second.e->bindVars(es, i.second.inherited() ? env : newEnv); for (auto & i : dynamicAttrs) { i.nameExpr->bindVars(es, newEnv); @@ -418,7 +418,7 @@ void ExprLet::bindVars(EvalState & es, const std::shared_ptr & // No need to sort newEnv since attrs->attrs is in sorted order. for (auto & i : attrs->attrs) - i.second.e->bindVars(es, i.second.inherited ? env : newEnv); + i.second.e->bindVars(es, i.second.inherited() ? env : newEnv); if (es.debugRepl) es.exprEnvs.insert(std::make_pair(this, newEnv)); diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 1f944f10b..c8f47b02b 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -160,13 +160,24 @@ struct ExprAttrs : Expr bool recursive; PosIdx pos; struct AttrDef { - bool inherited; + enum class Kind { + /** `attr = expr;` */ + Plain, + /** `inherit attr1 attrn;` */ + Inherited, + /** `inherit (expr) attr1 attrn;` */ + InheritedFrom, + }; + + Kind kind; Expr * e; PosIdx pos; Displacement displ; // displacement - AttrDef(Expr * e, const PosIdx & pos, bool inherited=false) - : inherited(inherited), e(e), pos(pos) { }; + AttrDef(Expr * e, const PosIdx & pos, Kind kind = Kind::Plain) + : kind(kind), e(e), pos(pos) { }; AttrDef() { }; + + bool inherited() const { return kind == Kind::Inherited; } }; typedef std::map AttrDefs; AttrDefs attrs; diff --git a/src/libexpr/parser-state.hh b/src/libexpr/parser-state.hh index 87aeaeef5..ae38de130 100644 --- a/src/libexpr/parser-state.hh +++ b/src/libexpr/parser-state.hh @@ -89,7 +89,7 @@ inline void ParserState::addAttr(ExprAttrs * attrs, AttrPath && attrPath, Expr * if (i->symbol) { ExprAttrs::AttrDefs::iterator j = attrs->attrs.find(i->symbol); if (j != attrs->attrs.end()) { - if (!j->second.inherited) { + if (!j->second.inherited()) { ExprAttrs * attrs2 = dynamic_cast(j->second.e); if (!attrs2) dupAttr(attrPath, pos, j->second.pos); attrs = attrs2; diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index a3ba13c66..0898b81f7 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -313,7 +313,9 @@ binds if ($$->attrs.find(i.symbol) != $$->attrs.end()) state->dupAttr(i.symbol, state->at(@3), $$->attrs[i.symbol].pos); auto pos = state->at(@3); - $$->attrs.emplace(i.symbol, ExprAttrs::AttrDef(new ExprVar(CUR_POS, i.symbol), pos, true)); + $$->attrs.emplace( + i.symbol, + ExprAttrs::AttrDef(new ExprVar(CUR_POS, i.symbol), pos, ExprAttrs::AttrDef::Kind::Inherited)); } delete $3; } @@ -323,7 +325,12 @@ binds for (auto & i : *$6) { if ($$->attrs.find(i.symbol) != $$->attrs.end()) state->dupAttr(i.symbol, state->at(@6), $$->attrs[i.symbol].pos); - $$->attrs.emplace(i.symbol, ExprAttrs::AttrDef(new ExprSelect(CUR_POS, $4, i.symbol), state->at(@6))); + $$->attrs.emplace( + i.symbol, + ExprAttrs::AttrDef( + new ExprSelect(CUR_POS, $4, i.symbol), + state->at(@6), + ExprAttrs::AttrDef::Kind::InheritedFrom)); } delete $6; } From 1f542adb3e18e7078e6a589182a53a47d971748a Mon Sep 17 00:00:00 2001 From: pennae Date: Sat, 27 Jan 2024 16:33:34 +0100 Subject: [PATCH 4/9] add ExprAttrs::AttrDef::chooseByKind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit in place of inherited() — not quite useful yet since we don't distinguish plain and inheritFrom attr kinds so far. --- src/libexpr/eval.cc | 22 +++++++++++++++------- src/libexpr/nixexpr.cc | 28 +++++++++++++++++----------- src/libexpr/nixexpr.hh | 14 ++++++++++++++ 3 files changed, 46 insertions(+), 18 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 12d7d825f..91341e167 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1209,9 +1209,9 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v) Value * vAttr; if (hasOverrides && !i.second.inherited()) { vAttr = state.allocValue(); - mkThunk(*vAttr, env2, i.second.e); + mkThunk(*vAttr, *i.second.chooseByKind(&env2, &env, &env2), i.second.e); } else - vAttr = i.second.e->maybeThunk(state, i.second.inherited() ? env : env2); + vAttr = i.second.e->maybeThunk(state, *i.second.chooseByKind(&env2, &env, &env2)); env2.values[displ++] = vAttr; v.attrs->push_back(Attr(i.first, vAttr, i.second.pos)); } @@ -1243,9 +1243,14 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v) } } - else - for (auto & i : attrs) - v.attrs->push_back(Attr(i.first, i.second.e->maybeThunk(state, env), i.second.pos)); + else { + for (auto & i : attrs) { + v.attrs->push_back(Attr( + i.first, + i.second.e->maybeThunk(state, *i.second.chooseByKind(&env, &env, &env)), + i.second.pos)); + } + } /* Dynamic attrs apply *after* rec and __overrides. */ for (auto & i : dynamicAttrs) { @@ -1281,8 +1286,11 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v) while the inherited attributes are evaluated in the original environment. */ Displacement displ = 0; - for (auto & i : attrs->attrs) - env2.values[displ++] = i.second.e->maybeThunk(state, i.second.inherited() ? env : env2); + for (auto & i : attrs->attrs) { + env2.values[displ++] = i.second.e->maybeThunk( + state, + *i.second.chooseByKind(&env2, &env, &env2)); + } auto dts = state.debugRepl ? makeDebugTraceStacker( diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index 4c06864fd..f967777f2 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -334,16 +334,19 @@ void ExprAttrs::bindVars(EvalState & es, const std::shared_ptr es.exprEnvs.insert(std::make_pair(this, env)); if (recursive) { - auto newEnv = std::make_shared(nullptr, env.get(), recursive ? attrs.size() : 0); + auto newEnv = [&] () -> std::shared_ptr { + auto newEnv = std::make_shared(nullptr, env.get(), attrs.size()); - Displacement displ = 0; - for (auto & i : attrs) - newEnv->vars.emplace_back(i.first, i.second.displ = displ++); + Displacement displ = 0; + for (auto & i : attrs) + newEnv->vars.emplace_back(i.first, i.second.displ = displ++); + return newEnv; + }(); // No need to sort newEnv since attrs is in sorted order. for (auto & i : attrs) - i.second.e->bindVars(es, i.second.inherited() ? env : newEnv); + i.second.e->bindVars(es, i.second.chooseByKind(newEnv, env, newEnv)); for (auto & i : dynamicAttrs) { i.nameExpr->bindVars(es, newEnv); @@ -352,7 +355,7 @@ void ExprAttrs::bindVars(EvalState & es, const std::shared_ptr } else { for (auto & i : attrs) - i.second.e->bindVars(es, env); + i.second.e->bindVars(es, i.second.chooseByKind(env, env, env)); for (auto & i : dynamicAttrs) { i.nameExpr->bindVars(es, env); @@ -409,16 +412,19 @@ void ExprCall::bindVars(EvalState & es, const std::shared_ptr & void ExprLet::bindVars(EvalState & es, const std::shared_ptr & env) { - auto newEnv = std::make_shared(nullptr, env.get(), attrs->attrs.size()); + auto newEnv = [&] () -> std::shared_ptr { + auto newEnv = std::make_shared(nullptr, env.get(), attrs->attrs.size()); - Displacement displ = 0; - for (auto & i : attrs->attrs) - newEnv->vars.emplace_back(i.first, i.second.displ = displ++); + Displacement displ = 0; + for (auto & i : attrs->attrs) + newEnv->vars.emplace_back(i.first, i.second.displ = displ++); + return newEnv; + }(); // No need to sort newEnv since attrs->attrs is in sorted order. for (auto & i : attrs->attrs) - i.second.e->bindVars(es, i.second.inherited() ? env : newEnv); + i.second.e->bindVars(es, i.second.chooseByKind(newEnv, env, newEnv)); if (es.debugRepl) es.exprEnvs.insert(std::make_pair(this, newEnv)); diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index c8f47b02b..2d8dafe44 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -178,6 +178,20 @@ struct ExprAttrs : Expr AttrDef() { }; bool inherited() const { return kind == Kind::Inherited; } + + template + const T & chooseByKind(const T & plain, const T & inherited, const T & inheritedFrom) const + { + switch (kind) { + case Kind::Plain: + return plain; + case Kind::Inherited: + return inherited; + default: + case Kind::InheritedFrom: + return inheritedFrom; + } + } }; typedef std::map AttrDefs; AttrDefs attrs; From 6c08fba533ef31cad2bdc03ba72ecf58dc8ee5a0 Mon Sep 17 00:00:00 2001 From: pennae Date: Sat, 27 Jan 2024 16:33:34 +0100 Subject: [PATCH 5/9] use the same bindings print for ExprAttrs and ExprLet this also has the effect of sorting let bindings lexicographically rather than by symbol creation order as was previously done, giving a better canonicalization in the process. --- src/libexpr/nixexpr.cc | 21 ++++++++----------- src/libexpr/nixexpr.hh | 2 ++ tests/functional/lang/parse-okay-inherits.exp | 2 +- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index f967777f2..c0812bb30 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -70,10 +70,8 @@ void ExprOpHasAttr::show(const SymbolTable & symbols, std::ostream & str) const str << ") ? " << showAttrPath(symbols, attrPath) << ")"; } -void ExprAttrs::show(const SymbolTable & symbols, std::ostream & str) const +void ExprAttrs::showBindings(const SymbolTable & symbols, std::ostream & str) const { - if (recursive) str << "rec "; - str << "{ "; typedef const decltype(attrs)::value_type * Attr; std::vector sorted; for (auto & i : attrs) sorted.push_back(&i); @@ -97,6 +95,13 @@ void ExprAttrs::show(const SymbolTable & symbols, std::ostream & str) const i.valueExpr->show(symbols, str); str << "; "; } +} + +void ExprAttrs::show(const SymbolTable & symbols, std::ostream & str) const +{ + if (recursive) str << "rec "; + str << "{ "; + showBindings(symbols, str); str << "}"; } @@ -152,15 +157,7 @@ void ExprCall::show(const SymbolTable & symbols, std::ostream & str) const void ExprLet::show(const SymbolTable & symbols, std::ostream & str) const { str << "(let "; - for (auto & i : attrs->attrs) - if (i.second.inherited()) { - str << "inherit " << symbols[i.first] << "; "; - } - else { - str << symbols[i.first] << " = "; - i.second.e->show(symbols, str); - str << "; "; - } + attrs->showBindings(symbols, str); str << "in "; body->show(symbols, str); str << ")"; diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 2d8dafe44..4a93143b4 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -207,6 +207,8 @@ struct ExprAttrs : Expr ExprAttrs() : recursive(false) { }; PosIdx getPos() const override { return pos; } COMMON_METHODS + + void showBindings(const SymbolTable & symbols, std::ostream & str) const; }; struct ExprList : Expr diff --git a/tests/functional/lang/parse-okay-inherits.exp b/tests/functional/lang/parse-okay-inherits.exp index 050b54afd..722101ceb 100644 --- a/tests/functional/lang/parse-okay-inherits.exp +++ b/tests/functional/lang/parse-okay-inherits.exp @@ -1 +1 @@ -(let c = { }; b = 2; in { a = 1; inherit b ; d = (c).d; e = (c).e; f = 3; }) +(let b = 2; c = { }; in { a = 1; inherit b ; d = (c).d; e = (c).e; f = 3; }) From ecf8b12d60ad2929f9998666cf0966475b91e291 Mon Sep 17 00:00:00 2001 From: pennae Date: Sat, 27 Jan 2024 16:33:34 +0100 Subject: [PATCH 6/9] group inherit by source during Expr::show for plain inherits this is really just a stylistic choice, but for inherit-from it actually fixes an exponential size increase problem during expr printing (as may happen during assertion failure reporting, on during duplicate attr detection in the parser) --- src/libexpr/nixexpr.cc | 32 +++++++++++++++++-- tests/functional/lang/parse-okay-inherits.exp | 2 +- .../functional/lang/parse-okay-subversion.exp | 2 +- 3 files changed, 31 insertions(+), 5 deletions(-) diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index c0812bb30..82e69de51 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -79,10 +79,36 @@ void ExprAttrs::showBindings(const SymbolTable & symbols, std::ostream & str) co std::string_view sa = symbols[a->first], sb = symbols[b->first]; return sa < sb; }); + std::vector inherits; + std::map> inheritsFrom; for (auto & i : sorted) { - if (i->second.inherited()) - str << "inherit " << symbols[i->first] << " " << "; "; - else { + switch (i->second.kind) { + case AttrDef::Kind::Plain: + break; + case AttrDef::Kind::Inherited: + inherits.push_back(i->first); + break; + case AttrDef::Kind::InheritedFrom: { + auto & select = dynamic_cast(*i->second.e); + inheritsFrom[select.e].push_back(i->first); + break; + } + } + } + if (!inherits.empty()) { + str << "inherit"; + for (auto sym : inherits) str << " " << symbols[sym]; + str << "; "; + } + for (const auto & [from, syms] : inheritsFrom) { + str << "inherit ("; + from->show(symbols, str); + str << ")"; + for (auto sym : syms) str << " " << symbols[sym]; + str << "; "; + } + for (auto & i : sorted) { + if (i->second.kind == AttrDef::Kind::Plain) { str << symbols[i->first] << " = "; i->second.e->show(symbols, str); str << "; "; diff --git a/tests/functional/lang/parse-okay-inherits.exp b/tests/functional/lang/parse-okay-inherits.exp index 722101ceb..1355527e6 100644 --- a/tests/functional/lang/parse-okay-inherits.exp +++ b/tests/functional/lang/parse-okay-inherits.exp @@ -1 +1 @@ -(let b = 2; c = { }; in { a = 1; inherit b ; d = (c).d; e = (c).e; f = 3; }) +(let b = 2; c = { }; in { inherit b; inherit (c) d e; a = 1; f = 3; }) diff --git a/tests/functional/lang/parse-okay-subversion.exp b/tests/functional/lang/parse-okay-subversion.exp index 4168ee8bf..2303932c4 100644 --- a/tests/functional/lang/parse-okay-subversion.exp +++ b/tests/functional/lang/parse-okay-subversion.exp @@ -1 +1 @@ -({ fetchurl, localServer ? false, httpServer ? false, sslSupport ? false, pythonBindings ? false, javaSwigBindings ? false, javahlBindings ? false, stdenv, openssl ? null, httpd ? null, db4 ? null, expat, swig ? null, j2sdk ? null }: assert (expat != null); assert (localServer -> (db4 != null)); assert (httpServer -> ((httpd != null) && ((httpd).expat == expat))); assert (sslSupport -> ((openssl != null) && (httpServer -> ((httpd).openssl == openssl)))); assert (pythonBindings -> ((swig != null) && (swig).pythonSupport)); assert (javaSwigBindings -> ((swig != null) && (swig).javaSupport)); assert (javahlBindings -> (j2sdk != null)); ((stdenv).mkDerivation { builder = /foo/bar; db4 = (if localServer then db4 else null); inherit expat ; inherit httpServer ; httpd = (if httpServer then httpd else null); j2sdk = (if javaSwigBindings then (swig).j2sdk else (if javahlBindings then j2sdk else null)); inherit javaSwigBindings ; inherit javahlBindings ; inherit localServer ; name = "subversion-1.1.1"; openssl = (if sslSupport then openssl else null); patches = (if javahlBindings then [ (/javahl.patch) ] else [ ]); python = (if pythonBindings then (swig).python else null); inherit pythonBindings ; src = (fetchurl { md5 = "a180c3fe91680389c210c99def54d9e0"; url = "http://subversion.tigris.org/tarballs/subversion-1.1.1.tar.bz2"; }); inherit sslSupport ; swig = (if (pythonBindings || javaSwigBindings) then swig else null); })) +({ fetchurl, localServer ? false, httpServer ? false, sslSupport ? false, pythonBindings ? false, javaSwigBindings ? false, javahlBindings ? false, stdenv, openssl ? null, httpd ? null, db4 ? null, expat, swig ? null, j2sdk ? null }: assert (expat != null); assert (localServer -> (db4 != null)); assert (httpServer -> ((httpd != null) && ((httpd).expat == expat))); assert (sslSupport -> ((openssl != null) && (httpServer -> ((httpd).openssl == openssl)))); assert (pythonBindings -> ((swig != null) && (swig).pythonSupport)); assert (javaSwigBindings -> ((swig != null) && (swig).javaSupport)); assert (javahlBindings -> (j2sdk != null)); ((stdenv).mkDerivation { inherit expat httpServer javaSwigBindings javahlBindings localServer pythonBindings sslSupport; builder = /foo/bar; db4 = (if localServer then db4 else null); httpd = (if httpServer then httpd else null); j2sdk = (if javaSwigBindings then (swig).j2sdk else (if javahlBindings then j2sdk else null)); name = "subversion-1.1.1"; openssl = (if sslSupport then openssl else null); patches = (if javahlBindings then [ (/javahl.patch) ] else [ ]); python = (if pythonBindings then (swig).python else null); src = (fetchurl { md5 = "a180c3fe91680389c210c99def54d9e0"; url = "http://subversion.tigris.org/tarballs/subversion-1.1.1.tar.bz2"; }); swig = (if (pythonBindings || javaSwigBindings) then swig else null); })) From cefd0302b55b3360dbca59cfcb4bf6a750d6cdcf Mon Sep 17 00:00:00 2001 From: pennae Date: Sat, 27 Jan 2024 16:33:34 +0100 Subject: [PATCH 7/9] evaluate inherit (from) exprs only once per directive desugaring inherit-from to syntactic duplication of the source expr also duplicates side effects of the source expr (such as trace calls) and expensive computations (such as derivationStrict). --- doc/manual/rl-next/inherit-from-by-need.md | 7 +++ src/libexpr/eval.cc | 24 ++++++++-- src/libexpr/nixexpr.cc | 44 ++++++++++++++++--- src/libexpr/nixexpr.hh | 16 +++++++ src/libexpr/parser-state.hh | 11 +++++ src/libexpr/parser.y | 7 ++- .../lang/eval-okay-inherit-from.err.exp | 1 - .../lang/eval-okay-inherit-from.exp | 2 +- .../lang/eval-okay-inherit-from.nix | 12 ++++- 9 files changed, 109 insertions(+), 15 deletions(-) create mode 100644 doc/manual/rl-next/inherit-from-by-need.md diff --git a/doc/manual/rl-next/inherit-from-by-need.md b/doc/manual/rl-next/inherit-from-by-need.md new file mode 100644 index 000000000..67c2cdedf --- /dev/null +++ b/doc/manual/rl-next/inherit-from-by-need.md @@ -0,0 +1,7 @@ +--- +synopsis: "`inherit (x) ...` evaluates `x` only once" +prs: 9847 +--- + +`inherit (x) a b ...` now evaluates the expression `x` only once for all inherited attributes rather than once for each inherited attribute. +This does not usually have a measurable impact, but side-effects (such as `builtins.trace`) would be duplicated and expensive expressions (such as derivations) could cause a measurable slowdown. diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index 91341e167..a353571af 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1186,6 +1186,18 @@ void ExprPath::eval(EvalState & state, Env & env, Value & v) } +Env * ExprAttrs::buildInheritFromEnv(EvalState & state, Env & up) +{ + Env & inheritEnv = state.allocEnv(inheritFromExprs->size()); + inheritEnv.up = &up; + + Displacement displ = 0; + for (auto from : *inheritFromExprs) + inheritEnv.values[displ++] = from->maybeThunk(state, up); + + return &inheritEnv; +} + void ExprAttrs::eval(EvalState & state, Env & env, Value & v) { v.mkAttrs(state.buildBindings(attrs.size() + dynamicAttrs.size()).finish()); @@ -1197,6 +1209,7 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v) Env & env2(state.allocEnv(attrs.size())); env2.up = &env; dynamicEnv = &env2; + Env * inheritEnv = inheritFromExprs ? buildInheritFromEnv(state, env2) : nullptr; AttrDefs::iterator overrides = attrs.find(state.sOverrides); bool hasOverrides = overrides != attrs.end(); @@ -1209,9 +1222,9 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v) Value * vAttr; if (hasOverrides && !i.second.inherited()) { vAttr = state.allocValue(); - mkThunk(*vAttr, *i.second.chooseByKind(&env2, &env, &env2), i.second.e); + mkThunk(*vAttr, *i.second.chooseByKind(&env2, &env, inheritEnv), i.second.e); } else - vAttr = i.second.e->maybeThunk(state, *i.second.chooseByKind(&env2, &env, &env2)); + vAttr = i.second.e->maybeThunk(state, *i.second.chooseByKind(&env2, &env, inheritEnv)); env2.values[displ++] = vAttr; v.attrs->push_back(Attr(i.first, vAttr, i.second.pos)); } @@ -1244,10 +1257,11 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v) } else { + Env * inheritEnv = inheritFromExprs ? buildInheritFromEnv(state, env) : nullptr; for (auto & i : attrs) { v.attrs->push_back(Attr( i.first, - i.second.e->maybeThunk(state, *i.second.chooseByKind(&env, &env, &env)), + i.second.e->maybeThunk(state, *i.second.chooseByKind(&env, &env, inheritEnv)), i.second.pos)); } } @@ -1282,6 +1296,8 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v) Env & env2(state.allocEnv(attrs->attrs.size())); env2.up = &env; + Env * inheritEnv = attrs->inheritFromExprs ? attrs->buildInheritFromEnv(state, env2) : nullptr; + /* The recursive attributes are evaluated in the new environment, while the inherited attributes are evaluated in the original environment. */ @@ -1289,7 +1305,7 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v) for (auto & i : attrs->attrs) { env2.values[displ++] = i.second.e->maybeThunk( state, - *i.second.chooseByKind(&env2, &env, &env2)); + *i.second.chooseByKind(&env2, &env, inheritEnv)); } auto dts = state.debugRepl diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index 82e69de51..4b805d710 100644 --- a/src/libexpr/nixexpr.cc +++ b/src/libexpr/nixexpr.cc @@ -80,7 +80,7 @@ void ExprAttrs::showBindings(const SymbolTable & symbols, std::ostream & str) co return sa < sb; }); std::vector inherits; - std::map> inheritsFrom; + std::map> inheritsFrom; for (auto & i : sorted) { switch (i->second.kind) { case AttrDef::Kind::Plain: @@ -90,7 +90,8 @@ void ExprAttrs::showBindings(const SymbolTable & symbols, std::ostream & str) co break; case AttrDef::Kind::InheritedFrom: { auto & select = dynamic_cast(*i->second.e); - inheritsFrom[select.e].push_back(i->first); + auto & from = dynamic_cast(*select.e); + inheritsFrom[&from].push_back(i->first); break; } } @@ -102,7 +103,7 @@ void ExprAttrs::showBindings(const SymbolTable & symbols, std::ostream & str) co } for (const auto & [from, syms] : inheritsFrom) { str << "inherit ("; - from->show(symbols, str); + (*inheritFromExprs)[from->displ]->show(symbols, str); str << ")"; for (auto sym : syms) str << " " << symbols[sym]; str << "; "; @@ -328,6 +329,12 @@ void ExprVar::bindVars(EvalState & es, const std::shared_ptr & this->level = withLevel; } +void ExprInheritFrom::bindVars(EvalState & es, const std::shared_ptr & env) +{ + if (es.debugRepl) + es.exprEnvs.insert(std::make_pair(this, env)); +} + void ExprSelect::bindVars(EvalState & es, const std::shared_ptr & env) { if (es.debugRepl) @@ -351,6 +358,27 @@ void ExprOpHasAttr::bindVars(EvalState & es, const std::shared_ptrbindVars(es, env); } +std::shared_ptr ExprAttrs::bindInheritSources( + EvalState & es, const std::shared_ptr & env) +{ + if (!inheritFromExprs) + return nullptr; + + // the inherit (from) source values are inserted into an env of its own, which + // does not introduce any variable names. + // analysis must see an empty env, or an env that contains only entries with + // otherwise unused names to not interfere with regular names. the parser + // has already filled all exprs that access this env with appropriate level + // and displacement, and nothing else is allowed to access it. ideally we'd + // not even *have* an expr that grabs anything from this env since it's fully + // invisible, but the evaluator does not allow for this yet. + auto inner = std::make_shared(nullptr, env.get(), 0); + for (auto from : *inheritFromExprs) + from->bindVars(es, env); + + return inner; +} + void ExprAttrs::bindVars(EvalState & es, const std::shared_ptr & env) { if (es.debugRepl) @@ -368,8 +396,9 @@ void ExprAttrs::bindVars(EvalState & es, const std::shared_ptr // No need to sort newEnv since attrs is in sorted order. + auto inheritFromEnv = bindInheritSources(es, newEnv); for (auto & i : attrs) - i.second.e->bindVars(es, i.second.chooseByKind(newEnv, env, newEnv)); + i.second.e->bindVars(es, i.second.chooseByKind(newEnv, env, inheritFromEnv)); for (auto & i : dynamicAttrs) { i.nameExpr->bindVars(es, newEnv); @@ -377,8 +406,10 @@ void ExprAttrs::bindVars(EvalState & es, const std::shared_ptr } } else { + auto inheritFromEnv = bindInheritSources(es, env); + for (auto & i : attrs) - i.second.e->bindVars(es, i.second.chooseByKind(env, env, env)); + i.second.e->bindVars(es, i.second.chooseByKind(env, env, inheritFromEnv)); for (auto & i : dynamicAttrs) { i.nameExpr->bindVars(es, env); @@ -446,8 +477,9 @@ void ExprLet::bindVars(EvalState & es, const std::shared_ptr & // No need to sort newEnv since attrs->attrs is in sorted order. + auto inheritFromEnv = attrs->bindInheritSources(es, newEnv); for (auto & i : attrs->attrs) - i.second.e->bindVars(es, i.second.chooseByKind(newEnv, env, newEnv)); + i.second.e->bindVars(es, i.second.chooseByKind(newEnv, env, inheritFromEnv)); if (es.debugRepl) es.exprEnvs.insert(std::make_pair(this, newEnv)); diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 4a93143b4..4bb2ee2f9 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -135,6 +135,18 @@ struct ExprVar : Expr COMMON_METHODS }; +struct ExprInheritFrom : ExprVar +{ + ExprInheritFrom(PosIdx pos, Displacement displ): ExprVar(pos, {}) + { + this->level = 0; + this->displ = displ; + this->fromWith = nullptr; + } + + void bindVars(EvalState & es, const std::shared_ptr & env); +}; + struct ExprSelect : Expr { PosIdx pos; @@ -195,6 +207,7 @@ struct ExprAttrs : Expr }; typedef std::map AttrDefs; AttrDefs attrs; + std::unique_ptr> inheritFromExprs; struct DynamicAttrDef { Expr * nameExpr, * valueExpr; PosIdx pos; @@ -208,6 +221,9 @@ struct ExprAttrs : Expr PosIdx getPos() const override { return pos; } COMMON_METHODS + std::shared_ptr bindInheritSources( + EvalState & es, const std::shared_ptr & env); + Env * buildInheritFromEnv(EvalState & state, Env & up); void showBindings(const SymbolTable & symbols, std::ostream & str) const; }; diff --git a/src/libexpr/parser-state.hh b/src/libexpr/parser-state.hh index ae38de130..9aa18a0ae 100644 --- a/src/libexpr/parser-state.hh +++ b/src/libexpr/parser-state.hh @@ -118,13 +118,24 @@ inline void ParserState::addAttr(ExprAttrs * attrs, AttrPath && attrPath, Expr * auto ae = dynamic_cast(e); auto jAttrs = dynamic_cast(j->second.e); if (jAttrs && ae) { + if (ae->inheritFromExprs && !jAttrs->inheritFromExprs) + jAttrs->inheritFromExprs = std::make_unique>(); for (auto & ad : ae->attrs) { auto j2 = jAttrs->attrs.find(ad.first); if (j2 != jAttrs->attrs.end()) // Attr already defined in iAttrs, error. dupAttr(ad.first, j2->second.pos, ad.second.pos); jAttrs->attrs.emplace(ad.first, ad.second); + if (ad.second.kind == ExprAttrs::AttrDef::Kind::InheritedFrom) { + auto & sel = dynamic_cast(*ad.second.e); + auto & from = dynamic_cast(*sel.e); + from.displ += jAttrs->inheritFromExprs->size(); + } } jAttrs->dynamicAttrs.insert(jAttrs->dynamicAttrs.end(), ae->dynamicAttrs.begin(), ae->dynamicAttrs.end()); + if (ae->inheritFromExprs) { + jAttrs->inheritFromExprs->insert(jAttrs->inheritFromExprs->end(), + ae->inheritFromExprs->begin(), ae->inheritFromExprs->end()); + } } else { dupAttr(attrPath, pos, j->second.pos); } diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y index 0898b81f7..b0aee7b41 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -321,14 +321,17 @@ binds } | binds INHERIT '(' expr ')' attrs ';' { $$ = $1; - /* !!! Should ensure sharing of the expression in $4. */ + if (!$$->inheritFromExprs) + $$->inheritFromExprs = std::make_unique>(); + $$->inheritFromExprs->push_back($4); + auto from = new nix::ExprInheritFrom(state->at(@4), $$->inheritFromExprs->size() - 1); for (auto & i : *$6) { if ($$->attrs.find(i.symbol) != $$->attrs.end()) state->dupAttr(i.symbol, state->at(@6), $$->attrs[i.symbol].pos); $$->attrs.emplace( i.symbol, ExprAttrs::AttrDef( - new ExprSelect(CUR_POS, $4, i.symbol), + new ExprSelect(CUR_POS, from, i.symbol), state->at(@6), ExprAttrs::AttrDef::Kind::InheritedFrom)); } diff --git a/tests/functional/lang/eval-okay-inherit-from.err.exp b/tests/functional/lang/eval-okay-inherit-from.err.exp index 51881205b..3227501f2 100644 --- a/tests/functional/lang/eval-okay-inherit-from.err.exp +++ b/tests/functional/lang/eval-okay-inherit-from.err.exp @@ -1,2 +1 @@ trace: used -trace: used diff --git a/tests/functional/lang/eval-okay-inherit-from.exp b/tests/functional/lang/eval-okay-inherit-from.exp index 43bd0e899..024daff6b 100644 --- a/tests/functional/lang/eval-okay-inherit-from.exp +++ b/tests/functional/lang/eval-okay-inherit-from.exp @@ -1 +1 @@ -[ 1 2 { __overrides = { y = { d = [ ]; }; }; c = [ ]; d = 4; x = { c = [ ]; }; y = «repeated»; } ] +[ 1 2 { __overrides = { y = { d = [ ]; }; }; c = [ ]; d = 4; x = { c = [ ]; }; y = «repeated»; } { inner = { c = 3; d = 4; }; } ] diff --git a/tests/functional/lang/eval-okay-inherit-from.nix b/tests/functional/lang/eval-okay-inherit-from.nix index d1fad7d69..b72a1c639 100644 --- a/tests/functional/lang/eval-okay-inherit-from.nix +++ b/tests/functional/lang/eval-okay-inherit-from.nix @@ -2,5 +2,15 @@ let inherit (builtins.trace "used" { a = 1; b = 2; }) a b; x.c = 3; y.d = 4; + + merged = { + inner = { + inherit (y) d; + }; + + inner = { + inherit (x) c; + }; + }; in - [ a b rec { x.c = []; inherit (x) c; inherit (y) d; __overrides.y.d = []; } ] + [ a b rec { x.c = []; inherit (x) c; inherit (y) d; __overrides.y.d = []; } merged ] From 1cd87b7042d14aae1fafa47b1c28db4c5bd20de7 Mon Sep 17 00:00:00 2001 From: pennae Date: Mon, 26 Feb 2024 15:33:52 +0100 Subject: [PATCH 8/9] remove ExprAttrs::AttrDef::inherited it's no longer widely used and has a rather confusing meaning now that inherit-from is handled very differently. --- src/libexpr/eval.cc | 2 +- src/libexpr/nixexpr.hh | 2 -- src/libexpr/parser-state.hh | 2 +- 3 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc index a353571af..2e7c8207c 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1220,7 +1220,7 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v) Displacement displ = 0; for (auto & i : attrs) { Value * vAttr; - if (hasOverrides && !i.second.inherited()) { + if (hasOverrides && i.second.kind != AttrDef::Kind::Inherited) { vAttr = state.allocValue(); mkThunk(*vAttr, *i.second.chooseByKind(&env2, &env, inheritEnv), i.second.e); } else diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 4bb2ee2f9..2390c4286 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -189,8 +189,6 @@ struct ExprAttrs : Expr : kind(kind), e(e), pos(pos) { }; AttrDef() { }; - bool inherited() const { return kind == Kind::Inherited; } - template const T & chooseByKind(const T & plain, const T & inherited, const T & inheritedFrom) const { diff --git a/src/libexpr/parser-state.hh b/src/libexpr/parser-state.hh index 9aa18a0ae..34aef661f 100644 --- a/src/libexpr/parser-state.hh +++ b/src/libexpr/parser-state.hh @@ -89,7 +89,7 @@ inline void ParserState::addAttr(ExprAttrs * attrs, AttrPath && attrPath, Expr * if (i->symbol) { ExprAttrs::AttrDefs::iterator j = attrs->attrs.find(i->symbol); if (j != attrs->attrs.end()) { - if (!j->second.inherited()) { + if (j->second.kind != ExprAttrs::AttrDef::Kind::Inherited) { ExprAttrs * attrs2 = dynamic_cast(j->second.e); if (!attrs2) dupAttr(attrPath, pos, j->second.pos); attrs = attrs2; From f24e445bc024cfd3c26be5f061280af549321c22 Mon Sep 17 00:00:00 2001 From: pennae <82953136+pennae@users.noreply.github.com> Date: Mon, 26 Feb 2024 15:43:51 +0100 Subject: [PATCH 9/9] add doc comment justifying ExprInheritFrom Co-authored-by: Robert Hensing --- src/libexpr/nixexpr.hh | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh index 2390c4286..94356759b 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -135,6 +135,11 @@ struct ExprVar : Expr COMMON_METHODS }; +/** + * A pseudo-expression for the purpose of evaluating the `from` expression in `inherit (from)` syntax. + * Unlike normal variable references, the displacement is set during parsing, and always refers to + * `ExprAttrs::inheritFromExprs` (by itself or in `ExprLet`), whose values are put into their own `Env`. + */ struct ExprInheritFrom : ExprVar { ExprInheritFrom(PosIdx pos, Displacement displ): ExprVar(pos, {})