Merge pull request #10564 from edolstra/remove-forceErrors

AttrCursor: Remove forceErrors

src/libexpr/eval-cache.cc

@@ -7,6 +7,25 @@
 
 namespace nix::eval_cache {
 
+CachedEvalError::CachedEvalError(ref<AttrCursor> cursor, Symbol attr)
+    : EvalError(cursor->root->state, "cached failure of attribute '%s'", cursor->getAttrPathStr(attr))
+    , cursor(cursor), attr(attr)
+{ }
+
+void CachedEvalError::force()
+{
+    auto & v = cursor->forceValue();
+
+    if (v.type() == nAttrs) {
+        auto a = v.attrs()->get(this->attr);
+
+        state.forceValue(*a->value, a->pos);
+    }
+
+    // Shouldn't happen.
+    throw EvalError(state, "evaluation of cached failed attribute '%s' unexpectedly succeeded", cursor->getAttrPathStr(attr));
+}
+
 static const char * schema = R"sql(
 create table if not exists Attributes (
     parent      integer not null,
@@ -470,7 +489,7 @@ Suggestions AttrCursor::getSuggestionsForAttr(Symbol name)
     return Suggestions::bestMatches(strAttrNames, root->state.symbols[name]);
 }
 
-std::shared_ptr<AttrCursor> AttrCursor::maybeGetAttr(Symbol name, bool forceErrors)
+std::shared_ptr<AttrCursor> AttrCursor::maybeGetAttr(Symbol name)
 {
     if (root->db) {
         if (!cachedValue)
@@ -487,12 +506,9 @@ std::shared_ptr<AttrCursor> AttrCursor::maybeGetAttr(Symbol name, bool forceErro
                 if (attr) {
                     if (std::get_if<missing_t>(&attr->second))
                         return nullptr;
-                    else if (std::get_if<failed_t>(&attr->second)) {
-                        if (forceErrors)
-                            debug("reevaluating failed cached attribute '%s'", getAttrPathStr(name));
+                    else if (std::get_if<failed_t>(&attr->second))
+                        throw CachedEvalError(ref(shared_from_this()), name);
                     else
-                            throw CachedEvalError(root->state, "cached failure of attribute '%s'", getAttrPathStr(name));
-                    } else
                         return std::make_shared<AttrCursor>(root,
                             std::make_pair(shared_from_this(), name), nullptr, std::move(attr));
                 }
@@ -537,9 +553,9 @@ std::shared_ptr<AttrCursor> AttrCursor::maybeGetAttr(std::string_view name)
     return maybeGetAttr(root->state.symbols.create(name));
 }
 
-ref<AttrCursor> AttrCursor::getAttr(Symbol name, bool forceErrors)
+ref<AttrCursor> AttrCursor::getAttr(Symbol name)
 {
-    auto p = maybeGetAttr(name, forceErrors);
+    auto p = maybeGetAttr(name);
     if (!p)
         throw Error("attribute '%s' does not exist", getAttrPathStr(name));
     return ref(p);
@@ -550,11 +566,11 @@ ref<AttrCursor> AttrCursor::getAttr(std::string_view name)
     return getAttr(root->state.symbols.create(name));
 }
 
-OrSuggestions<ref<AttrCursor>> AttrCursor::findAlongAttrPath(const std::vector<Symbol> & attrPath, bool force)
+OrSuggestions<ref<AttrCursor>> AttrCursor::findAlongAttrPath(const std::vector<Symbol> & attrPath)
 {
     auto res = shared_from_this();
     for (auto & attr : attrPath) {
-        auto child = res->maybeGetAttr(attr, force);
+        auto child = res->maybeGetAttr(attr);
         if (!child) {
             auto suggestions = res->getSuggestionsForAttr(attr);
             return OrSuggestions<ref<AttrCursor>>::failed(suggestions);
@@ -751,7 +767,7 @@ bool AttrCursor::isDerivation()
 
 StorePath AttrCursor::forceDerivation()
 {
-    auto aDrvPath = getAttr(root->state.sDrvPath, true);
+    auto aDrvPath = getAttr(root->state.sDrvPath);
     auto drvPath = root->state.store->parseStorePath(aDrvPath->getString());
     drvPath.requireDerivation();
     if (!root->state.store->isValidPath(drvPath) && !settings.readOnlyMode) {

src/libexpr/eval-cache.hh

@@ -10,14 +10,28 @@
 
 namespace nix::eval_cache {
 
-MakeError(CachedEvalError, EvalError);
-
 struct AttrDb;
 class AttrCursor;
 
+struct CachedEvalError : EvalError
+{
+    const ref<AttrCursor> cursor;
+    const Symbol attr;
+
+    CachedEvalError(ref<AttrCursor> cursor, Symbol attr);
+
+    /**
+     * Evaluate this attribute, which should result in a regular
+     * `EvalError` exception being thrown.
+     */
+    [[noreturn]]
+    void force();
+};
+
 class EvalCache : public std::enable_shared_from_this<EvalCache>
 {
     friend class AttrCursor;
+    friend class CachedEvalError;
 
     std::shared_ptr<AttrDb> db;
     EvalState & state;
@@ -73,6 +87,7 @@ typedef std::variant<
 class AttrCursor : public std::enable_shared_from_this<AttrCursor>
 {
     friend class EvalCache;
+    friend class CachedEvalError;
 
     ref<EvalCache> root;
     typedef std::optional<std::pair<std::shared_ptr<AttrCursor>, Symbol>> Parent;
@@ -102,11 +117,11 @@ public:
 
     Suggestions getSuggestionsForAttr(Symbol name);
 
-    std::shared_ptr<AttrCursor> maybeGetAttr(Symbol name, bool forceErrors = false);
+    std::shared_ptr<AttrCursor> maybeGetAttr(Symbol name);
 
     std::shared_ptr<AttrCursor> maybeGetAttr(std::string_view name);
 
-    ref<AttrCursor> getAttr(Symbol name, bool forceErrors = false);
+    ref<AttrCursor> getAttr(Symbol name);
 
     ref<AttrCursor> getAttr(std::string_view name);
 
@@ -114,7 +129,7 @@ public:
      * Get an attribute along a chain of attrsets. Note that this does
      * not auto-call functors or functions.
      */
-    OrSuggestions<ref<AttrCursor>> findAlongAttrPath(const std::vector<Symbol> & attrPath, bool force = false);
+    OrSuggestions<ref<AttrCursor>> findAlongAttrPath(const std::vector<Symbol> & attrPath);
 
     std::string getString();
 

src/libexpr/eval-error.cc

@@ -98,7 +98,6 @@ template class EvalErrorBuilder<TypeError>;
 template class EvalErrorBuilder<UndefinedVarError>;
 template class EvalErrorBuilder<MissingArgumentError>;
 template class EvalErrorBuilder<InfiniteRecursionError>;
-template class EvalErrorBuilder<CachedEvalError>;
 template class EvalErrorBuilder<InvalidPathError>;
 
 }

src/libexpr/eval-error.hh

@@ -43,7 +43,6 @@ MakeError(Abort, EvalError);
 MakeError(TypeError, EvalError);
 MakeError(UndefinedVarError, EvalError);
 MakeError(MissingArgumentError, EvalError);
-MakeError(CachedEvalError, EvalError);
 MakeError(InfiniteRecursionError, EvalError);
 
 struct InvalidPathError : public EvalError

src/nix/flake.cc

@@ -1176,7 +1176,7 @@ struct CmdFlakeShow : FlakeCommand, MixJSON
                 // If we don't recognize it, it's probably content
                 return true;
             } catch (EvalError & e) {
-                // Some attrs may contain errors, eg. legacyPackages of
+                // Some attrs may contain errors, e.g. legacyPackages of
                 // nixpkgs. We still want to recurse into it, instead of
                 // skipping it at all.
                 return true;

src/nix/main.cc

@@ -18,6 +18,7 @@
 #include "terminal.hh"
 #include "users.hh"
 #include "network-proxy.hh"
+#include "eval-cache.hh"
 
 #include <sys/types.h>
 #include <regex>
@@ -532,7 +533,15 @@ void mainWrapped(int argc, char * * argv)
     if (args.command->second->forceImpureByDefault() && !evalSettings.pureEval.overridden) {
         evalSettings.pureEval = false;
     }
+
+    try {
         args.command->second->run();
+    } catch (eval_cache::CachedEvalError & e) {
+        /* Evaluate the original attribute that resulted in this
+           cached error so that we can show the original error to the
+           user. */
+        e.force();
+    }
 }
 
 }

tests/functional/flakes/eval-cache.sh

@@ -0,0 +1,22 @@
+source ./common.sh
+
+requireGit
+
+flake1Dir="$TEST_ROOT/eval-cache-flake"
+
+createGitRepo "$flake1Dir" ""
+
+cat >"$flake1Dir/flake.nix" <<EOF
+{
+  description = "Fnord";
+  outputs = { self }: {
+    foo.bar = throw "breaks";
+  };
+}
+EOF
+
+git -C "$flake1Dir" add flake.nix
+git -C "$flake1Dir" commit -m "Init"
+
+expect 1 nix build "$flake1Dir#foo.bar" 2>&1 | grepQuiet 'error: breaks'
+expect 1 nix build "$flake1Dir#foo.bar" 2>&1 | grepQuiet 'error: breaks'

tests/functional/local.mk

@@ -17,6 +17,7 @@ nix_tests = \
   flakes/build-paths.sh \
   flakes/flake-in-submodule.sh \
   flakes/prefetch.sh \
+  flakes/eval-cache.sh \
   gc.sh \
   nix-collect-garbage-d.sh \
   remote-store.sh \