make all PartialApplied class AnyVal to achieve zero cost #1631

kailuowang · 2017-04-24T20:39:16Z

A trick from scalaz thanks to @edmundnoble from this comment

johnynek · 2017-04-24T20:40:24Z

this is great!

👍

fthomas · 2017-04-24T20:56:13Z

If you define dummy with a default value (val dummy: Boolean = true), you don't need to change the call sites.

kailuowang · 2017-04-24T21:04:48Z

@fthomas thanks. done

ceedubs · 2017-04-24T21:14:48Z

Scala is silly sometimes 😄

I'm in favor of this change, but I think that it would be good to leave @param dummy <explanation> scaladoc comments wherever this appears, because someone encountering this could reasonably be pretty bewildered by the dummy parameter.

I also always forget that you can make classes package-private but still statically return their type from a public method. Should there be any concern about doing this?

codecov-io · 2017-04-24T21:26:44Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #1631      +/-   ##
==========================================
+ Coverage   93.37%   93.37%   +<.01%     
==========================================
  Files         240      240              
  Lines        3941     3942       +1     
  Branches      144      135       -9     
==========================================
+ Hits         3680     3681       +1     
  Misses        261      261

Impacted Files	Coverage Δ
core/src/main/scala/cats/data/EitherT.scala	`96.42% <ø> (ø)`	⬆️
core/src/main/scala/cats/data/Const.scala	`100% <ø> (ø)`	⬆️
free/src/main/scala/cats/free/Free.scala	`88.23% <ø> (ø)`	⬆️
core/src/main/scala/cats/syntax/either.scala	`99.1% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/Validated.scala	`100% <100%> (ø)`	⬆️
core/src/main/scala/cats/data/OptionT.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 58d680f...b0593e4. Read the comment docs.

kailuowang · 2017-04-24T21:30:39Z

@ceedubs I added the docs. As for making them package private, I couldn't think of any downside either.

sellout · 2017-04-25T02:26:53Z

At SlamData, the PartiallyApplied variant we use looks like https://github.com/quasar-analytics/quasar/wiki/Scala%5BQuasar%5D#partially-applied-type-parameters

we scope the PartiallyApplied class inside the object, which means we don’t need unique names and
is there some alternative to (val dummy: Boolean = true) that won’t trigger the DefaultArguments wart? (so we can steal it at SD, too)

I’d also be inclined to use (val dummy: Unit = ()), just to make it clearer that there’s no info in the argument.

Finally, I think there should just be one place where this trick is documented, and maybe link to it from the use sites, because there is far more than dummy that is confusing as hell about this technique (as useful as it is).

sellout · 2017-04-25T02:42:28Z

To answer Q2, I guess I could not use the default argument, and instead use new PartiallyApplied[W](()). That seems fine.

sellout · 2017-04-25T02:56:08Z

And, as you people probably already know, Unit causes the typer to throw a fit, so Boolean it is …

ceedubs · 2017-04-25T11:30:18Z

we scope the PartiallyApplied class inside the object, which means we don’t need unique names

@sellout are you suggesting that we should have something like private[this] final class OfPartiallyApplied or private[Const] final class OfPartiallyApplied instead of the current private[data] final class OfPartiallyApplied? I don't see any reason that we need it scoped to the package as opposed to just the object, so that seems like a decent suggestion to me if that's what you are saying.

I agree that it makes sense to document this pattern somewhere and link to it in the relevant call sites. I'm fine with that either happening in this PR or creating an issue to follow up on it, since I think that this PR still represents progress and these classes weren't documented before anyway.

sellout · 2017-04-25T15:24:26Z

@ceedubs I meant instead of

  private[data] final class OfPartiallyApplied[B](val dummy: Boolean = true ) extends AnyVal {
    def apply[A](a: A): Const[A, B] = Const(a)
  }
  def of[B]: OfPartiallyApplied[B] = new OfPartiallyApplied

having

  object of {
    def apply[B]: PartiallyApplied[B] = new PartiallyApplied
    private[of] final class PartiallyApplied[B](val dummy: Boolean = true) extends AnyVal {
      def apply[A](a: A): Const[A, B] = Const(a)
    }
  }

I put the tight private[of] in there only because of your comment, but it definitely wasn’t what I meant. I meant turning the non-private def into an object with an apply, then moving the private class inside it.

And this isn’t a “my way is better” thing, just a “I’ve been doing it this way, and I have no idea what the tradeoffs are, and I’d generally like to do what Cats/Typelevel thinks is best” (especially if it’s documented somewhere that I am not maintaining 😄).

The downside of my approach, I think, is that the Scaladoc gets even worse. Yours at least looks like a def and has some (the required) type parameters on it. Mine shows up as an object with no type params at all (see transPrepro vs cond). The fact that the non-nested approach is so much clearer there makes me inclined to switch to it.

And yes – these comments are less suggestions for this PR and more that it raised questions about how we do the same thing at SlamData.

So, I think my new approach is basically “do it like in Cats, except for the default argument.” I’ll update my docs, and hopefully it’ll eventually end up in some official Typelevel contributor doc.

kailuowang · 2017-04-25T15:45:49Z

@sellout @ceedubs thanks for all the input. I added an entry to the guideline document and link to it from the use sites. I agree it's quite necessary.

edmundnoble

Nice work. Would be nice if the compiler could just do this (for zero-param value classes)

edmundnoble · 2017-04-26T07:44:18Z

docs/src/main/tut/guidelines.md


 Ops classes should be marked final and extend AnyVal, to take full advantage of inlining and prevent unnecessary allocations.

 The most notable exception is the case where all of the ops in the class are provided by zero-cost macros anyway,
 for example with Simulacrum.

+### <a id="partially-applied-type" href="#partially-applied-type"></a> Partially-Applied Type


Is this perhaps more accurately referred to as "partially applied type params"?

I used the term "Partially-Applied Type" to refer to the intermediate class as a new type with type parameters partially applied. Now that I said it out loud, It's quite a mouthful. I'll change it.

edmundnoble · 2017-04-26T07:44:43Z

docs/src/main/tut/guidelines.md

+### <a id="partially-applied-type" href="#partially-applied-type"></a> Partially-Applied Type
+
+In Scala, when there are multiple type parameters in a function, either scalac infers all type parameters or the user has to
+specify all of them. Often we have functions where there are one or more types that are inferable but not all of them. For example, there is helper function in `OptionT` that creates an `OptionT[A]` from an `A`. It could be written as:


s/OptionT[A]/OptionT[F, A]?

edmundnoble · 2017-04-26T07:45:12Z

docs/src/main/tut/guidelines.md

+pure[List, Int](1)
+```
+
+Note that the type `A` should've been given by the `a: A` argument, but since scalac cannot infer `F[_]`, user still have to specify all type params.


s/user/the user, s/have/has?

kailuowang · 2017-04-28T00:20:45Z

@ceedubs, @fthomas do either of you have time to take another look?

ceedubs

Thanks @kailuowang! This is great stuff. I left a couple of very minor comments, but as soon as they are addressed I think that this is good to go.

ceedubs · 2017-04-28T13:25:00Z

core/src/main/scala/cats/data/Const.scala

@@ -40,7 +40,8 @@ object Const extends ConstInstances {
  def empty[A, B](implicit A: Monoid[A]): Const[A, B] =
    Const(A.empty)

-  final class OfPartiallyApplied[B] {
+/** Uses the [[http://typelevel.org/cats/guidelines.html#partially-applied-type-params Partially Applied Type Params technique]] for ergonomics. */
+  private[data] final class OfPartiallyApplied[B](val dummy: Boolean = true ) extends AnyVal {


Minor, unnecessarily picky comment: the alignment of the scaladoc comment is a little off here.

ceedubs · 2017-04-28T13:27:52Z

free/src/main/scala/cats/free/Free.scala

@@ -221,8 +221,9 @@ object Free {

  /**
   * Pre-application of an injection to a `F[A]` value.
+   * @param dummy is introduced solely for the sake of making this a value class and thus zero allocation cost.


Should this have the same comment with link that the other places have?

kailuowang · 2017-04-29T18:46:30Z

@ceedubs comments addressed.

kailuowang · 2017-05-11T19:36:27Z

ping @ceedubs again

ceedubs · 2017-05-11T20:41:40Z

@kailuowang sorry for the long delay and on such trivial nitpicky comments 😲. This looks great to me! 👍

fthomas · 2017-08-21T23:25:16Z

For posterity: the reason why the dummy parameter is of type Boolean instead of Unit is scala/bug#9240

make all PartialApplied class AnyVal to achieve zero cost

51ee1aa

kailuowang added the in progress label Apr 24, 2017

kailuowang force-pushed the improve-partially branch 3 times, most recently from 46dd6a6 to 3dbb7dc Compare April 24, 2017 21:03

default value for dummy variable

d95ee44

kailuowang force-pushed the improve-partially branch from 3dbb7dc to d95ee44 Compare April 24, 2017 21:04

fthomas approved these changes Apr 24, 2017

View reviewed changes

added doc for dummy param

447b1b3

moved a PartialApplied of either syntax into an object.

9a66697

added an entry in guidelines to explain partially applied type technique

8d7f80e

edmundnoble approved these changes Apr 26, 2017

View reviewed changes

more feedback

59f4802

ceedubs suggested changes Apr 28, 2017

View reviewed changes

doc format correction

b0593e4

kailuowang force-pushed the improve-partially branch from 56fe374 to b0593e4 Compare April 28, 2017 13:47

fthomas approved these changes Apr 29, 2017

View reviewed changes

ceedubs approved these changes May 11, 2017

View reviewed changes

ceedubs merged commit e5ff7d1 into typelevel:master May 11, 2017

ceedubs removed the in progress label May 11, 2017

kailuowang deleted the improve-partially branch May 11, 2017 21:24

kailuowang modified the milestone: 1.0.0-MF May 18, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make all PartialApplied class AnyVal to achieve zero cost #1631

make all PartialApplied class AnyVal to achieve zero cost #1631

kailuowang commented Apr 24, 2017

johnynek commented Apr 24, 2017

fthomas commented Apr 24, 2017

kailuowang commented Apr 24, 2017

ceedubs commented Apr 24, 2017

codecov-io commented Apr 24, 2017 •

edited

Loading

kailuowang commented Apr 24, 2017

sellout commented Apr 25, 2017

sellout commented Apr 25, 2017

sellout commented Apr 25, 2017

ceedubs commented Apr 25, 2017

sellout commented Apr 25, 2017

kailuowang commented Apr 25, 2017

edmundnoble left a comment •

edited

Loading

edmundnoble Apr 26, 2017

kailuowang Apr 26, 2017

edmundnoble Apr 26, 2017

edmundnoble Apr 26, 2017

kailuowang commented Apr 28, 2017

ceedubs left a comment

ceedubs Apr 28, 2017

ceedubs Apr 28, 2017

kailuowang commented Apr 29, 2017

kailuowang commented May 11, 2017

ceedubs commented May 11, 2017

fthomas commented Aug 21, 2017

make all PartialApplied class AnyVal to achieve zero cost #1631

make all PartialApplied class AnyVal to achieve zero cost #1631

Conversation

kailuowang commented Apr 24, 2017

johnynek commented Apr 24, 2017

fthomas commented Apr 24, 2017

kailuowang commented Apr 24, 2017

ceedubs commented Apr 24, 2017

codecov-io commented Apr 24, 2017 • edited Loading

Codecov Report

kailuowang commented Apr 24, 2017

sellout commented Apr 25, 2017

sellout commented Apr 25, 2017

sellout commented Apr 25, 2017

ceedubs commented Apr 25, 2017

sellout commented Apr 25, 2017

kailuowang commented Apr 25, 2017

edmundnoble left a comment • edited Loading

Choose a reason for hiding this comment

edmundnoble Apr 26, 2017

Choose a reason for hiding this comment

kailuowang Apr 26, 2017

Choose a reason for hiding this comment

edmundnoble Apr 26, 2017

Choose a reason for hiding this comment

edmundnoble Apr 26, 2017

Choose a reason for hiding this comment

kailuowang commented Apr 28, 2017

ceedubs left a comment

Choose a reason for hiding this comment

ceedubs Apr 28, 2017

Choose a reason for hiding this comment

ceedubs Apr 28, 2017

Choose a reason for hiding this comment

kailuowang commented Apr 29, 2017

kailuowang commented May 11, 2017

ceedubs commented May 11, 2017

fthomas commented Aug 21, 2017

codecov-io commented Apr 24, 2017 •

edited

Loading

edmundnoble left a comment •

edited

Loading