Uninterruptible loop

[fingolfin]

This code snippet cannot be interrupted with ctrl-c:

repeat continue; until false;

This is present in master but also in GAP 4.8 and older. The problem also affects for loops and while loops.

One way to fix this would be to move T_CONTINUE out of the FIRST_NON_INTERRUPT_STAT .. LAST_NON_INTERRUPT_STAT range. Another would be to leave it in there, but add additional interrupt checks to all loop constructs. If we do that, we could then also move T_EMPTY into the range of non-interruptible stats.

[ChrisJefferson]

Why would we want t_empty to be non-interuptable? I would think we'd want as much to be interupttable as reasonable?

[fingolfin]

Well, T_EMPTY is a special case of T_SEQ_STAT, T_SEQ_STAT2, T_SEQ_STAT3, ... (i.e., it i T_SEQ_STAT0, if you will). My personal explanation for why we do not interrupt these (I could not find any documentation, not even in a commit message, explaining it), as well as other compound statements like loops and if/than/else statements is: because that leads to poor user experience in the break loop. To illustrate what I mean, first consider the example, where I interrupt a simple loop with a non-empty body:

gap> x:=0;; repeat x:=0; until false;
^CError, user interrupt in
  x := 0; at *stdin*:2 called from
<function "unknown">( <arguments> )
 called from read-eval loop at *stdin*:2
you can 'return;'
brk>

As you can see, in the break loop, we try to print the statement that was interrupted. Which makes a lot of sense for single statements like x:=0. In contrast, if we interrupt a T_SEQ_STAT, it wouldn't be to helpful to print all the statements in it. So instead, we do not interrupt it, and instead rely on the sub-statements to be interrupted. For the same reason, loops like T_REPEAT are not

Now consider what happens for a T_EMPTY statement:

gap> x:=0;; repeat until false;
^CError, user interrupt in
  ; at *stdin*:2 called from
<function "unknown">( <arguments> )
 called from read-eval loop at *stdin*:2
you can 'return;'
brk>

It prints out the empty statement, which is not very helpful. If instead the loop code handled the interrupt (and I don't mean making T_REPEAT etc. interruptible, I mean making it explicitly handle the interrupt in a "clever" way), then it could print something more intelligent.

The reason I don't like making T_REPEAT etc. interruptible via the usual mechanism is because then it'd print the full loop as the place the the user interrupt occurred in. And for most loops (i.e. those with non-empty body) it would do that only sometimes, depending on when exactly you press ctrl-c, which I think is quite counter-intuitive to the user.

All of that said, for the bug at hand, I think the quickest solution would be to just move all of

            START_ENUM_RANGE(FIRST_CONTROL_FLOW_STAT),

            T_BREAK,
            T_CONTINUE,
            T_RETURN_OBJ,
            T_RETURN_VOID,

            END_ENUM_RANGE(LAST_CONTROL_FLOW_STAT),

out of the FIRST_NON_INTERRUPT_STAT .. LAST_NON_INTERRUPT_STAT range. Each of them can be sensibly interrupted. With that change, the range of non-interruptible statements effectively is equal to that of compound statements.

[stevelinton]

This looks correct to me. I think you do want any interrupt to be associated with a statement which is actually present in the source code, rather than a filler or compounds of such. None of those statements should ever take significant time, so the only reason you might need to interrupt one of them is if you have a loop or recursion with no other interruptible statement in it, but that can basically never be useful, so let's avoid doing it.