From 4d28cea27a797b318f9c5cb83cad1f90bebfaf53 Mon Sep 17 00:00:00 2001 From: Max Horn Date: Tue, 7 Jan 2025 22:51:41 +0100 Subject: [PATCH] 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 | 7 +++--- src/compiler.c | 4 +--- src/error.c | 16 +++++++++++++ src/error.h | 4 +++- src/intrprtr.c | 10 +-------- src/stats.c | 9 ++------ tst/test-compile/assert.g.dynamic.c | 35 +++++------------------------ tst/test-compile/assert.g.out | 9 ++++++-- 8 files changed, 38 insertions(+), 56 deletions(-) diff --git a/doc/ref/debug.xml b/doc/ref/debug.xml index 8f21f0341f..4f9882e6f9 100644 --- a/doc/ref/debug.xml +++ b/doc/ref/debug.xml @@ -344,10 +344,9 @@ if AssertionLevel() >= lev and not then fi; ]]>

-With the message argument form of the statement, -if the global assertion level is at least lev, condition cond -is tested and if it does not return true then message is -evaluated and printed. +If the message argument form of the statement +is provided, and if an error is raised, then this message is printed as part of +the error.

Assertions are used at various places in the library. Thus turning assertions on can slow code execution significantly. diff --git a/src/compiler.c b/src/compiler.c index 25d14f94c0..857fa364f4 100644 --- a/src/compiler.c +++ b/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" ); diff --git a/src/error.c b/src/error.c index dab98f8da2..49d4d1712b 100644 --- a/src/error.c +++ b/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(); + } +} + /**************************************************************************** ** diff --git a/src/error.h b/src/error.h index d9689a8ccc..64c3aa5a3f 100644 --- a/src/error.h +++ b/src/error.h @@ -383,10 +383,12 @@ Obj CALL_WITH_CATCH(Obj func, Obj args); /**************************************************************************** ** *F AssertionFailure() . . . . . . . . . . . trigger a GAP assertion failure +*F AssertionFailureWithMessage() ** -** 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); /**************************************************************************** diff --git a/src/intrprtr.c b/src/intrprtr.c index 77953a666b..140ed94697 100644 --- a/src/intrprtr.c +++ b/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; diff --git a/src/stats.c b/src/stats.c index 5ab723ab98..130562ad00 100644 --- a/src/stats.c +++ b/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; diff --git a/tst/test-compile/assert.g.dynamic.c b/tst/test-compile/assert.g.dynamic.c index f07c7aae7f..f68cc7a892 100644 --- a/tst/test-compile/assert.g.dynamic.c +++ b/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); } } diff --git a/tst/test-compile/assert.g.out b/tst/test-compile/assert.g.out index a3c1178e19..2c5b99c372 100644 --- a/tst/test-compile/assert.g.out +++ b/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 +( ) + called from read-eval loop at *stdin*:1 +you may 'return;' + \ No newline at end of file