admit weaker typeclass instances for OptionT when the wrapped Functor…

[stew]

… is weaker

[xuwei-k]

👎

scalaz/scalaz#581

[codecov-io]

Current coverage is 75.92%

Merging #569 into master will increase coverage by +0.21% as of 2b0a2b8

@@            master    #569   diff @@
======================================
  Files          157     159     +2
  Stmts         2158    2185    +27
  Branches        68      68       
  Methods          0       0       
======================================
+ Hit           1634    1659    +25
  Partial          0       0       
- Missed         524     526     +2

Review entire Coverage Diff as of 2b0a2b8

Powered by Codecov. Updated on successful CI builds.

[non]

Lets try to add a law here that tests consistency between ap and flatMap-- I think that is the issue @xuwei-k is bringing up.

[xuwei-k]

I think there is the law here.

https://github.com/non/cats/blob/f69b3e19778/laws/src/main/scala/cats/laws/FlatMapLaws.scala#L18-L19

[non]

Ah, I see. In order to get the tests to fail on this change we'd have to create a situation where Applicative was not a Monad.

[xuwei-k]

• https://travis-ci.org/xuwei-k/cats/jobs/85504869#L1764
• xuwei-k@abece1292962582b50

[info] - OptionT[List, Int].monad.flatMap consistent apply *** FAILED *** (20 milliseconds)
[info]   GeneratorDrivenPropertyCheckFailedException was thrown during property evaluation.
[info]    (Discipline.scala:14)
[info]     Falsified after 20 successful property evaluations.
[info]     Location: (Discipline.scala:14)
[info]     Occurred when passed generated values (
[info]       arg0 = OptionT(List(Some(1676234460), Some(934633364), Some(2029976710), Some(830245382), Some(579257234), Some(1239180264), Some(28665143))),
[info]       arg1 = OptionT(List(None))
[info]     )
[info]     Label of failing property:
[info]       (OptionT(List(None, None, None, None, None, None, None)) ?== OptionT(List(None))) failed
[info]   org.scalatest.exceptions.GeneratorDrivenPropertyCheckFailedException:
[info]   at org.scalatest.prop.Checkers$.doCheck(Checkers.scala:403)

[non]

Thanks @xuwei-k.

[stew]

yes, thanks @xuwei-k !

[ceedubs]

@stew should the MonadCombine instance override ap to help catch this sort of issue in the future?

[stew]

@ceedubs I believe as it currently stands, the MonadCombine instance gets its ap definition from FlatMap:

https://github.com/non/cats/blob/master/core/src/main/scala/cats/FlatMap.scala#L29-L30

[ceedubs]

@stew right. I guess what I'm saying is that right now there is no test coverage on OptionT.ap. The laws will help ensure that MonadCombine[OptionT[F, ?]] has an ap that is consistent with its flatMap, but OptionT.ap has a separate implementation. Adding in an override seemed like an easy way to ensure the two were consistent. An alternative would be to add a test to ensure that they are consistent (though at that point it seems sort of silly to not just make the code guarantee that they are consistent). Does that make sense?

[stew]

I guess. I can tell you with near certainty that the ap method there, which I implemented with flatmap, will be consistent with flatmap.

Another option is to remove ap from OptionT. I'm not sure the circumstance when someone uses ap that isn't doing so from an applicative instance.

[ceedubs]

@stew Removing ap from OptionT is fine with me. I just think that if it's there then we should be testing it.

[ceedubs]

👍

[xuwei-k]

Why is this test here? (does not use OptionT)

[adelbertc]

👍