[RFC] Making `assert` a keyword, or a method of `Object` ?

[mhermier]

Frequently/trivially, we need to do assertion checks in code. I would like to hear about what you think about adding it to the language. Trivially, we need to be able to do:

assert(check) { assert(check, "Assertion failed") }
assert(check, value) /* Note that we don't need to enforce a string here */ {
  if (!check) Fiber.abort(value)
  return check
}

Because of the language restrictions, I think it should be a keyword. In the cons for it, it would break quite some existing code, but it should be trivial to adapt since they should provide similar behavior/features.

An alternative, that is a little less intrusive, would be to make it part of Object methods. It has the advantage of being callable trivially from methods, but would make it odd in the scripting part, since one would have to Object.assert(...) and it probably needs to be present in static and object method to ease writing.

A last alternative, would be to make it in an outside class as a functor:

class Assert {
  call(check) { assert(check, "Assertion failed") }
  call(check, value) /* Note that we don't need to enforce a string here */ {
    if (!check) Fiber.abort(value)
    return check
  }
}

But It it raise the question of the location of such piece of code, in core, meta, {insert your favorite name here} module?

Comments welcome.

[mhermier]

ONe big pro for keyword, is that it can lazy evaluate the value to throw, and be removed by the compiler on release mode.

[ChayimFriedman2]

The biggest advantage for a keyword is that it can display the failed condition: assert(!"this is falsey") -> Assertion failed: '!"this is falsey"'. The biggest advantage for a method is not breaking existing code, as you stated.

Personally, I think we don't need that, let libraries roll their own if they need to.

[PureFox48]

As someone who writes a lot of code of this nature, I'd personally welcome some form of assertion being added to the language.

The question is whether it's important enough and would save enough code compared to what we do already to warrant a new keyword.

From a backwards compatibility viewpoint, I don't think that 'assert' is a particularly common word and it's most likely to have been used in the past by those who have been rolling their own assertions and who probably therefore wouldn't object to having to update their code if this feature were introduced.

There are, of course, other words one could use such as 'ensure' or 'require' but I suspect these words may be commoner than 'assert'.

If we were to use a method instead, then I would favor adding it as a static method to the Bool class as the 'check' parameter is boolean in nature. This also has the merit of being shorter to type than Object.

[ChayimFriedman2]

If we want to provide such thing, we also may consider additional assertions, like assertEq(), assertNe(), assertIs(), ....

I think leaving that untouched and let a library fill the gap is the best option.

I think Wren already have an assertion library, but I don't remember. Update: Here's: wren-please.

[PureFox48]

Interesting, I haven't seen that library before.

It looks reasonably complete. The only drawback is that you have to import it rather than it being 'on tap'.

[PureFox48]

I think after seeing what can be achieved by an external library, I'd be inclined now to just add a simple Bool.assert to the standard library so folks have something immediately available. Anyone who's needs are more sophisticated can import a library such as wren-please.

[ChayimFriedman2]

wren-please is nothing compared to chai.js 😄.

I prefer this as a method on Object, that seems more natural to me.

[ChayimFriedman2]

wren-test is a testing library that provides even better assertion API (IMHO). The only problem is that it's tied to the whole testing infra, but I guess it can be decoupled without hardness.

[PureFox48]

wren-test looks really interesting. It's a pity that it's no longer being actively maintained. No commits since 2017 :(

[ruby0x1]

(Commits aren't a sign of anything except that. Sometimes things are done and don't need any maintenance.)

[ChayimFriedman2]

And wren-please doesn't have new commits since 2015. If you'll find a bug, report it and don't get any answer, that's an indicator that the repo is no longer maintained.

[PureFox48]

(Commits aren't a sign of anything except that. Sometimes things are done and don't need any maintenance.)

True, though the author actually said here that he was no longer actively working on it and couldn't therefore guarantee to respond to PRs.

[clsource]

Although not super exhaustive I created a small testing lib for wren 👍

https://github.com/NinjasCL/domepunk/wiki/test.unit.wren

assert is just a simple object that contains helpful methods :)

/// Single Fiber Test with Description
  static testThatEpsilonExists {[
    "epsilon value exists",
    Fn.new { |assert|
      assert.isNotNull(Ss.epsilon)
    }
  ]}

If this can be added as a "native" assertion lib. Then maybe something like this: a method that accepts a function that will be called passing the assertion object as the first param.

class Assert {
  static isNotNull(got) {
     if (got is Null) {
       Fiber.abort(got.toString + " must not be null")
     } 
  }
}

class Object {
  assert (block) {
    return block.call(Assert, this)
  }
}

myobject.assert { |that|
  that.isNotNull(myobject.x)
}

[mhermier]

Most of usage talked here is about testing frameworks. It is a great technique, and they use alternative names to help producing quality of reports. But they only test the behavior externally.

But they only test one side of the code and sometimes you need to assert internally, like checking that some states are not reachable even if they heal themselves magically, and in the worst case even in production.

Having it as a keyword can allow to remove the check in production, even if it is a great source of heisenbug.

The Bool approach while interesting, is not realistic, because it requires boolean conversion which is not automatic.

[ChayimFriedman2]

But they only test one side of the code and sometimes you need to assert internally, like checking that some states are not reachable even if they heal themselves magically, and in the worst case even in production.

I completely agree. I brought wren-test as an example of an assertion library, that accidentally is part of a testing library, but the discussion slid...

[mhermier]

I made a working prototype with assert as keyword. Current implementation require a lot 3 new opcodes: DUP, JUMP_IFTRUE, ABORT (last one could have been avoided by calling `Fiber.abort()` directly, but I was lazy/will need it for my personal experiments). It currently lacks reporting the text of the code that failed but it should be more or less trivial to implement, and is not a priority for me.

From c12d04807dc88ba8667100330b21cced116ee440 Mon Sep 17 00:00:00 2001
From: Michel Hermier <michel.hermier@gmail.com>
Date: Sun, 20 Dec 2020 10:48:09 +0100
Subject: [PATCH 1/1] wren/compiler: Add `assert` keyword to the core language.

---
 src/vm/wren_compiler.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/src/vm/wren_compiler.c b/src/vm/wren_compiler.c
index ac1d6fc9..d90fabce 100644
--- a/src/vm/wren_compiler.c
+++ b/src/vm/wren_compiler.c
@@ -87,6 +87,7 @@ typedef enum
   TOKEN_BANGEQ,

   TOKEN_AS,
+  TOKEN_ASSERT,
   TOKEN_BREAK,
   TOKEN_CONTINUE,
   TOKEN_CLASS,
@@ -569,6 +570,7 @@ typedef struct
 static Keyword keywords[] =
 {
   {"as",        2, TOKEN_AS},
+  {"assert",    6, TOKEN_ASSERT},
   {"break",     5, TOKEN_BREAK},
   {"continue",  8, TOKEN_CONTINUE},
   {"class",     5, TOKEN_CLASS},
@@ -2280,6 +2282,30 @@ static void name(Compiler* compiler, bool canAssign)
   bareName(compiler, canAssign, variable);
 }

+static void assert(Compiler* compiler, bool canAssign)
+{
+  WrenVM* vm = compiler->parser->vm;
+
+  // Compile the condition.
+  consume(compiler, TOKEN_LEFT_PAREN, "Expect '(' after 'assert'.");
+  expression(compiler);
+  emitOp(compiler, CODE_DUP);
+  int jump = emitJump(compiler, CODE_JUMP_IF_TRUE);
+  if (match(compiler, TOKEN_COMMA))
+  {
+    expression(compiler);
+  }
+  else
+  {
+    // FIXME: Create a proper string: wrenStringFormat(vm, "Assertion failed: @", test_string);
+    Value error = wrenStringFormat(vm, "Assertion failed");
+    emitConstant(compiler, error);
+  }
+  emitOp(compiler, CODE_ABORT);
+  patchJump(compiler, jump);
+  consume(compiler, TOKEN_RIGHT_PAREN, "Expect ')' after after condition.");
+}
+
 static void null(Compiler* compiler, bool canAssign)
 {
   emitOp(compiler, CODE_NULL);
@@ -2648,6 +2674,7 @@ GrammarRule rules[] =
   /* TOKEN_BANGEQ        */ INFIX_OPERATOR(PREC_EQUALITY, "!="),

   /* TOKEN_AS            */ UNUSED,
+  /* TOKEN_ASSERT        */ PREFIX(assert),
   /* TOKEN_BREAK         */ UNUSED,
   /* TOKEN_CONTINUE      */ UNUSED,
   /* TOKEN_CLASS         */ UNUSED,
-- 
2.29.2

[ChayimFriedman2]

1. "Expect ')' after after condition"? Twice "after" 😄.
2. Where's the VM implementation?
3. Why is the CODE_DUP required?
4. JUMP_IF_TRUE can be avoided by negating the result, but I'm not sure this is better.
5. Truth, it's better to call Fiber.abort(_). This was the decision made in similar parts of the compiler (list literals, string interpolation - all call methods instead of use special opcode).
6. It will be good to be able to enable/disable assertions through a preprocessor flag, say ENABLE_ASSERTIONS. We can then skip the check in release builds. The problem is what to do with the expressions, currently we have no way to just skip an expression. Maybe a field in the Compiler struct will do, but it'll slow down almost everything in the compilation (startup times). Needs investigation.
7. Do we want to allow arbitrary expression as the error message? I thought about a simple string.

Great work!

[mhermier]

1. Thanks for noticing.
2. Is detail for now. we have to agree or disagree about it for now.
3. Took a function approach, so assert(42) returns 42, and because of JUMP_IF_TRUE consuming stack top, I had to DUP. But, I'm not against promoting it to a statement, I just thought it would be more fluent to have that feature.
4. I took that route to had it because it could help to do do { ... } while()
5. I also agree considering the current state of the VM. But I wanted it anyway because of my ~130 unpublished patches I manages privately, I wanted that anyway for future stuff.
6. That can be decided later, but I think 3 levels are needed for heisenbug: enabled/evaluate/disabled. But maybe it is overkill.
7. We might want string formatting, or use some class to handle some cases more gracefully, and since it was free I did it that way.

[ChayimFriedman2]

Took a function approach, so assert(42) returns 42, and because of JUMP_IF_TRUE consuming stack top, I had to DUP. But, I'm not against promoting it to a statement, I just thought it would be more fluent to have that feature.

I prefer it as a statement because it allows to elide it more easily. But we can also make it an expression that returns null.