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

Add fully automatic tree / class hierarchy generators #152

Closed
wants to merge 1 commit into from

Conversation

cvogt
Copy link
Contributor

@cvogt cvogt commented Mar 7, 2015

more from @xdotai :)

@rickynils
Copy link
Contributor

@cvogt How does this compare to scalacheck-shapeless?

@cvogt
Copy link
Contributor Author

cvogt commented Mar 12, 2015

I wasn't aware of scalacheck-shapeless. I really like its cleanlyness. @alexarchambault please correct me if this is wrong: As far as I can tell from the tests, scalacheck-shapeless does not allow recursive structures, such as trees. See https://github.com/alexarchambault/scalacheck-shapeless/blob/master/src/test/scala/org/scalacheck/Tests.scala
The explanation sounds like the compiler chokes on too large implicit inference chains, which can be arbitrarily large for recursive structures. My macro-based solution works well for recursive structures.

Also my macro supports creating arbitrary classes, not just case classes. Not sure what Shapeless supports. Maybe @milessabin has an opinion on this topic as well.

Also the macro doesn't have a shapeless dependency ;).

@milessabin
Copy link
Member

scalacheck-shapeless uses shapeless's typeclass deriving infrastructure which supports a wide variety of recursion patterns. See for example here.

@cvogt
Copy link
Contributor Author

cvogt commented Mar 12, 2015

@milessabin I interpreted @alexarchambault's comment as Shapeless principally supporting these recursion patterns, but the depths created by random generation overwhelming scalac. https://github.com/alexarchambault/scalacheck-shapeless/blob/master/src/test/scala/org/scalacheck/Tests.scala#L62
I didn't look at it in detail.

@milessabin
Copy link
Member

I think you should look at the detail. That type has no finite unfolding other than by terminating with a null or tying a recursive knot somehow.

shapeless has infrastructure for tying recursive knots, but there's no general way that could be applied automatically in all particular cases. shapeless's deriving mechanism also allows manually written implicit instances to be incorporated into a derivation, so someone trying to derive an instance for such a type would be able to manually provide the minimum needed to close a loop and leave the derivation process to handle everything else.

shapeless also handles Symbol prefixes correctly, which from a quick eyeball your macro doesn't. I think you'll be able to trip it up fairly easily with fairly trivial dependent types.

@alexarchambault
Copy link
Contributor

@cvogt As @milessabin said, scalacheck-shapeless is using the typeclass facilities from shapeless, which support a wide range of recursion patterns. But it's still possible to stumble upon cases that fail for unclear reasons. (Same thing with argonaut-shapeless, I'm still using the macro-based codec derivation from argonaut as a fallback from time to time.)

I'm not strongly opiniated about all that. The derivation from shapeless is quite general, so there will still be cases it will support that a macro-based approach won't. Inversely, you may handle better the rare cases in which it fails. Imo, it's interesting to have both, and they are the fallback of each other!

Edit: and as @milessabin added, there are aspects of automatic derivation that can be quite difficult to handle, and which it does well. Writing and maintaining yet another macro-based automatic derivation is likely to require quite an amount of effort.

@cvogt
Copy link
Contributor Author

cvogt commented Mar 12, 2015

I looked at it again and realize I misread the test code regarding recursion. If scalacheck-shapeless does everything and more/better, I am more than happy. Shapeless is the cleaner abstraction for sure. Closing this. Thx guys.

@cvogt cvogt closed this Mar 12, 2015
@cvogt
Copy link
Contributor Author

cvogt commented Mar 13, 2015

I tried it but didn't have much success with scalacheck-shapeless. In particular migrating my test

  • doesn't compile, because there is no implicit Arbitrary available when the tree contains non case-classes.
  • Removing the non-case classes makes it compile, but gives a stack overflow (see below), probably because of the recursion.
  • Breaking the recursion in the type allowed me to generate objects, but I saw only instances of classes of the first inheritance depth level. I'd however like instances of classes anywhere in the tree with potentially multiple intermediate sealed traits.
  • There is no built-in mechanism to safeguard me from forgetting to seal some trait and thus not getting any subclass instances generated. That's why I have tree, which fails for this at compile time and partialTree which makes it obvious that it may not be complete.

(See #152)

I'd be in favor of a Shapeless-based implementation that satisfies the above use cases. It seems cleaner. But until then I suggest to merge the tree macros, which solve the above uses cases. I am happy to make adjustments beforehand if desired.

    ...
    at org.scalacheck.Shapeless$$anonfun$instanceArbitrary$1.apply(Shapeless.scala:42)
    at org.scalacheck.Shapeless$$anonfun$instanceArbitrary$1.apply(Shapeless.scala:42)
    at org.scalacheck.Arbitrary$$anon$3.arbitrary$lzycompute(Arbitrary.scala:66)
    at org.scalacheck.Arbitrary$$anon$3.arbitrary(Arbitrary.scala:66)
    at org.scalacheck.Shapeless$$anonfun$cconsEquallyWeightedArbitrary$1.apply(Shapeless.scala:33)
    at org.scalacheck.Shapeless$$anonfun$cconsEquallyWeightedArbitrary$1.apply(Shapeless.scala:32)
    at org.scalacheck.Arbitrary$$anon$3.arbitrary$lzycompute(Arbitrary.scala:66)
    at org.scalacheck.Arbitrary$$anon$3.arbitrary(Arbitrary.scala:66)
    at org.scalacheck.Shapeless$$anonfun$cconsEquallyWeightedArbitrary$1.apply(Shapeless.scala:34)
    at org.scalacheck.Shapeless$$anonfun$cconsEquallyWeightedArbitrary$1.apply(Shapeless.scala:32)
    at org.scalacheck.Arbitrary$$anon$3.arbitrary$lzycompute(Arbitrary.scala:66)
    at org.scalacheck.Arbitrary$$anon$3.arbitrary(Arbitrary.scala:66)
    at org.scalacheck.Shapeless$$anonfun$instanceArbitrary$1.apply(Shapeless.scala:42)
    at org.scalacheck.Shapeless$$anonfun$instanceArbitrary$1.apply(Shapeless.scala:42)
    at org.scalacheck.Arbitrary$$anon$3.arbitrary$lzycompute(Arbitrary.scala:66)
    at org.scalacheck.Arbitrary$$anon$3.arbitrary(Arbitrary.scala:66)

@rickynils
Copy link
Contributor

@cvogt I don't think putting this in scalacheck core is a good idea. Since there's obviously different solutions in this problem area, I don't want make one seem more "authorative". Also, I really don't see this as core functionality.

As a side-note, for a long time I've been pondering having some sort of scalacheck-contrib or scalacheck-extra repo where we can be a bit more genereous with dependencies etc. In such a repo we could have both your solution and scalacheck-shapeless, if it would make sense. But, these contrib repos often grow large and unmanageable, so maybe it is just better to have a spread of small repos each implementing some non-core functionality of scalacheck. I don't know what approach is best.

@milessabin
Copy link
Member

@cvogt your tests ought to work more or less as written with scalacheck-shapeless. If you move the non-recursive cases out of the body of the property then they pretty much do. I'll look into a fix in shapeless to support the nesting correctly.

The form or recursion exhibited by your Tree should be fine, but it does SO as you've observed. @alexarchambault could you take a look ... I think that for some reason if a non-Leaf is selected then non-leaves will be selected forever after, but I can't immediately see why that should be.

In any case, useful test cases for both shapeless and scalacheck-shapeless ... thanks!

@ceedubs
Copy link
Contributor

ceedubs commented Mar 13, 2015

@rickynils for what it's worth, I would put in a strong vote for a collection of smaller repos as opposed to one scalacheck-contrib repo. I love the shapeless-contrib project, but I've hardly ever been able to use it because it's been rare that its been compatible versions of all of scala, scalaz, shapeless, and scalacheck with the code base I've wanted to use it in. But shapeless-contrib is forced into only releasing with very specific versions of these projects since they all have to move in step for a release.

Sorry to kind of hijack this thread for a scalacheck tangent :(

@cvogt
Copy link
Contributor Author

cvogt commented Mar 13, 2015

@rickynils Sounds good. I'll release it separately. Posting a reference here once I did.
@milessabin @alexarchambault Glad this produced some insightful test case.

@cvogt cvogt closed this Mar 13, 2015
@cvogt cvogt deleted the adt-macros branch March 13, 2015 16:16
@cvogt
Copy link
Contributor Author

cvogt commented Mar 13, 2015

@rickynils In case you didn't do it yet, could you release another snapshot so I can depend on that? the tree macro requires my changes you recently merged.

@cvogt
Copy link
Contributor Author

cvogt commented Mar 13, 2015

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.

5 participants