Lifting instances for Reflex.Profiled

[endgame]

@mpickering asked me whether it was possible to use Reflex.Profiled
with reflex-basic-host. I said that it probably wouldn't be too
hard, you'd just need lifts here and there.

This PR adds lifting instances for all the classes I could find in
reflex, and tidies up a couple of warts I found along the way. I
don't really understand Reflex.Profiled (I just hit things with
coerce and lift until GHC was happy), so please look closely to
make sure useful things aren't getting dropped on the floor.

[ryantrinkle]

@endgame Awesome! It looks like we've gotten the Obsidian CI to work, but Travis is failing on ghc 8.6.5. I don't see how this could be due to your patch, but it would be nice to figure out how to fix this.

Here are some things that might be relevant:
haskell/haskell-ide-engine#1580
haskell/cabal#6483
haskell/cabal#6211

[endgame]

Many currently-open PRs have travis failures on 8.6.5 in similar-looking ways. Will this block merging?

[endgame]

I'm likely to be tied up for a while and not able to go digging through a CI that I don't understand. Has there been movement elsewhere to un-break this, and if not, will it block merging?

[3noch]

@endgame You're right. Many other PRs are suffering from the same CI failure. You shouldn't need to solve that. However, can you split this PR into separate PRs for each logical change you made? Each has its own merits but needs to be considered separately from the others.

[endgame]

@3noch To be clear, are you asking for one commit per instance? I can do that.

[3noch]

It looks like your commits each represent a valuable contribution on their own, so a PR for each one would be ideal. Some of them should be merged immediately and some may require more thought. I don't want any of your contribution getting stuck!

[endgame]

Split the minor fixes off into #399, #400, #401 .

[endgame]

This PR still uses the new name for BehaviorWriter, introduced in #401 . The 3 other PRs should be uncontroversial. Once they're in or rejected, I will rebase this on latest develop and we can talk about which instances are OK to have.

[3noch]

Thank you!

[3noch]

This is a legitimate CI failure.

[matthewbauer]

This one looks wrong. Could you add a signature to ensure it's correct with:

Suggested change

	newEventWithLazyTriggerWithOnComplete f = coerce <$> lift (newEventWithLazyTriggerWithOnComplete f)
	newEventWithLazyTriggerWithOnComplete f = coerce <$> lift ((newEventWithLazyTriggerWithOnComplete :: ((a -> IO () -> IO ()) -> IO (IO ())) -> m (Event t a)) f)

[matthewbauer]

(wrong because you should have to coerce f in some way)

[endgame]

I understand your concern for other instances, but I don't see how it applies here. Why should f be coerced?

newEventWithLazyTriggerWithOnComplete :: ((a -> IO () -> IO ()) -> IO (IO ())) -> m (Event t a)
-- vs.
newEventWithLazyTriggerWithOnComplete :: ((a -> IO () -> IO ()) -> IO (IO ())) -> ProfiledM m (Event (ProfiledTimeline t) a)

I don't see how f changes.

[matthewbauer]

This one might be too

[endgame]

Similarly:

getPostBuild :: m (Event t ())
-- vs.
getPostBuild :: ProfiledM m (Event (ProfiledTimeline t) ())

[matthewbauer]

We had an issue with bad instances for Reflex.Profiled before (#389). It's actually pretty easy to accidentally recursively call the "ProfiledTimeline" instance in its own instance definition, instead of the inner timeline. Most of these look pretty reasonable, but newEventWithLazyTriggerWithOnComplete, getPostBuild, and askQueryResult look suspicious. The rule I have is that there should be a coerce happening before the inner instance is called.

Adding type signatures to each call in the instance definition so that t is used instead of ProfiledTimeline t can be used to make sure these are correct.

[endgame]

I've taken your concerns seriously but believe there's nothing to do here. I checked each changed function in this PR by giving the lifted function a partial type signature (e.g., askQueryResult = coerce <$> lift (askQueryResult :: _)) and checking that GHC does not show ProfiledM or ProfiledTimeline in the inferred type.

askQueryResult and getPostBuild do not accept arguments, so the is nothing to coerce before passing to the lifted instance, and the function argument to newEventWithLazyTriggerWithOnComplete does not speak of m or t at all.

[endgame]

I understand your concern for other instances, but I don't see how it applies here. Why should f be coerced?

newEventWithLazyTriggerWithOnComplete :: ((a -> IO () -> IO ()) -> IO (IO ())) -> m (Event t a)
-- vs.
newEventWithLazyTriggerWithOnComplete :: ((a -> IO () -> IO ()) -> IO (IO ())) -> ProfiledM m (Event (ProfiledTimeline t) a)

I don't see how f changes.

[endgame]

Similarly:

getPostBuild :: m (Event t ())
-- vs.
getPostBuild :: ProfiledM m (Event (ProfiledTimeline t) ())

[endgame]

Note that this branch is still based off #400 where MonadQuery has a Monad superclass. Once #400 is merged, I'll rebase this so it can merge cleanly.

[endgame]

Rebased on develop and force-pushed to tidy up.

[cgibbard]

lgtm

[matthewbauer]

Looks good! Sorry @endgame I wasn't paying close enough attention to how those were setup, but can now confirm with GHCI as well that they are correct.