getClass fails for doubly nested inner-class

scabug commented 14 years ago

The following code fails with java.lang.ClassNotFoundException: TestScalaBug$$foo

workaround is to add a class foo

may be related to bug 1572

object TestScalaBug {

  def main(args: Array[String]) {
    val fooz = new foo.foo2
    println(fooz)
  }

  object foo {
    class foo2 {
      override def toString = getClass.getSimpleName
    }
  }

}

scabug commented 14 years ago

Imported From: https://issues.scala-lang.org/browse/SI-2034?orig=1 Reporter: hassan chafi (hchafi) Attachments:

0001-SI-2034-calling-getSimpleName-crashes-java-reflectio.patch (created on Aug 15, 2013 11:05:55 PM UTC, 29404 bytes)
Mar2015-0001-SI-2034-calling-getSimpleName-crashes-java-reflectio.patch (created on Mar 3, 2015 2:25:55 PM UTC, 26644 bytes)

scabug commented 14 years ago

@phaller said: Added test (in pending/run/t2034.scala).

scabug commented 14 years ago

Eric Willigers (ewilligers) said: The error is

java.lang.InternalError: Malformed class name
        at java.lang.Class.getSimpleName(Unknown Source)

like Ticket #286 - getSimpleName expects a $$ between TestScalaBug$$foo$$ and foo2's class name?

Could TestScalaBug$$foo$$foo2 instead be named TestScalaBug$$foo$$$$foo2 ? I understand that this conflicts with the goal of having nice names when accessing Scala classes from Java.

The alternate approach would be to generate a TestScalaBug$$foo class, with TestScalaBug$$foo$$foo2 being an inner class of that class. But I suspect that approach may have already been considered and rejected in Ticket #1572

scabug commented 14 years ago

@odersky said: Iulian, I am stuck here. Do you have an idea what to do?

scabug commented 14 years ago

Barry Kaplan (memelet) said: I'm running into this with 2.8.Beta1 when interfacing with a java library (Drools). Can't change the Drools code so I'm quite stuck as well.

scabug commented 13 years ago

@paulp said: See also #3845.

scabug commented 13 years ago

@eengbrec said: See #4118, it is most likely at least related if not a duplicate.

scabug commented 12 years ago

Ed Gow (egow) said (edited on Oct 4, 2011 6:34:04 PM UTC): This is a sudden showstopper for me too.

Test case, in the REPL, is

class Foo println(classOf[Foo].getSimpleName)

This bug has been around for over two years. I'm really curious whether it's a permanent feature of Scala.

Let me explain my predicament. I'm extremely interested in Scala, and I'd like very much to use it. For my first foray, I tried doing some experimenting with the OrientDB document/object database using Scala. The very first time I tried to store an instance of a class created in the Scala REPL I got the getSimpleName() exception. That ended my use of Scala for that project.

The function getSimpleName() has been in Java for seven years, and I don't think it's going away. Any java framework, library, etc. is free to use it, and if they don't use it today they just might in their next version. Then any Scala code using that java library is toast.

So that is my concern. Java interoperability is a key feature of Scala, but if this bug is any indication, then it's not a very reliable feature. This bug, and others associated with it, have named several java libraries that are broken by the getSimpleName() problem. I understand that it's difficult to fix, but it seems to be such a basic flaw that allowing it to persist is essentially a declaration that Java interop is a secondary concern.

Given the current Scala focus on stability of the binary, is this bug likely to ever be fixed, or is the interop flaw permanent?

scabug commented 12 years ago

@paulp said: If the ticket is still open, it's because someone (me for instance) would still like to fix it. If the day should come where we resign ourselves to the present state of affairs, I'll close the ticket. I do not anticipate that happening anytime soon, but neither will this be fixed tomorrow.

scabug commented 12 years ago

Matthew Pocock (drdozer) said: I'm getting this from akka. I've learned that I can't nest an actor class in an object in an object and expect to avoid hitting this issue.

scabug commented 11 years ago

Sarah Gerweck (gerweck) said: This is a really nasty issue that prevents Scala from being used with a great many modern frameworks that rely on reflection and annotation scanning for service detection. This issue is a showstopper for anybody who wants to use Scala in a Java EE environment (like me).

(I apologize that this is a bit of a "me too," but I think this issue is serious enough that it warrants the bump. This, along with the related #6546, are in danger of killing the Scala pilot at my company. I can't even load our code into the same container without breaking the whole deployment.)

If binary compatibility is the concern, maybe we can have a compiler flag or something? If a new compiler could support both versions, I could happily throw the switch and not worry about old compilers being able to use my libraries. I might even be willing to write some code here, but I'm not sure that it would actually save any time by the time someone finished pointing me in the right direction as I don't really know the Scala compiler. :-)

scabug commented 11 years ago

@paulp said: It isn't compiler knowledge that is necessary so much as a great deal of time and patience. The algorithm by which names are derived requires comprehensive modification. A single character, $, is used to encode dozens of different pieces of information. That character is also used by java. There is no way to correctly handle all competing uses while still only using $, especially given the hardcoded assumptions in java.lang.Class which cause the getSimpleName issue. The workable answer in my opinion is to reserve characters from a larger set and start using meaningfully: see http://blogs.oracle.com/jrose/entry/symbolic_freedom_in_the_vm . There is much which would have to be kept working and little documentation about what it is. That however is the task.

scabug commented 10 years ago

Charles Oliver Nutter (headius) said: We (JRuby) have had this issue brought to our attention due to an issue caused when attempting to bind one of these classes in JRuby. We use the reflection APIs in question, including getSimpleName...and as a result, the class fails to bind.

I do not have a solution for you, but I will mention that JRuby has been using the techniques in John Rose's "symbolic freedom" post for many of our internal classes. It works pretty well...but there are some JVMs that are more strict (than they are supposed to be) that have had issues with some of our method names (J9 that I recall offhand).

In any case, we'll probably have to work around this in JRuby as well. I'm not sure I agree that it's a Scala issue, since getSimpleName really shouldn't blow up like this. But the way things are is the way things are. I hope Scala will find an alternative that works with current getSimpleName, and I hope the JDK gets patched to not blow up on unusually-named classes.

scabug commented 10 years ago

@paulp said: I fixed this but pushing the fix through exceeds my abilities, so I am attaching it here in case it is of interest to others.

scabug commented 10 years ago

Olivier Toupin (oliviertoupin) said (edited by @dragos on Jun 23, 2014 1:20:14 PM UTC): Double nested class fail: with java.lang.InternalError: Malformed class name

object A {
  object B {
    class C {

    }
  }
}

val c = new A.B.C()
c.getClass.getSimpleName() // Throws: java.lang.InternalError: Malformed class name

scabug commented 9 years ago

@dragos said (edited on Jun 23, 2014 2:08:41 PM UTC): There must be some truth in saying "better late than never", so almost 5 years after Martin's question, here are some thoughts:

the deeply nested class C is a member of object B, whose binary name is A$B$. However, the name module suffix (the last $) is added during bytecode generation. In the AST, one only sees the flattened name, which is A$B.
using the flattened name as the outer name won't work, since the flattened name corresponds to a companion class, which may not exist (in this case, there isn't one). Emitting such a name migth break getEnclosingClass. The bytecode generator therefore uses the binary name for outer_name inside the InnerClasses attribute
since objects are "the equivalent of static" in the Java world, we wanted to make it easier for Java code to use static inner classes. When there's only one level of nesting, there's a guaranteed companion: every top-level object gets a mirror class (if there isn't one) that creates static forwarders to module methods (that's how one can run a main method defined inside an object). Therefore, a special case for one-level nesting: those classes use the flattened name (without a $ suffix) as outer_name. So, Java code can say new Outer.Inner.

The issue in this case is the inconsistency between the InnerClasses attribute and the binary name of the inner class. In Olivier's example, the flattened name of C is A$B$C, which ends up as the binary name of C. However, the JLS requires a separating $ between the outer name and the inner name, which is missing in this case.

The code that decides the binary name of C is this:

    override def name: TermName = {
      if (Statistics.hotEnabled) Statistics.incCounter(nameCount)
      if (!isMethod && needsFlatClasses) {
        if (flatname eq null)
          flatname = nme.flattenedName(rawowner.name, rawname)

        flatname
      }
      else rawname
    }
  }

This code does not know about the logic of encoding objects, so it simply uses the flattened name of the owner followed by $ and inner name (A$B + $ + C). This misses a $, because the binary name of B (after code generation) is A$B$, and the InnerClasses attribute will use it too. At this point, the invariant that the JLS specifies in 13.1, binary_name == outer_name + $ + inner_name, is broken.

The whole scheme needs a rehaul. The fact that these names are computed at different times and with completely disconnected logic will bite us in the future too.

one solution would be to move the module-generation code from the backe-end to the AST, and expose nesting at the tree level (some initialization code in the module constructor might not be expressible as AST nodes, so that's a concern).
a more local fix might be to give up the "optimization" for Java convenience and simplify the problem -- and less likely to make mistakes. Still, the disconnect between the logic of flattening names, module code generation and InnerClasses attribute generation is risky

An even more ambitious plan would be to rehaul all synthetic name generation. I find the way we generate anonfun names to be unnecessarily verbose. Having a spec would be a great step forward, and justify breaking some tools if the new scheme is sane and well defined. There's material for a SIP here (not a very glamorous one, but a very useful one).

scabug commented 9 years ago

@paulp said: Did you look at the attachment? Synthetic name generation needs an overhaul anyway, but when I say "I fixed this" I meant it literally.

scabug commented 9 years ago

@dragos said: Paul, I had a look at your fix, but I thought a summary would be beneficial for whoever wants to fix this. One thing that jumped at me was

-          flatname = nme.flattenedName(rawowner.name, rawname)
+          flatname = nme.flattenedName(rawowner.javaSimpleName,rawname)

This seems to only look at the simple name of the owner, while the previous code would recurse through .name. I didn't build your code though, so I might have missed a compensating change in the rest of the patch. Since I don't plan to fix this myself (just realized it was assigned to me!), I didn't investigate further.

scabug commented 9 years ago

@paulp said: I think it's easier to look at the test case than at the code, especially a year later. The patch also includes all the changes necessary for all the tests to pass, which shows in various ways how it differs from code presently being generated.

// All eight combinations of class/object nesting to three levels.
package s {
  object o1 {
    object o2 { object o3; class o3 }
    class o2  { object o3; class o3 }
  }
  class o1 {
    object o2 { object o3; class o3 }
    class o2  { object o3; class o3 }
  }
}

// Yes, that's the only way to write these types.
show[s.o1.o2.o3.type]("O-O-O")
show[s.o1.o2.o3]("O-O-C")
show[p.o3.type forSome { val p: s.o1.o2 }]("O-C-O")
show[s.o1.o2#o3]("O-C-C")

show[p.o2.o3.type forSome { val p: s.o1 }]("C-O-O")
show[p.o2.o3 forSome { val p: s.o1 }]("C-O-C")
show[p.o3.type forSome { val p: s.o1#o2 }]("C-C-O")
show[s.o1#o2#o3]("C-C-C")

// Now again, but calling getClass on an instance instead
// of using the type to summon a class object. This should
// print the same eight lines as above.
showRef("O-O-O", s.o1.o2.o3)
showRef("O-O-C", new s.o1.o2.o3)
showRef("O-C-O", (new s.o1.o2).o3)
showRef("O-C-C", { val p = new s.o1.o2 ; new p.o3 })
showRef("C-O-O", (new s.o1).o2.o3)
showRef("C-O-C", { val p = (new s.o1).o2 ; new p.o3 })
showRef("C-C-O", { val p = (new s.o1) ; (new p.o2).o3 })
showRef("C-C-C", { val p = new s.o1 ; val q = new p.o2 ; new q.o3 })

Nesting  Outer       Simple  Full
-------  ----------  ------  ----------
O-O-O    s.o1$$o2$   o3$     s.o1$$o2$$o3$
O-O-C    s.o1$$o2$   o3      s.o1$$o2$$o3
O-C-O    s.o1$$o2    o3$     s.o1$$o2$o3$
O-C-C    s.o1$$o2    o3      s.o1$$o2$o3
C-O-O    s.o1$o2$    o3$     s.o1$o2$$o3$
C-O-C    s.o1$o2$    o3      s.o1$o2$$o3
C-C-O    s.o1$o2     o3$     s.o1$o2$o3$
C-C-C    s.o1$o2     o3      s.o1$o2$o3

O-O-O    s.o1$$o2$   o3$     s.o1$$o2$$o3$
O-O-C    s.o1$$o2$   o3      s.o1$$o2$$o3
O-C-O    s.o1$$o2    o3$     s.o1$$o2$o3$
O-C-C    s.o1$$o2    o3      s.o1$$o2$o3
C-O-O    s.o1$o2$    o3$     s.o1$o2$$o3$
C-O-C    s.o1$o2$    o3      s.o1$o2$$o3
C-C-O    s.o1$o2     o3$     s.o1$o2$o3$
C-C-C    s.o1$o2     o3      s.o1$o2$o3

scabug commented 9 years ago

@paulp said: Oh, and in case it's not clear, that's not the "artist's conception" or anything like that. Those names are obtained from getEnclosingClass and getSimpleName.

+  def show[T: ClassTag](nesting: String) = showClass(nesting, classTag[T].runtimeClass)
+  def showRef(nesting: String, x: AnyRef) = showClass(nesting, x.getClass)
+  def showClass(nesting: String, c: Class[_]) {
+    print(nesting, c.getEnclosingClass.getName, c.getSimpleName, c.getName)
+  }

scabug commented 9 years ago

@adriaanm said: Paul, I did look at your patch (https://github.com/adriaanm/scala/compare/scala:2.11.x...t2034), but I agree with Iulian's analysis: the fix is to make sure the inner class metadata matches the flattened names. Your patch does not fix that as far as I can tell.

scabug commented 9 years ago

@paulp said: How far is that?

Maybe you could pinpoint which it is that you think is wrong among these names. Or define what is "inner class metadata" if it's something other than getEnclosingClass.getName + getSimpleName, which is what these names are built from.

Nesting  Outer       Simple  Full
-------  ----------  ------  ----------
O-O-O    s.o1$$o2$   o3$     s.o1$$o2$$o3$
O-O-C    s.o1$$o2$   o3      s.o1$$o2$$o3
O-C-O    s.o1$$o2    o3$     s.o1$$o2$o3$
O-C-C    s.o1$$o2    o3      s.o1$$o2$o3
C-O-O    s.o1$o2$    o3$     s.o1$o2$$o3$
C-O-C    s.o1$o2$    o3      s.o1$o2$$o3
C-C-O    s.o1$o2     o3$     s.o1$o2$o3$
C-C-C    s.o1$o2     o3      s.o1$o2$o3

O-O-O    s.o1$$o2$   o3$     s.o1$$o2$$o3$
O-O-C    s.o1$$o2$   o3      s.o1$$o2$$o3
O-C-O    s.o1$$o2    o3$     s.o1$$o2$o3$
O-C-C    s.o1$$o2    o3      s.o1$$o2$o3
C-O-O    s.o1$o2$    o3$     s.o1$o2$$o3$
C-O-C    s.o1$o2$    o3      s.o1$o2$$o3
C-C-O    s.o1$o2     o3$     s.o1$o2$o3$
C-C-C    s.o1$o2     o3      s.o1$o2$o3

scabug commented 9 years ago

@paulp said: I am not sure how to explain this any more than it is already explained, but I can offer some concrete measures for "me" vs. "status quo".

Me: no exceptions from java reflection. Status quo: lots of exceptions from java reflection.

For this code sample:

package s {
  object o1 {
    object o2 { object o3; class o3 }
    class o2  { object o3; class o3 }
  }
  class o1 {
    object o2 { object o3; class o3 }
    class o2  { object o3; class o3 }
  }
}

Me: It compiles and creates the appropriate classfiles for eight distinct entities, each of which has correct and consistent names. Status quo: It does this.

scalac ./a.scala
./a.scala:4: error: name clash: class o2 defines object o3
and its companion object o2 also defines class o3
    class o2  { object o3; class o3 }
                       ^
./a.scala:4: error: name clash: class o2 defines class o3
and its companion object o2 also defines class o3
    class o2  { object o3; class o3 }
                                 ^
./a.scala:7: error: name clash: class o1 defines object o2
and its companion object o1 also defines class o2
    object o2 { object o3; class o3 }
           ^
./a.scala:8: error: name clash: class o1 defines class o2
and its companion object o1 also defines class o2
    class o2  { object o3; class o3 }
          ^
./a.scala:8: error: name clash: class o2 defines object o3
and its companion object o2 also defines class o3
    class o2  { object o3; class o3 }
                       ^
./a.scala:8: error: name clash: class o2 defines class o3
and its companion object o2 also defines class o3
    class o2  { object o3; class o3 }
                                 ^
6 errors found

It seems like status quo should be armed with more than some nebulous and as-yet unfalsifiable analysis.

scabug commented 9 years ago

Sarah Gerweck (gerweck) said: Paul, the issue is that Java requires classes to define their inner classes in metadata. See for example §4.7.6 of the Java 7 VM spec. If these names don't match with the actual names of classes, utilities that try to use reflection or detection to find inner classes may start to fail.

scabug commented 9 years ago

@paulp said: "Start" to fail?

scabug commented 9 years ago

@paulp said: I can see from trying to run current tests that none of you will have nearly the level of interest necessary to make this work, because the bootstrap process is full of hidden binary dependencies and every single one of them breaks when you change binary names. So forget it.

It seems like neither of you observed the addition and usage of rawOwnerChain. You should know that it's demonstrably false that this bug can be fixed by anything involving the metadata. All you have to do is read the source of java.lang.Class.

scabug commented 9 years ago

@paulp said: For some reason I've continued to nurse this patch. I attached a new one against current 2.12.x, d30098c30d. It still fails when it comes time to run the usual test suite because of the binary artifacts, but since my last comment a bunch of junit tests have been added and here's where junit really shines: it doesn't have to break the way anything scala will. So all the junit tests pass, and that's as much as I know.

Now I'm deleting the branch.

scabug commented 9 years ago

@paulp said: Simon elucidates what I meant by hidden binary dependencies here: https://groups.google.com/forum/#!msg/scala-internals/CUftD2E0_UM/ZqczBE71JbMJ

scabug commented 9 years ago

@som-snytt said: http://bugs.java.com/bugdatabase/view_bug.do?bug_id=8057919

scabug commented 9 years ago

@paulp said: It sounds like they may someday falsify my statement that this cannot be fixed by changing the metadata, so I'll add that it refers to Java versions 8 and prior.

scabug commented 8 years ago

@paulp said: In scala-internals thread https://groups.google.com/d/msg/scala-internals/CUftD2E0_UM/3rxKGowO0AIJ adriaan says

Bootstrapping binary incompatible versions of partest and scala should work with the bootstrap script.

I realize that either everyone has me muted or nobody thinks I have a clue, but this grossly underestimates the difficulty of this task. Simon is not up against a few binary incompatibilities. He is up against the binary compatibility apocalypse.

scabug commented 8 years ago

@adriaanm said: Not to worry, you're coming through loud and clear. I have seen no evidence to disprove my hypothesis that it's possible to bootstrap in all the ways that we could before modularizing, although it does involve more publish-locals, version overrides and repo-switching. By no means would I imply this is an easy task, though.

scabug commented 8 years ago

@adriaanm said: As far as that thread goes, my understanding is that he managed to bootstrap, but ran into some (other?) problem down the line. It's not clear to me what that problem is yet, though.

scabug commented 8 years ago

@paulp said: The problem is going to be logic which is (intentionally or otherwise) influenced by the precise configuration of dollar signs in generated names. All you have to do is scrutinize every usage of Name or String in the repository.

scabug commented 8 years ago

@adriaanm said: Yes, that's going to be hard, but not directly related to how we bootstrap, unless I'm missing something.

scabug commented 8 years ago

@paulp said: Oh, maybe you are defining things differently. To me "bootstrapping" is an actually working scala on the other side of whatever invasive change requires a reference to bootstrapping. I wasn't trying to say that your bootstrapping infrastructure is necessarily the issue - although whether or not it is, the general difficulty identifying the complete set of inputs makes this sort of effort that much harder.

scabug commented 8 years ago

@adriaanm said: Yes, I was thinking more modestly of just the mechanics of bootstrapping. Agreed, changing name mangling is an epic undertaking.

scabug commented 7 years ago

@retronym said: JDK9 has fixed this: https://bugs.openjdk.java.net/browse/JDK-8057919

scabug commented 7 years ago

@paulp said: Fixed in java 9, as I hypothesized in may 2015, which crowns #2034 as the dumbest misallocation of my time among the many strong competitors for that title. Long may it reign.

scabug commented 7 years ago

@som-snytt said: It was resolved in 2015 already; I don't remember posting the link above, but I really don't remember the comment on the dead backport issue, that it's "annoying but not critical" because it's just "pretty-printing."

scabug commented 7 years ago

@paulp said: To keep the timeline straight, my comment was in May 2015 and the events which could be described as resolving anything on the java side were in June 2015.

dwijnand commented 6 years ago

@retronym said: JDK9 has fixed this: https://bugs.openjdk.java.net/browse/JDK-8057919

~~It would appear that's not true:~~

After https://bugs.openjdk.java.net/browse/JDK-8057919, it also "fails" in OpenJDK9.

(from: https://github.com/scala/scala-dev/issues/415#issuecomment-322935565)

[edit: I misunderstood]

xuwei-k commented 6 years ago

$ java -version
java version "9.0.4"
Java(TM) SE Runtime Environment (build 9.0.4+11)
Java HotSpot(TM) 64-Bit Server VM (build 9.0.4+11, mixed mode)

$ scala
Welcome to Scala 2.12.4 (Java HotSpot(TM) 64-Bit Server VM, Java 9.0.4).
Type in expressions for evaluation. Or try :help.

scala> object A { object B { class C } }
defined object A

scala> classOf[A.B.C].getSimpleName
res0: String = C

$ java -version
java version "1.8.0_162"
Java(TM) SE Runtime Environment (build 1.8.0_162-b12)
Java HotSpot(TM) 64-Bit Server VM (build 25.162-b12, mixed mode)

$ scala
Welcome to Scala 2.12.4 (Java HotSpot(TM) 64-Bit Server VM, Java 1.8.0_162).
Type in expressions for evaluation. Or try :help.

scala> object A { object B { class C } }
defined object A

scala> classOf[A.B.C].getSimpleName
java.lang.InternalError: Malformed class name
  at java.lang.Class.getSimpleName(Class.java:1330)
  ... 28 elided

jroper commented 6 years ago

Just to point out that the "this isn't critical because it's just pretty printing" argument is false, the implementation of Class.isAnonymousClass invokes Class.getSimpleName:

    public boolean isAnonymousClass() {
        return "".equals(getSimpleName());
    }

I guess it's fixed in JDK9, but I don't see a rush of people upgrading.

som-snytt commented 4 years ago

Closing this because there is Android and there is "oh is jdk 13 available already? I mean 14?".

Perhaps closing will dissuade "pushed a commit that referenced this issue."

This issue is no longer "open to Scala" in the sense of actionable under the aegis of this project or "the Scala team", aka "team Scala", aka "team Adriaan", aka "team Flash".

scala / bug

getClass fails for doubly nested inner-class #2034