Drop noop for Functor unzip with Liskov evidence #3318

yangzai · 2020-02-24T19:52:42Z

The main motivation of this PR is to reduce the verbosity of calling unzip.

As mentioned by @kailuowang in #3062 (comment) the $conforms related implicit conversion obscures type signature.

This PR proposes an alternative solution with an implicit Liskov evidence that requires explicit conversions instead.

travisbrown · 2020-02-25T09:22:41Z

We can't change the signature of the unzip in Functor before 3.0, but we could probably just add an unzip to FunctorSyntax (possibly with a workaround like this if that's necessary)? We've done that in many other cases where Simulacrum didn't work for some reason.

yangzai · 2020-02-25T13:32:03Z

@travisbrown thanks for the feedback. I'm looking at how it it was implemented here: #3234 . Hope I'm on the same page.

codecov-io · 2020-02-25T16:14:54Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #3318      +/-   ##
==========================================
+ Coverage   93.41%   93.42%   +<.01%     
==========================================
  Files         378      378              
  Lines        7629     7631       +2     
  Branches      206      201       -5     
==========================================
+ Hits         7127     7129       +2     
  Misses        502      502

Flag	Coverage Δ
#scala_version_212	`93.49% <100%> (ø)`	⬆️
#scala_version_213	`93.2% <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 bce605e...1cac349. Read the comment docs.

yangzai · 2020-02-25T16:55:49Z

core/src/main/scala/cats/Functor.scala

+  //TODO: Drop from Cats 3.0
+  @noop
+  def unzip[A, B](fab: F[(A, B)]): (F[A], F[B]) = (map(fab)(_._1), map(fab)(_._2))
+
+  //TODO: Drop `final` when the `noop` version deprecates
+  // `final` restricts functor instance overriding of `unzip` to the noop version
+  final def unzip[A, X, Y](fa: F[A])(implicit ev: A <~< (X, Y)): (F[X], F[Y]) = {
    val fxy = map(fa)(ev)

-    (map(fxy)(_._1), map(fxy)(_._2))
+    unzip(fxy)
  }


@travisbrown Overloading seems to work as well but I'm not sure if there are any negative implication.

@yangzai I think it's a good idea to avoid overloading when it's not absolutely necessary—see e.g. this SO answer for some reasons. Not everything there is still applicable in the latest Scala versions, but some is. For example, this is a nicer error message:

scala> import cats.Functor, cats.instances.all._ import cats.Functor import cats.instances.all._ scala> Functor[Option].unzip(1) ^ error: type mismatch; found : Int(1) required: Option[(?, ?)]

…than this:

scala> Functor[Option].unzip(1) <console>:16: error: overloaded method value unzip with alternatives: [A, X, Y](fa: Option[A])(implicit ev: A <~< (X, Y))(Option[X], Option[Y]) <and> [A, B](fab: Option[(A, B)])(Option[A], Option[B]) cannot be applied to (Int) Functor[Option].unzip(1) ^

If we had to do this to get the syntax method, it'd be different, but I don't think overloading is worth it just to get Simulacrum to generate the syntax method. Implementing it explicitly in FunctorOps is also more consistent with the approach we use in other cases where Simulacrum hasn't done what we needed.

@travisbrown Yeah it was a lazy hack. Sorry was kind of busy the past week. Will reference #3234 for the FunctorOps approach if the <~< based type signature is fine going forward.

codecov-io · 2020-02-25T18:10:46Z

Codecov Report

Merging #3318 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3318      +/-   ##
==========================================
+ Coverage   93.32%   93.34%   +0.01%     
==========================================
  Files         378      379       +1     
  Lines        7689     7694       +5     
  Branches      206      206              
==========================================
+ Hits         7176     7182       +6     
+ Misses        513      512       -1

Flag	Coverage Δ
#scala_version_212	`93.40% <100.00%> (+0.04%)`	⬆️
#scala_version_213	`93.12% <100.00%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
core/src/main/scala/cats/syntax/functor.scala	`100.00% <100.00%> (ø)`
core/src/main/scala/cats/instances/either.scala	`100.00% <0.00%> (ø)`
core/src/main/scala/cats/syntax/either.scala	`83.33% <0.00%> (+0.24%)`	⬆️
core/src/main/scala/cats/instances/option.scala	`100.00% <0.00%> (+1.69%)`	⬆️

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 fc23079...aff4a51. Read the comment docs.

…3234) * backported unzip method in functor typeclass * fixed nonfunctioning doctest

…ementation

core/src/main/scala/cats/syntax/functor.scala

…n into FunctorSyntax

travisbrown · 2020-03-11T09:17:08Z

Is there any reason not to just write the following?

final class FunctorTupleOps[F[_], A, B](private val fab: F[(A, B)]) extends AnyVal {
  def unzip(implicit F: Functor[F]): (F[A], F[B]) = F.unzip(fab)
}

I just tried it with your tests and everything seems fine.

johnynek · 2020-08-31T00:00:09Z

core/src/main/scala/cats/syntax/functor.scala

+   * }}}
+   *
+   */
+  def unzip[X, Y](implicit F: Functor[F], ev: A <~< (X, Y)): (F[X], F[Y]) = F.unzip(F.map(fa)(ev))


I don't think we should use map with ev. Instead, we should use substitution to avoid any chance we do any work. If instead we took ev: A Is (X, Y), then we can use F.unzip(ev.substitute[F](fa))

I think it is fine to use Is rather than As (<~<) because in practice we never really have subclasses of tuples. We don't know that F is covariant, even though I think effectively it has to be in order to be a functor, but we didn't require that on the trait.

@johnynek sorry, I was MIA for awhile. Let me have a look, will get back to you sometime this week.

@johnynek I've done some testing withIs/Leibinz. I had to flip the type parameters A and (X, Y) around to make it work, i.e:

def unzip[X, Y](implicit F: Functor[F], ev: (X, Y) Is A): (F[X], F[Y]) = F.unzip(ev.flip.substitute(fa))

Also, because tuples are covariant, there is still a possibility of a Tuple2 to be a subtype of another Tuple2, eg:

sealed trait Animal final case class Cat() extends Animal final case class Dog() extends Animal val l: NonEmptyList[(Cat, Dog)] = NonEmptyList one Tuple2(Cat(), Dog()) Functor[NonEmptyList].unzip[Animal, Animal](l) //compiles for both `Is` and `As` l.unzip[Animal, Animal] //does not compile for `Is`

This would result in slight inconsistency between the prefix and postfix versions of unzip.

As for As I've tried to replace map with widen like this:

def unzip[X, Y](implicit F: Functor[F], ev: A <~< (X, Y)): (F[X], F[Y]) = F.unzip(F.widen[A, (X, Y)](fa))

but it would not fit the type bound. Otherwise, calling asInstanceOf directly would work:

def unzip[X, Y](implicit F: Functor[F], ev: A <~< (X, Y)): (F[X], F[Y]) = F.unzip(fa.asInstanceOf[F[(X, Y)]])

or we could implement another widen that requires an evidence of As instead of a type bound constraint:

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

what about this:

final class FunctorTuple2Ops[F[_], A, B](private val fa: F[(A, B)]) extends AnyVal { def _1F(implicit F: Functor[F]): F[A] = F.map(fa)(_._1) def _2F(implicit F: Functor[F]): F[B] = F.map(fa)(_._2) def swapF(implicit F: Functor[F]): F[(B, A)] = F.map(fa)(_.swap) def unzip(implicit F: Functor[F]): (F[A], F[B]) = F.unzip(fa) }

or something... sidestep using the evidence at all and only put the enrichment on Tuple2 types?

actually I see @travisbrown suggested this originally before I derailed things: #3318 (comment)

@johnynek @travisbrown I've made the change to FunctorTuple2Ops. I think some of my comments on that might have gone to the wrong PR when I accidentally closed and reopened this PR. The original intention was to fix unzip directly in the Functor typeclass but either should be fine now since a method extension would be required either way. We might still need type evidence for efficientunzip for specific types.

codecov-commenter · 2020-09-09T15:30:54Z

Codecov Report

Merging #3318 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3318   +/-   ##
=======================================
  Coverage   91.31%   91.32%           
=======================================
  Files         386      387    +1     
  Lines        8617     8622    +5     
  Branches      243      248    +5     
=======================================
+ Hits         7869     7874    +5     
  Misses        748      748

…ple2 extension methods

johnynek · 2020-09-11T16:23:07Z

I think this looks good other than a comment about the Functor to use in the doc tests.

yangzai · 2020-09-11T17:45:57Z

I think this looks good other than a comment about the Functor to use in the doc tests.

@johnynek do you mean that the doc tests with "Lifts XXX to Functor" should to be more concise about the shape of the functor?

johnynek · 2020-09-11T17:48:05Z

I mean doing doc tests using Id[(Int, Int)] might be less enlightening to new comers since Id seems useless to newcomers. Option would be just about as easy to write the doc test examples and wouldn't be as trivial.

johnynek · 2020-09-11T16:22:36Z

core/src/main/scala/cats/syntax/functor.scala

+   * scala> import cats.Id
+   * scala> import cats.syntax.functor._
+   *
+   * scala> ((1, 2): Id[(Int, Int)])._2F == 2


I think these docs and test would be nicer with a more interesting Functor than Id. Maybe Option?

@johnynek the doc test for unzip was ported from the 2.11 branch so I decided to keep to the same type. Tried Option but it failed for 2.13 because Option#unzip is already defined in 2.13. Switched to Chain as it is Cats specific.

@johnynek is Chain fine or should replace it with something else?

johnynek · 2020-09-18T18:17:57Z

@travisbrown can you double check before we merge? Any time we change API surface area I like to make sure there is good consensus.

Side note: it would be interesting if mima could tell you if a patch does or doesn't change surface area and we could label each PR. PRs that only change implementations for which there is coverage can be merged with less review IMO.

travisbrown · 2020-09-18T18:55:37Z

@johnynek Sure, I'll take a look tomorrow.

tests/src/test/scala/cats/tests/FunctorSuite.scala

Drop noop for Functor unzip with Liskov evidence

3f19e23

Overload unzip for bin compat

35a5f97

yangzai commented Feb 25, 2020

View reviewed changes

Make type params explicit

1cac349

gagandeepkalra and others added 3 commits March 9, 2020 03:00

Backport unzip method in functor typeclass for scala_2.11 (typelevel#…

999f735

…3234) * backported unzip method in functor typeclass * fixed nonfunctioning doctest

Clean up cherry-picked forwardport

08c1ad8

Replace UnzipFunctorOps with FunctorOps0 with Liskov based unzip impl…

47312b4

…ementation

travisbrown suggested changes Mar 9, 2020

View reviewed changes

core/src/main/scala/cats/syntax/functor.scala Outdated Show resolved Hide resolved

core/src/main/scala/cats/syntax/functor.scala Outdated Show resolved Hide resolved

yangzai added 3 commits March 10, 2020 05:19

Merge branch 'master' into hy-unzip-liskov

da2980f

Drop bincompat traits and numerical suffixes, move implicit conversio…

ffcaac1

…n into FunctorSyntax

Refactored unzip

aff4a51

yangzai requested a review from travisbrown March 10, 2020 17:06

yangzai closed this Mar 11, 2020

yangzai reopened this Mar 11, 2020

johnynek reviewed Aug 31, 2020

View reviewed changes

Merge branch 'master' into hy-unzip-liskov

ce88554

yangzai added 4 commits September 10, 2020 00:23

Fix formatting

052ee9c

Add missing assertions and rewrite unzip compilation test for munit

059ec70

Reimplement Functor unzip (together with other helpful methods) as Tu…

295d9ad

…ple2 extension methods

Switch NEL to List, not testing unzip here

2e9d877

yangzai requested a review from johnynek September 11, 2020 09:11

Switch Id to Option for doctests

c298b7e

johnynek previously approved these changes Sep 11, 2020

View reviewed changes

Fix test name grammar

49ad8b1

yangzai dismissed johnynek’s stale review via 49ad8b1 September 11, 2020 19:32

Switch to Chain as Option#unzip already defined in Scala 2.13

3a6b1f2

yangzai requested a review from johnynek September 12, 2020 07:33

barambani previously approved these changes Sep 17, 2020

View reviewed changes

johnynek previously approved these changes Sep 17, 2020

View reviewed changes

Merge branch 'master' into hy-unzip-liskov

22b52e2

larsrh added this to the 2.4 milestone Feb 5, 2021

larsrh reviewed Feb 5, 2021

View reviewed changes

tests/src/test/scala/cats/tests/FunctorSuite.scala Outdated Show resolved Hide resolved

Update tests/src/test/scala/cats/tests/FunctorSuite.scala

79b6f06

larsrh dismissed stale reviews from johnynek and barambani via 79b6f06 February 5, 2021 21:08

larsrh approved these changes Feb 6, 2021

View reviewed changes

larsrh merged commit 1edbbea into typelevel:master Feb 6, 2021

armanbilge mentioned this pull request Feb 8, 2022

Add F[TupleN] syntax #4125

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop noop for Functor unzip with Liskov evidence #3318

Drop noop for Functor unzip with Liskov evidence #3318

yangzai commented Feb 24, 2020

travisbrown commented Feb 25, 2020

yangzai commented Feb 25, 2020

codecov-io commented Feb 25, 2020 •

edited

Loading

yangzai Feb 25, 2020 •

edited

Loading

travisbrown Feb 26, 2020

yangzai Mar 4, 2020

codecov-io commented Feb 25, 2020 •

edited

Loading

travisbrown commented Mar 11, 2020

johnynek Aug 31, 2020 •

edited

Loading

yangzai Sep 6, 2020

yangzai Sep 9, 2020

johnynek Sep 9, 2020 •

edited

Loading

johnynek Sep 9, 2020

yangzai Sep 11, 2020

codecov-commenter commented Sep 9, 2020 •

edited

Loading

johnynek commented Sep 11, 2020

yangzai commented Sep 11, 2020

johnynek commented Sep 11, 2020

johnynek Sep 11, 2020

yangzai Sep 11, 2020

yangzai Sep 16, 2020

johnynek commented Sep 18, 2020

travisbrown commented Sep 18, 2020

Drop noop for Functor unzip with Liskov evidence #3318

Drop noop for Functor unzip with Liskov evidence #3318

Conversation

yangzai commented Feb 24, 2020

travisbrown commented Feb 25, 2020

yangzai commented Feb 25, 2020

codecov-io commented Feb 25, 2020 • edited Loading

Codecov Report

yangzai Feb 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 25, 2020 • edited Loading

Codecov Report

travisbrown commented Mar 11, 2020

johnynek Aug 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek Sep 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 9, 2020 • edited Loading

Codecov Report

johnynek commented Sep 11, 2020

yangzai commented Sep 11, 2020

johnynek commented Sep 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnynek commented Sep 18, 2020

travisbrown commented Sep 18, 2020

codecov-io commented Feb 25, 2020 •

edited

Loading

yangzai Feb 25, 2020 •

edited

Loading

codecov-io commented Feb 25, 2020 •

edited

Loading

johnynek Aug 31, 2020 •

edited

Loading

johnynek Sep 9, 2020 •

edited

Loading

codecov-commenter commented Sep 9, 2020 •

edited

Loading