From 2b48e8110df548a5dc748310f212182709ca81c7 Mon Sep 17 00:00:00 2001 From: Max Horn Date: Tue, 28 Aug 2018 12:28:41 +0200 Subject: [PATCH] kernel: unify 'and' for filters in interpreter and executor Also start using IS_FILTER. Note that previously, this input was silently accepted: gap> Center and IsAssociative; >"> This was effectively treated equivalently to `HasCenter and IsAssociative;`. We now could reject such code. But unfortunately a few packages accidentally relied on this bug. We thus only print a warning for now, and will turn this into a proper error only once all affect packages released a fixed version. --- src/exprs.c | 20 ++++++++--- src/intrprtr.c | 20 ++++++++--- src/opers.h | 10 ++++++ tst/testinstall/boolean.tst | 69 +++++++++++++++++++++++++++++++++---- 4 files changed, 105 insertions(+), 14 deletions(-) diff --git a/src/exprs.c b/src/exprs.c index ef5011a1cd6..51e1e34fd35 100644 --- a/src/exprs.c +++ b/src/exprs.c @@ -183,15 +183,27 @@ Obj EvalAnd ( } /* handle the 'and' of two filters */ - else if ( TNUM_OBJ(opL) == T_FUNCTION ) { + else if (IS_PSEUDO_FILTER(opL)) { + if (!IS_FILTER(opL)) { + // support this for backwards compatibility; see discussion on + // https://github.com/gap-system/gap/pull/2732 + Warning("operation '%g' used in AND-filter is not a filter", + (Int)NAME_FUNC(opL), 0); + } tmp = READ_EXPR(expr, 1); opR = EVAL_EXPR( tmp ); - if ( TNUM_OBJ(opR) == T_FUNCTION ) { + if (IS_PSEUDO_FILTER(opR)) { + if (!IS_FILTER(opR)) { + // support this for backwards compatibility; see discussion on + // https://github.com/gap-system/gap/pull/2732 + Warning("operation '%g' used in AND-filter is not a filter", + (Int)NAME_FUNC(opR), 0); + } return NewAndFilter( opL, opR ); } else { ErrorQuit( - " must be 'true' or 'false' (not a %s)", + " must be a filter (not a %s)", (Int)TNAM_OBJ(opL), 0L ); } } @@ -199,7 +211,7 @@ Obj EvalAnd ( /* signal an error */ else { ErrorQuit( - " must be 'true' or 'false' (not a %s)", + " must be 'true' or 'false' or a filter (not a %s)", (Int)TNAM_OBJ(opL), 0L ); } diff --git a/src/intrprtr.c b/src/intrprtr.c index 8fea3a8f5ba..0c39daffbf0 100644 --- a/src/intrprtr.c +++ b/src/intrprtr.c @@ -1319,13 +1319,25 @@ void IntrAnd ( void ) } /* handle the 'and' of two filters */ - else if ( IS_OPERATION(opL) ) { - if ( IS_OPERATION(opR) ) { + else if (IS_PSEUDO_FILTER(opL)) { + if (!IS_FILTER(opL)) { + // support this for backwards compatibility; see discussion on + // https://github.com/gap-system/gap/pull/2732 + Warning("operation '%g' used in AND-filter is not a filter\n", + (Int)NAME_FUNC(opL), 0); + } + if (IS_PSEUDO_FILTER(opR)) { + if (!IS_FILTER(opR)) { + // support this for backwards compatibility; see discussion on + // https://github.com/gap-system/gap/pull/2732 + Warning("operation '%g' used in AND-filter is not a filter", + (Int)NAME_FUNC(opR), 0); + } PushObj( NewAndFilter( opL, opR ) ); } else { ErrorQuit( - " must be 'true' or 'false' (not a %s)", + " must be a filter (not a %s)", (Int)TNAM_OBJ(opL), 0L ); } } @@ -1333,7 +1345,7 @@ void IntrAnd ( void ) /* signal an error */ else { ErrorQuit( - " must be 'true' or 'false' (not a %s)", + " must be 'true' or 'false' or a filter (not a %s)", (Int)TNAM_OBJ(opL), 0L ); } } diff --git a/src/opers.h b/src/opers.h index 123e1e3be1c..7b67334b6b9 100644 --- a/src/opers.h +++ b/src/opers.h @@ -247,6 +247,16 @@ static inline Int IS_FILTER(Obj oper) return v & OPER_IS_FILTER; } +// temporary HACK, until all affected packages are fixed +static inline Int IS_PSEUDO_FILTER(Obj oper) +{ + if (!IS_OPERATION(oper)) + return 0; + Obj flags = FLAGS_FILT(oper); + return flags && TNUM_OBJ(flags) == T_FLAGS; +} + + /**************************************************************************** ** *F SET_IS_FILTER( ) . . . . . . . . . . . mark operation as a filter diff --git a/tst/testinstall/boolean.tst b/tst/testinstall/boolean.tst index 8b9514451fd..caf921c037a 100644 --- a/tst/testinstall/boolean.tst +++ b/tst/testinstall/boolean.tst @@ -54,7 +54,17 @@ gap> ViewString(true); ViewString(false); ViewString(fail); gap> TNAM_OBJ(fail); "boolean or fail" -# test error handling +# crazy stuff that is accepted by interpreter and executor +gap> false and 1; +false +gap> true or 1; +true +gap> function() return false and 1; end(); +false +gap> function() return true or 1; end(); +true + +# test error handling in interpreter gap> not 1; Error, must be 'true' or 'false' (not a integer) gap> false or 1; @@ -64,17 +74,64 @@ Error, must be 'true' or 'false' (not a integer) gap> true and 1; Error, must be 'true' or 'false' (not a integer) gap> 1 and true; -Error, must be 'true' or 'false' (not a integer) +Error, must be 'true' or 'false' or a filter (not a integer) gap> ReturnTrue and ReturnTrue; -Error, must be 'true' or 'false' (not a function) +Error, must be 'true' or 'false' or a filter (not a function) gap> ReturnTrue and true; -Error, must be 'true' or 'false' (not a function) +Error, must be 'true' or 'false' or a filter (not a function) gap> IsAssociative and ReturnTrue; -Error, must be 'true' or 'false' (not a function) +Error, must be a filter (not a function) gap> IsAssociative and true; -Error, must be 'true' or 'false' (not a function) +Error, must be a filter (not a function) +gap> IsAssociative and Center; +Warning, operation 'Centre' used in AND-filter is not a filter in stream:1 + +gap> IsAssociative and FirstOp; +Error, must be a filter (not a function) gap> true and IsAssociative; Error, must be 'true' or 'false' (not a function) +gap> Center and IsAssociative; +Warning, operation 'Centre' used in AND-filter is not a filter + in stream:1 + +gap> FirstOp and IsAssociative; +Error, must be 'true' or 'false' or a filter (not a function) +gap> IsAssociative and IsAssociative; + + +# test error handling in executor +gap> function() return not 1; end(); +Error, must be 'true' or 'false' (not a integer) +gap> function() return false or 1; end(); +Error, must be 'true' or 'false' (not a integer) +gap> function() return 1 or false; end(); +Error, must be 'true' or 'false' (not a integer) +gap> function() return true and 1; end(); +Error, must be 'true' or 'false' (not a integer) +gap> function() return 1 and true; end(); +Error, must be 'true' or 'false' or a filter (not a integer) +gap> function() return ReturnTrue and ReturnTrue; end(); +Error, must be 'true' or 'false' or a filter (not a function) +gap> function() return ReturnTrue and true; end(); +Error, must be 'true' or 'false' or a filter (not a function) +gap> function() return IsAssociative and ReturnTrue; end(); +Error, must be a filter (not a function) +gap> function() return IsAssociative and true; end(); +Error, must be a filter (not a function) +gap> function() return IsAssociative and Center; end(); +Warning, operation 'Centre' used in AND-filter is not a filter in stream:1 + +gap> function() return IsAssociative and FirstOp; end(); +Error, must be a filter (not a function) +gap> function() return true and IsAssociative; end(); +Error, must be 'true' or 'false' (not a function) +gap> function() return Center and IsAssociative; end(); +Warning, operation 'Centre' used in AND-filter is not a filter in stream:1 + +gap> function() return FirstOp and IsAssociative; end(); +Error, must be 'true' or 'false' or a filter (not a function) +gap> function() return IsAssociative and IsAssociative; end(); + # gap> STOP_TEST( "boolean.tst", 1);