libexpr: throw a more helpful eval-error if a builtin is not available due to a missing feature-flag

[Ma27]

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: Experimental Nix feature 'flakes' is missing to call builtins.getFlake! You can 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.

cc @edolstra @regnat

[thufschmitt]

The logic seems right, but I’m (very vaguely) worried about a possible performance impact on the evaluation (because the check for a missing feature needs to happen at each primop call).

Maybe you could rewrite the primop registration logic as something along the lines of:

            if (!primOp.requiredFeature || settings.isExperimentalFeatureEnabled(*primOp.requiredFeature))
                addPrimOp({
                    .fun = primOp.fun,
                    ...
                });
            else
                addPrimOp({
                    .fun = [](EvalState foo, Pos pos, Value ** args, Value & v){ throw EvalError("Builtin %s requires the experimental feature %s", primOp.name, *primOp.requiredFeature),
                     ...
                 });
                    ...
                });

so that we don’t need to check anything anymore when calling it

[Ma27]

@regnat this is basically what I wanted to implement first, however it didn't really work out:

• In order to throw an EvalError with the missing feature and primop name, the function needs access to the primOp which has to be done via a capture expression (i.e. [&]() or something similar).
• However, a lambda with a capture expression cannot be cast into a function pointer (which is the type of PrimOpFun). AFAIU this is because a function pointer cannot hold any "state" such as variables from the upper scope.
• Hacking around with static variables doesn't work out either because that'll lead us to bogus results as soon as we have multiple primops that are only available if a feature-flag is enabled (AFAICS only getFlake is affected currently).
• I tried to convert PrimOpFun into a std::function which fixed the compilation error, however I ran into weird memory corruptions that caused a segfault in the end when trying to call getFlake without flakes being enabled (I think this is because of primOp being copied into the function's closure somehow).

In the end, this is why I left PrimOpFun to be a function-pointer. Unless someone of you has a better idea - I mean, I'm not very seasoned in C++, so it's possible that I totally missed something - I think we could add a isFunctionEnabled-field to the PrimOp struct so that we don't have to check for feature-flags on each invocation, but only if a boolean is true/false.

[Ma27]

@regnat FWIW this is what I tried first and is now lying around in my git stash: https://gist.github.com/Ma27/32c502dcc39b46bfe3f4b301d0b616eb

[thufschmitt]

this is basically what I wanted to implement first, however it didn't really work out

Right. I think we could make this work by ensuring that we capture by copy everything that needs to be, but we can leave it out for now.

I just have one minor naming comment then, otherwise it’s good-to-go

[thufschmitt]

Suggested change

	std::optional<std::string> missingFeature;
	std::optional<std::string> requiredFeature;

(Unless you really want it to mean missingFeature, in which case I assume it should only be non-null if the feature is actually missing, but that’s not what the code is doing currently)

[edolstra]

This breaks the intent of why it's implemented the way it is: namely to allow Nix expressions to check for the availability of a feature by doing e.g. builtins ? getFlake. If it wasn't for that, we could just call requireExperimentalFeature("foo") inside the primop, no need for any special handling in callPrimOp().

[Ma27]

@edolstra I see your point and yeah, it's probably even better to just use requireExperimentalFeature (perhaps with some positioning information).

However, I think it's a rather obscure way to check for feature-availability via builtins ? getFlake for two reasons:

• It's not really clear for the reader of such an expression why this is done that way (i.e. on each occasion a comment is needed).
• This only works for flakes. I could imagine that at least for recursive-nix & ca-derivaitons (or some features in the future) it may be helpful to have a way to check for feature-availability.

Hence my suggestion to

• use requireExerpimentalFeature as you suggested
• add a new builtin builtins.isExperimentalFeatureEnabled (or a catchier name, idk :p)

From my PoV this can be done until 2.4 is out, after that it's difficult because changing the outcome of builtins ? getFlake also affects expressions not using experimental features and is thus a breaking change in the expression language itself.

WDYT? :)

[edolstra]

It just occurred to me that allowing any kind of feature check in Nix expressions (whether builtins ? foo or builtins.isExperimentalFeatureEnabled) is bad for purity, since it allows writing code like "if CA derivations are enabled then return a CA derivation else return a non-CA derivation". This breaks reproducibility and the evaluation cache (which currently doesn't take features into account).

So it's probably best to turn this into an uncatchable error using requireExperimentalFeature.

[Ma27]

Fair. Will update the PR accordingly later today :)

[Ma27]

@edolstra @regnat I implemented an alternative requireExperimentalFeature that throws an evaluation-error with a Pos. AFAICS this isn't an error that will be caught somewhere else where it's checked which feature is missing.

[Ma27]

Applied, thanks!

[Ma27]

@edolstra applied your suggestions, thanks!

[edolstra]

Thanks!

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-2-8-0-released/18714/14

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-2-8-0-released/18714/16