Add buildable for java.util.HashMap

[RustedBones]

As java.util.ArrayList is supported in containerOf, introduce builder to generate java.util.HashMap from tuple generator.

The doc states java.util.ArrayList is supported by default, but the implicit evidence evt: C[T] => Traversable[T] is missing. Adding it in BuildableVersionSpecific.

Add new instances in BuildableSpecification to verify compile ability.

[satorg]

@RustedBones thank you for the contribution!

May I clarify one question please? What is the reasoning for adding the following implicit conversions:

  implicit def wrapArrayList[T](xs: ArrayList[T]): Traversable[T] = xs.asScala
  implicit def wrapHashMap[K, V](xs: HashMap[K, V]): Traversable[(K, V)] = xs.asScala

Does not seem to me that those are required for the "Buildable from HashMap" job... Or am I missing something?

[RustedBones]

Those implicit are there to fulfill the evidence evt: C[T] => Traversable[T] required by containerOf and buildableOf. To my knowledge, scala does not give those implicit evidence for java collections.

Without them,

val arrayListGen: Gen[java.util.ArrayList[String]] = Gen.containerOf(Gen.alphaStr)

fails with

No implicit view available from java.util.ArrayList[String] => collection.Traversable[String]

And

val hashMapGen: Gen[java.util.HashMap[String, Long]]  = Gen.buildableOf {
  for (str <- Gen.alphaStr; lng <- Gen.long) yield (str, lng)
}

fails with

 No implicit view available from java.util.HashMap[String,Long] => collection.Traversable[(String, Long)]

[satorg]

Oh, I see now, thank you for the clarification!

To be honest, that constraint looks quite odd to me. It does not seem to be used in the code for the sake of generation, but as just a constraint only:

  implicit def arrayListAsIterable[A](al: util.ArrayList[A]): Iterable[A] = null

  forAll { (al: util.ArrayList[Int]) =>
    println(al)
    true
  }

I.e. the conversion function is not called from the generation code at all.

Looks like that constraint was first introduced by @rickynils in this commit: https://github.com/typelevel/scalacheck/commit/d2e6f07cd43c1a04ec246c8fb8c8f5e90306918c (more than 10 years ago) with the following explanation:

This also restricts Gen.containerOf to only handle types that are implicitly convertible to Traversable. This is required in order to be able to inspect simplified containers.

But I am not sure what could it mean exactly and whether collections like ArrayList are supposed to be supported in this way.

[RustedBones]

It looks to me this constraint has been added so that we can also create a Shrink for containers with shrinkContainer.

An implicit Shrink is required by forAll, and the default implementation requires the evt: C[T] => Traversable[T] evidence.

IMHO the unused evidence on containerOf and buildableOf is there to ensure we'll be able to create a default Shrink for them.

[satorg]

Yes, I see what are you talking about. Perhaps, it was the idea behind that decision indeed.

However, I personally don't feel it should be done that way: shrinkContainer takes its own Buildable implicit which itself does not require the C => Traversable evidence to exist. But it is arbContainer who asks for such an evidence (even though it does not use it). Whereas Buildable can be implemented regardless of whether the evidence exists or not.

_{On the other hand, it is hard to say for sure. It was 2013 after all – the reign of Scala 2.9 and a time of numerous experiments with implicits and controversial decisions sometimes.}

Back to the PR... Why I am a bit hesitant about those Java-to-Scala conversions is that it does not seem the right direction to me. Although back in 2013 it was quite a legit approach and it was pretty easy to get such conversions simply with

import scala.collection.convert.ImplicitConversionsToScala._

It still works in Scala 2.13 and even 3.x but deprecated and most likely will be removed somewhen. And Scala3 takes it even further and suggests implementing scala.Conversion typeclass instead of the old-fashion implicit conversion.

So I am on a fence here - do we really want to get a quick solution by re-introducing the deprecated behavior back to the library? Or may it be worth of thinking out a better approach from a long-term perspective, wdyt?

[RustedBones]

I think this statement is not correct

shrinkContainer takes its own Buildable implicit which itself does not require the C => Traversable evidence to exist.

In order to remove a chunk from the container C, the evidence is required. The Buildable itself is not enough to do that.

Concerning the direction of the library, even if scala.Conversion is the way forward, I think the API will stay with the deprecated implicit conversion until scala 2 cross building gets dropped. Also, the implicit conversions are in binary specific folder. If containerOf/buildableOf gets to request a scala.Conversion, it would be quite easy to define that in the scala-3 sources.

TBH I mainly opened this PR because of the documentation here

By default, ScalaCheck supports generation of List, Stream (Scala 2.10 -2.12, deprecated in 2.13), LazyList (Scala 2.13), Set, Array, and ArrayList (from java.util)

In practice, ArrayList is not directly supported due to the missing evidence.

I would be fine to remove ArrayList this from the doc and let users define their custom container/buildable for java collections since this isn't much work. Now we're in a state where only half of the solution is implemented, which is to avoid.

[satorg]

think this statement is not correct

shrinkContainer takes its own Buildable implicit which itself does not require the C => Traversable evidence to exist.

Actually I mean that Buildable itself does not require the C => Traversable evidence. Sorry for the confusing wording. My point here is that since shrinkContainer already asks for both Buildable and C => Traversable implicits, there is no practical sense to make the conteinerOf* methods requiring the same evidence if the latters do not use it.

In practice, ArrayList is not directly supported due to the missing evidence. ... Now we're in a state where only half of the solution is implemented, which is to avoid.

In fact, at the moment when Buildable for ArrayList was first introduced, it seemed to be a pretty complete solution because the aforementioned import scala.collection.convert.ImplicitConversionsToScala._ was not deprecated at that time. So everyone could add that import and get the full support for ArrayList. Moreover, the import still works now (it really does!), but produces deprecation warnings.

So maybe as a trade off solution, and instead of baking our own ad-hoc implicit conversions into Buildable, it would make sense to add something like

@nowarn("cat=deprecation")
val JavaCollectionSupport = scala.collection.convert.ImplicitConversionsToScala`

to allow users simply add this import to their code

import org.scalacheck.util.JavaCollectionSupport._

and therefore help them to avoid the deprecation warnings in there.

Or ultimately we can simply add a note about it to the docs and let users take care of it theirselves.

[RustedBones]

Sounds like a good solution. Will update the PR and the doc accordingly.

[RustedBones]

Sorry for the delay @satorg I lost track of this PR.

[satorg]

@RustedBones , I apologize that reviewing your PR is taking that much time. Honestly, I lost tracking of it for a while, sorry for that. It looks great to me overall, however I noticed a couple of concerning places, could you clarify on them please? Thank you!