Merge pull request #8819 from dotty-staging/fix-#8799-v2

Fix #8799: Make sure signatures are computed before erasure

Author: odersky

compiler/src/dotty/tools/dotc/Compiler.scala

@@ -61,8 +61,7 @@ class Compiler {
          new CheckStatic,            // Check restrictions that apply to @static members
          new BetaReduce,             // Reduce closure applications
          new init.Checker) ::        // Check initialization of objects
-    List(new CompleteJavaEnums,      // Fill in constructors for Java enums
-         new ElimRepeated,           // Rewrite vararg parameters and arguments
+    List(new ElimRepeated,           // Rewrite vararg parameters and arguments
          new ExpandSAMs,             // Expand single abstract method closures to anonymous classes
          new ProtectedAccessors,     // Add accessors for protected members
          new ExtensionMethods,       // Expand methods of value classes with extension methods
@@ -101,6 +100,7 @@ class Compiler {
          new ArrayApply,             // Optimize `scala.Array.apply([....])` and `scala.Array.apply(..., [....])` into `[...]`
          new ElimPolyFunction,       // Rewrite PolyFunction subclasses to FunctionN subclasses
          new TailRec,                // Rewrite tail recursion to loops
+         new CompleteJavaEnums,      // Fill in constructors for Java enums
          new Mixin,                  // Expand trait fields and trait initializers
          new LazyVals,               // Expand lazy vals
          new Memoize,                // Add private fields to getters and setters

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

@@ -141,7 +141,7 @@ object Signature {
   export MatchDegree._
 
   /** The signature of everything that's not a method, i.e. that has
-   *  a type different from PolyType, MethodType, or ExprType.
+   *  a type different from PolyType or MethodType.
    */
   val NotAMethod: Signature = Signature(List(), EmptyTypeName)
 

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

@@ -1896,30 +1896,39 @@ object Types {
       case sym: Symbol => sym.originDenotation.name
     }
 
-    /** The signature of the last known denotation, or if there is none, the
-     *  signature of the symbol
+    /** The signature computed from the last known denotation with `sigFromDenot`,
+     *  or if there is none, the signature of the symbol. Signatures are always
+     *  computed before erasure, since some symbols change their signature at erasure.
      */
-    protected def computeSignature(implicit ctx: Context): Signature = {
+    protected def computeSignature(implicit ctx: Context): Signature =
       val lastd = lastDenotation
-      if (lastd != null) lastd.signature
+      if lastd != null then sigFromDenot(lastd)
+      else if ctx.erasedTypes then computeSignature(using ctx.withPhase(ctx.erasurePhase))
       else symbol.asSeenFrom(prefix).signature
-    }
 
-    /** The signature of the current denotation if it is known without forcing.
+    /** The signature computed from the current denotation with `sigFromDenot` if it is
+     *  known without forcing.
      *  Otherwise the signature of the current symbol if it is known without forcing.
-     *  Otherwise NotAMethod.
+     *  Otherwise NotAMethod. Signatures are always computed before erasure, since
+     *  some symbols change their signature at erasure.
      */
     private def currentSignature(implicit ctx: Context): Signature =
-      if (ctx.runId == mySignatureRunId) mySignature
-      else {
+      if ctx.runId == mySignatureRunId then mySignature
+      else
         val lastd = lastDenotation
-        if (lastd != null) lastd.signature
-        else {
+        if lastd != null then sigFromDenot(lastd)
+        else if ctx.erasedTypes then currentSignature(using ctx.withPhase(ctx.erasurePhase))
+        else
           val sym = currentSymbol
-          if (sym.exists) sym.asSeenFrom(prefix).signature
+          if sym.exists then sym.asSeenFrom(prefix).signature
           else Signature.NotAMethod
-        }
-      }
+
+    /** The signature of a pre-erasure version of denotation `lastd`. */
+    private def sigFromDenot(lastd: Denotation)(using Context) =
+      if lastd.validFor.firstPhaseId <= ctx.erasurePhase.id then lastd.signature
+      else lastd match
+        case lastd: SingleDenotation => lastd.initial.signature
+        case _ => Signature.OverloadedSignature
 
     final def symbol(implicit ctx: Context): Symbol =
       // We can rely on checkedPeriod (unlike in the definition of `denot` below)
@@ -3066,7 +3075,7 @@ object Types {
     }
   }
 
-  trait MethodicType extends SignatureCachingType {
+  trait MethodicType extends TermType {
     protected def resultSignature(implicit ctx: Context): Signature = try resultType match {
       case rtp: MethodicType => rtp.signature
       case tp =>
@@ -3086,7 +3095,7 @@ object Types {
     override def resultType(implicit ctx: Context): Type = resType
     override def underlying(implicit ctx: Context): Type = resType
 
-    def computeSignature(implicit ctx: Context): Signature = resultSignature
+    override def signature(implicit ctx: Context): Signature = Signature.NotAMethod
 
     def derivedExprType(resType: Type)(implicit ctx: Context): ExprType =
       if (resType eq this.resType) this else ExprType(resType)
@@ -3192,7 +3201,7 @@ object Types {
     final override def equals(that: Any): Boolean = equals(that, null)
   }
 
-  abstract class MethodOrPoly extends UncachedGroundType with LambdaType with MethodicType {
+  abstract class MethodOrPoly extends UncachedGroundType with LambdaType with MethodicType with SignatureCachingType {
     final override def hashCode: Int = System.identityHashCode(this)
 
     final override def equals(that: Any): Boolean = equals(that, null)

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

@@ -77,38 +77,14 @@ class CompleteJavaEnums extends MiniPhase with InfoTransformer { thisPhase =>
       case p => p
     }
 
-  /** 1. If this is a constructor of a enum class that extends, add $name and $ordinal parameters to it.
-   *
-   *  2. If this is a $new method that creates simple cases, pass $name and $ordinal parameters
-   *     to the enum superclass. The $new method looks like this:
-   *
-   *       def $new(..., ordinal: Int, name: String) = {
-   *         class $anon extends E(...) { ... }
-   *         new $anon
-   *       }
-   *
-   *     After the transform it is expanded to
-   *
-   *       def $new(..., ordinal: Int, name: String) = {
-   *         class $anon extends E(..., name, ordinal) { ... }
-   *         new $anon
-   *       }
-   */
+  /** If this is a constructor of a enum class that extends, add $name and $ordinal parameters to it. */
   override def transformDefDef(tree: DefDef)(implicit ctx: Context): DefDef = {
     val sym = tree.symbol
     if (sym.isConstructor && sym.owner.derivesFromJavaEnum)
       val tree1 = cpy.DefDef(tree)(
         vparamss = tree.vparamss.init :+ (tree.vparamss.last ++ addedParams(sym, Param)))
       sym.setParamssFromDefs(tree1.tparams, tree1.vparamss)
       tree1
-    else if (sym.name == nme.DOLLAR_NEW && sym.owner.linkedClass.derivesFromJavaEnum) {
-      val Block((tdef @ TypeDef(tpnme.ANON_CLASS, templ: Template)) :: Nil, call) = tree.rhs
-      val args = tree.vparamss.last.takeRight(2).map(param => ref(param.symbol)).reverse
-      val templ1 = cpy.Template(templ)(
-        parents = addEnumConstrArgs(sym.owner.linkedClass, templ.parents, args))
-      cpy.DefDef(tree)(
-        rhs = cpy.Block(tree.rhs)(cpy.TypeDef(tdef)(tdef.name, templ1) :: Nil, call))
-    }
     else tree
   }
 
@@ -159,7 +135,8 @@ class CompleteJavaEnums extends MiniPhase with InfoTransformer { thisPhase =>
         parents = addEnumConstrArgs(defn.JavaEnumClass, templ.parents, addedSyms.map(ref)),
         body = params ++ addedDefs ++ addedForwarders ++ rest)
     }
-    else if (cls.isAnonymousClass && cls.owner.isAllOf(EnumCase) && cls.owner.owner.linkedClass.derivesFromJavaEnum) {
+    else if (cls.isAnonymousClass && ((cls.owner.name eq nme.DOLLAR_NEW) || cls.owner.isAllOf(EnumCase)) &&
+             cls.owner.owner.linkedClass.derivesFromJavaEnum) {
       def rhsOf(name: TermName) =
         templ.body.collect {
           case mdef: DefDef if mdef.name == name => mdef.rhs

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

@@ -112,7 +112,10 @@ class Mixin extends MiniPhase with SymTransformer { thisPhase =>
   override def relaxedTypingInGroup: Boolean = true
     // Because it changes number of parameters in trait initializers
 
-  override def runsAfter: Set[String] = Set(Erasure.name)
+  override def runsAfter: Set[String] = Set(
+    Erasure.name,
+    CompleteJavaEnums.name // This phase changes constructor parameters which Mixin translates into super-calls
+  )
 
   override def changesMembers: Boolean = true  // the phase adds implementions of mixin accessors
 

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

@@ -87,6 +87,19 @@ class TreeChecker extends Phase with SymTransformer {
 
     checkCompanion(symd)
 
+    // Signatures are used to disambiguate overloads and need to stay stable
+    // until erasure, see the comment above `Compiler#phases`.
+    if (ctx.phaseId <= ctx.erasurePhase.id) {
+      val cur = symd.info
+      val initial = symd.initial.info
+      assert(cur.signature == initial.signature,
+        i"""Signature of $symd changed at phase ${ctx.phase}
+           |Initial info: ${initial}
+           |Initial sig : ${initial.signature}
+           |Current info: ${cur}
+           |Current sig : ${cur.signature}""")
+    }
+
     symd
   }
 

compiler/test/dotty/tools/SignatureTest.scala

@@ -0,0 +1,38 @@
+package dotty.tools
+
+import vulpix.TestConfiguration
+
+import org.junit.Test
+
+import dotc.ast.Trees._
+import dotc.core.Decorators._
+import dotc.core.Contexts._
+import dotc.core.Types._
+
+import java.io.File
+import java.nio.file._
+
+class SignatureTest:
+  @Test def signatureCaching: Unit =
+    inCompilerContext(TestConfiguration.basicClasspath, separateRun = true, "case class Foo(value: Unit)") {
+      val (ref, refSig) = ctx.atPhase(ctx.erasurePhase.next) {
+        val cls = ctx.requiredClass("Foo")
+        val ref = cls.requiredMethod("value").termRef
+        (ref, ref.signature)
+      }
+      ctx.atPhase(ctx.typerPhase) {
+        // NamedType#signature is always computed before erasure, which ensures
+        // that it stays stable and therefore can be cached as long as
+        // signatures are guaranteed to be stable before erasure, see the
+        // comment above `Compiler#phases`.
+        assert(refSig == ref.signature,
+          s"""The signature of a type should never change but the signature of $ref was:
+             |${ref.signature} at typer, whereas it was:
+             |${refSig} after erasure""".stripMargin)
+        assert(ref.signature == ref.denot.signature,
+          s"""Before erasure, the signature of a TypeRef should be the signature of its denotation,
+             |but the cached signature of $ref was:
+             |${ref.signature}, whereas its denotation signature at typer was:
+             |${ref.denot.signature}""".stripMargin)
+      }
+    }

compiler/test/dotty/tools/compilerSupport.scala

@@ -1,7 +1,7 @@
 package dotty.tools
 
 import javax.tools._
-import java.io.File
+import java.io._
 import java.nio.file._
 import java.net.URI
 import scala.jdk.CollectionConverters._
@@ -10,14 +10,22 @@ import core._
 import core.Contexts._
 import dotc.core.Comments.{ContextDoc, ContextDocstrings}
 
-/** Initialize a compiler context with the given classpath and
- *  pass it to `op`.
+/** Initialize a compiler context with the given `classpath`, compile all
+ *  `scalaSources`, then run `op`.
+ *
+ *  If `separateRun` is true , then `op` will be run in a new run different from the
+ *  one used to compile the sources, this makes it easier to test for potential
+ *  issues involving retrieving symbols defined in a previous run.
  */
-def inCompilerContext[T](classpath: String)(op: Context ?=> T): T =
+def inCompilerContext[T](classpath: String, separateRun: Boolean = true, scalaSources: String*)(op: Context ?=> T): T =
   val compiler = Compiler()
-  val run = compiler.newRun(initCtx(classpath))
-  run.compileUnits(Nil) // Initialize phases
-  op(using run.runContext)
+  val rootCtx = initCtx(classpath)
+  val firstRun = compiler.newRun(rootCtx)
+  firstRun.compileFromStrings(scalaSources.toList)
+  val opRun = if separateRun
+    then compiler.newRun(rootCtx)
+    else firstRun
+  op(using opRun.runContext)
 
 private def initCtx(classpath: String): Context =
   val ctx0 = (new ContextBase).initialCtx.fresh