@phadej thanks for catching the Traversal/Setter difference and the unlawfulness. I buy that adjoin is potentially unlawful for non-disjoint Traversals (and I agree with your comments wrt "unsound" being too strong). But is there an example of breaking the laws for Setter alone? I can't immediately see one, because Setter doesn't give you a way to observe how many times the update function is applied.

@adamgundry you can break over setter f . over setter g = over setter (f . g) law quite easily.

Prelude Optics Optics.Monoidal Data.Char> let setter = castOptic @A_Setter simple <++> castOptic @A_Setter simple
Prelude Optics Optics.Monoidal Data.Char> over setter (+1) . over setter (*2) $ (3 :: Int)
14
Prelude Optics Optics.Monoidal Data.Char> over setter ((+1) . (*2)) $ (3 :: Int)
15

This is similar to what I tried to do in #91 (comment)

Combination of results with read-only optics works out because you can discard the structure you traverse, so *> does the job fine, but the setter part needs to preserve the structure of traversed data and I suspect it cannot be done in general, so it simply won't work for read-write optics.

What incrLevels does when used as a traversal? I suspect it doesn't work as advertised.

I have just pushed a new attempt, which satisfies the traversal laws if the traversals focus on different parts of the value.

*Optics Optics.Monoidal Data.Char> data BinaryTree a = Leaf { _leaf :: a } | Node { _left, _right :: BinaryTree a } deriving Show
*Optics Optics.Monoidal Data.Char> makeLenses ''BinaryTree;
*Optics Optics.Monoidal Data.Char> let preOrder :: Traversal' (BinaryTree a) a; preOrder = leaf <++> (left % preOrder) <++> (right % preOrder)
*Optics Optics.Monoidal Data.Char> toListOf preOrder $ Node (Leaf 'x') (Node (Leaf 'y') (Leaf 'z'))
"xyz"
*Optics Optics.Monoidal Data.Char> over preOrder toUpper $ Node (Leaf 'x') (Node (Leaf 'y') (Leaf 'z'))
Node {_left = Leaf {_leaf = 'X'}, _right = Node {_left = Leaf {_leaf = 'Y'}, _right = Leaf {_leaf = 'Z'}}}

Note that my end goal is also to provide a generalized interface to failing, which does not only work on Fold. So for example you could compose two affine traversals and that would focus on the first one that matches.

I'm not convinced that introducing a type class for this is a good idea. The only non-trivial instances currently are for Traversal and Setter, both of which come with a set of caveats. In addition, pure setters are somewhat rare.

Since split and After instance for Traversal are already in lens under lensProduct and adjoin, I'd be inclined to include them under these names in optics and avoid unnecessary api differences.

As a side note, I'm curious how well adjoin optimizes... @serras perhaps you could add an inspection test to Optics.Tests.Misc that at least profunctor classes optimize away?

After some experimentation I got this. Seems to optimize very well (existing version that calls partsOf also optimizes well for small examples, but this one has much less code after inlining takes place so should compile faster and potentially optimize better for non-trivial cases).

adjoin
  :: (Is k A_Traversal, Is l A_Traversal)
  => Optic' k is s a
  -> Optic' l js s a
  -> Traversal' s a
adjoin o1 o2 = combined % traversed
  where
    combined = lensVL $ \f s -> evalState (    traverseOf o1 update s
                                           >>= traverseOf o2 update)
      <$> f (   toListOf (castOptic @A_Traversal o1) s
             ++ toListOf (castOptic @A_Traversal o2) s)

    update a = get >>= \case
      a' : as' -> put as' >> pure a'
      []       ->            pure a
{-# INLINE adjoin #-}

-- indexed variant
iadjoin
  :: (Is k A_Traversal, Is l A_Traversal, is `HasSingleIndex` i)
  => Optic' k is s a
  -> Optic' l is s a
  -> IxTraversal' i s a
iadjoin o1 o2 = conjoined (adjoin o1 o2) (combined % traversed % itraversed)
  where
    combined = lensVL $ \f s -> evalState (    traverseOf o1 update s
                                           >>= traverseOf o2 update)
      <$> f (   itoListOf (castOptic @A_Traversal o1) s
             ++ itoListOf (castOptic @A_Traversal o2) s)

    update a = get >>= \case
      (_, a') : as' -> put as' >> pure a'
      []            ->            pure a
{-# INLINE iadjoin #-}

One more argument against the type class: things like _1 <++> _2 don't type check, whereas adjoin _1 _2 does, because adjoin accepts anything that's a traversal, whereas <++> accepts a pair of traversals or a pair of folds etc.

That's the reason we have summing and asumming instead of having summing as a class method.

@arybczak thanks for looking at this. I'd be happy with lensProduct and adjoin for lens compatibility. I don't have a strong sense of whether it is worth offering a version for Setter.

We should have "Monoid structure" sections of the Optics.[Ix](Setter|Traversal) docs, and perhaps an Optics.Monoidal module that documents all these monoids, whether or not it actually exposes them as a type class.

Given this discussion, I am going to close the PR, since clearly is should not be merged as is. Let me know if I could help bringing the other operators to optics, as I find them quite useful, even though they might not be "safe" unless you point to different parts of a value.