haskell / core-libraries-committee

96 stars 16 forks source link

Set `Data.List.NonEmpty.map = fmap` #300

Open mpilgrem opened 2 days ago

mpilgrem commented 2 days ago

Motivation: eliminate existing code duplication.

In base, Data.List.NonEmpty.NonEmpty is, ultimately, a re-export from ghc-internal of GHC.Internal.Base.NonEmpty. GHC.Internal.Base also defines an instance of Functor, namely:

-- | @since base-4.9.0.0
instance Functor NonEmpty where
  fmap f ~(a :| as) = f a :| fmap f as
  b <$ ~(_ :| as)   = b   :| (b <$ as)

Data.List.NonEmpty exports map, defined as:

-- | Map a function over a 'NonEmpty' stream.
map :: (a -> b) -> NonEmpty a -> NonEmpty b
map f ~(a :| as) = f a :| fmap f as

To eliminate code duplication, I propose that should be:

-- | Map a function over a 'NonEmpty' stream.
map :: (a -> b) -> NonEmpty a -> NonEmpty b
map = fmap
mixphix commented 1 day ago

This would be the exact opposite of the current status quo for [] in ghc-internal:

-- | @since base-2.01
instance Functor [] where
    {-# INLINE fmap #-}
    fmap = map

map :: (a -> b) -> [a] -> [b]
map _ []     = []
map f (x:xs) = f x : map f xs
mpilgrem commented 21 hours ago

@mixphix, I think that [] and NonEmpty can be distinguished because [] and map feature in the Haskell 2010 Language Report but NonEmpty does not. Section 9 has:

instance Functor [] where
fmap = map

-- Map and append
map :: (a -> b) -> [a] -> [b]
map f [] = []
map f (x:xs) = f x : map f xs
phadej commented 19 hours ago

I usually tend to do fmap = map, rather then opposite. (I cannot think when I'd do otherwise). For example, IMHO. it's more natural to say that list has a Monad instance with return = singleton and (>>=) = concatMap rather than to define singleton = return and concatMap = (>>=). (Especially if there are multiple possible instances; not the case for Functor though). Sometimes specific combinators may also have less constraints than an instance member (of a multi-member type-class). I'm also quite sure that fmap = map way is a must for [] for rewrite RULES to work, not the case for NonEmpty as it doesn't have any RULES, but it could.

mpilgrem commented 17 hours ago

Below, I've set out a comparison of List a and NonEmpty a.

List a NonEmpty a
package ghc-prim
* Defines type List a = [] : List a
package ghc-internal (depends on ghc-prim) package ghc-internal
Defines type NonEmpty a = a :\| [a]
* Defines Functor instance Defines Functor instance
* Defines map
* fmap = map * fmap = <source code>
package base (depends on ghc-internal) package base (depends on ghc-internal)
* Re-exports above * Re-exports above
* Defines map
* map duplicates fmap source code

Another approach to eliminating source code duplication would be to move the source code for map :: (a -> b) -> NonEmpty a -> NonEmpty b out of the base package into the ghc-internal package and have base re-export it.