Parse classfile to extract inner class info#516
Conversation
Fixes sbt/sbt#117 Inside `ClassToAPI`, Java reflection is used to calculate the inner classes. Under certain circumstances this fails to load some of the classes and result to a `NoClassDefFoundError`. I've switched to parse the classfile manually, and wrapped `cl.loadClass` with a try-catch. I was able to confirm the fix using https://github.com/duboisf/sbt_issue_117_example
eed3si9n
force-pushed
the
wip/innerclass
branch
from
1b7a1c7 to
e15c88b
Compare
|
The validator has checked the following projects against Scala 2.12,
✅ The result is: SUCCESS |
|
noting for the record before I smash the restart button.. failure in 2.11.11:
failures in 2.12.3:
|
|
Need some time to have a look at this. Will try to review tomorrow. |
|
I have a few questions:
|
| val classes = merge[Class[_]](c, | ||
| c.getDeclaredClasses, | ||
| c.getClasses, | ||
| innerClasses.toArray[Class[_]], |
There was a problem hiding this comment.
In all the examples I've seen, this weird bug seems to happen only in getClasses. Why are we re-implementing getDeclaredClasses too?
getDeclaredClasses()| Returns an array ofClassobjects reflecting all the classes and interfaces declared as members of the class represented by this Class object.
getClasses()| Returns an array containingClassobjects representing all the public classes and interfaces that are members of the class represented by this Class object.
These are more or less the same thing except one is public. I figured I would do a double duty here to save a reflection call.
There was a problem hiding this comment.
Is your implementation also reflecting all interface classes or only inner classes? e.g. maybe you can add a test proving that interfaces defined inside classes are detected by the new implementation.
There was a problem hiding this comment.
It's specified in the JVM spec, so I think we are good here. https://docs.oracle.com/javase/specs/jvms/se10/html/jvms-4.html#jvms-4.7.6
Every CONSTANT_Class_info entry in the constant_pool table which represents a class or interface C that is not a package member must have exactly one corresponding entry in the classes array.
Bold added by me.
| } | ||
| } | ||
| } catch { | ||
| case _: ClassNotFoundException => // do nothing |
There was a problem hiding this comment.
- Do we have any idea what we're after here? Why is this exception being thrown? Are we now not throwing this exception because we're potentially doing less work, which could affect the incremental compiler for java sources?
The case that we have repro project (https://github.com/duboisf/sbt_issue_117_example) is a simple case of java.lang.NoClassDefFoundError: org/jboss/virtual/VirtualFileVisitor while loading some class. Why is this happening? Look no further than spring-core 3.0.0.RELEASE's POM file:
<dependency>
<groupId>org.jboss.vfs</groupId>
<artifactId>com.springsource.org.jboss.virtual</artifactId>
<version>2.1.0.GA</version>
<scope>compile</scope>
<optional>true</optional>
</dependency>Maven allows optional dependency, and Spring optionally depends on JBoss VFS. We can find that some of the inner classes in Spring Core exposes org.jboss.virtual.VirtualFileVisitor as parent class. Based on the release date, it's likely this code here: https://github.com/spring-projects/spring-framework/blob/784068346df62225661d9ce1bbd1a32a1fddde66/org.springframework.core/src/main/java/org/springframework/core/io/support/PathMatchingResourcePatternResolver.java#L662:
import org.jboss.virtual.VirtualFileVisitor;
...
private static class PatternVirtualFileVisitor implements VirtualFileVisitor {Zinc is written to keep tracking parent classes, and doing so requires loading the class. I am doing the same, but I am just ignoring the ClassNotFoundException.
There was a problem hiding this comment.
- Have we tried this in different JVM versions?
No, but optional JAR missing from classpath is a problem that's independent from JVM versions. Since it's reported in 2011 and 2014, likely it covers Java 6, 7, and 8.
Not at this point since Zinc's scripted build.json doesn't have the feature to resolve library dependencies. |
jvican
left a comment
jvican
left a comment
There was a problem hiding this comment.
I have two more comments, thanks for the extensive reply describing the problem.
| val constructors = | ||
| mergeMap(c, c.getDeclaredConstructors, c.getConstructors, constructorToDef(enclPkg)) | ||
| val cl = c.getClassLoader | ||
| val name = c.getCanonicalName |
There was a problem hiding this comment.
Is the original implementation of getDeclaredClasses and getClasses method using getCanonicalName?
There was a problem hiding this comment.
I can't read the original implementation to avoid license violation. Here the name has to match what's stored in the classfile, and then later converted to dot-notation via getClassConstantName. In any case, I am now thinking c.getName might actually be the correct answer since at the classfile level they'll likely store foo/bar/A$B.
There was a problem hiding this comment.
Can you add a battery of tests to check that both implementations are behaviourally the same? I want to be as certain as possible that this doesn't cause regressions, as Java incremental compilation is not battle-tested in our scripted tests.
There was a problem hiding this comment.
I can't read the original implementation to avoid license violation.
My understanding was that this only applies if you're doing a a clean-room implementation of the Java specification, and even then it's not clear what license you'd be violating.
At any rate, OpenJDK is GPL (at least the parts related to java.lang.Class), for instance here or here.
| val classes = merge[Class[_]](c, | ||
| c.getDeclaredClasses, | ||
| c.getClasses, | ||
| innerClasses.toArray[Class[_]], |
There was a problem hiding this comment.
Is your implementation also reflecting all interface classes or only inner classes? e.g. maybe you can add a test proving that interfaces defined inside classes are detected by the new implementation.
|
@eed3si9n Are you interested in picking this up and adding the test battery to make sure that they are the same? |
|
I guess eventually, but it's not high on my work or hobby hour list. |
|
No rush @eed3si9n 😄 |
jvican
changed the title
|
closing for inactivity; I left a breadcrumb on the original issue, in case anyone wants to pick this up again. |
Fixes sbt/sbt#117 ClassToAPI used Java reflection (c.getClasses, c.getDeclaredClasses) to discover inner classes during post-compile analysis. When an inner class references types not on the classpath (e.g. optional dependencies like JBoss VFS in Spring), these reflection calls throw NoClassDefFoundError, crashing compilation. Instead, parse the classfile's InnerClasses attribute directly to discover inner classes, and gracefully skip any that can't be loaded via the classloader. This is a revival of sbt#516 by @eed3si9n, rebased onto the current codebase with tests added. Co-Authored-By: Eugene Yokota <[email protected]> Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Fixes sbt/sbt#117 ClassToAPI used Java reflection (c.getClasses, c.getDeclaredClasses) to discover inner classes during post-compile analysis. When an inner class references types not on the classpath (e.g. optional dependencies like JBoss VFS in Spring), these reflection calls throw NoClassDefFoundError, crashing compilation. Instead, parse the classfile's InnerClasses attribute directly to discover inner classes, and gracefully skip any that can't be loaded via the classloader. This is a revival of sbt#516 by @eed3si9n, rebased onto the current codebase with tests added. Co-Authored-By: Eugene Yokota <[email protected]> Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Fixes sbt/sbt#117 ClassToAPI used Java reflection (c.getClasses, c.getDeclaredClasses) to discover inner classes during post-compile analysis. When an inner class references types not on the classpath (e.g. optional dependencies like JBoss VFS in Spring), these reflection calls throw NoClassDefFoundError, crashing compilation. Instead, parse the classfile's InnerClasses attribute directly to discover inner classes, and log a warning for any that can't be loaded via the classloader. This is a revival of sbt#516 by @eed3si9n, rebased onto the current codebase with tests added. Co-Authored-By: Eugene Yokota <[email protected]> Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Fixes sbt/sbt#117 ClassToAPI used Java reflection (c.getClasses, c.getDeclaredClasses) to discover inner classes during post-compile analysis. When an inner class references types not on the classpath (e.g. optional dependencies like JBoss VFS in Spring), these reflection calls throw NoClassDefFoundError, crashing compilation. The fix tries reflection first (preserving exact existing semantics), and falls back to parsing the classfile's InnerClasses attribute when reflection fails. The fallback logs a warning and gracefully skips any inner classes that can't be loaded. This is a revival of sbt#516 by @eed3si9n, rebased onto the current codebase with tests added. Co-Authored-By: Eugene Yokota <[email protected]> Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
6 participants
Fixes sbt/sbt#117
I was browsing around old sbt issue, and still was able to reproduce sbt/sbt#117 using https://github.com/duboisf/sbt_issue_117_example, so I fixed it.
Inside
ClassToAPI, Java reflection is used to calculate the inner classes. Under certain circumstances this fails to load some of the classes and result to aNoClassDefFoundError.I've switched to parse the classfile manually, and wrapped
cl.loadClasswith a try-catch. I was able to confirm the fix using sbt_issue_117_example.