Generic client #640

fierce-katie · 2016-11-21T11:42:29Z

This PR is related to #344 and adds the class that matches client functions generated using client with data structure (its representation as sum of products).
The structure itself has to be written manually (no TH used here), but we avoid pattern-matching, which is replaced with just one line of code.

jkarni

This is really cool! Thanks a lot for it!

jkarni · 2016-11-21T11:46:55Z

servant-client/src/Servant/Client/Generic.hs

+--
+-- Example:
+--
+-- type API


Can you make this a code section (start each line with >)?

jkarni · 2016-11-21T11:47:09Z

servant-client/src/Servant/Client/Generic.hs

+-- type API
+--     = "foo" :> Capture "x" Int :> Get '[JSON] Int
+--  :<|> "bar" :> QueryParam "a" Char :> QueryParam "b" String :> Post '[JSON] [Int]
+--  :<|> Captre "nested" Int :> NestedAPI


Captre -> Capture.

jkarni · 2016-11-21T11:52:33Z

servant-client/src/Servant/Client/Generic.hs

+--  } deriving GHC.Generic
+--
+-- instance Generic.SOP.Generic
+-- instance (Client NestedAPI ~ client) => ClientLike client NestedAPI


I think the second NestedAPI should be NestedClient

jkarni · 2016-11-21T11:53:26Z

servant-client/src/Servant/Client/Generic.hs

+-- mkAPIClient :: APIClient
+-- mkAPIClient = mkClient (client (Proxy :: Proxy API))
+class ClientLike client custom where
+  mkClient :: client -> custom


It make be that I'm missing something, but if we had:

class (HasClient api) => ClientLike api custom where mkClient :: Client client -> custom

Then wouldn't we be able to say:

instance ClientLike NestedAPI NestedClient

Instead of:

instance (Client NestedAPI ~ client) => ClientLike client NestedClient

And maybe even get away with:

data NestedClient = ... deriving (ClientLike NestedAPI)

Well, GHC simply does not allow type family synonym instances (even with TypeSynonymInstances enabled). So we work around that with type equality constraint. Since in practice client structure (e.g. NestedClient) will only have one instance, I don't see this as a problem (only as an inconvenience).
I am not sure why that's not allowed, but I didn't investigate it.

We have also explored the possibility of having ClientLike parametrized with an API type rather than client type. But that leads to many more problems (e.g. you now have to know API type for every recursing mkClient call and also have separate instances for API combinators).

Fair enough - thanks for the explanation!

fizruk · 2016-11-21T12:03:23Z

A minor warning: for some APIs this might cause GHC panic (@fierce-katie can provide the error message). We've noticed this when tried to refactor our API a bit and haven't yet come up with a reason/solution (might be unrelated).

fierce-katie · 2016-11-21T12:06:05Z

The error is:

ghc: panic! (the 'impossible' happened)
  (GHC version 8.0.1 for x86_64-unknown-linux):
	pprIfaceCo

Please report this as a GHC bug:  http://www.haskell.org/ghc/reportabug

It may appear if you have a more complicated client structure.

jkarni · 2016-11-21T12:08:19Z

Yeah, that's a bug I've been hitting in a different project too. Presumably you're using stack? I don't think it's triggered without ddump-hi (which stack turns out by default).

GHC 8.0.2 has the fix, but it hasn't been released yet. If it's only triggered in client code that uses mkClient, in my opinion that's okay.

alpmestan · 2016-11-21T13:00:53Z

That's terrific! In my opinion this is even blog-post worthy (either on servant's or on your own).

fierce-katie · 2016-11-22T09:25:47Z

Also, I played with TH trying to generate client structures (or at least separate functions) declarations, as proposed in the mentioned issue, but encountered a problem getting TH types representations. TH does not expand type synonyms, so runQ [t| Client API|] results in an obvious AppT (ConT Client) (ConT API). There's th-expand-syns, but it doesn't support type families now.
My current code is very awkward and error-prone, but if I come up with something better, this could be in a separate PR and most probably be an Experimental module. Any suggestions are highly appreciated :)

phadej · 2016-11-22T11:55:42Z

servant-client/src/Servant/Client/Generic.hs

+-- >
+-- > mkAPIClient :: APIClient
+-- > mkAPIClient = mkClient (client (Proxy :: Proxy API))
+class ClientLike client custom where


I'd add a note why there isn't | client -> xs functional dependency.

With very quick reasoning

type family FlatClient client where FlatClient (a :<|> b) = FlatClient a ++ FlatClient b FlatClient (a -> b) = Map ((->) a) (FlatClient b) FlatClient (ClientM a) = '[ClientM a]

or alternative single-type-class definition could work too. That relies on the fact that atm there is always a type constructor to match on.

I have used type level ++ and Map successfully.

Haven't noticed that this PR doesn't flatten the client. That might be useful too. Anyhow, might be a good idea to mention the fact in the documentation / blog post.

I'd add a note why there isn't | client -> xs functional dependency.

Sure. I would say that there is more that one way to turn Client api into another data type.

One example we're working on is for APIs like (a :<|> b) :<|> c (note the parentheses!). Currently this code will only match a client for this API with a structure like this:

data TopClient = TopClient { mkNestedClient :: NestedClient , getC :: ... } data NestedClient = NestedClient { getA :: ... , getB :: ... }

In practice sometimes it is reasonable to embed NestedClient in TopClient:

data TopClient = TopClient { getA :: ... , getB :: ... , getC :: ... }

But I'm not sure how to allow both structures.

Haven't noticed that this PR doesn't flatten the client. That might be useful too.

Sure, that can be handy too. But that could be a different transformation that can be combined with an approach shown here.

I had also been thinking about flattening clients, but I agree that we can add that later - it seems to be just a type family away.

@jkarni it's a class with type family — you have to flatten on term-level too.

Why? Shouldn't

type family Flatten client where ... mkAPIClient = mkClient (client (Proxy :: Proxy (Flatten API)))

Just work?

I think I'd intuitively prefer accepting the non-nested definition for left-nested occurrences of :<|> instead of requiring the nested one. This would be easy to do, and seems a bit more symmetric to me.

That's actually not that easy to do. Now we take only the head of the type level list, but if we go left and right, we'd have to use xs ++ ys which can't be used in an instance head. So we'd need to define a separate class with associated type family to split zs into xs and ys based on a :<|> b client. This is doable, of course, just messy.

This approach also makes a :<|> b decide how the custom client should be split... which is probably not a good idea, although I am not 100% sure. One concern is that a :<|> b :<|> c client can be represented by either 3 fields in a client record, or 1 field (client sub-record). I suppose that means that if we flatten the nested :<|> on the left, sometimes GHC wouldn't be able to resolve the types. To eliminate this issue we could add instance TypeError ... => ClientLike (a :<|> b) '[x] so that :<|> would have to be flattened always...

So I guess, we can do it, but I am not sure yet that we should.

I don't understand the argument about N-tuples here. I think @jkarni is right. Flattening is an orthogonal operation. If you have a type-level computation Flatten that turns your API into flattened form, then you can do the entire construction on Flatten API, and then successfully map to a flat datatype.

Here's a simple example of a client and flattened client. Which instances should I add to ClientLike to allow mkClient :: Client -> FlattenedClient? I doesn't work out of the box right now and I don't see an immediate solution here (accept the one that is essentially its own type class for flattening, such as one here)

type Client = Maybe Int -> Maybe String -> (ClientM Int :<|> ClientM String) type FlattenedClient = (Maybe Int -> Maybe String -> ClientM Int) :<|> (Maybe Int -> Maybe String -> ClientM String)

Associativity:

You don't need (++) if you use an accumulator / continuation. I know this because I implemented the whole thing myself and then compared to your version, and this is the only real difference.

I agree that it becomes difficult if you want to allow both the nested and un-nested variants, but I think not nesting the left-associative occurrences of :<|> has the nice property the nesting only occurs if there's nesting via :>.

Flattening:

You don't need to go from Client to FlattenedClient, because you compute the client for FlattenedClient in the first place. Then you only need to go from FlattenedClient to a custom datatype corresponding to FlattenedClient.

You don't need (++) if you use an accumulator / continuation. I know this because I implemented the whole thing myself and then compared to your version, and this is the only real difference.

Interesting! Can you share your version?

I agree that it becomes difficult if you want to allow both the nested and un-nested variants, but I think not nesting the left-associative occurrences of :<|> has the nice property the nesting only occurs if there's nesting via :>.

Actually there has to be a nesting via -> (we are talking client, not API). And not every :> results in an extra parameter.
But yeah, I like the idea of having only one option for matching :<|>.

You don't need to go from Client to FlattenedClient, because you compute the client for FlattenedClient in the first place. Then you only need to go from FlattenedClient to a custom datatype corresponding to FlattenedClient.

Yeah, that part I understand. It's just that @jkarni mentioned (in #640 (comment)) that Flatten type family suffice whereas I think it should be a class with associated type family and a flatten method since you can't use mkClient to flatten.

My version is here: https://gist.github.com/kosmikus/108b4bb11a17a9955495fc3db8b40661

And I still also think that a Flatten type family suffices. Why would you want to convert at the term level?

Ok, I've finally understood what I've been missing about flattening: one doesn't have to flatten the client on the term-level since we can generate already flattened client for a flattened API! Sorry everyone! :)

jkarni

Mind adding an entry into the changelog?

kosmikus · 2016-11-22T15:00:08Z

Looks nice. I'll try to take a more detailed look tonight. But if you want to merge already, don't let that stop you.

kosmikus · 2016-11-23T11:28:59Z

servant-client/servant-client.cabal

@@ -42,6 +43,7 @@ library
    , base64-bytestring     >= 1.0.0.1  && < 1.1
    , bytestring            >= 0.10     && < 0.11
    , exceptions            >= 0.8      && < 0.9
+    , generics-sop


Dependency should have bounds.

kosmikus · 2016-11-23T11:29:11Z

servant-client/src/Servant/Client/Generic.hs

+-- >  , postBaz   :: Maybe Char -> ClientM ()
+-- >  } deriving GHC.Generic
+-- >
+-- > instance Generic.SOP.Generic


This looks incomplete.

kosmikus · 2016-11-23T11:33:21Z

servant-client/src/Servant/Client/Generic.hs

+-- > mkAPIClient = mkClient (client (Proxy :: Proxy API))
+class ClientLike client custom where
+  mkClient :: client -> custom
+  default mkClient :: (Generic custom, GClientLikeP client xs, SOP I '[xs] ~ Rep custom)


I prefer Code custom ~ '[ xs ] over SOP I '[ xs ] ~ Rep custom, but both conditions are equivalent.

I agree, Code looks nicer!

jkarni · 2016-11-23T13:21:51Z

Here's the Canonicalize that @alpmestan mentioned. Note that there was a bug in this! I don't remember what it was, but @alpmestan might - he worked on it, if I recall correctly:

-type family Canonicalize api :: * where
 -- requires UndecidableInstances
 Canonicalize (a :> (b :<|> c)) = a :> Canonicalize b :<|> a :> Canonicalize c
 Canonicalize ((a :<|> b) :> c) = a :> Canonicalize c :<|> b :> Canonicalize c
 Canonicalize (a :> b)          = Redex b (Canonicalize b) a
 Canonicalize (a :<|> b)        = Canonicalize a :<|> Canonicalize b
 Canonicalize a                 = a

type family Redex a b c :: * where
  Redex a a first = Canonicalize first :> a
  Redex a b first = Canonicalize (first :> b)

alpmestan · 2016-11-23T13:59:38Z

@jkarni Are you thinking of #558 (comment) and #558 (comment) ?

alpmestan · 2016-11-23T14:02:19Z

By the way, this all makes me think we really ought to put Canonicalize (with another name) somewhere in one of the core packages. Duplicating it (with possibly different bugs in different implementations, that haven't been as battle-tested) is not the best option.

kosmikus · 2016-11-23T15:04:36Z

@alpmestan Agreed.

jkarni · 2016-12-07T12:34:24Z

@fierce-katie could you address @kosmikus' comments?

fierce-katie · 2016-12-12T11:59:52Z

I've implemented what @kosmikus suggests, but I think it's reasonable to have both implementations, use one of them as default and manually declare instance ClientLike if you want to use the other one. We've tried this on some real APIs, and it turned out to be rather convenient to have separate clients for logically separated nested APIs. So the idea is, depending on how instance ClientLike is declared for client data structure, its nested clients are either expanded or not.

If there are no objections, I'll add it to this PR.

fizruk · 2016-12-13T19:56:17Z

servant-client/src/Servant/Client/Generic.hs

+instance ClientLike (ClientM a) (ClientM a) where
+  mkClient = id
+
+-- GClientLikeP


This comment is not helpful.

fizruk · 2016-12-13T19:56:26Z

servant-client/src/Servant/Client/Generic.hs

+instance ClientLike a x => GClientLikeP a '[x] where
+  gMkClientP a = I (mkClient a) :* Nil
+
+-- GClientLikeL


And this one too.

phadej · 2017-01-17T11:54:36Z

I see that everyone is happy with this, can @fierce-katie rebase this so we can merge?

fierce-katie · 2017-01-17T13:45:42Z

@phadej rebased

phadej · 2017-01-17T15:06:30Z

Thanks, merged!

jkarni requested changes Nov 21, 2016

View reviewed changes

fierce-katie force-pushed the generic-client branch from 62a0cba to ab8bff2 Compare November 21, 2016 11:58

phadej reviewed Nov 22, 2016

View reviewed changes

jkarni approved these changes Nov 22, 2016

View reviewed changes

fierce-katie force-pushed the generic-client branch from 93f1569 to ea16c30 Compare November 23, 2016 10:04

kosmikus reviewed Nov 23, 2016

View reviewed changes

fierce-katie force-pushed the generic-client branch from e6d3709 to 0876ac9 Compare December 13, 2016 14:10

fizruk reviewed Dec 13, 2016

View reviewed changes

fizruk approved these changes Dec 14, 2016

View reviewed changes

phadej added this to the 0.10 milestone Jan 17, 2017

fierce-katie force-pushed the generic-client branch from bc11a35 to b623bd4 Compare January 17, 2017 12:31

Catherine Galkina added 4 commits January 17, 2017 16:29

Add Servant.Client.Generic module

31e1ceb

Add tests for Servant.Client.Generic

3cc6678

Update example in Servant.Client.Generic

682d7ea

Fix typos in examples

06217ac

Catherine Galkina added 6 commits January 17, 2017 16:29

Update CHANGELOG

d128fae

Expand left-nested APIs by default

5fa99be

Update comment in Servant.Client.Generic

0132475

Fix ghc-7.8.4 build

9d52cff

Add comments for GClientLikeP and GClientLikeL classes

aa4b10c

Fix servant-client tests after rebase

af1b267

fierce-katie force-pushed the generic-client branch from b623bd4 to af1b267 Compare January 17, 2017 13:35

phadej merged commit c50fdef into haskell-servant:master Jan 17, 2017

fierce-katie deleted the generic-client branch June 2, 2017 15:20

jkarni mentioned this pull request Mar 6, 2018

Generate client types and functions with TH #344

Closed

Generic client #640

Generic client #640

Conversation

fierce-katie commented Nov 21, 2016

jkarni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fizruk commented Nov 21, 2016

fierce-katie commented Nov 21, 2016

jkarni commented Nov 21, 2016

alpmestan commented Nov 21, 2016

fierce-katie commented Nov 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kosmikus Nov 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fizruk Nov 23, 2016 • edited Loading

Choose a reason for hiding this comment

jkarni left a comment

Choose a reason for hiding this comment

kosmikus commented Nov 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkarni commented Nov 23, 2016

alpmestan commented Nov 23, 2016 • edited Loading

alpmestan commented Nov 23, 2016

kosmikus commented Nov 23, 2016

jkarni commented Dec 7, 2016

fierce-katie commented Dec 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phadej commented Jan 17, 2017

fierce-katie commented Jan 17, 2017

phadej commented Jan 17, 2017

kosmikus Nov 23, 2016 •

edited

Loading

fizruk Nov 23, 2016 •

edited

Loading

alpmestan commented Nov 23, 2016 •

edited

Loading