Skip to content

SI-8931 make generic signature consistent with interface list in classfiles#4080

Merged
retronym merged 1 commit intoscala:2.11.xfrom
gourlaysama:wip/t8931-redundant-interfaces-2
Nov 7, 2014
Merged

SI-8931 make generic signature consistent with interface list in classfiles#4080
retronym merged 1 commit intoscala:2.11.xfrom
gourlaysama:wip/t8931-redundant-interfaces-2

Conversation

@gourlaysama
Copy link
Contributor

An optimization was introduced in 7a99c03 (SI-5278) to remove redundant
interfaces from the list of implemented interfaces in the bytecode.
However the same change was not propagated to the generic signature
of a class, which also contains a list of direct parent classes and
interfaces.

The JVM does not check the well-formedness of signatures at class
loading or linking (see §4.3.4 of jdk7 jvms), but other tools might
assume the number of implemented interfaces is the same whether one
asked for generic or erased interfaces.

It doesn't break reflection so nobody complained, but it does show:

scala> val c = classOf[Tuple1[String]]
c: Class[(String,)] = class scala.Tuple1

scala> c.getInterfaces // Product is gone
res0: Array[Class[_]] = Array(interface scala.Product1, interface
scala.Serializable)

scala> c.getGenericInterfaces // Product is back!
res1: Array[java.lang.reflect.Type] = Array(scala.Product1<T1>,
interface scala.Product, interface scala.Serializable)

This moves the optimization to erasure, for use in emitting the generic
signature, and the backend calls into it later for the list of
interfaces.

@scala-jenkins scala-jenkins added this to the 2.11.5 milestone Oct 28, 2014
@gourlaysama gourlaysama force-pushed the wip/t8931-redundant-interfaces-2 branch from 7134259 to dca78a4 Compare October 29, 2014 13:07
@gourlaysama
Copy link
Contributor Author

Hmm, that what I get for testing on quick without building locker...

@gourlaysama gourlaysama reopened this Oct 31, 2014
@gourlaysama gourlaysama force-pushed the wip/t8931-redundant-interfaces-2 branch 2 times, most recently from 41f461d to cc1c3f6 Compare October 31, 2014 12:26
@gourlaysama
Copy link
Contributor Author

Here is the diff for scala-library.jar for this change: https://gist.github.com/gourlaysama/91e2d23ec1b60a9a186e (collections and tuples are the most affected). It isn't incredibly readable, but it helps.

@gourlaysama
Copy link
Contributor Author

Review @retronym :)

@lrytz
Copy link
Member

lrytz commented Nov 4, 2014

There's still duplication in the new backend (totally not your fault..!): https://github.com/scala/scala/blob/2.11.x/src/compiler/scala/tools/nsc/backend/jvm/BTypesFromSymbols.scala#L202

Can you also use erasure.minimizeInterfaces there?

@lrytz
Copy link
Member

lrytz commented Nov 4, 2014

Looks good otherwise!

@gourlaysama gourlaysama force-pushed the wip/t8931-redundant-interfaces-2 branch from cc1c3f6 to a329917 Compare November 4, 2014 15:12
@gourlaysama
Copy link
Contributor Author

@lrytz done. Thanks, I had completely overlooked that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would actually write this test as a run test that uses Java reflection to extract the list of generic interfaces. Furthermore, by setting up the classes in a self contained manner in the test, the intent is clearer.

scala> trait B; trait A extends B; class C extends A with B
defined trait B
defined trait A
defined class C

scala> classOf[C].getGenericInterfaces
res2: Array[java.lang.reflect.Type] = Array(interface A)

In the test, you would just need:

trait B; trait A extends B; class C extends A with B
object Test extends App {
   println(classOf[C].getGenericInterfaces.toList)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

…sfiles

An optimization was introduced in 7a99c03 (SI-5278) to remove redundant
interfaces from the list of implemented interfaces in the bytecode.
However the same change was not propagated to the generic signature
of a class, which also contains a list of direct parent classes and
interfaces.

The JVM does not check the well-formedness of signatures at class
loading or linking (see §4.3.4 of jdk7 jvms), but other tools might
assume the number of implemented interfaces is the same whether one
asked for generic or erased interfaces.

It doesn't break reflection so nobody complained, but it does show:

scala> val c = classOf[Tuple1[String]]
c: Class[(String,)] = class scala.Tuple1

scala> c.getInterfaces // Product is gone
res0: Array[Class[_]] = Array(interface scala.Product1, interface
scala.Serializable)

scala> c.getGenericInterfaces // Product is back!
res1: Array[java.lang.reflect.Type] = Array(scala.Product1<T1>,
interface scala.Product, interface scala.Serializable)

This moves the optimization to erasure, for use in emitting the generic
signature, and the backend calls into it later for the list of
interfaces.
@gourlaysama gourlaysama force-pushed the wip/t8931-redundant-interfaces-2 branch from a329917 to ced3ca8 Compare November 5, 2014 10:55
@gourlaysama
Copy link
Contributor Author

PLS REBUILD/pr-scala@ced3ca8

@scala-jenkins
Copy link

(kitty-note-to-self: ignore 61896124)
🐱 Roger! Rebuilding pr-scala for ced3ca8. 🚨

@retronym
Copy link
Member

retronym commented Nov 7, 2014

LGTM!

retronym added a commit that referenced this pull request Nov 7, 2014
…ces-2

SI-8931 make generic signature consistent with interface list in classfiles
@retronym retronym merged commit ecdac37 into scala:2.11.x Nov 7, 2014
@gourlaysama gourlaysama deleted the wip/t8931-redundant-interfaces-2 branch November 12, 2014 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants