Skip to content

Emphasize similarities between traversalVL and foldVL #427

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
arybczak wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Emphasize similarities between traversalVL and foldVL #427

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions optics-core/src/Optics/Fold.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,14 +117,17 @@ import Optics.Internal.Utils
-- | Type synonym for a fold.
type Fold s a = Optic' A_Fold NoIx s a

-- | Obtain a 'Fold' by lifting 'traverse_' like function.
-- | Construct a 'Fold' from a 'traverse_' like function.
--
-- /Note:/ for lifting a 'Data.Traversable.traverse' like function see
-- 'Optics.Traversal.traversalVL'.
--
-- @
-- 'foldVL' '.' 'traverseOf_' ≡ 'id'
-- 'traverseOf_' '.' 'foldVL' ≡ 'id'
-- @
foldVL
:: (forall f. Applicative f => (a -> f u) -> s -> f v)
:: (forall f. Applicative f => (a -> f b) -> s -> f ())
-> Fold s a
foldVL f = Optic (foldVL__ f)
{-# INLINE foldVL #-}
Expand Down
9 changes: 7 additions & 2 deletions optics-core/src/Optics/Traversal.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,13 +124,18 @@ type TraversalVL s t a b = forall f. Applicative f => (a -> f b) -> s -> f t
-- | Type synonym for a type-preserving van Laarhoven traversal.
type TraversalVL' s a = TraversalVL s s a a

-- | Build a traversal from the van Laarhoven representation.
-- | Construct a 'Traversal' from a 'traverse' like function.
--
-- /Note:/ for lifting a 'Data.Foldable.traverse_' like function see
-- 'Optics.Fold.foldVL'.
--
-- @
-- 'traversalVL' '.' 'traverseOf' ≡ 'id'
-- 'traverseOf' '.' 'traversalVL' ≡ 'id'
-- @
traversalVL :: TraversalVL s t a b -> Traversal s t a b
traversalVL
:: (forall f. Applicative f => (a -> f b) -> s -> f t)
Copy link
Contributor

@phadej phadej Jun 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this. We have unfolded FoldVL definition for some reason (and it's not correct). Yet we very much could, as Contravariant is in base since 4.12 (we'd need to drop ghc-8.2 support, that's imo fine). We also use synonyms in optics-vl and in Optics.Lens.

Copy link
Collaborator Author

@arybczak arybczak Jun 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by "it's not correct"?

Copy link
Collaborator Author

@arybczak arybczak Jun 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think you mean that it's not how Fold from lens looks like.

This is fine. Contravariant in Fold from lens is a quirk emerging from their implementation. We don't need Contravariant because we have Bicontravariant internally to do the same thing it does, just behind the scenes.

You mentioned in #426 that foldVL traverse works, but that's just because foldVL type is too permissive. I actually changed it here so that foldVL traverse no longer works, but foldVL traverse_ works just fine.

Copy link
Contributor

@phadej phadej Jun 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not fine. I cannot convert lens Fold into optics Fold using current, or new foldVL.

import qualified Control.Lens as L
import qualified Optics as O

ex1 :: Foldable f => L.Fold (f a) a
ex1 = L.folding id

ex2 :: Foldable f => O.Fold (f a) a
ex2 = O.foldVL ex1

{-

fold.hs:8:16: error:
    • Could not deduce (L.Contravariant f1) arising from a use of ‘ex1’
      from the context: Foldable f
        bound by the type signature for:
                   ex2 :: forall (f :: * -> *) a. Foldable f => O.Fold (f a) a
        at fold.hs:7:1-35
      or from: Applicative f1
        bound by a type expected by the context:
                   forall (f1 :: * -> *).
                   Applicative f1 =>
                   (a -> f1 a) -> f a -> f1 (f a)
        at fold.hs:8:7-18
      Possible fix:
        add (L.Contravariant f1) to the context of
          a type expected by the context:
            forall (f1 :: * -> *).
            Applicative f1 =>
            (a -> f1 a) -> f a -> f1 (f a)
    • In the first argument of ‘O.foldVL’, namely ‘ex1’
      In the expression: O.foldVL ex1
      In an equation for ‘ex2’: ex2 = O.foldVL ex1
  |
8 | ex2 = O.foldVL ex1

-}

We can have the foldVL-like introduction form, but it cannot be named foldVL, that is misleading.

Copy link
Collaborator Author

@arybczak arybczak Jun 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mentioned at least once (#179 and #183) before the initial release the discrepancy you're pointing and we agreed it was fine.

It seems it's just a case of insufficient documentation.

In any case foldVL can't "easily" take Contravariant because wander doesn't.

It seems we just need to communicate the fact that foldVL is not compatible with lens. I had a patch lying around, so if you really want this, I rebased and pushed it: #430.

Copy link
Contributor

@phadej phadej Jun 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's not much discussion in #183, and in light of new knowledge I think it wasn't that good of the move. mkFold is not great name either, but foldVL is simply wrong and with #430 in it would be misleading and confusing.

Copy link
Collaborator Author

@arybczak arybczak Jul 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to resolve this situation, otherwise these two PRs (this and #430) will keep hanging.

Let's just rename foldVL in Optics.Fold back to mkFold. Yes, it's not a great name, but it's better to rename it than be in the current situation.

Unless you have propositions for a better name 🙂

-> Traversal s t a b
traversalVL t = Optic (wander t)
{-# INLINE traversalVL #-}

Expand Down