shiftMap is broken

[ajnsit]

This place where I used shiftMap is broken - https://github.com/ajnsit/purescript-concur/blob/master/src/Concur/React.purs#L41.

That code assumes a

forall a. (s a -> s a) -> t a -> t a

instead of

(forall a. s a -> s a) -> (forall a. t a -> t a)

I overrode the compiler error with unsafeCoerce thinking it was safe, but this breaks at runtime whenever the a gets specialised to something other than just a.

Either the class ShiftMap should be changed to have shiftMap :: forall a. (s a -> s a) -> t a -> t a, though that might not be implementable for everything (for example I don't think it's possible to write an instance for Signals anymore). Or this might be time to reach out for MonadControl (Not sure if that would solve the issue) or another abstraction (MonadBi ?).

With MonadBi s t we can write something like the following, which might work -

shiftMap :: MonadBi s t. s ~> s -> t ~> t
shiftMap f t = (raise <<< f) =<< lower t

[ajnsit]

Thinking about this more, the view model needs an overhaul to be completely safe. I need to be able to specify a view modification function that depends on the type of result returned by the view.

I think implementing #14 would also fix the view model, and would be the right way to go about it.

[pkamenarsky]

Am I correct in understanding that while theoretically unsafe the el* functions form a safe wrapper around shiftMap? I.e. user code shouldn't be affected at all insofar one doesn't resort to manual use of shiftMap?

[ajnsit]

Unfortunately the el* functions are also unsafe. They use shiftMap (in an unsafe way) to allow using monad transformer stacks with the dom creation methods. However, explicitly lifting widgets into the monad transformer is safe.

For example, writing a stateful widget like this is safe -

counterWidget :: forall a. StateT Int (Widget HTML) a
counterWidget = forever do
  count <- get
  -- This is safe
  -- We use lift so the dom creation methods run in `Widget HTML` monad
  lift $ D.div'
    [ D.text  "Current count is: " <> (show count)
    , D.button [P.onClick] [D.text "Increment count"]
    ]
  put (count + 1)

However the following is unsafe. This particular example works, but something like this could fail at runtime. I still need to figure out the exact condition that would cause it to fail.

counterWidget :: forall a. StateT Int (Widget HTML) a
counterWidget = forever do
  count <- get
  -- This is unsafe
  -- We don't use lift so the dom creation methods run in `StateT Int (Widget HTML)` monad
  D.div'
    [ D.text  "Current count is: " <> (show count)
    , D.button [P.onClick] [D.text "Increment count"]
    ]
  put (count + 1)

[ajnsit]

All right, after testing a few designs, I settled on -

-- | Avoiding monad-control for as long as possible
class ShiftMap s t where
  shiftMap :: forall a. (forall b. (a -> b) -> s b -> s b) -> t a -> t a

Now we don't have unsafeCoerce and everything seems to work. Will continue to test.

[pkamenarsky]

Need to think this through in more detail, but I'm still not sure how one could use el* unsafely.

el ::
  forall m a.
  ShiftMap (Widget HTML) m =>
  NodeTag ->
  Array (Props a) ->
  m a ->
  m a

Isn't the forall m a there constraining a to be the same in both rank N types here (forall a. s a -> s a) -> (forall a. t a -> t a) in all cases?

[ajnsit]

That was my reasoning too when I wrote this. But the result of ‘el’ is a red herring as we have already used ‘unsafeCoerce’ by then. Instead think about the function being passed to ‘shiftMap’. This function only works for a specific ‘a’ (constrained by the type of props passed). However the implementation of ‘shiftMap’ for a specific monad is free to use that function for other ‘a’. For example, the implementation for ‘StateT’ uses it as a ‘m (Tuple s a) -> m (Tuple s a)’. Infact only the implementation for ‘ReaderT’ was actually safe.

The lesson here is that no matter how improbable it seems, if the compiler thinks something is unsafe, it will fail at runtime at some point 😀

[pkamenarsky]

Ah I see, now it makes sense!

With your new design, wouldn't the implementation for StateT need to create a Tuple s a from just an a? I.e. s would have to be a Monoid or something?

[ajnsit]

No because it can use the state that was passed in initially. See the implementation for the various transformers in ShiftMap.purs