Opened 4 years ago

Closed 3 years ago

#10448 closed task (fixed)

Implement rest of "Add bifunctor related classes to base"-Proposal

Reported by: hvr Owned by:
Priority: normal Milestone: 8.2.1
Component: libraries/base Version:
Keywords: Cc: core-libraries-committee@…, RyanGlScott, hvr, ekmett
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case:
Blocked By: Blocking:
Related Tickets: #9682 Differential Rev(s): Phab:D2284
Wiki Page:

Description

Cloned from #9682:


See original libraries@ proposal for more details

Change History (12)

comment:1 Changed 4 years ago by ekmett

I'll happily set up the API changes in bifunctors to match what we want to have move into `base.

comment:2 Changed 4 years ago by thoughtpolice

Milestone: 7.12.18.0.1

Milestone renamed

comment:3 Changed 4 years ago by thomie

Owner: ekmett deleted

comment:4 Changed 4 years ago by RyanGlScott

Ed pointed out to me that one possible stumbling block in migrating Bitraversable to base is its current definition:

class (Bifunctor t, Bifoldable t) => Bitraversable t where
    bitraverse :: Applicative f => (a -> f c) -> (b -> f d) -> t a b -> f (t c d)
    bisequenceA :: Applicative f => t (f a) (f b) -> f (t a b)
    bimapM :: Monad m => (a -> m c) -> (b -> m d) -> t a b -> m (t c d)
    bisequence :: Monad m => t (m a) (m b) -> m (t a b)

Like Traversable, Bitraversable currently has two duplicate method signatures with stronger constraints (Monad instead of Applicative). We should probably aim to make bimapM and bisequence top-level definitions instead of class methods during the migration, but there has been a lingering question of whether this would affect runtime performance.

AFAICT, the consensus is that any remaining obstacles to redefining mapM = traverse have been overcome since #8189 was fixed, so is there any reason that we couldn't also redefine bimapM = bitraverse and bisequence = bisequenceA?

Other than that, I don't see any reason why we couldn't migrate Data.Bifoldable and Data.Bitraversable as they're implemented right now. One thing that was a concern for Foldable (#9621) was turning functions like toList into class methods for efficiency purposes, but I can't think of a Bifoldable instance that would benefit significantly from this, so we can probably leave that aside.

comment:5 Changed 4 years ago by ekmett

I support moving a minimal flavor of Bifoldable/Bitraversable in.

They don't have any legacy users to consider in terms of any bad interactions with different Monad's lack of implementation of (*>) vs. (>>). In fact I'd go so far as to remove bisequenceA, bimapM and bisequence from the class entirely in anticipation of doing the same to Traversable, and as a much smaller, low-impact laboratory for exploring that change.

comment:6 in reply to:  4 Changed 4 years ago by hvr

Differential Rev(s): Phab:D336

comment:7 Changed 4 years ago by RyanGlScott

Oh, I didn't realize that you wanted to remove bisequenceA from the Bitraversable class as well. What is your reasoning for that one? Is it because it's just bitraverse id id?

comment:8 Changed 4 years ago by ekmett

It is in the list of simplifications to consider for Traversable. If we can show it doesn't help measurably, it'd be a win for simplicity, and the (bi)sequenceA name that nobody likes can then eventually die in favor of (bi)sequence, whereas the other way there is no actual viable upgrade path that supports its removal.

comment:9 Changed 4 years ago by thomie

Milestone: 8.0.1

comment:10 Changed 3 years ago by RyanGlScott

Differential Rev(s): Phab:D2284
Status: newpatch

comment:11 Changed 3 years ago by Ben Gamari <ben@…>

In 270d545/ghc:

Add Bifoldable and Bitraversable to base

This adds `Data.Bifoldable` and `Data.Bitraversable` from the
`bifunctors` package to `base`, completing the migration started in
D336.  This is fairly straightforward, although there were a suprising
amount of reinternal organization in `base` that was needed for this to
happen:

* `Data.Foldable`, `Data.Traversable`, `Data.Bifoldable`, and
  `Data.Bitraversable` share some nonexported datatypes (e.g., `StateL`,
  `StateR`, `Min`, `Max`, etc.) to implement some instances. To avoid
  code duplication, I migrated this internal code to a new hidden
  module, `Data.Functor.Utils` (better naming suggestions welcome).

* `Data.Traversable` and `Data.Bitraversable` also make use of an
  identity newtype, so I modified them to use
  `Data.Functor.Identity.Identity`. This has a ripple effect on several
  other modules, since I had to move instances around in order to avoid
  dependency cycles.

Fixes #10448.

Reviewers: ekmett, hvr, austin, bgamari

Reviewed By: bgamari

Subscribers: thomie

Differential Revision: https://phabricator.haskell.org/D2284

GHC Trac Issues: #9682, #10448

comment:12 Changed 3 years ago by RyanGlScott

Milestone: 8.2.1
Resolution: fixed
Status: patchclosed
Note: See TracTickets for help on using tickets.