Skip to content

Commit

Permalink
kernel: unify 'and' for filters in interpreter and executor
Browse files Browse the repository at this point in the history 
Also start using IS_FILTER. Note that previously, this input was silently accepted:

  gap> Center and IsAssociative;
  <Filter "<<and-filter>>">

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.
  • Loading branch information
@fingolfin
fingolfin committed Aug 28, 2018
1 parent 163e622 commit 2b48e81
Show file tree
Hide file tree
Showing 4 changed files with 105 additions and 14 deletions.
20 changes: 16 additions & 4 deletions src/exprs.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,23 +183,35 @@ 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(
"<expr> must be 'true' or 'false' (not a %s)",
"<expr> must be a filter (not a %s)",
(Int)TNAM_OBJ(opL), 0L );
}
}

/* signal an error */
else {
ErrorQuit(
"<expr> must be 'true' or 'false' (not a %s)",
"<expr> must be 'true' or 'false' or a filter (not a %s)",
(Int)TNAM_OBJ(opL), 0L );
}

Expand Down
20 changes: 16 additions & 4 deletions src/intrprtr.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1319,21 +1319,33 @@ 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(
"<expr> must be 'true' or 'false' (not a %s)",
"<expr> must be a filter (not a %s)",
(Int)TNAM_OBJ(opL), 0L );
}
}

/* signal an error */
else {
ErrorQuit(
"<expr> must be 'true' or 'false' (not a %s)",
"<expr> must be 'true' or 'false' or a filter (not a %s)",
(Int)TNAM_OBJ(opL), 0L );
}
}
Expand Down
10 changes: 10 additions & 0 deletions src/opers.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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( <oper> ) . . . . . . . . . . . mark operation as a filter
Expand Down
69 changes: 63 additions & 6 deletions tst/testinstall/boolean.tst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, <expr> must be 'true' or 'false' (not a integer)
gap> false or 1;
Expand All @@ -64,17 +74,64 @@ Error, <expr> must be 'true' or 'false' (not a integer)
gap> true and 1;
Error, <expr> must be 'true' or 'false' (not a integer)
gap> 1 and true;
Error, <expr> must be 'true' or 'false' (not a integer)
Error, <expr> must be 'true' or 'false' or a filter (not a integer)
gap> ReturnTrue and ReturnTrue;
Error, <expr> must be 'true' or 'false' (not a function)
Error, <expr> must be 'true' or 'false' or a filter (not a function)
gap> ReturnTrue and true;
Error, <expr> must be 'true' or 'false' (not a function)
Error, <expr> must be 'true' or 'false' or a filter (not a function)
gap> IsAssociative and ReturnTrue;
Error, <expr> must be 'true' or 'false' (not a function)
Error, <expr> must be a filter (not a function)
gap> IsAssociative and true;
Error, <expr> must be 'true' or 'false' (not a function)
Error, <expr> 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
<Filter "(IsAssociative and Centre)">
gap> IsAssociative and FirstOp;
Error, <expr> must be a filter (not a function)
gap> true and IsAssociative;
Error, <expr> 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
<Filter "(Centre and IsAssociative)">
gap> FirstOp and IsAssociative;
Error, <expr> must be 'true' or 'false' or a filter (not a function)
gap> IsAssociative and IsAssociative;
<Property "IsAssociative">

# test error handling in executor
gap> function() return not 1; end();
Error, <expr> must be 'true' or 'false' (not a integer)
gap> function() return false or 1; end();
Error, <expr> must be 'true' or 'false' (not a integer)
gap> function() return 1 or false; end();
Error, <expr> must be 'true' or 'false' (not a integer)
gap> function() return true and 1; end();
Error, <expr> must be 'true' or 'false' (not a integer)
gap> function() return 1 and true; end();
Error, <expr> must be 'true' or 'false' or a filter (not a integer)
gap> function() return ReturnTrue and ReturnTrue; end();
Error, <expr> must be 'true' or 'false' or a filter (not a function)
gap> function() return ReturnTrue and true; end();
Error, <expr> must be 'true' or 'false' or a filter (not a function)
gap> function() return IsAssociative and ReturnTrue; end();
Error, <expr> must be a filter (not a function)
gap> function() return IsAssociative and true; end();
Error, <expr> 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
<Filter "(IsAssociative and Centre)">
gap> function() return IsAssociative and FirstOp; end();
Error, <expr> must be a filter (not a function)
gap> function() return true and IsAssociative; end();
Error, <expr> 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
<Filter "(Centre and IsAssociative)">
gap> function() return FirstOp and IsAssociative; end();
Error, <expr> must be 'true' or 'false' or a filter (not a function)
gap> function() return IsAssociative and IsAssociative; end();
<Property "IsAssociative">

#
gap> STOP_TEST( "boolean.tst", 1);
Expand Down

0 comments on commit 2b48e81

Please sign in to comment.