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

Adds AnyTap and AnyPipe #6767

Closed
wants to merge 4 commits into from

Conversation

@eed3si9n
Copy link
Member

commented Jun 11, 2018

Fixes scala/bug#5324

This implements an opt-in enrichment for any type called tap and pipe:

scala> import scala.util.chainingOps._
import scala.util.chainingOps._

scala> val xs = List(1, 2, 3).tap(ys => println("debug " + ys.toString))
debug List(1, 2, 3)
xs: List[Int] = List(1, 2, 3)

scala> val times6 = (_: Int) * 6
times6: Int => Int = $$Lambda$1727/1479800269@10fbbdb

scala> (1 + 2 + 3).pipe(times6)
res0: Int = 36

scala> (1 - 2 - 3).pipe(times6).pipe(scala.math.abs)
res1: Int = 24
Adds AnyTap and AnyPipe
Fixes scala/bug#5324

This implements two implicit classes `AnyTap` and `AnyPipe`, which enrich every type `A` to inject `tap` and `pipe` method respectively.

```scala
scala> val xs = List(1, 2, 3).tap(ys => println("debug " + ys.toString))
debug List(1, 2, 3)
xs: List[Int] = List(1, 2, 3)

scala> val s = List(1, 2, 3).pipe(xs => xs.mkString(","))
s: String = 1,2,3
```
* }}}
*
* @group implicit-classes-any */
implicit final class AnyTap[A](private val self: A) extends AnyVal {

This comment has been minimized.

Copy link
@Ichoran

Ichoran Jun 11, 2018

Contributor

Why have two separate classes for these?

Also, any chance of turning these into a macro? That would be a huge value-add because you wouldn't need to create the closure etc. in order to use this workflow.

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n Jun 11, 2018

Author Member
  1. I wasn't sure if someone wants to unimport either. I'm happy to bundle them into one class like AnyOps.
  2. Besides things like String_+ do we have precedence of using macros in standard library? I'm not sure if Scala team wants to add a macro responsibility at this juncture.

This comment has been minimized.

Copy link
@hrhino

hrhino Jun 11, 2018

Member

String_+ isn't a macro; it's a synthetic method that the compiler injects into String.

That said, there are quite a few macros in stdlib, such as the f"" string interpolator, reify, and quasiquotes. Their implementations are linked up in FastTrack, which is presumably how you'd want to link these in.

def testAnyTap: Unit = {
var x: Int = 0
val result = List(1, 2, 3)
.tap(xs => x = xs(0))

This comment has been minimized.

Copy link
@Ichoran

Ichoran Jun 11, 2018

Contributor

Not a big deal, but isn't it a little non-idiomatic to index into a list instead of taking the head or something?

@sjrd

This comment has been minimized.

Copy link
Member

commented Jun 11, 2018

What's the rationale for this to be in Predef? It could and should be explicitly imported.

There should be much stronger resistance against putting stuff in Predef, including pimps to Any.

@dwijnand

This comment has been minimized.

Copy link
Member

commented Jun 11, 2018

LGTM! Having these in the standard library, even not in Predef, is great.

Unofficially .

@eed3si9n

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2018

@sjrd

What's the rationale for this to be in Predef? It could and should be explicitly imported.

See scala/bug#5324 for the rationale. Whether this change should or should not be part of Scala Library or Predef should be discussed there, I think.

@lrytz

This comment has been minimized.

Copy link
Member

commented Jun 11, 2018

The number of tests that need an update (even if it's a minor one) is a hint that adding extensions to Any has far-reaching effects. So I agree this should require an import.

Other than that, +1, it's handy.

* @tparam U the result type of the function `f`.
* @return the original value `self`.
*/
def tap[U](f: A => U): A = { f(self); self }

This comment has been minimized.

Copy link
@lrytz

lrytz Jun 11, 2018

Member
  • Could the return type be self.type?
  • Why the U type parameter, not just A => Unit (or self.type => Unit)?

This comment has been minimized.

Copy link
@dwijnand

dwijnand Jun 11, 2018

Member

U avoids value discarding warnings.

This comment has been minimized.

Copy link
@lrytz

lrytz Jun 11, 2018

Member

Is that desirable? The value (if any) is discarded.

This comment has been minimized.

Copy link
@dwijnand

dwijnand Jun 11, 2018

Member

IMO yes, as the function is specified as a side-effect.

This comment has been minimized.

Copy link
@martijnhoekstra

martijnhoekstra Jun 11, 2018

Contributor

Is there an advantage over having this polymorphic version in U over A => Any?

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n Jun 11, 2018

Author Member

Hot take. Martin likes genericity. The parametric function A => U is more beautiful (to me) than A => Any though the purists would probably sneer at tap as being "lawless."

This comment has been minimized.

Copy link
@martijnhoekstra

martijnhoekstra Jun 12, 2018

Contributor

A => U implies - to me - a more parametric, prettier relation than A => Any implies - but the less beautiful thing is actually what you're getting, so I'd argue that should be reflected in the signature.

I don't know who the purists are, why they would consider the tap method lawless, or what lawfulness has to do with anything here in the first place.

I'm also not sure what hot takes means here, and at this point I'm too afraid to ask.

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n Jun 12, 2018

Author Member

Hot take

In journalism, a hot take is a "piece of deliberately provocative commentary that is based almost entirely on shallow moralizing" in response to a news story, "usually written on tight deadlines with little research or reporting, and even less thought".

sounds about right.

By laws, I meant something like Monoid law that can limit the properties of the types. Sorry if I came off as cryptic.

This comment has been minimized.

Copy link
@martijnhoekstra

martijnhoekstra Jun 12, 2018

Contributor

Right. But tap is lawful (for all types A, for all x: A, for all f: A => Any, x.tap(f) == x). And I still don't know what lawfulness has to do with the entire thing, and I have the feeling it's a red herring and doesn't have to do with anything.

But maybe we can ask the purists who would sneer. Who are they, so we can ask?

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n Jun 12, 2018

Author Member

I have the feeling it's a red herring and doesn't have to do with anything.

You could be right, but tap is also related to K-combinator A => B => A, as seen in Kestrels. K can be seen as Const, which does form a Functor. In all these cases, they probably can be replaced withAny (after all it's ignored), but I think [U] helps in thinking of tap that it has an explicit phantom type.

@martin-g

This comment has been minimized.

Copy link

commented Jun 11, 2018

IMO the example of pipe should be a bit more complex than the current one.
Looking at val s = List(1, 2, 3).pipe(xs => xs.mkString(",")) my first question was Why do I need pipe() at all? I can just call List(...).mkString(",")

@hrhino
Copy link
Member

left a comment

👍 to the macros.

I'm not opposed to these in the standard library, but I agree with @sjrd that they probably don't belong in Predef. I'd rather see ensuring/requiring wind up as explicit imports than be used as justification to adding more extension methods on Any.

* }}}
*
* @group implicit-classes-any */
implicit final class AnyTap[A](private val self: A) extends AnyVal {

This comment has been minimized.

Copy link
@hrhino

hrhino Jun 11, 2018

Member

String_+ isn't a macro; it's a synthetic method that the compiler injects into String.

That said, there are quite a few macros in stdlib, such as the f"" string interpolator, reify, and quasiquotes. Their implementations are linked up in FastTrack, which is presumably how you'd want to link these in.

@som-snytt

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2018

I worked on -Yimports yesterday which I hope will make it easy to add a predef with extensions such as this. I'm also considering -Yadd-import:eed3s1gn or however you spell it, eed3si9n, so you don't have to -Yimports:java.lang,scala,scala.Predef,eed3si9n. I see, it uses 3 and 9 like magic numbers.

@dwijnand

This comment has been minimized.

Copy link
Member

commented Jun 11, 2018

IMO the example of pipe should be a bit more complex than the current one.
Looking at val s = List(1, 2, 3).pipe(xs => xs.mkString(",")) my first question was Why do I need pipe() at all? I can just call List(...).mkString(",")

Yeah, better not use a method already on the type. I suggest as an alternative:

val s = List(1, 2, 3).pipe(xs => scala.util.Random.shuffle(xs))
@SethTisue

This comment has been minimized.

Copy link
Member

commented Jun 11, 2018

val s = List(1, 2, 3).pipe(xs => scala.util.Random.shuffle(xs))

val s = List(1, 2, 3).pipe(scala.util.Random.shuffle)

?

@dwijnand

This comment has been minimized.

Copy link
Member

commented Jun 11, 2018

Sure. I was just conservatively being verbose 🙂

@eed3si9n

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2018

@dwijnand The problem with scala.util.Random.shuffle is that I can't write a JUnit test against it.

@Ichoran

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2018

List(1, 2, 3).pipe(xs => xs zip xs.reverse)

@eed3si9n

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2018

scala> val times6 = (_: Int) * 6
times6: Int => Int = $$Lambda$1727/1479800269@10fbbdb

scala> (1 + 2 + 3).pipe(times6)
res0: Int = 36

scala> (1 - 2 - 3).pipe(times6).pipe(scala.math.abs)
res1: Int = 24
@OlegYch

This comment has been minimized.

Copy link

commented Jun 11, 2018

figuring out where to import it from or how to modify predef is as much work as writing it locally, i think it should be in predef

@som-snytt

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2018

@OlegYch for something bundled with scala-library, I think -Yadd-import:io.eed3si9n isn't too bad. The spelling could be made simpler.

I think newbies could -Yadd-import:scala.concurrent.ExecutionContext.Implicits for easy access to the global context. Well, now that I've typed it in, it's not trivial, and I can already imagine the SO questions.

@Ichoran

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2018

I'm rather torn about whether this is a good idea. If they were macros, awesome, great, that's considerable work to create, and therefore is a big value-add (as it is then a zero-cost abstraction, and also enables high-performance strategies like early return in case of error).

But colliding with the names of existing non-macro pipe/tap with a Predef import seems bad. And having an import statement that is nearly as long as the (trivial) definitions of the chaining extension methods is also, well, not all that helpful. For instance, I can't envision why I'd ever want to use these instead of the ones I've already got in my own library.

It looks like as I was writing this you were making them macros, so it's all awesome! 🥇

@eed3si9n

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2018

@Ichoran yea. I wasn't confident about making them macros, so I made the import changes first :)

f(self)
self
}
def tap[U](f: A => U): A = macro ???

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n Jun 12, 2018

Author Member

I am not sure how to return self.type here using macro. If I just change the type to self.type I get:

util-chainingops.scala:12: error: type mismatch;
 found   : x$macro$1.type (with underlying type List[Int])
 required: _1.self.type where val _1: scala.util.ChainingOps[List[Int]]
        .tap(xs => x = xs.head)
            ^
one error found

When the macro is expanded ChainingOps[List[Int]] goes away, so.. can I still get _1.self.type?

This comment has been minimized.

Copy link
@dwijnand

dwijnand Jun 12, 2018

Member

Yeah can't use self.type when using an enriched method.

You could do it if you did it as a synthetic method, like String.+


/** Adds chaining methods `tap` and `pipe` to every type.
*/
final class ChainingOps[A](val self: A) extends AnyVal {

This comment has been minimized.

Copy link
@dwijnand

dwijnand Jun 12, 2018

Member

private val self. Otherwise every type gets a self member!

This comment has been minimized.

Copy link
@eed3si9n

eed3si9n Jun 12, 2018

Author Member

Yes. I had to open it up for self.type. Another reason not do it I guess.

@dwijnand

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

Implemented as a macro I'm guessing this would probably benefit from having a few tests for the different code styles. I'm not sure which are overkill, but I'm thinking of variations like infix x pipe f, eta-expansions x pipe (f _), braces styles x.pipe { n => f(n) }...

@allanrenucci

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2018

This shouldn't be implemented as a macro to be usable with Dotty

@sjrd

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

This shouldn't be implemented as a macro to be usable with Dotty

Not to mention it's super easy for an optimizer to inline and get rid of the overhead anyway. Don't use macros to improve performance when a simple inliner can do it.

@eed3si9n

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2018

@allanrenucci @sjrd ok. I am indifferent about they being macros. You and @Ichoran @hrhino should hash it out.

@hrhino

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

nah, I'm not going to hash it out with anyone. I'm okay with it being a normal method, too.

@som-snytt

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2018

AnyTap and AnyPipe sounds like my new favorite pub.

@hrhino

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

@allanrenucci out of curiosity, does f"" just not work on Dotty, then?

@allanrenucci

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2018

@allanrenucci out of curiosity, does f"" just not work on Dotty, then?

No, it doesn't

Starting dotty REPL...
scala> f"Hello" 
java.lang.NoSuchMethodError: scala.StringContext.f(Lscala/collection/Seq;)Ljava/lang/String;
@dwijnand

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

See also the "Intrinsfy raw and s String interpolators" patch (makes the compiler replace those calls with an equivalent, but more efficient, implementations).

#6093

@Ichoran

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2018

@sjrd - Can you please fix the optimizer to do this with high reliability, then?

Right now, it falls completely flat.

With tests of

   def zlen(xs: Array[Int]): Int = {
      var i = 0
      while (i < xs.length) {
        if (xs(i) == 0) return i
        i += 1
      }
      i
    }

    def zlenTap(xs: Array[Int]): Int = {
      var i = 0
      while (i < xs.length) {
        xs(i).tap(x => if (x == 0) return i)
        i += 1
      }
      i
    }

    def zsum(xs: Array[Int]) = {
      var s, i = 0
      while (i < xs.length) {
        val x = xs(i)
        s += x*x + x + 2
        i += 1
      }
      i
    }

    def zsumTap(xs: Array[Int]) = {
      var s, i = 0
      while (i < xs.length) {
        s += xs(i).pipe(x => x*x + x + 2)
        i += 1
      }
      i
    }

I get results like

Benchmark comparison (in 543.7 ms): len tap
Significantly different (p ~= 0)
  Time ratio:    12.88999   95% CI 12.60819 - 13.17180   (n=20)
    First     1.591 us   95% CI 1.569 us - 1.613 us
    Second    20.51 us   95% CI 20.16 us - 20.86 us

Benchmark comparison (in 746.3 ms): sum pipe
Significantly different (p ~= 0)
  Time ratio:    31.11847   95% CI 30.86872 - 31.36822   (n=20)
    First     1.011 us   95% CI 1.005 us - 1.016 us
    Second    31.45 us   95% CI 31.27 us - 31.63 us

Gist: https://gist.github.com/Ichoran/4ba6460aadd93d7bd109bb712f75007f

Having a secret performance gotcha feature, where you use pipe instead of match and go 30x slower isn't something I think we should give to people by default.

@OlegYch

This comment has been minimized.

Copy link

commented Jun 12, 2018

@Ichoran "a secret gotcha feature" here is the lambda, and they are already given by default

@Ichoran

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2018

@OlegYch - It isn't a gotcha if the macro inlines the expression, eliminating the lambda.

@sjrd

This comment has been minimized.

Copy link
Member

commented Jun 12, 2018

By that argument, we should turn List.map and many many other functions from the standard library into macros.

Your test with tap has a non-local return. That has terrible consequences. Did you did on purpose to make a point?

The test with pipe should really be just as fast as the hand-written one, though. If it isn't, I blame the optimizer.

Can you please fix the optimizer to do this with high reliability, then?

No, I cannot, because I'm not familiar enough with the scalac/JVM optimizer to a) make even the start of a guess about what's going wrong and b) make the required changes in a reasonable time frame. I can tell you that it is straightforward for the Scala.js optimizer, though. It generates the same code up to renaming and 1 more temporary variable for the pipe example:

$c_Lbench_Bench.prototype.zsum$1__p1__AI__I = (function(xs) {
  var s = 0;
  var i = 0;
  while ((i < xs.u.length)) {
    var x = xs.get(i);
    s = ((s + ((2 + (($imul(x, x) + x) | 0)) | 0)) | 0);
    i = ((1 + i) | 0)
  };
  return i
});
$c_Lbench_Bench.prototype.zsumPipe$1__p1__AI__I = (function(xs) {
  var s = 0;
  var i = 0;
  while ((i < xs.u.length)) {
    var jsx$1 = s;
    var a = xs.get(i);
    s = ((jsx$1 + ((2 + (($imul(a, a) + a) | 0)) | 0)) | 0);
    i = ((1 + i) | 0)
  };
  return i
});

It doesn't even need the explicit @inlines because it has the right heuristics for this.

@Ichoran

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2018

@sjrd - It sounds to me like the your original characterization of fixing the optimizer was misleading, then. Maybe you meant, "If the optimizer can handle this case robustly, then all such code is sped up, which is much better than having to write special macros; thus, it is worth the time investment." To which I would respond: "Does anyone who can have time to do it now? If not, what is the harm in adding a macro that can be reverted to a plain extension method once the optimizers all consistently work on it?"

Also, my use of the "non-local" return was of course to illustrate a point, which is that conceptually there's no reason that the return should be non-local. That it is is an unfortunate implementation detail, and manual splicing of the tree in a macro prevents it from being non-local. If it is in a match it is also local.

Finally, List isn't a very good example because it doesn't have the same kind of usage as pipe. Conceptually, all of the following are equivalent:

{ val temp = f(); g(temp) }
g(f())
f() match { case x => g(x) }
f().pipe(g)

With data structures like List, the computational complexity of various operations is always a consideration. But with pipe the only difference is syntactic. The data structures are seemingly identical; one is just highly inefficient if implemented in the most natural way.

I'd love for the optimizer to robustly handle this. But I don't think introducing a feature where the lore is "Don't use that feature--it might kill your performance" is a wise move. (Also: the lore used to be "don't use the optimizer, it does nothing"; this isn't true any more, but I'm not sure we can count on everyone turning it on on the JVM after so many years of it not helping.)

@dwijnand

This comment has been minimized.

Copy link
Member

commented Jul 2, 2018

It would be lovely if the strong-minded participants here negotiated with one another and found path forward so that this doesn't miss the release.... 🚢

@Ichoran

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2018

Well, I think it's better to not have it at all than to have a version without macros. Anyone can trivially add these things, and possibly do so in a better way (e.g. with specialization) than the non-macro version. The macro version is not trivial, and is a substantial value-add.

As an alternative, we could tweak things to allow calling match with method syntax. Then we wouldn't need pipe at all, and tap wouldn't be that hard to do either (i.e. {case x => foo(); x }).

@shado23

This comment has been minimized.

Copy link

commented Jul 6, 2018

For pipe it would be very nice to have the |> operator from F# as an alias. For symmetry it might make sense to also add <|, which is basically the reverse.

@som-snytt

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2018

I've often wanted a "newbie" mode; maybe also a "deluxe" mode with various conveniences which may not be efficient, but are nice to have for REPL, scripting or for composing things run just once in main. Maybe that's the definition of a "unified scripting experience" that would also replace App.

@eed3si9n

This comment has been minimized.

Copy link
Member Author

commented Jul 6, 2018

@shado23

For pipe it would be very nice to have the |> operator from F# as an alias. For symmetry it might make sense to also add <|, which is basically the reverse.

I disagree.

Scala has a long and bloody history of library/tooling authors introducing symbolic operators, the Scala Days talks mercilessly ridiculing the unreadability of the operators, and maintainers disappearing in the dark or someone else quickly covering them up. See <<=, and its unfortunate cousins <+= and <++=, dubbed fishbone operators etc. <<= almost looks cute, like a mirror image of Haskell's bind >>=, and I am sure it made some sense at the time of introduction.

This doesn't mean that symbolic operators are always bad, but I'd really caution against anything that doesn't have clear meaning understood among the target user audience. I see pipe to be either a total version or a pattern match of mapping over Id datatype. In both cases Scala uses English words match and map to accept a case-function literal or a function.

@OlegYch

This comment has been minimized.

Copy link

commented Jul 6, 2018

with this being hidden under import, i don't see much use
but just curious, why macros and not inline ?

@Ichoran

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2018

@OlegYch - Inline would be fine if the optimizer actually removed all the overhead, but it doesn't, at least not when I tested it.

@SethTisue

This comment has been minimized.

Copy link
Member

commented Aug 7, 2018

my take on where we stand, here:

  • including this in Predef isn't happening. it should require importing
  • sorry, no symbolic aliases either
  • the macro version of this won't fly

otoh, these methods have been requested a million billion trillion times over the last 10+ years in every Scala venue under the sun. I don't think everyone rolling their own slightly different variant is a good solution

and I don't think having a little bit of performance overhead, which will likely go away in some future Scala, is a show-stopper. if that overhead's a back-breaker for somebody, they can just not use it.

so, I would support merging a non-macro-based version of this.

closing, but I invite @eed3si9n to submit a new, non-macro-based version.

@eed3si9n

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2018

closing, but I invite @eed3si9n to submit a new, non-macro-based version.

Here you go #7007

@dwijnand dwijnand referenced this pull request Aug 10, 2018

@eed3si9n eed3si9n deleted the eed3si9n:wip/tap branch Oct 9, 2018

@eed3si9n eed3si9n restored the eed3si9n:wip/tap branch Oct 10, 2018

@eed3si9n eed3si9n deleted the eed3si9n:wip/tap branch Jan 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.