Widen "oneOf" argument to "Iterable" from "Seq"

[Hydrotoast]

Hi maintainers,

For convenience, we would like to use the oneOf combinator on sets in our tests. So we have widened the argument to Iterable from Seq and added a test. After merging, the following snippet should compile.

val set: Set[A] = ???
Gen.oneOf(set)

Background. Currently, the oneOf combinator is restricted to Seq. To use sets, there are currently two workarounds. First, we could convert it to a sequence. Second, we could use the Gen.pick[T] combinator. The workarounds are shown below.

val set: Set[A] = ???

// Workaround 1
Gen.oneOf(set.toSeq)

// Workaround 2
Gen.pick(1, set).map(_.head)

However, both of these workarounds are inconvenient if the oneOf combinator could be widened to accept Iterable.

Additionally, we noticed that the following combinators also accept Iterable, so we believe that this change should make the API more uniform.

• Gen.someOf[T]
• Gen.atLeastOne[T]

As a final note, the Iterable trait should be compatible with the Scala 2.13 collections work. :)

[ashawley]

Seems like an oversight that was never caught. I'm surprised it never came up before.

This is a binary compatibility change. It will require an override in MiMa. Something like:

--- project/MimaSettings.scala
+++ project/MimaSettings.scala
@@ -12,9 +12,14 @@ object MimaSettings {
       removedPrivateMethods.map(exclude[DirectMissingMethodProblem](_)) ++
       newMethods.map(exclude[ReversedMissingMethodProblem](_)) ++
       removedPrivateClasses.map(exclude[MissingClassProblem](_)) ++
+      changedMethods.map(exclude[IncompatibleMethTypeProblem](_)) ++
       otherProblems
   )
 
+  private def changedMethods = Seq(
+    "org.scalacheck.Gen.oneOf"
+  )
+
   private def newMethods = Seq(
   )
 

[Hydrotoast]

Thanks for the patch @ashawley! I have not used MiMa before, so that may have taken me a while to resolve. I squashed the patch into the current commit. Hopefully the build passes now.

[Hydrotoast]

Wow! The previous build seems to have ran into an extremely rare flaky test.

[info] ! Gen.frequency 3: Exception raised on property evaluation.
[info] > ARG_0: 0
[info] > Exception: java.lang.IllegalArgumentException: no items with positive weights

The test samples a number n from [0, 100000] uniformly and attempts to prove the property Gen.frequency(List.fill(n)((1, const(0))): _*) { _ == 0 } . However, for n == 0, Gen.frequency throws an IllegalArgumentException.

I believe the odds of encountering this error are 1 / 1000 (assuming that 100 samples are generated for each invocation of forAll). I have split the test into two test cases: n == 0 and n in [1, 100000].

Looks like we get to submit a new feature and a bug fix in the same PR. :)

[Hydrotoast]

Hi @ashawley and @Philippus , I have addressed the comments in the first round of reviews. Is there anything else we need to do before merging? :)

[ashawley]

LGTM. But @rickynils will have the final word on this.

[rickynils]

This seems fine. But that MiMa change shouldn't be there, since this actually is a binary incompatible change, and we shouldn't merge this until the major version number has been bumped. @ashawley Or am I wrong?

[ashawley]

Yeah, I was thinking it would have to wait for the next major version, but we could probably use function overloading to keep oneOf(xs: Seq) and have it call oneOf(xs: Iterable).

[Hydrotoast]

Thanks for reviewing the work @rickynils . The consensus appears to be that the original proposed change introduces a binary incompatible change, so we should revert the MiMa change and perhaps consider introducing this work as part of the next major release.

I am unfamiliar with the release process for this project, but perhaps I can help expedite the general process. I will proceed as follows.

1. Revert the MiMa changes.
2. Split the PR into two: a) the fix for the flaky test and b) the original proposed change

For 2(a), I believe an independent PR for fixing a flaky test should be straightforward to merge. I will create a new PR for this and tag the relevant current reviewers.

For 2(b), I will reuse this PR and isolate the change to relax the method signature. As a question for the maintainers, @rickynils and @ashawley , how should we proceed with merging the proposed change in 2(b) as part of a major release?

[ashawley]

I predict if you just revert the MiMa change and use function overloading to keep both methods it would be fine. For 2(b), you could add a comment that mentions this issue number and says the old method is around for binary compatibility and can be deleted in the next major version.

[Hydrotoast]

Thanks @ashawley . Based on your advice, I have updated the PR as follows.

1. Reverted the MiMa change.
2. Overloaded the oneOf(_: Seq[A]) method to call oneOf(_: Iterable[A]).
3. Added a TODO on the oneOf(_: Seq[A]) to remove it on the next major release.

To continue isolating the two commits for the proposed change and the test fix, the commit history of the PR has been rewritten.

[ashawley]

Is the cast necessary? I assumed that it would just be oneOf(xs).

[Hydrotoast]

Yes, a form of static casting is necessary. Without the cast, the following example would lead to an infinite loop.

val xs: Seq[Int] = ???
Gen.oneOf(xs)

For background, Scala (and Java) dispatches methods dynamically based on the run-time type of the receiver, i.e. this, and then statically based on the compile-time type of the arguments, e.g. xs. Hence, a recursive overloaded method with the same arguments (without casting) should always dispatch to itself.

For static casting, I believe that an alternative syntax (unique to Scala) is type ascription, which has the form

oneOf(xs: Iterable[T])

However, I am uncertain of how it would be behave differently than asInstanceOf; I can look into this later tonight.

[Hydrotoast]

After reading into the subject, it seems that the type ascription will provide a compile-time hint to the compiler to dispatch the appropriate overloaded method. Since this is a minor run-time improvement and likely better Scala style, I will proceed with this unless there is a better option.

Thanks for bringing this to my attention @ashawley . :)

[ashawley]

The only other option would be doing oneOf(xs.toIterable). It sounds like the type annotation is best and does the right thing. Thanks.

[ashawley]

I looked at the results of deconstructing the class file with javap and it seems the type annotation is the equivalent of using asInstanceOf, and toIterable gives an unnecessary extra call, so really it's a question of readability between the type annotation and asInstanceOf.

[charleso]

type annotation is the equivalent of using asInstanceOf

Sorry to butt in, I'm afraid that's only true of the final byte code, there is a very important difference at compile time. The type annotation is safer in that it has a compile time check, the asInstanceOf throws all type safety out the window:

> "" : Int
<console>:8: error: type mismatch;
 found   : String("")
 required: Int
              "" : Int

> "".asInstanceOf[Int]
java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Integer

As you can see, String is definitely not Int, but you can force the conversation anyway with asInstanceOf and it will quite happily compile (but may fail at runtime).

In terms of the code in question, you should absolutely use a type annotation. asInstanceOf will continue to work even if xs later changes to something that's not actually an Iterable.

[Hydrotoast]

Thanks for verifying the assumptions @ashawley and @charleso . Nice to see that we have two empirical tests for the runtime behavior; learning something new about the internals everyday :)

For completeness, the compile-time conversion behavior of type ascription is documented in the Scala 2.11 specification.

The typed expression e:T has type T. The type of expression e is expected to conform to T. The result of the expression is the value of e converted to type T.

[ashawley]

This looks good to merge to me. Sorry that I didn't think of avoiding the binary compatibility change earlier.