Community
Participate
Working Groups
3.2 - create a project 'p' - add the attached JAR to the build path the JAR has been created from the following source: public class A { public static class Inner1 { public static class Inner2 { public static class Inner3 { } } } } - create new type B: package pack; public class B { Inner3 i; } - set a breakpoint in TypeNameRequestorWrapper.acceptType - do a organize import on B breakpoint is reached with the following values: packageName char[4] 'pack' simpleTypeName char[6] 'Inner3' enclosingTypeNames char[1][] 'A$Inner1$Inner2' should be: enclosingTypeNames char[3][] 'A', 'Inner1', 'Inner2'
Created attachment 45092 [details] JAR containing A.Inner1.Inner2.Inner3
This comes from BinaryIndexer which stores informations provided by ClassFileReader. Enclosing type name '$' are not replaced by '.' as it cannot know if they are types separators or characters of the names. For example, with following class: X$Y.java: public class X$Y { public static class A$B { public static class C { } } } Enclosing type of C is X$Y$A$B and not all '$' characters have to be replaced by '.'... There's no real good solution actually. One point is that, in your test case (comment 0) if you complete after Inner3, then you get correct proposal (ie. A.Inner1.Inner2.Inner3). This is due to the fact that CompletionEngine currently replaces systematically all '$' with '.'. This is correct for your comment 0 test case, but not when classes have '$' character in their name... In my example above, it would propose X.Y.A.B.C which was not a valid type name! In fact, we have 2 possibilities here: 1) Try to fix the problem in BinaryIndexer 2) Do not change indexes. Then all searchAllTypeNames callers (CompletionEngine, Organize imports, etc...) have to fix it. CompletionEngine already does it and so, current bug will belong to JDT/UI to fix it in TypeNameRequestor implementor (ie. TypeInfoRequestor.acceptType(...)) Of course, it seems natural to answer 1) as all callers will get benefit of the change. However, do not forget that there's no real complete solution as there's no enough information to distinguish '$' as separator or as name character. So, whatever the fix is, it will be false in some corner cases. That's why, I'm a little bit relunctant to apply a workaround on binary indexer which makes us lose all information about '$' separators and would prefer local fixes managed by callers themselves...
Created attachment 45462 [details] Proposed patch This fix makes following test case working: X$Y$Z.java: public class X$Y$Z { public static class Inner {} } Although it will fail with simple workaround to replace all '$' with '.' Unfortunately all other corner cases (typically comment 2 one) will still fail... Note that it has a real minor impact on indexing performances (<0.5%)...
Postpone decision to release the fix or not. If there's a strong demand for this behavior, then we'll reconsider our point of view... Set as REMIND accordingly to initial bug 148195.
bug 148195 is only set to REMIND as we're waiting for this bug to be fixed. I would definitly welcome a fix here; bugs in organize import are nasty as the operation is executed very often and a bug forces you for manual correction. I think '$' in names are much more rare than the usage of inner classes, so if no 100% correct solution is possible, I would support your patch. Can maybe the indexer use a different separator? Or does the indexer have the same problem of not really knowing where the simple name ends?
Binary indexder has the same problem: it cannot know when '$' is an enclosing type name separator or a part of a (enclosing) type name... Completion Engine already makes the assumption that '$' are always enclosing types name separator, why Organize Imports code cannot make the same? As I said in comment 2, I'm reluctant to apply a solution which will be wrong in some corner cases. BinaryIndexer store class files information and should not modify them if it is not 100% sure on the way how to do it... I reopen the bug, but will talk about it with Philippe if we apply the patch or close the bug as WONTFIX
Created attachment 51637 [details] screenshot Nowhere in our code we replace '$' by '.' an I don't think it's a good idea to start. The model should try to provide the best available in a consistent way. When getting the Java element for Inner3, things look correct, see screeshot. How did they do it? Can the search engine use the same algorithm to be consistent here?
It seems weird Sun didn't spot this potential problem earlier. Inner classes are compiled with a '$' character to mark the inner relationship AND it's possible to have a '$' character in the filename. One of both must be forbidden then ? :-)
Martin, I cannot reproduce this bug using last I-build. It seems that the OrganizeImport operation now uses SearchEngine.searchAllTypeNames with TypeNameMatchRequestor instead of TypeNameRequestor when this bug was opened. As the TypeNameMatchRequestorWrapper retrieves the IType, the name is now correctly computed and the inserted import declaration looks OK. Could you confirm this and then we can close both this bug and bug 148195? Thanks
We wrapped all invocations TypeNameMatch#getFullyQualifiedName() though a static utility that fixes the JDT core bug our side. (JavaModelUtil.getFullyQualifiedName(TypeNameMatch)) When you fix this bug we can remove the workaround again and directly use TypeNameMatch#getFullyQualifiedName()
(In reply to comment #10) > We wrapped all invocations TypeNameMatch#getFullyQualifiedName() though a > static utility that fixes the JDT core bug our side. > (JavaModelUtil.getFullyQualifiedName(TypeNameMatch)) > > When you fix this bug we can remove the workaround again and directly use > TypeNameMatch#getFullyQualifiedName() > This problem is described by the bug 182179, not by this one. I verified that: 1) applying fix for bug 156168 (as bug 182179 is in fact a duplicate of that bug) 2) removing JDT/UI workaround in JavaModelUtil: public static String getFullyQualifiedName(TypeNameMatch typeNameMatch) { // return internalGetQualifiedName(typeNameMatch.getType()); return typeNameMatch.getType().getFullyQualifiedName('.'); } the problem is fixed :-) So, set this bug as a duplicate of bug 182179 *** This bug has been marked as a duplicate of bug 182179 ***
Verified for 3.4M6 using build I20080324-1300
