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

Hide ambiguous implicit buildableSeq #788

Merged
merged 1 commit into from
Apr 10, 2021

Conversation

ashawley
Copy link
Contributor

@ashawley ashawley commented Apr 8, 2021

Fixes #721.

@ashawley ashawley changed the title Remove ambiguous implicit buildableSeq Hide ambiguous implicit buildableSeq Apr 9, 2021
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

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.

@larsrh larsrh merged commit 4accdb0 into typelevel:main Apr 10, 2021
@ashawley ashawley deleted the rm-buildableSeq branch April 10, 2021 15:08
@ashawley ashawley added this to the 1.15.4 milestone Apr 16, 2021
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.

Ambiguous implicit values
2 participants