deduplicate inherit-from source expr work

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.

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();
@@ -1207,11 +1220,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));
         }
@@ -1243,9 +1256,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) {
@@ -1277,12 +1296,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(

src/libexpr/nixexpr.cc

@@ -70,21 +70,46 @@ 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<Attr> sorted;
     for (auto & i : attrs) sorted.push_back(&i);
     std::sort(sorted.begin(), sorted.end(), [&](Attr a, Attr b) {
         std::string_view sa = symbols[a->first], sb = symbols[b->first];
         return sa < sb;
     });
+    std::vector<Symbol> inherits;
+    std::map<ExprInheritFrom *, std::vector<Symbol>> 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<ExprSelect &>(*i->second.e);
+            auto & from = dynamic_cast<ExprInheritFrom &>(*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<const StaticEnv> &
     this->level = withLevel;
 }
 
+void ExprInheritFrom::bindVars(EvalState & es, const std::shared_ptr<const StaticEnv> & env)
+{
+    if (es.debugRepl)
+        es.exprEnvs.insert(std::make_pair(this, env));
+}
+
 void ExprSelect::bindVars(EvalState & es, const std::shared_ptr<const StaticEnv> & env)
 {
     if (es.debugRepl)
@@ -328,31 +358,58 @@ void ExprOpHasAttr::bindVars(EvalState & es, const std::shared_ptr<const StaticE
             i.expr->bindVars(es, env);
 }
 
+std::shared_ptr<const StaticEnv> ExprAttrs::bindInheritSources(
+    EvalState & es, const std::shared_ptr<const StaticEnv> & 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<StaticEnv>(nullptr, env.get(), 0);
+    for (auto from : *inheritFromExprs)
+        from->bindVars(es, env);
+
+    return inner;
+}
+
 void ExprAttrs::bindVars(EvalState & es, const std::shared_ptr<const StaticEnv> & env)
 {
     if (es.debugRepl)
         es.exprEnvs.insert(std::make_pair(this, env));
 
     if (recursive) {
-        auto newEnv = std::make_shared<StaticEnv>(nullptr, env.get(), recursive ? attrs.size() : 0);
+        auto newEnv = [&] () -> std::shared_ptr<const StaticEnv> {
+            auto newEnv = std::make_shared<StaticEnv>(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);
             i.valueExpr->bindVars(es, newEnv);
         }
     }
     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<const StaticEnv> &
 
 void ExprLet::bindVars(EvalState & es, const std::shared_ptr<const StaticEnv> & env)
 {
-    auto newEnv = std::make_shared<StaticEnv>(nullptr, env.get(), attrs->attrs.size());
+    auto newEnv = [&] () -> std::shared_ptr<const StaticEnv> {
+        auto newEnv = std::make_shared<StaticEnv>(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));

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<const StaticEnv> & 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<typename T>
+        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<Symbol, AttrDef> AttrDefs;
     AttrDefs attrs;
+    std::unique_ptr<std::vector<Expr *>> 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<const StaticEnv> bindInheritSources(
+        EvalState & es, const std::shared_ptr<const StaticEnv> & env);
+    Env * buildInheritFromEnv(EvalState & state, Env & up);
+    void showBindings(const SymbolTable & symbols, std::ostream & str) const;
 };
 
 struct ExprList : Expr

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<ExprAttrs *>(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<ExprAttrs *>(e);
             auto jAttrs = dynamic_cast<ExprAttrs *>(j->second.e);
             if (jAttrs && ae) {
+                if (ae->inheritFromExprs && !jAttrs->inheritFromExprs)
+                    jAttrs->inheritFromExprs = std::make_unique<std::vector<Expr *>>();
                 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<ExprSelect &>(*ad.second.e);
+                        auto & from = dynamic_cast<ExprInheritFrom &>(*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);
             }