Don’t ignore mappend failures in validation

[matthewbauer]

Previously, a <> b would give success if either a or b were a success!
This should not happen when doing validation. We want any errors to be
propagated.

Fixes #35

/cc @tonymorris @gwils

[matthewbauer]

This case is still awkward. Why drop the right side here?

[eborden]

Why not?

instance (Semigroup e, Semigroup a) => Semigroup (Validation e a) where
  x <> y = liftA2 (<>) x y

instance (Semigroup e, Monoid a) => Monoid (Validation e a) where
  mempty = Success mempty

The currently left biased implementation using appValidation caused me all kinds of confusion when trying to foldMap a bunch of Validations. This is akin to the left biased Map implementation in containers that causes all kinds of woe elsewhere.

[chshersh]

It looks like the process of combining two values of the Validation type can have different semantics depending on what we want:

1. Output only the first error.
2. Combine all errors.
3. Return Sucess if there's at least one success.
4. Return Success if all are Success.

I wonder, would it be possible to reproduce all behaviours by using proper Semigroups with the instance proposed by @eborden? 🤔 Though we probably don't need to be able to have everything with Semigroup because there is also Alternative for Validation. However, this is a separate question, how Alternative for Validation should look like because it also can have multiple semantics 😃

[eborden]

1. Output only the first error.

This behavior seems very confusing to me. I would expect someone to use Either or toEither if they want this flavor of <>.

2. Combine all errors.

Yes please. This is satisfied by a liftA2 instance.

3. Return Success if there's at least one success.

This seems like a Dual instance.

4. Return Success if all are Success.

Yes please. This is also satisfied by a liftA2 instance.

[matthewbauer]

Why not?

instance (Semigroup e, Semigroup a) => Semigroup (Validation e a) where
  x <> y = liftA2 (<>) x y

instance (Monoid e, Monoid a) => Monoid (Validation e a) where
  mempty = Success mempty

This requires that the success side is a semigroup. That seems pretty rare case to me (and bound to be confusing). Looking at the examples, the success side is a semigroup only very rarely. In the cases where it is, I'm not sure if you want this. For instance:

(Success "abc") <> (Success "def") = Success "abcdef"

This works but seems surprising. I would much rather it just take one of the successes. But, I would agree it's probably better than the status quo.

[matthewbauer]

3. Return Success if there's at least one success.

This seems like a Dual instance.

This is actually how Either works. For instance:

Left "error" <> Right "success" = Right "success"

I suspect this is why validation implements semigroup that way. I would argue that it's wrong for Either as well and instead should be Left "error" to match the applicative instance for either:

Left "error" *> Right "success" = Left "error"

But either doesn't use liftA2 here. I wonder if there is any discussion on why that is?

[eborden]

Ugh, I forgot that was the instance for Either.

[matthewbauer]

I found this thread on Either's semigroup instance:

https://mail.haskell.org/pipermail/libraries/2018-May/028826.html

/cc @andrewthad

[andrewthad]

The Semigroup and Monoid instances for Either are currently bad to the point that I would consider them pathological. But whatever, it's hard to change things in base. For what it's worth, my favorite Semigroup instance for Validation is the one that @eborden suggests:

instance (Semigroup e, Semigroup a) => Semigroup (Validation e a) where

Why? It generalizes the other option:

instance Semigroup e => Semigroup (Validation e a) where

We can recover this second instance with Validation e (First a) if we really want it, using coerce for a zero-cost conversion between the two. And if users happen to be using Validation e (), then they both do the same thing anyway.

This requires that the success side is a semigroup. That seems pretty rare case to me (and bound to be confusing). Looking at the examples, the success side is a semigroup only very rarely. In the cases where it is, I'm not sure if you want this.

The example you bring up involving String/Text is a good example of when the combining behavior is annoying, but you run into this same problem if you use a string-like type as the exception type. There are plenty of examples where combining is useful or where it's useful that we get a compile-time error when we cannot combine things. What if you have a type like Validation [MyException] InformationAboutSuccess and you're combining these? Throwing away nearly all of your InformationAboutSuccess values seems like a bad idea. The fact that InformationAboutSuccess doesn't have a Semigroup instance is a strength here. It means the user gets a compile-time error that lets them know that they probably wanted to be using something like Validation [MyException] [InformationAboutSuccess], and if they really do want to throw about information, they can make that explicit with First or Last. Lifted the Semigroup instance of the success type lets us do cool things like:

• IO (Validation (Set IPv4) (Min Double, Max Double)) (hosts we couldn't reach or greatest and least latencies)
• IO (Validation [ErrorNote] (Set IPv4)) (stuff that went wrong or every host we discovered on the network)

Anyway, I don't particularly care what this library does since it's easy to just roll my own type whenever I need this, but that's my preference.

[chshersh]

Just in case you happen to use relude, it contains its own version of Validation with Functor, Applicative, Alternative, Semigroup and Monoid instances and provides different behaviour for max flexibility. A new version of relude released yesterday contains awesome documentation, a lot of examples, so you may find it useful 🙂

• http://hackage.haskell.org/package/relude-0.6.0.0/docs/Relude-Extra-Validation.html

[masaeedu]

@matthewbauer

This requires that the success side is a semigroup. That seems pretty rare case to me (and bound to be confusing).

Every type actually has a trivial semigroup which just throws away one argument. We can access this using the Const newtype wrapper. You can also further wrap it in Dual to throw away the other side unconditionally.

[matthewbauer]

@matthewbauer

This requires that the success side is a semigroup. That seems pretty rare case to me (and bound to be confusing).

Every type actually has a trivial semigroup which just throws away one argument. We can access this using the Const newtype wrapper. You can also further wrap it in Dual to throw away the other side unconditionally.

Yeah perhaps there are enough newtype's here that we don't need a semigroup instance at all.

I think pretty much any change is better than what it's currently doing, so feel free to open a PR doing something else. This is just the change that seems most logical and least likely to break things to me.