purescript-contrib / purescript-profunctor-lenses

Pure profunctor lenses
MIT License
144 stars 52 forks source link

Costrong too magical? Crash in the JS layer when using it. #120

Closed mikesol closed 3 years ago

mikesol commented 4 years ago

I've been digging into this (great!) repo and trying to create accumulating getters, meaning getters that can "pause" at a certain point of the lens, take a value, and persist it down to the final getter.

Rolling one by hand is pretty easy for a specific scenario, but coming up with a generic way to do this seems impossible. Nonetheless, the signature of Costar makes it seem like it's possible, as Costar can theoretically be used to "upgrade" a vanilla optic to an optic that takes a tuple (as I do below). However, it crashes with an odd message. The only crash I've ever seen in purescript before is maximum recursion depth (aka bottom), but this seems like a different beast...

I can also write this up in the purescript-profunctor library if the issue lies more there than here. Curious to hear everyone's thoughts.


Environment

Current behavior

This compiles fine but crashes when run:

module Main where

import Prelude
import Data.Lens (Iso, _1, _2, iso)
import Data.Profunctor (class Profunctor)
import Data.Profunctor.Costrong (class Costrong, unfirst, unsecond)
import Data.Profunctor.Strong (class Strong, first)
import Data.Tuple (Tuple(..), fst, snd)
import Effect (Effect)
import Effect.Console (log)
import Data.Identity (Identity(..))
import Data.Newtype (class Newtype, unwrap)
import Data.Profunctor.Costar (Costar(..))

newtype Forget' r a b
  = Forget' (a -> r)

derive instance newtypeForget' :: Newtype (Forget' r a b) _

instance profunctorForget' :: Profunctor (Forget' r) where
  dimap f _ (Forget' z) = Forget' (z <<< f)

instance strongForget' :: Strong (Forget' r) where
  first (Forget' z) = Forget' (z <<< fst)
  second (Forget' z) = Forget' (z <<< snd)

instance costrongForget' :: Costrong (Forget' r) where
  unfirst (Forget' z) = Forget' $ ((unwrap <<< unfirst <<< Costar) (\(Identity t@(Tuple x y)) -> Tuple (z t) y)) <<< Identity
  unsecond (Forget' z) = Forget' $ ((unwrap <<< unsecond <<< Costar) (\(Identity t@(Tuple x y)) -> Tuple x (z t))) <<< Identity

-- adds pathway for c to flow through in snd
firstAlley :: forall r s t a b c. (Forget' r a b -> Forget' r s t) -> Forget' r (Tuple a c) (Tuple b c) -> Forget' r (Tuple s c) (Tuple t c)
firstAlley o = first <<< o <<< unfirst

-- adds pathway for c to flow through in fst
secondAlley :: forall r s t a b c. (Forget' r a b -> Forget' r s t) -> Forget' r (Tuple a c) (Tuple b c) -> Forget' r (Tuple s c) (Tuple t c)
secondAlley o = first <<< o <<< unfirst

-- _1 with and arbitrary `c` flowing through snd
_1' :: ∀ r a b c d. Forget' r (Tuple a c) (Tuple b c) → Forget' r (Tuple (Tuple a d) c) (Tuple (Tuple b d) c)
_1' = firstAlley _1

-- _2 with and arbitrary `c` flowing through snd
_2' :: ∀ r a b c d. Forget' r (Tuple a c) (Tuple b c) → Forget' r (Tuple (Tuple d a) c) (Tuple (Tuple d b) c)
_2' = firstAlley _2

-- duplicates a value, pegging it as snd in a tuple
pegAtSecond :: forall a b. Iso a b (Tuple a a) (Tuple b a)
pegAtSecond = iso (\i -> Tuple i i) (\(Tuple f s) -> f)

main :: Effect Unit
main =
  log
    ( show
        ( ( unwrap
              ( (_2 <<< pegAtSecond <<< _2' <<< _2') -- lens
                  (Forget' fst) -- profunctor for fold
              )
          )
            (Tuple 0 (Tuple 1 (Tuple 2 3))) -- data structure
        )
    )

Yields:

purs compile: No files found using pattern: test/**/*.purs
[info] Build succeeded.
C:\Users\MikeSolomon\devel\random-scripts\free-monoid\output\Data.Tuple\index.js:64
    return v.value1;
             ^

TypeError: Cannot read property 'value1' of undefined
    at Object.snd (C:\Users\MikeSolomon\devel\random-scripts\free-monoid\output\Data.Tuple\index.js:64:14) 
    at C:\Users\MikeSolomon\devel\random-scripts\free-monoid\output\Data.Profunctor.Costar\index.js:98:59  
    at C:\Users\MikeSolomon\devel\random-scripts\free-monoid\output\Data.Identity\index.js:58:16
    at C:\Users\MikeSolomon\devel\random-scripts\free-monoid\output\Data.Profunctor.Costar\index.js:99:15  
    at C:\Users\MikeSolomon\devel\random-scripts\free-monoid\output\Main\index.js:56:20
    at C:\Users\MikeSolomon\devel\random-scripts\free-monoid\output\Main\index.js:33:16
    at C:\Users\MikeSolomon\devel\random-scripts\free-monoid\output\Main\index.js:29:16
    at C:\Users\MikeSolomon\devel\random-scripts\free-monoid\output\Main\index.js:20:24
    at C:\Users\MikeSolomon\devel\random-scripts\free-monoid\output\Main\index.js:33:16
    at Object.<anonymous> (C:\Users\MikeSolomon\devel\random-scripts\free-monoid\output\Main\index.js:90:226)
[error] Running failed; exit code: 1

Expected behavior

Not quite sure... definitely not a crash, but it's still unclear what the expected would be.

thomashoneyman commented 4 years ago

At a glance I'm not sure what the issue is. Pinging @MonoidMusician and @LiamGoodacre if they have any extra insight here. Regardless -- definitely a bug, thank you for reporting it!

MonoidMusician commented 4 years ago

Is it this line that is blowing up? https://github.com/purescript/purescript-profunctor/blob/9b3d014b38c69ea2dd4b1f597330b095034fe2c4/src/Data/Profunctor/Costar.purs#L62

It's a questionable instance because it relies on laziness to work – it doesn't work out mathematically. In particular, bd is defined by passing a value to f that depends on snd bd!

I'm trying to think if there's any possible instances where it works … it might be that PureScript is too strict. (Ideally the call to snd bd would be guarded under another lambda.) I guess it depends on how the Functor f instance works. If it's something lazy like (co)yoneda, and the function f does not inspect its argument at all, it could work. But that's almost never the case.

Anyways, it also doesn't work out for your example either. For Costrong you want a function (Tuple a c -> r) -> a -> r (desugared), but you cannot magically come up with a value of the arbitrary c type to pass to the f function. More particularly, (Tuple Unit Void -> Void) -> Unit -> Void is uninhabited in a consistent type theory.

mikesol commented 4 years ago

Yup, that's the line. I talked about it a bit on the purescript Slack as well. One course of action suggested by @LiamGoodacre is to delete the Costrong implementation in Costar. Conceptually, Costrong only makes sense if you have a type that is guaranteed to ignore mapping (ie Const) in a strict language, and in a lazy language, I'm assuming this translates to "guaranteed to never touch c in Tuple a c", which seems like a really severe constraint. Otherwise, I don't see how you could magically uncook the pizza and go from Tuple a c -> Tuple b c to a -> b.

thomashoneyman commented 3 years ago

@LiamGoodacre I'm curious about fixing this for the upcoming breaking release of this library. Do you have a suggested course of action we could take in a PR?

LiamGoodacre commented 3 years ago

It's not clear to me that there's a fix. Seems like something that can't/shouldn't exist. My suggestion is to delete the rogue instance. Seems unlikely that any code depends on it - any that does is likely also broken or unused. (Cochoice (Costar f) may also be suspect?).

thomashoneyman commented 3 years ago

Thanks, Liam. @mikesol, is there any chance you'd be interested in taking on a PR to remove that instance?

MonoidMusician commented 3 years ago

I agree that Cochoice (Costar f) is also dodgy and should be removed if we are removing Costrong (Costar f).

thomashoneyman commented 3 years ago

Fixed by https://github.com/purescript/purescript-profunctor/pull/38