Summary: | Missing InnerClasses attribute for nested interfaces created by AspectJ | ||||||
---|---|---|---|---|---|---|---|
Product: | [Tools] AspectJ | Reporter: | Mark Fulton <mfulton26> | ||||
Component: | Compiler | Assignee: | aspectj inbox <aspectj-inbox> | ||||
Status: | RESOLVED FIXED | QA Contact: | |||||
Severity: | critical | ||||||
Priority: | P3 | CC: | aclement, gudnabrsam | ||||
Version: | 1.8.4 | ||||||
Target Milestone: | 1.8.10 | ||||||
Hardware: | PC | ||||||
OS: | Windows 7 | ||||||
Whiteboard: | |||||||
Attachments: |
|
Description
Mark Fulton
2016-05-12 11:47:08 EDT
Actually, as far as I can tell, the InnerClasses attribute of the $ajcMightHaveAspect class also looks incorrect. Using ASM's ASMifier, I get: cw.visitInnerClass("example/aspect/FooAspect$ajcMightHaveAspect", null, "FooAspect", ACC_PUBLIC + ACC_ABSTRACT + ACC_INTERFACE); This appears to define inner class example.aspect.FooAspect$ajcMightHaveAspect with NO outer class (null), and an inner alias of "FooAspect". Contrast with the ASMifier results on java.util.Map$Entry: cw.visitInnerClass("java/util/Map$Entry", "java/util/Map", "Entry", ACC_PUBLIC + ACC_STATIC + ACC_ABSTRACT + ACC_INTERFACE); If FooAspect$ajcMightHaveAspect interface is indeed supposed to be an inner class, I would expect its definition to be: cw.visitInnerClass("example/aspect/FooAspect$ajcMightHaveAspect", "example/aspect/FooAspect", "ajcMightHaveAspect", ACC_PUBLIC + ACC_STATIC + ACC_ABSTRACT + ACC_INTERFACE); (nested interfaces are always static) Further, the FooAspect classfile should include the same inner class definition as the interface. OTOH, perhaps there is no reason for *$ajcMightHaveAspect to actually be an inner class (since it's basically not, apparently; note that if you call #getDeclaredClasses() from the java.util.Class instance representing this type you receive an empty array), in which case its InnerClasses info could be dropped entirely, and it would be up to other libraries to make sure they can process top-level classes that happen to contain a $ in their name. OR, is there some deliberate (hopefully documented) technical reason (hack?) for the weird bytecode limbo in which $ajcMightHaveAspect interfaces seem to live? Thanks for the clear bug report, having a dig into it now. I don't think that handling around ajcMightHaveAspect has been properly looked at it many many (many) years and I don't recall any testcases explicitly verifying the structure. I think I'll go with what Matt was thinking, that thing doesn't need to be an inner class. Right now having browsed the old code it looks like some attempts were made long ago to make it a kind of anonymous member type, but it was half baked. The enclosing type not updated properly and the attribute in the anonymous class not finished properly either. If using annotation style as opposed to code style, there is also another code path that can generate the interface too and that code path does not make it an inner class. So I'm inclined to remove the attribute from the generated interface. However, at the same time I don't really want to change the name, there is code out there (nasty, evil code) relying on the name. I'm not even sure I know all the places where that might be relied on - I just know the few I can see. It does indeed mean that anyone assuming $ means inner could be affected. Regression tests still all pass with it removed. As far as I can tell, outer$inner is a convention and doesn't seem to be explicitly dictated in the JLS. I can attest that it is perfectly legal to create classes with $ and sequences of that character and have them function at runtime. So I think if you resolve the inner/nested (the JLS actually implies that "inner" classes are non-static, stating that interfaces, being explicitly static, are not inner classes per se), you can safely keep *$ajcMightHaveAspect and push supporting top-level (really all) types back to other libraries with compatibility issues... which, had they already done so, would probably have meant this issue report would never have been filed. Still, it's probably for the best to clean up the generated bytecode. several comments above I said java.util.Class; of course I meant java.lang.Class :P Changes are in. No longer a (half baked) inner class. If someone needs a workaround for this ahead of the next release, you should be able to translate your aspect to annotation style - that will exercise the alternate route that produces an ajcMightHaveAspect without the InnerClasses tag. Dev build here for anyone who wants to kick it around: http://www.eclipse.org/downloads/download.php?file=/tools/aspectj/dev/aspectj-DEVELOPMENT-20160512153500.jar |