throw vs assert in packages

[Profpatsch]

@roberth I feel like we are moving in two different directions here, and we should standardize on one of them.

nixpkgs/pkgs/tools/networking/connman/connman.nix

Lines 52 to 53 in 92b09d4

	assert lib.asserts.assertOneOf "firewallType" firewallType [ "iptables" "nftables" ];
	assert lib.asserts.assertOneOf "dnsType" dnsType [ "internal" "systemd-resolved" ];

^ this was the way most packages wrote assertions so far, and lib.asserts was just a slight improvement over assert.

The new approach is different and diverges from the previous best practice.

Originally posted by @Profpatsch in #153740 (comment)

cc @roberth

[roberth]

The main reason I prefer throw over assert is the user experience, specifically error messages:

• The message always includes a verbose source location. This will point to the reusable validation logic, rather than the function that was called with unexpected inputs. This is mostly a problem for checkable values that reside in a reusable data structure like a list or attrset.
• The message can not be customized.

But the author also suffers:

• It is not a first-class value. You'd have to write (x: assert foo != bar; x) if you wanted to pass it anywhere, which is probably a higher order function, so now you'd get ridiculous lambdas like seqMapAttrs (k: v: x: assert k != v; x).

And some more reasons:

• "assertions" generally refer to internal assumptions about a program's own implementation, rather than inputs from a caller, let alone user. NixOS/module system may still move to "checks".
• It is redundant as a language construct.
• It encourages use of incomplete syntax in writing (docs etc)

One area where assertions are actually nice, is in simple checks, such as
assert withJpegSupport -> libjpeg != null;, because here the original expression is a useful description of the error. It does still produce a verbose source location, which does not point to the probable cause, which in this case is the caller of the package function.

My preferred direction for this issue, is to make types better suited for input validation: #154178

[sternenseemann]

Note that nothing prevents us from having asserts and nice error messages:

assert foo || throw "my nice error message";
# …

I've been putting off converting lib.assertMsg to do something similar for a while now — I think that would already be an easy improvement.

The trick that makes throwIfNot work, return the identity function, is nice, but I fear it may be confusing to casual readers how this code (imperative looking, missing expected parentheses) even works. The assert syntax is clearer here and would be covered in an introduction to the expression language.

[Profpatsch]

Oh, using || here is a cute trick, can we package that up into a function? Maybe even change assertMsg to use that?

[Profpatsch]

Also refer to this nix issue: NixOS/nix#749

[roberth]

Oh, using || here is a cute trick, can we package that up into a function? Maybe even change assertMsg to use that?

So that would be something like

cond: msg: x: assert cond || throw msg; x

or

cond: msg: assert cond || throw msg; x: x

These are subtly different and I believe that the latter is better because it the prior will not fire until the last application, which may not occur.

The latter is behaviorally indistinguishable from throwIfNot.

Again, the assert keyword is useless when it occurs in helper functions like these.

I guess you could currently use its syntax at the call sites instead, you please, but it is semantically questionable and, like any hack, may complicate the evolution of the evaluator if, assuming nix devs take nixpkgs into account (they should).

# I wouldn't recommend this, but I think this is what you're proposing:
assert throwIfNot foo "my nice error message";
# …

I wouldn't rename throwIfNot to assertMsg, because the function itself has nothing to do with assert, making it a confusing name.

[sternenseemann]

Oh, using || here is a cute trick, can we package that up into a function? Maybe even change assertMsg to use that?

As said, this has been my plan for quite some while.

I wouldn't rename throwIfNot to assertMsg, because the function itself has nothing to do with assert, making it a confusing name.

They are different things as well and changing assertMsg to return a function makes little sense, as it would break existing code.

These are subtly different and I believe that the latter is better because it the prior will not fire until the last application, which may not occur.

How so? The assert would be evaluated before the first application, if I understanding it correctly. In any case I think this is a very slim benefit and I'm still a bit concerned creating such a foreign “DSL” type thing (see #154292 (comment)).

[sternenseemann]

As said, this has been my plan for quite some while.

#155400