refactor(libutil/topo-sort): return variant instead of throwing

The variant has on the left-hand side the topologically sorted vector
and the right-hand side is a pair showing the path and its parent that
represent a cycle in the graph making the sort impossible.

This change prepares for enhanced cycle error messages that can provide
more context about the cycle. The variant approach allows callers to
handle cycles more flexibly, enabling better error reporting that shows
the full cycle path and which files are involved.

Adapted from Lix commit f7871fcb5.

Change-Id: I70a987f470437df8beb3b1cc203ff88701d0aa1b
Co-Authored-By: Maximilian Bosch <maximilian@mbosch.me>

src/libstore/local-store.cc

@@ -989,19 +989,22 @@ void LocalStore::registerValidPaths(const ValidPathInfos & infos)
            error if a cycle is detected and roll back the
            transaction.  Cycles can only occur when a derivation
            has multiple outputs. */
-        topoSort(
+        auto topoSortResult = topoSort(paths, {[&](const StorePath & path) {
-            paths,
-            {[&](const StorePath & path) {
                                            auto i = infos.find(path);
                                            return i == infos.end() ? StorePathSet() : i->second.references;
-            }},
+                                       }});
-            {[&](const StorePath & path, const StorePath & parent) {
+
-                return BuildError(
+        std::visit(
+            overloaded{
+                [&](Cycle<StorePath> & cycle) {
+                    throw BuildError(
                         BuildResult::Failure::OutputRejected,
                         "cycle detected in the references of '%s' from '%s'",
-                    printStorePath(path),
+                        printStorePath(cycle.path),
-                    printStorePath(parent));
+                        printStorePath(cycle.parent));
-            }});
+                },
+                [](auto &) { /* Success, continue */ }},
+            topoSortResult);
 
         txn.commit();
     });

src/libstore/misc.cc

@@ -311,22 +311,25 @@ MissingPaths Store::queryMissing(const std::vector<DerivedPath> & targets)
 
 StorePaths Store::topoSortPaths(const StorePathSet & paths)
 {
-    return topoSort(
+    auto result = topoSort(paths, {[&](const StorePath & path) {
-        paths,
-        {[&](const StorePath & path) {
                                try {
                                    return queryPathInfo(path)->references;
                                } catch (InvalidPath &) {
                                    return StorePathSet();
                                }
-        }},
+                           }});
-        {[&](const StorePath & path, const StorePath & parent) {
+
-            return BuildError(
+    return std::visit(
+        overloaded{
+            [&](Cycle<StorePath> & cycle) -> StorePaths {
+                throw BuildError(
                     BuildResult::Failure::OutputRejected,
                     "cycle detected in the references of '%s' from '%s'",
-                printStorePath(path),
+                    printStorePath(cycle.path),
-                printStorePath(parent));
+                    printStorePath(cycle.parent));
-        }});
+            },
+            [](auto & sorted) { return sorted; }},
+        result);
 }
 
 std::map<DrvOutput, StorePath>

src/libstore/unix/build/derivation-builder.cc

@@ -1473,9 +1473,7 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
         outputStats.insert_or_assign(outputName, std::move(st));
     }
 
-    auto sortedOutputNames = topoSort(
+    auto topoSortResult = topoSort(outputsToSort, {[&](const std::string & name) {
-        outputsToSort,
-        {[&](const std::string & name) {
                                        auto orifu = get(outputReferencesIfUnregistered, name);
                                        if (!orifu)
                                            throw BuildError(
@@ -1500,16 +1498,21 @@ SingleDrvOutputs DerivationBuilderImpl::registerOutputs()
                                                },
                                            },
                                            *orifu);
-        }},
+                                   }});
-        {[&](const std::string & path, const std::string & parent) {
+
+    auto sortedOutputNames = std::visit(
+        overloaded{
+            [&](Cycle<std::string> & cycle) -> std::vector<std::string> {
                 // TODO with more -vvvv also show the temporary paths for manual inspection.
-            return BuildError(
+                throw BuildError(
                     BuildResult::Failure::OutputRejected,
                     "cycle detected in build of '%s' in the references of output '%s' from output '%s'",
                     store.printStorePath(drvPath),
-                path,
+                    cycle.path,
-                parent);
+                    cycle.parent);
-        }});
+            },
+            [](auto & sorted) { return sorted; }},
+        topoSortResult);
 
     std::reverse(sortedOutputNames.begin(), sortedOutputNames.end());
 

src/libutil/include/nix/util/topo-sort.hh

@@ -2,39 +2,61 @@
 ///@file
 
 #include "nix/util/error.hh"
+#include <variant>
 
 namespace nix {
 
+template<typename T>
+struct Cycle
+{
+    T path;
+    T parent;
+};
+
+template<typename T>
+using TopoSortResult = std::variant<std::vector<T>, Cycle<T>>;
+
 template<typename T, typename Compare>
-std::vector<T> topoSort(
+TopoSortResult<T> topoSort(std::set<T, Compare> items, std::function<std::set<T, Compare>(const T &)> getChildren)
-    std::set<T, Compare> items,
-    std::function<std::set<T, Compare>(const T &)> getChildren,
-    std::function<Error(const T &, const T &)> makeCycleError)
 {
     std::vector<T> sorted;
     decltype(items) visited, parents;
 
-    auto dfsVisit = [&](this auto & dfsVisit, const T & path, const T * parent) {
+    std::function<std::optional<Cycle<T>>(const T & path, const T * parent)> dfsVisit;
-        if (parents.count(path))
-            throw makeCycleError(path, *parent);
 
-        if (!visited.insert(path).second)
+    dfsVisit = [&](const T & path, const T * parent) -> std::optional<Cycle<T>> {
-            return;
+        if (parents.count(path)) {
+            return Cycle{path, *parent};
+        }
+
+        if (!visited.insert(path).second) {
+            return std::nullopt;
+        }
         parents.insert(path);
 
         auto references = getChildren(path);
 
         for (auto & i : references)
             /* Don't traverse into items that don't exist in our starting set. */
-            if (i != path && items.count(i))
+            if (i != path && items.count(i)) {
-                dfsVisit(i, &path);
+                auto result = dfsVisit(i, &path);
+                if (result.has_value()) {
+                    return result;
+                }
+            }
 
         sorted.push_back(path);
         parents.erase(path);
+
+        return std::nullopt;
     };
 
-    for (auto & i : items)
+    for (auto & i : items) {
-        dfsVisit(i, nullptr);
+        auto cycle = dfsVisit(i, nullptr);
+        if (cycle.has_value()) {
+            return *cycle;
+        }
+    }
 
     std::reverse(sorted.begin(), sorted.end());