From 9cd3d7e8364214cd2c7897ad3065087a62170f0a Mon Sep 17 00:00:00 2001
From: Max Horn <max@quendi.de>
Date: Wed, 9 May 2018 14:05:29 +0200
Subject: [PATCH] Fix two bugs related to empty magmas

First, SubadditiveMagma(a,[]) computes an empty magma, but we erroneously set
IsTrivial for it, which implies size 1. This was changed to IsEmpty.

Secondly, there was an implication from "IsFiniteOrderElementCollection and
IsMagma" to IsMagmaWithInverses. But such a collection may be empty, hence the
implication is invalid as given.

Fixes #2400
---
 lib/addmagma.gi                        |  2 +-
 lib/magma.gd                           |  5 ++-
 tst/testbugfix/2018-05-09-submagma.tst | 54 ++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 3 deletions(-)
 create mode 100644 tst/testbugfix/2018-05-09-submagma.tst

diff --git a/lib/addmagma.gi b/lib/addmagma.gi
index 72f692d91b..ab653cbf46 100644
--- a/lib/addmagma.gi
+++ b/lib/addmagma.gi
@@ -175,7 +175,7 @@ InstallGlobalFunction( SubadditiveMagmaNC, function( M, gens )
     if IsEmpty( gens ) then
       K:= NewType( FamilyObj(M),
                        IsAdditiveMagma
-                   and IsTrivial
+                   and IsEmpty
                    and IsAttributeStoringRep );
       S:= Objectify( K, rec() );
       SetGeneratorsOfAdditiveMagma( S, [] );
diff --git a/lib/magma.gd b/lib/magma.gd
index d939bcd620..feeebea879 100644
--- a/lib/magma.gd
+++ b/lib/magma.gd
@@ -129,8 +129,9 @@ DeclareCategory( "IsMagmaWithInverses",
         IsMagmaWithInversesIfNonzero
     and IsMultiplicativeElementWithInverseCollection );
 
-InstallTrueMethod( IsMagmaWithInverses,
-    IsFiniteOrderElementCollection and IsMagma );
+# FIXME: this is wrong for empty magmas
+# InstallTrueMethod( IsMagmaWithInverses,
+#     IsFiniteOrderElementCollection and IsMagma );
 
 #############################################################################
 ##
diff --git a/tst/testbugfix/2018-05-09-submagma.tst b/tst/testbugfix/2018-05-09-submagma.tst
new file mode 100644
index 0000000000..498676c808
--- /dev/null
+++ b/tst/testbugfix/2018-05-09-submagma.tst
@@ -0,0 +1,54 @@
+#
+# There was a bug where we marked a sub additive magma
+# with empty generator list as trivial, even though it is empty.
+#
+# There was also a wrong implication, from "IsFiniteOrderElementCollection and
+# IsMagma" to IsMagmaWithInverses. But a collection with the former filters may
+# be empty, and then it isn't a IsMagmaWithInverses.
+#
+gap> amgm:=AdditiveMagma([0]);
+<additive magma with 1 generators>
+gap> Size(amgm);
+1
+gap> IsTrivial(amgm);
+true
+gap> IsEmpty(amgm);
+false
+gap> IsMagmaWithInverses(amgm);
+false
+
+#
+gap> asub:=SubadditiveMagma(amgm, []);
+<additive magma with 0 generators>
+gap> Size(asub);
+0
+gap> IsTrivial(asub);
+false
+gap> IsEmpty(asub);
+true
+gap> IsMagmaWithInverses(asub);
+false
+
+#
+gap> mgm:=Magma( () );
+<commutative semigroup with 1 generator>
+gap> Size(mgm);
+1
+gap> IsTrivial(mgm);
+true
+gap> IsEmpty(mgm);
+false
+gap> IsMagmaWithInverses(mgm);
+true
+
+#
+gap> sub:=Submagma(mgm,[]);
+<commutative semigroup with 0 generators>
+gap> Size(sub);
+0
+gap> IsTrivial(sub);
+false
+gap> IsEmpty(sub);
+true
+gap> IsMagmaWithInverses(sub);
+false