Add support for tuples #237
Conversation
There was a problem hiding this comment.
Thank you, very nice looking PR!
I just have a few remarks.
I really liked that you've splitted the PR in two steps, adding support for tuples as argument, then as returned values. This really eases the review :-)
I believe you didn't run the tests on every commit.
I suggest you fix the qualify bug (40edf86) before adding the test case that reveals it, as it generates incorrect code.
Now, for per commit remarks:
You modified the documentation (the example_limitations.mli file) and the code. I think you didn't intend to as 1674f3c is dedicated to update the documentation.
It also contains some new test cases that would fit better in the commit adding the test in question (1d4dfda)
You've added a new warning and filter out functions based on tuple arity in the ir_of_gospel phase. Which is a very good idea. This leads to removing a check you've just added in the previous commit (9b10630). Going in the reverse order, you wouldn't have to implement the removed check.
Also, the name is (rightfully) changed from no_functional_arg_or_returned_tuple to no_functional_arg_or_big_tuple later (5bda4dd). I believe it would make more sense to change the function name in the same commit than its definition.
last_e is not a very informative name, may I suggest ty_show as it is what is being build?
Here, you uses QCheck.Print.tupX. The rest of the generated code (and STM in general) uses Util.Pp.pp_* which exposes pp_tupleX for X from 2 to 10 (included). Maybe e should stick to the latter, for consistency sake.
Also, I think it would be better to use qualify rather than evar on a string containing dots (for consistency sake again, as evar uses Longident.parse internally which I've discovered recently and I'm now wondering if we should get rid of our qualify... but that would be a story for another PR anyway).
In tuple_types, you can include the access to the returned type in your auxiliary function. This way you don't have to allocate the ret_tys list (not that ortac will be run on huge interfaces).
This adds functionality to test functions which return tuples. This is conceptually different from having tuples in argument positions as we need to extend the STM.ty type and provide a constructor so that Res values can be created for it.
This PR adds support for tuples as arguments and as return values.
This responds to #203