Skip to content
New issue

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

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

Already on GitHub? # to your account

Remove redundant Monad constraints from Parallel syntax #3856

Merged
merged 1 commit into from
Apr 18, 2021

Conversation

joroKr21
Copy link
Member

@joroKr21 joroKr21 commented Apr 8, 2021

Binary compatible version of #2180
Fixes #2254

implicit final def catsSyntaxParallelSequence[T[_]: Traverse, M[_]: Monad, A](
tma: T[M[A]]
): ParallelSequenceOps[T, M, A] = new ParallelSequenceOps[T, M, A](tma)
@deprecated("Kept for binary compatibility", "2.5.1")
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure if this should be 2.5.1 or 2.6.0

@joroKr21
Copy link
Member Author

joroKr21 commented Apr 9, 2021

Since this is not working, the other option is to create new AnyVals - should I go for it?

@tnielens
Copy link
Contributor

tnielens commented Apr 9, 2021

Would something along this line be binary compatible?

final class ParallelTraversableOps[T[_], A](private val ta: T[A]) extends AnyVal {
  def parTraverse[M[_]: Monad, B](f: A => M[B])(implicit T: Traverse[T], P: Parallel[M]): M[T[B]] =
    Parallel.parTraverse(ta)(f)
}

final class ParallelTraversableOps2[T[_], A](private val ta: T[A]) extends AnyVal {
  def parTraverse[M[_], B](f: A => M[B])(implicit T: Traverse[T], P: Parallel[M]): M[T[B]] =
    Parallel.parTraverse(ta)(f)
}

@joroKr21
Copy link
Member Author

joroKr21 commented Apr 9, 2021

Would something along this line be binary compatible?

Yes, that's what I meant. Unfortunately adding an overload in Scala 2.12 changes the name mangling of extension methods.
There is also an opportunity to consolidate extension methods (if we drop the constraints from the implicit conversions).

@@ -108,9 +123,11 @@ trait ParallelFoldMapASyntax {
}

final class ParallelTraversableOps[T[_], A](private val ta: T[A]) extends AnyVal {
def parTraverse[M[_]: Monad, B](f: A => M[B])(implicit T: Traverse[T], P: Parallel[M]): M[T[B]] =
@deprecated("Kept for binary compatibility", "2.5.1")
def parTraverse[M[_], B](f: A => M[B], M: Monad[M], T: Traverse[T], P: Parallel[M]): M[T[B]] =
Copy link
Contributor

@tnielens tnielens Apr 9, 2021

Choose a reason for hiding this comment

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

shouldn't this have a second third parameter list instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

WDYM? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

You moved the monad constraint in the first position of the implicit parameter list. My understanding of context bounds is that a new parameter list is appended to the method definition in this way:

   def parTraverse[M[_]: Monad, B](f: A => M[B])(implicit T: Traverse[T], P: Parallel[M]): M[T[B]] = 
   // is equivalent to
    def parTraverse[M[_], B](f: A => M[B])(implicit T: Traverse[T], P: Parallel[M])(implicit M: Monad[M]): M[T[B]] = 

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, in Scala 2 you can have only one implicit list and the context bounds come first.

Copy link
Contributor

Choose a reason for hiding this comment

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

Didn't know that, thanks for the explanation.

Copy link
Member Author

Choose a reason for hiding this comment

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

But in the bytecode there is only one parameter list anyway (and no implicits) which is why both are binary compatible:

def parTraverse[M[_]: Monad, B](f: A => M[B])(implicit T: Traverse[T], P: Parallel[M]): M[T[B]]
def parTraverse[M[_], B](f: A => M[B], M: Monad[M], T: Traverse[T], P: Parallel[M]): M[T[B]]

That's a neat trick for library authors 😄

@joroKr21 joroKr21 marked this pull request as ready for review April 9, 2021 14:02
larsrh
larsrh previously approved these changes Apr 10, 2021
Copy link
Contributor

@larsrh larsrh left a comment

Choose a reason for hiding this comment

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

LGTM, but I think this warrants a bump to 2.6.0

@larsrh larsrh merged commit c993049 into typelevel:main Apr 18, 2021
@joroKr21 joroKr21 deleted the parallel-syntax branch April 18, 2021 13:33
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Syntax extensions for Parallel require Monad[M], but implementation doesn't
4 participants