Bump ScalaCheck to 1.13.1, and fix the fallout. #1345

non · 2016-09-01T17:14:36Z

Status

Get PR building correctly
Decide what to do about MonadCombine laws
Fix cryptic errors in the js subproject

Summary

This commit gets all our law tests working with a new, more robust
ScalaCheck version. It currently relies upon an unreleased Discipline
snapshot, so it definitely won't build in Travis.

There were a bunch of test bugs that got fixed, mostly around
foldRight/reduceRight consistency for non-empty data structures.

One law violation was uncovered: MonadCombine[Option] doesn't pass the
left-distributivity law we wrote. We should figure out whether this
represents a real law violation and remove the instance, or whether
the law is too strict somehow.

One counter-example is:

def f(x: Int): Option[Int] = if (x == 0) None else Some(x)
val a = Option(0)
val b = Option(1)
(a <+> b).flatMap(f) != (a.flatMap(f) <+> b.flatMap(f))

Review by @adelbertc et al. We will need to wait for a new Discipline
release before merging this, and we'll need to do something about the
MonadCombine laws/instances (currently the MonadCombine[Option] test
is commented out).

non · 2016-09-01T17:16:40Z

I should also note that I commented out some other implicits that we can probably remove. I definitely need to do a sweep through this PR before we consider merging it, but I wanted to get it up now to show people how things work.

We also probably want to add Cogen instances for all our data types: I did the minimal set necessary to get our current tests running again. We could also stand to do some renaming, since cats.laws.discipline.arbitrary is no longer a totally correct name (it also contains Cogen instances).

non · 2016-09-02T22:59:38Z

Hit another cryptic issue, with the js subproject. It seems to be building against the wrong version of ScalaCheck, and then crashing during one of the JS linking steps.

[info] Compiling 1 Scala source to /Users/erik/w/cats/js/target/scala-2.11/test-classes...
[info] Fast optimizing /Users/erik/w/cats/js/target/scala-2.11/cats-js-test-fastopt.js
[error] Referring to non-existent method org.scalacheck.Arbitrary$.arbFunction1(org.scalacheck.Arbitrary)org.scalachec$
.Arbitrary
[error] called from cats.js.tests.FutureTests.()
[error] called from cats.js.tests.FutureTests.__exportedInits
[error] exported to JavaScript with @JSExport
[error] involving instantiated classes:
[error] cats.js.tests.FutureTests

I was able to work around this, but it's really unnerving. I pushed the workaround to get the PR to turn green, but ideally we should fix this before merging.

I'm going to create a checklist in the PR to try to track these deferred issues.

non · 2016-09-02T23:19:01Z

I was wrong -- all of the tests seem to be broken under scala.js. I'm going to put this down and come back to it. I should probably download some ScalaCheck jars to be sure that they look OK.

non · 2016-09-03T15:40:27Z

I still haven't figured out what is happening here. I tried on a different computer and was able to test some of these sub-projects but encountered the error in another place (free). I've tried cleaning (and manually nuking target) but none of these have helped.

I also asked in the Scala.js Gitter and no one had any ideas.

I'm a bit stumped. If anyone is looking for a frustrating thing to work on this weekend I'd appreciate some help. Even just checking out this branch and running validate yourself might give us another data point.

(I'm going to be out of town for a couple days, so I may not be as responsive as I have been recently.)

durban · 2016-09-03T22:07:07Z

I've sent a pull request, please take a look (direct link to the commit: non@191e51f). With this commit, validate succeeds for me.

I'm not exactly sure what's actually happening, but an educated guess is the following:

yes, JS projects (at least some of them) use scalacheck 1.12.5
JVM projects doesn't
both kinds of projects use the doctest sbt plugin
if doctestWithDependencies is true (it seems to be the default), the doctest plugin directly modifies libraryDependencies, and adds scalacheck 1.12.5 (and I guess this has a higher priority than the transitively used scalacheck 1.13)
for JVM projects, this setting is false
for JS projects, also setting it to false seems to solve the version mismatch
(I also did some other modifications to make it compile; also there was a typo in build.sbt)

I hope this helps :-)

non · 2016-09-04T04:05:16Z

@durban Thanks so much!!!

I merged this to get Travis started but I'm also going to run it locally. The explanation seems plausible, and might explain why the problems are somewhat unpredictable.

codecov-io · 2016-09-04T04:41:11Z

Current coverage is 91.73% (diff: 76.92%)

Merging #1345 into master will decrease coverage by 0.02%

@@             master      #1345   diff @@
==========================================
  Files           238        238          
  Lines          3579       3594    +15   
  Methods        3508       3528    +20   
  Messages          0          0          
  Branches         70         65     -5   
==========================================
+ Hits           3284       3297    +13   
- Misses          295        297     +2   
  Partials          0          0

Powered by Codecov. Last update 10b80df...91fdbbf

ceedubs · 2016-09-05T15:45:06Z

Thanks a bunch, @non. This is great. I'm wondering if @fthomas has any thoughts on the law violation?

non · 2016-09-06T05:07:07Z

For now I've elected to remove the left distributivity law from MonadCombine. Would love a review by @fthomas, but I think if no one objects and we can get the sign-offs we should just go for it. I think this PR is ready to go (also I can rebase if people think that would be useful).

If anyone feels strongly I'm also fine with downgrading MonadCombine[Option] to MonoidK[Option] and MonadFilter[Option].

non · 2016-09-06T05:42:10Z

(Sorry for all this test noise. I will probably rebase the last few commits here, since they basically just consist of me using Travis to do testing.)

@adelbertc

This commit gets all our law tests working with a new, more robust ScalaCheck version. It currently relies upon an unreleased Discipline snapshot, so it definitely won't build in Travis. There were a bunch of test bugs that got fixed, mostly around foldRight/reduceRight consistency for non-empty data structures. One law violation was uncovered: MonadCombine[Option] doesn't pass the left-distributivity law we wrote. We should figure out whether this represents a real law violation and remove the instance, or whether the law is too strict somehow. One counter-example is: def f(x: Int): Option[Int] = if (x == 0) None else Some(x) val a = Option(0) val b = Option(1) (a <+> b).flatMap(f) != (a.flatMap(f) <+> b.flatMap(f)) Review by @adelbertc et al. We will need to wait for a new Discipline release before merging this, and we'll need to do something about the MonadCombine laws/instances (currently the MonadCombine[Option] test is commented out).

Also remove various obsolete/commented code.

adelbertc · 2016-09-06T07:35:39Z

If I recall @mpilquist had knowledge around weak/strong laws, might the left-distributivity law of MonadCombine be related?

non · 2016-09-06T16:24:34Z

I don't think we should worry about the coverage errors here. AFAIK the uncovered parts of the patch are cogen instances I created for some of our data types. I thought they were needed (but maybe weren't used) but I think we should include these anyway.

eed3si9n · 2016-09-07T17:45:31Z

+1 on removing monadCombineLeftDistributivity. A useful law to add might be enforcing empty added by MonadFilter matches up with that of MonoidK if you don't want to remove MonadFilter altogether in #1342.

non · 2016-09-07T17:47:11Z

@eed3si9n Agreed on that! I think we get this for free since we're also checking both the MonadFilter laws and MonoidK laws using the same type class instance. (If it turns out I'm wrong we should add that explicitly.)

eed3si9n · 2016-09-07T18:26:48Z

@non They both introduce empty, but I don't think any laws are checking that those values match up for a datatype.

non · 2016-09-07T18:36:08Z

@eed3si9n One of us must be confused. I'll try to lay out my reasoning to figure out where the confusion lies:

MonadFilter and MonoidK are both traits with a def empty[A]: F[A] declaration.
Instances of MonadCombine (e.g. MonadCombine[Option]) extend both of those traits and define a concrete empty method.
MonadFilter and MonoidK each have a laws trait which requires that certain laws hold for its empty member.
The laws trait for MonadCombine extends both of the above law traits, so it checks that each of their laws hold for the MonadCombine instance (and its concrete empty method) being checked.
We check the laws for our instances (e.g. MonadCombine[List]) which includes those tests above.

Am I missing something? It seems like we're already ensuring consistency through the law-checking, right?

eed3si9n · 2016-09-07T18:57:15Z

@non For clarification, let's call the MonodK's empty fzero.

Here's my logic:

Given a datatype F[_], MonoidK[F], MonadFilter[F], and MonadCombine[F] can be implemented as distinct stack of traits that ends up as distinct implicit values.
Within each instances, the empty method (and fzero method) can be implemented in accordance of the law.
It's conceivable that MonoidK[F].fzero and MonadFilter[F].empty have different semantics for some datatypes (This comment supports this idea; right-bias Either could be such example?)
Since MonadCombine brings two hierarchies together, I think the emptiness should be consistent for datatypes that support it, i.e. fzero == empty should be a law.

It's completely possible that I'm confused.

non · 2016-09-07T18:58:50Z

@eed3si9n If the method was called fzero then I totally agree with you. My sense was that because the same method name is used in MonadFilter and MonoidK, it's not possible to have two different "versions" of empty that end up getting used for the same MonadCombine instance, due to the way method overriding works. Can that happen?

eed3si9n · 2016-09-07T19:24:01Z

So you're saying that the implementation of emptiness would never diverge because no one would define instances like this in real life?

implicit def instancesForEither[A1: Monoid]: MonadCombine[Either[A1, ?]] with Monad[Either[A1, ?]] with Alternative[Either[A1, ?]] {
  def empty[A2]: Either[A1, A2] = Left(???)
  ....
}
implicit def instancesForEither2[A1]: MonadFilter[Either[A1, ?]] {
  def empty[A2]: Either[A1, A2] = Right(???)
  ....
}

non · 2016-09-07T19:30:36Z

@eed3si9n Right. Or at least, defined that way there would be ambiguous implicit problems.

Now I understand the issue you're pointing out. I think it's a valid concern but it's a lot deeper than just this case: for example, what if there were both Functor and Applicative instances that had different map methods? For now, we rely on implementors to ensure that implicit scope remains reasonable.

If you want I can open an issue to think about how best to ensure this doesn't happen (even if we write out own test, there's no guarantee that someone else down the line doesn't set up a bizarre set of implicits somewhere).

ceedubs · 2016-09-07T20:19:37Z

kernel-laws/src/test/scala/cats/kernel/laws/LawTests.scala

+    Cogen[Long].contramap(_.toLong)
+
+  implicit val cogenBigDecimal: Cogen[BigDecimal] =
+    Cogen[Double].contramap(_.toDouble)


It looks like scalacheck has Cogen instances for BigInt and BigDecimal. Is there a reason we need separate instances here? If so, we probably should document it.

Those are only in master and were added (by me) recently in a just-merged PR. I think we still need them until another release is out.

ceedubs · 2016-09-07T21:06:17Z

👍 LGTM but I think that we should create issues to follow up on the left-distributivity law and the short-lived cogen instances.

ceedubs · 2016-09-07T21:14:02Z

By the way, @non I'm really excited about this, and it's fantastic that it caught an issue!

We provide some Cogen instances which were not included with 1.13.2. These instances will be provided in a future release, so when they become available we should remove ours. This commit also removes one instance that wasn't in 1.13.1 but was available in 1.13.2: Cogen[Stream[A]].

non · 2016-09-08T01:53:36Z

@ceedubs It turns out one of the instances (Cogen[Stream[A]]) was no longer needed, so I removed it. I added the following comment to all the other instances:

this instance is not available in scalacheck 1.13.2.
remove this once a newer version is available.

johnynek · 2016-09-08T02:08:25Z

I'm not super excited about losing a law. I'd rather lose an instance unless we can make a strong case as to why the law is overly tight.

non · 2016-09-08T03:00:41Z

@johnynek One reason not to mandate it is that there seems to be disagreement about what laws this structure should obey: https://wiki.haskell.org/MonadPlus

It seems like the options are:

Left Distribution: fa.flatMap(f) <+> fb.flatMap(f) = (fa <+> fb).flatMap(f)
Left Catch: pure(a) <+> fb = pure(a)

Either of these would be valid laws to have. One suggestion would be to have two type classes, each with one of these laws. Supporting the left catch style might be nice for types like Try (since a take-first-defined universal monoid is often what you want when doing things like parsing).

That said, I'm fine with removing it if everyone else agrees that we want to commit to the left distribution flavor of MonadCombine.

ghost · 2016-09-16T16:31:09Z

Hi Guys,

I'm keen to get this PR in if possible so I can (hopefully) put in a PR for 2.12.0-RC1 but it needs the new versions of libs this PR brings in.

If there are some open questions, is it possible that they could be moved to a new issue/PR so I can PR on top of this one?

ghost · 2016-09-16T17:09:34Z

Or, I see that this is on the 0.8.0 milestone, so perhaps merge to a 0.8.x branch? (along with #1333 ;) )

johnynek · 2016-09-16T17:13:36Z

@non what about following Haskell's proposal:
https://wiki.haskell.org/MonadPlus_reform_proposal
So, MonadCombine would satisfy left distribution and MonadOr would satisfy left-catch.

adelbertc · 2016-09-18T07:54:11Z

I'm 👍 on this since the scope of this already rather large PR was to get us up to date on dependencies.

That Haskell proposal is interesting though - if folks are OK let's call this good and we can create a ticket to follow up with figuring out what we want to do with MonadPlus?

johnynek · 2016-09-18T17:20:11Z

👍 on the merge for this along with a ticket that we address the idea of
which laws we/MonadOr before the next release.
On Sat, Sep 17, 2016 at 21:54 Adelbert Chang notifications@github.com
wrote:

I'm 👍 on this since the scope of this already rather large PR was to get
us up to date on dependencies.

That Haskell proposal is interesting though - if folks are OK let's call
this good and we can create a ticket to follow up with figuring out what we
want to do with MonadPlus?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#1345 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEJdpb5SmR1F9k50H6-zM8NexDyWlzSks5qrO4kgaJpZM4Jy7Jq
.

ceedubs

👍 per @adelbertc and @johnynek's above comments.

I've created #1381 to follow up on the left-distributivity law.

non added the in progress label Sep 1, 2016

non added enhancement question testing Binary Breaking labels Sep 2, 2016

nrinaudo mentioned this pull request Sep 4, 2016

Update to later versions of Scalacheck nrinaudo/kantan.codecs#15

Closed

ceedubs mentioned this pull request Sep 5, 2016

ScalaCheck 1.13.0 #942

Closed

erik-stripe and others added 6 commits September 6, 2016 01:47

Use new Discipline 0.6 release.

d6a9e02

Fix JS tests.

64b297e

Work around cryptic scala.js compilation errors.

4dcafc1

Fix scalacheck dependency error

80a63f9

Remove left-distributivity law from MonadCombine.

742e1d2

Also remove various obsolete/commented code.

non force-pushed the topic/scalacheck-1.13.x branch from 62fccae to 742e1d2 Compare September 6, 2016 05:47

erik-stripe added 2 commits September 6, 2016 11:09

Add some missing instances.

9f45fe9

Appease ScalaStyle.

1732ec1

ceedubs reviewed Sep 7, 2016
View reviewed changes

kailuowang added this to the 0.8.0 milestone Sep 11, 2016

ghost mentioned this pull request Sep 18, 2016

Bump ScalaCheck to 1.13.0 typelevel/algebra#144

Closed

ceedubs mentioned this pull request Sep 18, 2016

Figure out what to do with MonadCombine and left-distributivity #1381

Closed

ceedubs approved these changes Sep 18, 2016

View reviewed changes

ceedubs merged commit 155f7f5 into typelevel:master Sep 18, 2016

ceedubs removed the in progress label Sep 18, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump ScalaCheck to 1.13.1, and fix the fallout. #1345

Bump ScalaCheck to 1.13.1, and fix the fallout. #1345

non commented Sep 1, 2016 •

edited

Loading

non commented Sep 1, 2016

non commented Sep 2, 2016

non commented Sep 2, 2016

non commented Sep 3, 2016

durban commented Sep 3, 2016

non commented Sep 4, 2016

codecov-io commented Sep 4, 2016 •

edited

Loading

ceedubs commented Sep 5, 2016

non commented Sep 6, 2016 •

edited

Loading

non commented Sep 6, 2016

adelbertc commented Sep 6, 2016

non commented Sep 6, 2016

eed3si9n commented Sep 7, 2016

non commented Sep 7, 2016 •

edited

Loading

eed3si9n commented Sep 7, 2016

non commented Sep 7, 2016 •

edited

Loading

eed3si9n commented Sep 7, 2016

non commented Sep 7, 2016 •

edited

Loading

eed3si9n commented Sep 7, 2016

non commented Sep 7, 2016

ceedubs Sep 7, 2016

non Sep 7, 2016

ceedubs Sep 7, 2016

ceedubs commented Sep 7, 2016

ceedubs commented Sep 7, 2016

non commented Sep 8, 2016

johnynek commented Sep 8, 2016

non commented Sep 8, 2016 •

edited

Loading

ghost commented Sep 16, 2016

ghost commented Sep 16, 2016

johnynek commented Sep 16, 2016

adelbertc commented Sep 18, 2016

johnynek commented Sep 18, 2016

ceedubs left a comment

Bump ScalaCheck to 1.13.1, and fix the fallout. #1345

Bump ScalaCheck to 1.13.1, and fix the fallout. #1345

Conversation

non commented Sep 1, 2016 • edited Loading

Status

Summary

non commented Sep 1, 2016

non commented Sep 2, 2016

non commented Sep 2, 2016

non commented Sep 3, 2016

durban commented Sep 3, 2016

non commented Sep 4, 2016

codecov-io commented Sep 4, 2016 • edited Loading

Current coverage is 91.73% (diff: 76.92%)

ceedubs commented Sep 5, 2016

non commented Sep 6, 2016 • edited Loading

non commented Sep 6, 2016

adelbertc commented Sep 6, 2016

non commented Sep 6, 2016

eed3si9n commented Sep 7, 2016

non commented Sep 7, 2016 • edited Loading

eed3si9n commented Sep 7, 2016

non commented Sep 7, 2016 • edited Loading

eed3si9n commented Sep 7, 2016

non commented Sep 7, 2016 • edited Loading

eed3si9n commented Sep 7, 2016

non commented Sep 7, 2016

ceedubs Sep 7, 2016

Choose a reason for hiding this comment

non Sep 7, 2016

Choose a reason for hiding this comment

ceedubs Sep 7, 2016

Choose a reason for hiding this comment

ceedubs commented Sep 7, 2016

ceedubs commented Sep 7, 2016

non commented Sep 8, 2016

johnynek commented Sep 8, 2016

non commented Sep 8, 2016 • edited Loading

ghost commented Sep 16, 2016

ghost commented Sep 16, 2016

johnynek commented Sep 16, 2016

adelbertc commented Sep 18, 2016

johnynek commented Sep 18, 2016

ceedubs left a comment

Choose a reason for hiding this comment

non commented Sep 1, 2016 •

edited

Loading

codecov-io commented Sep 4, 2016 •

edited

Loading

non commented Sep 6, 2016 •

edited

Loading

non commented Sep 7, 2016 •

edited

Loading

non commented Sep 7, 2016 •

edited

Loading

non commented Sep 7, 2016 •

edited

Loading

non commented Sep 8, 2016 •

edited

Loading