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 buildable for java.util.HashMap #1023

Merged
merged 3 commits into from
May 24, 2024
Merged

Conversation

RustedBones
Copy link
Contributor

@RustedBones RustedBones commented Nov 28, 2023

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
Copy link
Contributor

satorg commented Jan 29, 2024

@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
Copy link
Contributor Author

RustedBones commented Jan 29, 2024

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
Copy link
Contributor

satorg commented Jan 29, 2024

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:
d2e6f07
(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
Copy link
Contributor Author

RustedBones commented Jan 30, 2024

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
Copy link
Contributor

satorg commented Jan 31, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

satorg commented Jan 31, 2024

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
Copy link
Contributor Author

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

@RustedBones
Copy link
Contributor Author

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

doc/UserGuide.md Outdated Show resolved Hide resolved
doc/UserGuide.md Outdated Show resolved Hide resolved
@satorg
Copy link
Contributor

satorg commented May 19, 2024

@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!

Copy link
Contributor

@satorg satorg left a comment

Choose a reason for hiding this comment

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

Thank you!

@satorg satorg merged commit 3a5601c into typelevel:main May 24, 2024
18 checks passed
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.

3 participants