reflex-frp/patch

Getting rid of `Data.Monoid.DecidablyEmpty`

endgame opened this issue · 6 comments

endgame commented

There's a comment at the top of Data.Monoid.DecidablyEmpty which reads: -- TODO upstream somwhere else?

That "somewhere else" already exists: Data.Monoid.Null from monoid-subclasses.

I had a look at the instances it provides, and it is missing the following ones:

instance MonoidNull a => MonoidNull (Identity a)
instance MonoidNull a => MonoidNull (WrappedMonoid a)
instance (Ord a, Bounded a) => MonoidNull (Max a)
instance (Ord a, Bounded a) => MonoidNull (Min a)
instance MonoidNull (Proxy a)
instance MonoidNull a => MonoidNull (Const a b)
instance MonoidNull a => MonoidNull (Down a)
instance MonoidNull p => MonoidNull (Par 1 p)
instance MonoidNull (U1 p)
instance MonoidNull (f p) => MonoidNull (Rec1 f p)
instance MonoidNull (f p) => MonoidNull (M1 i c f p)
instance MonoidNull c => MonoidNull (K1 i c p)
instance (MonoidNull (f p), MonoidNull (g p)) => MonoidNull ((f :*: g) p)
instance (MonoidNull (f p), MonoidNull (g p)) => MonoidNull ((f :.: g) p)
instance (...) => MonoidNull (a, b, c, d, e)

-- Should go to dependent-map
instance GCompare k => MonoidNull (DMap k v)

This would solve #42 but is too much work to hold up GHC 9.2 support. Tasks, as I see them:

  • Add the missing instances to monoid-subclasses and release
  • Add the missing instance to dependent-map and release
  • Replace Data.Monoid.DecidablyEmpty.DecidablyEmpty with type alias, set isEmpty = Data.Monoid.Null.null, deprecate both, and release
  • Remove deprecated Data.Monoid.DecidablyEmpty.

None of this seems particularly controversial. We might want monoid-subclasses anyway because of #37. It's quite a light package, so depending on it from dependent-map shouldn't be a problem either.

endgame commented

@Ericson2314 Let me know if you're happy with this plan, and I will begin making PRs.

Ericson2314 commented

@endgame this one I have more mixed feelings about because monoid-subclasses is such overkill for our use-case.

Ericson2314 commented

Also, if we we had more "non-empty" data types, like haskell/containers#616, we could do this in a more "type directed" way which would be more satisfying.

(If you have spare cycles, I think they would be more usefully spent getting haskell/containers#616 over the finish line. I hope the conflicts are not too bad! The only thing remaining before that I remember was dealing with some perf regressions, which I think is just a matter of tweaking the inlining.)

endgame commented

@endgame this one I have more mixed feelings about because monoid-subclasses is such overkill for our use-case.

It's not that big a package, and its library component has a pretty reasonable dependency footprint. Regardless, I opened blamario/monoid-subclasses#50 .

  • 👍1
ryantrinkle commented

@endgame This is awesome, thank you!

endgame commented

Since I got the github notification, I thought I'd do dependent-map while I had a couple of minutes.