Add scala.util.control.TailCalls.TailRec instances #3041

johnynek · 2019-09-08T20:18:02Z

Adds Defer and Monad instances for TailRec. Now that we don't support 2.10 anymore, we can also re-enable the benchmarks. For my (somewhat slow) machine, TailRec is about 17% faster than Trampoline (but much slower than Eval).

[info] Benchmark                    Mode  Cnt      Score       Error  Units    
[info] TrampolineBench.eval        thrpt    3  16603.447 ± 28185.656  ops/s    
[info] TrampolineBench.stdlib      thrpt    3   4829.133 ±  1911.742  ops/s    
[info] TrampolineBench.trampoline  thrpt    3   4152.917 ±  1948.304  ops/s

The motivation to add this are for users interacting with code that already uses TailRec and for instance interpreting Free into TailRec, or someone who wants to use TailRec with generic code that uses Defer (e.g. ContT).

We could add Semigroup, Monoid and Group to kernel. Since this is further from the main use of TailRec and independent of this current PR, I will not include it here.

johnynek · 2019-09-08T20:28:40Z

core/src/main/scala/cats/Eval.scala

@@ -378,6 +378,7 @@ sealed abstract private[cats] class EvalInstances extends EvalInstances0 {
      def flatMap[A, B](fa: Eval[A])(f: A => Eval[B]): Eval[B] = fa.flatMap(f)
      def extract[A](la: Eval[A]): A = la.value
      def coflatMap[A, B](fa: Eval[A])(f: Eval[A] => B): Eval[B] = Later(f(fa))
+      override def unit: Eval[Unit] = Eval.Unit


unrelated, but I noticed we could add this and avoid allocations for some folks that use Applicative[F].unit with Eval.

travisbrown

Can you add an object tailRec extends TailRecInstances to the instances package object? Unfortunately we've had to make these version-specific, so you'll have to do it twice.

travisbrown · 2019-09-08T20:27:51Z

core/src/main/scala/cats/Eval.scala

@@ -378,6 +378,7 @@ sealed abstract private[cats] class EvalInstances extends EvalInstances0 {
      def flatMap[A, B](fa: Eval[A])(f: A => Eval[B]): Eval[B] = fa.flatMap(f)
      def extract[A](la: Eval[A]): A = la.value
      def coflatMap[A, B](fa: Eval[A])(f: Eval[A] => B): Eval[B] = Later(f(fa))
+      override def unit: Eval[Unit] = Eval.Unit


This is unrelated?

yes. I added a comment, I just noticed it was missing while looking at Eval.

travisbrown · 2019-09-08T20:29:08Z

bench/src/main/scala/cats/bench/TrampolineBench.scala

@@ -30,14 +31,12 @@ class TrampolineBench {
      y <- Trampoline.defer(trampolineFib(n - 2))
    } yield x + y

-  // TailRec[A] only has .flatMap in 2.11.
+   @Benchmark


Good catch. Doing a search for "2.10" shows four or five other things like this we should do, but none of them look urgent.

travisbrown · 2019-09-08T20:32:28Z

core/src/main/scala-2.12-/cats/instances/all.scala

@@ -61,4 +61,7 @@ trait AllInstancesBinCompat4 extends SortedMapInstancesBinCompat1 with MapInstan

 trait AllInstancesBinCompat5 extends SortedSetInstancesBinCompat0

-trait AllInstancesBinCompat6 extends SortedSetInstancesBinCompat1 with SortedMapInstancesBinCompat2
+trait AllInstancesBinCompat6


Doesn't this break bincompat on 2.11? Only with the recent release candidates, I guess, so maybe it's fine?

I would assume so. I think we should only bump mima on actual releases (not including any pre-release).

@johnynek Right, but it's still useful to know, so we can decide whether we need to publish new pre-releases of cats-effect or Circe after a Cats pre-release, etc.

the plan is to drop 2.11 on master immediately after 2.0.0 release (this evening). So we can add this to the root trait.

codecov-io · 2019-09-08T21:21:02Z

Codecov Report

Merging #3041 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master   #3041      +/-   ##
=========================================
+ Coverage   93.57%   93.6%   +0.03%     
=========================================
  Files         371     376       +5     
  Lines        7097    7839     +742     
  Branches      185     228      +43     
=========================================
+ Hits         6641    7338     +697     
- Misses        456     501      +45

Flag	Coverage Δ
#scala_version_212	`93.88% <100%> (+0.3%)`	⬆️
#scala_version_213	`91.32% <100%> (?)`

Impacted Files	Coverage Δ
core/src/main/scala/cats/Eval.scala	`98.83% <100%> (+0.01%)`	⬆️
core/src/main/scala/cats/instances/tailrec.scala	`100% <100%> (ø)`
core/src/main/scala/cats/Apply.scala	`76.47% <0%> (-2.48%)`	⬇️
core/src/main/scala/cats/ApplicativeError.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/instances/list.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/data/Const.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/data/Validated.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/SemigroupK.scala	`100% <0%> (ø)`	⬆️
core/src/main/scala/cats/instances/vector.scala	`100% <0%> (ø)`	⬆️
.../src/main/scala/cats/syntax/applicativeError.scala	`100% <0%> (ø)`	⬆️
... and 15 more

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 6d191a5...e24a602. Read the comment docs.

travisbrown

Thanks, looks good to me!

johnynek · 2019-09-10T02:52:54Z

@kailuowang any concerns with merging?

kailuowang · 2019-09-10T13:26:11Z

@johnynek sorry it was too last minute to be carried on the 2.0.0 train.
Now 2.0.0 is released this PR as is breaks BC on 2.11.
The plan is to drop Scala 2.11 on master (I'll find time to do that this week if no one beats me to it), after that we can change this PR to simply extend the AllInstances instead of a BinCompat trait.

johnynek · 2019-09-10T17:10:39Z

@kailuowang I'm confused why we can't merge now, and then when you make the change to merge in the trait and drop 2.11 everything is fine?

kailuowang · 2019-09-10T17:17:38Z

@johnynek we can't merge the existing bincompat traits because of BC. If we merge this now then we need to
a) remember to move it to a bincompat trait on the new 2.11 branch or remove it.
and
b) on master branch we can either leave it on the bin compat branch or move it to the AllInstance trait.

It's just easier if we do it the other order. I am already working on dropping 2.11 on master so this shouldn't be held for long.
#3051

LukaJCB

LGTM

…_monad

travisbrown · 2019-11-05T11:48:00Z

Just formatted, which should fix the build.

travisbrown · 2019-11-05T14:18:18Z

Now there's a bincompat error, and it looks legit. I don't know how this ever passed, given that adding a val to a trait should always have been a breaking change?

johnynek · 2019-11-05T16:12:50Z

I guess the idea that we can just add new traits to AllInstances in 2.0.0 is wrong in some way.

travisbrown · 2019-11-05T17:29:25Z

@johnynek We can, just not as vals. I've got a bincompat version here (or if you don't mind I could just push to this branch instead): #3127

johnynek · 2019-11-05T17:38:32Z

I would welcome your push, as my vanity has a slight preference for this PR be the one merged.

johnynek · 2019-11-05T17:38:44Z

Thank you for doing this work.

travisbrown · 2019-11-05T17:44:12Z

@johnynek Pushed, thanks! (They'd be your commits either way. 😄)

johnynek · 2019-11-06T00:42:54Z

👍

Thanks @kailuowang and @travisbrown

…and Defer

…#3332)

johnynek added 2 commits September 8, 2019 10:09

Add scala.util.control.TailCalls.TailRec instances

40c7848

format

eb1271f

johnynek requested a review from travisbrown September 8, 2019 20:18

johnynek commented Sep 8, 2019

View reviewed changes

travisbrown reviewed Sep 8, 2019

View reviewed changes

review comments

3f0177c

travisbrown previously approved these changes Sep 9, 2019

View reviewed changes

LukaJCB previously approved these changes Sep 10, 2019

View reviewed changes

kailuowang added 2 commits October 18, 2019 14:45

Merge remote-tracking branch 'upstream/master' into oscar/add_tailrec…

bc144d9

…_monad

fix conflict

71ac6a4

kailuowang dismissed stale reviews from LukaJCB and travisbrown via 71ac6a4 October 18, 2019 18:51

kailuowang previously approved these changes Oct 18, 2019

View reviewed changes

LukaJCB previously approved these changes Oct 18, 2019

View reviewed changes

Format

cc304ca

travisbrown dismissed stale reviews from LukaJCB and kailuowang via cc304ca November 5, 2019 11:47

travisbrown added this to the 2.1.0-RC1 milestone Nov 5, 2019

travisbrown added 2 commits November 5, 2019 14:11

Merge branch 'master' into oscar/add_tailrec_monad

24f308e

Avoid val in trait for bincompat

e24a602

travisbrown mentioned this pull request Nov 5, 2019

Add instances for the standard library's TailRec #3127

Closed

travisbrown approved these changes Nov 6, 2019

View reviewed changes

travisbrown added the enhancement label Nov 6, 2019

LukaJCB approved these changes Nov 6, 2019

View reviewed changes

LukaJCB merged commit fc7b8b9 into master Nov 6, 2019

larsrh deleted the oscar/add_tailrec_monad branch November 9, 2019 17:26

travisbrown mentioned this pull request Nov 13, 2019

Backport 2.1.0 changes for Scala 2.11 #3143

Closed

22 tasks

gagandeepkalra added a commit to gagandeepkalra/cats that referenced this pull request Feb 29, 2020

backported typelevel#3041, added tailrec instance for StacksafeMonad …

855411a

…and Defer

travisbrown pushed a commit that referenced this pull request Mar 11, 2020

backported #3041, added tailrec instance for StacksafeMonad and Defer (…

8e50b78

…#3332)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add scala.util.control.TailCalls.TailRec instances #3041

Add scala.util.control.TailCalls.TailRec instances #3041

johnynek commented Sep 8, 2019

johnynek Sep 8, 2019

travisbrown left a comment

travisbrown Sep 8, 2019

johnynek Sep 8, 2019

travisbrown Sep 8, 2019

travisbrown Sep 8, 2019

johnynek Sep 8, 2019

travisbrown Sep 9, 2019

kailuowang Sep 9, 2019

codecov-io commented Sep 8, 2019 •

edited

Loading

travisbrown left a comment

johnynek commented Sep 10, 2019

kailuowang commented Sep 10, 2019

johnynek commented Sep 10, 2019

kailuowang commented Sep 10, 2019

LukaJCB left a comment

travisbrown commented Nov 5, 2019

travisbrown commented Nov 5, 2019

johnynek commented Nov 5, 2019

travisbrown commented Nov 5, 2019

johnynek commented Nov 5, 2019

johnynek commented Nov 5, 2019

travisbrown commented Nov 5, 2019

johnynek commented Nov 6, 2019

Add scala.util.control.TailCalls.TailRec instances #3041

Add scala.util.control.TailCalls.TailRec instances #3041

Conversation

johnynek commented Sep 8, 2019

Choose a reason for hiding this comment

travisbrown left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 8, 2019 • edited Loading

Codecov Report

travisbrown left a comment

Choose a reason for hiding this comment

johnynek commented Sep 10, 2019

kailuowang commented Sep 10, 2019

johnynek commented Sep 10, 2019

kailuowang commented Sep 10, 2019

LukaJCB left a comment

Choose a reason for hiding this comment

travisbrown commented Nov 5, 2019

travisbrown commented Nov 5, 2019

johnynek commented Nov 5, 2019

travisbrown commented Nov 5, 2019

johnynek commented Nov 5, 2019

johnynek commented Nov 5, 2019

travisbrown commented Nov 5, 2019

johnynek commented Nov 6, 2019

codecov-io commented Sep 8, 2019 •

edited

Loading