Deprecate forAll with Gens #444

charleso · 2018-12-07T19:22:43Z

Sorry if this gets confusing pretty quickly. This is one option for the first change to ScalaCheck to deprecate the forAll Gen functions, but leave the behaviour. Is it what you had in mind?

EDIT: This is based on the original disabling of Shrinking on #440

charleso · 2018-12-07T19:31:00Z

src/main/scala/org/scalacheck/Prop.scala

@@ -806,6 +806,7 @@ object Prop {

  /** Universal quantifier for an explicit generator. Shrinks failed arguments
   *  with the default shrink function for the type */
+  @deprecated("Use 'forAllNoShrink', shrinking will be disabled for Gen by default in the next release", "1.14.1")


I suck with useful error messages. Suggestions welcome for what might be more appropriate/useful/readable.

As I mentioned in #440, it would be better to provide users with forAllShrink, since forAllNoShrink does not provide shrinking.

This requires adding all the same arities of forAllShrink(Gen) as exist for forAll(Gen). This would probably end up needing to be released as 1.15.0.

The wording of the deprecation should probably be something like, "Shrink will be disabled when given explicit Gen, use forAllShrink instead", giving:

[warn] method forAll in object Prop is deprecated (since 1.15.0): [warn] Shrink will be disabled when given explicit Gen, use forAllShrink instead

ashawley · 2019-02-19T14:46:48Z

src/test/scala/org/scalacheck/PropSpecification.scala

@@ -24,7 +24,7 @@ object PropSpecification extends Properties("Prop") {

  property("Prop.==> undecided") = forAll { p1: Prop =>
    val g = oneOf(falsified,undecided)
-    forAll(g) { p2 =>
+    forAllNoShrink(g) { p2 =>


Now that you've defined forAllShrink(Gen), you could use it in the test suite. But it would be better to just minimize the changes in the PR, by keeping forAll(Gen) in the tests. The deprecation warnings for forAll(Gen) are non-fatal in the test suite, anyway.

ashawley · 2019-02-19T16:00:18Z

src/main/scala/org/scalacheck/Gen.scala

@@ -163,7 +163,7 @@ sealed abstract class Gen[+T] extends Serializable { self =>
    if (lhs == rhs) Prop.proved(prms) else Prop.falsified(prms)
  }

-  def !=[U](g: Gen[U]) = Prop.forAll(this)(r => Prop.forAll(g)(_ != r))
+  def !=[U](g: Gen[U]) = Prop.forAllNoShrink(this)(r => Prop.forAllNoShrink(g)(_ != r))


Now that you've defined it, you can use forAllShrink. Otherwise, you're changing the semantics of !=.

ashawley · 2019-02-19T16:05:13Z

src/main/scala/org/scalacheck/commands/Commands.scala

@@ -215,7 +215,7 @@ trait Commands {
  final def property(threadCount: Int = 1, maxParComb: Int = 1000000): Prop = {
    val suts = collection.mutable.Map.empty[AnyRef,(State,Option[Sut])]

-    Prop.forAll(actions(threadCount, maxParComb)) { as =>
+    Prop.forAllNoShrink(actions(threadCount, maxParComb)) { as =>


Same here, you can use forAllShrink. Otherwise, you're changing the implementation. Additionally, there is a Shrink[Actions] in scope.

charleso · 2019-02-19T20:06:26Z

Thanks @ashawley, sorry some of that was a little sloppy.

ashawley · 2019-02-19T20:11:55Z

src/main/scala/org/scalacheck/Gen.scala

@@ -163,7 +163,7 @@ sealed abstract class Gen[+T] extends Serializable { self =>
    if (lhs == rhs) Prop.proved(prms) else Prop.falsified(prms)
  }

-  def !=[U](g: Gen[U]) = Prop.forAll(this)(r => Prop.forAll(g)(_ != r))
+  def !=[U](g: Gen[U]) = Prop.forAllShrink(this)(r => Prop.forAllNoShrink(g)(_ != r))


You fixed one, but theres a second one that needs to be forAllShrink.

Apologies, I completely didn't see that. Updated

ashawley · 2019-02-21T14:39:48Z

src/main/scala/org/scalacheck/Prop.scala

@@ -806,16 +907,18 @@ object Prop {

  /** Universal quantifier for an explicit generator. Shrinks failed arguments
   *  with the default shrink function for the type */
+  @deprecated("Use 'forAllNoShrink', shrinking will be disabled for Gen, use forAllShrink instead", "1.14.1")


The deprecation message still needs tweaking. As I suggested, it should be

Shrink will be disabled with explicit Gen, use forAllShrink instead

The version number should be 1.15.0, not 1.14.1.

I'm not sure how strict ScalaCheck is with semver, but I understand you increment the minor version when there is either new functionality or new deprecations.

It's not necessary to suggest forAllNoShrink, because that's what forAll will effectively become.

The tone of the deprecation message could be less commanding. Users could assume they have to migrate to forAllShrink. It's really their choice.

@ashawley Thanks, I wasn't paying close enough attention (again). I've just used your full/original suggestion and updated the version number. I've updated the PR. Sorry for the confusion and wasted time.

dwijnand · 2019-02-21T14:44:13Z

@ashawley do you have merge and release permissions? I'm wondering if I should weigh in on the changes you're suggesting or not.

ashawley · 2019-02-21T14:47:04Z

I don't, but hopefully you'd weigh in regardless.

ashawley · 2019-02-21T23:16:41Z

I don't support deprecating shrinking with an explicit generator. I'd rather see it fixed, but in the absence of a fix, @charleso has made a valid argument that it should be disabled by default. My concern of not providing an opt-in for shrinking via forAllShrink has been taken in to consideration here. There's no reason not to move forward with deprecation.

Thanks for doing this!

ashawley · 2019-02-22T00:11:11Z

Concerning Rickard's original question in #440:

The question is now if we should release a version with unchanged behavior and deprecated methods before actually making this change.

This deprecation warning forforAll is likely going to be pretty disruptive for users of ScalaCheck 1.x:

New users are going to write their first property test using forAll with a generator and be confused by the warning.
Users could follow the advice of the deprecation message, "use forAllShrink instead", but that is misleading. The consensus is to not use shrinking, since the default for forAll will become equivalent to forAllNoShrink. The suggestion of forAllShrink is intended for migrating tests of existing users.
Alternatively, users could avoid the warning by using forAlllNoShrink, but that's unfortunate because in the next ScalaCheck release (1.16.x, 2.0?) the semantics for forAll will become the same as forAllNoShrink.
Projects with fatal warnings enabled, will need to research how to modify their sbt build and disable fatal warnings in their test suites.
All instances where forAll uses an explicit generator in the documentation could be updated to use forAllNoShrink, however, like (2) that's a moving target, since the plan is to make forAll have shrinking disabled.

We could try to either massage the deprecation message or jam in more information. Any suggestions to the wording are welcome, but it has diminishing returns.

I'd wager there needs to be a comprehensive release notes about this.

The release that subsequently removes shrinking should follow closely behind the release that does deprecation. That would avoid confusing new users from the deprecation warning.

charleso · 2019-02-22T07:35:07Z

Hi @ashawley

Thanks for taking the time to review this PR. My sincere apologies for doing this but I'm going to close the PR. It was originally as a Proof Of Concept and to help communicate one of the options for "fixing" (for some definition) for `forAll (as part of #440 ).

I wont deny it, ScalaCheck is in a tough spot. There is an extremely commonly used function that is behaving in an (arguably) unexpected fashion. By changing the semantics are we breaking compatibility?

Honestly I don't think the there is a Right Answer. I just know that personally I'm going to feel extremely responsible if we merge this PR and, as you 100% correctly point out, "New users are going to write their first property test using forAll with a generator and be confused by the warning.".

I'm more than happy if @rickynils, yourself, or another maintainer want to re-open and merge this PR, but I don't want to be seen as the person who was responsible for 1000 deprecation warnings for (in my opinion) a behaviour they didn't realise they were getting.

I still think that #440 is the right approach in this situation. Let's just change the behaviour. The worst that can happen is that users get slightly more unhelpful failure cases, but that are true positives. The alternatives are:

They get false positives in the form of confusing errors by leaving broken shrinking
They have to refactor all their code to remove deprecation warnings for code that should otherwise by just fine.

I'm really not trying to push you to merge/support #440, I suspect you won't and I'm not trying to have that argument again. I'm just not comfortable with #444 (this PR) and don't want to have my name associated with potentially pissing off lots of people "fixing" something.

Thanks again though, I really mean that, and sorry for dragging out a PR I wasn't really comfortable with in the first place.

ashawley · 2019-02-22T13:16:45Z

Ok, we can resurrect the branch if we need to. I actually agree that there probably isn't much advantage to this approach of deprecating. It's probably better to just migrate over in #444. We still have a lot of the same issues with #444, but it will be less confusing than pushing a release with deprecation warnings.

shawjef3 · 2019-02-26T15:40:46Z

I'm wondering what y'all would think about making shrink/noshrink a boolean parameter. Is there an easy way to turn shrinking on and off? Like if I have a test suite and I want to turn shrinking on and off for the whole suite. For instance, perhaps I'm still working on the instance of Shrink for my type and want to turn it on or off in a simple way.

I'm convincing myself this isn't a good idea, since forAllShrink would take a Shrink parameter, whereas forAllNoShrink would not. And so the method with the boolean parameter would still require you to have an instance of Shrink. So what I'm thinking of is maybe best if any shrink switch is turned on by default.

charleso mentioned this pull request Dec 7, 2018

Remove shrinking from gen-based Prop.forAll #440

Closed

charleso commented Dec 7, 2018

View reviewed changes

charleso mentioned this pull request Dec 7, 2018

Disable shrinking for forAll with Gen #445

Closed

ashawley reviewed Feb 19, 2019

View reviewed changes

charleso force-pushed the topic/forall-gen-deprecated branch from f5c3563 to 3bf4d72 Compare February 19, 2019 20:05

ashawley reviewed Feb 19, 2019

View reviewed changes

charleso force-pushed the topic/forall-gen-deprecated branch from 011cbd7 to 8aa8e2e Compare February 19, 2019 20:20

ashawley mentioned this pull request Feb 21, 2019

Deprecate internal forAllShrink #457

Closed

ashawley reviewed Feb 21, 2019

View reviewed changes

charleso added 2 commits February 22, 2019 09:33

Deprecate forAll with Gens

b5abe48

Introduce explicit, multi-arity forAllShrink functions

848a47f

charleso force-pushed the topic/forall-gen-deprecated branch from 5993f71 to 848a47f Compare February 21, 2019 22:34

ashawley approved these changes Feb 21, 2019

View reviewed changes

charleso closed this Feb 22, 2019

ashawley mentioned this pull request Feb 22, 2019

1.14.1 release? #453

Closed

18 tasks

charleso mentioned this pull request Aug 28, 2019

Comprehensive review of Test.Parameters. #522

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate forAll with Gens #444

Deprecate forAll with Gens #444

charleso commented Dec 7, 2018 •

edited

Loading

charleso Dec 7, 2018

ashawley Feb 19, 2019

ashawley Feb 19, 2019 •

edited

Loading

ashawley Feb 19, 2019

ashawley Feb 19, 2019

charleso commented Feb 19, 2019

ashawley Feb 19, 2019

charleso Feb 19, 2019 •

edited

Loading

ashawley Feb 21, 2019

ashawley Feb 21, 2019

ashawley Feb 21, 2019

ashawley Feb 21, 2019

charleso Feb 21, 2019

dwijnand commented Feb 21, 2019

ashawley commented Feb 21, 2019

ashawley commented Feb 21, 2019

ashawley commented Feb 22, 2019

charleso commented Feb 22, 2019

ashawley commented Feb 22, 2019

shawjef3 commented Feb 26, 2019

Deprecate forAll with Gens #444

Deprecate forAll with Gens #444

Conversation

charleso commented Dec 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashawley Feb 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charleso commented Feb 19, 2019

Choose a reason for hiding this comment

charleso Feb 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwijnand commented Feb 21, 2019

ashawley commented Feb 21, 2019

ashawley commented Feb 21, 2019

ashawley commented Feb 22, 2019

charleso commented Feb 22, 2019

ashawley commented Feb 22, 2019

shawjef3 commented Feb 26, 2019

charleso commented Dec 7, 2018 •

edited

Loading

ashawley Feb 19, 2019 •

edited

Loading

charleso Feb 19, 2019 •

edited

Loading