#97, make sure OneofSubfield generates well-typed code for non-Maybe

[ndmitchell]

Before the test case in #97 generated:

((Hs.coerce @(Hs.Int64)
   @(HsProtobuf.Signed (HsProtobuf.Fixed Hs.Int64)))
   (HsProtobuf.ForceEmit y))

Now it generates:

HsProtobuf.ForceEmit
    ((Hs.coerce @(Hs.Int64)
        @(HsProtobuf.Signed (HsProtobuf.Fixed Hs.Int64)))
    y)

I'm assuming the ForceEmit is important, and that seemed like the only sensible place to put it. But I was totally guessing... I welcome feedback!

[themattchan]

Using dimap would be nice here.

[ndmitchell]

Sorry, I don't see how to use dimap at all. Any hints?

[themattchan]

Oops, you're right -- I thought both the pre- and post- operations were enabled by isMaybe.

This might read better though:

let adapt | isMaybe = lmap wrapJust
          | otherwise = rmap (fmap forceEmitE)

xE <- adapt (wrapE ctxt options dpType) (HsVar (unqual_ "y"))

[ndmitchell]

I find that harder to read. I guess I'd usually (. wrapJust) and (fmap forceEmitE .) rather than lmap/rmap - and in this case I'm not sure it's clearer at all. But happy to go that way if you want.

[themattchan]

I was thinking that it would be better to consolidate the adjustments predicated on isMaybe, but on second thought the extra indirection obfuscates matters.

[themattchan]

👍 (with changes)

As an aside, I think generating

((Hs.coerce @(Hs.Int64)
   @(HsProtobuf.ForceEmit (HsProtobuf.Signed (HsProtobuf.Fixed Hs.Int64))))
   y)

would be even nicer, but there isn't really a performance benefit and would complicate the wrapE machinery.

[ndmitchell]

Thanks for the review. Yep, I tried to do that coerce you mention first, but that requires adding ForceEmit to the type data type, which seemed to be not very clean.

[themattchan]

Thanks for the bug fix, @ndmitchell!