scala / scala-library-next

backwards-binary-compatible Scala standard library additions
Apache License 2.0
69 stars 17 forks source link

Specialised Option.tapEach to preserve the type #80

Closed gdiet closed 3 years ago

gdiet commented 3 years ago

Add specialised version of tapEach to scala.Option to preserve the type. This PR supersedes https://github.com/scala/scala-library-next/pull/79.

See also https://stackoverflow.com/questions/67017901/why-does-scala-option-tapeach-return-iterable-not-option?noredirect=1#comment118462412_67017901.

NthPortal commented 3 years ago

have you tested that calling tapEach on Option actually invokes this extension method, and not the conversion to Iterable?

gdiet commented 3 years ago

Yes, to the check that the extension method is indeed called, I created the test file TestOptionOpsExtensions.scala. It compiles, and it wouldn't without the extension method.

NthPortal commented 3 years ago

that test only checks that it works with a type ascription, which is not generally used as it requires breaking up the chain of method calls (and the main point of tapEach is to not have to do that).

to my surprise however, importing scala.collection.next._ does in fact make it call the new extension method without a type ascription.

scala> def foo: Unit = { Some(1).tapEach(println); () }
def foo: Unit

scala> :javap -c -p -filter foo
[...]
  public void foo();
    Code:
       0: getstatic     #28                 // Field scala/collection/next/package$OptionOpsExtensions$.MODULE$:Lscala/collection/next/package$OptionOpsExtensions$;
       3: getstatic     #33                 // Field scala/collection/next/package$.MODULE$:Lscala/collection/next/package$;
       6: new           #35                 // class scala/Some
       9: dup
      10: iconst_1
      11: invokestatic  #41                 // Method scala/runtime/BoxesRunTime.boxToInteger:(I)Ljava/lang/Integer;
      14: invokespecial #45                 // Method scala/Some."<init>":(Ljava/lang/Object;)V
      17: invokevirtual #49                 // Method scala/collection/next/package$.OptionOpsExtensions:(Lscala/Option;)Lscala/Option;
      20: invokedynamic #67,  0             // InvokeDynamic #0:apply:()Lscala/Function1;
      25: invokevirtual #71                 // Method scala/collection/next/package$OptionOpsExtensions$.tapEach$extension:(Lscala/Option;Lscala/Function1;)Lscala/Option;
      28: pop
      29: return

I might tweak the test so that the compiler wouldn't be able to coerce a specific implicit to be used; for example:

def testThing(): Option[Int] = {
  val opt = Some(1).tapEach(_ => ())
  opt.map(identity)
}