From cea9c29d1932b446b1275488a7f1e7e65c69c969 Mon Sep 17 00:00:00 2001
From: Wilf Wilson <wilf@wilf-wilson.net>
Date: Fri, 7 May 2021 18:14:02 +0100
Subject: [PATCH] Introduce associativity check to `AsSemigroup`

See https://github.com/gap-system/gap/issues/4030.

Fixes #4030.
---
 lib/semigrp.gd                                |  3 +-
 lib/semigrp.gi                                |  6 ++-
 .../2021-04-08-non-associative-semigroup.tst  | 52 +++++++++++++++++++
 tst/testinstall/magma.tst                     |  4 ++
 4 files changed, 63 insertions(+), 2 deletions(-)
 create mode 100644 tst/testbugfix/2021-04-08-non-associative-semigroup.tst

diff --git a/lib/semigrp.gd b/lib/semigrp.gd
index 9cf4c9df75..1b7fc8e505 100644
--- a/lib/semigrp.gd
+++ b/lib/semigrp.gd
@@ -174,7 +174,8 @@ DeclareOperation( "SemigroupByGenerators", [ IsCollection ] );
 ##  <Oper Name="AsSemigroup" Arg='C'/>
 ##
 ##  <Description>
-##  If <A>C</A> is a collection whose elements form a semigroup
+##  If <A>C</A> is a
+##  multiplicative element collection whose elements form a semigroup
 ##  (see&nbsp;<Ref Filt="IsSemigroup"/>) then <Ref Oper="AsSemigroup"/>
 ##  returns this semigroup.
 ##  Otherwise <K>fail</K> is returned.
diff --git a/lib/semigrp.gi b/lib/semigrp.gi
index 55dec81cd5..43ecbc5a45 100644
--- a/lib/semigrp.gi
+++ b/lib/semigrp.gi
@@ -540,10 +540,14 @@ InstallMethod( AsSemigroup,
 
 InstallMethod( AsSemigroup,
     "generic method for collections",
-    [ IsCollection ],
+    [ IsMultiplicativeElementCollection ],
     function ( D )
     local   S,  L;
 
+    if not IsAssociative( D ) then
+      return fail;
+    fi;
+
     D := AsSSortedList( D );
     L := ShallowCopy( D );
     S := Submagma( SemigroupByGenerators( D ), [] );
diff --git a/tst/testbugfix/2021-04-08-non-associative-semigroup.tst b/tst/testbugfix/2021-04-08-non-associative-semigroup.tst
new file mode 100644
index 0000000000..c99a0a8af5
--- /dev/null
+++ b/tst/testbugfix/2021-04-08-non-associative-semigroup.tst
@@ -0,0 +1,52 @@
+# https://github.com/gap-system/gap/issues/4030
+#
+gap> T := [
+>   [ 1, 2, 6, 5, 4, 3 ],
+>   [ 2, 1, 4, 2, 2, 5 ],
+>   [ 6, 5, 1, 6, 6, 4 ],
+>   [ 5, 6, 3, 4, 1, 2 ],
+>   [ 4, 3, 2, 1, 5, 6 ],
+>   [ 3, 4, 5, 3, 3, 1 ]
+> ];;
+gap> M := MagmaByMultiplicationTable(T);
+<magma with 6 generators>
+gap> IsAssociative(M);
+false
+gap> AsSemigroup(M);
+fail
+gap> M := MagmaByMultiplicationTable(T);
+<magma with 6 generators>
+gap> AsSemigroup(M);
+fail
+gap> AsSemigroup(Elements(M));
+fail
+
+#
+gap> T := [
+>   [ 1, 2, 3, 4, 5 ],
+>   [ 2, 2, 2, 5, 5 ],
+>   [ 3, 2, 3, 4, 5 ],
+>   [ 4, 2, 4, 3, 5 ],
+>   [ 5, 2, 5, 2, 5 ]
+> ];;
+gap> M := MagmaByMultiplicationTable(T);
+<magma with 5 generators>
+gap> IsAssociative(M);
+true
+gap> S := AsSemigroup(M);;
+gap> IsSemigroup(S);
+true
+gap> Size(S);
+5
+gap> M := MagmaByMultiplicationTable(T);
+<magma with 5 generators>
+gap> S := AsSemigroup(M);;
+gap> IsSemigroup(S);
+true
+gap> Size(S);
+5
+gap> S := AsSemigroup(Elements(M));;
+gap> IsSemigroup(S);
+true
+gap> Size(S);
+5
diff --git a/tst/testinstall/magma.tst b/tst/testinstall/magma.tst
index 2a8a543695..10ebdbaf47 100644
--- a/tst/testinstall/magma.tst
+++ b/tst/testinstall/magma.tst
@@ -30,6 +30,10 @@ gap> IsAssociative(M) or IsCommutative(M);
 false
 gap> Filtered(Combinations(Elements(M)), x -> Size(x) > 0 and IsAssociative(x));
 [ [ m1 ], [ m1, m3 ], [ m2 ], [ m2, m4 ], [ m3 ], [ m4 ] ]
+gap> AsSemigroup([Elements(M)[1], Elements(M)[2]]);
+fail
+gap> AsSemigroup([Elements(M)[1], Elements(M)[3]]);
+<semigroup of size 2, with 2 generators>
 gap> Filtered(Combinations(Elements(M)), x -> Size(x) > 0 and IsCommutative(x));
 [ [ m1 ], [ m1, m2 ], [ m1, m2, m3 ], [ m1, m2, m5 ], [ m1, m3 ], [ m1, m5 ], 
   [ m2 ], [ m2, m3 ], [ m2, m4 ], [ m2, m5 ], [ m3 ], [ m4 ], [ m5 ] ]