Applicative / Monad instances for Result are invalid

[bitemyapp]

{-# LANGUAGE TemplateHaskell #-}

module Main where

import           Control.Monad
import           Data.DeriveTH
import           Test.Hspec
import           Test.Hspec.QuickCheck           (prop)
import           Test.QuickCheck hiding (Result)
import           Test.QuickCheck.Checkers
import           Test.QuickCheck.Classes

import Text.Reform
import Text.Reform.Result

$(derive makeArbitrary ''FormId)
$(derive makeArbitrary ''FormRange)
$(derive makeArbitrary ''Result)

instance (Eq e, Eq a) => EqProp (Result e a) where (=-=) = eq

main :: IO ()
main = hspec $ do
  describe "Applicative/Monad instances" $ do
    it "Applicative and Monad should match behavior" $ do
      let result :: Result String (Int, String)
          result = Ok (1, "blah")
      quickBatch $ monadApplicative result

  ap:   *** Failed! Falsifiable (after 2 tests):  
Error []
Error [(FormRange -fval[0] -fval[0],"\249")]

[Icelandjack]

This is the Validation [(FormRange, e)] Applicative (aka Lift (Const [(FormRange, e)])), these two instances can't agree by their very nature.

[Icelandjack]

You only need a Semigroup so Result (NonEmpty (FormRange, e)) is valid too.

[stepcut]

I implemented these instance back when Applicative was new and it was not so clear that the instances for Applicative and Monad needed to match. Unfortunately, making the match is a bit of a step backwards.

The idea is that when you use only the Applicative instance -- you can collect all the errors that occur in a form.

If you use the Monad instance, then you can only get the first error, since validating later form elements could depend on the results of earlier elements.

I am not quite sure what to do expect perhaps drop the Monad instance. A form library that can only report one invalid field at a time seems pretty useless?

Or -- maybe there is some way to have my cake and eat it too?

I don't quite understand the Semigroup solution -- would it help?

I added the test suite on the new bifunctors branch which uses the bifunctor library. For some reason it exits with exit code 0 even when it encountered errors :-/

[bitemyapp]

@stepcut I understand why you want this. I've had this exact conversation with Ed about EitherT and Validation in the past too. He was right and I was wrong - you should not introduce a law-violating instance.

Or -- maybe there is some way to have my cake and eat it too?

Drop the Monad, make combinators/helpers. Any code working with this library is likely to be concrete in terms of Result anyway. The invalid Monad instance is a useless trap.

Alternately, do like validation and offer isos between the two types, but I think my first reco suits this better.

[Icelandjack]

I don't quite understand the Semigroup solution -- would it help?

It won't help you get a Monad / Applicative instances that match, it only helps if you don't want empty errors.

Side comment: If you want a non-empty list of errors you cannot define it in terms of Lift.

data Lift f a = Pure a | Other (f a)

Sadly the Applicative f => Applicative (Lift f) instance is overly restrictive. It doesn't require the pure method at all using instead using the Pure :: a -> Lift f a constructor.

All it really needs is an Apply f => ... constraint.

When used as Lift (Const err) this means we would like a Semigroup err constraint instead of the overly restrictive Monoid err required by Applicative (Lift (Const err)).

[Icelandjack]

If you need Monad you can — giving up on collecting multiple errors.

If you only want Monad for do-notation use ApplicativeDo! A lot of functions from Control.Monad like (*>), forever, filterM, replicateM, when only require Applicative.