Fix java signature generation for traits: no cl...

Fix java signature generation for traits: no classes as parents. Closes
SI-4891, review by grek.

10 files changed, 165 insertions, 50 deletions

diff --git a/compare-signatures b/compare-signatures
new file mode 100755
index 0000000000..17e9ace476
--- /dev/null
+++ b/compare-signatures
@@ -0,0 +1,13 @@
+#!/usr/bin/env bash
+#
+
+file=${1:-test/files/run/bug4891/S_1.scala}
+# src/library/scala/collection/immutable/MapLike.scala
+
+rm -rf /tmp/o1 /tmp/o2
+mkdir /tmp/o1 /tmp/o2
+
+scalac3 -d /tmp/o1 $file
+pscalac -d /tmp/o2 $file
+
+jpdiff /tmp/o1 /tmp/o2
diff --git a/src/compiler/scala/reflect/internal/Types.scala b/src/compiler/scala/reflect/internal/Types.scala
index 4a2d8e9f08..7bcc0f8913 100644
--- a/src/compiler/scala/reflect/internal/Types.scala
+++ b/src/compiler/scala/reflect/internal/Types.scala
@@ -3070,6 +3070,18 @@ A type's typeSymbol should never be inspected directly.
     }
   }
 
+  /** Substitutes the empty scope for any non-empty decls in the type. */
+  object dropAllRefinements extends TypeMap {
+    def apply(tp: Type): Type = tp match {
+      case rt @ RefinedType(parents, decls) if !decls.isEmpty =>
+        mapOver(copyRefinedType(rt, parents, EmptyScope))
+      case ClassInfoType(parents, decls, clazz) if !decls.isEmpty =>
+        mapOver(ClassInfoType(parents, EmptyScope, clazz))
+      case _ =>
+        mapOver(tp)
+    }
+  }
+
   // Set to true for A* => Seq[A]
   //   (And it will only rewrite A* in method result types.)
   //   This is the pre-existing behavior.
diff --git a/src/compiler/scala/tools/nsc/backend/jvm/GenJVM.scala b/src/compiler/scala/tools/nsc/backend/jvm/GenJVM.scala
index be98b314a4..c0a9192c86 100644
--- a/src/compiler/scala/tools/nsc/backend/jvm/GenJVM.scala
+++ b/src/compiler/scala/tools/nsc/backend/jvm/GenJVM.scala
@@ -11,7 +11,6 @@ import java.io.{ DataOutputStream, OutputStream }
 import java.nio.ByteBuffer
 import scala.collection.{ mutable, immutable }
 import scala.reflect.internal.pickling.{ PickleFormat, PickleBuffer }
-import scala.tools.reflect.SigParser
 import scala.tools.nsc.util.ScalaClassLoader
 import scala.tools.nsc.symtab._
 import scala.reflect.internal.ClassfileConstants._
@@ -603,14 +602,6 @@ abstract class GenJVM extends SubComponent with GenJVMUtil with GenAndroid with
       nannots
     }
 
-    /** Run the signature parser to catch bogus signatures.
-     */
-    def isValidSignature(sym: Symbol, sig: String) = (
-      if (sym.isMethod) SigParser verifyMethod sig
-      else if (sym.isTerm) SigParser verifyType sig
-      else SigParser verifyClass sig
-    )
-
     // @M don't generate java generics sigs for (members of) implementation
     // classes, as they are monomorphic (TODO: ok?)
     private def needsGenericSignature(sym: Symbol) = !(
@@ -632,12 +623,8 @@ abstract class GenJVM extends SubComponent with GenJVMUtil with GenAndroid with
         // println("addGenericSignature: "+ (sym.ownerChain map (x => (x.name, x.isImplClass))))
         erasure.javaSig(sym, memberTpe) foreach { sig =>
           debuglog("sig(" + jmember.getName + ", " + sym + ", " + owner + ")      " + sig)
-          /** Since we're using a sun internal class for signature validation,
-           *  we have to allow for it not existing or otherwise malfunctioning:
-           *  in which case we treat every signature as valid.  Medium term we
-           *  should certainly write independent signature validation.
-           */
-          if (settings.Xverify.value && SigParser.isParserAvailable && !isValidSignature(sym, sig)) {
+
+          if (settings.Xverify.value && !erasure.isValidSignature(sym, sig)) {
             clasz.cunit.warning(sym.pos,
                 """|compiler bug: created invalid generic signature for %s in %s
                    |signature: %s
diff --git a/src/compiler/scala/tools/nsc/transform/Erasure.scala b/src/compiler/scala/tools/nsc/transform/Erasure.scala
index fc3aae9046..dbceb1f721 100644
--- a/src/compiler/scala/tools/nsc/transform/Erasure.scala
+++ b/src/compiler/scala/tools/nsc/transform/Erasure.scala
@@ -6,6 +6,7 @@
 package scala.tools.nsc
 package transform
 
+import scala.tools.reflect.SigParser
 import scala.reflect.internal.ClassfileConstants._
 import scala.collection.{ mutable, immutable }
 import symtab._
@@ -101,6 +102,12 @@ abstract class Erasure extends AddInterfaces
   // for debugging signatures: traces logic given system property
   // performance: get the value here
   val traceSignatures = (sys.BooleanProp keyExists "scalac.sigs.trace").value
+  private object traceSig extends util.Tracer(() => traceSignatures) {
+    override def stringify(x: Any) = x match {
+      case tp: Type   => super.stringify(dropAllRefinements(tp))
+      case _          => super.stringify(x)
+    }
+  }
 
   override protected def verifyJavaErasure = settings.Xverify.value || settings.debug.value
   private def needsJavaSig(tp: Type) = !settings.Ynogenericsig.value && NeedsSigCollector.collect(tp)
@@ -130,7 +137,6 @@ abstract class Erasure extends AddInterfaces
       case ch                 => last = ch ; ch
     }
   }
-  private val traceSig  = util.Tracer(traceSignatures)
 
   /** This object is only used for sanity testing when -check:genjvm is set.
    *  In that case we make sure that the erasure of the `normalized` type
@@ -200,16 +206,60 @@ abstract class Erasure extends AddInterfaces
     }
   }
 
+  /** Run the signature parser to catch bogus signatures.
+   */
+  def isValidSignature(sym: Symbol, sig: String) = (
+    /** Since we're using a sun internal class for signature validation,
+     *  we have to allow for it not existing or otherwise malfunctioning:
+     *  in which case we treat every signature as valid.  Medium term we
+     *  should certainly write independent signature validation.
+     */
+    SigParser.isParserAvailable && (
+      if (sym.isMethod) SigParser verifyMethod sig
+      else if (sym.isTerm) SigParser verifyType sig
+      else SigParser verifyClass sig
+    )
+  )
+
+  private def hiBounds(bounds: TypeBounds): List[Type] = bounds.hi.normalize match {
+    case RefinedType(parents, _) => parents map (_.normalize)
+    case tp                      => tp :: Nil
+  }
+
   /** The Java signature of type 'info', for symbol sym. The symbol is used to give the right return
    *  type for constructors.
    */
   def javaSig(sym0: Symbol, info: Type): Option[String] = atPhase(currentRun.erasurePhase) {
+    val isTraitSignature = sym0.enclClass.isTrait
+
+    def superSig(parents: List[Type]) = traceSig("superSig", parents) {
+      val ps = (
+        if (isTraitSignature) {
+          // java is unthrilled about seeing interfaces inherit from classes
+          val ok = parents filter (p => p.typeSymbol.isTrait || p.typeSymbol.isInterface)
+          // traits should always list Object.
+          if (ok.isEmpty || ok.head.typeSymbol != ObjectClass) ObjectClass.tpe :: ok
+          else ok
+        }
+        else parents
+      )
+      ps map boxedSig mkString
+    }
     def boxedSig(tp: Type) = jsig(tp, primitiveOK = false)
-
-    def hiBounds(bounds: TypeBounds): List[Type] = bounds.hi.normalize match {
-      case RefinedType(parents, _) => parents map normalize
-      case tp                      => tp :: Nil
+    def boundsSig(bounds: List[Type]) = traceSig("boundsSig", bounds) {
+      val (isTrait, isClass) = bounds partition (_.typeSymbol.isTrait)
+      val classPart = isClass match {
+        case Nil    => ":" // + boxedSig(ObjectClass.tpe)
+        case x :: _ => ":" + boxedSig(x)
+      }
+      classPart :: (isTrait map boxedSig) mkString ":"
     }
+    def paramSig(tsym: Symbol) = tsym.name + boundsSig(hiBounds(tsym.info.bounds))
+    def polyParamSig(tparams: List[Symbol]) = traceSig("polyParamSig", tparams) (
+      if (tparams.isEmpty) ""
+      else tparams map paramSig mkString ("<", "", ">")
+    )
+
     // Anything which could conceivably be a module (i.e. isn't known to be
     // a type parameter or similar) must go through here or the signature is
     // likely to end up with Foo<T>.Empty where it needs Foo<T>.Empty$.
@@ -276,19 +326,9 @@ abstract class Erasure extends AddInterfaces
           else jsig(erasure(sym0, tp), existentiallyBound, toplevel, primitiveOK)
         case PolyType(tparams, restpe) =>
           assert(tparams.nonEmpty)
-          def boundSig(bounds: List[Type]) = {
-            val (isTrait, isClass) = bounds partition (_.typeSymbol.isTrait)
+          val poly = if (toplevel) polyParamSig(tparams) else ""
+          poly + jsig(restpe)
 
-            ":" + (
-              if (isClass.isEmpty) "" else boxedSig(isClass.head)
-            ) + (
-              isTrait map (x => ":" + boxedSig(x)) mkString
-            )
-          }
-          def paramSig(tsym: Symbol) = tsym.name + boundSig(hiBounds(tsym.info.bounds))
-
-          val paramString = if (toplevel) tparams map paramSig mkString ("<", "", ">") else ""
-          paramString + jsig(restpe)
         case MethodType(params, restpe) =>
           "("+(params map (_.tpe) map (jsig(_))).mkString+")"+
           (if (restpe.typeSymbol == UnitClass || sym0.isConstructor) VOID_TAG.toString else jsig(restpe))
@@ -296,7 +336,7 @@ abstract class Erasure extends AddInterfaces
         case RefinedType(parent :: _, decls) =>
           boxedSig(parent)
         case ClassInfoType(parents, _, _) =>
-          (parents map (boxedSig(_))).mkString
+          superSig(parents)
         case AnnotatedType(_, atp, _) =>
           jsig(atp, existentiallyBound, toplevel, primitiveOK)
         case BoundedWildcardType(bounds) =>
@@ -308,13 +348,22 @@ abstract class Erasure extends AddInterfaces
           else jsig(etp)
       }
     }
-    traceSig("javaSig", (sym0, info)) {
+    val result = traceSig("javaSig", (sym0, info)) {
       if (needsJavaSig(info)) {
         try Some(jsig(info, toplevel = true))
         catch { case ex: UnknownSig => None }
       }
       else None
     }
+    // Debugging: immediately verify signatures when tracing.
+    if (traceSignatures) {
+      result foreach { sig =>
+        if (!isValidSignature(sym0, sig))
+          println("**** invalid signature for " + sym0 + ": " + sig)
+      }
+    }
+
+    result
   }
 
   class UnknownSig extends Exception
diff --git a/src/compiler/scala/tools/nsc/typechecker/Typers.scala b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
index 149763cf44..8dd6855464 100644
--- a/src/compiler/scala/tools/nsc/typechecker/Typers.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/Typers.scala
@@ -186,6 +186,16 @@ trait Typers extends Modes with Adaptations {
     var context = context0
     def context1 = context
 
+    def dropExistential(tp: Type): Type = tp match {
+      case ExistentialType(tparams, tpe) =>
+        new SubstWildcardMap(tparams).apply(tp)
+      case TypeRef(_, sym, _) if sym.isAliasType =>
+        val tp0 = tp.normalize
+        val tp1 = dropExistential(tp0)
+        if (tp1 eq tp0) tp else tp1
+      case _ => tp
+    }
+
     /** Check that <code>tree</code> is a stable expression.
      *
      *  @param tree ...
@@ -4263,17 +4273,6 @@ trait Typers extends Modes with Adaptations {
     def typed(tree: Tree, mode: Int, pt: Type): Tree = {
       indentTyping()
 
-      def dropExistential(tp: Type): Type = tp match {
-        case ExistentialType(tparams, tpe) =>
-          debuglog("Dropping existential: " + tree + " " + tp)
-          new SubstWildcardMap(tparams).apply(tp)
-        case TypeRef(_, sym, _) if sym.isAliasType =>
-          val tp0 = tp.normalize
-          val tp1 = dropExistential(tp0)
-          if (tp1 eq tp0) tp else tp1
-        case _ => tp
-      }
-
       var alreadyTyped = false
       try {
         if (Statistics.enabled) {
diff --git a/src/compiler/scala/tools/nsc/util/Tracer.scala b/src/compiler/scala/tools/nsc/util/Tracer.scala
index ec1eaa13ea..acbf60da5b 100644
--- a/src/compiler/scala/tools/nsc/util/Tracer.scala
+++ b/src/compiler/scala/tools/nsc/util/Tracer.scala
@@ -11,8 +11,6 @@ import scala.runtime.ScalaRunTime
 
 class Tracer(enabled: () => Boolean) {
   def out: PrintStream = System.out
-  def intoString(x: Any): String = "" + x
-
   def stringify(x: Any) = ScalaRunTime stringOf x
 
   // So can pass tuples, lists, whatever as arguments and don't
@@ -52,14 +50,18 @@ class Tracer(enabled: () => Boolean) {
 
     if (enabled()) {
       passign(name, stringifyArgs(args))
+      val resultToPrint = result match {
+        case Some(x)  => x
+        case _        => result
+      }
       // concise output optimization
-      val isOneliner = result match {
+      val isOneliner = resultToPrint match {
         case _: Boolean | _: None.type => true
         case s: String                 => s.length < 40
         case _                         => false
       }
-      if (isOneliner) p(stringify(result) + "\n")
-      else pblock(result)
+      if (isOneliner) p(stringify(resultToPrint) + "\n")
+      else pblock(resultToPrint)
     }
 
     result
diff --git a/test/files/run/bug4891.check b/test/files/run/bug4891.check
new file mode 100644
index 0000000000..2e84168ed4
--- /dev/null
+++ b/test/files/run/bug4891.check
@@ -0,0 +1,9 @@
+test.generic.T1
+  (m) public abstract A test.generic.T1.t1(A)
+test.generic.C1
+  (m) public void test.generic.C1.m1()
+test.generic.C2
+  (m) public void test.generic.C1.m1()
+  (m) public A test.generic.C2.t1(A) (bridge)
+null
+interface scala.ScalaObject
diff --git a/test/files/run/bug4891/J_2.java b/test/files/run/bug4891/J_2.java
new file mode 100644
index 0000000000..db1cc52b13
--- /dev/null
+++ b/test/files/run/bug4891/J_2.java
@@ -0,0 +1,13 @@
+import test.generic.*;
+
+public class J_2 {
+  public static <A> void foo(T1<A> x) {
+    // x.m1();
+  }
+
+  public static void main(String[] args) {
+    Bug4891.main(null);
+    T1<Object> x = new C2<Object>();
+    foo(x);
+  }
+}
diff --git a/test/files/run/bug4891/S_1.scala b/test/files/run/bug4891/S_1.scala
new file mode 100644
index 0000000000..927c3ad93d
--- /dev/null
+++ b/test/files/run/bug4891/S_1.scala
@@ -0,0 +1,26 @@
+package test.generic {
+  class C1[A] {
+    def m1(): Unit = ()
+  }
+
+  trait T1[A] extends C1[A] {
+    def t1(x: A) = x
+  }
+
+  class C2[A] extends T1[A]
+}
+
+import scala.tools.partest._
+
+object Bug4891 extends SigTest {
+  import test.generic._
+
+  def main(args: Array[String]): Unit = {
+    show[T1[_]]()
+    show[C1[_]]()
+    show[C2[_]]()
+
+    println(classOf[T1[_]].getGenericSuperclass)
+    classOf[T1[_]].getGenericInterfaces foreach println
+  }
+}
diff --git a/test/files/run/bug4891/S_3.scala b/test/files/run/bug4891/S_3.scala
new file mode 100644
index 0000000000..0da4912bc5
--- /dev/null
+++ b/test/files/run/bug4891/S_3.scala
@@ -0,0 +1,5 @@
+object Test {
+  def main(args: Array[String]): Unit = {
+    J_2 main null
+  }
+}