Fix MagmaWith[One|Inverses] "no method found" errors

[gwhitney]

The current master version of GAP has the following bug: (otherwise unreported, please let me know if I should have opened an issue before submitting this pull request)

The 2-argument forms of MagmaWithOne and MagmaWithInverses typically do not (perhaps never) succeed as documented, but rather report "no method found" errors. See the following gap transcript for a simple example of this behavior:

gap> Magma(CollectionsFamily(PermutationsFamily), [(1,2),(1,2,3)]);
<group with 2 generators>
gap> Elements(last);
[ (), (2,3), (1,2), (1,2,3), (1,3,2), (1,3) ]
gap> MagmaWithOne(CollectionsFamily(PermutationsFamily), [(1,2),(1,2,3)]);
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `MagmaWithOneByGenerators' on 1 arguments at /home/gwhitney/code/dev/gap/lib/methsel2.g:241 called from
MagmaWithOneByGenerators( arg ) at /home/gwhitney/code/dev/gap/lib/magma.gi:426 called from
<function "MagmaWithOne">( <arguments> )
 called from read-eval loop at *stdin*:3
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue
brk> quit;
gap> MagmaWithInverses(CollectionsFamily(PermutationsFamily), [(1,2),(1,2,3)]);
Error, no method found! For debugging hints type ?Recovery from NoMethodFound
Error, no 1st choice method found for `MagmaWithInversesByGenerators' on 1 arguments at /home/gwhitney/code/dev/gap/lib/methsel2.g:241 called from
MagmaWithInversesByGenerators( arg ) at /home/gwhitney/code/dev/gap/lib/magma.gi:503 called from
<function "MagmaWithInverses">( <arguments> )
 called from read-eval loop at *stdin*:3
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue

Checking the code in lib/magma.gi, the difficulty is clear: in the portions of the code for the "#family plus list of generators" alternative for the arguments to both of these functions, the first argument is correctly checked to be a family, but then the first argument, rather than the second, is checked to be a list. Since no family is a list, that branch of the code is never taken, and it falls through to the "# generators" option for the arguments, eventually failing because a family is not a sensible generator for a magma.

The submitted two-character change corrects the code to check the second argument in these situations for being a list. The following log shows the transcript of the same commands executed on the corrected code; they succeed as expected:

gap> MagmaWithOne(CollectionsFamily(PermutationsFamily), [(1,2),(1,2,3)]);
Group([ (1,2), (1,2,3) ])
gap> Elements(last);
[ (), (2,3), (1,2), (1,2,3), (1,3,2), (1,3) ]
gap> MagmaWithInverses(CollectionsFamily(PermutationsFamily), [(1,2),(1,2,3)]);
Group([ (1,2), (1,2,3) ])
gap> Elements(last);
[ (), (2,3), (1,2), (1,2,3), (1,3,2), (1,3) ]

[codecov]

Codecov Report

Merging #1798 into master will increase coverage by 0.12%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1798      +/-   ##
==========================================
+ Coverage    62.8%   62.92%   +0.12%     
==========================================
  Files         968      968              
  Lines      294774   294058     -716     
  Branches    13026    12963      -63     
==========================================
- Hits       185130   185043      -87     
+ Misses     106845   106216     -629     
  Partials     2799     2799

Impacted Files	Coverage Δ
lib/magma.gi	49.9% <100%> (+1.57%)	⬆️
src/stats.c	75.47% <0%> (-0.14%)	⬇️
src/hpc/traverse.c	78.21% <0%> (-0.09%)	⬇️
src/compiler.c	61.92% <0%> (-0.05%)	⬇️
src/hpc/thread.c	46.44% <0%> (ø)	⬆️
src/objset.c	82.59% <0%> (ø)	⬆️
src/gap.c	56.83% <0%> (+0.08%)	⬆️
src/lists.c	56.71% <0%> (+0.11%)	⬆️
src/funcs.c	71.96% <0%> (+0.14%)	⬆️
... and 2 more

[fingolfin]

@gwhitney Hi there, thanks for your report, even including a fix! Very much appreciated.

I took the liberty of slightly editing your report, and in particular, of inlining the two transcripts.

Ideally, if you feel up to it, you could use your "corrected" transcript as basis for a proper test which ensures that the bug will not resurface later on, by adding a new .tst file under tst/testbugfix/ (look at that dir for some examples, and feel free to ask if you have any questions.

That would help us safe some effort (if you cannot or do not want to do it, one of us will have to do it, which means it will take longer to process this pull request).

[gwhitney]

OK, hopefully that does it; let me know if there's any other way I can facilitate this patch. Best, Glen

[stevelinton]

This looks clearly correct to me.