`ErrorQuit` vs `ErrorMayQuit` in `error.c`

[markuspf]

In the GAP master branch the following happens

gap> list := [];
[  ]
gap> list{ [fail] };
Error, List Elements: <positions> must be a dense list of positive integers
not in any function at *stdin*:3
gap> 

So, we don't get a break loop. This turns out to be a bit annoying when debugging code, because if one ends up in a situation where an index list contains invalid entries, one cannot inspect it anymore.

An easy solution to this is to replace ErrorQuit in error.c by ErrorMayQuit, but there might be a deeper reason why this was not used.

[ChrisJefferson]

Unless @fingolfin Knows if some reason not to use ErrorMayQuit, to do with parsing possibly? (I don't expect one, but I'd check) I would personally prefer we always use ErrorMayQuit instead of ErrorQuit.

[fingolfin]

This seems like a rather drastic change. It also would affects lots packages.

That said, I agree in so far as in many places, it is unclear why and when ErrorQuit is used, and when ErrorMayQuit. And I guess that the majority if not all could be changed to call ErrorMayQuit.

But in order to not risk randomly breaking stuff, including stuff that is not even under our direct control, I think it would make much more sense to convert calls to ErrorQuit to instead use ErrorMayQuit one by one (possibly a whole bunch at a time, but after having briefly looked at them, not just a global search and replace.). Then ideally also make sure there are tests which trigger these calls; and then move on to the next. That's slower and more work, but far less likely to deeply break stuff.

Or if whoever does it volunteers for taking care of any regressions that are caused by it, by all means do this globals search and replace in the GAP kernel.

However, what I strongly object to is to change the behaviour of ErrorQuit.

[ChrisJefferson]

Sorry, to be clear, I don't suggest we make ErrorQuit call ErrorMayQuit. However, we should/could try to use ErrorMayQuit wherever reasonable, unless there is some consistent rule for not using it (I have a memory that I tried using it in some places in the parser and things went badly wrong, as the parser didn't like entering a break loop in certain situations).

[stevelinton]

In this case, it just seems like this specific error never enters the break loop, even if it happens inside a function, which just seems wrong to me. There isn't much point in calling ErrorMayQuit from immediately interpreted code, except perhaps consistency for the user, because there can't be any local variables to inspect.

[fingolfin]

OK, after reading #3011, and re-reading this, I realize that I may have completely misunderstood this report the first time around :-(. I thought that the proposal was to completely replace ErrorQuit by ErrorMayQuit; but perhaps the suggestion was to only replace it inside CheckIsPossList? Anyway, this is now done by PR #3013. Question is whether @markuspf would then consider this issue resolved; or whether we should keep it after all, to track that we want to get rid of most (but not quite all!) calls to ErrorReturn{Obj,Void} in the kernel. (Though I don't think we strictlyneed an issue for that, do we?)

[markuspf]

My suggestion was indeed to only replace ErrorQuit by ErrorMayQuit in CheckIsPossList (or, maybe other places where we want a break loop to inspect state)

[fingolfin]

Fixed by PR #3013