Community
Participate
Working Groups
Build ID: I20080523-0100 Steps To Reproduce: 1. type "String newText = String.format" 2. press 'Ctrl+Space' to get the default proposals 3. select the first proposal -> result is: "String newText = String.format(newText, null)" but 'newText' should not be a proposal. It is not initialized yet! More information:
I can reproduce it if the preference 'Java>Editor>Content Assist>Fill method arguments and show guessed arguments>Insert best guessed arguments' is enabled. In 3.3 'newText' is not proposed. In 3.3 JDT/Text use ICodeAssist#codeComplete() before the local variable declaration to guess arguments. So 'newText' is not proposed. In 3.4 JDT/Text use the new API CompletionContext#getVisibleElement() and 'newText' is visible from the completion location. So 'newText' is proposed To avoid that JDT/Text propose 'newText' as a possible argument, i see two possibilities: 1) CompletionContext#getVisibleElement() would not return an enclosing local variable as a visible element. 2) JDT/Text would filter the visible element 'newText'. To do that JDT/Text need to know that the completion occurs in a local variable initialization. That's currently not possible but CompletionContext#getEnclosingElement() could return an enclosing ILocalVariable instead of the enclosing IMethod. I think that 2) is the best way to solve the problem because 'newText' is really visible from the completion location. Daniel - What do you think about this bug ?
>Daniel - What do you think about this bug ? Agree to go with 2) but this a 3.5 item.
Not for 3.5
*** Bug 254756 has been marked as a duplicate of this bug. ***
Created attachment 162679 [details] proposed fix v1.0 + regression tests InternalExtendedCompletionContext.getEnclosingElement() relies upon CompilationUnit.getElementAt(int), which uses JavaElement.getSourceElementAt(int). This method only works on compilation units and types and narrows down to an enclosing element not beyond a method. The methods are know there because SourceTypeElementInfo$children contains the methods and fields defined in a type. On the other hand, to know what all local variables are present in the method, we will have to collect all local variables just like we already do for InternalExtendedCompletionContext.getVisibleElements(String) via InternalExtendedCompletionContext.searchVisibleVariablesAndMethods(..), and then check each local variable's declaration for the completion node. This duplication of code is useless. We can better do the filtering directly in InternalExtendedCompletionContext.searchVisibleVariablesAndMethods(..). As far as the visibility issue is concerned, since its a private method and not used anywhere, its safe to say that we assume those local variables as not visible that are being initialised and where a proposal has been requested for during the initialisation.
I talked about your patch with David and he confirmed my initial feeling that the patch is not good enough, mainly for two reasons. First, only check the type of certain expressions (AllocationExpression, MessageSend, ConditionalExpression) and also only for certain type on completion nodes expose your fix to a hole in case the AST nodes structure will be different from what you currently expected. After having made some tests, we found a simple test case for which the proposed fix does not work: String x2 = (x3 = String.format When completing after the format, x2 is inserted when selecting the first proposal... Second, doing the verification to remove this kind of local variables might be costly, especially if it's done for all local variables of all blocks in the stack... Hence, the patch needs to be deeply reworked. To address those two issues, our advice is 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. Then, we'll be sure to accept any kind of AST nodes structure. Note that the ASTVisitor might not be necessary if you can prove that the declarationSourceEnd will ever be set (David does not precisely remember whether this assumption is true or not)...
Created attachment 163101 [details] proposed fix v2.0 + regression tests Thanks Frederic. I agree that the strategy you mentioned above sounds more reasonable. The new patch captures this. The ASTVisitor was indeed required (called here via CompletionNodeDetector) since declarationSourceEnd was not set in all cases.
Created attachment 163103 [details] proposed fix v2.0 + regression tests same patch with small formatting change
(In reply to comment #8) > Created an attachment (id=163103) [details] > proposed fix v2.0 + regression tests > > same patch with small formatting change The patch looks good to me. Just an aesthetic point, personally I would prefer the conditions written as follow: if (local.declaration.initialization != null) { if (local.declaration.initialization.sourceEnd > 0) { if (this.assistNode.sourceEnd <= local.decl...initial...sourceEnd && this.assistNode.sourceStart >= local.decl...initial...sourceStart){ continue next; } } else { CompletionNodeDetector detector = new CompletionNodeDetector( this.assistNode, local.declaration.initialization); if (detector.containsCompletionNode()) { continue next; } } } IMO, it would better show that in case the source end is not set, then the AST Visitor was used to see whether the completion node is inside the initialization or not... However, I set the review as +, because it's only a suggestion not a mandatory change. Anyone may have different feeling reading this code...
Olivier, can you please release this patch? Thanks!
Created attachment 165092 [details] Modified patch as Frédéric suggested
Released for 3.6M7. Added regression test in: org.eclipse.jdt.core.tests.model.CompletionContextTests#test0167 org.eclipse.jdt.core.tests.model.CompletionContextTests#test0168 org.eclipse.jdt.core.tests.model.CompletionContextTests#test0169 org.eclipse.jdt.core.tests.model.CompletionContextTests#test0170 org.eclipse.jdt.core.tests.model.CompletionContextTests#test0171 org.eclipse.jdt.core.tests.model.CompletionContextTests#test0172 org.eclipse.jdt.core.tests.model.CompletionContextTests#test0173
I could verify that the test case attached to this bug works ok: However I found the following similar problems are still there: (1) public class X { public static void main(String args[]) { int xyk = 10; int xyz = xy|; } } Completing at the | symbol proposes xyz, which would immediately lead to an error (uninitialized) (2) public class X { int xyk = 0; int xyz = xy|; int xyp = 1; } Completing at the | symbol proposes xyz and xyp, which would immediately lead to an error (field referenced b4 definition) (3) String newText = String.format(newText, null) proposal still shows up if newText declaration were to happen as a field and not as local. I'll leave the current bug as RESOLVED, but have raised a follow up defect: bug# 310427. Verified for 3.6M7 using build I20100424-
