Bug in break loop backtrace

[fingolfin]

The following happens in the latest master branch. Consider this test code:

test := function(a)
    Print("");
    Print(a("bla"), ",\n");
end;
test(1);

Entering it results in this incorrect error message:

Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `CallFuncList' on 2 arguments at /foobar/lib/methsel2.g:241 called from
Print( "" ); at *stdin*:6 called from
<function "test">( <arguments> )
 called from read-eval loop at *stdin*:9

If one removes the first Print statement, one instead gets this:

Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `CallFuncList' on 2 arguments at /foobar/lib/methsel2.g:241 called from
<compiled or corrupted statement>  called from
<function "test">( <arguments> )
 called from read-eval loop at *stdin*:12

Indeed, this is an even shorter variant:

gap> a:=1;a("bla");
1
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `CallFuncList' on 2 arguments at /foobar/lib/methsel2.g:241 called from
<function "HANDLE_METHOD_NOT_FOUND">( <arguments> )
 called from read-eval loop at *stdin*:30

I'd rather expect an error like "Error, object is an integer, not a function or method". Also, the backtrace should reference the correct line, and not lead to "corrupt statement" messages...

[ChrisJefferson]

Just to say, while I agree these are all problems, they are long-standing problems. This is generally an area which needs a good cleanup.

[ChrisJefferson]

I've added a test framework in #918 which can be used to at least test this kind of thing (normal tests can't check all the information).

[olexandr-konovalov]

@ChrisJefferson thanks - I will look at running this test in Jenkins with some regularity.

[fingolfin]

The problem is that GAP in principle allows providing custom "function objects", as demonstrated in tst/testinstall/callfunc.tst. That explains the perhaps strange error.

But that also means we might be able to fix it by installing a generic CallFuncList method with minimal rank, which shows a better error message.

[gwhitney]

I just want to add that if the statements before the bad function call are more elaborate, e.g.,

mislead := function(list)
  local n, i;
  n := Length(list);
  if n < 1 then
    Error("Zero length list");
  elif n < 2 then
    return list[1];
  fi;
  for i in [2..n] do
    if IsInt(list(i)) then
      return Sum(list);
    fi;
  od;
  return 0;
end;

then the distance between the reported troublesome line and the actual typo can be reasonably significant. The above is a nonsense cooked-up example adapted from some code I was writing. I mistyped () instead of [] in the list access on line 10, but GAP told me there was an error in "n<2" on line 6, and then when I commented out the "elif" clause the error mysteriously moved up to "n<1" in line 4. But in the break loop, the expressions n<1 and n<2 all evaluated fine (of course). In retrospect, the actual problem is a standard sort of typo that I should have been able to see right away, so I feel a bit foolish, but at the time, the messages were diverting my attention from the correct place to look in the code, and I ate several hours varying my syntax in the first "if" statement, etc,, finally finding the issue by a binary search of commenting out sections of code.

I totally do not intend this as whining -- GAP is a wonderful system -- I am commenting only for two reasons. (1) to support the point that this is a substantive usability issue that can create a drain on would-be GAP programmers, and (2) having been bitten by this, I am quite motivated to lend a hand if it would be helpful in curing the problem. (If there had not been an open issue I would have opened one and dived right into debugging it.) It seems as though @ChrisJefferson is/was the most active on this, so Chris, please let me know if there's any way I can be useful; or if nobody is really active on it, anyone who feels it's worthwhile for me to dive in, let me know. The aspect I am most motivated to attack is the mislocalization of the error in the backtrace, as opposed to what sort of error is generated or whether this particular error could have been averted by another CallFuncList method, which seem like the main thrust of @fingolfin's comment just above... I imagine if this particular error is being mislocalized, there are probably other possible errors that may report the wrong source line.

[fingolfin]

@gwhitney Thanks for the feedback (which did not at any point seem like "whining" to me).

I actually think I know what the issue is, and how to fix it (this will be byproduct of the lvars / CurrState / BRK_CALL_TO cleanup I worked on a month or so ago; it's just been slow-going as I keep running into other quirks that take time to cleanup properly (this then e.g. lead to other fixes, e.g. for UpEnv / DownEnv).

Very roughly speaking, the problem is that we have to places to track the "current statement". One goes via SET_BRK_CALL_TO, and that is not called in enough places (in fact, @ChrisJefferson fixed a similar bug some time ago by inserting a call to SET_BRK_CALL_TO into EvalElmList -- however, I believe that's not the "right" fix, it addressed that symptom in that particular cas, but not the general problem.

A simple way to fix this more thoroughly would be to call SET_BRK_CALL_TO in EXEC_STAT resp. EVAL_EXPR.

However, things are still more complicated. For example, for some things we really want the last executed statement (not expressions), for others the last statement or expression. The whole code is (was?) quite a mess, but I've been slowly grinding away at it, and things are IMHO quite a bit clearer now. If somebody wants to experiment with updating the "last statement" in EXEC_STAT, be my guest (otherwise, I plan to get back to it eventually)

[fingolfin]

(actually, I think it's SET_BRK_CURR_STAT you'd want to call in EXEC_STAT... but it's been some time I last looked at this, and in any case, this is part of what still needs untangling)

[ChrisJefferson]

Yes, I also looked at this and got lost in the weeds. The main problem is to decide what we want to produce in which situations.

[gwhitney]

@fingolfin I do not doubt that globally it might improve matters to add a SET_BRK_CURR_STAT to EXEC_STAT (and presumably then remove various (most? all?) other SET_BRK_CURR_STATs which would become redundant in such a case). However, after rattling around in the code a while last night, it turned out the the necessary SET_BRK_... calls for this issue are already in the code (they happen to be SET_BRK_CALL_TOs), they were just misordered with respect to the DispatchFuncCall calls that were generating the errors. So I concluded that it would not interfere with your ongoing overall cleanup/restructuring, and would improve current matters, to simply swap the SET_BRK_CALL_TO( )s and the DispatchFuncCall()s in funcs.c.

Thanks so much for your pointers, and thank goodness that the order of these two operations was more helpful in one of the 14 cases in the existing code, otherwise I might not have managed to track this down. Hope the pull request #1814 is good and that it saves someone some debugging time...