Community
Participate
Working Groups
Build Identifier: I20100129-1300 This is raised to handle case (3) of bug 310427 String newText = String.format(newText, null) If newText is being declared as a field, then it is proposed as an argument to String.format, which is wrong because its declaration is still not complete. Reproducible: Always Steps to Reproduce: 1.Declare the field newText in any class as shown above 2.Invoke Content assist to complete the method String.format. Select the method format(String, Object...) and note the proposals to fill the first argument of this method. newText is proposed which is wrong
Created attachment 168330 [details] proposed fix v1.0 + regression tests The fix for this one follows from the fix for bug 236306. For the field inside whose initializer content assist is being provoked will be removed from the list of visible fields, because in the real sense that field is still in the process of declaration. Also, as Frederic rightly pointed out in bug 236306#comment 6 - "first to do the verification only if the position of the completion is inside the declaration positions (i.e. between the declarationSourceStart and the declarationSourceEnd) if they are well set. Second, in case the declaration positions are not set (i.e. declarationSourceEnd <= 0), typically when the recovery wasn't able to close it, then visit the entire AST nodes tree from the initialization to search whether a completion node belongs to it or not", I observed that for fields, we only do the first check ie. checking the source positions, but in cases where the sourceEnd is not set, this check fails. For this we need the CompletionNodeDetector to find if the completion node is inside the field initialization. The changes in InternalExtendedCompletionContext.java:191 to 200 pertain to this. Modified existing test CompletionContextTests#test0145() Added tests CompletionContextTests#test0174() -> test 0176().
Created attachment 168752 [details] Another proposal to fix the issue (In reply to comment #1) > Created an attachment (id=168330) [details] > proposed fix v1.0 + regression tests > One thing worried me in this patch: the loop break was done even if the completion was not done in the initializer. Hence I tried the following test case: public class X { String text1 = String.format, text2, text3 = String.format; } When completing after the second String.format, I did expect to show that the break was wrongly done on the first field declaration, but surprisingly, it still worked...! Investigating why I observed that behavior, I saw that the 'initialization' field of the FieldDeclaration was null for 'text1'. Investigating further, I realized that the 'initialization' field was reset in consumeExitVariableWithInitialization() method of CompletionParser when the completion is not located in the initialization... Hence, the new proposed patch which passes all the completion tests and takes advantage of the fact that the 'initialization' field can only be not null when the completion is located inside it... Maybe I missed some points, but it does sound simpler for me and avoid to make additional tests to know whether the completion is located in the initializion or not. Note that it would also make the fix for bug 236306 simpler!
(In reply to comment #2) > Created an attachment (id=168752) [details] > Another proposal to fix the issue > Hence, the new proposed patch which passes all the completion tests and takes > advantage of the fact that the 'initialization' field can only be not null when > the completion is located inside it... This is an interesting observation. And this fix indeed looks better. However, I'm not too confident about this fix because of the following: The initialization field being null relies solely on consumeExitVar... method of CompletionParser. But sometimes when the parser goes into error recovery mode, depending on the compilation unit, the parser may/may not be able to correctly reduce to a rule. The good news is that I tried a few cases in which error recovery would be triggered, and I found that this particular rule was correctly reduced to. It may possible, though that there's a case in which it might not happen, which could then spell trouble. Even in the current implementation for both fields and locals, we were trying to check the source start and source end in addition to checking whether initialization is null or not. Maybe this was an intentional conservative approach?! I do agree though that this fix is better and avoids too much checking. I'd vouch for it over mine.
Frederic, I think as a possible solution, we can release your fix for now, with the unneeded lines commented instead of deleted, and additional comments pointing to this bug incase a corner case comes up anytime. This would make it easier to fix any case where the 'fact that the 'initialization' field can only be not null when the completion is located inside it' doesnt hold true. What do you think?
(In reply to comment #4) > Frederic, I think as a possible solution, we can release your fix for now, with > the unneeded lines commented instead of deleted, and additional comments > pointing to this bug incase a corner case comes up anytime. This would make it > easier to fix any case where the 'fact that the 'initialization' field can only > be not null when the completion is located inside it' doesnt hold true. > > What do you think? Yep, I think this would be the safest approach. So, it would be ok for me to release it using it :-)
Created attachment 178587 [details] committed patch
Released in HEAD for 3.7M2
I tried: public class X { public static void main(String[] args) { String newText = String.format(new|CODE ASSIST } } And I still get newText as a proposal. Reopening.
The following test shows that the unclosed field declaration is proposed: //https://bugs.eclipse.org/bugs/show_bug.cgi?id=312603 public void test312603() throws JavaModelException { this.workingCopies = new ICompilationUnit[1]; this.workingCopies[0] = getWorkingCopy( "/Completion/src3/test/X.java", "package test;\n" + "public class X {\n" + " void foo(String s) {}\n" + " String myString = \"\";\n" + " String myString2 = foo(my\n" + "}"); CompletionTestsRequestor2 requestor = new CompletionTestsRequestor2(true); String str = this.workingCopies[0].getSource(); final String completeBehind = "String myString2 = foo(my"; int cursorLocation = str.lastIndexOf(completeBehind) + completeBehind.length(); this.workingCopies[0].codeComplete(cursorLocation, requestor, this.wcOwner); assertResults( "MyClass[TYPE_REF]{mypackage.MyClass, mypackage, Lmypackage.MyClass;, null, null, 14}\n" + "mypackage[PACKAGE_REF]{mypackage, mypackage, null, null, null, 24}\n" + "myString[FIELD_REF]{myString, Ltest.X;, Ljava.lang.String;, myString, null, 57}", requestor.getResults()); } This can be added to the test suite org.eclipse.jdt.core.tests.model.CompletionTests.
(In reply to comment #8) > I tried: > > public class X { > > public static void main(String[] args) { > String newText = String.format(new|CODE ASSIST > > } > > } > > And I still get newText as a proposal. > Reopening. This case is slightly different than the one reported for the bug. Actually, in the original case of comment 0, we're asking for the proposal of a method, and as a byproduct of that we propose arguments. So the problem was in InternalExtendedCompletionContext while suggesting arguments AFTER code assist has already proposed a method and the user has chosen one. The problem in this case occurs when the user explicitly asks proposals of arguments. This is handled by the CompletionEngine, so the same fix just needs to be extended for the findFields(..) method in CompletionEngine. Working on a patch.
I noticed another problem here public class Test { String field1 = "hello"; String field2 = String.forma|<CTRL-SPACE here> String field3 = ""; } This proposes field3 in the argument proposals for String.format. Clearly, the above fix removed the field being declared but doesnt remove the fields declared AFTER it. This problem is more valid to combine with this bug. For the problem reported in comment 8, I'll open a new bug and fix it there, along with another related issue i've stumbled upon, since they're related to CompletionEngine.
Created attachment 179038 [details] fix for comment 11 + test This fix addresses the issue in comment 11 by removing the fields declared after the field being declared from the list of proposals. (In the previous fix, we had just removed the currently declared field).
(In reply to comment #11) [..] > For the problem reported in comment 8, I'll open a new bug and fix it there, > along with another related issue i've stumbled upon, since they're related to > CompletionEngine. Opened bug 325481.
Olivier, can you please review? Thanks.
Created attachment 179380 [details] Proposed fix + regression tests Same patch with one more regression test (the test from comment 9).
+1
Released in HEAD for 3.7M3
Verified for 3.7M3 using I20101025-1602 (4.1 build)
