lib/trivial: add assertMsg and assertMsgOneOf #44136
Conversation
Profpatsch
force-pushed
the
lib-trivial-assertMessage
branch
from
a29c9cb
to
1e6af87
Compare
lib/trivial.nix
Outdated
| assertMsg = pred: msg: | ||
| if pred | ||
| then true | ||
| else builtins.trace msg false; |
There was a problem hiding this comment.
Why not make the assert part of the function?
There was a problem hiding this comment.
Hm, it has to do with the fact that assert is not really an expression (it doesn’t return a value) but a statement (that influences the state of evaluation) of sorts.
We could change it to include an assert, but that means it can’t appear in statement position any longer. Maybe that’s not bad, though.
There was a problem hiding this comment.
let
assertMsgIf = pred: msg: v:
if pred v then v else builtins.trace msg; assert false; /*never reached*/ {};
assertMsg = pred: assertMsgIf (const pred)
in
(assertMsg (bar != borz)) /*where to put the derivation?*/
It kind of inverts the flow; one could use reverse application, but it’s not clear to me what values to pipe through.
Every time you want to throw a more helpful message for a failing assert. I did have some examples in code that was not pushed ultimately. |
|
It would be good to start using it in nixpkgs, otherwise it will be removed by dead code elimination analysis. |
Profpatsch
changed the title
|
I’ve added a few examples for the asserts in |
lib/trivial.nix
Outdated
| (lib.elem val xs) | ||
| "${name} must be one of ${ | ||
| lib.generators.toPretty {} xs}, but is: ${ | ||
| lib.generators.toPretty {} val}"; |
There was a problem hiding this comment.
How about creating a more general assertType, which takes a type and calls .check on the value. Then this function is a specialization of it with type = types.enum xs.
|
IMHO, these functions should not be in |
|
good point. if it's the start of a new assertion system it might make sense to group them in a new |
|
- I think, syntactically, with current nix it would be nice to rename `assertMsg` to `withMessage` or something:
```
assert withMessage pred message
```
or
```
assert predWithMsg pred message
```
That function is `trivial`, btw. Should be reusable for other stuff like warnings too.
- I think `assertOneOf` should not exists, it does type checking for a specific type, I think we need to make `types-simple` to check the whole of NixOS, replace `types` with `types-simple` (I plan to get to that the next week), and then make a general type checking assert using those.
- Also, wouldn't it be nice if `assert` was a library function? We could then just define `assertWithMsg` too.
I think this should be pretty easy to do too. If we were to interpret top-level nix expression `a; b` as `seq a b` and define
```
assert = p: if p then null else throw "assert";
assertWithMsg = p: m: if p then null else throw m;
```
then `assert false; whatever` does exactly what you would expect. Migration paths is a bit tricky, though.
@edolstra, thoughts?
|
I thought about that and am open to changing the name(s).
Yeah, it’s pretty specific and should be generalized sometime, but I think it’s a good and pragmatic start. We can deprecate it or supersede it once we have figured out something more general.
Can you create a minimal working example? I don’t see how that could work in practice. |
|
name
I'm wholeheartedly for changing it.
the specific thing
I'm like really ewww about this, stuff like that will start being used outside of `nixpkgs` and getting rid of it gracefully will take two whole releases.
> assert = p: if p then null else throw "assert";
> assertWithMsg = p: m: if p then null else throw m;
Can you create a minimal working example? I don’t see how that could work in practice.
Like I said, you would need a patched nix for that. I mean nothing prevents nix from intepreting
```
assert false; "test"
```
more or less as
```
let
nonBultinAssert = p: if p then null else throw "assert";
assertWithMsg = p: m: if p then null else throw m;
in bultins.seq (nonBultinAssert false) "test"
```
|
Since the `assertOneOf` uses `lib.generators`, they are not really trivial anymore and should go into their own library file.
Profpatsch
force-pushed
the
lib-trivial-assertMessage
branch
from
34911e5
to
eca136a
Compare
|
Amended the last commit, apparently I missed a few unqualified uses after moving it out of |
|
Any more comments? Otherwise I’ll merge in a bit. |
|
FYI, I'm still 👎 on `assertOneOf`, still looks too ugly to me.
|
|
@oxij You are free to generalize it, but I wouldn’t want to block this issue until somebody does. |
|
@Profpatsch @infinisil can you cherry-pick this to release-18.09? |
|
According to the IRC bot, it was backported (to somewhere at least, can’t tell). |
|
Hmm, I don't see the |
| assertMsg = pred: msg: | ||
| if pred | ||
| then true | ||
| else builtins.trace msg false; |
There was a problem hiding this comment.
What about else throw "Assertion failed: ${msg}" instead? Using trace will log with lvlError and thus might be filtered out (eg. on Hydra, where only C++ exceptions are captured), throw on the other hand is fatal (at least until used with tryEval, but the same can be used for assertions anyway, so no need to abort).
There was a problem hiding this comment.
Sounds like a good improvement to me.
There was a problem hiding this comment.
Wait, how would that look in practice? Would it still be used like
assert (assertMsg ("foo" == "bar") "foo is not bar, silly"); ""
There was a problem hiding this comment.
Sure, the difference is just that assert here essentially becomes a no-op if pred doesn't match, because throw already takes care of raising an error before the actual assertion fails.
There was a problem hiding this comment.
Then the API stays the same, I see. I think we should do that.
This is a cute little trick to make nix print an error message if an assertion fails.
It was lingering in my local repo for quite some time.