opened
bgamari
username_0
Thanks for the great library. This is the current state of my ill-fated attempt at adding support for the compact protocol. I say ill-fated because it unfortunately seems impossible to correctly implement this protocol within the confines of the existing interface.

While this patch implements most of the protocol correctly, it unfortunately falls short in handling empty maps and other containers. In this case the compact protocol omits any type information, which means we are unable to provide an unambiguous `TType` for the resulting `Value`. Unfortunately it's really not clear how to fix this while maintaining the nice semantics of the library (e.g. allowing access to a `Value`'s `TType`). In this sense it appears that the compact protocol sacrifices the fully self-describing property that the original binary protocol preserved.

I can see two ways forward, neither very appealing,
 1. Require that type information is provided during deserialization
 2. Add a `TAny` type, meaning that the type of a term depends upon its value (since we would need to assign an empty list the element type `TAny` whereas a non-empty type would have a proper type.
 
Perhaps you have some ideas on how to proceed?
[RFC] Add support for compact protocol
issue
created
abhinav
username_1
Hi, thanks for implementing this! I'll give the code a look tonight.

I didn't realize that the compact protocol loses that information. Makes sense, though. I think we can get away with an approach like `TAny`, but calling it `TStop` instead (this appears to be closer to what the official implementations do).

We can introduce a new `TType`, `TStop`, which is invalid if passed to serialize/deserialize. There will be no `Value` constructors that build a value with type `TStop`. When parsing, if the map size is 0, an empty list of `MapItem TStop TStop` will be returned. Something like,

```haskell
if count == 0
  then return $ VMap (FL.empty :: FL.FoldList (MapItem TStop TStop))
  else parseStuff
```

This will type check just fine because `VMap` is just `Value TMap` and the inner types aren't exposed.

The `Pinchable` instances for maps will simply have to special case `TStop` as "don't look at the value", it's empty. Although, I suspect that the special casing won't be necessary because the logic to [`unpinch` a map](https://github.com/username_1/pinch/blob/master/src/Pinch/Internal/Pinchable.hs#L223) doesn't look at the `TType`s associated with the map, it looks at the `TType`s of the individual elements in the map. So for an empty map, it won't care that the `TType` doesn't match the type it's expecting to parse, and it'll just work.

Does that sound like a good approach?
comment
created
bgamari
username_0
It turns out that modifying the type system isn't strictly necessary.
Indeed all that is necessary is to add a constructor to Value, `VNullMap
:: Value TMap`. This is what the patch now does; let me know what you
think.

Otherwise all of the tests currently pass so I don't expect there will
be any further challenges.

Cheers,

- Ben
comment
created
abhinav
username_1
Hey. I'm done reviewing this. This looks great. I mostly had stylistic concerns. 

Also, the build seems to be broken on all versions of GHC. For pre-7.10, it's because of:

```
src/Pinch/Internal/Value.hs:168:45:
    Not in scope: type constructor or class ‘Foldable’
    Perhaps you meant ‘F.Foldable’ (imported from Data.Foldable)
```

For 7.10 and above, it's because of:

```
tests/Pinch/Protocol/CompactSpec.hs:21:8:
    Could not find module ‘Pinch.Protocol.Compact’
    it is a hidden module in the package ‘pinch-0.2.0.1@pinch_CxCWmmHvJQVLA3tJTm4NVM’
```

I suspect this one is happening because the new module is not listed in package.yaml as an exposed module and `hpack` overwrites your change to the cabal file, defaulting the new module to `other-modules`.
comment
created
abhinav
username_1
I addressed a couple of minor issues (including the >=7.10 failures) and created pull requests against your PR branch.

- https://github.com/username_0/pinch/pull/1
- https://github.com/username_0/pinch/pull/2
- https://github.com/username_0/pinch/pull/3

You can merge them in with the changes you're already making. Let me know if you need help getting this finished. 

Abhinav
comment
created
abhinav
username_1
Merged. Thanks!
comment
created
abhinav
username_1
This is out now in pinch-0.3.0.0.
comment
created
bgamari
username_0
Thanks!
comment