libexpr: Remove the source accessor from path values and ExprPaths

Co-authored-by: Eelco Dolstra <[email protected]>

We can use the rootFS accessor for everything and just mount any other
accessor (like corepkgsFS) on top of that.

1. removes 16 bytes per ExprPath
2. enables us in the future to shrink path value payload to 8 bytes
3. enables us in the future to directly refer to path values where we
are currently referring to ExrPaths that forward us to a path value
4. enables getting rid of enable_shared_from_this<SourceAccessor>

Author: Radvendii

src/libcmd/installable-value.cc

@@ -43,7 +43,7 @@ std::optional<DerivedPathWithInfo>
 InstallableValue::trySinglePathToDerivedPaths(Value & v, const PosIdx pos, std::string_view errorCtx)
 {
     if (v.type() == nPath) {
-        auto storePath = fetchToStore(state->fetchSettings, *state->store, v.path(), FetchMode::Copy);
+        auto storePath = fetchToStore(state->fetchSettings, *state->store, state->rootPath(v.path()), FetchMode::Copy);
         return {{
             .path =
                 DerivedPath::Opaque{

src/libexpr-c/nix_api_value.cc

@@ -514,7 +514,7 @@ nix_err nix_init_path_string(nix_c_context * context, EvalState * s, nix_value *
         context->last_err_code = NIX_OK;
     try {
         auto & v = check_value_out(value);
-        v.mkPath(s->state.rootPath(nix::CanonPath(str)));
+        v.mkPath(nix::CanonPath(str));
     }
     NIXC_CATCH_ERRS
 }

src/libexpr-test-support/include/nix/expr/tests/libexpr.hh

@@ -139,8 +139,8 @@ MATCHER_P(IsPathEq, p, fmt("Is a path equal to \"%1%\"", p))
         return false;
     } else {
         auto path = arg.path();
-        if (path.path != CanonPath(p)) {
-            *result_listener << "Expected a path that equals \"" << p << "\" but got: " << path.path;
+        if (path != CanonPath(p)) {
+            *result_listener << "Expected a path that equals \"" << p << "\" but got: " << path;
             return false;
         }
     }

src/libexpr-tests/json.cc

@@ -72,7 +72,7 @@ TEST_F(JSONValueTest, StringQuotes)
 TEST_F(JSONValueTest, DISABLED_Path)
 {
     Value v;
-    v.mkPath(state.rootPath(CanonPath("/test")));
+    v.mkPath("/test");
     ASSERT_EQ(getJSONValue(v), "\"/nix/store/g1w7hy3qg1w7hy3qg1w7hy3qg1w7hy3q-x\"");
 }
 } /* namespace nix */

src/libexpr-tests/value/print.cc

@@ -353,7 +353,7 @@ TEST_F(ValuePrintingTests, ansiColorsStringElided)
 TEST_F(ValuePrintingTests, ansiColorsPath)
 {
     Value v;
-    v.mkPath(state.rootPath(CanonPath("puppy")));
+    v.mkPath("/puppy");
 
     test(v, ANSI_GREEN "/puppy" ANSI_NORMAL, PrintOptions{.ansiColors = true});
 }

src/libexpr/eval-cache.cc

@@ -408,8 +408,8 @@ Value & AttrCursor::forceValue()
         if (v.type() == nString)
             cachedValue = {root->db->setString(getKey(), v.c_str(), v.context()), string_t{v.c_str(), {}}};
         else if (v.type() == nPath) {
-            auto path = v.path().path;
-            cachedValue = {root->db->setString(getKey(), path.abs()), string_t{path.abs(), {}}};
+            auto path = v.path().abs();
+            cachedValue = {root->db->setString(getKey(), path), string_t{path, {}}};
         } else if (v.type() == nBool)
             cachedValue = {root->db->setBool(getKey(), v.boolean()), v.boolean()};
         else if (v.type() == nInt)
@@ -541,7 +541,7 @@ std::string AttrCursor::getString()
     if (v.type() != nString && v.type() != nPath)
         root->state.error<TypeError>("'%s' is not a string but %s", getAttrPathStr(), showType(v)).debugThrow();
 
-    return v.type() == nString ? v.c_str() : v.path().to_string();
+    return v.type() == nString ? v.c_str() : v.path().abs();
 }
 
 string_t AttrCursor::getStringWithContext()
@@ -582,7 +582,7 @@ string_t AttrCursor::getStringWithContext()
         copyContext(v, context);
         return {v.c_str(), std::move(context)};
     } else if (v.type() == nPath)
-        return {v.path().to_string(), {}};
+        return {v.path().abs(), {}};
     else
         root->state.error<TypeError>("'%s' is not a string but %s", getAttrPathStr(), showType(v)).debugThrow();
 }

src/libexpr/eval.cc

@@ -213,6 +213,8 @@ EvalState::EvalState(
     , settings{settings}
     , symbols(StaticEvalSymbols::staticSymbolTable())
     , repair(NoRepair)
+    , corepkgsFS(make_ref<MemorySourceAccessor>())
+    , corepkgsPath(StorePath::random("source"))
     , storeFS(makeMountedSourceAccessor({
           {CanonPath::root, makeEmptySourceAccessor()},
           /* In the pure eval case, we can simply require
@@ -266,7 +268,6 @@ EvalState::EvalState(
 
         return accessor;
     }())
-    , corepkgsFS(make_ref<MemorySourceAccessor>())
     , internalFS(make_ref<MemorySourceAccessor>())
     , derivationInternal{corepkgsFS->addFile(
           CanonPath("derivation-internal.nix"),
@@ -293,6 +294,9 @@ EvalState::EvalState(
     corepkgsFS->setPathDisplay("<nix", ">");
     internalFS->setPathDisplay("«nix-internal»", "");
 
+    storeFS->mount(CanonPath(store->printStorePath(corepkgsPath)), corepkgsFS);
+    allowPath(corepkgsPath);
+
     countCalls = getEnv("NIX_COUNT_CALLS").value_or("0") != "0";
 
     static_assert(sizeof(Env) <= 16, "environment must be <= 16 bytes");
@@ -852,9 +856,9 @@ void Value::mkStringMove(const char * s, const NixStringContext & context)
     mkStringNoCopy(s, encodeContext(context));
 }
 
-void Value::mkPath(const SourcePath & path)
+void Value::mkPath(const CanonPath & path)
 {
-    mkPath(&*path.accessor, makeImmutableString(path.path.abs()));
+    mkPath(makeImmutableString(path.abs()));
 }
 
 inline Value * EvalState::lookupVar(Env * env, const ExprVar & var, bool noEval)
@@ -2127,7 +2131,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
                 .atPos(pos)
                 .withFrame(env, *this)
                 .debugThrow();
-        v.mkPath(state.rootPath(CanonPath(str())));
+        v.mkPath(CanonPath(str()));
     } else
         v.mkStringMove(c_str(), context);
 }
@@ -2388,8 +2392,8 @@ BackedStringView EvalState::coerceToString(
                    ? // FIXME: hack to preserve path literals that end in a
                      // slash, as in /foo/${x}.
                    v.pathStr()
-                   : copyToStore ? store->printStorePath(copyPathToStore(context, v.path()))
-                                 : std::string(v.path().path.abs());
+                   : copyToStore ? store->printStorePath(copyPathToStore(context, rootPath(v.path())))
+                                 : std::string(v.path().abs());
     }
 
     if (v.type() == nAttrs) {
@@ -2498,7 +2502,7 @@ SourcePath EvalState::coerceToPath(const PosIdx pos, Value & v, NixStringContext
 
     /* Handle path values directly, without coercing to a string. */
     if (v.type() == nPath)
-        return v.path();
+        return rootPath(v.path());
 
     /* Similarly, handle __toString where the result may be a path
        value. */
@@ -2652,13 +2656,6 @@ void EvalState::assertEqValues(Value & v1, Value & v2, const PosIdx pos, std::st
         return;
 
     case nPath:
-        if (v1.pathAccessor() != v2.pathAccessor()) {
-            error<AssertionError>(
-                "path '%s' is not equal to path '%s' because their accessors are different",
-                ValuePrinter(*this, v1, errorPrintOptions),
-                ValuePrinter(*this, v2, errorPrintOptions))
-                .debugThrow();
-        }
         if (strcmp(v1.pathStr(), v2.pathStr()) != 0) {
             error<AssertionError>(
                 "path '%s' is not equal to path '%s'",
@@ -2828,9 +2825,7 @@ bool EvalState::eqValues(Value & v1, Value & v2, const PosIdx pos, std::string_v
         return strcmp(v1.c_str(), v2.c_str()) == 0;
 
     case nPath:
-        return
-            // FIXME: compare accessors by their fingerprint.
-            v1.pathAccessor() == v2.pathAccessor() && strcmp(v1.pathStr(), v2.pathStr()) == 0;
+        return strcmp(v1.pathStr(), v2.pathStr()) == 0;
 
     case nNull:
         return true;
@@ -3136,7 +3131,7 @@ SourcePath EvalState::findFile(const LookupPath & lookupPath, const std::string_
     }
 
     if (hasPrefix(path, "nix/"))
-        return {corepkgsFS, CanonPath(path.substr(3))};
+        return rootPath(CanonPath(store->printStorePath(corepkgsPath)) / CanonPath(path.substr(3)));
 
     error<ThrownError>(
         settings.pureEval ? "cannot look up '<%s>' in pure evaluation mode (use '--impure' to override)"

src/libexpr/include/nix/expr/eval.hh

@@ -382,6 +382,13 @@ public:
      */
     RepairFlag repair;
 
+    /**
+     * The in-memory filesystem for <nix/...> paths.
+     */
+    const ref<MemorySourceAccessor> corepkgsFS;
+
+    const StorePath corepkgsPath;
+
     /**
      * The accessor corresponding to `store`.
      */
@@ -392,11 +399,6 @@ public:
      */
     const ref<SourceAccessor> rootFS;
 
-    /**
-     * The in-memory filesystem for <nix/...> paths.
-     */
-    const ref<MemorySourceAccessor> corepkgsFS;
-
     /**
      * In-memory filesystem for internal, non-user-callable Nix
      * expressions like `derivation.nix`.

src/libexpr/include/nix/expr/nixexpr.hh

@@ -211,21 +211,19 @@ struct ExprString : Expr
 
 struct ExprPath : Expr
 {
-    ref<SourceAccessor> accessor;
     Value v;
 
-    ExprPath(std::pmr::polymorphic_allocator<char> & alloc, ref<SourceAccessor> accessor, std::string_view sv)
-        : accessor(accessor)
+    ExprPath(std::pmr::polymorphic_allocator<char> & alloc, std::string_view sv)
     {
         auto len = sv.length();
         if (len == 0) {
-            v.mkPath(&*accessor, "");
+            v.mkPath("");
             return;
         }
         char * s = alloc.allocate(len + 1);
         sv.copy(s, len);
         s[len] = '\0';
-        v.mkPath(&*accessor, s);
+        v.mkPath(s);
     }
 
     Value * maybeThunk(EvalState & state, Env & env) override;

src/libexpr/include/nix/expr/value.hh

@@ -39,6 +39,7 @@ typedef enum {
     tExternal,
     tPrimOp,
     tAttrs,
+    tPath,
     /* layout: Pair of pointers payload */
     tListSmall,
     tPrimOpApp,
@@ -48,7 +49,6 @@ typedef enum {
     /* layout: Single untaggable field */
     tListN,
     tString,
-    tPath,
 } InternalType;
 
 /**
@@ -225,7 +225,6 @@ struct ValueBase
 
     struct Path
     {
-        SourceAccessor * accessor;
         const char * path;
     };
 
@@ -415,7 +414,7 @@ class alignas(16) ValueStorage<ptrSize, std::enable_if_t<detail::useBitPackedVal
      * That leaves the first 8 bytes free for storing the InternalType in the upper
      * bits.
      *
-     * PrimaryDiscriminator::pdListN - pdPath - Only has 3 available padding bits
+     * PrimaryDiscriminator::pdListN - pdString - Only has 3 available padding bits
      * because:
      * - tListN needs a size, whose lower bits we can't borrow.
      * - tString and tPath have C-string fields, which don't necessarily need to
@@ -436,7 +435,6 @@ class alignas(16) ValueStorage<ptrSize, std::enable_if_t<detail::useBitPackedVal
         /* The order of these enumations must be the same as in InternalType. */
         pdListN, //< layout: Single untaggable field.
         pdString,
-        pdPath,
         pdPairOfPointers, //< layout: Pair of pointers payload
     };
 
@@ -469,7 +467,7 @@ class alignas(16) ValueStorage<ptrSize, std::enable_if_t<detail::useBitPackedVal
     template<PrimaryDiscriminator discriminator, typename T, typename U>
     void setUntaggablePayload(T * firstPtrField, U untaggableField) noexcept
     {
-        static_assert(discriminator >= pdListN && discriminator <= pdPath);
+        static_assert(discriminator >= pdListN && discriminator <= pdString);
         auto firstFieldPayload = std::bit_cast<PackedPointer>(firstPtrField);
         assertAligned(firstFieldPayload);
         payload[0] = static_cast<int>(discriminator) | firstFieldPayload;
@@ -515,7 +513,6 @@ protected:
         /* The order must match that of the enumerations defined in InternalType. */
         case pdListN:
         case pdString:
-        case pdPath:
             return static_cast<InternalType>(tListN + (pd - pdListN));
         case pdPairOfPointers:
             return static_cast<InternalType>(tListSmall + (payload[1] & discriminatorMask));
@@ -578,6 +575,11 @@ protected:
         attrs = std::bit_cast<Bindings *>(payload[1]);
     }
 
+    void getStorage(Path & path) const noexcept
+    {
+        path.path = std::bit_cast<const char *>(payload[1]);
+    }
+
     void getStorage(List & list) const noexcept
     {
         list.elems = untagPointer<decltype(list.elems)>(payload[0]);
@@ -590,12 +592,6 @@ protected:
         string.c_str = std::bit_cast<const char *>(payload[1]);
     }
 
-    void getStorage(Path & path) const noexcept
-    {
-        path.accessor = untagPointer<decltype(path.accessor)>(payload[0]);
-        path.path = std::bit_cast<const char *>(payload[1]);
-    }
-
     void setStorage(NixInt integer) noexcept
     {
         setSingleDWordPayload<tInt>(integer.value);
@@ -631,6 +627,11 @@ protected:
         setSingleDWordPayload<tAttrs>(std::bit_cast<PackedPointer>(bindings));
     }
 
+    void setStorage(Path path) noexcept
+    {
+        setSingleDWordPayload<tPath>(std::bit_cast<PackedPointer>(path.path));
+    }
+
     void setStorage(List list) noexcept
     {
         setUntaggablePayload<pdListN>(list.elems, list.size);
@@ -640,11 +641,6 @@ protected:
     {
         setUntaggablePayload<pdString>(string.context, string.c_str);
     }
-
-    void setStorage(Path path) noexcept
-    {
-        setUntaggablePayload<pdPath>(path.accessor, path.path);
-    }
 };
 
 /**
@@ -1002,11 +998,11 @@ public:
 
     void mkStringMove(const char * s, const NixStringContext & context);
 
-    void mkPath(const SourcePath & path);
+    void mkPath(const CanonPath & path);
 
-    inline void mkPath(SourceAccessor * accessor, const char * path) noexcept
+    inline void mkPath(const char * path) noexcept
     {
-        setStorage(Path{.accessor = accessor, .path = path});
+        setStorage(Path{.path = path});
     }
 
     inline void mkNull() noexcept
@@ -1102,9 +1098,9 @@ public:
      */
     bool isTrivial() const;
 
-    SourcePath path() const
+    CanonPath path() const
     {
-        return SourcePath(ref(pathAccessor()->shared_from_this()), CanonPath(CanonPath::unchecked_t(), pathStr()));
+        return CanonPath(CanonPath::unchecked_t(), pathStr());
     }
 
     std::string_view string_view() const noexcept
@@ -1176,11 +1172,6 @@ public:
     {
         return getStorage<Path>().path;
     }
-
-    SourceAccessor * pathAccessor() const noexcept
-    {
-        return getStorage<Path>().accessor;
-    }
 };
 
 extern ExprBlackHole eBlackHole;