Defer evaluation of Prop labels

[mrdziuban]

I've discovered that a bottleneck in my tests is the generation of labels to be displayed if the test fails. I'm using pprint like this:

myProp :| s"expected: ${pprinter(...)}, actual: ${pprinter(...)}"

and I see in a stack dump that pprinter is being called even when the test succeeds.

This does a few things to defer evaluation:

• Adds new Prop.label, Prop.:|, and Prop.|: methods that take a by-name parameter
• Marks the old Prop.label, Prop.:|, and Prop.|: with by-value parameters as private[scalacheck]
• Only adds the label to Prop.Result.labels if the result's status is Failed or Exception -- this ensures the by-name parameter isn't evaluated when the test succeeds

[mrdziuban]

Also of note, the by-name parameter of Prop.|: won't take effect on scala 2.12 because of scala/bug#1980 (fixed in scala 2.13). The compiler reports:

[warn] /Users/matt/scalacheck/core/shared/src/main/scala/org/scalacheck/Prop.scala:158:7: by-name parameters will be evaluated eagerly when called as a right-associative infix operator. For more details, see scala/bug#1980.
[warn]   def |:(l: => String) = label(l)

[mrdziuban]

Made some changes here to ensure that this is binary compatible, thank you @armanbilge!

[armanbilge]

Oh this is annoying, I wonder if we should move the test suites out of the scalacheck package.

I know it's very common but I actually think tests in the same package is an anti-pattern because it's not properly exercising the user-facing experience (e.g. you could make publicly source-breaking changes without realizing it).

[mrdziuban]

I'm open to it and happy to include it as part of this if there's another package people agree on

[mrdziuban]

I went ahead and moved all tests to the scalacheck.tests package. Happy to change or revert it if we discover a better path forward

[mrdziuban]

@satorg is there any chance you could review this? I have a hack in place in my codebase to do the same but would love to bring the change to all scalacheck users and resolve #969. Thanks in advance!

[satorg]

@mrdziuban thank you for the ping. To be honest, I forgot about your PR a little bit, sorry about that.

I'm trying to wrap my head around the proposed changes, so my judgements may or may not be correct, feel free to argue please – I am totally open to change my mind.

• Adds new Prop.label, Prop.:|, and Prop.|: methods that take a by-name parameter
• Marks the old Prop.label, Prop.:|, and Prop.|: with by-value parameters as private[scalacheck]

That assumes – we're changing the behavior of the existing APIs, aren't we?
I believe the initial intent for labels was simply to provide constant-string values – this is why they were made just values.

I am pretty sure there are a lot of tests around that do exactly that, i.e. just use constant strings as a clue.
Since by-name parameters come at the price of an additional allocation, then some of such tests (that happen to use labels a lot) can start experiencing unwanted effects.

Moreover, there are also labels available for Gen and those are not deferred either.
Therefore to me it seems that making labels deferred for Prop but keeping them as values for Gen might be quite confusing.

Wrapping up, perhaps (maybe) instead of replacing the existing behavior silently, it would make sense to provide an alternative APIs for lazy labels. I.e. similar to List vs LazyList in Scala: add lazyLabel and keep the existing label intact. Also I think we could adopt the idea of # symbol in shortcuts similar to the Scala library: :| and #:| for label and lazyLabel correspondingly.

[mrdziuban]

@satorg no problem, thanks for taking a look!

Yes, this does change the behavior of existing APIs, but in a way that I think will benefit more users than it will hurt.

the initial intent for labels was simply to provide constant-string values

Perhaps, but in my codebase (and I would guess a number of others) I use the labels to provide debug info so someone looking at a test failure has somewhere to start.

making labels deferred for Prop but keeping them as values for Gen might be quite confusing

Interesting point. My thinking was that the different behavior made sense because a Gen is always used in a test, whereas the Prop's label is only used if the Prop fails. When does the label attached to a Gen appear to a user?

perhaps (maybe) instead of replacing the existing behavior silently, it would make sense to provide an alternative APIs for lazy labels

I'm not opposed to this and would be willing to implement it if we decide it's the best path forward, I just might suggest using :#| and |#: to preserve the operator associativity

[noresttherein]

So, is this going to be integrated to master in some form? What is the current obstacle to do so?

[mrdziuban]

@satorg gentle ping, do you have any thoughts on this?

[satorg]

@mrdziuban ,

My thinking was that the different behavior made sense because a Gen is always used in a test, whereas the Prop's label is only used if the Prop fails. When does the label attached to a Gen appear to a user?

To my best knowledge, pretty much the same thing – when a test fails:

import org.scalacheck._

object LabelsTestApp extends Properties("LabelsTestApp") {
  val myIntGen: Gen[Int] = Arbitrary.arbitrary[Int].label("myIntGen")

  def myProp(n: Int): Prop = (n % 2) == 1

  property("check_it_out") =
    Prop.forAllNoShrink(Arbitrary.arbitrary[Int], myIntGen) { (n1: Int, n2: Int) =>
      myProp(n1).label("default-gen") || myProp(n2).label("custom-gen")
    }
}

which results to:

failing seed for LabelsTestApp.check_it_out is zs1V5_ibhRwBRBjoYrGG-0-Y-S9DKSrbbuSHaZqQgYF=
! LabelsTestApp.check_it_out: Falsified after 7 passed tests.
> Labels of failing property: 
default-gen
custom-gen
> ARG_0: -628739385
> myIntGen: -1
Found 1 failing properties.

[satorg]

My initial idea was to keep the existing functionality intact while add a new one as necessary (i.e. via lazyLabel or something). However, I might not be the best person to justify that. To be honest, I have never used labels with Prop, although I do use labels with Gen sometimes (basically when there are a bunch of custom generators in my code and I want to make it clearer which of those provides which data).

My assumption was that adding new functionality without changing the existing one has its advantages:

• no need to move tests around (so if it is still desired, it can be addressed separately);
• no unexpected changes neither in behavior nor memory footprint for existing tests;

But perhaps the existing functionality in regards to labels can be considered so bad that it might be worth changing anyway...

[som-snytt]

The linked PR proposes =|= for this purpose. That gives the op the desirable precedence, since one side is likely to be a boolean expression a && b.

I'd have to be shown that there is a performance consequence. But I haven't been researching; I just needed to do some scalachecking today.

There may be future hiccups or restrictions with right-associative operators in Scala 3. It's not a terrible idea to avoid them.

[mrdziuban]

I'm cool with =|=, I definitely see the binary compatibility benefit of defining a new operator instead of modifying the existing one. Just added some comments to your PR @som-snytt, and feel free to close this if that one seems preferable @satorg.