Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow non-constant arbitrary functions for (Co)Kleisli, State, Func #1619

Closed
wants to merge 1 commit into from

Conversation

ceedubs
Copy link
Contributor

@ceedubs ceedubs commented Apr 21, 2017

Resolves #1605.

This is a replacement for #1606.

Now that Arbitrary instances for functions have been improved with the introduction of Cogen in scalacheck, we can pick up better arbitrary functions than the constant ones we were using.

@ceedubs
Copy link
Contributor Author

ceedubs commented Apr 21, 2017

Requesting a review from @non. As you are the one who introduced Cogen into Scalacheck, I'm interested in your input on the decision that was made in #1606 (review) to require implicit arbitrary instances for functions as opposed to requiring implicit Gen and Cogen instances and deriving the arbitrary functions ourselves.

Copy link
Contributor

@edmundnoble edmundnoble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Curious about @non's input.

@ceedubs
Copy link
Contributor Author

ceedubs commented Apr 25, 2017

Oops there's a compile error in the free module. I'll fix it.

@ceedubs
Copy link
Contributor Author

ceedubs commented Apr 25, 2017

@edmundnoble hmm actually I guess this compile error is one potential reason to favor the direct Cogen/Arbitrary implicit parameters as opposed to the function Arbitrary instance. In some cases the implicit search for the function Arbitrary instance fails while the Arbitrary[A] and Cogen[A] implicit search succeeds. I also kind of like the idea that conceptually the Cogen and Arbitrary instances are the fundamental things that we need to provide our instance and the fact that we get there through scalacheck's Function1 instance is an implementation detail, but I suppose one could argue this in the opposite direction.

Now I'm second-guessing this approach :\

@edmundnoble
Copy link
Contributor

Huh. I can't see why that'd be the case, the implicit error that is. TBH I am more inclined toward the argument in the other direction; users can provide their own Arbitrary function instances that way, and the Function1 instance is what I'd see people choosing to use or not use. Having different Cogen and output Arbitrary instances in scope will already feed into the Function1 instance generator. Also... is there not an error in the Kleisli monoid in the tests? https://travis-ci.org/typelevel/cats/jobs/224346639#L1658

@ceedubs
Copy link
Contributor Author

ceedubs commented May 9, 2017

@edmundnoble hmm I guess that the implicit divergence only is happening in the scala.js build:

[info] Compiling 7 Scala sources to /home/travis/build/typelevel/cats/free/.js/target/scala-2.10/test-classes...
[error] /home/travis/build/typelevel/cats/free/src/test/scala/cats/free/FreeTTests.scala:211: diverging implicit expansion for type org.scalacheck.Arbitrary[Int => cats.Eval[(Int, A)]]
[error] starting with method arbContainer2 in trait ArbitraryLowPriority
[error]   implicit def intStateArb[A: Arbitrary]: Arbitrary[IntState[A]] = catsLawArbitraryForState[Int, A]
[error]                                                                                            ^
[error] one error found
[error] (freeJS/test:compileIncremental) Compilation failed

And yeah you are right, it is reporting a problem with the Kleisli Monoid. That's concerning. I'll try to take a look at this this evening (Eastern US time).

ceedubs added a commit to ceedubs/cats that referenced this pull request May 13, 2017
Resolves typelevel#1605.

This is a replacement for typelevel#1619. The approach taken in that PR led to
implicit resolution failures in scala.js.
@ceedubs
Copy link
Contributor Author

ceedubs commented May 13, 2017

Closing in favor of #1666. @edmundnoble sorry I'm departing from your preferred approach, but I seem to need to in order for this to compile in the scala.js build 😬

@ceedubs ceedubs closed this May 13, 2017
kailuowang pushed a commit that referenced this pull request May 15, 2017
Resolves #1605.

This is a replacement for #1619. The approach taken in that PR led to
implicit resolution failures in scala.js.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants