Skip to content

Hide ambiguous implicit buildableSeq#788

Merged
larsrh merged 1 commit intotypelevel:mainfrom
ashawley:rm-buildableSeq
Apr 10, 2021
Merged

Hide ambiguous implicit buildableSeq#788
larsrh merged 1 commit intotypelevel:mainfrom
ashawley:rm-buildableSeq

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