Revert "Merge pull request #13938 from NixOS/import-thunk"

This has multiple dangling pointer issues that lead to segfaults in e.g.:

nix eval --expr '(builtins.getFlake "github:nixos/nixpkgs/25.05")' --impure

This reverts commit ad17572, reversing
changes made to d314750.

Author: xokdvium

src/libexpr/eval.cc

@@ -38,7 +38,6 @@
 
 #include <nlohmann/json.hpp>
 #include <boost/container/small_vector.hpp>
-#include <boost/unordered/concurrent_flat_map.hpp>
 
 #include "nix/util/strings-inline.hh"
 
@@ -265,9 +264,6 @@ EvalState::EvalState(
     , debugRepl(nullptr)
     , debugStop(false)
     , trylevel(0)
-    , srcToStore(make_ref<decltype(srcToStore)::element_type>())
-    , importResolutionCache(make_ref<decltype(importResolutionCache)::element_type>())
-    , fileEvalCache(make_ref<decltype(fileEvalCache)::element_type>())
     , regexCache(makeRegexCache())
 #if NIX_USE_BOEHMGC
     , valueAllocCache(std::allocate_shared<void *>(traceable_allocator<void *>(), nullptr))
@@ -1030,85 +1026,63 @@ Value * ExprPath::maybeThunk(EvalState & state, Env & env)
     return &v;
 }
 
-/**
- * A helper `Expr` class to lets us parse and evaluate Nix expressions
- * from a thunk, ensuring that every file is parsed/evaluated only
- * once (via the thunk stored in `EvalState::fileEvalCache`).
- */
-struct ExprParseFile : Expr
-{
-    SourcePath & path;
-    bool mustBeTrivial;
-
-    ExprParseFile(SourcePath & path, bool mustBeTrivial)
-        : path(path)
-        , mustBeTrivial(mustBeTrivial)
-    {
-    }
-
-    void eval(EvalState & state, Env & env, Value & v) override
-    {
-        printTalkative("evaluating file '%s'", path);
-
-        auto e = state.parseExprFromFile(path);
-
-        try {
-            auto dts =
-                state.debugRepl
-                    ? makeDebugTraceStacker(
-                          state, *e, state.baseEnv, e->getPos(), "while evaluating the file '%s':", path.to_string())
-                    : nullptr;
-
-            // Enforce that 'flake.nix' is a direct attrset, not a
-            // computation.
-            if (mustBeTrivial && !(dynamic_cast<ExprAttrs *>(e)))
-                state.error<EvalError>("file '%s' must be an attribute set", path).debugThrow();
-
-            state.eval(e, v);
-        } catch (Error & e) {
-            state.addErrorTrace(e, "while evaluating the file '%s':", path.to_string());
-            throw;
-        }
-    }
-};
-
 void EvalState::evalFile(const SourcePath & path, Value & v, bool mustBeTrivial)
 {
-    auto resolvedPath = getConcurrent(*importResolutionCache, path);
-
-    if (!resolvedPath) {
-        resolvedPath = resolveExprPath(path);
-        importResolutionCache->emplace(path, *resolvedPath);
+    FileEvalCache::iterator i;
+    if ((i = fileEvalCache.find(path)) != fileEvalCache.end()) {
+        v = i->second;
+        return;
     }
 
-    if (auto v2 = getConcurrent(*fileEvalCache, *resolvedPath)) {
-        forceValue(**v2, noPos);
-        v = **v2;
+    auto resolvedPath = resolveExprPath(path);
+    if ((i = fileEvalCache.find(resolvedPath)) != fileEvalCache.end()) {
+        v = i->second;
         return;
     }
 
-    Value * vExpr;
-    ExprParseFile expr{*resolvedPath, mustBeTrivial};
+    printTalkative("evaluating file '%1%'", resolvedPath);
+    Expr * e = nullptr;
 
-    fileEvalCache->try_emplace_and_cvisit(
-        *resolvedPath,
-        nullptr,
-        [&](auto & i) {
-            vExpr = allocValue();
-            vExpr->mkThunk(&baseEnv, &expr);
-            i.second = vExpr;
-        },
-        [&](auto & i) { vExpr = i.second; });
+    auto j = fileParseCache.find(resolvedPath);
+    if (j != fileParseCache.end())
+        e = j->second;
+
+    if (!e)
+        e = parseExprFromFile(resolvedPath);
+
+    fileParseCache.emplace(resolvedPath, e);
 
-    forceValue(*vExpr, noPos);
+    try {
+        auto dts = debugRepl ? makeDebugTraceStacker(
+                                   *this,
+                                   *e,
+                                   this->baseEnv,
+                                   e->getPos(),
+                                   "while evaluating the file '%1%':",
+                                   resolvedPath.to_string())
+                             : nullptr;
+
+        // Enforce that 'flake.nix' is a direct attrset, not a
+        // computation.
+        if (mustBeTrivial && !(dynamic_cast<ExprAttrs *>(e)))
+            error<EvalError>("file '%s' must be an attribute set", path).debugThrow();
+        eval(e, v);
+    } catch (Error & e) {
+        addErrorTrace(e, "while evaluating the file '%1%':", resolvedPath.to_string());
+        throw;
+    }
 
-    v = *vExpr;
+    fileEvalCache.emplace(resolvedPath, v);
+    if (path != resolvedPath)
+        fileEvalCache.emplace(path, v);
 }
 
 void EvalState::resetFileCache()
 {
-    importResolutionCache->clear();
-    fileEvalCache->clear();
+    fileEvalCache.clear();
+    fileEvalCache.rehash(0);
+    fileParseCache.clear();
+    fileParseCache.rehash(0);
     inputCache->clear();
 }
 
@@ -2427,26 +2401,24 @@ StorePath EvalState::copyPathToStore(NixStringContext & context, const SourcePat
     if (nix::isDerivation(path.path.abs()))
         error<EvalError>("file names are not allowed to end in '%1%'", drvExtension).debugThrow();
 
-    auto dstPathCached = getConcurrent(*srcToStore, path);
-
-    auto dstPath = dstPathCached ? *dstPathCached : [&]() {
-        auto dstPath = fetchToStore(
+    std::optional<StorePath> dstPath;
+    if (!srcToStore.cvisit(path, [&dstPath](const auto & kv) { dstPath.emplace(kv.second); })) {
+        dstPath.emplace(fetchToStore(
             fetchSettings,
             *store,
             path.resolveSymlinks(SymlinkResolution::Ancestors),
             settings.readOnlyMode ? FetchMode::DryRun : FetchMode::Copy,
             path.baseName(),
             ContentAddressMethod::Raw::NixArchive,
             nullptr,
-            repair);
-        allowPath(dstPath);
-        srcToStore->try_emplace(path, dstPath);
-        printMsg(lvlChatty, "copied source '%1%' -> '%2%'", path, store->printStorePath(dstPath));
-        return dstPath;
-    }();
+            repair));
+        allowPath(*dstPath);
+        srcToStore.try_emplace(path, *dstPath);
+        printMsg(lvlChatty, "copied source '%1%' -> '%2%'", path, store->printStorePath(*dstPath));
+    }
 
-    context.insert(NixStringContextElem::Opaque{.path = dstPath});
-    return dstPath;
+    context.insert(NixStringContextElem::Opaque{.path = *dstPath});
+    return *dstPath;
 }
 
 SourcePath EvalState::coerceToPath(const PosIdx pos, Value & v, NixStringContext & context, std::string_view errorCtx)

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

@@ -20,9 +20,8 @@
 // For `NIX_USE_BOEHMGC`, and if that's set, `GC_THREADS`
 #include "nix/expr/config.hh"
 
+#include <boost/unordered/concurrent_flat_map.hpp>
 #include <boost/unordered/unordered_flat_map.hpp>
-#include <boost/unordered/concurrent_flat_map_fwd.hpp>
-
 #include <map>
 #include <optional>
 #include <functional>
@@ -404,30 +403,37 @@ private:
 
     /* Cache for calls to addToStore(); maps source paths to the store
        paths. */
-    ref<boost::concurrent_flat_map<SourcePath, StorePath>> srcToStore;
+    boost::concurrent_flat_map<SourcePath, StorePath, std::hash<SourcePath>> srcToStore;
 
     /**
-     * A cache that maps paths to "resolved" paths for importing Nix
-     * expressions, i.e. `/foo` to `/foo/default.nix`.
+     * A cache from path names to parse trees.
      */
-    ref<boost::concurrent_flat_map<SourcePath, SourcePath>> importResolutionCache;
+    typedef boost::unordered_flat_map<
+        SourcePath,
+        Expr *,
+        std::hash<SourcePath>,
+        std::equal_to<SourcePath>,
+        traceable_allocator<std::pair<const SourcePath, Expr *>>>
+        FileParseCache;
+    FileParseCache fileParseCache;
 
     /**
-     * A cache from resolved paths to values.
+     * A cache from path names to values.
      */
-    ref<boost::concurrent_flat_map<
+    typedef boost::unordered_flat_map<
         SourcePath,
-        Value *,
+        Value,
         std::hash<SourcePath>,
         std::equal_to<SourcePath>,
-        traceable_allocator<std::pair<const SourcePath, Value *>>>>
-        fileEvalCache;
+        traceable_allocator<std::pair<const SourcePath, Value>>>
+        FileEvalCache;
+    FileEvalCache fileEvalCache;
 
     /**
      * Associate source positions of certain AST nodes with their preceding doc comment, if they have one.
      * Grouped by file.
      */
-    boost::unordered_flat_map<SourcePath, DocCommentMap> positionToDocComment;
+    boost::unordered_flat_map<SourcePath, DocCommentMap, std::hash<SourcePath>> positionToDocComment;
 
     LookupPath lookupPath;
 

src/libfetchers/filtering-source-accessor.cc

@@ -59,12 +59,12 @@ void FilteringSourceAccessor::checkAccess(const CanonPath & path)
 struct AllowListSourceAccessorImpl : AllowListSourceAccessor
 {
     std::set<CanonPath> allowedPrefixes;
-    boost::unordered_flat_set<CanonPath> allowedPaths;
+    boost::unordered_flat_set<CanonPath, std::hash<CanonPath>> allowedPaths;
 
     AllowListSourceAccessorImpl(
         ref<SourceAccessor> next,
         std::set<CanonPath> && allowedPrefixes,
-        boost::unordered_flat_set<CanonPath> && allowedPaths,
+        boost::unordered_flat_set<CanonPath, std::hash<CanonPath>> && allowedPaths,
         MakeNotAllowedError && makeNotAllowedError)
         : AllowListSourceAccessor(SourcePath(next), std::move(makeNotAllowedError))
         , allowedPrefixes(std::move(allowedPrefixes))
@@ -86,7 +86,7 @@ struct AllowListSourceAccessorImpl : AllowListSourceAccessor
 ref<AllowListSourceAccessor> AllowListSourceAccessor::create(
     ref<SourceAccessor> next,
     std::set<CanonPath> && allowedPrefixes,
-    boost::unordered_flat_set<CanonPath> && allowedPaths,
+    boost::unordered_flat_set<CanonPath, std::hash<CanonPath>> && allowedPaths,
     MakeNotAllowedError && makeNotAllowedError)
 {
     return make_ref<AllowListSourceAccessorImpl>(

src/libfetchers/git-utils.cc

@@ -817,7 +817,7 @@ struct GitSourceAccessor : SourceAccessor
         return toHash(*git_tree_entry_id(entry));
     }
 
-    boost::unordered_flat_map<CanonPath, TreeEntry> lookupCache;
+    boost::unordered_flat_map<CanonPath, TreeEntry, std::hash<CanonPath>> lookupCache;
 
     /* Recursively look up 'path' relative to the root. */
     git_tree_entry * lookup(State & state, const CanonPath & path)
@@ -1254,7 +1254,7 @@ GitRepoImpl::getAccessor(const WorkdirInfo & wd, bool exportIgnore, MakeNotAllow
                                            makeFSSourceAccessor(path),
                                            std::set<CanonPath>{wd.files},
                                            // Always allow access to the root, but not its children.
-                                           boost::unordered_flat_set<CanonPath>{CanonPath::root},
+                                           boost::unordered_flat_set<CanonPath, std::hash<CanonPath>>{CanonPath::root},
                                            std::move(makeNotAllowedError))
                                            .cast<SourceAccessor>();
     if (exportIgnore)

src/libfetchers/include/nix/fetchers/filtering-source-accessor.hh

@@ -72,7 +72,7 @@ struct AllowListSourceAccessor : public FilteringSourceAccessor
     static ref<AllowListSourceAccessor> create(
         ref<SourceAccessor> next,
         std::set<CanonPath> && allowedPrefixes,
-        boost::unordered_flat_set<CanonPath> && allowedPaths,
+        boost::unordered_flat_set<CanonPath, std::hash<CanonPath>> && allowedPaths,
         MakeNotAllowedError && makeNotAllowedError);
 
     using FilteringSourceAccessor::FilteringSourceAccessor;

src/libutil/include/nix/util/canon-path.hh

@@ -8,8 +8,6 @@
 #include <set>
 #include <vector>
 
-#include <boost/container_hash/hash.hpp>
-
 namespace nix {
 
 /**
@@ -260,26 +258,20 @@ public:
      */
     std::string makeRelative(const CanonPath & path) const;
 
-    friend std::size_t hash_value(const CanonPath &);
+    friend struct std::hash<CanonPath>;
 };
 
 std::ostream & operator<<(std::ostream & stream, const CanonPath & path);
 
-inline std::size_t hash_value(const CanonPath & path)
-{
-    boost::hash<std::string_view> hasher;
-    return hasher(path.path);
-}
-
 } // namespace nix
 
 template<>
 struct std::hash<nix::CanonPath>
 {
     using is_avalanching = std::true_type;
 
-    std::size_t operator()(const nix::CanonPath & path) const noexcept
+    std::size_t operator()(const nix::CanonPath & s) const noexcept
     {
-        return nix::hash_value(path);
+        return std::hash<std::string>{}(s.path);
     }
 };

src/libutil/include/nix/util/source-path.hh

@@ -119,23 +119,15 @@ struct SourcePath
 
 std::ostream & operator<<(std::ostream & str, const SourcePath & path);
 
-inline std::size_t hash_value(const SourcePath & path)
-{
-    std::size_t hash = 0;
-    boost::hash_combine(hash, path.accessor->number);
-    boost::hash_combine(hash, path.path);
-    return hash;
-}
-
 } // namespace nix
 
 template<>
 struct std::hash<nix::SourcePath>
 {
-    using is_avalanching = std::true_type;
-
     std::size_t operator()(const nix::SourcePath & s) const noexcept
     {
-        return nix::hash_value(s);
+        std::size_t hash = 0;
+        hash_combine(hash, s.accessor->number, s.path);
+        return hash;
     }
 };

src/libutil/include/nix/util/util.hh

@@ -220,17 +220,6 @@ typename T::mapped_type * get(T & map, const K & key)
 template<class T, typename K>
 typename T::mapped_type * get(T && map, const K & key) = delete;
 
-/**
- * Look up a value in a `boost::concurrent_flat_map`.
- */
-template<class T>
-std::optional<typename T::mapped_type> getConcurrent(const T & map, const typename T::key_type & key)
-{
-    std::optional<typename T::mapped_type> res;
-    map.cvisit(key, [&](auto & x) { res = x.second; });
-    return res;
-}
-
 /**
  * Get a value for the specified key from an associate container, or a default value if the key isn't present.
  */

src/libutil/posix-source-accessor.cc

@@ -95,7 +95,9 @@ std::optional<struct stat> PosixSourceAccessor::cachedLstat(const CanonPath & pa
     // former is not hashable on libc++.
     Path absPath = makeAbsPath(path).string();
 
-    if (auto res = getConcurrent(cache, absPath))
+    std::optional<Cache::mapped_type> res;
+    cache.cvisit(absPath, [&](auto & x) { res.emplace(x.second); });
+    if (res)
         return *res;
 
     auto st = nix::maybeLstat(absPath.c_str());