Remove shrinking from gen-based Prop.forAll

[charleso]

Shrinking is only safe when used with Arbitrary. Generators can have a smaller range than the global shrinking, which results in invalid shrunken values.

This fixes issues like #129, #260, #317, #370, #442 and #443.

I suspect this is going to be controversial and I completely understand if it's not accepted. I'm opening this to show how I suggest it should be tackled, but it obviously affects people who are currently relying on this behaviour.

EDIT: Note that Haskell's QuickCheck forAll doesn't shrink in this instance either

[charleso]

This breaks binary compatibility (that's a great build check to have). I'm not sure what the suggested approach would be. It can actually be made binary compatible if we keep the Shrink implicits but ignore them. That might be safer than having to do a whole new major release. I'm happy to discuss whatever approach people think is more appropriate.

[dwijnand]

It's probably more acceptable to:

• keep the implicits in the type signature,
• don't use them in the implementation,
• deprecate the methods,
• Introduce non deprecated methods that doesn't take the Shrink instances,
• and bump the minor version (x.Y.z).

Or maybe they should be deprecated without changing the behaviour?

[charleso]

@dwijnand Thanks for the comment/suggestions. Keeping the implicits is probably the safe way to go. I might wait for a conceptual thumbs up from a maintainer though, I'm worried that it may not be something they're interested in.

I'm not as sure about introducing even more methods though. People can already use forAllNoShrink now but forAll might appear to be the default except for the shrinking issue. I'm also assuming that we can't have the same forAll function with and without an implicit, as that would be ambiguous.

Thanks again.

[charleso]

Sorry, that was a clumsy way of saying that I don’t know whether Rickard or others consider this an issue or not. If people are interested I’m very happy to make whatever changes are required. Thanks again.

[dwijnand]

I might wait for a conceptual thumbs up from a maintainer though, I'm worried that it may not be something they're interested in.

Makes sense to me!

I'm not as sure about introducing even more methods though.

I'm also assuming that we can't have the same forAll function with and without an implicit, as that would be ambiguous.

I got curious so I tried it out and was able restore the binary API, which I also deprecated: charleso#1.

People can already use forAllNoShrink now but forAll might appear to be the default except for the shrinking issue.

Good point. So I deprecated Gen-taking forAllNoShrink in charleso#2 😄

[charleso]

I got curious so I tried it out and was able restore the binary API, which I also deprecated: charleso#1

Yeah that's brilliant.

Thank you.

[dwijnand]

Seems I forgot to define the deprecation version in my last change. I'm logging off, but I can fix it up when I'm back (unless you beat me to it).

[charleso]

Seems I forgot to define the deprecation version in my last change. I'm logging off, but I can fix it up when I'm back (unless you beat me to it).

Fixed. There were (funnily enough) a few warnings around calling the deprecated forAllNoShrink that I've fixed up now. Hopefully it's all building correctly now. :)

[charleso]

Another person caught out by the forAll behaviour #443 (comment)

[rickynils]

@charleso @dwijnand Sorry for my unresponsiveness. I think your approach here makes sense. In one way it makes ScalaCheck less powerful since it removes shrinking that could have been useful in many situations. On the other hand it removes a big source of confusion that a lot of people have stumbled on.

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

I wonder if we also can remove the hideous sieve-thingy from Gen when this PR has been merged. It was added as a (poor) attempt at fixing invalid shrinking in some situations.

[dwijnand]

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

If you prefer, I'm happy if you'd like to do things that way.

1.14.1 with deprecated, same behaviour methods, 1.15.0 with this PR?

I wonder if we also can remove the hideous sieve-thingy from Gen when this PR has been merged. It was added as a (poor) attempt at fixing invalid shrinking in some situations.

Sounds good.

[ashawley]

How does one still use shrinking? In QuickCheck, the default for forAll is no shrinking, but there is still support for shrinking with forAllShrink. From what I see so far, that isn't available here. This reads like turning off shrinking entirely, when the proposal was changing the default.

[charleso]

How does one still use shrinking?

It's still there, but only for the variants of the forAll function with the implicit Arbitrary. I'm proposing to disable it for the forAll functions that take Gen.

def forAll[A1,P] (f: A1 => P)(implicit p: P => Prop, a1: Arbitrary[A1], s1: Shrink[A1]): Prop

[charleso]

In QuickCheck, the default for forAll is no shrinking

Do you mean ScalaCheck? QuickCheck has always only shrunk with Arbitrary (if implemented for a specific instance) and never with Gen, which is what I'm proposing to do in this PR.

[ashawley]

Maybe I missed the nuance of the change then.

So you're proposing four cases:

1. forAll and explicit Gens with shrinking disabled
2. forAll with implicit Arbitrarys with shrinking preserved
3. forAllShrink and explicit Gens with shrinking preserved
4. forAllNoShrink with implicit Arbitrarys

And still supporting all the possible arities of each.

Right now, ScalaCheck does:

1. forAll and explicit Gens with shrinking
2. forAll with implicit Arbitrarys with shrinking
3. forAllNoShrink with explicit Gens
4. forAllNoShrink with implicit Arbitrarys

[charleso]

So you're proposing four cases:

Yeah something like that. The forAllShrink already exists, but takes a manual shrink function. I'm not touching that. I'm only changing the behaviour of forAll with Gen which is where the problem is. Everything else stays the same.

I suspect what I've proposed at the moment might have to be tweaked based on @rickynils comment and my own counter-question below (which is totally fine).

[joroKr21]

FWIW I've been working around this issue for years by using implicit val noShrink: Shrink[MyType] = Shrink.shrinkAny (couldn't use forAllNoShrink because of scalatest).

I don't think Gen with implicit Shrink makes any sense. But I like the idea of combining them. Then the usability issue could be solved by adding a withShrink(implicit shrink: Shrink[A]) method on Gen.

[charleso]

Then the usability issue could be solved by adding a withShrink(implicit shrink: Shrink[A]) method on Gen.

I'm curious. I hope you don't mind me asking. What Gens are you constructing/using for your code base, and in particular what is the range of the values? And in what circumstances would you want to summon an implicit Shrink?

I ask, because I don't really think the implicit part is a good idea (ie withShrink(shrink: Shrink[A]) would be what I would suggest). But I don't have enough data points to know what people are actually doing with Shrink. I only have my biases. :)

[joroKr21]

I ask, because I don't really think the implicit part is a good idea (ie withShrink(shrink: Shrink[A]) would be what I would suggest). But I don't have enough data points to know what people are actually doing with Shrink. I only have my biases. :)

I tend to agree here, I don't think I would be using the implicitness here very often, but sometimes it might come in handy (say I have my own type with implicit Shrink[MyType]) and then I do Gen.listOf(arbitrary[MyType]).withShrink then shrinking would be automatically propagated to the list generator, but I could still provide explicit shrinkers for other generators.

[woparry]

Explicit shrinks (ie. forAllShrink with arity > 1) would be extremely useful for me.

My workflow for working around this issue has always been to write Gen and Shrink instances in parallel, and to wrap them (case class AsciiString(string: String), Gen[AsciiString], Shrink[AsciiString]) if there is ambiguity (because that's less annoying than passing in the Pretty instances).

@charleso 's case class GenShrink[A](gen: Gen[A], shrink: Shrink[A]) looks perfect for this, especially .filter could pretty easily operate on both sides.

[charleso]

@woparry Thanks for the comment, it's good to hear from someone who is using this.

Just curious, any reason you don't provide an implicit Arbitrary to make that Gen as the global/implicit version? Is it just that Gen works with the global Shrink, or something else?

[charleso]

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

I don't particularly have an agenda here, other than to stop people getting confused about the shrinking, I'm super happy to do whatever you think is best for ScalaCheck.

I'm just wondering out-loud what upgrading might look like. If we deprecate users upgrading to 1.14.1 will most likely switch every forAll to forAllNoShrink. But if they ignore the deprecation warning, and upgrade again to 1.15, the warning goes away.

I guess I'm wondering if we shouldn't re-add the non-shrinking non-deprecated forAll back in at all (which is what we're doing in this PR at the moment)? If you're using Gen you're always encouraged to use forAllNoShrinking? And in 1.15 do we perhaps leave forAll with Gen deprecated but now the behaviour is not to shrink. Purely visually forAll is definitely much shorter/nicer than forAllNoShrink but perhaps it makes sense to make the split clear given the previous behaviour and history. I'm not sure though. :( (EDIT: see #445 for what I mean here)

Again, I'd like to help get this change through in whatever form makes sense. I'm actually pretty indecisive and so I'm happy if you have an opinion on how you want the change(s) implemented and I can do the leg work.

[charleso]

I've created a PR that just deprecates the forAll Gen functions as suggested (#444). Let me know if that's a good first step.

[charleso]

And a second PR #445 which extends #444 just to disable shrinking but leave the deprecation warning which is slightly different to what this PR is proposing. Sorry for any confusion around having multiple PRs.

[ashawley]

I'm just wondering out-loud what upgrading might look like.

An implicit is being deprecated, so the upgrading is the difficult part of this. Deprecating a method typically will result in its subsequent removal, or a subsequent removal of some signature of the method. It's not possible to provide a deprecation time period that gives the old version and simultaneously offers a new alternative without the implicits, since the compiler would be confused about conflicting signatures. And a deprecatedName won't work, since presumably nobody is explicitly passing a Shrink[T] to forAll.

As a result, when the implicit is removed the user's code will still compile. If they miss or ignore the deprecation warning, there will be no indication of the behavior change. Since most test suites succeed when ScalaCheck is upgraded, there won't be any indication that the shrinking policy changed for forAll(Gen). They would need to be interacting with a failing property test to notice. So after deprecating, a migration annotation should be added when forAll(Gen) is changed to mitigate that problem.

It wasn't a poor choice to use an implicit for shrinking in ScalaCheck. This is a major behavior change in the library, so it's unsurprising this change requires delicate handling.

The steps I would propose:

1. When forAll(Gen) is deprecated, users should be provided a forAllShrink(Gen) method (which doesn't exist for all arities up to 8), rather than forAllNoShrink. During deprecation, forAllShrink(Gen) will just be an alias to forAll(Gen).
2. In the release after deprecation, forAll(Gen) would become an alias to forAllNoShrink(Gen), and forAllShrink(Gen) would remain.
3. forAll(Gen) would have the deprecated annotation changed to a migration.
4. The forAllNoShrink(Gen) is redundant of forAll(Gen), but should be preserved to maintain parity with forAllNoShrink(Arbitrary).

[charleso]

@ashawley

I've pushed to #444 with changes to the deprecation warning and the multi-arity forAllShrink functions as you've suggested. Please let me know if that looks like what you had in mind?

[non]

@charleso First of all, thanks for working on this. I'm sorry it didn't get more attention from me back when you were actively working on it.

In the long run, I think ScalaCheck 2 will likely move to combining generation and shrinking, similar to other modern libraries like Hedgehog. That would mean moving away from having a separate Shrink type class as well as separate generation/shrinking. In some ways it's similar to your GenShrink idea but it would "bake-in" shrinking to the generation to make it basically impossible for authors to shrink to "invalid" values.

Given the new -disableLegacyShrinking flag, do you think there's any other tactical work (that would be source- and binary-compatible) which we should be considering? I'm reluctant to use deprecation cycles here because I think the eventual change to shrinking will be much bigger (and we don't even know exactly what we expect people to migrate to yet).

I was thinking of closing out this PR but I wanted to check-in with you first about your thoughts.

[charleso]

@non Thanks again for the detailed response!

I completely agree that a Hedgehog "integrated shrinking" is absolutely 100% the way to go if there's an appetite for it. Even though I'm the author of the Scala Hedgehog library, I strongly believe the wider Scala community would greatly benefit if ScalaCheck gets the same shrinking capability (in the process making Scala Hedgehog mostly redundant).

Given the new -disableLegacyShrinking flag, do you think there's any other tactical work

I guess I can't help (for the last time I promise) being a broken record and say I still believe disabling shrinking for forAll with Gen is something we should just do now. I basically see it as a bug fix and bringing it back inline somewhat with the original QuickCheck. It's a source and binary compatible change, and I think it will greatly improve the default interaction with ScalaCheck for most users (as per my being optimistic comment here). Perhaps it wold make sense to include a -enableGenShrinking for users who still really want that ability back (despite the risks).

Anyway, I can see I'm not getting any traction with that argument, and I'm not trying to antagonise anyone over it. I'll close the PR now but I'm happy to re-open or help push something through if there's any subsequent interest. :)

Thanks again for really stepping up to help maintain ScalaCheck, it's already made a huge difference and I'm excited about its future. Good luck!

[non]

@charleso I think if this was a less high-profile library I would do exactly what you suggest and just start ignoring the Shrink parameter when working with Gen. My reason for avoiding that here is that people have very old code that is almost certainly relying on this behavior, and it seems irresponsible to change this behavior with an upgrade, since it's very likely people won't even realize things have changed.

Thanks again for your work, and I'm sure we'll be consulting you on the new shrinking design once we get there! ✨