Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Defer evaluation of Prop labels #979

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mrdziuban
Copy link

@mrdziuban mrdziuban commented Jul 5, 2023

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
Copy link
Author

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 mrdziuban force-pushed the by-name-prop-labels branch 2 times, most recently from debc313 to d211c81 Compare July 5, 2023 19:27
@mrdziuban
Copy link
Author

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

s"average = $avg" |: avg >= 0.49 && avg <= 0.51
(avg >= 0.49 && avg <= 0.51).labelImpl2(s"average = $avg")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 mrdziuban force-pushed the by-name-prop-labels branch 3 times, most recently from 7ab14ce to a6dfec7 Compare July 6, 2023 13:40
@mrdziuban
Copy link
Author

@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
Copy link
Contributor

satorg commented Oct 27, 2023

@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
Copy link
Author

@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
Copy link

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

@mrdziuban
Copy link
Author

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

@satorg
Copy link
Contributor

satorg commented Jan 10, 2024

@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
Copy link
Contributor

satorg commented Jan 10, 2024

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
Copy link
Contributor

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants