Skip to content

Commit

Permalink
Fix generation of derived value classes wrapping Unit, Null, and Noth…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
hrhino committed Jun 14, 2017
1 parent 21d12e9 commit c9d84a1
Show file tree
Hide file tree
Showing 5 changed files with 52 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -528,8 +528,10 @@ abstract class BCodeBodyBuilder extends BCodeSkelBuilder {
else if (l.isPrimitive) {
bc drop l
if (cast) {
devWarning(s"Tried to emit impossible cast from primitive type $l to $r (at ${app.pos})")
mnode.visitTypeInsn(asm.Opcodes.NEW, jlClassCastExceptionRef.internalName)
bc dup ObjectRef
mnode.visitMethodInsn(asm.Opcodes.INVOKESPECIAL, jlClassCastExceptionRef.internalName, INSTANCE_CONSTRUCTOR_NAME, "()V", true)
emit(asm.Opcodes.ATHROW)
} else {
bc boolconst false
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,17 @@ trait TypeAdaptingTransformer { self: TreeDSL =>
val ldef = deriveLabelDef(tree)(unbox(_, pt))
ldef setType ldef.rhs.tpe
case _ =>
def preservingSideEffects(side: Tree, value: Tree): Tree =
if (treeInfo isExprSafeToInline side) value
else BLOCK(side, value)
val tree1 = pt match {
case ErasedValueType(clazz, BoxedUnitTpe) =>
cast(preservingSideEffects(tree, REF(BoxedUnit_UNIT)), pt)
case ErasedValueType(clazz, underlying) => cast(unboxValueClass(tree, clazz, underlying), pt)
case _ =>
pt.typeSymbol match {
case UnitClass =>
if (treeInfo isExprSafeToInline tree) UNIT
else BLOCK(tree, UNIT)
preservingSideEffects(tree, UNIT)
case x =>
assert(x != ArrayClass)
// don't `setType pt` the Apply tree, as the Apply's fun won't be typechecked if the Apply tree already has a type
Expand Down
30 changes: 23 additions & 7 deletions src/compiler/scala/tools/nsc/typechecker/SyntheticMethods.scala
Original file line number Diff line number Diff line change
Expand Up @@ -159,25 +159,41 @@ trait SyntheticMethods extends ast.TreeDSL {
def thatCast(eqmeth: Symbol): Tree =
gen.mkCast(Ident(eqmeth.firstParam), clazz.tpe)

/* The equality method core for case classes and inline classes.
/* The equality method core for case classes and derived value classes.
* Generally:
* 1+ args:
* (that.isInstanceOf[this.C]) && {
* val x$1 = that.asInstanceOf[this.C]
* (this.arg_1 == x$1.arg_1) && (this.arg_2 == x$1.arg_2) && ... && (x$1 canEqual this)
* }
* Drop canBuildFrom part if class is final and canBuildFrom is synthesized
* Drop:
* - canEqual part if class is final and canEqual is synthesized.
* - test for arg_i if arg_i has type Nothing, Null, or Unit
* - asInstanceOf if no equality checks need made (see scala/bug#9240, scala/bug#10361)
*/
def equalsCore(eqmeth: Symbol, accessors: List[Symbol]) = {
def usefulEquality(acc: Symbol): Boolean = {
val rt = acc.info.resultType
rt != NothingTpe && rt != NullTpe && rt != UnitTpe
}

val otherName = context.unit.freshTermName(clazz.name + "$")
val otherSym = eqmeth.newValue(otherName, eqmeth.pos, SYNTHETIC) setInfo clazz.tpe
val pairwise = accessors map (acc => fn(Select(mkThis, acc), acc.tpe member nme.EQ, Select(Ident(otherSym), acc)))
val pairwise = accessors collect {
case acc if usefulEquality(acc) =>
fn(Select(mkThis, acc), acc.tpe member nme.EQ, Select(Ident(otherSym), acc))
}
val canEq = gen.mkMethodCall(otherSym, nme.canEqual_, Nil, List(mkThis))
val tests = if (clazz.isDerivedValueClass || clazz.isFinal && syntheticCanEqual) pairwise else pairwise :+ canEq

thatTest(eqmeth) AND Block(
ValDef(otherSym, thatCast(eqmeth)),
AND(tests: _*)
)
if (tests.isEmpty) {
thatTest(eqmeth)
} else {
thatTest(eqmeth) AND Block(
ValDef(otherSym, thatCast(eqmeth)),
AND(tests: _*)
)
}
}

/* The equality method for case classes.
Expand Down
1 change: 1 addition & 0 deletions test/files/run/wacky-value-classes.flags
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
-Xverify
20 changes: 20 additions & 0 deletions test/files/run/wacky-value-classes.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// scala/bug#10361
final class AnyValNothing(val self: Nothing) extends AnyVal
final class AnyValNull (val self: Null ) extends AnyVal
// scala/bug#9240
final class AnyValUnit (val self: Unit ) extends AnyVal

object Test extends App {
def avn = new AnyValNull(null)
assert(avn == avn)
/*this throws NPE right now b/c scala/bug#7396 */
//assert(avn.hashCode() == 0)

def avu = new AnyValUnit(())
assert((avu.self: Any).equals(()))
assert(avu equals avu)
assert((avu: Any).## == 0)

/* can't really test AnyValNothing, but summon it so it gets verified */
AnyValNothing.toString
}

0 comments on commit c9d84a1

Please sign in to comment.