-
-
Notifications
You must be signed in to change notification settings - Fork 158
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
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
818a681
rfc-0135: init
schuelermine 95f8286
rfc-0135: add better reasoning
schuelermine 4a11716
rfc-0135: add 1 question
schuelermine ad0b9ed
rfc-0135: reorder & spelling
schuelermine 7a5c67e
rfc-0135: remove note on inversion disadvantage
schuelermine 955f2e9
rfc-0135: remove incorrect statement
schuelermine File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,114 @@ | ||
--- | ||
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 the assertion itself is not triggered by the error, | ||
the function of the `assert` keyword is reduced to providing an imperative shorthand for `seq`. | ||
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: | ||
|
||
```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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
|
||
[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? |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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:
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.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 that’s neat! I still think
assert …;
is more idiomaticThere 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.
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.
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.
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.
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.
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.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.
If we had
$
like Haskell, we wouldn't miss theassert ..;
syntax so much because we wouldn't need more parentheses either way.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.
$
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…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.
Yeah, IMO
assert
is a more user-friendly idiom. And it already exists in the language!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.
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.