Move SET_BRK_CALL_TO()s in funcs.c to resolve issue #917

[gwhitney]

Consider the following interaction with a freshly-compiled gap from master; it is essentially the same phenomenon as the first example in #917, albeit with the offending bit of code as a procedure call rather than a function:

 ┌───────┐   GAP 4.8.8-4439-gbeeebb0 of today
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-pc-linux-gnu-gcc-default64
 Configuration:  gmp 6.1.2, readline
 Loading the library and packages ...
 Packages:   GAPDoc 1.6, PrimGrp 3.1.2, smallgrp 1.2, transgrp 2.0
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'
gap> mislead := function(l)
> Add(l,1);
> l(1);
> return; end;
function( l ) ... end
gap> mislead([]);
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `CallFuncList' on 2 arguments at /home/gwhitney/code/dev/gap/lib/methsel2.g:241 called from
Add( l, 1 ); at *stdin*:2 called from
<function "mislead">( <arguments> )
 called from read-eval loop at *stdin*:5
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue
brk> quit;
gap> quit;

I included the opening banner so that you can easily count the lines in stdin and see that the backtrace is misreporting the expression in which the error (namely that there is no method for the function-call syntax with the function object being a list) occurs and the line on which it occurred. But now contrast that session with the following one:

 ┌───────┐   GAP 4.8.8-4439-gbeeebb0 of today
 │  GAP  │   https://www.gap-system.org
 └───────┘   Architecture: x86_64-pc-linux-gnu-gcc-default64
 Configuration:  gmp 6.1.2, readline
 Loading the library and packages ...
 Packages:   GAPDoc 1.6, PrimGrp 3.1.2, smallgrp 1.2, transgrp 2.0
 Try '??help' for help. See also '?copyright', '?cite' and '?authors'
gap> inform := function(l)
> Add(l,1);
> l();
> return; end;
function( l ) ... end
gap> inform([]);
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `CallFuncList' on 2 arguments at /home/gwhitney/code/dev/gap/lib/methsel2.g:241 called from
l(  ); at *stdin*:3 called from
<function "inform">( <arguments> )
 called from read-eval loop at *stdin*:5
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue
brk> quit;
gap> quit;

Now the error message is corretly showing that the offending code was at line 3, and it lies in "l( )" (there's still no method for the function call of a list object). The difference might be mysterious until you look at code in funcs.c and compare ExecProccall0args, in which SET_BRK_CALL_TO() occurs before the DispatchFuncCall, with ExecProccall1args, in which SET_BRK_CALL_TO() occurs after the DispatchFuncCall. Examining the remaining ExecProc... and EvalFunc... functions suggests that similar bad behavior will occur in all of those corresponding cases, and sure enough, I have attached a transcript of the remaining 12 cases displaying misleading information.

It appears that when the code to allow arbitrary objects to implement the function call syntax was added, in almost all (but thankfully not all, otherwise I would have been very unlikely to find this) cases the DispatchFuncCall was added "too early" in the various ExecProc... and EvalFunc... functions. This pull request moves the SET_BRK_CALL_TO()s uniformly before the DispatchFuncCall, and the other attachment shows a transcript of all 14 cases suppling more informative messages.

I will update the pull request with a test based on the latter transcript as soon as I figure out how the extended testing for the full backtrace added in pull request #918 works.
issue_917_examples.txt

issue_917_corrected.txt

Resolves #917.

[fingolfin]

Seems good to me, though (as you acknowledge already) tests should be added; i.e. add new files to tst/test-error/, say based on your two original examples, and the corresponding .out files (created with the regenerate_error_tests.sh), which demonstrate the new fixed output.

[fingolfin]

This comment change is not mentioned in the commit message. That should be done, or else, a separate commit be added which performs this change

That said, the ExecProccall*args functions really always return 0, period. So both the new and the old comment are misleading.

[codecov]

Codecov Report

Merging #1814 into master will increase coverage by 12.6%.
The diff coverage is 80%.

@@            Coverage Diff             @@
##           master    #1814      +/-   ##
==========================================
+ Coverage   50.44%   63.05%   +12.6%     
==========================================
  Files         448      968     +520     
  Lines      234445   292773   +58328     
  Branches    10446    12924    +2478     
==========================================
+ Hits       118268   184608   +66340     
+ Misses     113385   105366    -8019     
- Partials     2792     2799       +7

Impacted Files	Coverage Δ
src/funcs.c	74.75% <80%> (-12.82%)	⬇️
src/intobj.h	81.48% <0%> (-7.71%)	⬇️
src/dteval.c	2.79% <0%> (-0.29%)	⬇️
src/dt.c	1.61% <0%> (-0.26%)	⬇️
hpcgap/pkg/gapdoc/lib/GAPDoc2LaTeX.gi	1.5% <0%> (-0.12%)	⬇️
hpcgap/pkg/gapdoc/lib/GAPDoc2Text.gi	0.54% <0%> (-0.05%)	⬇️
src/range.h	100% <0%> (ø)	⬆️
src/objfgelm.h	100% <0%> (ø)	⬆️
src/code.h	100% <0%> (ø)	⬆️
... and 838 more

[gwhitney]

OK, added test and fixed comment (sorry I couldn't tell whether that ReadEvalError() might do some sort of non-local exit, so I hedged a bit in the comment). Hope all is well now.

[fingolfin]

Well, of course the function could "exit" non-locally via a "thrown exception" (if this was C++ code; we do something similar using setjmp). But in that case, the function won't return at all, rather the execution continues in a completely different places; so the function still won't "return" anything directly.

[gwhitney]

Good point on the terminology. I suppose a macro could have literally returned a different value, but the camel case indicated quite strongly that ReadEvalError() was not a macro. I guess I just get antsy whenever I see "error handling" functions, sorry. Thanks for moving these changes along.

[gwhitney]

I see the Travis CI failed in the test "testerror" -- which seems odd because there is no testerror.g in the gap source code tree... Let me know if there's anything I can do to lend a hand.

[ChrisJefferson]

You can run testerror from the gap directory as follows:

cd tst/test-error
./run_error_tests.sh
cd ../test-compile
./run_compile_tests.sh

These tests aren't run by a GAP file, because they involve running lots of GAPs. I imagine you are hitting the test-error tests, which are exactly

[ChrisJefferson]

Sorry, went to have a look, something weird is happening. Will investigate.

[ChrisJefferson]

Ah, I see what's happened. We've renamed testerror to testspecial, and travis is being stupid.

If you rebase your commit, it should test fine. Not sure how git literate you are, but basically do a rebase. On my machine I do the following (which hopefully won't produe any errors).

git fetch --all
git rebase upstream/master
git push --force

[gwhitney]

Did I succeed? Sadly I am not that experienced with git. So I just tried your commands, and the last one failed with "fatal: The current branch fix/master/issue_917 has no upstream branch." So I messed around a bit, trying to follow the clues git was giving me, and couldn't get it to work unless I did a "git pull origin fix/master/issue_917". Then the push worked; hope it was all OK; I didn't really understand the commands I was issuing.

[ChrisJefferson]

Sometimes git just goes crazy. I have made a new commit, #1821, which is (I hope!) just a cleaned up version of your commit. If you could just check I didn't break it (I'm fairly sure I didn't, I diffed the two versions. I did merge the 3rd commit into the 1st.

[fingolfin]

@ChrisJefferson in git advice, it's generally not a good idea to assume that upstream means what it does on your system. I'd recommend to either explain to the user how to determine the right remote name, or else specify the remote via a URL.

[ChrisJefferson]

You are right, I gave bad advice. I suggest we move over to #1821 (where I've hopefully cleaned things up)

[gwhitney]

Sorry I wasn't experienced enough with git to know properly how to do what @ChrisJefferson was asking me to. I will switch over to pull request #1821; this one can be closed (should I do that?)

[ChrisJefferson]

We can close by just clicking "close and comment" at the bottom. I just left it open to check you were happy, and didn't get confused by the PR just disappearing.

Don't worry, git can behave strangely, and it's sometimes hard to debug remotely (to be honest, I find it hard to debug when I'm in front of a computer using it sometimes..)

[gwhitney]

Reopening as per @fingolfin's suggestion in #1821

[gwhitney]

The diffs look as expected, so I think/hope I have executed the instructions from @fingolfin in #1821 for fixing this pull request correctly. I will now check in the further changes he requested and tests for them.

[gwhitney]

Ok, and now I have made the changes suggested by @fingolfin in his code review in #1821, along with tests for them. This was a good catch, because all of the new test cases segfault without the change. Thanks!

[fingolfin]

Actually, I don't think there is need to test the full backtrace for the second fix. So the test can live in a plain.tst file - I'd just add it to ˋtst/testinstall/callfunc.tstˋ

[gwhitney]

Right, that's what I did, except I put it in testbugfix (which doesn't test the full backtrace like test-error does), since that's where it was suggested that I put the test for my recent fix to MagmaWithOne and MagmaWithInverses with two arguments.Should I move the test for this part to testinstall?

[fingolfin]

Calling into non-function objects was mostly undocumented, unknown, and buggy in all GAP version up to and including GAP 4.8.x. So 4.9 will be the first were this is usable. Hence, I wouldn't put the test under "bugfix", but rather into the normal testsuite. Indeed, I'd always prefer to put tests there; I only place tests into testbugfix if they are one-off shots, i.e. if they only test one isolated special case, in a non-systematic fashion. Once there is a systematic tests for that area, the testbugfix one can be removed.

I'd also keep the tests as simple, short and self-contained as possible. For that reason, I really would advice against using DirectProductElement in it, as that's a high-level object, and by using it, the tests now also implicitly relies on its behavior.

Hence, the test should go into tst/testinstall/callfunc.tst, were we already test calls into non-function objects. E.g. like this, added to the end:

# test overloading CallFuncList with a procedure call
gap> cat2 := NewCategory("IsXYZ2",IsObject);;
gap> type2 := NewType(fam, cat2 and IsPositionalObjectRep);;
gap> InstallMethod(CallFuncList,[cat2,IsList],function(func,args) result:=args; end);
gap> o2 := Objectify(type2,[]);;

# test edge case: expecting a func call, but doing a proc call
gap> f := function() return o2(); end;; f();
Error, Function Calls: <func> must return a value
gap> f := function() return o2(1); end;; f();
Error, Function Calls: <func> must return a value
gap> f := function() return o2(1,2); end;; f();
Error, Function Calls: <func> must return a value
gap> f := function() return o2(1,2,3); end;; f();
Error, Function Calls: <func> must return a value
gap> f := function() return o2(1,2,3,4); end;; f();
Error, Function Calls: <func> must return a value
gap> f := function() return o2(1,2,3,4,5); end;; f();
Error, Function Calls: <func> must return a value
gap> f := function() return o2(1,2,3,4,5,6); end;; f();
Error, Function Calls: <func> must return a value
gap> f := function() return o2(1,2,3,4,5,6,7); end;; f();
Error, Function Calls: <func> must return a value

[fingolfin]

Actually, why the return;? It is redundant, isn't it?

Perhaps also add a second semicolon after end;, to suppress printing the function, to reduce clutter in the .out file.

[fingolfin]

Perhaps insert Print("\n\n"); after each quit;, to make the output a bit more readable?

[gwhitney]

OK, I think I have covered all of your requests for changes. Thanks for the feedback.

[fingolfin]

@gwhitney Thanks. In principle, this could be merged now, but if you want to aim for "perfection", then you could still slightly improve the commit structure and messages. For example, the second commit has the commit message "Add a test to tst/test-error verifying behavior in all cases changed" which doesn't make much sense if read in isolation -- and it'll be read in isolation at times. So it's better to make that self-contained, or at the very least make clear it refers to, e.g. by saying "... in all cases changed by the previous commit.

However, in this case, I actually think it would make more sense to squash the first two commits together, and also the last three.

If you would like to try that, you can do it with an "interactive rebase". And if anything goes wrong, you can abort this operation (I'll tell you how).

So, you do this: checkout your branch. Then enter the command git rebase -i master to start the interactive rebase. This will open a file in your default editor (usually the same editor git commit opens; the default probably is vi).
In that file is a list of commits (it should show 5, namely the 5 in this PR), ordered linearly by parentage. Each commit is prefixed by a command, initially "pick"; at the bottom of the file will be a list of all possible commands. The one you'll want is "squash", which can be abbreviated to just "s".

So, you'd change the word "pick" in the second line to "s" (indicating to git that you want to squash the second commit into the second), and also in lines 4 and 5 (so that commits 4 and 5 get squashed into commit 3).

Once you changed the commands like that, you can save the file and exit the editor, at which point git starts to work. It'll open the editor two more times, to let you edit the commit message for the two resulting commits. The default will be the concatenation of the commit messages of the commits being squashed together, but you can of course freely edit those.

[gwhitney]

OK, that all worked on my laptop, but (sorry to be a dope) when I then naively tried git push origin fix/master/issue_917 I got the following output:

To https://github.com/gwhitney/gap.git
 ! [rejected]            fix/master/issue_917 -> fix/master/issue_917 (non-fast-forward)
error: failed to push some refs to 'https://github.com/gwhitney/gap.git'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Integrate the remote changes (e.g.
hint: 'git pull ...') before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

How do I transmit the revised PR back to the mother ship? Thanks!

[fingolfin]

Sorry, forgot the last step: you need to pass the option --force to push

[gwhitney]

Should have guessed. But I am always slightly frightened of using excessive force, I figure there's more chance to break things... ;-)

[fingolfin]

Thanks. However, now there is a new annoyance: the second commit contains some changes/fixes that belong into the first commit, namely tweaks to tst/test-error/func-and-proc-call-trace.g and tst/test-error/func-and-proc-call-trace.g.out.

If you are still with me and not fed up by me annoying you with constant change requests, I'll describe how to fix this below. However, if you rather would not, just let me know, and I'll take care of it (I am simply telling these things to you in case you'd like to learn; it is not my intention to "punish" you for being helpful to us!!!)

Anyway, here is what you could do:

Change into your clone of the GAP repo, and to your branch. Now enter the command git reset HEAD^ tst/test-error/func-and-proc-call-trace.g*.
This causes changes which undo the changes to those tst/test-error to be put into the staging area, while the files in the work dir themselves remain unchanged (i.e. containing the changes).

So now do git commit --amend; this will display the commit message of the second commit; just save it. The result will be that the changes to tst/test-error/func-and-proc-call-trace.g* are purged from the second commit. But we still wanted them! Luckily the changed file are still in the work dir. To get them into the first commit, you can use an interactive rebase again: First we commit the changes: git commit -m fixup tst.
Now you can git rebase -i master, which this time should show you a list of three commits. We want to squash the third commit into the first. To do that, you need to swap the second and third line. I.e. the line with the commit "fixup" should be the new second line. Once that is done, also change the "pick" in that line to "f" for "fixup" (which is just like "squash", except it keeps the commit message of the first commit as-is). Now save&exit.

Git should now have transferred the changes to the first commit... And you can git push -f once again.

(If anything during this fails: no worries, git keeps backups of everything, and we can revert to those.

[gwhitney]

Kay, as requested. Impressed with your patience for enforcing crispness at the individual-commit level, as opposed to the level of merges into the repository master (which obviously have to be clean and focused). I guess if I ever embark on any serious project on the GAP codebase I will have to replay all of my changes and shuffle them into neat units before submitting a pull request, since in my experience actual development never really follows such an orderly step-by-step process as the final pair of commits in this PR would represent.

Anyhow, how about as a reward for my being a good student of git ;-), you take a quick look at the pull request to endow DirectProductElements with a working String() method (I won't put the PR number here since it isn't really a contentful cross reference)? The fact that String() is choking on DPEs is blocking me from working on my actual project (working with the authors of LOOPS and some other researchers on a better-integrated rack/quandle package that could get in shape for refereeing) on top of master.

[fingolfin]

First off, thanks for updating the commits, they are fine now :-). I also commented on the other PR.

As to your other remarks: Enforcing "crispness" on the commit level pays off on the long run, namely when you need to debug a regression via git bisect and then actually understand what was going on back then, and are able to compile, run and test (almost) every commit.

Many companies sadly fail to do this well, claiming there is no time to do things "properly" -- in my personal experience (e.g. woking as a programming freelancer during my studies, and talking about this with friends who work in IT), this very often comes with severe penalties as the projects "mature", and it's doubtful whether the time saved by rushing things vs. the time lost due to not being able to navigate the mess, is really worth it ... shrug.

Anyway, git itself is developed using these standards, and it seems to be doing quite well with it :-).

The reason I spend time trying to teach people this is selfish: I hope they'll learn to "do it properly" next time, thus saving time for everybody...

Finally, of course I don't write new code like that, I make crazy commits with horrible commit messages etc., and then once a feature is complete, I go through the mess once and sort it into something that makes sense. With some practice and routine, that doesn't waste much time, but pays of (in my experience, at least) later, as describe above.