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.

Sign up for GitHub

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

Hide ambiguous implicit buildableSeq #788

Merged
merged 1 commit into from
Apr 10, 2021
Merged

Hide ambiguous implicit buildableSeq #788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions project/MimaSettings.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ object MimaSettings {
)

private def removedPrivateMethods = Seq(
"org.scalacheck.util.Buildable.buildableSeq"
)

private def removedPrivateClasses = Seq(
Expand Down
2 changes: 1 addition & 1 deletion src/main/scala/org/scalacheck/util/Buildable.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ object Buildable extends BuildableVersionSpecific {
def builder = new ArrayListBuilder[T]
}

implicit def buildableSeq[T]: Buildable[T, Seq[T]] =
private[scalacheck] implicit def buildableSeq[T]: Buildable[T, Seq[T]] =
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a no-go as far as binary compatibility is concerned. Instead, I propose:

  • make the definition non-implicit
  • slap a @deprecated on it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's an internal definition, so it needs to exist as an implicit.

Copy link
Contributor Author

@ashawley ashawley Apr 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think there's any reason to add @deprecated since nobody likely uses it, and it isn't visible, but I'll do so just to be thorough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Making it package-private is technically binary compatible, it's just not source compatible. It's an obscure method in an internal object, so I don't think there will be anyone who will even notice the source-breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's apparently not binary compatible in this instance, since you had to define a MiMa exception. But your argument that it this definition is not likely to be used publicly is persuasive 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually it is binary compatible. Mima is just being thorough. The emitted byte code is just not binary compatible for Java callers. ScalaCheck doesn't have any of those. And if we did, they still likely weren't using it for this implicit.

new Buildable[T, Seq[T]] {
def builder: mutable.Builder[T, Seq[T]] =
Seq.newBuilder[T]
Expand Down
27 changes: 27 additions & 0 deletions src/test/scala/scala/StringSpecification.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*-------------------------------------------------------------------------*\
** ScalaCheck **
** Copyright (c) 2007-2021 Rickard Nilsson. All rights reserved. **
** http://www.scalacheck.org **
** **
** This software is released under the terms of the Revised BSD License. **
** There is NO WARRANTY. See the file LICENSE for the full text. **
\*------------------------------------------------------------------------ */

package scala

import org.scalacheck.Gen
import org.scalacheck.Prop
import org.scalacheck.Properties

object StringSpecification extends Properties("scala.String") {

property("mkString") = { // Issue #721
import scala.collection.JavaConverters._
val listOfGens: List[Gen[Char]] = "".toList.map(Gen.const(_))
val g1: Gen[String] = Gen.sequence(listOfGens).map(_.asScala.mkString)
val g2: Gen[String] = Gen.sequence(List(Gen.listOf(' ').map(_.mkString))).map(_.asScala.mkString)
Prop.forAll(g1, g2) { (s1, s2) =>
true
}
}
}