-
-
Notifications
You must be signed in to change notification settings - Fork 13.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lib/trivial: add assertMsg and assertMsgOneOf #44136
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not make the assert
part of the function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there places where it could be used in nixpkgs?
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. |
I’ve added a few examples for the asserts in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! It's the start of a good assertion-testing library.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a good improvement to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.