Closed oleg-py closed 6 years ago
@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.
@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.
@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.
Also note that unapply
will match any instance of the underlying type (unlike value classes).
@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
@carymrobbins I put the project here https://github.com/oleg-py/newtype-unapply
Note the changes:
private[this]
and implement unapply in terms of asInstanceOf
Also these things did not seem to improve anything:
Some[String]
in signaturevar get
and val isEmpty = true
to save on allocationsBTW, 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 :)
@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 }
^
@oleg-py Experimental idea which improves the performance of unapply -
https://github.com/oleg-py/newtype-unapply/compare/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.
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.
Yes, I introduced ClassTag
only to support Array
s. 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.
I'm starting to use more and more of this library. In particular, newtypes are quite useful for defining custom typeclass instances:
It would be really nice if for these cases
unapply
would also be generated. This comes in handy in chains involving foldMap, e.g.