Change 3 arg Assert to raise an error (#5904)

Surprisingly, the three argument version of GAP's `Assert` statement
did not raise an error, unlike the more commonly used two argument
version. That is:

    gap> Assert(0, false);
    Error, Assertion failure
    not in any function at *stdin*:1
    you may 'return;'
    brk>
    gap> Assert(0, false, "MESSAGE");
    MESSAGE

This is a really surprising an unexpected behavior. After some
discussion we agreed that both should raise an error. While this
is technically a breaking change, we expect the impact of this on
the GAP package ecosystem to be minimal, and if at all positive:
all uses of the three argument version we found all seem to be
written by someone expecting them to raise an error!

So the new behaviour now is this:

    gap> Assert(0, false, "MESSAGE");
    Error, Assertion failure: MESSAGE
    not in any function at *stdin*:1
    you may 'return;'
    brk>

doc/ref/debug.xml

@@ -344,10 +344,9 @@ if AssertionLevel() >= lev and not <cond> then
 fi;
 ]]></Log>
 <P/>
-With the <A>message</A> argument form of the <Ref Func="Assert"/> statement,
-if the global assertion level is at least <A>lev</A>, condition <A>cond</A>
-is tested and if it does not return <K>true</K> then <A>message</A> is
-evaluated and printed.
+If the <A>message</A> argument form of the <Ref Func="Assert"/> statement
+is provided, and if an error is raised, then this message is printed as part of
+the error.
 <P/>
 Assertions are used at various places in the library.
 Thus turning assertions on can slow code execution significantly.

src/compiler.c

@@ -4994,9 +4994,7 @@ static void CompAssert3(Stat stat)
     cnd = CompBoolExpr(READ_STAT(stat, 1));
     Emit( "if ( ! %c ) {\n", cnd );
     msg = CompExpr(READ_STAT(stat, 2));
-    Emit( "if ( %c != (Obj)(UInt)0 )", msg );
-    Emit( "{\n if ( IS_STRING_REP ( %c ) )\n", msg);
-    Emit( "   PrintString1( %c);\n else\n   PrintObj(%c);\n}\n", msg, msg );
+    Emit( "AssertionFailureWithMessage(%c);\n", msg );
     Emit( "}\n" );
     Emit( "}\n" );
 

src/error.c

@@ -596,6 +596,23 @@ void AssertionFailure(void)
     ErrorReturnVoid("Assertion failure", 0, 0, "you may 'return;'");
 }
 
+void AssertionFailureWithMessage(Obj message)
+{
+    if (message == 0) {
+        // this case is triggered by code like this: Assert(0, false, Error("boo"));
+        // at least if the user enters `return;` into the break loop opened by this.
+        AssertionFailure();
+    }
+    else if (IS_STRING_REP(message)) {
+        ErrorReturnVoid("Assertion failure: %g", (Int)message, 0, "you may 'return;'");
+    }
+    else {
+        PrintObj(message);
+        Pr("\n", 0, 0);
+        AssertionFailure();
+    }
+}
+
 
 /****************************************************************************
 **

src/error.h

@@ -383,10 +383,12 @@ Obj CALL_WITH_CATCH(Obj func, Obj args);
 /****************************************************************************
 **
 *F  AssertionFailure() . . . . . . . . . . .  trigger a GAP assertion failure
+*F  AssertionFailureWithMessage(<obj>)
 **
-**  This helper function is used by GAP's 'Assert' statement.
+**  These helper functions are used by GAP's 'Assert' statement.
 */
 void AssertionFailure(void);
+void AssertionFailureWithMessage(Obj message);
 
 
 /****************************************************************************

src/intrprtr.c

@@ -4082,7 +4082,6 @@ void IntrAssertAfterCondition(IntrState * intr)
         return;
     }
 
-
     condition = PopObj(intr);
 
     if (condition == True)
@@ -4106,7 +4105,6 @@ void IntrAssertEnd2Args(IntrState * intr)
         return;
     }
 
-
     if (intr->ignoring == 0)
         AssertionFailure();
     else
@@ -4132,15 +4130,9 @@ void IntrAssertEnd3Args(IntrState * intr)
         return;
     }
 
-
     if (intr->ignoring == 0) {
         message = PopVoidObj(intr);
-        if (message != (Obj)0) {
-            if (IS_STRING_REP(message))
-                PrintString1(message);
-            else
-                PrintObj(message);
-        }
+        AssertionFailureWithMessage(message);
     }
     else
         intr->ignoring -= 2;

src/stats.c

@@ -925,13 +925,8 @@ static ExecStatus ExecAssert3Args(Stat stat)
         RequireTrueOrFalse("Assert", cond);
         if (cond == False) {
             message = EVAL_EXPR(READ_STAT(stat, 2));
-            if ( message != (Obj) 0 ) {
-                SET_BRK_CALL_TO( stat );
-                if (IS_STRING_REP( message ))
-                    PrintString1( message );
-                else
-                    PrintObj(message);
-            }
+            SET_BRK_CALL_TO( stat );
+            AssertionFailureWithMessage(message);
         }
     }
     return STATUS_END;

tst/test-compile/assert.g

@@ -10,9 +10,11 @@ runtest := function()
     Assert(3, false);
     Assert(2, true, "fail-D");
     Assert(2, true);
+
+    # ensure we don't abort after an error
+    BreakOnError := false;
+
     Assert(2, false, "pass!\n");
-    # We can't test this next line, as it produces
-    # <compiled or corrupted statement> when compiled
-    # Assert(2, false);
+    Assert(2, false);
     Print("end of function\n");
 end;

tst/test-compile/assert.g.dynamic.c

@@ -1,6 +1,6 @@
 /* C file produced by GAC */
 #include "compiled.h"
-#define FILE_CRC  "47091879"
+#define FILE_CRC  "-125967076"
 
 /* global variables used in handlers */
 static GVar G_Print;
@@ -10,6 +10,7 @@ static Obj  GF_SetAssertionLevel;
 static GVar G_AssertionLevel;
 static Obj  GF_AssertionLevel;
 static GVar G_runtest;
+static GVar G_BreakOnError;
 
 /* record names used in handlers */
 
@@ -53,12 +54,7 @@ static Obj  HdlrFunc2 (
   t_1 = (Obj)(UInt)(t_2 != False);
   if ( ! t_1 ) {
    t_2 = MakeString( "fail-A" );
-   if ( t_2 != (Obj)(UInt)0 ){
-     if ( IS_STRING_REP ( t_2 ) )
-       PrintString1( t_2);
-     else
-       PrintObj(t_2);
-   }
+   AssertionFailureWithMessage(t_2);
   }
  }
 
@@ -77,12 +73,7 @@ static Obj  HdlrFunc2 (
   t_1 = (Obj)(UInt)(t_2 != False);
   if ( ! t_1 ) {
    t_2 = MakeString( "fail-B" );
-   if ( t_2 != (Obj)(UInt)0 ){
-     if ( IS_STRING_REP ( t_2 ) )
-       PrintString1( t_2);
-     else
-       PrintObj(t_2);
-   }
+   AssertionFailureWithMessage(t_2);
   }
  }
 
@@ -128,12 +119,7 @@ static Obj  HdlrFunc2 (
   t_1 = (Obj)(UInt)(t_2 != False);
   if ( ! t_1 ) {
    t_2 = MakeString( "fail-C" );
-   if ( t_2 != (Obj)(UInt)0 ){
-     if ( IS_STRING_REP ( t_2 ) )
-       PrintString1( t_2);
-     else
-       PrintObj(t_2);
-   }
+   AssertionFailureWithMessage(t_2);
   }
  }
 
@@ -152,12 +138,7 @@ static Obj  HdlrFunc2 (
   t_1 = (Obj)(UInt)(t_2 != False);
   if ( ! t_1 ) {
    t_2 = MakeString( "fail-D" );
-   if ( t_2 != (Obj)(UInt)0 ){
-     if ( IS_STRING_REP ( t_2 ) )
-       PrintString1( t_2);
-     else
-       PrintObj(t_2);
-   }
+   AssertionFailureWithMessage(t_2);
   }
  }
 
@@ -170,18 +151,26 @@ static Obj  HdlrFunc2 (
   }
  }
 
+ /* BreakOnError := false; */
+ t_1 = False;
+ AssGVar( G_BreakOnError, t_1 );
+
  /* Assert( 2, false, "pass!\n" ); */
  if ( STATE(CurrentAssertionLevel) >= 2 ) {
   t_2 = False;
   t_1 = (Obj)(UInt)(t_2 != False);
   if ( ! t_1 ) {
    t_2 = MakeString( "pass!\n" );
-   if ( t_2 != (Obj)(UInt)0 ){
-     if ( IS_STRING_REP ( t_2 ) )
-       PrintString1( t_2);
-     else
-       PrintObj(t_2);
-   }
+   AssertionFailureWithMessage(t_2);
+  }
+ }
+
+ /* Assert( 2, false ); */
+ if ( STATE(CurrentAssertionLevel) >= 2 ) {
+  t_2 = False;
+  t_1 = (Obj)(UInt)(t_2 != False);
+  if ( ! t_1 ) {
+   AssertionFailure();
   }
  }
 
@@ -223,15 +212,17 @@ static Obj  HdlrFunc1 (
       Assert( 3, false );
       Assert( 2, true, "fail-D" );
       Assert( 2, true );
+      BreakOnError := false;
       Assert( 2, false, "pass!\n" );
+      Assert( 2, false );
       Print( "end of function\n" );
       return;
   end; */
  t_1 = NewFunction( NameFunc[2], 0, 0, HdlrFunc2 );
  SET_ENVI_FUNC( t_1, STATE(CurrLVars) );
  t_2 = NewFunctionBody();
  SET_STARTLINE_BODY(t_2, 1);
- SET_ENDLINE_BODY(t_2, 18);
+ SET_ENDLINE_BODY(t_2, 20);
  SET_FILENAME_BODY(t_2, FileName);
  SET_BODY_FUNC(t_1, t_2);
  AssGVar( G_runtest, t_1 );
@@ -250,6 +241,7 @@ static Int PostRestore ( StructInitInfo * module )
  G_SetAssertionLevel = GVarName( "SetAssertionLevel" );
  G_AssertionLevel = GVarName( "AssertionLevel" );
  G_runtest = GVarName( "runtest" );
+ G_BreakOnError = GVarName( "BreakOnError" );
 
  /* record names used in handlers */
 
@@ -309,7 +301,7 @@ static Int InitLibrary ( StructInitInfo * module )
 static StructInitInfo module = {
  .type        = MODULE_DYNAMIC,
  .name        = "assert.g",
- .crc         = 47091879,
+ .crc         = -125967076,
  .initKernel  = InitKernel,
  .initLibrary = InitLibrary,
  .postRestore = PostRestore,

tst/test-compile/assert.g.out

@@ -1,4 +1,4 @@
 0
 2
-pass!
-end of function
+Error, Assertion failure: pass!
+

tst/testinstall/kernel/intrprtr.tst

@@ -22,9 +22,12 @@ gap> Assert(0, 0, "message\n");
 Error, Assert: <cond> must be 'true' or 'false' (not the integer 0)
 gap> Assert(0, true, "message\n");
 gap> Assert(0, false, "message\n");
-message
+Error, Assertion failure: message
+
 gap> Assert(0, false, 1); Print("\n"); # message can also be any object
 1
+Error, Assertion failure
+
 gap> Assert(100, 0, "message\n");
 gap> Assert(100, true, "message\n");
 gap> Assert(100, false, "message\n");

tst/testinstall/kernel/stats.tst

@@ -30,9 +30,11 @@ gap> function() Assert(0, 0, "message\n"); end();
 Error, Assert: <cond> must be 'true' or 'false' (not the integer 0)
 gap> function() Assert(0, true, "message\n"); end();
 gap> function() Assert(0, false, "message\n"); end();
-message
+Error, Assertion failure: message
+
 gap> function() Assert(0, false, 1); Print("\n"); end(); # message can also be any object
 1
+Error, Assertion failure
 gap> function() Assert(100, 0, "message\n"); end();
 gap> function() Assert(100, true, "message\n"); end();
 gap> function() Assert(100, false, "message\n"); end();