Skip to content
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

[RFC 0135] Custom asserts #135

Closed
wants to merge 6 commits into from
Closed
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
116 changes: 116 additions & 0 deletions rfcs/0135-custom-asserts.md
@@ -0,0 +1,116 @@
---
feature: custom-asserts
start-date: 2022-09-21
author: Anselm Schüler
co-authors: None
shepherd-team: None
shepherd-leader: None
related-issues: None
---

# Summary
[summary]: #summary

Allow users to use attribute sets with a boolean attribute `success` and a string attribute `message` instead of a boolean in `assert …; …` expressions.

# Motivation
[motivation]: #motivation

Since Nix is an untyped language, asserts are often needed to ensure that a function or program works correctly. However, the current assert system is unsuitable for more sophisticated error reporting that aims to inform the user as to what has happened.

Consider a nixpkgs package expression that wants to validate its arguments. Currently, the best way to
provide a custom error message is to use `assert … -> throw …; …`.
This method has several disadvantages: Since an implication from a falsehood is always true, you are required
to invert the condition. Additionally, since the assertion itself is not triggered by the error,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed this bit on my first read through. Note that this drawback is not an issue if you do assert condition || throw "…"; …. No need to invert, quite straightforward.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I removed that.

the function of the `assert` keyword is reduced to providing an imperative shorthand for `seq`. This also means that by default,
the error location is not printed, and there is no mention of an assert in the error message.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not true. throw also retains the location where it is called and it is displayed correctly to the user.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, I thought it was.

Instead, expressions could use this more natural syntax:

```nix
{ foo, bar }:
assert {
success = foo || bar;
message = "At least one of foo or bar must be set";
};
[ foo bar ]
```

Also consider, for instance, this simple type system:

```nix
rec {
assertType = type: locDescr: value:
if ! type.check value
then throw "Value at ${locDescr} was not of type ${type.name}"
else builtins.traceVerbose "Successfully checked type ${type.name} at ${locDescr}" value;
assertTypeSeq = type: locDescr: value1: value2:
builtins.seq (assertType type locDescr value1) value2;
intType = {
check = builtins.isInt;
name = "integer";
};
attrsOfType = subType: {
check = value:
builtins.isAttrs value
&& builtins.all subType.check (builtins.attrValues value);
name = "attribute set of ${subType.name}";
};
}
```

Users of this system would be forced to forgo the convenience of imperative-style `assert …; …` in favor of `seq`-like syntax in order to benefit from improved type errors. With this change, no longer! A variant `isType` function could be declared:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can kind of have your cake and eat it, too, with a custom system. Such one has actually been added to nixpkgs by @roberth:

lib.throwIfNot cond1 "msg1"
lib.throwIfNot cond2 "msg2"
stdenv.mkDerivation { /* … */ }

This relies on a neat trick that you can return the identity function from a function, causing anything further in the source file to act as if that lib.throwIfNot and its first two arguments hadn't been there.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh that’s neat! I still think assert …; is more idiomatic

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this prior discussion NixOS/nixpkgs#154292

Nitpick: Idioms evolve from a language and its use, not the other way around. We're designing a language feature, so existing idioms are relevant but of secondary importance.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that’s a nice way to view it! I rephrase: I think my suggestion would make it more natural, easier to understand and more distinctive. It uses the syntax specifically for asserts and only for that, instead of co-opting returning identity sometimes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, these other idioms do not demonstrate that this change is not needed, but demonstrate that there is a lack of unified assertion functionality, which has left the conciseness of the dedicated assert syntax behind.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we had $ like Haskell, we wouldn't miss the assert ..; syntax so much because we wouldn't need more parentheses either way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ keeps getting brought up, but it is an even more invasive change and would introduce more unfamiliar syntax to a language non FP programmers already struggle with conceptually…

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, IMO assert is a more user-friendly idiom. And it already exists in the language!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's little a bit unfair: for anybody with no lazy FP experience, syntax will be the least of their problems. No debugger, no printf(), and brain-meltingly weird stack traces.

But yeah, language stability is precious.


```nix
{
isType = type: locDescr: value:
let
success = type.check value;
message = if success
then "Value at ${locDescr} was not of type ${type.name}"
else "Successfully checked type ${type.name} at ${locDescr}";
in { inherit success message; };
}
```

Compare three implementations of a function that only takes attribute sets of integers:

```nix
with types;
{
onlyTakesAttrsOfInt1 = value:
assertType (attrsOfType intType) "onlyTakesAttrsOfInt1" value;
onlyTakesAttrsOfInt2 = value:
assertTypeSeq (attrsOfType intType) "onlyTakesAttrsOfInt1" value value;
onlyTakesAttrsOfInt3 = value:
assert isType (attrsOfType intType) "onlyTakesAttrsOfInt1" value;
value;
}
```

While these implementations get more and more verbose, they also get more and more idiomatic and flexible.

# Detailed design
[design]: #detailed-design

Allow users to use attribute sets with a boolean attribute `success` and a string attribute `message` instead of a boolean in `assert …; …` expressions.
If the `success` attribute is false, the assertion fails with a message including the `message` attribute.

This change requires no change to the language grammar.

# Drawbacks
[drawbacks]: #drawbacks

- Implementation clutter
- This could end up as a confusing and underutilized feature that hardly anybody knows about

# Alternatives
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should first consider the options we have today without language changes which don't seem to be mentioned in your RFC at all:

  • lib.assertMsg which outputs a message via builtins.trace if it is false: assert lib.assertMsg condition "message"; …
  • An idiom of using || throw to get an even nicer error message at the expense of it reading a bit strange to the uninitiated reader assert condition || throw "message"; …. This will also have correct location information etc. while hiding the often ugly and uninformative AST dump assert does by default.

[alternatives]: #alternatives

- Doing nothing and continuing to use whichever assertion method is most appropriate for a given use case
- Deprecating `assert …; …` expressions in favour of user-made type systems

schuelermine marked this conversation as resolved.
Show resolved Hide resolved
# Unresolved questions
[questions]: #unresolved-questions

- What should the exact name of `message` and `success` be?
- What should the error messsage be?