Merge pull request #1682 from dotty-staging/vclass

Fix checks related to value classes

Author: odersky

compiler/src/dotty/tools/dotc/core/TypeErasure.scala

@@ -438,7 +438,7 @@ class TypeErasure(isJava: Boolean, semiEraseVCs: Boolean, isConstructor: Boolean
   private def eraseDerivedValueClassRef(tref: TypeRef)(implicit ctx: Context): Type = {
     val cls = tref.symbol.asClass
     val underlying = underlyingOfValueClass(cls)
-    if (underlying.exists) ErasedValueType(tref, valueErasure(underlying))
+    if (underlying.exists && !isCyclic(cls)) ErasedValueType(tref, valueErasure(underlying))
     else NoType
   }
 

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

@@ -135,14 +135,6 @@ class ExtensionMethods extends MiniPhaseTransform with DenotTransformer with Ful
   // TODO: this is state and should be per-run
   // todo: check that when transformation finished map is empty
 
-  private def checkNonCyclic(pos: Position, seen: Set[Symbol], clazz: ClassSymbol)(implicit ctx: Context): Unit =
-    if (seen contains clazz)
-      ctx.error("value class may not unbox to itself", pos)
-    else {
-      val unboxed = underlyingOfValueClass(clazz).typeSymbol
-      if (isDerivedValueClass(unboxed)) checkNonCyclic(pos, seen + clazz, unboxed.asClass)
-    }
-
   override def transformTemplate(tree: tpd.Template)(implicit ctx: Context, info: TransformerInfo): tpd.Tree = {
     if (isDerivedValueClass(ctx.owner)) {
       /* This is currently redundant since value classes may not

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

@@ -135,11 +135,14 @@ class TreeChecker extends Phase with SymTransformer {
     }
   }
 
-  class Checker(phasesToCheck: Seq[Phase]) extends ReTyper {
+  class Checker(phasesToCheck: Seq[Phase]) extends ReTyper with Checking {
 
     val nowDefinedSyms = new mutable.HashSet[Symbol]
     val everDefinedSyms = new mutable.HashMap[Symbol, Tree]
 
+    // don't check value classes after typer, as the constraint about constructors doesn't hold after transform
+    override def checkDerivedValueClass(clazz: Symbol, stats: List[Tree])(implicit ctx: Context) = ()
+
     def withDefinedSym[T](tree: untpd.Tree)(op: => T)(implicit ctx: Context): T = tree match {
       case tree: DefTree =>
         val sym = tree.symbol

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

@@ -53,4 +53,14 @@ object ValueClasses {
   def underlyingOfValueClass(d: ClassDenotation)(implicit ctx: Context): Type =
     valueClassUnbox(d).info.resultType
 
+  /** Whether a value class wraps itself */
+  def isCyclic(cls: ClassSymbol)(implicit ctx: Context): Boolean = {
+    def recur(seen: Set[Symbol], clazz: ClassSymbol)(implicit ctx: Context): Boolean =
+      (seen contains clazz) || {
+        val unboxed = underlyingOfValueClass(clazz).typeSymbol
+        (isDerivedValueClass(unboxed)) && recur(seen + clazz, unboxed.asClass)
+      }
+
+    recur(Set[Symbol](), cls)
+  }
 }

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

@@ -29,6 +29,7 @@ import ErrorReporting.{err, errorType}
 import config.Printers.typr
 import collection.mutable
 import SymDenotations.NoCompleter
+import dotty.tools.dotc.transform.ValueClasses._
 
 object Checking {
   import tpd._
@@ -56,7 +57,7 @@ object Checking {
     checkBounds(args, poly.paramBounds, _.substParams(poly, _))
 
   /** Check applied type trees for well-formedness. This means 
-   *   - all arguments are within their corresponding bounds 
+   *   - all arguments are within their corresponding bounds
    *   - if type is a higher-kinded application with wildcard arguments,
    *     check that it or one of its supertypes can be reduced to a normal application.
    *     Unreducible applications correspond to general existentials, and we
@@ -88,12 +89,12 @@ object Checking {
             checkWildcardHKApply(tp.superType, pos)
         }
       case _ =>
-    }    
+    }
     def checkValidIfHKApply(implicit ctx: Context): Unit =
       checkWildcardHKApply(tycon.tpe.appliedTo(args.map(_.tpe)), tree.pos)
     checkValidIfHKApply(ctx.addMode(Mode.AllowLambdaWildcardApply))
   }
-  
+
   /** Check that `tp` refers to a nonAbstract class
    *  and that the instance conforms to the self type of the created class.
    */
@@ -406,6 +407,43 @@ object Checking {
     notPrivate.errors.foreach { case (msg, pos) => ctx.errorOrMigrationWarning(msg, pos) }
     info
   }
+
+  /** Verify classes extending AnyVal meet the requirements */
+  def checkDerivedValueClass(clazz: Symbol, stats: List[Tree])(implicit ctx: Context) = {
+    def checkValueClassMember(stat: Tree) = stat match {
+      case _: ValDef if !stat.symbol.is(ParamAccessor) =>
+        ctx.error(s"value class may not define non-parameter field", stat.pos)
+      case d: DefDef if d.symbol.isConstructor =>
+        ctx.error(s"value class may not define secondary constructor", stat.pos)
+      case _: MemberDef | _: Import | EmptyTree =>
+      // ok
+      case _ =>
+        ctx.error(s"value class may not contain initialization statements", stat.pos)
+    }
+    if (isDerivedValueClass(clazz)) {
+      if (clazz.is(Trait))
+        ctx.error("Only classes (not traits) are allowed to extend AnyVal", clazz.pos)
+      if (clazz.is(Abstract))
+        ctx.error("`abstract' modifier cannot be used with value classes", clazz.pos)
+      if (!clazz.isStatic)
+        ctx.error(s"value class may not be a ${if (clazz.owner.isTerm) "local class" else "member of another class"}", clazz.pos)
+      if (isCyclic(clazz.asClass))
+        ctx.error("value class cannot wrap itself", clazz.pos)
+      else {
+        val clParamAccessors = clazz.asClass.paramAccessors.filter(_.isTerm)
+        clParamAccessors match {
+          case List(param) =>
+            if (param.is(Mutable))
+              ctx.error("value class parameter must not be a var", param.pos)
+
+          case _ =>
+            ctx.error("value class needs to have exactly one val parameter", clazz.pos)
+        }
+      }
+      stats.foreach(checkValueClassMember)
+    }
+
+  }
 }
 
 trait Checking {
@@ -553,6 +591,10 @@ trait Checking {
       errorTree(tpt, ex"Singleton type ${tpt.tpe} is not allowed $where")
     }
     else tpt
+
+  /** Verify classes extending AnyVal meet the requirements */
+  def checkDerivedValueClass(clazz: Symbol, stats: List[Tree])(implicit ctx: Context) =
+    Checking.checkDerivedValueClass(clazz, stats)
 }
 
 trait NoChecking extends Checking {
@@ -568,4 +610,5 @@ trait NoChecking extends Checking {
   override def checkParentCall(call: Tree, caller: ClassSymbol)(implicit ctx: Context) = ()
   override def checkSimpleKinded(tpt: Tree)(implicit ctx: Context): Tree = tpt
   override def checkNotSingleton(tpt: Tree, where: String)(implicit ctx: Context): Tree = tpt
+  override def checkDerivedValueClass(clazz: Symbol, stats: List[Tree])(implicit ctx: Context) = ()
 }

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

@@ -18,7 +18,6 @@ import config.{ScalaVersion, NoScalaVersion}
 import Decorators._
 import typer.ErrorReporting._
 import DenotTransformers._
-import ValueClasses.isDerivedValueClass
 
 object RefChecks {
   import tpd._
@@ -688,39 +687,6 @@ object RefChecks {
     }
   }
 
-  /** Verify classes extending AnyVal meet the requirements */
-  private def checkDerivedValueClass(clazz: Symbol, stats: List[Tree])(implicit ctx: Context) = {
-    def checkValueClassMember(stat: Tree) = stat match {
-      case _: ValDef if !stat.symbol.is(ParamAccessor) =>
-        ctx.error(s"value class may not define non-parameter field", stat.pos)
-      case _: DefDef if stat.symbol.isConstructor =>
-        ctx.error(s"value class may not define secondary constructor", stat.pos)
-      case _: MemberDef | _: Import | EmptyTree =>
-      // ok
-      case _ =>
-        ctx.error(s"value class may not contain initialization statements", stat.pos)
-    }
-    if (isDerivedValueClass(clazz)) {
-      if (clazz.is(Trait))
-        ctx.error("Only classes (not traits) are allowed to extend AnyVal", clazz.pos)
-      if (clazz.is(Abstract))
-        ctx.error("`abstract' modifier cannot be used with value classes", clazz.pos)
-      if (!clazz.isStatic)
-        ctx.error(s"value class may not be a ${if (clazz.owner.isTerm) "local class" else "member of another class"}", clazz.pos)
-      else {
-        val clParamAccessors = clazz.asClass.paramAccessors.filter(sym => sym.isTerm && !sym.is(Method))
-        clParamAccessors match {
-          case List(param) =>
-            if (param.is(Mutable))
-              ctx.error("value class parameter must not be a var", param.pos)
-          case _ =>
-            ctx.error("value class needs to have exactly one val parameter", clazz.pos)
-        }
-      }
-      stats.foreach(checkValueClassMember)
-    }
-  }
-
   type LevelAndIndex = immutable.Map[Symbol, (LevelInfo, Int)]
 
   class OptLevelInfo extends DotClass {
@@ -836,7 +802,6 @@ class RefChecks extends MiniPhase { thisTransformer =>
       checkParents(cls)
       checkCompanionNameClashes(cls)
       checkAllOverrides(cls)
-      checkDerivedValueClass(cls, tree.body)
       tree
     } catch {
       case ex: MergeError =>

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

@@ -1293,6 +1293,10 @@ class Typer extends Namer with TypeAssigner with Applications with Implicits wit
       ctx.featureWarning(nme.dynamics.toString, "extension of type scala.Dynamic", isScala2Feature = true,
           cls, isRequired, cdef.pos)
     }
+
+    // check value class constraints
+    checkDerivedValueClass(cls, body1)
+
     cdef1
 
     // todo later: check that

tests/neg/i1642.scala

@@ -0,0 +1 @@
+class Test2(val valueVal: Test2) extends AnyVal // error: value class cannot wrap itself

tests/neg/i1670.scala

@@ -0,0 +1 @@
+class A(a:Int, b:Int) extends AnyVal // error: value class needs to have exactly one val parameter

tests/neg/i705-inner-value-class2.scala

@@ -1,5 +1,5 @@
 class Foo {
-  class B(val a: Int) extends AnyVal
+  class B(val a: Int) extends AnyVal // error: value class may not be a member of another class`
 }
 
 object Test {

tests/pickling/extmethods.scala

@@ -3,6 +3,5 @@ package extMethods
 trait That1[A]
 class T[A, This <: That1[A]](val x: Int) extends AnyVal {
   self: This =>
-  var next: This = _
   final def loop(x: This, cnt: Int): Int = loop(x, cnt + 1)
 }

tests/pos/i1642.scala

@@ -0,0 +1,2 @@
+class Test1(val x: Int) extends AnyVal
+class Test2(val y: Test1) extends AnyVal