Add simple check for functions containing both return; and return <obj> (DO NOT MERGE)

[fingolfin]

This is a rebased and tweaked version of PR #466. The main changes compared to that are:

• This PR also resolves most of the warnings (see also PR Change several functions to consistently return something, or nothing; other misc changes #1539, which contains just those changes and could be merged right away).
• It now detects TryNextMethod(), which internally is coded as return TRY_NEXT_METHOD, and does not treat it as the function returning an object; this fixes a false positive in the ViewObj method in lib/teaching.g (and probably elsewhere).
• it also modifies CodeReturnVoidWhichIsNotProfiled (which did not exist when (WIP) Add a simple check against functions containing both return; and return <obj>; #466 was created)

I found this useful, as it flagged several places in GAP and packages where I genuinely think it was a bug that a function sometimes returns a value and sometimes nothing.

However, even with all the work combined, one false (?) positive remains: ErrorInner in lib/error.g is meant to sometimes return something or not, intentionally. However, I am not sure anything ever uses its return value? There are a few return Error(...) in the GAP library (most of them in the meataxe code), but I think most could and should be replaced by a call to ErrorNoReturn. Similarly for packages.

And lots of packages produce warnings. And the code is still not doing an actual code flow analysis; so in order to avoid tons of spurious complaints about functions caused by the implicit return; we add to the end of function that don't end in a return statement, we ignore those implicit returns. This ensures we don't get a warning about this function:

foo := function(x)
if x = 1 then return 99;
else return 42;
fi;
end;

But on the other hand, it also means we don't get a warning about this function, even though we'd like to:

bar := function(x)
if x = 1 then return 99;
fi;
end;

Anyway, ultimately, we should not add this directly, but only enable it when requested -- see issue #1191.

[codecov]

Codecov Report

Merging #1541 into master will increase coverage by 14.04%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           master    #1541       +/-   ##
===========================================
+ Coverage   49.88%   63.93%   +14.04%     
===========================================
  Files         461      992      +531     
  Lines      239017   324056    +85039     
  Branches    10682    13005     +2323     
===========================================
+ Hits       119237   207180    +87943     
+ Misses     116960   114078     -2882     
+ Partials     2820     2798       -22

Impacted Files	Coverage Δ
src/code.c	86.95% <100%> (+3.16%)	⬆️
lib/type.gi	3.84% <0%> (-64.08%)	⬇️
lib/reesmat.gi	81.01% <0%> (-17.88%)	⬇️
src/funcs.c	69.8% <0%> (-16.01%)	⬇️
src/blister.h	92.3% <0%> (-7.7%)	⬇️
src/objects.h	80% <0%> (-7.5%)	⬇️
lib/adjoin.gi	69.38% <0%> (-6.13%)	⬇️
lib/function.gi	72.72% <0%> (-4.86%)	⬇️
src/sctable.c	47.01% <0%> (-3.37%)	⬇️
lib/hpc/thread1.g	51.02% <0%> (-3.33%)	⬇️
... and 885 more

[fingolfin]

It would actually be quite simple to do better tracking of the return statements, without a full blown code flow analysis (and that would also get rid of pointless returns when displaying functions, like here:

gap> foo := function(x)
> if x = 1 then return 99;
> else return 42;
> fi;
> end;;
gap> Display(foo);
function ( x )
    if x = 1 then
        return 99;
    else
        return 42;
    fi;
    return;
end
gap>

Namely, we add a single bit to every statement header, called returns -- it is true if that statement always ends up running into a T_RETURN_VOID or T_RETURN_OBJ statement, otherwise false. For leaf statements, it is true precisely for those two. For compound statements, we compute it recursively:

• for T_SEQ_STAT it is true if and only if it is true for any sub-statements
• for T_IF and friends, it is true if it is true for all sub-statements / bodies and if we can prove at least one "branch" of it is taken (so basically, if there is an else, or any expression that we know evaluates to true -- of course one could enhance this, to handle if FOO then ... elif not FOO then ... fi but (a) these are hopefully rare, and (b) this gets complicated quite quickly)
• for loops, it is true iff we can prove the loop is entered at least once, and the returns flag of the loop body is true.

Then, we call CodeReturnVoidWhichIsNotProfiled only if the final statement does not have the returns flag set to true. This will still insert some redundant such return statements, but far fewer.

I think at that point, only very few, if any, false positives will remain. Though of course one could still construct one on purpose, I just think (believe? hope?) that they will rarely if ever occur in normal code.

[fingolfin]

While this would be kinda nice, I don't have time or interest to work on this for now.