Skip to content

Specialised Option.tapEach to preserve the type #80

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

Merged
merged 8 commits into from
Jul 14, 2021

Conversation

gdiet
Copy link
Contributor

@gdiet gdiet commented Apr 13, 2021

Add specialised version of tapEach to scala.Option to preserve the type. This PR supersedes #79.

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

@gdiet gdiet mentioned this pull request Apr 13, 2021
@NthPortal
Copy link
Contributor

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

@gdiet
Copy link
Contributor Author

gdiet commented Apr 21, 2021

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.

OndrejSpanel
OndrejSpanel previously approved these changes Apr 21, 2021
@NthPortal
Copy link
Contributor

NthPortal commented Apr 22, 2021

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)
}

Co-authored-by: Julien Richard-Foy <julien@richard-foy.fr>
Copy link
Contributor

@julienrf julienrf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @gdiet!

@julienrf julienrf merged commit 17cea9d into scala:main Jul 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants