Fix generation of derived value classes wrapping Unit, Null, and Nothing #5938

hrhino · 2017-06-11T23:12:22Z

All sorts o' specialness going on here. Unit morphs into a BoxedUnit when it's in a field, but void when it's the return type of a method, which is expected. This means, though, that the unboxing method of a Unit-wrapping value class has the signature ()V, not ()Lscala/runtime/BoxedUnit, so attempting to use its value in the equals method spits out some wonderful invalid bytecode, instead. Similar sadness occurs for Nothing and Null as well.

The "solution" is to not even bother to check for equality, as we've only got at most one legitimate value of each of these types. Because the code is shared with case classes, this also changes the bytecode we generate for them. Obviously this is an "unrelated change" as far as the bugs this is meant to fix go, but it's innocuous enough as far as I can tell.

I also slipped a constructor call into the generated ClassCastException that gets thrown when we are asked to emit a cast for a primitive type in BCodeBodyBuilder, so we generate valid bytecode in that case. I've got no idea how NEW; DUP; ATHROW is possibly ever legitimate, but hey.

Fixes scala/bug#9240
Fixes scala/bug#10361

hrhino · 2017-06-11T23:13:16Z

src/compiler/scala/tools/nsc/typechecker/SyntheticMethods.scala

     */
    def equalsCore(eqmeth: Symbol, accessors: List[Symbol]) = {
+      def usefulEquality(acc: Symbol): Boolean =
+        acc.info.resultType != NothingTpe && acc.info.resultType != NullTpe && acc.info.resultType != UnitTpe


I would think that these three types would have an alias somewhere, but I couldn't find it. Feel free to make me swap it out for something else.

The three comparisons look OK to me, but you could extract acc.info.resultType into a local.

dwijnand · 2017-06-12T10:21:14Z

test/files/run/wacky-value-classes.scala

+// scala/bug#9240
+final class AnyValNothing(val self: Nothing) extends AnyVal
+final class AnyValNull   (val self: Null   ) extends AnyVal
+// scala/bug#10361


Your references are inverted. The below is scala/bug#9240, and the ones above are scala/bug#10361.

Woops. Good catch, thanks!

dwijnand · 2017-06-12T10:22:50Z

test/files/run/wacky-value-classes.scala

+  def avn = new AnyValNull(null)
+  assert(avn == avn)
+  /*this throws NPE right now b/c null.hashCode() does...*/
+  //assert(avn.hashCode() == 0)


Switch this (and avu below) to .##. If this here returns 0 rather than throw an NPE then you also fixed scala/bug#7396 with this patch!

Sadly, that problem is deeper than this, as far as I can tell. No hat tricks here 😞

lrytz · 2017-06-12T13:14:01Z

src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala

-                emit(asm.Opcodes.ATHROW)
+                if (l == UNIT && r == srBoxedUnitRef) {
+                  /* special case: we can "cast" V to Lscala/runtime/BoxedUnit; by summoning the latter */
+                  mnode.instructions.add(backendUtils.getBoxedUnit)


I think this should be handled in erasure, at the place where the cast is inserted, see where cast is invoked.

👍 It would also fix the problem for the three backends at the same time :)

How rude of me, I almost forgot that non-JVM backends exist... Changed!

lrytz · 2017-06-12T13:15:05Z

src/compiler/scala/tools/nsc/typechecker/SyntheticMethods.scala

     */
    def equalsCore(eqmeth: Symbol, accessors: List[Symbol]) = {
+      def usefulEquality(acc: Symbol): Boolean =
+        acc.info.resultType != NothingTpe && acc.info.resultType != NullTpe && acc.info.resultType != UnitTpe


The three comparisons look OK to me, but you could extract acc.info.resultType into a local.

lrytz

Very nice work, thanks a lot!

lrytz · 2017-06-13T08:50:07Z

src/compiler/scala/tools/nsc/backend/jvm/BCodeBodyBuilder.scala

@@ -530,6 +530,7 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
              if (cast) {
                mnode.visitTypeInsn(asm.Opcodes.NEW, jlClassCastExceptionRef.internalName)
                bc dup ObjectRef
+                mnode.visitMethodInsn(asm.Opcodes.INVOKESPECIAL, jlClassCastExceptionRef.internalName, INSTANCE_CONSTRUCTOR_NAME, "()V", true)


This certainly looks right, but I wonder how we can end up here. We'd need an x.asInstanceOf[T] where x has primitive type. Any idea? When I write 1.asInstanceOf[String], the value is boxed.

FWIW, Scala.js has corresponding code in its codegen, but I'm not sure it is ever exercised ^^

I can't figure out how to get this branch to get hit... we should have already inserted the boxing by now. This somewhat implies to me that this could, possibly should, be an assertion or similar; before this patch, this would always generate bytecode that doesn't pass verification.

I agree with your analysis.

When in doubt about old code like this, we tend to add a devWarning (which issues a warning under -Ydebug or -Xdev) rather than a hard assert to fail gracefully

Devs are duly warned. Good idea!

lrytz · 2017-06-13T08:53:27Z

TODO: run jardiff -- no diff in library+reflect+compiler

…ing. All sorts o' specialness going on here. Unit morphs into a BoxedUnit when it's in a field, but void when it's the return type of a method, which is expected. This means, though, that the unboxing method of a Unit-wrapping value class has the signature `()V`, not `()Lscala/runtime/BoxedUnit`, so attempting to use its value in the equals method spits out some wonderful invalid bytecode, instead. Similar sadness occurs for Nothing and Null as well. The "solution" is to not even bother to check for equality, as we've only got at most one legitimate value of each of these types. Because the code is shared with `case class`es, this also changes the bytecode we generate for them. Obviously this is an "unrelated change" as far as the bugs this is meant to fix go, but it's innocuous enough as far as I can tell. I also slipped a constructor call into the generated `ClassCastException` that gets thrown when we are asked to emit a cast for a primitive type in `BCodeBodyBuilder`, so we generate valid bytecode if we ever wind in that branch. Discussion on scala#5938 implies that this branch shouldn't ever be reached, so add a devWarning now that it doesn't cause an obvious error. Fixes scala/bug#9240 Fixes scala/bug#10361

scala-jenkins added this to the 2.12.3 milestone Jun 11, 2017

hrhino commented Jun 11, 2017

View reviewed changes

dwijnand reviewed Jun 12, 2017

View reviewed changes

lrytz requested changes Jun 12, 2017

View reviewed changes

hrhino force-pushed the bugfix/wacky-value-classes branch from b4061c2 to 251c1fd Compare June 13, 2017 01:57

lrytz approved these changes Jun 13, 2017

View reviewed changes

lrytz added the welcome hello new contributor! label Jun 13, 2017

hrhino force-pushed the bugfix/wacky-value-classes branch from 251c1fd to c9d84a1 Compare June 14, 2017 12:22

lrytz removed the welcome hello new contributor! label Jun 15, 2017

lrytz merged commit 34da49b into scala:2.12.x Jun 19, 2017

xuwei-k referenced this pull request in scalaz/scalaz Jun 20, 2017

add EitherT.rightU, leftU

a0711f0

hrhino mentioned this pull request Oct 26, 2017

AnyVal case class with Nothing member crashes compiler scala/bug#10570

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix generation of derived value classes wrapping Unit, Null, and Nothing #5938

Fix generation of derived value classes wrapping Unit, Null, and Nothing #5938

hrhino commented Jun 11, 2017

hrhino Jun 11, 2017

lrytz Jun 12, 2017

dwijnand Jun 12, 2017

hrhino Jun 13, 2017

dwijnand Jun 12, 2017

hrhino Jun 13, 2017

lrytz Jun 12, 2017

sjrd Jun 12, 2017

hrhino Jun 13, 2017

lrytz Jun 12, 2017

lrytz left a comment

lrytz Jun 13, 2017

sjrd Jun 13, 2017

hrhino Jun 13, 2017

retronym Jun 14, 2017

hrhino Jun 14, 2017

lrytz commented Jun 13, 2017 •

edited

Fix generation of derived value classes wrapping Unit, Null, and Nothing #5938

Fix generation of derived value classes wrapping Unit, Null, and Nothing #5938

Conversation

hrhino commented Jun 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrytz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lrytz commented Jun 13, 2017 • edited

lrytz commented Jun 13, 2017 •

edited