Never update values after setting the type

Thunks are now overwritten by a helper function `Value::finishValue(newType, payload)` (where `payload` is the original anonymous union inside `Value`). This helps to ensure we never update a value elsewhere, since that would be incompatible with parallel evaluation (i.e. after a value has transitioned from being a thunk to being a non-thunk, it should be immutable). There were two places where this happened: `Value::mkString()` and `ExprAttrs::eval()`. This PR also adds a bunch of accessor functions for value contents, like `Value::integer()` to access the integer field in the union.
NixOS · Mar 25, 2024 · 8c0590f · 8c0590f
1 parent 6d90287
commit 8c0590f
Show file tree

Hide file tree

Showing 35 changed files with 530 additions and 556 deletions.
diff --git a/src/libcmd/installable-flake.cc b/src/libcmd/installable-flake.cc
@@ -49,7 +49,7 @@ Value * InstallableFlake::getFlakeOutputs(EvalState & state, const flake::Locked
 
     callFlake(state, lockedFlake, *vFlake);
 
-    auto aOutputs = vFlake->attrs->get(state.symbols.create("outputs"));
+    auto aOutputs = vFlake->attrs()->get(state.symbols.create("outputs"));
     assert(aOutputs);
 
     state.forceValue(*aOutputs->value, aOutputs->value->determinePos(noPos));

diff --git a/src/libcmd/installables.cc b/src/libcmd/installables.cc
@@ -289,7 +289,7 @@ void SourceExprCommand::completeInstallable(AddCompletions & completions, std::s
             state->autoCallFunction(*autoArgs, v1, v2);
 
             if (v2.type() == nAttrs) {
-                for (auto & i : *v2.attrs) {
+                for (auto & i : *v2.attrs()) {
                     std::string name = state->symbols[i.name];
                     if (name.find(searchWord) == 0) {
                         if (prefix_ == "")
@@ -461,7 +461,7 @@ ref<eval_cache::EvalCache> openEvalCache(
 
             state.forceAttrs(*vFlake, noPos, "while parsing cached flake data");
 
-            auto aOutputs = vFlake->attrs->get(state.symbols.create("outputs"));
+            auto aOutputs = vFlake->attrs()->get(state.symbols.create("outputs"));
             assert(aOutputs);
 
             return aOutputs->value;

diff --git a/src/libcmd/repl.cc b/src/libcmd/repl.cc
@@ -290,7 +290,7 @@ StringSet NixRepl::completePrefix(const std::string & prefix)
             e->eval(*state, *env, v);
             state->forceAttrs(v, noPos, "while evaluating an attrset for the purpose of completion (this error should not be displayed; file an issue?)");
 
-            for (auto & i : *v.attrs) {
+            for (auto & i : *v.attrs()) {
                 std::string_view name = state->symbols[i.name];
                 if (name.substr(0, cur2.size()) != cur2) continue;
                 completions.insert(concatStrings(prev, expr, ".", name));
@@ -490,7 +490,7 @@ ProcessLineResult NixRepl::processLine(std::string line)
                 auto path = state->coerceToPath(noPos, v, context, "while evaluating the filename to edit");
                 return {path, 0};
             } else if (v.isLambda()) {
-                auto pos = state->positions[v.lambda.fun->pos];
+                auto pos = state->positions[v.payload.lambda.fun->pos];
                 if (auto path = std::get_if<SourcePath>(&pos.origin))
                     return {*path, pos.line};
                 else
@@ -742,17 +742,17 @@ void NixRepl::loadFiles()
 void NixRepl::addAttrsToScope(Value & attrs)
 {
     state->forceAttrs(attrs, [&]() { return attrs.determinePos(noPos); }, "while evaluating an attribute set to be merged in the global scope");
-    if (displ + attrs.attrs->size() >= envSize)
+    if (displ + attrs.attrs()->size() >= envSize)
         throw Error("environment full; cannot add more variables");
 
-    for (auto & i : *attrs.attrs) {
+    for (auto & i : *attrs.attrs()) {
         staticEnv->vars.emplace_back(i.name, displ);
         env->values[displ++] = i.value;
         varNames.emplace(state->symbols[i.name]);
     }
     staticEnv->sort();
     staticEnv->deduplicate();
-    notice("Added %1% variables.", attrs.attrs->size());
+    notice("Added %1% variables.", attrs.attrs()->size());
 }
 
 

diff --git a/src/libexpr/attr-path.cc b/src/libexpr/attr-path.cc
@@ -72,10 +72,10 @@ std::pair<Value *, PosIdx> findAlongAttrPath(EvalState & state, const std::strin
             if (attr.empty())
                 throw Error("empty attribute name in selection path '%1%'", attrPath);
 
-            Bindings::iterator a = v->attrs->find(state.symbols.create(attr));
-            if (a == v->attrs->end()) {
+            auto a = v->attrs()->get(state.symbols.create(attr));
+            if (!a) {
                 std::set<std::string> attrNames;
-                for (auto & attr : *v->attrs)
+                for (auto & attr : *v->attrs())
                     attrNames.insert(state.symbols[attr.name]);
 
                 auto suggestions = Suggestions::bestMatches(attrNames, attr);

diff --git a/src/libexpr/attr-set.cc b/src/libexpr/attr-set.cc
@@ -23,23 +23,6 @@ Bindings * EvalState::allocBindings(size_t capacity)
 }
 
 
-/* Create a new attribute named 'name' on an existing attribute set stored
-   in 'vAttrs' and return the newly allocated Value which is associated with
-   this attribute. */
-Value * EvalState::allocAttr(Value & vAttrs, Symbol name)
-{
-    Value * v = allocValue();
-    vAttrs.attrs->push_back(Attr(name, v));
-    return v;
-}
-
-
-Value * EvalState::allocAttr(Value & vAttrs, std::string_view name)
-{
-    return allocAttr(vAttrs, symbols.create(name));
-}
-
-
 Value & BindingsBuilder::alloc(Symbol name, PosIdx pos)
 {
     auto value = state.allocValue();

diff --git a/src/libexpr/attr-set.hh b/src/libexpr/attr-set.hh
@@ -65,39 +65,49 @@ public:
 
     typedef Attr * iterator;
 
+    typedef const Attr * const_iterator;
+
     void push_back(const Attr & attr)
     {
         assert(size_ < capacity_);
         attrs[size_++] = attr;
     }
 
-    iterator find(Symbol name)
+    const_iterator find(Symbol name) const
     {
         Attr key(name, 0);
-        iterator i = std::lower_bound(begin(), end(), key);
+        const_iterator i = std::lower_bound(begin(), end(), key);
         if (i != end() && i->name == name) return i;
         return end();
     }
 
-    Attr * get(Symbol name)
+    const Attr * get(Symbol name) const
     {
         Attr key(name, 0);
-        iterator i = std::lower_bound(begin(), end(), key);
+        const_iterator i = std::lower_bound(begin(), end(), key);
         if (i != end() && i->name == name) return &*i;
         return nullptr;
     }
 
     iterator begin() { return &attrs[0]; }
     iterator end() { return &attrs[size_]; }
 
+    const_iterator begin() const { return &attrs[0]; }
+    const_iterator end() const { return &attrs[size_]; }
+
     Attr & operator[](size_t pos)
     {
         return attrs[pos];
     }
 
+    const Attr & operator[](size_t pos) const
+    {
+        return attrs[pos];
+    }
+
     void sort();
 
-    size_t capacity() { return capacity_; }
+    size_t capacity() const { return capacity_; }
 
     /**
      * Returns the attributes in lexicographically sorted order.
@@ -166,6 +176,20 @@ public:
     {
         return bindings;
     }
+
+    size_t capacity()
+    {
+        return bindings->capacity();
+    }
+
+    void grow(Bindings * newBindings)
+    {
+        for (auto & i : *bindings)
+            newBindings->push_back(i);
+        bindings = newBindings;
+    }
+
+    friend class ExprAttrs;
 };
 
 }
diff --git a/src/libexpr/eval-cache.cc b/src/libexpr/eval-cache.cc
@@ -387,7 +387,7 @@ Value & AttrCursor::getValue()
         if (parent) {
             auto & vParent = parent->first->getValue();
             root->state.forceAttrs(vParent, noPos, "while searching for an attribute");
-            auto attr = vParent.attrs->get(parent->second);
+            auto attr = vParent.attrs()->get(parent->second);
             if (!attr)
                 throw Error("attribute '%s' is unexpectedly missing", getAttrPathStr());
             _value = allocRootValue(attr->value);
@@ -448,9 +448,9 @@ Value & AttrCursor::forceValue()
             cachedValue = {root->db->setString(getKey(), path.abs()), string_t{path.abs(), {}}};
         }
         else if (v.type() == nBool)
-            cachedValue = {root->db->setBool(getKey(), v.boolean), v.boolean};
+            cachedValue = {root->db->setBool(getKey(), v.boolean()), v.boolean()};
         else if (v.type() == nInt)
-            cachedValue = {root->db->setInt(getKey(), v.integer), int_t{v.integer}};
+            cachedValue = {root->db->setInt(getKey(), v.integer()), int_t{v.integer()}};
         else if (v.type() == nAttrs)
             ; // FIXME: do something?
         else
@@ -510,7 +510,7 @@ std::shared_ptr<AttrCursor> AttrCursor::maybeGetAttr(Symbol name, bool forceErro
         return nullptr;
         //error<TypeError>("'%s' is not an attribute set", getAttrPathStr()).debugThrow();
 
-    auto attr = v.attrs->get(name);
+    auto attr = v.attrs()->get(name);
 
     if (!attr) {
         if (root->db) {
@@ -652,7 +652,7 @@ bool AttrCursor::getBool()
     if (v.type() != nBool)
         root->state.error<TypeError>("'%s' is not a Boolean", getAttrPathStr()).debugThrow();
 
-    return v.boolean;
+    return v.boolean();
 }
 
 NixInt AttrCursor::getInt()
@@ -674,7 +674,7 @@ NixInt AttrCursor::getInt()
     if (v.type() != nInt)
         root->state.error<TypeError>("'%s' is not an integer", getAttrPathStr()).debugThrow();
 
-    return v.integer;
+    return v.integer();
 }
 
 std::vector<std::string> AttrCursor::getListOfStrings()
@@ -730,7 +730,7 @@ std::vector<Symbol> AttrCursor::getAttrs()
         root->state.error<TypeError>("'%s' is not an attribute set", getAttrPathStr()).debugThrow();
 
     std::vector<Symbol> attrs;
-    for (auto & attr : *getValue().attrs)
+    for (auto & attr : *getValue().attrs())
         attrs.push_back(attr.name);
     std::sort(attrs.begin(), attrs.end(), [&](Symbol a, Symbol b) {
         std::string_view sa = root->state.symbols[a], sb = root->state.symbols[b];

diff --git a/src/libexpr/eval-inline.hh b/src/libexpr/eval-inline.hh
@@ -85,8 +85,8 @@ Env & EvalState::allocEnv(size_t size)
 void EvalState::forceValue(Value & v, const PosIdx pos)
 {
     if (v.isThunk()) {
-        Env * env = v.thunk.env;
-        Expr * expr = v.thunk.expr;
+        Env * env = v.payload.thunk.env;
+        Expr * expr = v.payload.thunk.expr;
         try {
             v.mkBlackhole();
             //checkInterrupt();
@@ -98,7 +98,7 @@ void EvalState::forceValue(Value & v, const PosIdx pos)
         }
     }
     else if (v.isApp())
-        callFunction(*v.app.left, *v.app.right, v, pos);
+        callFunction(*v.payload.app.left, *v.payload.app.right, v, pos);
 }