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

add unzip to Functor #3062

Merged
merged 8 commits into from
Nov 6, 2019

Conversation

gagandeepkalra
Copy link
Contributor

@gagandeepkalra gagandeepkalra commented Sep 14, 2019

addresses #3023

@gagandeepkalra
Copy link
Contributor Author

gagandeepkalra commented Sep 14, 2019

I need some help with this weird compiler situation

[error] /home/travis/build/typelevel/cats/core/src/main/scala/cats/Functor.scala:56:21: not found: type A
[error] def widen[A, B >: A](fa: F[A]): F[B] = fa.asInstanceOf[F[B]]
[error] ^
[error] /home/travis/build/typelevel/cats/core/src/main/scala/cats/Functor.scala:12:2: type arguments [C,B] do not conform to method widen's type parameter bounds [A,B >: A]
[error] @typeclass trait Functor[F[_]] extends Invariant[F] { self =>
[error] ^
[error] two errors found

the current version won't compile, but if I just introduce a dummy parameter to unzip method, it works just fine.

def unzip[A, B](dummy: Int, fab: F[(A, B)]): (F[A], F[B]) = (map(fab)(_.1), map(fab)(._2))

Similarly if I remove the B >: A constraint from widen, it again works just fine.

@LukaJCB @kailuowang

@oleg-py
Copy link

oleg-py commented Sep 25, 2019

@gagandeepkalra looks like an issue with Simulacrum-generated extension method (from @typeclass annotation).

Annotating unzip with @noop helps too, and I think we have to do it anyway for bincompat reasons.

@codecov-io
Copy link

codecov-io commented Sep 27, 2019

Codecov Report

Merging #3062 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3062      +/-   ##
==========================================
+ Coverage   93.17%   93.17%   +<.01%     
==========================================
  Files         372      372              
  Lines        7182     7183       +1     
  Branches      198      188      -10     
==========================================
+ Hits         6692     6693       +1     
  Misses        490      490
Flag Coverage Δ
#scala_version_212 93.53% <100%> (+0.02%) ⬆️
#scala_version_213 90.83% <100%> (ø) ⬆️
Impacted Files Coverage Δ
core/src/main/scala/cats/Functor.scala 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34e7c43...b01f21c. Read the comment docs.

@gagandeepkalra
Copy link
Contributor Author

gagandeepkalra commented Sep 27, 2019

Annotating unzip with @noop helps too, and I think we have to do it anyway for bincompat reasons.

@oleg-py yes thank you, this fixes the compilation. I'm not so sure of the bincompat, I still get this error

[error] * method unzip(java.lang.Object)scala.Tuple2 in trait cats.Functor is present only in current version
[error] filter with: ProblemFilters.exclude[ReversedMissingMethodProblem]
[error] java.lang.RuntimeException: Cats core: Binary compatibility check failed!

compiler suggested I add the above line to build.sbt, so I did, but I'm not very this is the right way.

@gagandeepkalra
Copy link
Contributor Author

@LukaJCB @kailuowang some eyes on this one please

@kailuowang
Copy link
Contributor

Sorry I just realized the review comments I left earlier was in pending status and not actually posted. I suggested that it's odd that the mima bin compat check failed. I am curious, have you merged in the master?

@gagandeepkalra
Copy link
Contributor Author

@kailuowang @oleg-py I updated the method signature.

def unzip[A, A1, A2](fab: F[A])(implicit asPair: A => (A1, A2)): (F[A1], F[A2])

This fixed the Simulacrum error, we won't need @noop now.

# Conflicts:
#	core/src/main/scala/cats/Functor.scala
@kailuowang
Copy link
Contributor

def unzip[A, A1, A2](fab: F[A])(implicit asPair: A => (A1, A2)): (F[A1], F[A2])

If I am not mistaken this relies on the implicit $comply in Predef. I am not familiar with this approach. But at the very least, it obscures the type signature. Can you confirm again that the original version proposed in the issue breaks bincompat after you merged in master?

@gagandeepkalra
Copy link
Contributor Author

But at the very least, it obscures the type signature.

yes, agreed. I was trying to avoid using @noop here, since we no longer need it for bincompat reasons, which got fixed with the recent master as you suggested.

@travisbrown
Copy link
Contributor

This looks fine to me.

@travisbrown travisbrown added this to the 2.1.0-RC1 milestone Nov 5, 2019
@LukaJCB LukaJCB merged commit 254ec63 into typelevel:master Nov 6, 2019
@gagandeepkalra gagandeepkalra deleted the functor/addUnzipFunction branch November 6, 2019 11:11
@gagandeepkalra
Copy link
Contributor Author

Thank you everyone

@yangzai
Copy link
Contributor

yangzai commented Mar 11, 2020

(accidentally closed and reopened pr with an incomplete comment...)

@travisbrown As I was saying, the original attempt was a direct fix in the Functor trait that work with Simulacrum so it would be slightly beneficial if the intention is to move as much back to a single typeclass/trait when bincompat isn't an issue. However, having FunctorTupleOps per complex shape like F[(A, B)] seems like a defacto convention you would want to keep to. Let me know your thoughts.

On a side note regarding #3062 (comment) , having the type signature of widen changed to something like

  def widen[A, B: A <:< *](fa: F[A]): F[B] = fa.asInstanceOf[F[B]]

would fix(i.e. not require @noop for unzip) but that would break bincompat. It's probably not equivalent but just an interesting note.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants