next iteration: _needs_tracking_julia_to_gap

- add `_needs_tracking_julia_to_gap(T)`, with default value `true`

- change `GAP.@install` to install a `_needs_tracking_julia_to_gap` method

- change the tracking logic:

  - Do not *create* a dictionary for tracking identical objects only if
    `_needs_tracking_julia_to_gap` for the subobject type returns `false`.

  - Let each object decide whether it is searched in the dictionary
    (in the beginning of its conversion method) or whether it gets added
    to the dictionary (as soon as the return value is available).

    For the current conversion methods for integers, rationals, etc.,
    this means that these objects are *not* added to the dictionary,
    that is, identical large Julia integers are converted to nonidentical
    large integers in GAP;
    if we want to change this, we have to change their `GapObj` methods.

- add a method for converting `Set{T}`:

  This had been added to Oscar.jl but should better be in GAP.jl.
  (There are tests for the return value whether the result is regarded
  as a set by GAP; perhaps we should restrict the allowed values of `T`.)

- changed `GAP.Wrappers.Add(x::GapObj, y::GapObj, z::Int)`
  to `GAP.Wrappers.Add(x::GapObj, y::Any, z::Int)`,
  in order to avoid the automatic conversion `GapObj(y)`

(Once the relevant code in Oscar.jl is changed to use `GAP.@install`,
the current changes in GAP.jl do not need further changes in Oscar.jl.)

src/julia_to_gap.jl

@@ -62,6 +62,7 @@ The following `GapObj` conversions are supported by GAP.jl.
 | `Char`                               | `IsChar`     |
 | `Vector{T}`                          | `IsList`     |
 | `Vector{Bool}`, `BitVector`          | `IsBList`    |
+| `Set{T}`                             | `IsList`     |
 | `Tuple{T}`                           | `IsList`     |
 | `Matrix{T}`                          | `IsList`     |
 | `Dict{String, T}`, `Dict{Symbol, T}` | `IsRecord`   |
@@ -74,6 +75,23 @@ GapObj(x, cache::GapCacheDict = nothing; recursive::Bool = false) = GapObj_inter
 # so we must make sure it is declared before
 function GapObj_internal end
 
+
+# The idea behind `_needs_tracking_julia_to_gap` is to avoid the creation
+# of a dictionary for tracking subobjects if their type implies that
+# no such tracking is needed/wanted.
+#
+# We need anyhow a `_needs_tracking_julia_to_gap` method for `Any`,
+# for example in order to convert a Julia object of type `Dict{Symbol, Any}`
+# to a GAP record.
+# Thus we have a default method returning `true`.
+#
+# Methods for those types that want `false` have to be installed;
+# the `GapObj` methods arising from `GAP.@install` calls are unary,
+# therefore the macro automatically installs such a
+# `_needs_tracking_julia_to_gap` method.
+_needs_tracking_julia_to_gap(::Any) = true
+
+
 GAP.@install GapObj(x::FFE) = x    # Default for actual GAP objects is to do nothing
 GAP.@install GapObj(x::Bool) = x    # Default for actual GAP objects is to do nothing
 
@@ -145,31 +163,85 @@ function GapObj_internal(
     recursive::Bool,
 ) where {T}
 
-    if recursion_dict !== nothing && haskey(recursion_dict, obj)
-      return recursion_dict[obj]
-    end
+    # If I have a dictionary then the converted value may be stored.
+    recursion_dict !== nothing && haskey(recursion_dict, obj) && return recursion_dict[obj]
+
+    # Initialize the return value.
     len = length(obj)
     ret_val = NewPlist(len)
-    if recursive
-        if recursion_dict === nothing
-          recursion_dict = RecDict()
-        end
-        recursion_dict[obj] = ret_val
+
+    rec = recursive && _needs_tracking_julia_to_gap(T)
+    if rec && recursion_dict === nothing
+        # Tracking of identical subobjects is needed,
+        # create the dictionary.
+        recursion_dict = RecDict()
+    end
+
+    # If we track identical subobjects then add `obj` to the dictionary.
+    if recursion_dict !== nothing
+      recursion_dict[obj] = ret_val
     end
+
+    # Set the subobjects.
     for i = 1:len
         x = obj[i]
         if x === nothing
             continue
         end
         if recursive
-            res = get!(recursion_dict::RecDict, x) do
-                GapObj_internal(x, recursion_dict::RecDict, recursive)
+            # Convert the subobjects.
+            # Do not care about findíng them in the dictionary
+            # or adding them to the dictionary,
+            # since their conversion method decides about that.
+            if rec
+              res = GapObj_internal(x, recursion_dict::RecDict, recursive)
+            else
+              res = GapObj_internal(x, nothing, recursive)
             end
         else
             res = x
         end
         ret_val[i] = res
     end
+
+    return ret_val
+end
+
+## Sets
+function GapObj_internal(
+    obj::Set{T},
+    recursion_dict::GapCacheDict,
+    recursive::Bool,
+) where {T}
+
+    recursion_dict !== nothing && haskey(recursion_dict, obj) && return recursion_dict[obj]
+
+    ret_val = GAP.NewPlist(length(obj))
+
+    rec = recursive && _needs_tracking_julia_to_gap(T)
+    if rec && recursion_dict === nothing
+        recursion_dict = RecDict()
+    end
+
+    if recursion_dict !== nothing
+        recursion_dict[obj] = ret_val
+    end
+
+    for x in obj
+        if recursive
+          if rec
+            res = GapObj_internal(x, recursion_dict::RecDict, recursive)
+          else
+            res = GapObj_internal(x, nothing, recursive)
+          end
+        else
+          res = x
+        end
+        Wrappers.Add(ret_val, res)
+    end
+    Wrappers.Sort(ret_val)
+    @assert Wrappers.IsSet(ret_val)
+
     return ret_val
 end
 
@@ -179,21 +251,29 @@ function GapObj_internal(
     recursion_dict::GapCacheDict,
     recursive::Bool,
 ) where {T}
-    if recursion_dict !== nothing && haskey(recursion_dict, obj)
-      return recursion_dict[obj]
-    end
-    (rows, cols) = size(obj)
+
+    recursion_dict !== nothing && haskey(recursion_dict, obj) && return recursion_dict[obj]
+
+    rows = size(obj, 1)
     ret_val = NewPlist(rows)
-    if recursive
-        if recursion_dict === nothing
-          recursion_dict = RecDict()
-        end
-        recursion_dict[obj] = ret_val
+
+    rec = recursive && _needs_tracking_julia_to_gap(T)
+    if rec && recursion_dict === nothing
+        recursion_dict = RecDict()
+    end
+
+    if recursion_dict !== nothing
+      recursion_dict[obj] = ret_val
     end
+
     for i = 1:rows
-        # Note that we need not check whether the row is in `recursion_dict`
-        # because we are just now creating the object.
-        ret_val[i] = GapObj_internal(obj[i, :], recursion_dict, recursive)
+      # We need not distinguish between recursive or not
+      # because we are just now creating the "row objects" in Julia.
+      if rec
+        ret_val[i] = GapObj_internal(obj[i, :], recursion_dict::RecDict, recursive)
+      else
+        ret_val[i] = GapObj_internal(obj[i, :], nothing, recursive)
+      end
     end
     return ret_val
 end
@@ -222,26 +302,34 @@ function GapObj_internal(
     recursive::Bool,
 ) where {S} where {T<:Union{Symbol,AbstractString}}
 
-    record = NewPrecord(0)
-    if recursive
-        if recursion_dict === nothing
-          recursion_dict = RecDict()
-        end
-        recursion_dict[obj] = record
+    recursion_dict !== nothing && haskey(recursion_dict, obj) && return recursion_dict[obj]
+
+    ret_val = NewPrecord(0)
+
+    rec = recursive && _needs_tracking_julia_to_gap(S)
+    if rec && recursion_dict === nothing
+        recursion_dict = RecDict()
     end
+
+    if recursion_dict !== nothing
+      recursion_dict[obj] = ret_val
+    end
+
     for (x, y) in obj
         x = Wrappers.RNamObj(MakeString(string(x)))
         if recursive
-            res = get!(recursion_dict::RecDict, y) do
-                GapObj_internal(y, recursion_dict::RecDict, recursive)
-            end
+          if rec
+            res = GapObj_internal(y, recursion_dict::RecDict, recursive)
+          else
+            res = GapObj_internal(y, nothing, recursive)
+          end
         else
             res = y
         end
-        Wrappers.ASS_REC(record, x, res)
+        Wrappers.ASS_REC(ret_val, x, res)
     end
 
-    return record
+    return ret_val
 end
 
 ## GAP objects:
@@ -260,27 +348,25 @@ function GapObj_internal(
         len = length(obj)
         ret_val = NewPlist(len)
         if recursion_dict === nothing
+          # We have no type information that allows us to avoid the dictionary.
           recursion_dict = RecDict()
         end
         recursion_dict[obj] = ret_val
         for i = 1:len
             x = obj[i]
-            ret_val[i] = get!(recursion_dict::RecDict, x) do
-                GapObj_internal(x, recursion_dict::RecDict, recursive)
-            end
+            ret_val[i] = GapObj_internal(x, recursion_dict::RecDict, recursive)
         end
     elseif Wrappers.IsRecord(obj)
         ret_val = NewPrecord(0)
         if recursion_dict === nothing
+          # We have no type information that allows us to avoid the dictionary.
           recursion_dict = RecDict()
         end
         recursion_dict[obj] = ret_val
         for xx in Wrappers.RecNames(obj)::GapObj
             x = Wrappers.RNamObj(xx)
             y = Wrappers.ELM_REC(obj, x)
-            res = get!(recursion_dict::RecDict, y) do
-                GapObj_internal(y, recursion_dict::RecDict, recursive)
-            end
+            res = GapObj_internal(y, recursion_dict::RecDict, recursive)
             Wrappers.ASS_REC(ret_val, x, res)
         end
     else

src/macros.jl

@@ -434,6 +434,11 @@ This way, the intended `GapObj(x::T)` method becomes available,
 and additionally its code is applicable in recursive calls,
 for example when `GapObj` is called with a vector of objects of type `T`.
 
+Since the `GapObj` method does not support a dictionary for tracking
+identical subobjects, the type `T` is marked as "not needing recursion",
+by automatically installing a method for `_needs_tracking_julia_to_gap`
+that returns `false`.
+
 The calls of the macro have the form `GAP.@install GapObj(x::T) = f(x)`
 or `GAP.@install function GapObj(x::T) ... end`.
 """
@@ -446,6 +451,7 @@ macro install(ex)
     catch
         error(errmsg)
     end
+
     def_dict[:name] === :GapObj || def_dict[:name] == :(GAP.GapObj) || error(errmsg)
     length(def_dict[:args]) == 1 || error(errmsg)
 
@@ -459,6 +465,17 @@ macro install(ex)
     # assemble the method definition again
     ex = MacroTools.combinedef(def_dict)
 
+    # install the `needs_conversion_tracking` method that returns `false`
+    x = def_dict[:args][1]
+    if x isa Symbol || !(x.head == :(::) && length(x.args) == 2)
+      error("argument of GapObj needs a type annotation")
+    else
+      typeannot = x.args[2]
+    end
+    Base.eval(__module__, quote
+        GAP._needs_tracking_julia_to_gap(::Type{$typeannot}) = false
+      end)
+
     return esc(Expr(
         :block,
         :(Base.@__doc__ $ex),

src/wrappers.jl

@@ -3,7 +3,8 @@ module Wrappers
 using GAP
 import GAP: @wrap
 
-@wrap Add(x::GapObj, y::GapObj, z::Int)::Nothing
+@wrap Add(x::GapObj, y::Any)::Nothing
+@wrap Add(x::GapObj, y::Any, z::Int)::Nothing
 @wrap AdditiveInverseSameMutability(x::Any)::Any
 @wrap Append(x::GapObj, y::GapObj)::Nothing
 @wrap ASS_LIST(x::Any, i::Int, v::Any)::Nothing
@@ -40,6 +41,7 @@ import GAP: @wrap
 @wrap IsRange(x::Any)::Bool
 @wrap IsRangeRep(x::Any)::Bool
 @wrap IsRecord(x::Any)::Bool
+@wrap IsSet(x::Any)::Bool
 @wrap IsSSortedList(x::Any)::Bool
 @wrap IsString(x::Any)::Bool
 @wrap IsStringRep(x::Any)::Bool
@@ -70,6 +72,7 @@ import GAP: @wrap
 @wrap SetInfoLevel(x::GapObj, y::Int)::Nothing
 @wrap SetPackagePath(x::GapObj, y::GapObj)::Nothing
 @wrap ShallowCopy(x::Any)::Any
+@wrap Sort(x::GapObj)::Nothing
 @wrap String(x::Any)::Any
 @wrap StringDisplayObj(x::Any)::GapObj
 @wrap StringViewObj(x::Any)::GapObj

test/conversion.jl

@@ -382,6 +382,13 @@ end
     @test GapObj([1 2; 3 4]) == x
   end
 
+  @testset "Sets" begin
+    x = GAP.evalstr("[1, 3, 4]")
+    @test GapObj(Set([4, 3, 1])) == x
+    x = GAP.evalstr("[\"a\", \"b\", \"c\"]")
+    @test GapObj(Set(["c", "b", "a"]); recursive = true) == x
+  end
+
   @testset "BitVectors" begin
     x = GAP.evalstr("BlistList([1,2],[1])")
     y = GapObj([true, false])
@@ -452,23 +459,32 @@ end
     # a GAP list with identical Julia subobjects
     l = GapObj([])
     x = BigInt(2)^100
+    y = [1]
     l[1] = x
     l[2] = x
-    @test l[1] === l[2]
+    l[3] = y
+    l[4] = y
     res = GapObj(l)
     @test res[1] === res[2]
+    @test res[3] === res[4]
     res = GapObj(l, recursive=true)
-    @test res[1] === res[2]
+    @test res[1] == res[2]
+    @test res[1] !== res[2]  # `BigInt` wants no identity tracking
+    @test res[3] === res[4]  # `Array` wants identity tracking
 
     # a GAP record with identical Julia subobjects
     r = GapObj(Dict{String, String}())
     setproperty!(r, :a, x)
     setproperty!(r, :b, x)
-    @test r.a === r.b
+    setproperty!(r, :c, y)
+    setproperty!(r, :d, y)
     res = GapObj(r)
     @test res.a === res.b
+    @test res.c === res.d
     res = GapObj(r, recursive=true)
-    @test res.a === res.b
+    @test res.a == res.b
+    @test res.a !== res.b  # `BigInt` wants no identity tracking
+    @test res.c === res.d
   end
 
   @testset "converting a list with circular refs" begin

test/macros.jl

@@ -60,14 +60,15 @@ end
 
 end
 
+struct TestType1 X::GapObj end
+struct TestType2 X::GapObj end
+
 @testset "@install GapObj" begin
     a = GAP.evalstr("(1,2)")
 
-    struct TestType1 X::GapObj end
     GAP.@install GapObj(x::TestType1) = x.X
     @test GapObj(TestType1(a)) === a
 
-    struct TestType2 X::GapObj end
     GAP.@install function GapObj(x::TestType2) return x.X; end
     @test GapObj(TestType2(a)) === a