Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Even more general map serialising #341

Closed
wants to merge 7 commits into from

Conversation

phadej
Copy link
Collaborator

@phadej phadej commented Jan 22, 2016

More general version of #339

screen shot 2016-01-22 at 10 38 26

screen shot 2016-01-22 at 10 38 47

I'd prefer this one. Will write more instances & documentation if the approach is otherwise ok

@phadej
Copy link
Collaborator Author

phadej commented Jan 22, 2016

This is a bit problematic on ghc-7.4: https://travis-ci.org/phadej/aeson/jobs/104088939

ekmett/tagged#34

@ekmett
Copy link
Member

ekmett commented Jan 22, 2016

Sadly, you can't have a polykinded proxy on GHC 7.4 due to GHC bugs.

@phadej
Copy link
Collaborator Author

phadej commented Jan 22, 2016

Well, here I don't actually need polykinded proxy. So I can make this work with special type.

@phadej phadej force-pushed the generalise-maps-keys branch 6 times, most recently from 5d366a8 to 6e723f8 Compare January 25, 2016 09:34
@phadej
Copy link
Collaborator Author

phadej commented Jan 25, 2016

http://oleg.fi/aeson-bench.html result of aeson-benchmark-map , Coerce makes a difference. with GHC 7.8.4 at least. Identity is for some reason slower than Parser on smaller maps.

@bos
Copy link
Collaborator

bos commented Jan 26, 2016

Should I be closing #339 in favour of this?

@phadej
Copy link
Collaborator Author

phadej commented Jan 27, 2016

@bos If you like this approach, feel free to close #339.

@phadej phadej force-pushed the generalise-maps-keys branch 2 times, most recently from 5c6da22 to 06de712 Compare January 28, 2016 05:44
@phadej
Copy link
Collaborator Author

phadej commented Jan 31, 2016

@bos @bergmark, any opinions on this pull request?

@bergmark
Copy link
Collaborator

I see the benefits of generalizing key handling, I've wanted it multiple times myself (we even have our own package for it!)

  • Will this make type errors harder to understand for unsupported keys, e.g. if I try a map with ByteString keys?
  • What happens with existing monomorphic instances,, e.g. ToJSON (HashMap (CI String) x), will they need to be marked OVERLAPPING? I can imagine there are a lot of these in the wild and I'd like to be able to give a migration path that's as simple as possible, even if the better migration might be to add instances for the new type classes.
  • Since (AFAICT) this is a breaking change I'm a bit wary of merging it for the 0.11 release since 0.11 will mostly be steps back to be compatible with 0.9. I'd lean towards putting this in a 0.12 release some time later after 0.11 has been widely adopted.

@phadej
Copy link
Collaborator Author

phadej commented Jan 31, 2016

@bergmark, from the last commit you can see that errors are clean, actually clearer than previously:

.../aeson/tests/UnitTests.hs:285:16:
    No instance for (aeson-0.11.0.0:Data.Aeson.Types.Class.ToJSONKey
                       L.ByteString)
      arising from a use of ‘encode’
    In the expression: encode (H.empty :: H.HashMap L.ByteString Int)
    In an equation for ‘compileError’:
        compileError = encode (H.empty :: H.HashMap L.ByteString Int)

For the second point, I marked instances OVERLAPPABLE also for GHC >=7.10, (as they are in older GHCs), and locally added instance was ok.

About 3rd point I'm not sure. It's true that if you have somewhere HashMap MyKey x instance, it's not an orphan. OTOH the previous point should make it compilable still.

If there is a way to grep Hackage for those instances, I could check the packages against this branch. Yet probably most of them live in private application code.

@phadej
Copy link
Collaborator Author

phadej commented Jan 31, 2016

P.S. travis fail due GHC8.0, looks like something dependency related...

@phadej
Copy link
Collaborator Author

phadej commented Feb 1, 2016

I made a small test:

  • took about all packages with open upper aeson bound
  • filtered out the ones which still compile with vanilla lts-5.1 dependencies
  • switched in this branch

results in:

https://gist.github.com/phadej/6dd08254e6fbcdc4fc13

There are errors like

    /Users/ogre/mess/aeson-foo/DSH/src/Database/DSH/Common/Lang.hs:41:10:
        Duplicate instance declarations:
          instance FromJSON C.Day
            -- Defined at src/Database/DSH/Common/Lang.hs:41:10
          instance FromJSON C.Day
            -- Defined in ‘aeson-0.11.0.0:Data.Aeson.Types.Instances’

and

    /Users/ogre/mess/aeson-foo/bower-json/src/Web/Bower/PackageMeta/Internal.hs:318:26:
        Overlapping instances for Aeson.ToJSON [a]
          arising from a use of ‘.=’
        Matching instances:
          instance [overlappable] Aeson.ToJSON a => Aeson.ToJSON [a]
            -- Defined in ‘aeson-0.11.0.0:Data.Aeson.Types.Instances’
          instance Aeson.ToJSON [Char]
            -- Defined in ‘aeson-0.11.0.0:Data.Aeson.Types.Instances’
        (The choice depends on the instantiation of ‘a’
         To pick the first instance above, use IncoherentInstances
         when compiling the other instance declarations)
        In the expression: k .= xs

but none related to HashMap or Map.

EDIT: not the cleanest experiment setup, but that's what I got today

@phadej phadej force-pushed the generalise-maps-keys branch 3 times, most recently from 254b6d6 to eb37633 Compare February 2, 2016 11:48
@phadej
Copy link
Collaborator Author

phadej commented Feb 2, 2016

Made it possible to serialise maps as list of key value pairs (for complex keys). Now GHC 7.4 panics :/
GHC8.0 cache seems to be broken or something...

@phadej
Copy link
Collaborator Author

phadej commented Feb 2, 2016

Also it would be possible to have default implementations for ToJSONKey 'JSONKeyCoerce a (as it's unit) and ToJSONKey 'JSONKeyValueParser (using ToJSON instance). But then I'd need to introduce additional proxy' a parameter or implement it only for GHC8.0 (using injective type families)

@bergmark bergmark added this to the 0.12 milestone Feb 4, 2016
@andrewthad
Copy link
Contributor

I would like to offer some criticism of this branch, but first I just wanted to say thanks for doing this. This will be a fairly helpful feature for me.

I strongly agree with the four options for key serialization that were decided on. Before looking at the branch, I hadn't even considered the ValueParser option for serializing to a list, but I like it and think that it's helpful. I also like how to coerce and identity options ensures that performance is not degraded.

Here's where I disagree with the design. I think that the use of GADTs, singletons, flexible instances, fundeps, MPTCs and type families is unnecessary for solving this problem. I would like to propose a feature-equivalent solution that doesn't use any of these.

Basically, instead of having:

data JSONKeyMethod = JSONKeyCoerce | JSONKeyIdentity | JSONKeyTextParser | JSONKeyValueParser

type family ToJSONKeyType (m :: JSONKeyMethod) a
type instance ToJSONKeyType 'JSONKeyCoerce      a = ()
type instance ToJSONKeyType 'JSONKeyIdentity    a = a -> Text
type instance ToJSONKeyType 'JSONKeyTextParser  a = a -> Text
type instance ToJSONKeyType 'JSONKeyValueParser a = a -> Value

type family FromJSONKeyType (m :: JSONKeyMethod) a
type instance FromJSONKeyType 'JSONKeyCoerce      a = ()
type instance FromJSONKeyType 'JSONKeyIdentity    a = Text -> a
type instance FromJSONKeyType 'JSONKeyTextParser  a = Text -> Parser a
type instance FromJSONKeyType 'JSONKeyValueParser a = Value -> Parser a

I think the following would be easier to understand and use:

data ToJSONKeyFunction a
  | ToJSONKeyCoerce
  | ToJSONKeyText (a -> Text)
  | ToJSONKeyValue (a -> Value, a -> Encoding)

data FromJSONKeyFunction a
  | FromJSONKeyCoerce
  | FromJSONKeyText (Text -> a)
  | FromJSONKeyTextParser (Text -> Parser a)
  | FromJSONKeyValue (Value -> Parser a)

With this, almost everything becomes simpler. The definition of ToJSONKey is currently:

class JSONKeyCoercible m a => ToJSONKey a (m :: JSONKeyMethod) | a -> m  where
  toJSONKey :: proxy m -> ToJSONKeyType m a

It would become the following:

class ToJSONKey a where
  toJSONKey :: ToJSONKeyFunction a

The definition of FromJSONKey changes similarly. A convenient result of this is that JSONKeyCoercible is no longer necessary. Neither are SJSONKeyMethod or its implicitly-generating typeclass IJSONKeyMethod.

Another thing to note is that even the ToJSON/FromJSON instance heads for Map becomes simpler. For example, ToJSON for Maps is currently (omitting the OVERLAPPABLE_ pragma for clarity):

instance (ToJSON v, ToJSONKey k m, IJSONKeyMethod m) => ToJSON (M.Map k v)

It would become:

instance (ToJSON v, ToJSONKey k) => ToJSON (M.Map k v)

Let me know if there is any concern that certain functionality or guarantees would be lost with this approach. I would be happy to implement these changes on this branch if others find this strategy agreeable. Thanks.

@phadej
Copy link
Collaborator Author

phadej commented May 4, 2016

@andrewthad We want to have uniform method per type, by using your ToJSONKeyFunction we'd force to do selection per value.

Not that my approach could be probably "beautified" by tweaking stuff a bit. I must admit, I don't remember why I ended into current version.

@andrewthad
Copy link
Contributor

andrewthad commented May 5, 2016

@phadej I don't understand what you mean when you say:

We want to have uniform method per type, by using your ToJSONKeyFunction we'd force to do selection per value.

If we have:

class ToJSONKey a where
  toJSONKey :: ToJSONKeyFunction a

We can then write:

instance ToJSONKey Int where
  toJSONKey = ToJSONKeyText (Text.pack . show) -- inefficient, but works as an example

instance ToJSONKey Text where
  toJSONKey = ToJSONKeyCoerce

This is still choosing a coersion technique on a per-type basis, not on a per-value basis.

@phadej
Copy link
Collaborator Author

phadej commented May 5, 2016

@andrewthad Ah sorry, I missread... Have to think about!

@andrewthad
Copy link
Contributor

Also, I was thinking about this more, and regardless of which strategy is used, I'm suspicious that the special case for coerce/unsafeCoerce doesn't actually improve performance. I'm not totally confident in this, so I wanted someone else to think over it.

Here's my line of reasoning. The benefit of both coersion mechanisms (safe and unsafe) is that you can avoid walking over a structure. So, if we have:

newtype Foo = Foo { getFoo :: Text }

And we want to write:

convertList :: [Text] -> [Foo]

Then if we consider the following implementations:

convertList = coerce  -- (1)
convertList = map Foo  -- (2)

The first one is more performant because it don't have to traverse the list. However, if we write:

convertList = map coerce  -- (3)

It's just as performant as implementation (2) because we're still traversing the list (and applications of Foo and coerce are both optimized away at compile time). When we look at the use of coersion in this json key implementation, we see (ignoring the compatibility pragmas):

toJSON = case jsonKeyMethodSing p of
  SJSONKeyCoerce      -> Object . mapKeyVal coerce toJSON
  SJSONKeyIdentity    -> Object . mapKeyVal (toJSONKey p) toJSON

We're still mapping over the HashMap, so it's unclear to me anything is gained by this. If there is, I wonder if it could alternatively be accomplished with a few SPECIALIZE pragmas. I would appreciate if someone else would think over this as well, because I haven't done any benchmarking, just pondering.

@andrewthad
Copy link
Contributor

I've been thinking about this a little more, and I'd like to present a situation in which the approach I've proposed ends up being more expressive than the existing implementation of this feature. If anyone is jumping into this conversation here, make sure to read [my earlier comment] where I outlined this alternative.

If we're in a situation involving that involving a higher kinded type, we would like to be able to write ToJSONKey instances for it. (I'll ignore FromJSONKey for this illustration, but all of the same things apply to it). The most trivial example is Identity:

-- Proposed alternative implementation
contramap :: (a -> b) -> ToJSONKeyFunction b -> ToJSONKeyFunction a
contramap = ... -- this function has the obvious definition
instance ToJSONKey a => ToJSONKey (Identity a) where
  toJSONKey = contramap runIdentity toJSONKey

-- Attempt with existing implementation
instance ToJSONKey a m => ToJSONKey (Identity a) m where
  toJSONKey = ... -- cannot be written

In this particular case, it's not a huge deal because I never have values of type Map (Identity a) b that I want to serialize. But, this affects other types: tuples, lists, Pair (and isomorphic types like Complex), Maybe, etc. All of these have ToJSONKey (and FromJSONKey) instances under the proposed implementation but not under the existing one.

These instances don't always produce super pretty keys. As an example, let's consider (,). If we have some values:

example1, example2 :: (Text,Text)
example1 = ("hello","world")
example2 = ("evil.key","bad")

We want a tuple serialization that would continue using textual keys (rather than value keys) if both types create textual key. One naive approach might use a separator:

{ "hello.world" : { ... } }

This approach will not work for example2 because the deserialization would be unclear. So we would actually want something like this (where the length of the first piece is encoded up front):

{ "5.hello.world" : { ... } }
{ "8.evil.key.bad" : {...} }

Anyway, that's kind of a tangent, but I think that being able to offer this would be nice. I do sometimes want to serialize Map (Int,Int,Int) Double or things like that, and this provides a way to do that.

@bergmark bergmark mentioned this pull request May 6, 2016
18 tasks
@andrewthad
Copy link
Contributor

Just had another thought. If the ToJSONKey class were given another function:

class ToJSONKey a where
  toJSONKey :: ToJSONKeyFunction a
  toJSONKeyList :: ToJSONKeyFunction [a]
  toJSONKeyList = ... -- some default list promotion

Then this function could be overriden in the Char instance and left alone for all the others, and we could get rid of the last remaining need for OverlappingInstances.

@andrewthad andrewthad mentioned this pull request May 8, 2016
@phadej phadej closed this Jun 1, 2016
@phadej phadej deleted the generalise-maps-keys branch June 1, 2016 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants