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

Conversation

schuelermine
Copy link

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/rfc-custom-asserts/21863/1

@schuelermine
Copy link
Author

I have removed the --trace-verbose feature from the proposal since it jeopardizes what is likely the most common use case.

- 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.

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.

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,
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.

}
```

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.

@edolstra
Copy link
Member

edolstra commented Oct 5, 2022

This RFC is open for shepherd nominations!

@edolstra edolstra added status: open for nominations Open for shepherding team nominations and removed status: new labels Oct 5, 2022
@kevincox
Copy link
Contributor

kevincox commented Nov 2, 2022

This RFC has not acquired enough shepherds. This typically shows lack of interest from the community. In order to progress a full shepherd team is required. Consider trying to raise interest by posting in Discourse, talking in Matrix or reaching out to people that you know.

If not enough shepherds can be found in the next month we will close this RFC until we can find enough interested participants. The PR can be reopened at any time if more shepherd nominations are made.

See more info on the Nix RFC process here

@edolstra
Copy link
Member

Since there are not enough shepherds yet, we have moved this RFC to draft state for now. It can be reopened at any time if more shepherd nominations are made.

@infinisil
Copy link
Member

Here's an interesting comment by @roberth:

Assertions are for checking the invariants of self-contained software units, and not for input validation across a public interface.

Thinking about this I'm coming to these conclusions:

  • The benefit of assert being a builtin language feature is that it can print the failing assertion expression, which is an easy way for programmers to know what went wrong without having to provide and maintain error messages for users.
    • Notably, to do this one doesn't need the assert syntax, a builtins.assert should theoretically also work. The main benefit with the assert syntax however is that you don't need to wrap the assertion expression in parenthesis, and that the ; clearly separates the assertion expression from the result expression:
      # assert syntax
      assert foo == bar;
      null
      # Potential assert builtin
      builtins.assert (foo == bar)
      null
  • Contrary to that, user-facing messages need to be manually written and maintained, so this use-case doesn't need any special builtin language feature, it can just use throw

Considering this, what's the benefit of this RFC then?

  • This RFC doesn't need the assertion expression printing ability of assert, since the programmer providers their own message to print.
  • This RFC also doesn't benefit from not having to wrap the assertion expression in parenthesis, because the assertion expression is an attribute value:
    assert {
      success = foo == bar;
      message = "failed";
    };
    null

This means that it's very much possible to implement almost exactly this using either a builtin function or a library function. And yes indeed, similar to the previously-mentioned throwIfNot:

# Could also be `builtins.assert`
lib.assert {
  success = foo == bar;
  message = "failed";
}
null

The only difference is that there's no ; indicating the end of the assertion expression, and from what I can tell this would therefore be the only advantage of making this a language builtin over just a builtins or library function.

I did think about how formatters might struggle formatting it correctly if there's no ;, but alejandra handles it just fine.

@infinisil
Copy link
Member

In addition, I want to point out that there's more than just boolean-style checks one might want to check. Notably there's also lib.asserts.assertOneOf, which wouldn't fit into this RFC's model. In addition I just opened a PR to introduce lib.asserts.assertAll. Undoubtedly there's other useful utilities that could be added. In comparison the proposal from this RFC is fairly limited and couldn't be used idiomatically for such cases.

@infinisil
Copy link
Member

infinisil commented Feb 15, 2023

So here's an alternate proposal:

  • Let's reinforce that the assert syntax is for internal invariant validation, with the benefit of always printing the assertion expression, it should not be used for user-facing errors.
    • In fact I think when an assertion expression fails to evaluate, it should not only throw that error, but also print the containing assertion expression
    • We might also consider making assert's non-tryCatch-able. If they're only for internal invariant checking, there's no good reason to catch it (though there's no good reason to catch any errors imo)
  • Let's introduce nixpkgs library functions in the style of this RFC. But we shouldn't use the name "assertions" because that would be confusing with assert. Instead I'm proposing "Guard". Here's some examples:
    lib.guard.success {
      value = foo;
      message = "something failed";
    }
    null
    lib.guard.oneOf {
      value = foo;
      valids = [ "foo" "bar" ];
      message = "something failed";
    }
    null

@schuelermine
Copy link
Author

That sounds good, I will close this RFC. If you want you can create an RFC for your idea.

@infinisil
Copy link
Member

Just realized a mistake I made above: Library functions like that only work for a subset of expressions of course, e.g.

lib.guard.success { ... }
if ... then ... else ...

wouldn't work of course.

While I'm not a huge fan of the original idea in the RFC here, how about this other idea I proposed, to have an apply expression like this:

apply foo;
apply bar;
baz

# Equivalent to
foo (bar baz)

This would then be general enough to be also usable for

apply lib.guard { ... };
if ... then ... else ...

but also much more, see NixOS/nix#1845.

@8573
Copy link

8573 commented May 22, 2023

how about this other idea I proposed, to have an apply expression like this

For these guards, would it be sufficient to use the existing with keyword, with lib.guard returning an empty set?

with lib.guard { ... };
if x then y else z

@roberth
Copy link
Member

roberth commented May 23, 2023

For these guards, would it be sufficient to use the existing with keyword

No, it's nicely lazy.

nix-repl> with throw "no"; true
true

nix-repl> let x = true; in with throw "no"; x          
true

with also has the unfortunate side effect of making scope checking ineffective at parsing time. Programming errors should be reported as soon as possible. Untested code paths rot even easier with with. This includes such things as exception messages, which are often harder to test than the main code, and are doubly painful when they rot.

nix-repl> { a = x: with {}; if x then y else z; }
{ a = «lambda @ (string):1:7»; }

nix-repl> { a = x: if x then y else z; }          
error: undefined variable 'y'

       at «string»:1:20:

            1| { a = x: if x then y else z; }
             |                    ^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants