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 c4e163b08..e51856358 100644 --- a/src/libexpr/eval.cc +++ b/src/libexpr/eval.cc @@ -1198,6 +1198,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()); @@ -1209,6 +1221,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(); @@ -1219,11 +1232,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.kind != AttrDef::Kind::Inherited) { vAttr = state.allocValue(); - mkThunk(*vAttr, env2, i.second.e); + mkThunk(*vAttr, *i.second.chooseByKind(&env2, &env, inheritEnv), 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, inheritEnv)); env2.values[displ++] = vAttr; v.attrs->push_back(Attr(i.first, vAttr, i.second.pos)); } @@ -1255,9 +1268,15 @@ 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 { + 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, inheritEnv)), + i.second.pos)); + } + } /* Dynamic attrs apply *after* rec and __overrides. */ for (auto & i : dynamicAttrs) { @@ -1289,12 +1308,17 @@ 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. */ 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, inheritEnv)); + } auto dts = state.debugRepl ? makeDebugTraceStacker( diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc index 46737fea6..4b805d710 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); @@ -81,10 +79,37 @@ void ExprAttrs::show(const SymbolTable & symbols, std::ostream & str) const 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); + auto & from = dynamic_cast(*select.e); + inheritsFrom[&from].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 ("; + (*inheritFromExprs)[from->displ]->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 << "; "; @@ -97,6 +122,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 +184,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 << ")"; @@ -305,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) @@ -328,22 +358,47 @@ 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) 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. + auto inheritFromEnv = bindInheritSources(es, newEnv); for (auto & i : attrs) - i.second.e->bindVars(es, i.second.inherited ? env : newEnv); + i.second.e->bindVars(es, i.second.chooseByKind(newEnv, env, inheritFromEnv)); for (auto & i : dynamicAttrs) { i.nameExpr->bindVars(es, newEnv); @@ -351,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, env); + i.second.e->bindVars(es, i.second.chooseByKind(env, env, inheritFromEnv)); for (auto & i : dynamicAttrs) { i.nameExpr->bindVars(es, env); @@ -409,16 +466,20 @@ 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. + auto inheritFromEnv = attrs->bindInheritSources(es, newEnv); 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, 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 1f944f10b..94356759b 100644 --- a/src/libexpr/nixexpr.hh +++ b/src/libexpr/nixexpr.hh @@ -135,6 +135,23 @@ 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, {}) + { + this->level = 0; + this->displ = displ; + this->fromWith = nullptr; + } + + void bindVars(EvalState & es, const std::shared_ptr & env); +}; + struct ExprSelect : Expr { PosIdx pos; @@ -160,16 +177,40 @@ 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() { }; + + 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; + std::unique_ptr> inheritFromExprs; struct DynamicAttrDef { Expr * nameExpr, * valueExpr; PosIdx pos; @@ -182,6 +223,11 @@ struct ExprAttrs : Expr ExprAttrs() : recursive(false) { }; 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; }; struct ExprList : Expr diff --git a/src/libexpr/parser-state.hh b/src/libexpr/parser-state.hh index 87aeaeef5..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; @@ -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 a3ba13c66..b0aee7b41 100644 --- a/src/libexpr/parser.y +++ b/src/libexpr/parser.y @@ -313,17 +313,27 @@ 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; } | 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), state->at(@6))); + $$->attrs.emplace( + i.symbol, + ExprAttrs::AttrDef( + new ExprSelect(CUR_POS, from, i.symbol), + state->at(@6), + ExprAttrs::AttrDef::Kind::InheritedFrom)); } delete $6; } 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..3227501f2 --- /dev/null +++ b/tests/functional/lang/eval-okay-inherit-from.err.exp @@ -0,0 +1 @@ +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..024daff6b --- /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»; } { 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 new file mode 100644 index 000000000..b72a1c639 --- /dev/null +++ b/tests/functional/lang/eval-okay-inherit-from.nix @@ -0,0 +1,16 @@ +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 = []; } merged ] diff --git a/tests/functional/lang/parse-okay-inherits.exp b/tests/functional/lang/parse-okay-inherits.exp new file mode 100644 index 000000000..1355527e6 --- /dev/null +++ b/tests/functional/lang/parse-okay-inherits.exp @@ -0,0 +1 @@ +(let b = 2; c = { }; in { inherit b; inherit (c) d e; a = 1; 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; +} 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); }))