Support partially variadic functions like function(a, b, arg)

[stevelinton]

Definitely not ready for merge, but presented for people to try. Turned out to be easier than I expected.

[frankluebeck]

Seems to work nicely, also with more than 7 named arguments!

How about the following example:

gap> g:=function(a,b,c,arg,d) return [[a,b,c,d],arg]; end;
function( a, b, c, arg, d ) ... end
gap> Print(g);
function ( a, b, c, arg, d )
    return [ [ a, b, c, d ], arg ];
end
gap> g(1,2,3,4,5,6,7,8);
Error, Function: number of arguments must be 5 (not 8)
not in any function at line 39 of *stdin*
you can replace the argument list <args> via 'return <args>;'

Here arg is considered a usual argument name. That is probably not so good.

Possibilities:

• raise an error during function definition, when another argument is following arg
• or would it be asked too much to allow named arguments in the beginning and the end? (so that in the example g(1,2,3,4,5,6,7,8); would return [1,2,3,8,[4,5,6,7]])?

[stevelinton]

I will add an error when arg appears as a not-final parameter. As we discussed the other approach is possible but would be more work since it needs to find a place to hide a bit more information in the function expression object.

[stevelinton]

Added the error. Interestingly found a few cases of it in the library.

[stevelinton]

This could probably be merged. Ideally it wants some tests, but it seems robust.

[olexandr-konovalov]

Thanks. This also reveals another case of incorrect usage of arg in the FR package by @laurentbartholdi:

%%% Loading fr 2.1.1
Syntax error: arg can only be the last argument in /home/hudson/hudson/workspa\
ce/GAP-pkg-update-master-quicktest/GAPCOPTS/64build/GAPGMP/gmp/GAPTARGET/packa\
gesload/label/graupius/GAP-pkg-update-master-snapshot/pkg/fr-2.1.1/gap/algebra\
.gi line 150
BindGlobal("STRINGSTOLMACHINE@", function(r,arg,creator)
                                               ^

[olexandr-konovalov]

And some more in Singular, liealgdb and wedderga packages:

%%% Loading hap 1.10.15
Syntax error: arg can only be the last argument in /home/hudson/hudson/workspa\
ce/GAP-pkg-update-master-quicktest/GAPCOPTS/64build/GAPGMP/gmp/GAPTARGET/packa\
gesload/label/graupius/GAP-pkg-update-master-snapshot/pkg/singular/gap/singula\
r.g line 2529
GapInterface := function ( func, arg, out )
                                    ^

%%% Loading liealgdb 2.1
Error, NonSolvableLieAlgebra: <method> must accept 2 arguments called from
<compiled or corrupted statement>  called from
<compiled or corrupted statement>  called from
<function "local function">( <arguments> )
 called from read-eval loop at line 
948
  of /home/hudson/hudson/workspace/GAP-pkg-update-master-quicktest/GAPCOPTS/64\
build/GAPGMP/gmp/GAPTARGET/packagesload/label/graupius/GAP-pkg-update-master-s\
napshot/pkg/liealgdb/gap/nonsolv/nonsolv.gi
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue
brk>

%%% Loading wedderga 4.7.2
#I  You may wish to install the xgap package
#I  and enjoy the graphic capabilities of SONATA.
Error, PrimitiveIdempotentsNilpotent: <method> must accept 5 arguments called from
<compiled or corrupted statement>  called from
<compiled or corrupted statement>  called from
<function "local function">( <arguments> )
 called from read-eval loop at line 
2539
  of /home/hudson/hudson/workspace/GAP-pkg-update-master-quicktest/GAPCOPTS/64\
build/GAPGMP/gmp/GAPTARGET/packagesload/label/graupius/GAP-pkg-update-master-s\
napshot/pkg/wedderga/lib/main.gi

[olexandr-konovalov]

@james-d-mitchell - semigroups produces an error too:

%%% Loading semigroups 2.3
Syntax error: arg can only be the last argument in /home/hudson/hudson/workspa\
ce/GAP-pkg-update-master-quicktest/GAPCOPTS/64build/GAPGMP/gmp/GAPTARGET/packa\
gesload/label/graupius/GAP-pkg-update-master-snapshot/pkg/semigroups-2.3/gap/g\
reens-acting.gi line 86
function(arg, type, rel)

[rbehrends]

Would it make sense to have a special syntax for varargs when a function has multiple arguments? I am thinking of something like:

foo := function(x, y, arg...) return [x, y, Length(arg)]; end;

I.e. three trailing dots after the last argument indicating that it's s vararg function and that the last argument should contain a list with the remaining arguments.

This would avoid backwards compatibility issues with arg occurring in the middle of an argument list (and possibly also allow a different name to be chosen).

[stevelinton]

For arg used not as last argument, I can simply remove the check or downgrade it to a warning if this is preferred. A bigger problem is that some of these packages might also use arg as the last of more than one arguments, which is a syntax whose meaning is actually changed by this Extension, and which does not give an error. I'll try and check for this.

[james-d-mitchell]

This should be really easy to fix in the Semigroup package, I only introduced the code containing arg and some other arguments in the last release. It is just a list, not intended to be used as any number of arguments. I was a bit surprised I was allowed to do it in the first place :)

[stevelinton]

Running LoadAllPackages reveals use of arg not as last arguyment:

In homalg at line 6299 of HiomalgFunctor.gi
In singular at like 2529 of singular.g
and a few issues in FR of which the most serious is the function STRING_GROUP@ which uses
arg as last argument and so fails.
in LieAlgDB there is another issue of the more serious kind

I have reduced the error when arg is used "not last" to a warning and added a temporary warning whenever the new syntax is read to help people convert.

[stevelinton]

More problems in wedderga (PrimitiveIdempotentsNilpotent)

[olexandr-konovalov]

Thanks - I should be able to update Wedderga next week. For now, I will just update the merged archive of packages used for regression tests to not to include some packages that cause problems, in order to try to deal with one problem at a time...

[olexandr-konovalov]

Ok, so now the only seriously broken / breaking tests packages are LieAlgDB and Wedderga. For now, they are excluded from the merged packages archive that I am using in Jenkins tests of the master branch.

[fingolfin]

Wait, so we merged this despite knowing that it breaks existing code? That seems wrong to me... Surely that means we cannot use the code in its current form, and must use some other syntax, like e.g. what Reimer proposes.

Or are we really willing to break existing and widely deployed code out there for no good reason?

[olexandr-konovalov]

We did not know about this prior to the merge. We were discussing yesterday that it would be nice to extend Travis tests to check manual examples and package loading as well - this would help to detect such problems before merging PRs.

I think these packages used the wrong syntax, and the merge just helped to reveal this:

• LieAlgDB installs a method for NonSolvableLieAlgebra for 2 arguments as function(F, arg)
• Wedderga installs a method for PrimitiveIdempotentsNilpotent for 5 arguments as function(FG,H,K,C,arg).

The fix is easy, to use another name of the argument.

[stevelinton]

We do allow changes in major releases which may break some packages, otherwise we could (for instance) never add new keywords. I think in this case the clarity and naturalness of the new syntax as an extension to function( arg ) probably justifies this. I would plan on leaving the warning in through the 4.8 beta releases and removing it (or making it a compile-time switch) for the first full 4.8 release.

I'd be willing to debate this further though.

[fingolfin]

The fix is easy, to use another name of the argument.

Well, of course the fix (in the sense of: the required code change in each package) is very easy. The problem is to convince package authors to apply the fix, and make releases... Also, somebody needs to contact the package maintainers, and get them to make those changes.

Anyway, if this breaking change is deemd OK, then let's at least try to be pro-active about getting packages compatible... This is a change package authors can do now, after all, without any other drawbacks.

So, let's collect which packages are affected, and what their status is

• fr -> @laurentbartholdi might take care of it; alternatively, somebody could submit a PR at https://github.com/laurentbartholdi/fr
• homalg -> @mohamed-barakat will take care of it (once he knows about it ;-)
• LieAlgDB -> somebody needs to contact the maintainers
• semigroups -> @james-d-mitchell will take care of it?
• singular -> somebody needs to contact the maintainers
• Wedderga -> @alex-konovalov will take care of it?

I hope there'll be "somebodies" to take care of the open tasks ;). In each case, sending a package authors a .patch with the relevant changes might increase the change of the change being implemented.

[mohamed-barakat]

Thanks all and thanks GitHub for the quick info: I renamed arg -> args in homalg v2015.03.31, which should now be available for download.

[olexandr-konovalov]

I will take care of Wedderga package, sure.

QaoS package is also affected, and it's missing in the list.

[olexandr-konovalov]

In spite of this thread I've just ticked some more boxes in @fingolfin's list of packages above for those packages that are already fixed.