Fix alleycats Set Functor ambiguous implicits

[LaurenceWarne]

Hi, on 2.12.0 I find that I get the error:

ambiguous implicit values:
  both value alleyCatsStdSetMonad in trait SetInstances of type cats.Monad[Set] with cats.Alternative[Set]
  and value alleyCatsSetTraverse in trait SetInstances of type cats.Traverse[Set]
  match expected type cats.Functor[Set] (lsp)

With for example the setup:

import alleycats.std.set._

object test {
  implicitly[cats.Functor[Set]]
}

To try and fix I've used the approach here: https://typelevel.org/cats/guidelines.html#implicit-instance-priority, which fixes the original problem though introduces binary compatibility issues which I don't know how to fix!:

[error]  * abstract synthetic method alleycats$std$SetInstances0$_setter_$alleyCatsStdSetMonad_=(cats.Monad)Unit in interface alleycats.std.SetInstances0 is inherited by class SetInstances in current version.
[error]    filter with: ProblemFilters.exclude[InheritedNewAbstractMethodProblem]("alleycats.std.SetInstances.alleycats$std$SetInstances0$_setter_$alleyCatsStdSetMonad_=")
[error]  * abstract synthetic method alleycats$std$SetInstances1$_setter_$alleyCatsSetTraverse_=(cats.Traverse)Unit in interface alleycats.std.SetInstances1 is inherited by class SetInstances in current version.
[error]    filter with: ProblemFilters.exclude[InheritedNewAbstractMethodProblem]("alleycats.std.SetInstances.alleycats$std$SetInstances1$_setter_$alleyCatsSetTraverse_=")

Suggestions very welcome! Thanks

[satorg]

@LaurenceWarne , thank you for your help!

Regarding the original issue:

ambiguous implicit values:
both value alleyCatsStdSetMonad in trait SetInstances of type cats.Monad[Set] with cats.Alternative[Set]
and value alleyCatsSetTraverse in trait SetInstances of type cats.Traverse[Set]
match expected type cats.Functor[Set] (lsp)

In theory, all these instances (i.e., Traverse, Monad, Alternative, etc) can be (and usually are) gathered in a single implementation class and therefore exposed by a single implicit val. For instance, see

cats/core/src/main/scala/cats/instances/list.scala

Lines 36 to 38 in 1cc04ec

	implicit val catsStdInstancesForList
	: Traverse[List] with Alternative[List] with Monad[List] with CoflatMap[List] with Align[List] =
	new Traverse[List] with Alternative[List] with Monad[List] with CoflatMap[List] with Align[List] {

I'm not sure why in alleycats these instances were separated in the first place. Perhaps, it was just an oversight. I seems that if we merged them in just one implementor, then it would solve the issue. Perhaps, there were a reason to keep them separated, but I feel it may make sense to give it a shot and merge them. And if the effort fails for some reason, it would be nice to leave a comment about it in the sources, because for now it is not obvious at all.

Regarding the binary compatibility issue:
In the case when an implicit val is intentionally removed from a class or trait (and Mima apparently does not like it), the simplest way to overcome that is to remove implicit specifier from the val and furthermore make that val package-private. It should make Mima pretty happy. In other words, do not remove the implicit val physically, but rather un-implicit it and hide.

[LaurenceWarne]

In theory, all these instances (i.e., Traverse, Monad, Alternative, etc) can be (and usually are) gathered in a single implementation class and therefore exposed by a single implicit val. For instance, see

cats/core/src/main/scala/cats/instances/list.scala

Lines 36 to 38 in 1cc04ec

	implicit val catsStdInstancesForList
	: Traverse[List] with Alternative[List] with Monad[List] with CoflatMap[List] with Align[List] =
	new Traverse[List] with Alternative[List] with Monad[List] with CoflatMap[List] with Align[List] {

I'm not sure why in alleycats these instances were separated in the first place. Perhaps, it was just an oversight. I seems that if we merged them in just one implementor, then it would solve the issue. Perhaps, there were a reason to keep them separated, but I feel it may make sense to give it a shot and merge them. And if the effort fails for some reason, it would be nice to leave a comment about it in the sources, because for now it is not obvious at all.

Makes sense, and FWIW moving all to one implementor doesn't appear to cause any test failures.

Regarding the binary compatibility issue: In the case when an implicit val is intentionally removed from a class or trait (and Mima apparently does not like it), the simplest way to overcome that is to remove implicit specifier from the val and furthermore make that val package-private. It should make Mima pretty happy. In other words, do not remove the implicit val physically, but rather un-implicit it and hide.

I may have misunderstood 😅 , but after moving to a common implementer and changing the old implicits:

trait SetInstances {
  private[std] val alleyCatsSetTraverse: Traverse[Set] = alleycatsStdInstancesForSet
  private[std] val alleyCatsStdSetMonad: Monad[Set] with Alternative[Set] = alleycatsStdInstancesForSet
  private[std] val alleyCatsSetTraverseFilter: TraverseFilter[Set] = alleycatsStdInstancesForSet

  implicit val alleycatsStdInstancesForSet
    : Monad[Set] with Alternative[Set] with Traverse[Set] with TraverseFilter[Set] = ...

I run into a few different mima issues:

[error]  * abstract synthetic method alleycats$std$SetInstances$_setter_$alleycatsStdInstancesForSet_=(cats.Monad)Unit in interface alleycats.std.SetInstances is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("alleycats.std.SetInstances.alleycats$std$SetInstances$_setter_$alleycatsStdInstancesForSet_=")
[error]  * abstract method alleycatsStdInstancesForSet()cats.Monad in interface alleycats.std.SetInstances is present only in current version
[error]    filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]("alleycats.std.SetInstances.alleycatsStdInstancesForSet")
[error]  * static method alleyCatsSetTraverseFilter()cats.TraverseFilter in class alleycats.std.all does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("alleycats.std.all.alleyCatsSetTraverseFilter")
[error]  * static method alleyCatsSetTraverse()cats.Traverse in class alleycats.std.all does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("alleycats.std.all.alleyCatsSetTraverse")
[error]  * static method alleyCatsStdSetMonad()cats.Monad in class alleycats.std.all does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("alleycats.std.all.alleyCatsStdSetMonad")
[error]  * static method alleyCatsSetTraverseFilter()cats.TraverseFilter in class alleycats.std.set does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("alleycats.std.set.alleyCatsSetTraverseFilter")
[error]  * static method alleyCatsSetTraverse()cats.Traverse in class alleycats.std.set does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("alleycats.std.set.alleyCatsSetTraverse")
[error]  * static method alleyCatsStdSetMonad()cats.Monad in class alleycats.std.set does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("alleycats.std.set.alleyCatsStdSetMonad")

[satorg]

Ah, that's right – Mima gets triggered if you change visibility from public to package-private. I misguided you, sorry. Now I'm recalling that in cases like this one the approach is usually not to change the visibility but rather mark it @deprecated (along with stripping off all implicit whatever), e.g.:

cats/kernel/src/main/scala/cats/kernel/instances/SortedMapInstances.scala

Lines 51 to 53 in 1cc04ec

	@deprecated("Use catsKernelStdHashForSortedMap override without Order", "2.2.0-M3")
	def catsKernelStdHashForSortedMap[K, V](hashK: Hash[K], orderK: Order[K], hashV: Hash[V]): Hash[SortedMap[K, V]] =
	new SortedMapHash[K, V]()(hashV, hashK)

Here, hashK: Hash[K], orderK: Order[K], hashV: Hash[V] along with catsKernelStdHashForSortedMap itself were implicit before it was deprecated.

[satorg]

Note, that it is not necessary to merge the TraverseFilter instance with all the others because it has no base type class in common. So technically, you can keep it separated as before. On the other hand I guess it won't hurt if you merge them, probably.

[LaurenceWarne]

Ah, that's right – Mima gets triggered if you change visibility from public to package-private. I misguided you, sorry. Now I'm recalling that in cases like this one the approach is usually not to change the visibility but rather mark it @deprecated (along with stripping off all implicit whatever), e.g.:

Nice, thanks that looks to have worked! Another thing was that I had to use implicit def rather than implicit val for the new combined instance, else I got a ReversedMissingMethodProblem error from mima, I hope that's ok.

Also, I wasn't sure what best to put in for since in the deprecations - please shout if it should be something else.

[satorg]

Hmm.. Even without Mima the statement like

val alleyCatsSetTraverse: Traverse[Set] = alleycatsStdInstancesForSet

where alleycatsStdInstancesForSet is val and defined down below in the trait
won't work
because it leads to the forward reference issue.
Perhaps, Mima tries to tell just about that but in its own way.

On the other hand, using implicit def is not exactly very optimal because it will result in reallocating a new instance every time it is demanded. Certainly, there are some cases where implicit def cannot be avoided, but as a rule of thumb Cats attempts to keep the number of allocations as minimal as possible.

Would you give it a shot and make implicit val alleycatsStdInstancesForSet again and move it before any dependent declaration occurs in the trait please?