Recursive fields: add Arg.fix and Schema.fix

[anmonteiro]

fixes #134 by implementing the proposed fix approach.

[anmonteiro]

drive-by change for better diffs when testing

[andreas]

Hi @anmonteiro! Thanks for implementing this 🙂 A couple of thoughts...

I think the type recursive is unnecessarily constraining, as it does not allow mutually recursive types with differing source type parameters. I think it could work with a single type parameter 'a. About the type name, my current favorite is fixor fixpoint, as it's so closely tied to the function of the same name.

Besides those details, what's previously held me back from landing something like this, is how to have a cohesive API design that encompasses interfaces and unions. I think it would be preferable to construct recursive types with unions and interfaces in a uniform manner, i.e. add fields interface and union to the recursive/fixpoint type. This would also improve a part of the current API, which I consider to be a sore point. There are some details I haven't thought through in regards to converting to an abstract value. I'll give it a think and back to you. Feel free to also take a stab at it 🙂

[anmonteiro]

@andreas Thanks for the prompt review.

I understand the limitation but I'm not sure how the type recursive can take less type parameters. Could you expand a little more on what you were thinking? I'm gonna dig into it further and try to figure it out myself anyway.

As for interfaces and unions, I agree it probably makes sense to explore its unification to this fixpoint API. I'm gonna play with it too. Do you think it makes sense to include it in this initial proposal or iterate on it over time?

[anmonteiro]

I just pushed a commit that makes the fixpoint type parameterized on just 'a

[anmonteiro]

commit d95b99e adds interface and union fields in the fixpoint definition too. I'm not sure if this is what you had in mind though.

[andreas]

Nice, I like the fixpoint type a lot better now 🙂

I think we need to test the proposed change with an example a group of mutually recursive types that include an interface, a union or both to make sure we address that complexity. It's not clear to me that it would work out well without having investigated it further.

Currently when you mix recursion and abstract types it works, but it's quite ugly and tricky. It requires use of lazy, mutability, Schema.add_typ and forcing lazy values with unit return value. Here's an example from irmin:

  (* ... *)
  and node = Schema.union "Node"
  and tree_as_node = lazy (Schema.add_type node (Lazy.force tree))
  and contents_as_node = lazy (Schema.add_type node (Lazy.force contents))

  [@@@ocaml.warning "-5"]

  let _ = Lazy.force tree_as_node
  let _ = Lazy.force contents_as_node

Maybe you can still get away with something like the above with the new fix function, but I would like to confirm that and see how it looks.

However, the new fixpoint construction means that we could instead require members of an abstract type to be specified when it is constructed, rather than added later with Schema.add_typ. This would mean we get rid of the mutability, lazy, etc. The above example would look something like the following:

  (* ... *)
  and node, MagicList.[tree_as_node; contents_as_node] = Schema.union "Node" ~types:AnotherMagicList.[tree; contents]

The complexity is instead shifted to the return value of union and interface, which needs to return functions that can project values into the abstract source type. This is what I've tried to indicate with MagicList and AnotherMagicList above. I haven't yet tried implementing the above to understand the tradeoff better.

I hope this clarifies my thinking a bit more 🙂

[anmonteiro]

Gotcha, I do understand your point now.

Some thoughts:

• the implementation in this PR makes it easier to describe the example you provided based on Irmin.
• agree we should probably include a test case that better exemplifies that mixing & matching of mutually-recursive GraphQL constructs
• I think, however, requiring members of an abstract type to be specified at the time of its creation might make a little harder to generate dynamic schemas, which is currently a use case of mine.

[andreas]

I've had a chance to try this out with irmin-graphql, and I think it's a net win, even with the inconsistencies for abstract types. I think union should be removed from fixpoint though, as it provides no value at this point. After that, I'm happy to merge this PR 🙂 (bonus points if you update the readme!)

I'm inclined to move towards specifying members of an abstract type at creation time as a follow-up. This would eliminate the last uses of lazy in irmin-graphql. I would appreciate if you can elaborate on your concerns though. I might have missed some limitations with this approach.

[anmonteiro]

@andreas thanks for trying this out! Happy to remove union and update the readme.

my concerns around dynamic schemas exist today even with the proposed approach. Let's say you want to define a self-recursive input object type with some dynamic arguments. Right now my code defines:

type _ dynamic_args =
  | DynamicArgs :
      { adapter : 'a -> 'args
      ; args : ('a, 'args) Gql.Arg.arg_list
      }
      -> 'a dynamic_args

In the above, we're hiding the value of 'args. How do I now create such type? Using the approach proposed in this PR, I could do something like:

Gql.Arg.fix (fun recursive ->
  recursive.obj
    name
    ~fields:(fun self ->
      let (DynamicArgs { args; adapter }) = get_dynamic_args_from_somewhere in
      args)
    ~coerce:(adapter StringMap.empty))

Right now there are 2 issues I'm working around:

1. I can't simply return args from the fields function. OCaml taps out with:

Error: This expression has type
         (Json.t StringMap.t, $DynamicArgsFields_'b1)
         Graphql_lwt.Schema.Arg.arg_list
       but an expression was expected of type
         (Json.t StringMap.t, 'a) Graphql_lwt.Schema.Arg.arg_list
       The type constructor $DynamicArgs_'b1 would escape its scope

2. there's no way to generate an adapter based on self, as it needs to be passed in outside the fields function

Hope this is helpful to understand the issue I'm facing.

[anmonteiro]

I don't necessarily see a way around the scope escape generally. I'll push a commit addressing the other suggestions shortly.

[anmonteiro]

Hey @andreas, gentle ping on this one.

[andreas]

Thanks @anmonteiro!