Is it really okay to convert msgpack's double to haskell's float?

[SX91]

I don't think so. Float to Double is perfectly fine, but not vice versa.

First of all, you should not actually use Float in haskell code :) https://wiki.haskell.org/Performance/Floating_point

[iphydf]

No, that's not okay. Let's fix it :).

[iphydf]

Ok, turns out we use Double for ObjectDouble. Where do you see those lossy conversions?

[SX91]

Look at the MessagePack Float instance.

[iphydf]

I'm looking at it - it looks fine to me. The MessagePack Float instance is something client code can use if they choose to use Float in their types. If client code uses Double, it will use the instance for Double, which does no conversions and generates msgpack double.

You shouldn't be looking at the MessagePack instances, since that's at a higher level. The decoding of msgpack doubles is done in Get.hs, in getDouble.

[SX91]

You shrink Double value to Float, that's not good. Float to Float is perfectly fine, Float to Double is perfectly fine, Double to Float is not fine at all. Some Float instace should not shrink ObjectDouble's Double to Float IMO.

[iphydf]

So what should the Float instance do instead? Note that the Float instance defines fromObject :: Object -> m Float.

[SX91]

Extract ObjectFloat's Float. Not ObjectDouble's Double and then shrinking it.

[iphydf]

I don't understand what you mean, since what I understand makes no sense. Can you make a PR for this?

[SX91]

@iphydf I mean that this https://github.com/TokTok/hs-msgpack/blob/master/src/Data/MessagePack/Class.hs#L144 is not okay

[iphydf]

What should it be doing instead?

[SX91]

Ignore ObjectFloat constructor (remove that line).

[iphydf]

Ok, make a PR that either fixes it or introduces a test case that's broken by this bug.

[iphydf]

Coercions to some extent are intentional. You're allowed to encode an Int64 and decode it as Int8 which will truncate it.