Skip to content

Commit a8008b3

Browse files
fingolfinfingolfin
committed
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; <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.
1 parent 163e622 commit a8008b3

4 files changed

Lines changed: 100 additions & 14 deletions

File tree

‎src/exprs.c‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -183,23 +183,35 @@ Obj EvalAnd (
183183
}
184184

185185
/* handle the 'and' of two filters */
186-
else if ( TNUM_OBJ(opL) == T_FUNCTION ) {
186+
else if (IS_PSEUDO_FILTER(opL)) {
187+
if (!IS_FILTER(opL)) {
188+
// support this for backwards compatibility; see discussion on
189+
// https://github.com/gap-system/gap/pull/2732
190+
Warning("operation '%g' used in AND-filter is not a filter",
191+
(Int)NAME_FUNC(opL), 0);
192+
}
187193
tmp = READ_EXPR(expr, 1);
188194
opR = EVAL_EXPR( tmp );
189-
if ( TNUM_OBJ(opR) == T_FUNCTION ) {
195+
if (IS_PSEUDO_FILTER(opR)) {
196+
if (!IS_FILTER(opR)) {
197+
// support this for backwards compatibility; see discussion on
198+
// https://github.com/gap-system/gap/pull/2732
199+
Warning("operation '%g' used in AND-filter is not a filter",
200+
(Int)NAME_FUNC(opR), 0);
201+
}
190202
return NewAndFilter( opL, opR );
191203
}
192204
else {
193205
ErrorQuit(
194-
"<expr> must be 'true' or 'false' (not a %s)",
206+
"<expr> must be a filter (not a %s)",
195207
(Int)TNAM_OBJ(opL), 0L );
196208
}
197209
}
198210

199211
/* signal an error */
200212
else {
201213
ErrorQuit(
202-
"<expr> must be 'true' or 'false' (not a %s)",
214+
"<expr> must be 'true' or 'false' or a filter (not a %s)",
203215
(Int)TNAM_OBJ(opL), 0L );
204216
}
205217

‎src/intrprtr.c‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1319,21 +1319,33 @@ void IntrAnd ( void )
13191319
}
13201320

13211321
/* handle the 'and' of two filters */
1322-
else if ( IS_OPERATION(opL) ) {
1323-
if ( IS_OPERATION(opR) ) {
1322+
else if (IS_PSEUDO_FILTER(opL)) {
1323+
if (!IS_FILTER(opL)) {
1324+
// support this for backwards compatibility; see discussion on
1325+
// https://github.com/gap-system/gap/pull/2732
1326+
Warning("operation '%g' used in AND-filter is not a filter\n",
1327+
(Int)NAME_FUNC(opL), 0);
1328+
}
1329+
if (IS_PSEUDO_FILTER(opR)) {
1330+
if (!IS_FILTER(opR)) {
1331+
// support this for backwards compatibility; see discussion on
1332+
// https://github.com/gap-system/gap/pull/2732
1333+
Warning("operation '%g' used in AND-filter is not a filter",
1334+
(Int)NAME_FUNC(opR), 0);
1335+
}
13241336
PushObj( NewAndFilter( opL, opR ) );
13251337
}
13261338
else {
13271339
ErrorQuit(
1328-
"<expr> must be 'true' or 'false' (not a %s)",
1340+
"<expr> must be a filter (not a %s)",
13291341
(Int)TNAM_OBJ(opL), 0L );
13301342
}
13311343
}
13321344

13331345
/* signal an error */
13341346
else {
13351347
ErrorQuit(
1336-
"<expr> must be 'true' or 'false' (not a %s)",
1348+
"<expr> must be 'true' or 'false' or a filter (not a %s)",
13371349
(Int)TNAM_OBJ(opL), 0L );
13381350
}
13391351
}

‎src/opers.h‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -247,6 +247,16 @@ static inline Int IS_FILTER(Obj oper)
247247
return v & OPER_IS_FILTER;
248248
}
249249

250+
// temporary HACK, until all affected packages are fixed
251+
static inline Int IS_PSEUDO_FILTER(Obj oper)
252+
{
253+
if (!IS_OPERATION(oper))
254+
return 0;
255+
Obj flags = FLAGS_FILT(oper);
256+
return flags && TNUM_OBJ(flags) == T_FLAGS;
257+
}
258+
259+
250260
/****************************************************************************
251261
**
252262
*F SET_IS_FILTER( <oper> ) . . . . . . . . . . . mark operation as a filter

‎tst/testinstall/boolean.tst‎

Lines changed: 58 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,17 @@ gap> ViewString(true); ViewString(false); ViewString(fail);
5454
gap> TNAM_OBJ(fail);
5555
"boolean or fail"
5656

57-
# test error handling
57+
# crazy stuff that is accepted by interpreter and executor
58+
gap> false and 1;
59+
false
60+
gap> true or 1;
61+
true
62+
gap> function() return false and 1; end();
63+
false
64+
gap> function() return true or 1; end();
65+
true
66+
67+
# test error handling in interpreter
5868
gap> not 1;
5969
Error, <expr> must be 'true' or 'false' (not a integer)
6070
gap> false or 1;
@@ -64,17 +74,59 @@ Error, <expr> must be 'true' or 'false' (not a integer)
6474
gap> true and 1;
6575
Error, <expr> must be 'true' or 'false' (not a integer)
6676
gap> 1 and true;
67-
Error, <expr> must be 'true' or 'false' (not a integer)
77+
Error, <expr> must be 'true' or 'false' or a filter (not a integer)
6878
gap> ReturnTrue and ReturnTrue;
69-
Error, <expr> must be 'true' or 'false' (not a function)
79+
Error, <expr> must be 'true' or 'false' or a filter (not a function)
7080
gap> ReturnTrue and true;
71-
Error, <expr> must be 'true' or 'false' (not a function)
81+
Error, <expr> must be 'true' or 'false' or a filter (not a function)
7282
gap> IsAssociative and ReturnTrue;
73-
Error, <expr> must be 'true' or 'false' (not a function)
83+
Error, <expr> must be a filter (not a function)
7484
gap> IsAssociative and true;
75-
Error, <expr> must be 'true' or 'false' (not a function)
85+
Error, <expr> must be a filter (not a function)
86+
gap> IsAssociative and Center;
87+
Error, <expr> must be a filter (not a function)
88+
gap> IsAssociative and FirstOp;
89+
Error, <expr> must be a filter (not a function)
7690
gap> true and IsAssociative;
7791
Error, <expr> must be 'true' or 'false' (not a function)
92+
gap> Center and IsAssociative;
93+
Error, <expr> must be 'true' or 'false' or a filter (not a function)
94+
gap> FirstOp and IsAssociative;
95+
Error, <expr> must be 'true' or 'false' or a filter (not a function)
96+
gap> IsAssociative and IsAssociative;
97+
<Property "IsAssociative">
98+
99+
# test error handling in executor
100+
gap> function() return not 1; end();
101+
Error, <expr> must be 'true' or 'false' (not a integer)
102+
gap> function() return false or 1; end();
103+
Error, <expr> must be 'true' or 'false' (not a integer)
104+
gap> function() return 1 or false; end();
105+
Error, <expr> must be 'true' or 'false' (not a integer)
106+
gap> function() return true and 1; end();
107+
Error, <expr> must be 'true' or 'false' (not a integer)
108+
gap> function() return 1 and true; end();
109+
Error, <expr> must be 'true' or 'false' or a filter (not a integer)
110+
gap> function() return ReturnTrue and ReturnTrue; end();
111+
Error, <expr> must be 'true' or 'false' or a filter (not a function)
112+
gap> function() return ReturnTrue and true; end();
113+
Error, <expr> must be 'true' or 'false' or a filter (not a function)
114+
gap> function() return IsAssociative and ReturnTrue; end();
115+
Error, <expr> must be a filter (not a function)
116+
gap> function() return IsAssociative and true; end();
117+
Error, <expr> must be a filter (not a function)
118+
gap> function() return IsAssociative and Center; end();
119+
Error, <expr> must be a filter (not a function)
120+
gap> function() return IsAssociative and FirstOp; end();
121+
Error, <expr> must be a filter (not a function)
122+
gap> function() return true and IsAssociative; end();
123+
Error, <expr> must be 'true' or 'false' (not a function)
124+
gap> function() return Center and IsAssociative; end();
125+
Error, <expr> must be 'true' or 'false' or a filter (not a function)
126+
gap> function() return FirstOp and IsAssociative; end();
127+
Error, <expr> must be 'true' or 'false' or a filter (not a function)
128+
gap> function() return IsAssociative and IsAssociative; end();
129+
<Property "IsAssociative">
78130

79131
#
80132
gap> STOP_TEST( "boolean.tst", 1);

0 commit comments

Comments
 (0)