Deprecate Prop.BooleanOperators in anticipation of Dotty, add scalafix rewrite rule to rewrite usages

[smarter]

For a Boolean b and a prop p, the following is ambiguous in dotty:

import Prop._
b ==> p

Because Prop contains:

implicit def BooleanOperators(b: => Boolean) = new ExtendedBoolean(b)
implicit def propBoolean(b: Boolean): Prop = Prop(b)

And both ExtendedBoolean and Prop provide a ==> method, the one in
ExtendedBoolean just forwards to the one in Prop. This is not
ambiguous in scalac because Boolean wins against => Boolean but it
is in dotty (and intentionally so).

In general, it seems that all the methods in BooleanOperators are also
available on Prop, so BooleanOperators does not serve any purpose and
can be deprecated. We can then make it non-implicit in a subsequent
release of scalacheck (thus breaking source-compatiblity but not
binary-compatiblity) to finally be able to compile it with Dotty (which
also requires getting #423 in).

[non]

@smarter My proposal is to wait on adding any new deprecations until we get a new release out (since it's been so long). Then we can merge this (and any other PRs that deprecate parts of the API) and prepare for future API changes and/or breakage.

[smarter]

So, the issue is that we cannot cross-compile scalacheck with Dotty until after a scalacheck with this deprecation in is released, and we'll then need to wait another release to be able to publish scalacheck against Dotty. So, the quicker we get this in, the better. It should be easy to make a scalafix rewrite rule for this, which mean we can get scala-steward to automatically upgrade everyone's code (https://github.com/fthomas/scala-steward/blob/master/docs/scalafix-migrations.md), so the cost should be minimal.

[smarter]

Just wrote my first scalafix rule for this :). I'll make a PR against scala-steward once this is in (in fact, I wonder if we could just ask scala-steward to run the migration rule now even if the method isn't deprecated yet, would that be possible @fthomas ?)

[non]

@smarter Got it. I think the reasoning makes sense, especially since it already has to be explicitly imported to be used (i.e. it's very likely the rewrite would fix all warnings).

[fthomas]

@smarter Scala Steward runs any rule which is configured for the bump to 1.14.1. There is no requirement that rules need to replace deprecated symbols. The rule included here seems to be a perfect candidate for being added to Scala Steward.

[ashawley]

I'm pretty sure dropping BooleanOperators will be a significant breaking change, even in a 2.0 release. I'm hoping there's a different workaround. Though, I haven't had time to investigative this closely, yet.

[smarter]

It doesn't need to be removed, just made non-implicit to be able to compile scalacheck with dotty.
Travis is flaky right now but locally the tests are passing.

[non]

@ashawley My understanding is that the process will be:

1. Deprecate the implicit now.
2. Release version 1.14.1.
3. Possibly in parallel with (1-2) use scalafix to encourage folks to switch their import to the equivalent, non-deprecated propBoolean.
4. Make the method non-implicit, breaking source compatibility but preserving binary compatibility.
5. Release version ???.

What do you think? Does that make sense? Anything I'm missing @smarter ?

[ashawley]

Oh, I thought it was going to be deleted. If it's not implicit, is it still possible to use the fencing (==>) and labels (:| and |:) in properties?

[non]

@ashawley I'm fairly sure that the propBoolean implicit will turn your Boolean into a Prop which also has that method. You could test this locally if you want (take some code using those, comment out BooleanOperators, and see if propBoolean works).

(EDIT: In fact this PR has code that demonstrates that that works.)

[ashawley]

I tried it locally back when I was discussing #423 with @smarter, but I'll look again.

[smarter]

@non Yes that's exactly what I'm proposing :)

[non]

I'm 👍 on this but I'll wait until @ashawley is on board to merge it.

[ashawley]

Sorry, I wasn't aware that Prop had the methods of BooleanOperators implemented, as well. I've always found the implicit conversion of Boolean => Prop to be squirrelly, and it shows here.

Why not deprecate and later drop propBoolean, and we rename it to BooleanOperators? Won't that have the same effect here? The name for BooleanOperators is a strange one, but I don't think propBoolean is much better. That implicit method is also a long documented feature of ScalaCheck, so it would be an easier route in that way. Although, it isn't binary compatible, it is source compatible. ScalaCheck is approaching a major version, so this could be done in 2.0, IMO.

I know you spent a bunch of time on this, @smarter, even creating a scalafix recipe. Maybe we can close this? I am happy to create a new PR that deprecates propBoolean and ExecuteExtended.

[non]

@ashawley I think that propBoolean plays a role at letting callers return a Boolean instead of a Prop in e.g. Prop.forAll so I think removing that one would be a bit more disruptive. (I haven't thought carefully about it, that was just my sense.)

[ashawley]

Yes, that's my sense what it does, as well. However, it seems like it's the same consequence, no?

I've tried to verify it in #504, but Travis failures may make that difficult.

[smarter]

I'm personally against anything that increases the timeline until scalacheck cross-compiles with Dotty, so I'm 👎 for #504 which would require waiting until scalacheck breaks bin-compat (by the way, is there really a pressing need to break scalacheck bin-compat again? It would have a ripple effect on the whole ecosystem, which still hasn't completed upgrading to the previous bin-compat breaking release).

That implicit method is also a long documented feature of ScalaCheck, so it would be an easier route in that way.

A binary breakage is not an easy route by any mean, on the other hand, the combination of scalafix and scala-steward mean that source breakages aren't as bad as they were before, this is especially the case here where we're giving users a heads-up by first deprecating before breaking source-compatibility.

[ashawley]

I've experimented with it by publishing a jar locally with the implicit keyword for BooleanOperators removed. Indeed, Mima doesn't complain. I then try to compile existing code that imports BooleanOperators, and it causes no warning or error about BooleanOperators. Instead, it gives confusing error messages about type mismatches and that ==> is not a member of Boolean...

I don't think this is what binary compatibility is supposed to look like. Is it?

[smarter]

I've experimented with it by publishing a jar locally with the implicit keyword for BooleanOperators removed.

This is why this PR doesn't remove the keyword immediately and instead just mark BoolenOperators as deprecated to give people a chance to fix their code before we mark it as non-implicit.

I don't think this is what binary compatibility is supposed to look like. Is it?

I'm not sure what you mean by that, binary compatibility does not imply source compatibility.

[non]

@ashawley I think the idea is that if you've already compiled code into a runnable jar, you can still run the jar against the version without the implicit keyword.

The fact that you can't compile it is an indicator that source compatibility has been broken, but binary compatibility should remain.

[ashawley]

I follow. It just seems like a distinction without a difference. I suppose ScalaCheck is fundamentally a source-based library, so binary compatibility isn't a useful measure.

[non]

I think the big issue is transitive dependencies. If I'm using ScalaCheck myself I need to fix my code to compile (which I have the power to do). But if I'm using Discipline, ScalaTest, or other libraries that use ScalaCheck (so I have transitive dependencies) a binary incompatible upgrade will break those libraries until they update. And if some of them update and others don't, my build will be broken (I'll be unable to use them together) until they sort it out.

The options (for a future Dotty-compatible release) are:

• Just break source compatibility. This is inconvenient for users that directly depend on ScalaCheck, but also something they can fix themselves (manually or potentially with ScalaFix).
• Just break binary compatibility. This has the potential drawbacks I mentioned above, but assuming users only depend on ScalaCheck directly won't be a big deal.
• Break binary and source compatibility. This is the worst of both scenarios (and represents the last incompatible release, if I remember correctly).
• Break neither. This is nicest, but according to @smarter means that Dotty won't be able to compile ScalaCheck. I guess an option would be to copy the sources to scala2.12-, scala2.13+, and scala3 and maintain binary compatiblity for this first two but not the third? I'm not very enthusiastic about this but it is a valid possibility.

[joroKr21]

In my project at work we definitely return booleans as props. I know it's not necessarily the way scalacheck is supposed to be used (i.e. assertion style) but I wouldn't be surprised if many users do that. So 👍 for deprecating boolean ops and keeping the implicit conversion. It could be moved to the Prop companion object in a major release if there is concern that we are moving away from orphan implicit conversions in scala.

[ashawley]

If someone uses BooleanOperators in their code, I'm not sure how transitive dependencies would change the situation. I'm not so sure people re-run their test suites as packaged jars.

If instead an implicit was being dropped on some hypothetical ScalaCheck internal that was only used by downstream libraries with a dependency, I guess we could pull this off, but it would seem BooleanOperators is primarily for use in client code.

I'm just trying to study the options and their impact. I know it's not popular to raise these concerns, but it seems like past breaking changes have frustrated users in the past. Scalafix recipes, deprecation messages and release notes are too easily missed.

[joroKr21]

I think classpath hell is a much bigger issue than changing an import. That's why people insist so much on binary compatibility. I guess the problem with scalacheck was that minor versions were not binary compatible, which is unexpected.

[smarter]

scala-steward PR: scala-steward-org/scala-steward#853

[ashawley]

Following-up on Dotty support, two things:

1. Artifacts for Dotty 0.19 were published as ScalaCheck snapshots. At this time, there are no plans to release non-snapshot versions of Dotty. The snapshot route will be how they'll be made available for now. I'll try and get a snapshot for Dotty 0.20, soon, since that's the latest version.
2. The BooleanOperators is now private[this], for backwards compatibility, in favor of importing the implicit propBoolean instead.

Add the following to your sbt:

resolvers +=
  "Sonatype OSS Snapshots" at
  "https://oss.sonatype.org/content/repositories/snapshots"

Use this snapshot of ScalaCheck for Dotty artifacts:

libraryDependencies += "org.scalacheck" %% "scalacheck" % "1.15.0-09a2431-SNAPSHOT" % "test"