From 2b02ce0e481b653bcc5b403ef5d9e3670a88e8e5 Mon Sep 17 00:00:00 2001
From: Maximilian Bosch <maximilian@mbosch.me>
Date: Tue, 28 Sep 2021 22:12:41 +0200
Subject: [PATCH] libexpr: throw a more helpful eval-error if a builtin is not
 available due to a missing feature-flag
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

I found it somewhat confusing to have an error like

    error: attribute 'getFlake' missing

if the required experimental-feature (`flakes`) is not enabled. Instead,
I'd expect Nix to throw an error just like it's the case when using e.g. `nix
flake` without `flakes` being enabled.

With this change, the error looks like this:

    $ nix-instantiate -E 'builtins.getFlake "nixpkgs"'
    error: Cannot call 'builtins.getFlake' because experimental Nix feature 'flakes' is disabled. You can enable it via '--extra-experimental-features flakes'.

           at «string»:1:1:

                1| builtins.getFlake "nixpkgs"
                 | ^

I didn't use `settings.requireExperimentalFeature` here on purpose
because this doesn't contain a position. Also, it doesn't seem as if we
need to catch the error and check for the missing feature here since
this already happens at evaluation time.
---
 src/libexpr/eval.cc        | 17 +++++++++++++++++
 src/libexpr/eval.hh        |  6 ++++++
 src/libexpr/flake/flake.cc |  4 +++-
 src/libexpr/primops.cc     | 19 ++++++++-----------
 src/libexpr/primops.hh     |  4 +---
 5 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/src/libexpr/eval.cc b/src/libexpr/eval.cc
index bc41a2cd9..800839a8d 100644
--- a/src/libexpr/eval.cc
+++ b/src/libexpr/eval.cc
@@ -465,6 +465,23 @@ EvalState::~EvalState()
 }
 
 
+void EvalState::requireExperimentalFeatureOnEvaluation(
+    const std::string & feature,
+    const std::string_view fName,
+    const Pos & pos)
+{
+    if (!settings.isExperimentalFeatureEnabled(feature)) {
+        throw EvalError({
+            .msg = hintfmt(
+                "Cannot call '%2%' because experimental Nix feature '%1%' is disabled. You can enable it via '--extra-experimental-features %1%'.",
+                feature,
+                fName
+            ),
+            .errPos = pos
+        });
+    }
+}
+
 Path EvalState::checkSourcePath(const Path & path_)
 {
     if (!allowedPaths) return path_;
diff --git a/src/libexpr/eval.hh b/src/libexpr/eval.hh
index b29feb134..9df6150c6 100644
--- a/src/libexpr/eval.hh
+++ b/src/libexpr/eval.hh
@@ -140,6 +140,12 @@ public:
         std::shared_ptr<Store> buildStore = nullptr);
     ~EvalState();
 
+    void requireExperimentalFeatureOnEvaluation(
+        const std::string & feature,
+        const std::string_view fName,
+        const Pos & pos
+    );
+
     void addToSearchPath(const string & s);
 
     SearchPath getSearchPath() { return searchPath; }
diff --git a/src/libexpr/flake/flake.cc b/src/libexpr/flake/flake.cc
index 492b47115..1a1fa6938 100644
--- a/src/libexpr/flake/flake.cc
+++ b/src/libexpr/flake/flake.cc
@@ -688,6 +688,8 @@ void callFlake(EvalState & state,
 
 static void prim_getFlake(EvalState & state, const Pos & pos, Value * * args, Value & v)
 {
+    state.requireExperimentalFeatureOnEvaluation("flakes", "builtins.getFlake", pos);
+
     auto flakeRefS = state.forceStringNoCtx(*args[0], pos);
     auto flakeRef = parseFlakeRef(flakeRefS, {}, true);
     if (evalSettings.pureEval && !flakeRef.input.isImmutable())
@@ -703,7 +705,7 @@ static void prim_getFlake(EvalState & state, const Pos & pos, Value * * args, Va
         v);
 }
 
-static RegisterPrimOp r2("__getFlake", 1, prim_getFlake, "flakes");
+static RegisterPrimOp r2("__getFlake", 1, prim_getFlake);
 
 }
 
diff --git a/src/libexpr/primops.cc b/src/libexpr/primops.cc
index c98a30177..ead8c1ded 100644
--- a/src/libexpr/primops.cc
+++ b/src/libexpr/primops.cc
@@ -3606,15 +3606,13 @@ static RegisterPrimOp primop_splitVersion({
 RegisterPrimOp::PrimOps * RegisterPrimOp::primOps;
 
 
-RegisterPrimOp::RegisterPrimOp(std::string name, size_t arity, PrimOpFun fun,
-    std::optional<std::string> requiredFeature)
+RegisterPrimOp::RegisterPrimOp(std::string name, size_t arity, PrimOpFun fun)
 {
     if (!primOps) primOps = new PrimOps;
     primOps->push_back({
         .name = name,
         .args = {},
         .arity = arity,
-        .requiredFeature = std::move(requiredFeature),
         .fun = fun
     });
 }
@@ -3688,14 +3686,13 @@ void EvalState::createBaseEnv()
 
     if (RegisterPrimOp::primOps)
         for (auto & primOp : *RegisterPrimOp::primOps)
-            if (!primOp.requiredFeature || settings.isExperimentalFeatureEnabled(*primOp.requiredFeature))
-                addPrimOp({
-                    .fun = primOp.fun,
-                    .arity = std::max(primOp.args.size(), primOp.arity),
-                    .name = symbols.create(primOp.name),
-                    .args = std::move(primOp.args),
-                    .doc = primOp.doc,
-                });
+            addPrimOp({
+                .fun = primOp.fun,
+                .arity = std::max(primOp.args.size(), primOp.arity),
+                .name = symbols.create(primOp.name),
+                .args = std::move(primOp.args),
+                .doc = primOp.doc,
+            });
 
     /* Add a wrapper around the derivation primop that computes the
        `drvPath' and `outPath' attributes lazily. */
diff --git a/src/libexpr/primops.hh b/src/libexpr/primops.hh
index 9d42d6539..5b16e075f 100644
--- a/src/libexpr/primops.hh
+++ b/src/libexpr/primops.hh
@@ -15,7 +15,6 @@ struct RegisterPrimOp
         std::vector<std::string> args;
         size_t arity = 0;
         const char * doc;
-        std::optional<std::string> requiredFeature;
         PrimOpFun fun;
     };
 
@@ -28,8 +27,7 @@ struct RegisterPrimOp
     RegisterPrimOp(
         std::string name,
         size_t arity,
-        PrimOpFun fun,
-        std::optional<std::string> requiredFeature = {});
+        PrimOpFun fun);
 
     RegisterPrimOp(Info && info);
 };