scala / bug

Scala 2 bug reports only. Please, no questions — proper bug reports only.
https://scala-lang.org
232 stars 21 forks source link

incorrect bytecode generated for inner objects #1572

Closed scabug closed 13 years ago

scabug commented 16 years ago

Given the following test code:

trait a {
  object foo {
    def bar = List(1,2).map(_*2)
  }
}

trait b {
  object foo {
    def bar = List(1,2).map(_*2)
  }
  class foo { }
}

...the generated bytecode for a$$foo$$ contains references to the non-existent a$$foo, as does the anon function generated for the map. In the case of b (which defines a class of the same name as the object) the references incorrectly point to the class instead of the object. This can be seen by way of reflection:

scala> Class.forName("a$$foo$$").getDeclaredClasses
res0: Array[java.lang.Class[_]] = Array()
scala> Class.forName("a$$foo").getDeclaredClasses
java.lang.ClassNotFoundException: a$$foo

scala> Class.forName("b$$foo$$").getDeclaredClasses
res1: Array[java.lang.Class[_]] = Array()
scala> Class.forName("b$$foo").getDeclaredClasses 
res2: Array[java.lang.Class[_]] = Array(class b$$foo$$$$anonfun$$bar$$2)

I came across this attempting to use proguard on the scala jars. It objects to processing them because of inconsistent InnerClasses attributes due to the above issue - for instance there are references to the non-existent class scala/tools/nsc/symtab/Definitions$$definitions .

scabug commented 16 years ago

Imported From: https://issues.scala-lang.org/browse/SI-1572?orig=1 Reporter: @paulp Attachments:

scabug commented 16 years ago

@paulp said: Sorry, I should better have demonstrated the other direction.

scala> Class.forName("a$$foo$$$$anonfun$$bar$$1").getDeclaringClass
java.lang.NoClassDefFoundError: a$$foo
    at java.lang.Class.getDeclaringClass(Native Method)
scala> Class.forName("b$$foo$$$$anonfun$$bar$$2").getDeclaringClass
res2: java.lang.Class[_] = class b$$foo
scabug commented 16 years ago

@paulp said: In a related issue which also saddens proguard, there are references to scala.Nothing and scala.Null in the bytecode, which I believe are all supposed to be transformed to scala.Nothing$$ and .Null$$. I am attaching a file listing the ones it found in the 2.7.2 final jars.

scabug commented 15 years ago

Lalit Pant (lpant) said: Here is some more information on this issue, to add to the information already provided:

I see two problems here in the generated class-files:

More details below:

== Problem 1 - Incorrect !InnerClasses attributes == This code demonstrates the problem:

object OneLevelOuter {
    def bar = List(1,2).map(_*2)
}

object TwoLevelOuter {
  object Outer {
    def bar = List(1,2).map(_*2)
  }
}  

Here, !OneLevelOuter$$.class has an !InnerClasses attribute with the following information:[[BR]] inner_class_info: !OneLevelOuter$$$$anonfun$$bar$$1[[BR]] outer_class_info: !OneLevelOuter. This looks incorrect. It should be: !OneLevelOuter$$. In this case, !OneLevelOuter actually does exist, so outer_class_info points to the wrong class.

!TwoLevelOuter$$Outer$$.class has an !InnerClasses attribute with the following information:[[BR]] inner_class_info: !TwoLevelOuter$$Outer$$$$anonfun$$bar$$2[[BR]] outer_class_info: !TwoLevelOuter$$Outer. This looks incorrect. It should be: !TwoLevelOuter$$Outer$$. In this case, !TwoLevelOuter$$Outer does not exist, so outer_class_info points to a non-existant class.

== Problem 2 - Incorrect Method Signature attributes involving Scala.Nothing and Scala.Null ==

This code demonstrates the problem:

object MethodSig {
  val Empty = new collection.immutable.Stack[Nothing]
  def m1: Nothing = {
    throw new RuntimeException()
  }
}

This translates to the following in the classfile for !MethodSig$$:

A method called Empty with:[[BR]] Descriptor: ()Lscala/collection/immutable/Stack;[[BR]] Signature: ()Lscala/collection/immutable/Stack<Lscala/Nothing;>;[[BR]]

A method called m1 with:[[BR]] Descriptor: ()Lscala/runtime/Nothing$$;[[BR]] Signature: none, because this is not a generic method.

As you can see, Nothing is referenced correctly in the Descriptor for m1, but not in the Signature for Empty.

scabug commented 15 years ago

@SethTisue said: tiny jar file you can pass to ProGuard to shut it up about Nothing & Null

scabug commented 15 years ago

@SethTisue said: I hope the jar I just attached helps someone. This is the workaround I am using in my own project. You just add it to the libraryjars (not the injars!) you pass ProGuard. It only makes the Nothing/Null errors go away; it doesn't do anything about the other stuff (the inner class stuff). The only thing I know to do about the inner class thing with ProGuard is use -ignorewarnings.

scabug commented 15 years ago

@dragos said: This is a thorny issue. The problem is that top level objects may generate two classfiles: one for the object itself, which ends in '$$', and a 'mirror' for Java compatibility, having the real name and containing static forwarders. The question is which one is the proper outer class for a given inner class? Right now, the outer is set to be the mirror, as it allows Java code to use the same names as Scala when referring to inner classes.

If I am not mistaken, InnerClasses attributes have to be consistent across a JVM, so the same class cannot appear as having different outer classes in different class files.

I don't see a good solution to this, so I am open to suggestions.

scabug commented 15 years ago

@dragos said: The Nothing and Null references have been fixed in r17396. I leave the ticket open for the inner classes issue.

scabug commented 15 years ago

@paulp said: Replying to [comment:8 dragos]:

If I am not mistaken, InnerClasses attributes have to be consistent across a JVM, so the same class cannot appear as having different outer classes in different class files.

Not that I think pursuing inconsistency is the way to go, but the spec says "The Java virtual machine does not currently check the consistency of the InnerClasses attribute with any class file actually representing a class or interface referenced by the attribute."

http://java.sun.com/docs/books/jvms/second_edition/html/ClassFile.doc.htmlSI-79996

Although I'm not sure it and you are talking about the same thing.

With respect to this issue, I would say anytime "using scala from java" comes into conflict with other reasonable goals, "using scala from java" should lose out, especially if it's just "using scala from java with pretty names." (I do think "using java from scala" should be a high priority.)

scabug commented 15 years ago

@dragos said: Replying to [comment:10 extempore]:

Replying to [comment:8 dragos]:

If I am not mistaken, InnerClasses attributes have to be consistent across a JVM, so the same class cannot appear as having different outer classes in different class files.

Not that I think pursuing inconsistency is the way to go, but the spec says "The Java virtual machine does not currently check the consistency of the InnerClasses attribute with any class file actually representing a class or interface referenced by the attribute."

I vaguely remember having tried to set the outer class to be the current class, so Outer for the mirror class, and Outer$$ for the module class, and getting runtime exceptions on the JVM. I don't think the spec talks about the same thing. I understand it as the outer name might not exist as a class on the class path. What I was hinting at, is having two different, loaded and linked classes, Outer and Outer$$, which have different outer entries for the same inner class. Maybe it was this bug: #1167.

I think this is quite easy to hack, but I am a bit short on time until next week. If someone here is ready to do it, I can point out relevant pieces of code.

With respect to this issue, I would say anytime "using scala from java" comes into conflict with other reasonable goals, "using scala from java" should lose out, especially if it's just "using scala from java with pretty names." (I do think "using java from scala" should be a high priority.)

Agreed. So you would make inner classes member of the module class, and live with '$$' names in Java? That was the way it worked some time ago, then it was 'fixed' to support nicer names for inner classes. I don't remember all the arguments, but we should check at least #1126 before doing the change.

scabug commented 15 years ago

@paulp said: Even for top level objects, forwarders are already installed only on a "best effort" basis, right? There's no getting around the need to know ugly details for anyone set on using scala from java. So yes, I would tentatively vote that scala inner classes should not profess knowledge of the mirror class. I see the implications need exploring, but I'm trying really hard not to get knocked too far off track on my parser work. I will come back to this when I can.

scabug commented 15 years ago

@dragos said: I change the way InnerClasses are reported for inner objects, but kept the old scheme for top level objects. That should not be a problem: in the case when there's a companion class, the reference is correct. If there's no companion, that the compiler generates a mirror class, and again the reference is 'bound'. I think this way we get the best of both worlds.

I had to fix another problem with the way java signatures were generated, but now proguard is happy with the scala compiler bytecode, except for two warnings about reflective calls in the interpreter.

The fix is in r18618.

scabug commented 15 years ago

@SethTisue said: I'm overjoyed!! Thank you.

scabug commented 14 years ago

Paul LaCrosse (pjlacrosse) said: Replying to [comment:13 dragos]:

I change the way InnerClasses are reported for inner objects, but kept the old scheme for top level objects. That should not be a problem: in the case when there's a companion class, the reference is correct. If there's no companion, that the compiler generates a mirror class, and again the reference is 'bound'. I think this way we get the best of both worlds.

I had to fix another problem with the way java signatures were generated, but now proguard is happy with the scala compiler bytecode, except for two warnings about reflective calls in the interpreter.

The fix is in r18618. While I've been using ProGuard 4.5Beta3 with no problems with Scala 2.8Beta1, switching to recent nightly builds of Scala 2.8 (re)introduces the problems of bytecode references to non-existant classes.

It is not limited to the Scala library, but shows up for user classes:

[proguard] Warning: com.test.app.abc.data.QueryDtl$$: can't find referenced class com.test.app.abc.data.QueryDtl$$$$anonfun$$apply$$1

scabug commented 14 years ago

@paulp said: Replying to [comment:16 pjlacrosse]:

It is not limited to the Scala library, but shows up for user classes:

[proguard] Warning: com.test.app.abc.data.QueryDtl$$: can't find referenced class com.test.app.abc.data.QueryDtl$$$$anonfun$$apply$$1

Please supply us with something more concrete than an error message if you want this to have any chance of being investigated. It's quite frustrating when people expend so little effort making a report.

scabug commented 14 years ago

Paul LaCrosse (pjlacrosse) said: Ok, I will only report errors where I've had the time to fully investigate how to create a minimal failing sample. Otherwise, I will keep the failures secret and let other people discover them on their own.