From ee6a76498846cb253e7b3567fe894a4847b90a50 Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Thu, 14 Mar 2024 14:11:36 +0100
Subject: [PATCH] Make path values lazy

This fixes the double copy problem and improves performance
for expressions that don't force the whole source to be added to the
store.

Rules for fast expressions:

- Use path literals where possible
   - import ./foo.nix
- Use + operator with slash in string
   - src = fetchTree foo + "/src";
- Use source filtering, lib.fileset

- AVOID toString
- If possible, AVOID interpolations ("${./.}")
- If possible, move slashes into the interpolation to add less to the store
   - "${./src}/foo" -> "${./src/foo}"

toString may be improved later as part of lazy-trees, so these
recommendations are a snapshot. Path values are quite nice though.
---
 src/libexpr/eval.cc                  | 13 +++++++++--
 src/libexpr/nixexpr.hh               |  5 ++++-
 src/libexpr/primops/fetchTree.cc     | 33 +++++++++++++++++++---------
 src/libutil/posix-source-accessor.cc |  5 +++++
 src/libutil/posix-source-accessor.hh |  2 ++
 src/libutil/source-accessor.cc       |  4 ++++
 src/libutil/source-accessor.hh       |  9 ++++++++
 7 files changed, 58 insertions(+), 13 deletions(-)

diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc
index 18abc92d7..8020b3b31 100644
--- a/src/libexpr/eval.cc
+++ b/src/libexpr/eval.cc
@@ -1978,6 +1978,7 @@ void EvalState::concatLists(Value & v, size_t nrLists, Value * const * lists, co
     v.mkList(list);
 }
 
+// FIXME limit recursion
 Value * resolveOutPath(EvalState & state, Value * v, const PosIdx pos)
 {
     state.forceValue(*v, pos);
@@ -2335,6 +2336,8 @@ BackedStringView EvalState::coerceToString(
               v.payload.path.path
             : copyToStore
             ? store->printStorePath(copyPathToStore(context, v.path()))
+            : v.path().accessor->toStringReturnsStorePath()
+            ? store->printStorePath(copyPathToStore(context, SourcePath(v.path().accessor, CanonPath::root))) + v.path().path.absOrEmpty()
             : std::string(v.path().path.abs());
     }
 
@@ -2447,10 +2450,14 @@ SourcePath EvalState::coerceToPath(const PosIdx pos, Value & v, NixStringContext
     if (v.type() == nPath)
         return v.path();
 
-    /* Similarly, handle __toString where the result may be a path
+    /* Similarly, handle outPath and __toString where the result may be a path
        value. */
     if (v.type() == nAttrs) {
-        auto i = v.attrs()->find(sToString);
+        auto i = v.attrs()->find(sOutPath);
+        if (i != v.attrs()->end()) {
+            return coerceToPath(pos, *i->value, context, errorCtx);
+        }
+        i = v.attrs()->find(sToString);
         if (i != v.attrs()->end()) {
             Value v1;
             callFunction(*i->value, v, v1, pos);
@@ -3115,6 +3122,8 @@ std::optional<SourcePath> EvalState::resolveLookupPathPath(const LookupPath::Pat
                 store,
                 fetchSettings,
                 EvalSettings::resolvePseudoUrl(value));
+            // Traditional search path lookups use the absolute path space for
+            // historical consistency.
             auto storePath = fetchToStore(*store, SourcePath(accessor), FetchMode::Copy);
             res.emplace(rootPath(CanonPath(store->toRealPath(storePath))));
         } catch (Error & e) {
diff --git a/src/libexpr/nixexpr.hh b/src/libexpr/nixexpr.hh
index 4bfed40db..dfe8df00a 100644
--- a/src/libexpr/nixexpr.hh
+++ b/src/libexpr/nixexpr.hh
@@ -136,7 +136,10 @@ struct ExprPath : Expr
     ExprPath(SourcePath && path)
         : path(path)
     {
-        v.mkPath(&*path.accessor, strdup(path.path.abs().c_str()));
+        v.mkPath(
+            &*path.accessor,
+            // TODO: GC_STRDUP
+            strdup(path.path.abs().c_str()));
     }
     Value * maybeThunk(EvalState & state, Env & env) override;
     COMMON_METHODS
diff --git a/src/libexpr/primops/fetchTree.cc b/src/libexpr/primops/fetchTree.cc
index 61bc3b985..62b612187 100644
--- a/src/libexpr/primops/fetchTree.cc
+++ b/src/libexpr/primops/fetchTree.cc
@@ -92,7 +92,7 @@ struct FetchTreeParams {
     bool emptyRevFallback = false;
     bool allowNameArgument = false;
     bool isFetchGit = false;
-    bool returnPath = true; // whether to return a SourcePath or a StorePath
+    bool returnPath = true; // whether to return a SourcePath instead of a StorePath
 };
 
 static void fetchTree(
@@ -212,18 +212,31 @@ static void fetchTree(
 
     state.checkURI(input.toURLString());
 
-    auto [storePath, input2] = input.fetchToStore(state.store);
+    if (params.returnPath) {
+        // Clang16+: change to `auto [accessor, input2] =`
+        auto pair = input.getAccessor(state.store);
+        auto & accessor = pair.first;
+        auto & input2 = pair.second;
 
-    state.allowPath(storePath);
+        emitTreeAttrs(state, input2, v,
+            [&](Value & vOutPath) {
+                state.registerAccessor(accessor);
+                vOutPath.mkPath(SourcePath { accessor, CanonPath::root });
+            },
+            params.emptyRevFallback, false);
+    } else {
+        auto pair = input.fetchToStore(state.store);
+        auto & storePath = pair.first;
+        auto & input2 = pair.second;
 
-    emitTreeAttrs(state, input2, v,
-        [&](Value & vOutPath) {
-            if (params.returnPath)
-                vOutPath.mkPath(state.rootPath(state.store->toRealPath(storePath)));
-            else
+        state.allowPath(storePath);
+
+        emitTreeAttrs(state, input2, v,
+            [&](Value & vOutPath) {
                 state.mkStorePathString(storePath, vOutPath);
-        },
-        params.emptyRevFallback, false);
+            },
+            params.emptyRevFallback, false);
+    }
 }
 
 static void prim_fetchTree(EvalState & state, const PosIdx pos, Value * * args, Value & v)
diff --git a/src/libutil/posix-source-accessor.cc b/src/libutil/posix-source-accessor.cc
index 2b1a485d5..3df84f635 100644
--- a/src/libutil/posix-source-accessor.cc
+++ b/src/libutil/posix-source-accessor.cc
@@ -195,4 +195,9 @@ ref<SourceAccessor> makeFSSourceAccessor(std::filesystem::path root)
 {
     return make_ref<PosixSourceAccessor>(std::move(root));
 }
+
+bool PosixSourceAccessor::toStringReturnsStorePath() const {
+    return false;
+}
+
 }
diff --git a/src/libutil/posix-source-accessor.hh b/src/libutil/posix-source-accessor.hh
index 40f60bb54..f80425276 100644
--- a/src/libutil/posix-source-accessor.hh
+++ b/src/libutil/posix-source-accessor.hh
@@ -57,6 +57,8 @@ struct PosixSourceAccessor : virtual SourceAccessor
      */
     static SourcePath createAtRoot(const std::filesystem::path & path);
 
+    virtual bool toStringReturnsStorePath() const override;
+
 private:
 
     /**
diff --git a/src/libutil/source-accessor.cc b/src/libutil/source-accessor.cc
index e797951c7..131e10e56 100644
--- a/src/libutil/source-accessor.cc
+++ b/src/libutil/source-accessor.cc
@@ -105,4 +105,8 @@ CanonPath SourceAccessor::resolveSymlinks(
     return res;
 }
 
+bool SourceAccessor::toStringReturnsStorePath() const {
+    return true;
+}
+
 }
diff --git a/src/libutil/source-accessor.hh b/src/libutil/source-accessor.hh
index b16960d4a..79cec5e99 100644
--- a/src/libutil/source-accessor.hh
+++ b/src/libutil/source-accessor.hh
@@ -161,6 +161,15 @@ struct SourceAccessor : std::enable_shared_from_this<SourceAccessor>
 
     virtual std::string showPath(const CanonPath & path);
 
+    /**
+     * System paths: `toString /foo/bar = "/foo/bar"`
+     * Virtual paths: fetched to the store
+     *
+     * In both cases, the returned string functionally identifies the path,
+     * and can still be read.
+     */
+    virtual bool toStringReturnsStorePath() const;
+
     /**
      * Resolve any symlinks in `path` according to the given
      * resolution mode.