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

Generate unapply for newtype case class #18

Closed
oleg-py opened this issue Apr 17, 2018 · 10 comments
Closed

Generate unapply for newtype case class #18

oleg-py opened this issue Apr 17, 2018 · 10 comments

Comments

@oleg-py
Copy link

oleg-py commented Apr 17, 2018

I'm starting to use more and more of this library. In particular, newtypes are quite useful for defining custom typeclass instances:

@newtype case class FirstOf[A](value: A)
object FirstOf {
   implicit def semigroup[A]: Semigroup[A] = Semigroup.instance((a, _) => a)
}

It would be really nice if for these cases unapply would also be generated. This comes in handy in chains involving foldMap, e.g.

list
  .foldMap(person => Map(person.name -> FirstOf(person.age))
  .map { case (name, FirstOf(age)) => ??? } // no need to use `.value` manually
@carymrobbins
Copy link
Member

@oleg-py

In particular, newtypes are quite useful for defining custom typeclass instances

Absolutely, that is really one of the killer features of newtype, IMHO.

It would be really nice if for these cases unapply would also be generated.

I intentionally omitted this due to the Option wrapping/unwrapping that occurs, and I'm skeptical that it could be JIT'd away (would love to see that it could be though).

Checking this in an ad-hoc way, consider the following -

@newtype case class Foo(x: Int)
object Foo {
  def unapply(foo: Foo): Option[Int] = Some(foo.x)
}

case class Bar(x: Int)

class Test {
  def testCaseClass = Bar(1) match { case Bar(x) => x }
  def testNewType = Foo(1) match { case Foo(x) => x }
}

Let's look at the generated bytecode.

testCaseClass

  public int testCaseClass();
    descriptor: ()I
    flags: ACC_PUBLIC
    Code:
      stack=3, locals=4, args_size=1
         0: new           #18                 // class $line10/$read$$iw$$iw$$iw$$iw$Bar
         3: dup
         4: iconst_1
         5: invokespecial #49                 // Method $line10/$read$$iw$$iw$$iw$$iw$Bar."<init>":(I)V
         8: astore_2
         9: aload_2
        10: ifnull        23
        13: aload_2
        14: invokevirtual #52                 // Method $line10/$read$$iw$$iw$$iw$$iw$Bar.x:()I
        17: istore_3
        18: iload_3
        19: istore_1
        20: goto          35
        23: goto          26
        26: new           #54                 // class scala/MatchError
        29: dup
        30: aload_2
        31: invokespecial #57                 // Method scala/MatchError."<init>":(Ljava/lang/Object;)V
        34: athrow
        35: iload_1
        36: ireturn

testNewType

  public int testNewType();
    descriptor: ()I
    flags: ACC_PUBLIC
    Code:
      stack=3, locals=7, args_size=1
         0: getstatic     #65                 // Field $line9/$read$$iw$$iw$$iw$$iw$Foo$.MODULE$:L$line9/$read$$iw$$iw$$iw$$iw$Foo$;
         3: iconst_1
         4: invokevirtual #69                 // Method $line9/$read$$iw$$iw$$iw$$iw$Foo$.apply:(I)Ljava/lang/Object;
         7: astore_2
         8: getstatic     #74                 // Field scala/reflect/ClassTag$.MODULE$:Lscala/reflect/ClassTag$;
        11: getstatic     #65                 // Field $line9/$read$$iw$$iw$$iw$$iw$Foo$.MODULE$:L$line9/$read$$iw$$iw$$iw$$iw$Foo$;
        14: invokevirtual #78                 // Method $line9/$read$$iw$$iw$$iw$$iw$Foo$.Foo$classTag:()Lscala/reflect/ClassTag;
        17: invokeinterface #84,  1           // InterfaceMethod scala/reflect/ClassTag.runtimeClass:()Ljava/lang/Class;
        22: invokevirtual #87                 // Method scala/reflect/ClassTag$.apply:(Ljava/lang/Class;)Lscala/reflect/ClassTag;
        25: aload_2
        26: invokeinterface #91,  2           // InterfaceMethod scala/reflect/ClassTag.unapply:(Ljava/lang/Object;)Lscala/Option;
        31: astore_3
        32: aload_3
        33: invokevirtual #97                 // Method scala/Option.isEmpty:()Z
        36: ifne          82
        39: aload_3
        40: invokevirtual #101                // Method scala/Option.get:()Ljava/lang/Object;
        43: astore        4
        45: getstatic     #65                 // Field $line9/$read$$iw$$iw$$iw$$iw$Foo$.MODULE$:L$line9/$read$$iw$$iw$$iw$$iw$Foo$;
        48: aload         4
        50: invokevirtual #102                // Method $line9/$read$$iw$$iw$$iw$$iw$Foo$.unapply:(Ljava/lang/Object;)Lscala/Option;
        53: astore        5
        55: aload         5
        57: invokevirtual #97                 // Method scala/Option.isEmpty:()Z
        60: ifne          79
        63: aload         5
        65: invokevirtual #101                // Method scala/Option.get:()Ljava/lang/Object;
        68: invokestatic  #108                // Method scala/runtime/BoxesRunTime.unboxToInt:(Ljava/lang/Object;)I
        71: istore        6
        73: iload         6
        75: istore_1
        76: goto          94
        79: goto          85
        82: goto          85
        85: new           #54                 // class scala/MatchError
        88: dup
        89: aload_2
        90: invokespecial #57                 // Method scala/MatchError."<init>":(Ljava/lang/Object;)V
        93: athrow
        94: iload_1
        95: ireturn

It seems scalac is already able to do some optimizations with the case class version, and the newtype version is having to do Option checks. There might be a better way to write the unapply method; however, I'm currently unaware of it.

Again, this might be optimizable by the JIT, and in practice it probably won't affect performance much, but I'd like to stay true to the "zero-cost" advertisement as much as possible, only giving in obvious ways.

@oleg-py
Copy link
Author

oleg-py commented Apr 17, 2018

@carymrobbins it seems that it's not JITed away (or I'm bad at JMH-ing), causing about 5x slowdown. Still would very much like the option of having it instead of copy-pasting unapply for my own convenience.

I also assumed Scalac did not optimize unapply of case classes, incurring boxing penalty too. It's nice to see it doesn't.

@carymrobbins
Copy link
Member

@oleg-py I need to add a benchmarks module to this library, mind sharing your JMH code? May provide a way for us to experiment with implementations to prove out zero (or near-zero) cost to get something like what you're looking for.

@joroKr21
Copy link
Contributor

Also note that unapply will match any instance of the underlying type (unlike value classes).

@carymrobbins
Copy link
Member

@joroKr21 Good point. However, as part of #10 I've been experimenting with different ClassTag encodings and have considered trying to just use the underlying type's ClassTag. If that happens, it may actually forbid matching invalid types (whereas now it will cause a runtime exception not caught by the compiler).

scala> :paste
@newtype case class Foo(x: Int)
object Foo {
  def unapply(foo: Foo): Option[Int] = Some(foo.x)
}

// Exiting paste mode, now interpreting.

defined type alias Foo
defined trait Foo$Types
defined object Foo

scala> ("foo": Any) match { case Foo(x) => x }
java.lang.ClassCastException: java.lang.String cannot be cast to java.lang.Integer
  at scala.runtime.BoxesRunTime.unboxToInt(BoxesRunTime.java:101)
  at Foo$Ops$newtype$.x$extension(<pastie>:14)
  at Foo$.unapply(<pastie>:16)
  ... 38 elided

@oleg-py
Copy link
Author

oleg-py commented Apr 17, 2018

@carymrobbins I put the project here
https://github.com/oleg-py/newtype-unapply

Note the changes:

  • Int replaced with String to exclude boxing overhead
  • Generated ops class seems to crash JMH with malformed class name, so I had to make a field private[this] and implement unapply in terms of asInstanceOf

Also these things did not seem to improve anything:

  • Using Some[String] in signature
  • Using a shared global / thread-local mutable variable with var get and val isEmpty = true to save on allocations

BTW, get and isEmpty are required to be instance methods, extensions would simply not work.

@joroKr21 it's a compile error to try to match a Int against a pattern expecting Foo when Foo is a newtype. It's only an issue if you try to match on e.g. Any, which is the opposite of what I do :)

@carymrobbins
Copy link
Member

@oleg-py Brings up a good point that it is indeed only an issue if you try to match on Any, which is Scalac may just handle as a special case, not enforcing exhaustiveness. Consider what happens if we match on a String type instead -

scala> "foo" match { case Foo(x) => x }
<console>:16: error: scrutinee is incompatible with pattern type;
 found   : Foo
    (which expands to)  Foo.Type
 required: String
       "foo" match { case Foo(x) => x }
                             ^

@carymrobbins
Copy link
Member

carymrobbins commented Apr 20, 2018

@oleg-py Experimental idea which improves the performance of unapply -

oleg-py/newtype-unapply@master...carymrobbins:unapply-class

The newtype unapply bytecode posted earlier shows ClassTag being used, which I suspected was slowing things down. So I tried returning Some as you did as well as returning my own Unapply value class. Both performed very close to the case class version.

Here are the benchmarks via sbt clean 'jmh:run TestBenchmark' -

Benchmark                     Mode  Cnt          Score         Error  Unit
testCaseClass                thrpt  200  402211203.871 ±  861246.654  ops/s
testManualSimpleUnapply      thrpt  200  393148121.470 ± 4173139.812  ops/s
testManualUnapplyValueClass  thrpt  200  384478663.022 ±  870967.771  ops/s
testNewTypeSimpleUnapply     thrpt  200  110200664.917 ±  283007.861  ops/s

the testManual benchmarks are for handwritten newtypes, whereas the testNewType one is the macro generated one which contains ClassTag instances.

This may convince me to try to get rid of the ClassTag instances. For one, I dislike that they enable type-based pattern matching on newtypes. And now this. The only use case that I know of for them is to support Array construction, but I think there's probably a better way to pull that off. For instance, we could have a NewTypeArray type class to support casting to and from newtypes as well as constructing arrays for newtypes. I'm thinking this is probably a better solution that the hacks we're using with ClassTag to work around the flaky Scala Array machinery.

@joroKr21 Feel free to chime in on this as I know you originally introduced the ClassTag instances.

carymrobbins added a commit that referenced this issue Apr 20, 2018
carymrobbins added a commit that referenced this issue Apr 20, 2018
carymrobbins added a commit that referenced this issue Apr 21, 2018
carymrobbins added a commit that referenced this issue Apr 21, 2018
carymrobbins added a commit that referenced this issue Apr 21, 2018
@carymrobbins
Copy link
Member

I'm including unapply support in the v0.4.0 release. You can get it by passing unapply = true to the @new(sub)type annotation. Note that when matching on Any compiler warnings will be emitted, so I think that should be enough to clue in users that they're being unsafe. Note that before #25 this was not the case, so having unapply for newtypes would have been slightly unsafe.

newtype

scala> @newtype(unapply = true) case class Foo(x: String)

scala> Foo("bar") match { case Foo(x) => x }
res0: String = bar

scala> 1 match { case Foo(x) => x }
<console>:16: error: scrutinee is incompatible with pattern type;
 found   : Foo
    (which expands to)  Foo.Type
 required: Int
       1 match { case Foo(x) => x }
                         ^

scala> "foo" match { case Foo(x) => x }
<console>:16: error: scrutinee is incompatible with pattern type;
 found   : Foo
    (which expands to)  Foo.Type
 required: String
       "foo" match { case Foo(x) => x }
                             ^



scala> (1: Any) match { case Foo(x) => x ; case _ => "nope" }
<console>:19: warning: abstract type pattern Foo.Type (the underlying of Foo) is unchecked since it is eliminated by erasure
       (1: Any) match { case Foo(x) => x }
                                ^
<console>:19: warning: The outer reference in this type test cannot be checked at run time.
       (1: Any) match { case Foo(x) => x }
                                ^
error: No warnings can be incurred under -Xfatal-warnings.

newsubtype

scala> @newsubtype(unapply = true) class Bar(x: String)

scala> "foo".coerce[Bar] match { case Bar(x) => x }
res13: String = foo

scala> "foo" match { case Bar(x) => x }
<console>:16: error: scrutinee is incompatible with pattern type;
 found   : Bar
    (which expands to)  Bar.Type
 required: String
       "foo" match { case Bar(x) => x }
                             ^

scala> 1 match { case Bar(x) => x }
<console>:19: error: scrutinee is incompatible with pattern type;
 found   : Bar
    (which expands to)  Bar.Type
 required: Int
       1 match { case Bar(x) => x }
                         ^

scala> (1: Any) match { case Bar(x) => x }
<console>:19: warning: abstract type pattern Bar.Type (the underlying of Bar) is unchecked since it is eliminated by erasure
       (1: Any) match { case Bar(x) => x }
                                ^
<console>:19: warning: The outer reference in this type test cannot be checked at run time.
       (1: Any) match { case Bar(x) => x }
                                ^
error: No warnings can be incurred under -Xfatal-warnings.

@joroKr21
Copy link
Contributor

Yes, I introduced ClassTag only to support Arrays. I would be perfectly happy with removing it if we find a replacement for that feature. Maybe even an Array constructor generated by the macro would be enough.

As for unapply I missed the point of pattern matching if the scrutinee is not a supertype. But I guess it's fine if people find it convenient.

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

No branches or pull requests

3 participants