makeOverridable forces its original attrset rendering assert+override unusable

[kirelagin]

# test.nix
with import <nixpkgs> {};

let
  f = { dep ? null, feature ? true }:
    assert feature1-> dep != null;
    hello;
in
  lib.makeOverridable f { }

› nix eval '(import ./test.nix).override { feature = false; }'
error: assertion failed at /private/tmp/test.nix:6:5

makeOverridable is not being lazy enough and forces a call to the function with the original set of arguments before adding the .override attr. This behaviour renders assert feature -> dep != null (which is a common pattern in nixpkgs) somewhat useless: it is impossible to disable a feature whose dependency is missing with an .override as the original assert will trigger in any case.

I discovered this trying to install lighttpd (which does assert enableWebDAV -> libuuid != null) on darwin, but it won’t let me unset enableWebDAV or even override libuuid with some implementation.

AFAICT, @edolstra’s original makeOverridable made .override available before forcing anything, which is what I would expect, but this was changed in 9e4202d (@urkud) so that it can work on functions.

P.S. To be honest, I am not sure that my analysis is correct, as this change is 8.5 (😮) years old and, apparently, no one noticed, which seems unlikely, but 🤷‍♂️. I’m not sure if there is any policy in place that says that all dep+feature asserts should evaluate to true by default, but if there is one, then it is probably what helps here, but then the lighttpd expression violates it (#40623, @rkoe, @xeji).

(FYI @shlevy @ElvishJerricco as I saw you work on makeOverridable)

[kirelagin]

Oh well, lighttpd is no supported on Darwin anyway :(.
(UPD: and now it is: #53488)

[veprbl]

@kirelagin
In your analysis you seem to conclude that the regression has happened in 9e4202d. I would argue this is not true. Consider this:

nix-repl> ((assert false; 239) // { override = 30; }).override
error: assertion failed at (string):1:3

This seems to prove that the // is not as lazy as one could think. I guess, it must, at very least, evaluate its first argument to ensure that it is an attrset to begin with.

I tried to tweak the implementation of makeOverridable to ignore the original failure. If the problem was just with the evaluation of builtins.isAttrs ff, then it would be easy to just wrap it with builtins.tryEval and optimistically assume that "if it doesn't evaluate, it's an attrset". But one also needs to wrap the ff in (ff // { override = ..., which I don't know how to do in a satisfactory way.

Another way would be to get rid of the asserts that are now polluting the nixpkgs. For example I was always puzzled by these:

nixpkgs/pkgs/misc/ghostscript/default.nix

Line 9 in 84c44d8

assert cupsSupport -> cups != null;

Is it really a concern that someone will accidentally evaluate this in a context where cups = null and that will cause a subtle failure?

The asserts stand in the way of naively evaluating nixpkgs and this complicates a lot of the introspection that can be done. Those few that actually implement some more sensible constraints can be implemented via meta.broken instead.

[veprbl]

Well, see #36229

[kirelagin]

@veprbl Hmmm, yes, I think, you are right. // needs to force the set of keys, to be able to merge the attrsets 🤔.

[ElvishJerricco]

Yea, attrsets are strict in the keys, lazy in the values. So it must call the function to determine what keys will be present before it can add any keys like override. I'd recommend closing this issue, @FRidh.

[veprbl]

Lazy attribute names support discussed here NixOS/nix#4090