Deprecate overloaded Prop.collect

[ashawley]

In support of #421, this PR deprecates the single-argument Prop.collect. The method collects statistics on the parameter to a property test. It conflicts with the second version of Prop.collect that takes two arguments, a value and a property. The two overloaded methods confuse the Scala compiler for two reasons 1) because the former accepts an implicit second argument and 2) because collections from the standard library typically extend PartialFunction.

[non]

I think the deprecation message could be slightly improved (I found the name prop instead of f to be a little bit confusing when I first read it).

Relatedly, I think we can add a law that the rewrite the deprecation warning suggests is guaranteed to work (which could replace the other tests in this PR):

property("forAll(collect(f)) = forAll(x => collect(x)(f(x)))") =
  forAll { (f: Int => Prop, init: Long) =>
    val seed = rng.Seed(init)
    val prop0 = forAll((x: Int) => collect(x)(f(x)))
    val prop1 = forAll(collect(f))
    def run(p: Prop): Prop.Status =
      p.useSeed("name", seed).apply(Gen.Parameters.default).status
    run(prop0) == run(prop1)
  }

What do you think?

[ashawley]

I think the deprecation message could be slightly improved (I found the name prop instead of f to be a little bit confusing when I first read it).

Well, prop, is the argument name for the version of collect that won't be deprecated. That's where that's from. However, I can understand the confusion. Maybe it should be more pseudocode-like:

Use Prop.forAll(t => Prop.collect(t){ ... }) instead of Prop.forAll(Prop.collect(t => ...))

[non]

@ashawley Do you want to try to get this in before 1.14.1?

[ashawley]

No, this PR needs further review. I wasn't expecting to include with 1.14.1.

[ashawley]

I think we can add a law that the rewrite the deprecation warning suggests is guaranteed to work (which could replace the other tests in this PR)

That is handy. It has the advantage of not outputting anything as collect usually does. Otherwise, I'm not sure what the value of such a test is in the long-term. Admittedly, there isn't much value of the new tests. They only check that the two syntaxes of collect will compile.

[larsrh]

Should we go ahead with this for 1.15.0?

[non]

👍