Lazy sampling allowing the removal of buildDynamic, introducing liftPushM

[parenthetical]

Reflex currently requires us to predict whether sampling the "now" value of a behavior operationally precedes the creation of the behavior in MonadFix instances. This makes it hard to implement high-level abstractions on top of Reflex. For example, a combination of DynamicWriter and a Reader of the result can lead to users running into mysterious "cycles in fixIO" errors just by changing the order of innocuous looking expressions in MonadFix. To keep laziness the library implementer would have to add buildDynamic style versions of their primitives, and teach the user how to use these.

This MR solves this issue and makes it so that sample in Spider matches the Pure semantics in MonadFix. This is achieved using unsafeInterleaveIO on the IORef read. The sampled values are forced to WHNF before event propagation to prevent incorrect results. If the values were left unforced the IORef might contain a future value instead of the one at the logical time of the sample:

 instance HasSpiderTimeline x => Reflex.Class.MonadSample (SpiderTimeline x) (EventM x) where
   {-# INLINABLE sample #-}
-  sample (SpiderBehavior b) = readBehaviorUntracked b
+  sample (SpiderBehavior b) = do
+    holdInits <- getDeferralQueue
+    res <- liftIO . unsafeInterleaveIO $ runBehaviorM (readBehaviorTracked b) Nothing holdInits
+    enqueueForce res
+    return res

The MR contains two commits. The first implements lazy sampling without adjusting the MonadHold class, while the second follows up and removes the no longer needed buildDynamic class member. To keep buildDynamic implementable, I've added a new function to MonadHold called liftPushM.

I was hoping the defaults would avoid a breaking change, but unfortunately the extra Reflex t constraint on the default implementation breaks downstream after all:

class MonadSample t m => MonadHold t m where
  ...
  -- | DEPRECATED. This function allowed for extra laziness when sampling
  -- behaviors, but is no longer needed. When implementing MonadHold use the new
  -- 'liftPushM' instead.
  buildDynamic :: PushM t a -> Event t a -> m (Dynamic t a)
  buildDynamic initialValue e = do
    iv <- liftPushM initialValue
    holdDyn iv e
  {-# INLINABLE liftPushM #-}
  liftPushM :: PushM t a -> m a
  default liftPushM :: (Reflex t) => PushM t a -> m a
  liftPushM m = sample . current =<< buildDynamic m never

Performance implications

If I'm interpreting things correctly the benchmarks didn't show any performance degradation, and running reflex-todomvc looks normal as well.

[Copilot]

Pull request overview

This PR updates Reflex’s Spider implementation to support lazy sample semantics compatible with MonadFix, and refactors the MonadHold API by removing buildDynamic in favor of a new liftPushM method (with buildDynamic retained as a deprecated helper).

Changes:

• Implement lazy sample for Spider using unsafeInterleaveIO, plus a new per-frame queue to force sampled thunks to WHNF before propagation.
• Remove buildDynamic from the MonadHold class API and introduce liftPushM to keep equivalent functionality expressible.
• Add micro tests covering the MonadFix sampling scenarios that previously could trigger “cycles in fixIO”.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
test/Reflex/Test/Micro.hs	Adds regression tests for MonadFix + sample ordering scenarios.
src/Reflex/Spider/Internal.hs	Implements lazy sampling + thunk-forcing queue; removes dynamic-init machinery tied to buildDynamic; adds liftPushM instances.
src/Reflex/Pure.hs	Refactors holdDyn/buildDynamic behavior for the Pure timeline; adds liftPushM.
src/Reflex/Profiled.hs	Updates MonadHold instance to work with liftPushM (and no longer implements buildDynamic).
src/Reflex/PerformEvent/Base.hs	Threads through the new liftPushM method for PerformEventT.
src/Reflex/Class.hs	Removes buildDynamic as a class method, adds liftPushM, adds default method signatures, and reintroduces buildDynamic as a deprecated helper.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[parenthetical]

I tried to make a non-breaking MonadHold change but unfortunately it's not possible due to having to introduce a Reflex t constraint.
There are two options:

• Merge only the first commit and deprecate buildDynamic later.
• Merge complete MR with breaking change. This would only affect people who have implemented MonadHold.

[maralorn]

This is very interesting but also quite intricate.

Would you mind giving a few examples of

• which code did not work before your change and works now
• and possibly which adjacent cases it does not fix?

[parenthetical]

I've added some new tests in the MR, copied below. Notice how x <- sample b comes before b <- hold "0" never. This is perfectly okay semantically (Reflex.Pure has no issue), but the default Spider implementation of Reflex didn't allow it.

This is not a problem you normally run into, until you do. The creation of the Behavior might be far removed from where the sampling happens, especially if these operations are happening within the implementation of some higher level library.

  , testB "sampling-monadfix" $ do
      rec
        x <- sample b
        b <- hold "0" never
      _ <- sample b
      return (x <$ b)
  , testB "sampling-monadfix-twice-removed" $ do
      rec
        e1 <- events1
        x <- sample b
        b' <- hold x e1
        b <- hold "0" e1
      _ <- sample b'
      return b'

Older tests do this:

  , testB "buildDynamicStrictness"  $ do
      rec
        d'' <- pushDyn return d'
        d' <- pushDyn return d
        d <- holdDyn "0" =<< events1

      _ <- sample (current d'')
      return (current d'')

where pushDyn is implemented using buildDynamic:

pushDyn :: (Reflex t, MonadHold t m) => (a -> PushM t b) -> Dynamic t a -> m (Dynamic t b)
pushDyn f d = buildDynamic (sample (current d) >>= f) (pushAlways f (updated d))

You have to do any sampling in the first argument of buildDynamic to avoid fixIO cycles.

As a user you'd need to "just know" that buildDynamic had to be used to solve the "cyclic evaluation in fixIO" error you got, if it's even something within your control.

I'm not sure if there are any combinations of hold and sample which still don't work. One thing pointing towards "no" is that test suite passes with buildDynamic implemented purely using sample and holdDyn. So the cases it was testing for are solved.

[parenthetical]

I've added a third commit switching to Spider's implementation of headE. This used to rely on slowHeadE due to lack of laziness ("TODO: Make this lazy in its input event").

The Spider headE now passes the test suite instead of having it hang forever, although I'm not 100% sure whether that's an accident of the test cases or wether the issue is fundamentally solved.