Add facilities to streamline GAP wrappers for GAP properties/attributes; reduce overhead

[fingolfin]

We have a lot of GAP wrapper functions that looks like this:

isperfect(G::GAPGroup)::Bool = GAP.Globals.IsPerfectGroup(G.X)

or this:

function complement_system(G::GAPGroup)
   if !issolvable(G) throw(ArgumentError("The group is not solvable")) end
   return _as_subgroups(G, GAP.Globals.ComplementSystem(G.X))
end

They all follow roughly this pattern:

1. test some preconditions
2. call a GAP function via GAP.Globals.FOOBAR
3. possibly convert or wrap the output

But both GAP functions called above are GAP attributes (the first one is a special case, a GAP property, which is an attribute that can only take on the values true or false). For GAP attributes, one really also should expose their corresponding testers (HasIsPerfectGroup -> hasisperfect or knowsisperfect or so; and HasComplementSystem -> hascomplement_system or has_complement_system or whatever), and possibly also setters (SetIsPerfect(group, bool) can be useful when building new group objects about which extra information is known, as is SetComplementSystem)). Right now, that means one should actually add three functions for each (and of course keep consistent naming in mind); e.g.

isperfect(G::GAPGroup)::Bool = GAP.Globals.IsPerfectGroup(G.X)
hasisperfect(G::GAPGroup)::Bool = GAP.Globals.HasIsPerfectGroup(G.X)
setisperfect(G::GAPGroup, val::Bool) = GAP.Globals.SetIsPerfectGroup(G.X, val)

Moreover, in GAP one can actually query any attribute for its tester or setter: Tester(IsPerfectGroup) returns HasIsPerfectGroup. We could offer a similar feature, e.g. tester(isperfect) would return hasisperfect.

And then there is another thing: using GAP.Globals.FOOBAR currently has quite some overhead (see oscar-system/GAP.jl#485 for some more information). While I hope to reduce this overhead in general, it will never be zero. We could mitigate this by fetching this value once and storing it in a Julia const var, like so:

const _cached_GAP_IsPerfectGroup = GAP.Globals.IsPerfectGroup
isperfect(G::GAPGroup)::Bool = _cached_GAP_IsPerfectGroup(G.X)

(This has one drawback: in GAP one can change almost everything; a user could with some effort replace IsPerfectGroup by something else, and then our "cache" wouldn't refer to that. However, this is a very nasty hack and I'd be happy to explicitly not support it; no GAP code or package I know does such a thing (well, some do something similar to a few functions, but nothing I am aware of does it to properties or attributes like that).

Anyway:

Implementing all this in a consistent way is somewhat tedious. Also, if decide to change some naming conventions, or add/remove/improve the caching method I sketched, we'd have to do it lots of places. So I think it would make sense to introduce some facilities that help doing this, and in a consistent way. This could e.g. be a macro (or a set of macros) which in their simples form, might take the name of a GAP function, the desired Julia name and information about the arguments and their types. Something like this pseudo code

@gap_wrapper isperfect IsPerfectGroup (G::GAPGroup, )

which could then emit code for the getter, tester and setter. A more powerful version of the macro could also let you specify a pre- and post-condition.

Another idea which is perhaps a bit more advanced but also more difficult to implement, would be to write a macro which takes a whole function as argument, which contains exactly one instances of GAP.Globals.FOOBAR; then the macro could extract everything it needs from that. I.e. you'd write

@gapwrap function complement_system(G::GAPGroup)
   if !issolvable(G) throw(ArgumentError("The group is not solvable")) end
   return _as_subgroups(G, GAP.Globals.ComplementSystem(G.X))
end

which might get turned into something like this by the macro:

const _cached_GAP_ComplementSystem = GAP.Globals.ComplementSystem
const _cached_GAP_HasComplementSystem = GAP.Globals.HasComplementSystem
const _cached_GAP_SetComplementSystem = GAP.Globals.SetComplementSystem
function complement_system(G::GAPGroup)
   if !issolvable(G) throw(ArgumentError("The group is not solvable")) end
   return _as_subgroups(G, _cached_GAP_ComplementSystem(G.X))
end
hascomplement_system(G::GAPGroup)::Bool = _cached_GAP_HasComplementSystem(G.X)
setcomplement_system(G::GAPGroup, val::GapObj) = _cached_GAP_SetComplementSystem(G.X, val)

That setter is of course not great (except for properties, I guess); but my idea would that one then could easily add a custom "better" setter in terms of that, by just adding this after the @gapwrap function ... end bit:

function setcomplement_system(G::T, val::Vector{T}) where T <: GAPGroup
  # option: verify here that all elements of val have G as parent, resp. are subgroups
  gaplist = ... turn val into a GAP list of subgroups ...
  setcomplement_system(G, gaplist)
end

[fingolfin]

After some discussion on the Julia Slack, I learned about the @generated macro which together with work I've begun on "merging" the types GapObj and MPtr (see oscar-system/GAP.jl#591) allows for an efficient solution:

@generated function isabelian(G::GAPGroup)
    isabelian_cache = GAP.Globals.IsAbelian
    quote
        $isabelian_cache(G.X)::Bool
    end
end

Bonus points if someone can produce a macro that makes it easier to do the above. Something which can be used like this:

@gap_wrapper isabelian(G::GAPGroup) = GAP.Globals.IsAbelian(G.X)::Bool

... and which takes all GAP.Globals.* references and "caches" them as described above.

[fingolfin]

The following is a working implementation of a macro as described above; I'll integrate this into Oscar (or GAP.jl?) later (don't want to generate too many conflicts with @GDeFranceschi work for now).

using GAP
using MacroTools

macro gapwrap(ex)

    # split the method definition
    def_dict = try
        MacroTools.splitdef(ex)
    catch
        error("@gapwrap must be applied to a method definition")
    end

    # take the body of the function
    body = def_dict[:body]

    # find, record and substitute all occurrences of GAP.Global.*
    symdict = IdDict{Symbol,Symbol}()
    body = MacroTools.postwalk(body) do x
        MacroTools.@capture(x, GAP.Globals.sym_) || return x
        new_sym = get!(() -> gensym(sym), symdict, sym)
        return Expr(:$, new_sym)
    end

    # now quote the old body, and prepend a list of assignments of
    # this form:
    #   ##XYZ##123 = GAP.Globals.XYZ
    def_dict[:body] = Expr(
        :block,
        # first the list of initializations ...
        (:(local $v = GAP.Globals.$k) for (k, v) in symdict)...,
        # ... then the quoted original-with-substitutions body
        Meta.quot(body),
    )

    # assemble the method definition again
    ex = MacroTools.combinedef(def_dict)
    ex2 = :(@generated $ex)

    # we must prevent Julia from applying gensym to all locals, as these
    # substitutions do not get applied to the quoted part of the new body,
    # leading to trouble if the wrapped function has arguments (as the
    # argument names will be replaced, but not their uses in the quoted part
    # of the body)
    return esc(ex2)
end

@gapwrap isevenint(x) = GAP.Globals.IsEvenInt(x)::Bool

[ThomasBreuer]

Concerning the question where to put the code (Oscar or GAP), I think that Oscar.jl/src/GAP would be the right place.
GAP.jl itself would not be so suitable already because the type GAPGroup is not known there.

[fingolfin]

The gapwrap macro does not use GAPGroup or anything else defined in Oscar.jl

[ThomasBreuer]

Yes, but all the examples (use cases) in earlier comments involve GAPGroup.
It seems that the two aspects of the issue should be separated:

The macro gapwrap helps to avoid the overhead of calling GAP.Globals.Something at runtime; this macro makes sense inside GAP.jl (and then can be used inside GAP.jl).

The aspect of uniformly/automatically modeling GAP's attribute getter/tester/setter mechanism (the original idea of this issue?) is not the job of the above gapwrap.
When I want to get the behaviour of the complement_system example above via a macro in GAP.jl, I think one has to assume that the macro gets code of a unary function where the argument has type T, say, and the code contains exactly one call of the form GAP.Globals.Something(X), where Something is a GAP attribute. Then the macro returns code for a getter/tester/setter where the arguments of the Julia functions have type T and the arguments in the calls of the GAP functions are X.
Such a macro can be part of GAP.jl, but I do not see use cases inside GAP.jl , except for something like GAP's IsSSortedList.
Or did I misunderstand something?

[ThomasBreuer]

Thinking again about the question where to put the macro(s) described in this issue, I am still undecided.
The code in GAP.jl itself will not benefit from the macro(s), this may indicate that Oscar.jl is the right place.
On the other hand, one can argue that any package based on GAP.jl (not only Oscar.jl) can benefit from it/them, thus GAP.jl would logically be the better choice.

[fingolfin]

We discussed this and agreed that we'll put it into GAP.jl, even if we don't really need it there.

[fingolfin]

We've dealt with this quite some time ago.