Merge changes I07d2da41,I864d7340,I86612c64 into main

* changes:
  Change error messages about 'invalid paths' to 'path does not exist'.
  Add a clearer error message for InvalidPathError during evaluation
  Harmonise the Store::queryPathInfoUncached interface

9 files changed, 55 insertions, 25 deletions

diff --git a/src/libexpr/eval-error.hh b/src/libexpr/eval-error.hh
index 4f0e0d24c..19540d612 100644
--- a/src/libexpr/eval-error.hh
+++ b/src/libexpr/eval-error.hh
@@ -47,12 +47,16 @@ MakeError(MissingArgumentError, EvalError);
 MakeError(RestrictedPathError, Error);
 MakeError(InfiniteRecursionError, EvalError);
 
+/**
+ * Represents an exception due to an invalid path; that is, it does not exist.
+ * It corresponds to `!Store::validPath()`.
+ */
 struct InvalidPathError : public EvalError
 {
 public:
     Path path;
     InvalidPathError(EvalState & state, const Path & path)
-        : EvalError(state, "path '%s' is not valid", path)
+        : EvalError(state, "path '%s' did not exist in the store during evaluation", path)
     {
     }
 };
diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc
index 3cc2659fb..dba56c011 100644
--- a/src/libexpr/primops.cc
+++ b/src/libexpr/primops.cc
@@ -383,7 +383,8 @@ void prim_exec(EvalState & state, const PosIdx pos, Value * * args, Value & v)
     try {
         auto _ = state.realiseContext(context); // FIXME: Handle CA derivations
     } catch (InvalidPathError & e) {
-        state.error<EvalError>("cannot execute '%1%', since path '%2%' is not valid", program, e.path).atPos(pos).debugThrow();
+        e.addTrace(state.positions[pos], "while realising the context for builtins.exec");
+        throw;
     }
 
     auto output = runProgram(program, true, commandArgs);
diff --git a/src/libstore/binary-cache-store.cc b/src/libstore/binary-cache-store.cc
index ea643fd31..7df55a32d 100644
--- a/src/libstore/binary-cache-store.cc
+++ b/src/libstore/binary-cache-store.cc
@@ -170,7 +170,7 @@ ref<const ValidPathInfo> BinaryCacheStore::addToStoreCommon(
             if (ref != info.path)
                 queryPathInfo(ref);
         } catch (InvalidPath &) {
-            throw Error("cannot add '%s' to the binary cache because the reference '%s' is not valid",
+            throw Error("cannot add '%s' to the binary cache because the reference '%s' does not exist",
                 printStorePath(info.path), printStorePath(ref));
         }
 
@@ -361,7 +361,7 @@ std::shared_ptr<const ValidPathInfo> BinaryCacheStore::queryPathInfoUncached(con
 
     auto data = getFile(narInfoFile);
 
-    if (!data) return {};
+    if (!data) return nullptr;
 
     stats.narInfoRead++;
 
diff --git a/src/libstore/local-fs-store.cc b/src/libstore/local-fs-store.cc
index b224fc3e9..56f13920c 100644
--- a/src/libstore/local-fs-store.cc
+++ b/src/libstore/local-fs-store.cc
@@ -23,7 +23,7 @@ struct LocalStoreAccessor : public FSAccessor
     {
         auto storePath = store->toStorePath(path).first;
         if (requireValidPath && !store->isValidPath(storePath))
-            throw InvalidPath("path '%1%' is not a valid store path", store->printStorePath(storePath));
+            throw InvalidPath("path '%1%' does not exist in the store", store->printStorePath(storePath));
         return store->getRealStoreDir() + std::string(path, store->storeDir.size());
     }
 
@@ -81,7 +81,7 @@ ref<FSAccessor> LocalFSStore::getFSAccessor()
 void LocalFSStore::narFromPath(const StorePath & path, Sink & sink)
 {
     if (!isValidPath(path))
-        throw Error("path '%s' is not valid", printStorePath(path));
+        throw Error("path '%s' does not exist in store", printStorePath(path));
     dumpPath(getRealStoreDir() + std::string(printStorePath(path), storeDir.size()), sink);
 }
 
diff --git a/src/libstore/local-store.cc b/src/libstore/local-store.cc
index f5eaa9f5f..5bdf0362d 100644
--- a/src/libstore/local-store.cc
+++ b/src/libstore/local-store.cc
@@ -899,7 +899,7 @@ std::shared_ptr<const ValidPathInfo> LocalStore::queryPathInfoInternal(State & s
     auto useQueryPathInfo(state.stmts->QueryPathInfo.use()(printStorePath(path)));
 
     if (!useQueryPathInfo.next())
-        return std::shared_ptr<ValidPathInfo>();
+        return nullptr;
 
     auto id = useQueryPathInfo.getInt(0);
 
@@ -907,7 +907,7 @@ std::shared_ptr<const ValidPathInfo> LocalStore::queryPathInfoInternal(State & s
     try {
         narHash = Hash::parseAnyPrefixed(useQueryPathInfo.getStr(1));
     } catch (BadHash & e) {
-        throw Error("invalid-path entry for '%s': %s", printStorePath(path), e.what());
+        throw BadStorePath("bad hash in store path '%s': %s", printStorePath(path), e.what());
     }
 
     auto info = std::make_shared<ValidPathInfo>(path, narHash);
@@ -957,8 +957,8 @@ void LocalStore::updatePathInfo(State & state, const ValidPathInfo & info)
 uint64_t LocalStore::queryValidPathId(State & state, const StorePath & path)
 {
     auto use(state.stmts->QueryPathInfo.use()(printStorePath(path)));
-    if (!use.next())
-        throw InvalidPath("path '%s' is not valid", printStorePath(path));
+    if (!use.next()) // TODO: I guess if SQLITE got corrupted..?
+        throw InvalidPath("path '%s' does not exist", printStorePath(path));
     return use.getInt(0);
 }
 
diff --git a/src/libstore/remote-fs-accessor.cc b/src/libstore/remote-fs-accessor.cc
index c8e20b3b5..8da8b9774 100644
--- a/src/libstore/remote-fs-accessor.cc
+++ b/src/libstore/remote-fs-accessor.cc
@@ -55,7 +55,7 @@ std::pair<ref<FSAccessor>, Path> RemoteFSAccessor::fetch(const Path & path_, boo
     auto [storePath, restPath] = store->toStorePath(path);
 
     if (requireValidPath && !store->isValidPath(storePath))
-        throw InvalidPath("path '%1%' is not a valid store path", store->printStorePath(storePath));
+        throw InvalidPath("path '%1%' does not exist in remote store", store->printStorePath(storePath));
 
     auto i = nars.find(std::string(storePath.hashPart()));
     if (i != nars.end()) return {i->second, restPath};
diff --git a/src/libstore/remote-store.cc b/src/libstore/remote-store.cc
index 93b1afabd..966dd3fe0 100644
--- a/src/libstore/remote-store.cc
+++ b/src/libstore/remote-store.cc
@@ -309,14 +309,14 @@ std::shared_ptr<const ValidPathInfo> RemoteStore::queryPathInfoUncached(const St
     try {
         conn.processStderr();
     } catch (Error & e) {
-        // Ugly backwards compatibility hack.
+        // Ugly backwards compatibility hack. TODO(fj#325): remove.
         if (e.msg().find("is not valid") != std::string::npos)
-            throw InvalidPath(std::move(e.info()));
+            return nullptr;
         throw;
     }
     if (GET_PROTOCOL_MINOR(conn->daemonVersion) >= 17) {
         bool valid; conn->from >> valid;
-        if (!valid) throw InvalidPath("path '%s' is not valid", printStorePath(path));
+        if (!valid) return nullptr;
     }
     return std::make_shared<ValidPathInfo>(
         StorePath{path},
diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc
index 952958d51..e0e842060 100644
--- a/src/libstore/store-api.cc
+++ b/src/libstore/store-api.cc
@@ -666,11 +666,19 @@ bool Store::isValidPathUncached(const StorePath & path)
 }
 
 
-static bool goodStorePath(const StorePath & expected, const StorePath & actual)
+static void ensureGoodStorePath(Store * store, const StorePath & expected, const StorePath & actual)
 {
-    return
-        expected.hashPart() == actual.hashPart()
-        && (expected.name() == Store::MissingName || expected.name() == actual.name());
+    if (expected.hashPart() != actual.hashPart()) {
+        throw Error(
+            "the queried store path hash '%s' did not match expected '%s' while querying the store path '%s'",
+            expected.hashPart(), actual.hashPart(), store->printStorePath(expected)
+        );
+    } else if (expected.name() != Store::MissingName && expected.name() != actual.name()) {
+        throw Error(
+            "the queried store path name '%s' did not match expected '%s' while querying the store path '%s'",
+            expected.name(), actual.name(), store->printStorePath(expected)
+        );
+    }
 }
 
 
@@ -683,7 +691,7 @@ ref<const ValidPathInfo> Store::queryPathInfo(const StorePath & storePath)
         if (res && res->isKnownNow()) {
             stats.narInfoReadAverted++;
             if (!res->didExist())
-                throw InvalidPath("path '%s' is not valid", printStorePath(storePath));
+                throw InvalidPath("path '%s' does not exist in the store", printStorePath(storePath));
             return ref<const ValidPathInfo>(res->value);
         }
     }
@@ -696,27 +704,31 @@ ref<const ValidPathInfo> Store::queryPathInfo(const StorePath & storePath)
                 auto state_(state.lock());
                 state_->pathInfoCache.upsert(std::string(storePath.to_string()),
                     res.first == NarInfoDiskCache::oInvalid ? PathInfoCacheValue{} : PathInfoCacheValue{ .value = res.second });
-                if (res.first == NarInfoDiskCache::oInvalid ||
-                    !goodStorePath(storePath, res.second->path))
-                    throw InvalidPath("path '%s' is not valid", printStorePath(storePath));
+                if (res.first == NarInfoDiskCache::oInvalid)
+                    throw InvalidPath("path '%s' does not exist in the store", printStorePath(storePath));
             }
             return ref<const ValidPathInfo>(res.second);
         }
     }
 
     auto info = queryPathInfoUncached(storePath);
+    if (info) {
+        // first, before we cache anything, check that the store gave us valid data.
+        ensureGoodStorePath(this, storePath, info->path);
+    }
 
-    if (diskCache)
+    if (diskCache) {
         diskCache->upsertNarInfo(getUri(), hashPart, info);
+    }
 
     {
         auto state_(state.lock());
         state_->pathInfoCache.upsert(std::string(storePath.to_string()), PathInfoCacheValue { .value = info });
     }
 
-    if (!info || !goodStorePath(storePath, info->path)) {
+    if (!info) {
         stats.narInfoMissing++;
-        throw InvalidPath("path '%s' is not valid", printStorePath(storePath));
+        throw InvalidPath("path '%s' does not exist in the store", printStorePath(storePath));
     }
 
     return ref<const ValidPathInfo>(info);
diff --git a/src/libstore/store-api.hh b/src/libstore/store-api.hh
index 397ebe759..186437f43 100644
--- a/src/libstore/store-api.hh
+++ b/src/libstore/store-api.hh
@@ -62,10 +62,18 @@ MakeError(SubstError, Error);
  * denotes a permanent build failure
  */
 MakeError(BuildError, Error);
+/**
+ * denotes that a path in the store did not exist (but it could, had it
+ * been put there, i.e. it is still legal).
+ */
 MakeError(InvalidPath, Error);
 MakeError(Unsupported, Error);
 MakeError(SubstituteGone, Error);
 MakeError(SubstituterDisabled, Error);
+/**
+ * denotes that a path could not possibly be a store path.
+ * e.g. outside of the nix store, illegal characters in the name, etc.
+*/
 MakeError(BadStorePath, Error);
 
 MakeError(InvalidStoreURI, Error);
@@ -328,6 +336,7 @@ public:
 
     /**
      * Check whether a path is valid.
+     * A path is valid when it exists in the store *now*.
      */
     bool isValidPath(const StorePath & path);
 
@@ -399,6 +408,10 @@ public:
 
 protected:
 
+    /**
+     * Queries the path info without caching.
+     * Note to implementors: should return `nullptr` when the path is not found.
+     */
     virtual std::shared_ptr<const ValidPathInfo> queryPathInfoUncached(const StorePath & path) = 0;
     virtual std::shared_ptr<const Realisation> queryRealisationUncached(const DrvOutput &) = 0;