Merge pull request #4490 from dotty-staging/fix-vc-equals

Fix `==` for value classes

Author: allanrenucci

compiler/src/dotty/tools/dotc/transform/AccessProxies.scala

@@ -47,7 +47,8 @@ abstract class AccessProxies {
           if (passReceiverAsArg(accessor.name))
             (argss.head.head.select(accessed), tps.takeRight(numTypeParams), argss.tail)
           else
-            (ref(TermRef(cls.thisType, accessed)), tps, argss)
+            (if (accessed.isStatic) ref(accessed) else ref(TermRef(cls.thisType, accessed)),
+             tps, argss)
         val rhs =
           if (accessor.name.isSetterName &&
               forwardedArgss.nonEmpty && forwardedArgss.head.nonEmpty) // defensive conditions
@@ -70,6 +71,11 @@ abstract class AccessProxies {
     def accessorNameKind: ClassifiedNameKind
     def needsAccessor(sym: Symbol)(implicit ctx: Context): Boolean
 
+    def ifNoHost(reference: RefTree)(implicit ctx: Context): Tree = {
+      assert(false, "no host found for $reference with ${reference.symbol.showLocated} from ${ctx.owner}")
+      reference
+    }
+
     /** A fresh accessor symbol */
     private def newAccessorSymbol(owner: Symbol, name: TermName, info: Type, pos: Position)(implicit ctx: Context): TermSymbol = {
       val sym = ctx.newSymbol(owner, name, Synthetic | Method, info, coord = pos).entered
@@ -125,18 +131,14 @@ abstract class AccessProxies {
     def useAccessor(reference: RefTree)(implicit ctx: Context): Tree = {
       val accessed = reference.symbol.asTerm
       var accessorClass = hostForAccessorOf(accessed: Symbol)
-      if (!accessorClass.exists) {
-        val curCls = ctx.owner.enclosingClass
-        transforms.println(i"${curCls.ownersIterator.toList}%, %")
-        ctx.error(i"illegal access to protected ${accessed.showLocated} from $curCls",
-          reference.pos)
-        accessorClass = curCls
+      if (accessorClass.exists) {
+        val accessorName = accessorNameKind(accessed.name)
+        val accessorInfo =
+          accessed.info.ensureMethodic.asSeenFrom(accessorClass.thisType, accessed.owner)
+        val accessor = accessorSymbol(accessorClass, accessorName, accessorInfo, accessed)
+        rewire(reference, accessor)
       }
-      val accessorName = accessorNameKind(accessed.name)
-      val accessorInfo =
-        accessed.info.ensureMethodic.asSeenFrom(accessorClass.thisType, accessed.owner)
-      val accessor = accessorSymbol(accessorClass, accessorName, accessorInfo, accessed)
-      rewire(reference, accessor)
+      else ifNoHost(reference)
     }
 
     /** Replace tree with a reference to an accessor if needed */
@@ -156,6 +158,12 @@ object AccessProxies {
   /** Where an accessor for the `accessed` symbol should be placed.
    *  This is the closest enclosing class that has `accessed` as a member.
    */
-  def hostForAccessorOf(accessed: Symbol)(implicit ctx: Context): Symbol =
-    ctx.owner.ownersIterator.findSymbol(_.derivesFrom(accessed.owner))
+  def hostForAccessorOf(accessed: Symbol)(implicit ctx: Context): Symbol = {
+    def recur(cls: Symbol): Symbol =
+      if (!cls.exists) NoSymbol
+      else if (cls.derivesFrom(accessed.owner)) cls
+      else if (cls.companionModule.moduleClass == accessed.owner) accessed.owner
+      else recur(cls.owner)
+    recur(ctx.owner)
+  }
 }

compiler/src/dotty/tools/dotc/transform/ProtectedAccessors.scala

@@ -8,6 +8,7 @@ import core.Flags._
 import core.Decorators._
 import MegaPhase.MiniPhase
 import ast.Trees._
+import config.Printers.transforms
 
 /** Add accessors for all protected accesses. An accessor is needed if
  *  according to the rules of the JVM a protected class member is not accesissible
@@ -56,6 +57,14 @@ class ProtectedAccessors extends MiniPhase {
     val insert = new Insert {
       def accessorNameKind = ProtectedAccessorName
       def needsAccessor(sym: Symbol)(implicit ctx: Context) = ProtectedAccessors.needsAccessor(sym)
+
+      override def ifNoHost(reference: RefTree)(implicit ctx: Context): Tree = {
+        val curCls = ctx.owner.enclosingClass
+        transforms.println(i"${curCls.ownersIterator.toList}%, %")
+        ctx.error(i"illegal access to protected ${reference.symbol.showLocated} from $curCls",
+          reference.pos)
+        reference
+      }
     }
   }
 

compiler/src/dotty/tools/dotc/transform/VCElideAllocations.scala

@@ -12,7 +12,7 @@ import ExtensionMethods._, TreeExtractors._, ValueClasses._
  *  For a value class V defined as:
  *    class V(val underlying: U) extends AnyVal
  *  we avoid unnecessary allocations:
- *     new V(u1) == new V(u2) => u1 == u2
+ *     new V(u1) == new V(u2) => u1 == u2   provided V does not redefine `equals`
  *    (new V(u)).underlying() => u
  */
 class VCElideAllocations extends MiniPhase with IdentityDenotTransformer {
@@ -27,7 +27,9 @@ class VCElideAllocations extends MiniPhase with IdentityDenotTransformer {
       // new V(u1) == new V(u2) => u1 == u2
       // (We don't handle != because it has been eliminated by InterceptedMethods)
       case BinaryOp(NewWithArgs(tp1, List(u1)), op, NewWithArgs(tp2, List(u2)))
-      if (tp1 eq tp2) && (op eq defn.Any_==) && isDerivedValueClass(tp1.typeSymbol) =>
+      if (tp1 eq tp2) && (op eq defn.Any_==) &&
+         isDerivedValueClass(tp1.typeSymbol) &&
+         !defn.Any_equals.overridingSymbol(tp1.typeSymbol.asClass).exists =>
         // == is overloaded in primitive classes
         u1.equal(u2)
 

compiler/src/dotty/tools/dotc/typer/Inliner.scala

@@ -81,11 +81,11 @@ object Inliner {
             ctx.error("Implementation restriction: cannot use private constructors in inline methods", tree.pos)
             tree // TODO: create a proper accessor for the private constructor
           }
-          else if (AccessProxies.hostForAccessorOf(tree.symbol).exists) useAccessor(tree)
-          else tree
+          else useAccessor(tree)
         case _ =>
           tree
       }
+      override def ifNoHost(reference: RefTree)(implicit ctx: Context): Tree = reference
     }
 
     /** Fallback approach if the direct approach does not work: Place the accessor method
@@ -124,7 +124,7 @@ object Inliner {
         if needsAccessor(tree.symbol) && tree.isTerm && !tree.symbol.isConstructor =>
           val (refPart, targs, argss) = decomposeCall(tree)
           val qual = qualifier(refPart)
-          inlining.println(i"adding receiver passing inline accessor for $tree -> (${qual.tpe}, $refPart: ${refPart.getClass}, [$targs%, %], ($argss%, %))")
+          println(i"adding receiver passing inline accessor for $tree/$refPart -> (${qual.tpe}, $refPart: ${refPart.getClass}, [$targs%, %], ($argss%, %))")
 
           // Need to dealias in order to cagtch all possible references to abstracted over types in
           // substitutions
@@ -142,7 +142,7 @@ object Inliner {
           def addQualType(tp: Type): Type = tp match {
             case tp: PolyType => tp.derivedLambdaType(tp.paramNames, tp.paramInfos, addQualType(tp.resultType))
             case tp: ExprType => addQualType(tp.resultType)
-            case tp => MethodType(qualType :: Nil, tp)
+            case tp => MethodType(qualType.simplified :: Nil, tp)
           }
 
           // Abstract accessed type over local refs

compiler/src/dotty/tools/dotc/typer/VarianceChecker.scala

@@ -6,6 +6,7 @@ import core._
 import Types._, Contexts._, Flags._, Symbols._, Annotations._, Trees._, NameOps._
 import Decorators._
 import Variances._
+import NameKinds._
 import util.Positions._
 import rewrite.Rewrites.patch
 import config.Printers.variances
@@ -134,6 +135,7 @@ class VarianceChecker()(implicit ctx: Context) {
       def skip =
         !sym.exists ||
         sym.is(PrivateLocal) ||
+        sym.name.is(InlineAccessorName) || // TODO: should we exclude all synthetic members?
         sym.is(TypeParam) && sym.owner.isClass // already taken care of in primary constructor of class
       tree match {
         case defn: MemberDef if skip =>