Community
Participate
Working Groups
User-Agent: Mozilla/5.0 (compatible; Konqueror/4.3; Linux) KHTML/4.3.1 (like Gecko) SUSE Build Identifier: I20080617-2000 One (commented) example is worth a thousand words: public class Test extends Thread { private class Nested extends LinkedList<String> { // here content assist works correctly private Runnable member[] = { new Runnable() { public void run() {}; } // here it thinks you're not in the array initializer anymore }; // here it thinks you're not in Nested anymore } // here it thinks you're not in Test anymore } Reproducible: Always
Looks like a JDT Core issue.
Ayush, please investigate -- Thanks.
Created attachment 163070 [details] Patch under consideration. This patch fixes the current problem and also passes RunAllCompletionTests. Patch does not include automated tests - these need to be created. As well, more testing is needed in the area of content assist in the presence of anonymous types. See that with or without patch, there is a bogus proposal with this example: import java.util.LinkedList; public class Test extends Thread { private class Nested extends LinkedList<String> { int field; // here content assist works correctly private Test member[] = { new Test(){ void abc() {} Nest| // completion here proposes the default ctor for Nested // which makes no sense. } }; } } Ayush, can you take over from here ? Please raise separate defects for any new issues you uncover unless it is very pertinent to the current subject at hand -- Thanks.
(In reply to comment #3) > See that with or without patch, there is a bogus > proposal with this example: FYI -- I have raised bug#307337 to track this.
(In reply to comment #3) > Created an attachment (id=163070) [details] > Patch under consideration. Patch looks good.
Created attachment 163490 [details] Final patch with tests Ayush, Thanks for the review and the unit test.
Released in HEAD for 3.6M7
There is no completion proposal inside the array initializer. On line "// here it thinks you're not in the array initializer anymore". Is this expected ?
(In reply to comment #8) > There is no completion proposal inside the array initializer. > On line "// here it thinks you're not in the array initializer anymore". > Is this expected ? Don't quite know what is expected here. (1) If you attempt completion at the point below, which is logically the same point as the one you have used, I get several proposals, none of which makes sense though: import java.util.LinkedList; public class Test extends Thread { private class Nested extends LinkedList<String> { private Runnable member[] = { // completion here brings up dubious proposals new Runnable() { public void run() {}; }, // completion here brings up no proposals. }; } } (2) Also try the following: public class Test { private Runnable member[] = { // completion here brings up useless proposals }; } In this case, if we are truly completing inside the initializer, we should perhaps be using Runnable as the expected type and filter out the rest as there is no point in proposing the methods of java.lang.Object as we do now.
Should I reopen? I believe the patch did improve the initial test case, but maybe not enough.
(In reply to comment #10) > Should I reopen? I believe the patch did improve the initial test case, but > maybe not enough. I have raised bug# 310747 to cover the case (2) in comment# 9. I have reopened the current defect to look into the anomaly around case (1) in comment# 9 -- Thanks.
No need to fix it for M7. Targetting RC1.
import java.util.LinkedList; public class Test extends Thread { private class Nested extends LinkedList<String> { // here content assist works correctly private Runnable member[] = { // completion works before and after the null // more or less the same way. // if null were to be replaced by an anonymous type, // completion works before it, not after it. null, }; } } Ayush, please see why and how this case differs from the original, that should offer a clue about the fix.
This is a bit more complicated than anticipated and needs a bit more time for fuller analysis. Retargetting to 3.6.1
Created attachment 167879 [details] proposed fix The above fix fixes the case at hand and passes all tests, but I'm not sure if it does fix the root cause of the problem. I'm also not sure whether its a good idea to use goForBlockStatements in this case, since its usually used for parsing inside method bodies during recovery. Also, Srikanth and I, after some analysis of the recovery mechanism in code assist, have observed that the parser doesnt seem to take a coherent path while trying to parse an anonymous class in an array initializer. In the case given below: private X[] field = { new X(){}, | // Completion asked here } Normally, we would expect the parser to first figure out new X(){} as an anonymous class declaration (which it does) and then figure out the whole thing as a variable initializer (which it currently fails to do). This has no problems if in place of anonymous class we simply had new X(). David, what do you think about the above issues?
David, it'll be really helpful if you could look at the patch and comment. Thanks!
Since only a small part of the original problem remains and the fix for that part seems to be complicated enough IMO, it is not a good idea to target this for the maintenance stream. Will look at this for 3.7
Created attachment 193572 [details] proposed fix v2.0 + tests This fix looks better than the fix above. Basically, there are 2 ways in which parsing can discover a completion node - (a)either in the diet parse mode at org.eclipse.jdt.internal.codeassist.CompletionEngine.complete(ICompilationUnit, int, int, ITypeRoot) line 1736. (b)or during the block statements parsing at line 1842 int the same method. The block statements parsing is particularly done for completion inside methods, class intializers or member types (see org.eclipse.jdt.internal.codeassist.impl.Engine.parseBlockStatements(TypeDeclaration, CompilationUnitDeclaration, int)). In the given case, we're completing inside a field array initializer, which is not covered by the block statements parsing here. A field array initializer is first found during the diet parsing, and thats where its statements should also get parsed in the case of completion, because the completion node may be inside the array intializer. This explains the change in AssistParser.resumeAfterRecovery() which now goes for BlockStatementsOpt parsing if we're in an array initializer. The use of BlockStatementsOpt has been inspired from how the block statements are parsed in org.eclipse.jdt.internal.codeassist.impl.AssistParser.parseBlockStatements(Initializer, TypeDeclaration, CompilationUnitDeclaration). The change in RecoveredField.java is a bit unrelated and is done because the alreadyCompletedFieldInitialization field gets set to true even if we havent exited the initializer yet. (this is shown in CompletionTest.test292087d() and CompletionparserTest.test292087b()) The other change in CompletionParser.recoveryExitFromVariable is to make sure that when focus is shifted from the field initializer to the corresponding class, the elementKindStack has the right topmost element kind.
Satyam, can you please review. Thanks!
Changes look good for me. +1.
Released in HEAD for 3.7M7
This seems to break test org.eclipse.jdt.core.tests.compiler.parser.DietRecoveryTest.test75() in the BETA_JAVA7 branch. Could you please investigate? Thanks. The same test passes fine in HEAD.
The problem seems to come from the method: // https://bugs.eclipse.org/bugs/show_bug.cgi?id=292087 protected boolean isInsideArrayInitializer(){ int i = this.elementPtr; if (i > -1 && this.elementKindStack[i] == K_ARRAY_INITIALIZER) { return true; } return false; } added in CompletionParser. If I remove it, the test passes. I'd like to identify why this is different from HEAD.
(In reply to comment #23) > I'd like to identify why this is different from HEAD. I was out of synch on HEAD. Once I synchronized, HEAD is also broken.
Because isInsideArrayInitializer() returns true for test75, the recovery is different. It doesn't go for headers, but it goes for goForBlockStatementsopt(). I am investigating if the recovery looks good.
Created attachment 193863 [details] updated test The recovery has changed inside the array initializer for int[] ii because it has an array initializer. The new parsed unit that we get is valid because now for completion, we go inside the array initializers and parse the statements as well. This is done via the goForBlockStatementsOpt() in the AssistParser.resumeAfterRecovery() since isInsideArrayInitializer() returns true. Earlier, we used to skip the contents of the array init and thus never found the class Local. Hence, updating the test is sufficient.
Released updated test in HEAD.
Are we 100% sure about this fix? It caused regressions several times now and we're just a few days away from RC0.
(In reply to comment #28) > Are we 100% sure about this fix? It caused regressions several times now and > we're just a few days away from RC0. Thats not quite correct. Any fix for this didnt really cause a regression, but as mentioned in comment 10, improved the assist but not enough. The latest test failure yesterday was because the recovery has got changed in one case and the test for that had not been updated. So i'd say its safe.
My recollection (could be faulty as I have more gray hairs than black :-() was that we had decided the best time to release this was in the M1/M2 frame of 3.7. That window gone, it would have been better to have waited for 3.8 M1/M2. Again my recollection was that though there was some uncertainty about the fix, it was fairly well localized.
Reopening. For safety reason, we should revert that fix. I am not too confident when one parser reports a different tree from other parsers. We might introduce subtle inconsistencies and this is late in the game to get fixed. Targeting 3.8M1 when available is better and safer.
(In reply to comment #31) > Targeting 3.8M1 when available is better and safer. Reverted fix in HEAD and BETA_JAVA7. Will re-release in 3.8M1.
Created attachment 201378 [details] patch updated for HEAD Patch to be released for 3.8M2
Released in HEAD for 3.8M2.
Verified for 3.8M2 using build I20110911-2000.
