remove getDerivations deduplication

deduplication does not currently work fully, showing derivations
multiple times if they have different underlying values. this can happen
by selecting the same derivation twice for two different attributes of a
set, using inherit-from (which reduces to the previous), importing
nixpkgs twice, or any other number of things.

since users already have to deal with duplicates for this reason it
won't hurt to add *more* duplicates. the alternative would be to
deduplicate fully, which would drop derivations that are currently
returned and those pose a regression risk.

Change-Id: I64b397351237e10375d270f1bddecb71f62aa131

1 files changed, 15 insertions, 13 deletions

diff --git a/src/libexpr/get-drvs.cc b/src/libexpr/get-drvs.cc
index 495407c39..e686ffe8c 100644
--- a/src/libexpr/get-drvs.cc
+++ b/src/libexpr/get-drvs.cc
@@ -296,22 +296,16 @@ void DrvInfo::setMeta(const std::string & name, Value * v)
 typedef std::set<Bindings *> Done;
 
 
-/* Evaluate value `v'.  If it evaluates to a set of type `derivation',
-   then put information about it in `drvs' (unless it's already in `done').
-   The result boolean indicates whether it makes sense
+/* The result boolean indicates whether it makes sense
    for the caller to recursively search for derivations in `v'. */
 static bool getDerivation(EvalState & state, Value & v,
-    const std::string & attrPath, DrvInfos & drvs, Done & done,
+    const std::string & attrPath, DrvInfos & drvs,
     bool ignoreAssertionFailures)
 {
     try {
         state.forceValue(v, v.determinePos(noPos));
         if (!state.isDerivation(v)) return true;
 
-        /* Remove spurious duplicates (e.g., a set like `rec { x =
-           derivation {...}; y = x;}'. */
-        if (!done.insert(v.attrs).second) return false;
-
         DrvInfo drv(state, attrPath, v.attrs);
 
         drv.queryName();
@@ -330,9 +324,8 @@ static bool getDerivation(EvalState & state, Value & v,
 std::optional<DrvInfo> getDerivation(EvalState & state, Value & v,
     bool ignoreAssertionFailures)
 {
-    Done done;
     DrvInfos drvs;
-    getDerivation(state, v, "", drvs, done, ignoreAssertionFailures);
+    getDerivation(state, v, "", drvs, ignoreAssertionFailures);
     if (drvs.size() != 1) return {};
     return std::move(drvs.front());
 }
@@ -347,6 +340,9 @@ static std::string addToPath(const std::string & s1, const std::string & s2)
 static std::regex attrRegex("[A-Za-z_][A-Za-z0-9-_+]*");
 
 
+/* Evaluate value `v'.  If it evaluates to a set of type `derivation',
+   then put information about it in `drvs'. If it evaluates to a different
+   kind of set recurse (unless it's already in `done'). */
 static void getDerivations(EvalState & state, Value & vIn,
     const std::string & pathPrefix, Bindings & autoArgs,
     DrvInfos & drvs, Done & done,
@@ -356,10 +352,14 @@ static void getDerivations(EvalState & state, Value & vIn,
     state.autoCallFunction(autoArgs, vIn, v);
 
     /* Process the expression. */
-    if (!getDerivation(state, v, pathPrefix, drvs, done, ignoreAssertionFailures)) ;
+    if (!getDerivation(state, v, pathPrefix, drvs, ignoreAssertionFailures)) ;
 
     else if (v.type() == nAttrs) {
 
+        /* Dont consider sets we've already seen, e.g. y in
+           `rec { x.d = derivation {...}; y = x; }`. */
+        if (!done.insert(v.attrs).second) return;
+
         /* !!! undocumented hackery to support combining channels in
            nix-env.cc. */
         bool combineChannels = v.attrs->find(state.symbols.create("_combineChannels")) != v.attrs->end();
@@ -376,7 +376,7 @@ static void getDerivations(EvalState & state, Value & vIn,
             std::string pathPrefix2 = addToPath(pathPrefix, state.symbols[i->name]);
             if (combineChannels)
                 getDerivations(state, *i->value, pathPrefix2, autoArgs, drvs, done, ignoreAssertionFailures);
-            else if (getDerivation(state, *i->value, pathPrefix2, drvs, done, ignoreAssertionFailures)) {
+            else if (getDerivation(state, *i->value, pathPrefix2, drvs, ignoreAssertionFailures)) {
                 /* If the value of this attribute is itself a set,
                    should we recurse into it?  => Only if it has a
                    `recurseForDerivations = true' attribute. */
@@ -390,9 +390,11 @@ static void getDerivations(EvalState & state, Value & vIn,
     }
 
     else if (v.type() == nList) {
+        // NOTE we can't really deduplicate here because small lists don't have stable addresses
+        // and can cause spurious duplicate detections due to v being on the stack.
         for (auto [n, elem] : enumerate(v.listItems())) {
             std::string pathPrefix2 = addToPath(pathPrefix, fmt("%d", n));
-            if (getDerivation(state, *elem, pathPrefix2, drvs, done, ignoreAssertionFailures))
+            if (getDerivation(state, *elem, pathPrefix2, drvs, ignoreAssertionFailures))
                 getDerivations(state, *elem, pathPrefix2, autoArgs, drvs, done, ignoreAssertionFailures);
         }
     }