From 6a3a87a714e1f3be1464f8fd4c82714b7d032879 Mon Sep 17 00:00:00 2001
From: John Ericson <John.Ericson@Obsidian.Systems>
Date: Tue, 9 May 2023 14:44:08 -0400
Subject: [PATCH] Improve error message for self reference with text hashing

The `ContentAddressWithReferences` method is made total, with error
handling now squarely the caller's job. This is better.
---
 src/libstore/build/local-derivation-goal.cc | 16 ++++++++---
 src/libstore/content-address.cc             | 32 ++++++++++++---------
 src/libstore/content-address.hh             |  7 +++--
 3 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/src/libstore/build/local-derivation-goal.cc b/src/libstore/build/local-derivation-goal.cc
index eb6c00e77..2e4020f33 100644
--- a/src/libstore/build/local-derivation-goal.cc
+++ b/src/libstore/build/local-derivation-goal.cc
@@ -2456,13 +2456,21 @@ SingleDrvOutputs LocalDerivationGoal::registerOutputs()
                 },
             }, outputHash.method.raw);
             auto got = caSink.finish().first;
+
+            auto optCA = ContentAddressWithReferences::fromPartsOpt(
+                outputHash.method,
+                std::move(got),
+                rewriteRefs());
+            if (!optCA) {
+                // TODO track distinct failure modes separately (at the time of
+                // writing there is just one but `nullopt` is unclear) so this
+                // message can't get out of sync.
+                throw BuildError("output path '%s' has illegal content address, probably a spurious self-reference with text hashing");
+            }
             ValidPathInfo newInfo0 {
                 worker.store,
                 outputPathName(drv->name, outputName),
-                ContentAddressWithReferences::fromParts(
-                    outputHash.method,
-                    std::move(got),
-                    rewriteRefs()),
+                *std::move(optCA),
                 Hash::dummy,
             };
             if (*scratchPath != newInfo0.path) {
diff --git a/src/libstore/content-address.cc b/src/libstore/content-address.cc
index b62818e11..04f7ac214 100644
--- a/src/libstore/content-address.cc
+++ b/src/libstore/content-address.cc
@@ -232,25 +232,29 @@ ContentAddressWithReferences ContentAddressWithReferences::withoutRefs(const Con
     }, ca.raw);
 }
 
-ContentAddressWithReferences ContentAddressWithReferences::fromParts(
-    ContentAddressMethod method, Hash hash, StoreReferences refs)
+std::optional<ContentAddressWithReferences> ContentAddressWithReferences::fromPartsOpt(
+    ContentAddressMethod method, Hash hash, StoreReferences refs) noexcept
 {
     return std::visit(overloaded {
-        [&](TextIngestionMethod _) -> ContentAddressWithReferences {
+        [&](TextIngestionMethod _) -> std::optional<ContentAddressWithReferences> {
             if (refs.self)
-                throw UsageError("Cannot have a self reference with text hashing scheme");
-            return TextInfo {
-                .hash = { .hash = std::move(hash) },
-                .references = std::move(refs.others),
+                return std::nullopt;
+            return ContentAddressWithReferences {
+                TextInfo {
+                    .hash = { .hash = std::move(hash) },
+                    .references = std::move(refs.others),
+                }
             };
         },
-        [&](FileIngestionMethod m2) -> ContentAddressWithReferences {
-            return FixedOutputInfo {
-                .hash = {
-                    .method = m2,
-                    .hash = std::move(hash),
-                },
-                .references = std::move(refs),
+        [&](FileIngestionMethod m2) -> std::optional<ContentAddressWithReferences> {
+            return ContentAddressWithReferences {
+                FixedOutputInfo {
+                    .hash = {
+                        .method = m2,
+                        .hash = std::move(hash),
+                    },
+                    .references = std::move(refs),
+                }
             };
         },
     }, method.raw);
diff --git a/src/libstore/content-address.hh b/src/libstore/content-address.hh
index 03a2e2a23..e1e80448b 100644
--- a/src/libstore/content-address.hh
+++ b/src/libstore/content-address.hh
@@ -303,10 +303,11 @@ struct ContentAddressWithReferences
      *
      * @param refs References to other store objects or oneself.
      *
-     * Do note that not all combinations are supported.
+     * Do note that not all combinations are supported; `nullopt` is
+     * returns for invalid combinations.
      */
-    static ContentAddressWithReferences fromParts(
-        ContentAddressMethod method, Hash hash, StoreReferences refs);
+    static std::optional<ContentAddressWithReferences> fromPartsOpt(
+        ContentAddressMethod method, Hash hash, StoreReferences refs) noexcept;
 
     ContentAddressMethod getMethod() const;