Change 3 arg Assert to raise an error

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:
the few uses of the three argument version we found all seem to be
written by someone expecting them to raise an error!

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,22 @@ 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);
+        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.dynamic.c

@@ -53,12 +53,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 +72,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 +118,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 +137,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);
   }
  }
 
@@ -176,12 +156,7 @@ static Obj  HdlrFunc2 (
   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);
   }
  }
 

tst/test-compile/assert.g.out

@@ -1,4 +1,9 @@
 0
 2
-pass!
-end of function
+Error, Assertion failure: pass!
+ in
+  Assert( 2, false, "pass!\n" ); at assert.g:13 called from 
+<function "runtest">( <arguments> )
+ called from read-eval loop at *stdin*:1
+you may 'return;'
+