Raise error if function is defined which sometimes returns a value and sometimes doesn't

[fingolfin]

Could we perhaps enhance the code reading functions to detect if they sometimes return nothing, and sometimes return a value? I can't think of any reason why a function like that would be sensible, on the other hand they can b a source of bugs.

Initially, this could be just a warning, but on the long run, it might become an error.

[stevelinton]

You might have to ignore the fact that functions can return nothing by running off the end. Otherwise you could have:

function(g)
  if not IsGroup(g) then
    Error();
  fi;
  if IsSolvableGroup(g) then
    return "foo";
 fi;
  stuff
 if Size(g) mod 2 = 0 then
    return "bar";
 fi;
end;

This actually always returns a value, but no reasonable checker will be able to prove that it never runs off the end.

You could detect the simpler case of a function which actually contains both return obj; and return; statements.

[fingolfin]

That would be a nice start. Though I wouldn't even mind if the above function produced a warning resp. an error: Because to understand that it is correct, you need to invoke a deep mathematical theorem. I think that's a bad way to write a function in the first place. And in your specific example, either the author is aware of that the theorem -- and then the final "if" check could be removed. Or the author is not aware, but then they should have added an return.

Of course there are other arguments one could make. E.g. to decide whether this is "correct" you should look at f, and possibly might have to decide the halting problem:

foo := function(x)
    if f(x) = true then
        return;
    fi;
    return x;
end;

My stance on that: It should trigger a warning / error, anyway.

[stevelinton]

See PR #466

[fingolfin]

Awesome, thanks for the quick work!

[frankluebeck]

I don't agree that it is desirable to disallow that a function may sometimes return a value and sometimes not. GAP functions can do many things and they can do different things, e.g., depending on the type or some properties of the arguments. So, why shouldn't they for certain input return something and for other not?

As already discussed above, it is just impossible that the parser decides this condition, therefore I don't see why there should be a warning in some cases. Actually, many GAP function are in fact formally of the kind you want to avoid (every function that does not end with an explicit return statement ends implicitly with a return;).

With PR #466 one gets:

gap> f := function(x) if x=0 then return 1; else return 2; fi; end;
function( x ) ... end
gap> f := function(x) if x=0 then return 1; else return 2; fi; return; end;
Syntax warning: Function contains both 'return <obj>;' and 'return;'
f := function(x) if x=0 then return 1; else return 2; fi; return; end;
                                                                    ^
function( x ) ... end
gap> f := function(x) if x=0 then return 1; else return 2; fi; end;
function( x ) ... end
gap> Print(f);
function ( x )
    if x = 0  then
        return 1;
    else
        return 2;
    fi;
    return;
end

Note that the two definitions of f are completely equivalent and GAP even prints the function in the second version.

I have checked 5 random cases of the warnings which occur in the #466 setup, and the warning was not appropriate in any of these cases. Some library functions seem to be formatted with the help of GAP's Print facility, so that there some explicit return; statments with cause warnings.

Please, do not merge this.

[stevelinton]

It might be useful to make this warning optional and off by default. The automatically added return at the end of functions does muddy the waters.

However, the problem with functions that actually sometimes return a value and sometimes not is that is basically impossible to program with them. There is no way (except for abuse of CallWithTimeout and similar) to call a function and find out whether it returned something. You either try and store the return value, or you ignore it. So they are certainly not good style in any normal situation.

[fingolfin]

@frankluebeck The reason why I want to make it annoying (and perhaps eventually, forbid it outright) for a function to sometimes return a value, and sometimes not, is precisely the one @stevelinton outlines: A caller has no safe way to figure out which will happen. Of course you can work around that by programming the caller with knowledge of when the called function will behave either way.

And of course you can write code that does just that.

But I argue that you can always refactor your code to not do this. The example you give gives two function which indeed in the current GAP sematics are equivalent. But I'd argue that one clearly is superior to the other; it is a no-brainer to remove the extra "return" statement.

You say you looked at the warnings on #466, and found all 5 random cases you looked at to be inappropriate. Interestingly, for me it is the reverse: So far, every instance I looked at, I found fully appropriate, and would prefer a version of the function which fixes the warning.

So, in order to not continue this discussion based on artifical examples, could you perhaps point out some concrete places this warning occurs where you find it inappropriate? Then we could try to understand why you consider it inappropriate there, resp. why we consider it appropriate (if we do -- I certainly haven't looked at all places there warning was reported).