Simplify inherited attribute handling

This reduces the difference between inherited and non-inherited
attribute handling to the choice of which env to use (in recs and lets)
by setting the AttrDef::e to a new ExprVar in the parser rather than
carrying a separate AttrDef::v VarRef member.

As an added bonus, this allows inherited attributes that inherit from a
with to delay forcing evaluation of the with's attributes.

Signed-off-by: Shea Levy <shea@shealevy.com>

7 files changed, 46 insertions, 38 deletions

diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc
index 4fecd2a70..82287f627 100644
--- a/src/libexpr/eval.cc
+++ b/src/libexpr/eval.cc
@@ -521,23 +521,17 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v)
            environment, while the inherited attributes are evaluated
            in the original environment. */
         unsigned int displ = 0;
-        foreach (AttrDefs::iterator, i, attrs)
-            if (i->second.inherited) {
-                /* !!! handle overrides? */
-                Value * vAttr = state.lookupVar(&env, i->second.var);
-                env2.values[displ++] = vAttr;
-                v.attrs->push_back(Attr(i->first, vAttr, &i->second.pos));
-            } else {
-                Value * vAttr;
-                if (hasOverrides) {
-                    vAttr = state.allocValue();
-                    mkThunk(*vAttr, env2, i->second.e);
-                } else
-                    vAttr = i->second.e->maybeThunk(state, env2);
-                env2.values[displ++] = vAttr;
-                v.attrs->push_back(Attr(i->first, vAttr, &i->second.pos));
-            }
-        
+        foreach (AttrDefs::iterator, i, attrs) {
+            Value * vAttr;
+            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);
+            env2.values[displ++] = vAttr;
+            v.attrs->push_back(Attr(i->first, vAttr, &i->second.pos));
+        }
+
         /* If the rec contains an attribute called `__overrides', then
            evaluate it, and add the attributes in that set to the rec.
            This allows overriding of recursive attributes, which is
@@ -563,10 +557,7 @@ void ExprAttrs::eval(EvalState & state, Env & env, Value & v)
 
     else {
         foreach (AttrDefs::iterator, i, attrs)
-            if (i->second.inherited)
-                v.attrs->push_back(Attr(i->first, state.lookupVar(&env, i->second.var), &i->second.pos));
-            else
-                v.attrs->push_back(Attr(i->first, i->second.e->maybeThunk(state, env), &i->second.pos));
+            v.attrs->push_back(Attr(i->first, i->second.e->maybeThunk(state, env), &i->second.pos));
     }
 }
 
@@ -583,10 +574,7 @@ void ExprLet::eval(EvalState & state, Env & env, Value & v)
        environment. */
     unsigned int displ = 0;
     foreach (ExprAttrs::AttrDefs::iterator, i, attrs->attrs)
-        if (i->second.inherited)
-            env2.values[displ++] = state.lookupVar(&env, i->second.var);
-        else
-            env2.values[displ++] = i->second.e->maybeThunk(state, env2);
+        env2.values[displ++] = i->second.e->maybeThunk(state, i->second.inherited ? env : env2);
 
     body->eval(state, env2, v);
 }
@@ -602,7 +590,7 @@ void ExprList::eval(EvalState & state, Env & env, Value & v)
 
 void ExprVar::eval(EvalState & state, Env & env, Value & v)
 {
-    Value * v2 = state.lookupVar(&env, info);
+    Value * v2 = state.lookupVar(&env, info, false);
     state.forceValue(*v2);
     v = *v2;
 }
diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh
index 9717fb66d..b7b527bc4 100644
--- a/src/libexpr/eval.hh
+++ b/src/libexpr/eval.hh
@@ -206,7 +206,7 @@ private:
     void addPrimOp(const string & name,
         unsigned int arity, PrimOpFun primOp);
 
-    inline Value * lookupVar(Env * env, const VarRef & var, bool noEval = false);
+    inline Value * lookupVar(Env * env, const VarRef & var, bool noEval);
     
     friend class ExprVar;
     friend class ExprAttrs;
diff --git a/src/libexpr/nixexpr.cc b/src/libexpr/nixexpr.cc
index 9b75bb644..72bc0a14b 100644
--- a/src/libexpr/nixexpr.cc
+++ b/src/libexpr/nixexpr.cc
@@ -230,14 +230,12 @@ void ExprAttrs::bindVars(const StaticEnv & env)
             newEnv.vars[i->first] = i->second.displ = displ++;
         
         foreach (AttrDefs::iterator, i, attrs)
-            if (i->second.inherited) i->second.var.bind(env);
-            else i->second.e->bindVars(newEnv);
+            i->second.e->bindVars(i->second.inherited ? env : newEnv);
     }
 
     else
         foreach (AttrDefs::iterator, i, attrs)
-            if (i->second.inherited) i->second.var.bind(env);
-            else i->second.e->bindVars(env);
+            i->second.e->bindVars(env);
 }
 
 void ExprList::bindVars(const StaticEnv & env)
@@ -274,8 +272,7 @@ void ExprLet::bindVars(const StaticEnv & env)
         newEnv.vars[i->first] = i->second.displ = displ++;
     
     foreach (ExprAttrs::AttrDefs::iterator, i, attrs->attrs)
-        if (i->second.inherited) i->second.var.bind(env);
-        else i->second.e->bindVars(newEnv);
+        i->second.e->bindVars(i->second.inherited ? env : newEnv);
     
     body->bindVars(newEnv);
 }
diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh
index f6dcb3935..69372a77b 100644
--- a/src/libexpr/nixexpr.hh
+++ b/src/libexpr/nixexpr.hh
@@ -159,12 +159,10 @@ struct ExprAttrs : Expr
     bool recursive;
     struct AttrDef {
         bool inherited;
-        Expr * e; // if not inherited
-        VarRef var; // if inherited
+        Expr * e;
         Pos pos;
         unsigned int displ; // displacement
-        AttrDef(Expr * e, const Pos & pos) : inherited(false), e(e), pos(pos) { };
-        AttrDef(const Symbol & name, const Pos & pos) : inherited(true), var(name), pos(pos) { };
+        AttrDef(Expr * e, const Pos & pos, bool inherited=false) : inherited(inherited), e(e), pos(pos) { };
         AttrDef() { };
     };
     typedef std::map<Symbol, AttrDef> AttrDefs;
diff --git a/src/libexpr/parser.y b/src/libexpr/parser.y
index 7690271a8..0e3086004 100644
--- a/src/libexpr/parser.y
+++ b/src/libexpr/parser.y
@@ -413,7 +413,7 @@ binds
           if ($$->attrs.find(*i) != $$->attrs.end())
               dupAttr(*i, makeCurPos(@3, data), $$->attrs[*i].pos);
           Pos pos = makeCurPos(@3, data);
-          $$->attrs[*i] = ExprAttrs::AttrDef(*i, pos);
+          $$->attrs[*i] = ExprAttrs::AttrDef(new ExprVar(*i), pos, true);
       }
     }
   | binds INHERIT '(' expr ')' attrs ';'
diff --git a/tests/lang/eval-okay-delayed-with-inherit.exp b/tests/lang/eval-okay-delayed-with-inherit.exp
new file mode 100644
index 000000000..eaacb55c1
--- /dev/null
+++ b/tests/lang/eval-okay-delayed-with-inherit.exp
@@ -0,0 +1 @@
+"b-overridden"
diff --git a/tests/lang/eval-okay-delayed-with-inherit.nix b/tests/lang/eval-okay-delayed-with-inherit.nix
new file mode 100644
index 000000000..84b388c27
--- /dev/null
+++ b/tests/lang/eval-okay-delayed-with-inherit.nix
@@ -0,0 +1,24 @@
+let
+  pkgs_ = with pkgs; {
+    a = derivation {
+      name = "a";
+      system = builtins.currentSystem;
+      builder = "/bin/sh";
+      args = [ "-c" "touch $out" ];
+      inherit b;
+    };
+
+    inherit b;
+  };
+
+  packageOverrides = p: {
+    b = derivation {
+      name = "b-overridden";
+      system = builtins.currentSystem;
+      builder = "/bin/sh";
+      args = [ "-c" "touch $out" ];
+    };
+  };
+
+  pkgs = pkgs_ // (packageOverrides pkgs_);
+in pkgs.a.b.name