Generate generic java signatures #3234

Duhemm · 2017-10-03T13:29:36Z

The code has been ported from scalac.

Two flags are added:

-Yno-generic-signatures disables the generation of generic signatures
-Xverify-signatures runs further verifications of the signatures, and emits a warning if the signature is not valid.

As discussed, the signatures are not generated if they contain symbols that we cannot encode for the JVM.

dottybot

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

smarter · 2017-10-03T13:34:16Z

test performance please

dottybot · 2017-10-03T13:34:57Z

performance test scheduled: 1 job(s) in queue, 1 running.

dottybot · 2017-10-03T22:44:42Z

Performance test finished successfully:

Visit http://dotty-bench.epfl.ch/3234/ to see the changes.

Benchmarks is based on merging with master (432ceac)

odersky · 2017-10-07T17:19:49Z

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

@@ -516,6 +516,18 @@ object Symbols {
    /** The current name of this symbol */
    final def name(implicit ctx: Context): ThisName = denot.name.asInstanceOf[ThisName]

+    def isHigherOrderTypeParameter(implicit ctx: Context): Boolean = this.maybeOwner.isTypeParameterOrSkolem
+    def isTypeParameterOrSkolem(implicit ctx: Context): Boolean = this.isTypeParam


Just use isTypeParam. isTypeParameterOrSkolem would be confusing in a dotty context.

I kept the original name to ease review. I don't really know what Skolem refers to, and was afraid to lose the change in the port from scalac.

odersky · 2017-10-07T17:22:03Z

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

@@ -516,6 +516,18 @@ object Symbols {
    /** The current name of this symbol */
    final def name(implicit ctx: Context): ThisName = denot.name.asInstanceOf[ThisName]

+    def isHigherOrderTypeParameter(implicit ctx: Context): Boolean = this.maybeOwner.isTypeParameterOrSkolem


It's just used once. I'd just use the RHS directly in Erasure. That would avoid having to look up what the term means.

odersky · 2017-10-07T17:31:49Z

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

+  private def isTypeParameterInSig(sym: Symbol, initialSymbol: Symbol)(implicit ctx: Context) = {
+    !sym.isHigherOrderTypeParameter &&
+      sym.isTypeParameterOrSkolem && (
+      (initialSymbol.enclClassChain.exists(sym isContainedIn _)) ||


I believe equivalent, and faster would be:

sym.isContainedIn(initialSymbol.topLevelClass)

That also means we don't need enclClassChain.

odersky · 2017-10-07T17:33:14Z

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

+    def isTypeParameterOrSkolem(implicit ctx: Context): Boolean = this.isTypeParam
+    def enclClassChain(implicit ctx: Context): List[Symbol] = this.maybeOwner.enclClassChain
+    final def isDerivedValueClass(implicit ctx: Context): Boolean =
+      isClass && !denot.is(Flags.Package) && !denot.is(Flags.Trait) &&


No need to write denot. It's used as an implicit everywhere else.

Symbols.scala, as most source files, imports all flags with a wildcard. So for consistency I would not use the Flags prefix.

You can combine the two tests like this:

!this.is(Package | Trait)

But in the end, you should just use ValueClasses.isDerivedValueClass. If the two definitions differ, we should find out which is the right one and use only that one.

odersky · 2017-10-07T17:38:35Z

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

+    /** If this is a derived value class, return its unbox method
+     *  or NoSymbol if it does not exist.
+     */
+    def derivedValueClassUnbox(implicit ctx: Context): Symbol = NoSymbol


Use ValueClasses.valueClassUnbox instead.

odersky · 2017-10-07T17:48:23Z

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

+import dotty.tools.dotc.core.Annotations.Annotation
+import dotty.tools.dotc.core.classfile.ClassfileConstants
+import dotty.tools.dotc.core.TypeApplications.TypeParamInfo
+import java.lang.StringBuilder

 class Erasure extends Phase with DenotTransformer { thisTransformer =>



Erasure is already rather large as is, and therefore people are scared to touch it. We should try to move signature generation to a separate sourcefile. It could be integrated as an object in erasure similarly to how Bridges.scala moves out bridge generation from erasure.

odersky · 2017-10-07T17:54:25Z

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

+  }
+
+  private def hiBounds(bounds: TypeBounds)(implicit ctx: Context): List[Type] = bounds.hi.widenDealias match {
+    case AndType(tp1, tp2) => tp1 :: tp2 :: Nil


What happens with nested AndTypes? Don't you need a recursive call here?

Interesting. I thought at first that it was not necessary, because all the parents would be picked up in jsig and the signature would be correct, but further investigation showed that I was wrong, and it's actually a bug in scalac:

Considering the following definitions:

class C1 { def c1 = 0 } trait T1 { def t1: Int } trait T2 { def t2: Int } class Foo[T <: (C1 with T1) with T2] { // Note the parentheses def foo(t: T): Int = t.c1 + t.t1 + t.t2 }

The signature of Foo that scalac generates is: <T:LC1;:LT2;>Ljava/lang/Object There's no mention of T1! This means that javac won't complain if someone passes an instance of C1 with T2 to def foo, resulting in a runtime crash.

With the recursive call that you suggest, dotty generates the expected signature: <T:LC1;:LT1;:LT2;>Ljava/lang/Object;

/cc @retronym

Thanks, noted as scala/bug#10543

Hrm, looks like another instance of too aggressively replacing normalize (which flattens refined types) with dealias in

private def hiBounds(bounds: TypeBounds): List[Type] = bounds.hi.dealiasWiden match { case RefinedType(parents, _) => parents map (_.dealiasWiden) case tp => tp :: Nil }

odersky · 2017-10-07T17:55:40Z

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

+    case GenericArray(level, core) if !(core <:< defn.AnyRefType) =>
+      level
+    case AndType(tp1, tp2) =>
+      Math.max(unboundedGenericArrayLevel(tp1), unboundedGenericArrayLevel(tp2))


Can use infix max instead.

odersky · 2017-10-07T17:56:49Z

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

+  }
+
+  final def javaSig(sym0: Symbol, info: Type, markClassUsed: Symbol => Unit)(implicit ctx: Context): Option[String] =
+    ctx.atPhase(ctx.erasurePhase) { implicit ctx => javaSig0(sym0, info, markClassUsed) }


Simpler:

javaSig0(sym0, info, markClassUsed)(ctx.withPhase(ctx.erasurePhase))

odersky · 2017-10-07T18:01:12Z

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

+    if (owner.is(Flags.PackageClass) || owner.isTerm) pre else cls.owner.info /* .tpe_* */
+  }
+
+  object GenericArray {


It would be good to base this on the Definitions.MultiArrayOf extractor. This would avoid duplication and simplify the code that needs to be added.

I simplified GenericArray as much as I could, but I cannot use MultiArrayOf directly, since it'll resolve the upper bound when encountering type bounds. This means that types such as

class Foo[T <: Array[Int]]

would get a wrong signature (T <: java.lang.Object instead of T <: int[])

Duhemm · 2017-10-10T12:30:02Z

This is ready for another round of review 😄

allanrenucci · 2017-10-10T12:37:51Z

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

@@ -0,0 +1,510 @@
+package dotty.tools
+package dotc


Should probably be in backend/jvm

Debatable where it should go. But since most of it is part of erasure I'd leave it in transform.

It is only called from backend/jvm/DottyBackendInterface.scala. So my reasoning was that this code is platform specific.
But I don't have a strong opinion about where it should be.

allanrenucci · 2017-10-10T12:38:12Z

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

+/** Helper object to generate generic java signatures, as defined in
+ *  the Java Virtual Machine Specification, §4.3.4
+ */
+object GenericSignatures {


GenericJavaSignatures?

odersky

I did not go through GenericSignatures in detail, since I trust that this was ported correctly from scalac. The integration with the rest of the compiler is now as clean and minimal as it can be, I think. Nice work!

odersky · 2017-10-10T14:19:21Z

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

@@ -745,6 +745,7 @@ object Types {
      else ctx.asSeenFrom(this, pre, cls)
    }

+


Spurious empty line?

Duhemm · 2017-10-23T14:01:55Z

Rebased and added -Xverify-signatures to the default test options.

The code has been ported from scalac.

xuwei-k · 2020-11-25T18:26:58Z

scala/bug#12246

dottybot reviewed Oct 3, 2017

View reviewed changes

smarter assigned OlivierBlanvillain Oct 3, 2017

odersky requested changes Oct 7, 2017

View reviewed changes

retronym mentioned this pull request Oct 9, 2017

Java Generic Signature for nested refinements imprecise scala/bug#10543

Open

Duhemm force-pushed the wip/java-generic-sig branch from c2f78da to a919e9a Compare October 10, 2017 11:18

allanrenucci reviewed Oct 10, 2017

View reviewed changes

odersky approved these changes Oct 10, 2017

View reviewed changes

allanrenucci added this to the 0.5 Tech Preview milestone Oct 17, 2017

Duhemm force-pushed the wip/java-generic-sig branch from a919e9a to 420706d Compare October 23, 2017 12:51

Duhemm added 13 commits October 27, 2017 15:07

Write generic java signatures to classfiles

63754e7

The code has been ported from scalac.

Add thrown exceptions to generic java signature

ced86e4

Generate signature for forwarders

2152bf5

Don't need to call atPhase here

96505c0

Don't generate signatures for backticked-names

ad952a6

Exclude phantom parameters from methods signatures

797a941

Remove unused parameters from scalac port

3f6af19

Fix generic signatures for nested and types

86e8779

Address most of review comments

5fc95dd

Move signature generation to its own object

d969d41

Move type to JVM tag conversion to Definitions

94203bc

Simplify GenericArray extractor

3f4be93

Run compilation tests with -Xverify-signatures

8adc33e

Duhemm force-pushed the wip/java-generic-sig branch from 420706d to 8adc33e Compare October 27, 2017 13:08

allanrenucci merged commit eca989b into scala:master Oct 30, 2017

smarter mentioned this pull request Jan 11, 2018

Emitted classfiles do not contain type parameter information used by javac #1303

Closed

Duhemm deleted the wip/java-generic-sig branch November 30, 2018 07:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate generic java signatures #3234

Generate generic java signatures #3234

Duhemm commented Oct 3, 2017

dottybot left a comment

smarter commented Oct 3, 2017

dottybot commented Oct 3, 2017

dottybot commented Oct 3, 2017

odersky Oct 7, 2017

Duhemm Oct 9, 2017

odersky Oct 7, 2017

odersky Oct 7, 2017

odersky Oct 7, 2017

odersky Oct 7, 2017

odersky Oct 7, 2017

odersky Oct 7, 2017

odersky Oct 7, 2017

odersky Oct 7, 2017

odersky Oct 7, 2017 •

edited

Loading

odersky Oct 7, 2017

Duhemm Oct 9, 2017

smarter Oct 9, 2017

retronym Oct 9, 2017

adriaanm Oct 9, 2017 •

edited

Loading

odersky Oct 7, 2017

odersky Oct 7, 2017

odersky Oct 7, 2017

Duhemm Oct 10, 2017

Duhemm commented Oct 10, 2017

allanrenucci Oct 10, 2017

odersky Oct 10, 2017

allanrenucci Oct 10, 2017

allanrenucci Oct 10, 2017

odersky left a comment

odersky Oct 10, 2017

Duhemm commented Oct 23, 2017

xuwei-k commented Nov 25, 2020

		@@ -745,6 +745,7 @@ object Types {
		else ctx.asSeenFrom(this, pre, cls)
		}

Generate generic java signatures #3234

Generate generic java signatures #3234

Conversation

Duhemm commented Oct 3, 2017

dottybot left a comment

Choose a reason for hiding this comment

smarter commented Oct 3, 2017

dottybot commented Oct 3, 2017

dottybot commented Oct 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odersky Oct 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adriaanm Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Duhemm commented Oct 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odersky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Duhemm commented Oct 23, 2017

xuwei-k commented Nov 25, 2020

odersky Oct 7, 2017 •

edited

Loading

adriaanm Oct 9, 2017 •

edited

Loading