Community
Participate
Working Groups
Build ID: 3.5-M7 Steps To Reproduce: 1. Use the following code snippet: import java.util.Iterator; public class TestClass implements Iterable, It| { public Iterator iterator() { return null; } } 2. At the '|' symbol do CTRL+Space 3. 'Iterable' is given as a choice for completion - choose it. 4. You will get an error: Type Duplicate interface Iterable for the type TestClass Shouldn't an already existing super interface be prevented from appearing in the completion choices?
Created attachment 138136 [details] Code plugin patch - v-0.5 Patch for CompletionEngine in plug-in org.eclipse.jdt.core.
Created attachment 138137 [details] Tests plugin patch - v-0.5 Patch for CompletionTests in org.eclipse.jdt.core.tests.model.
Attached patches contain the code fix and a couple of JUnits to verify the scenarios. The code fix involves, marking the existing super interfaces as forbidden in computeForbiddenBindings() and then making use of that info in two different palces - findTypesAndSubpackages() and acceptTypes(char[],char[]...). The fix in findTypesAndSubpackages() is a subset of the changes in the same area as part of the fix for bug 270436 (and so if that change is taken then this one-line change could be ignored) and it is required in cases where interfaces and class belong to the same package within the same compilation unit. The fix in acceptTypes() is required when the interfaces and class are in different compilation units. The two JUnit tests added to CompletionEngine verifies both these scenarios. I am not fully satisfied/happy about the fix in acceptTypes() (via the new added routine isFobiddenType(char[],char[]) as it is more on name based and not Binding based. Yet, thought of posting it anyway. Note that the fixes in findTypesAndSupackages() and acceptTypes() are independent and each fixes a particular use-case as verified by the two newly added JUnit tests.
I looked your patch and i think this is the right way to fix the problem but i found several issue. In acceptType() we only have access to type name because we indirectly use the content of the java search indexes and these indexes only contain names. It is possible to compute a binding from a type name but it is too costly so add isForbiddenType(char[], char[]) is the right approach. But in your patch isForbiddenType() take into account only top level types and you should also take into account member types. To take into account member types you could use the fully qualified type names as computed in acceptTypes(). Another point is that in computeForbiddenBindings() you added the interfaces references after the completion node in the list of forbidden types but code assist is not supposed to take into account elements after the completion node which are not a declaration. After the completion point the code is not complete because we are going to complete it, and it is hard to guess what is correct code and what the user want to write. eg. In this case, it is not good to filter Interface3: class X implements Interface1, Interface|, // do ctrl+space at | Interface3 var; } interface Interface1 {} interface Interface2 {} interface Interface3 {} In fact in your patch the filtering of interfaces after the completion node do not work because the references after the completion node are not resolved (this is the expected behavior because when the completion node is resolved an exception is thrown and stop the resolution).
Thank you David for reviewing the patch. (In reply to comment #4) > I looked your patch and i think this is the right way to fix the problem Glad that you have confirmed by strategy. Yes, I figured that computing a binding from type name is equally expensive, but wasn't absolutely sure. > But in your patch isForbiddenType() take into account only top level types and > you should also take into account member types. > To take into account member types you could use the fully qualified type names > as computed in acceptTypes(). This is already taken care of in computeForbiddenBindings() via the patch to bug 270437. (The order in which I fixed them: bug 270437, bug 270436, and this one). > Another point is that in computeForbiddenBindings() you added the interfaces > references after the completion node in the list of forbidden types but code > assist is not supposed to take into account elements after the completion node > which are not a declaration. Good point. I have now changed that piece of code to proceed only up to the completion node. I shall update another code patch once I am done with the tests. > In fact in your patch the filtering of interfaces after the completion node do > not work because the references after the completion node are not resolved > (this is the expected behavior because when the completion node is resolved an > exception is thrown and stop the resolution). Yes, I noticed that when I tried an example of inserting an interface between two existing interfaces. Thanks for enlightening me on this.
Created attachment 138631 [details] Code plugin patch - v-0.6 Updated the code in CompletionEngine#computeFobiddenBindings() to loop only up to the AST node as suggested by David. That's the only change. Note that tests plugin patch remains the same.
Bug 270437 is related to member types of the type which contain the completion, but isForbiddenType() do not work for member of types in other compilation units. eg. // p/Enclosing.java package p; public class Enclosing { public interface Interface1 {} public interface Interface2 {} } // X.java public class X implements p.Enclosing.Interface1, Interf| {} With your patch Interface1 is still proposed and should not. eg. // p/Enclosing.java package p; public class Enclosing { public interface Interface1 {} public interface Interface2 {} } // p/Interface1.java package p; public class Interface1{} // X.java public class X implements p.Interface1, Interf| {} In this other example p.Interface1 and p.Enclosing.Interface1 are filtered. Only p.Interface1 should be filtered.
qualifiedSourceName() should be used instead of shortReadableName().
(In reply to comment #7) > Bug 270437 is related to member types of the type which contain the completion, > but isForbiddenType() do not work for member of types in other compilation > units. Yes, good catch! Thanks for the examples - I have incorporated them as test cases (in v-0.7 of tests patch). Both the problems you have noted here are fixed in the newer patch (v-0.7). The problem was that the earlier change wasn't paying attention to the "Enclosing Types" info. Once that was incorporated into the strategy, it worked for these cases too. (In reply to comment #8) > qualifiedSourceName() should be used instead of shortReadableName(). > Yes, I have made that change too, in the newer patch (v-0.7).
Created attachment 142634 [details] Code plugin patch - v-0.7 Patch for CompletionEngine.java of plug-in org.eclipse.jdt.core. This fixes the issues David pointed out in Comment #7, #8. It is based on latest of HEAD as of an hour ago.
Created attachment 142636 [details] Tests plugin patch - v-0.7 Patch for CompletionTests.java in plug-in org.eclipse.jdt.core.tests.model. This includes two additional test cases (4 in total) that David suggested in Comment #7. This is based on the latest version of HEAD as of an hour ago.
The last patch works fine. But in isForbiddenType() the code which checks the presence of '<' is useless because typeBinding.qualifiedSourceName() returns only a qualified name and no type parameter.
JDT/UI and JDT/Text tests pass alright. If comment #12 is addressed and a fresh patch is posted after testing, we can resolve this.
Created attachment 142862 [details] Code plugin patch - v-0.8 Patch to CompletionEngine.java of plug-in org.eclipse.jdt.core by removing the extraneous check in isForbiddenType() as suggested in comment 12.
Thank you Srikanth and David for looking over the patch and verifying it.
Released Pradeep's fix in HEAD for 3.6M1
Verified for 3.6M1 using build 20090802-2000.
