Community
Participate
Working Groups
I20100424-2000 package test; interface In{} interface Inn{ interface Inn2{} } class ABC { interface ABCInterface {}; } public class Try implements In|{ } Attempt completion at |. You will see that @Inherited is one of the choices. Choosing this results immediately in an error: "The annotation type Inherited should not be used as a superinterface for Try"
Created attachment 169489 [details] proposed fix v1.0 + regression tests This patch suppresses annotation proposals after implements keyword. CompletionEngine$assistNodeIsInterface is currently used to denote whether the assistNode corresponds to either an interface or an annotation. But incase of implements we have to restrict proposals to not include annotations. Hence a separate boolean CompletionEngine$assistNodeIsInterfaceExcludingAnnotations has been introduced in the above patch. Another constant - K_INTERFACE_EXCLUDING_ANNOTATIONS has been introduced. This gets set whenever the rule Parser.consumeClassHeaderImplements is reduced to by the CompletionParser. And then CompletionEngine$assistNodeIsInterfaceExcludingAnnotations holds the info about whether the former constant is set. The latter then works much the same way as CompletionEngine$assistNodeIsInterface, except that it helps in filtering out annotations whenever it is set (ie. when assist node is after 'implements'). Note that for Parameterized qualified type binding we dont propose the static member types, and so a member annotation isnt proposed even in the current behaviour. So these changes dont affect behaviour wrt ParameterizedQualifiedTypeBinding. The new boolean variable has still been added though, for the sake of completion.
Created attachment 170934 [details] Alternate patch Ayush, Can you please take a look at this patch ? IMO, this is simpler & minimal and passes RunAllCompletionTests including your new tests.
(In reply to comment #2) > Ayush, Can you please take a look at this patch ? > IMO, this is simpler & minimal and passes RunAllCompletionTests > including your new tests. Srikanth, there seems to be something wrong with the patch. Its incomplete. Can you please upload it again?
(In reply to comment #3) > (In reply to comment #2) > > Ayush, Can you please take a look at this patch ? > > IMO, this is simpler & minimal and passes RunAllCompletionTests > > including your new tests. > > Srikanth, there seems to be something wrong with the patch. Its incomplete. Can > you please upload it again? Don't know what happened there, Unfortunately, I cleaned out my workspace of these changes :-( and I am not able to restore it from local history. Here is what I did though: The patch was a replica of the fix for https://bugs.eclipse.org/bugs/show_bug.cgi?id=99399 : that defect is about not proposing final types in extends context. (1) Introduce a field assistNodeIsImplementedType to completion engine. (2) This field is initialized/assigned to in exactly the same places where assistNodeIsExtendedType is assigned to, but by calling a new method assistNodeIsImplementedType. (3) This new method is modelled after the existing method assistNodeIsExtendedType (4) Then in every place assistNodeIsExtendedType is checked and final types are skipped, check for assistNodeIsImplementedType and skip annotations. (5) CompletionParser needs a new method consumeClassHeaderImplements very similar to consumeClassHeaderExtends which sets the assistNodeParent to typeDecl if assistNode happens to be an implemented supertype. Such a patch localizes the changes to just two source files and also "re-uses" (the design of) an earlier fix.
(In reply to comment #4) > (1) Introduce a field assistNodeIsImplementedType to completion engine. > (2) This field is initialized/assigned to in exactly the same places > where assistNodeIsExtendedType is assigned to, but by calling a new > method assistNodeIsImplementedType. > (3) This new method is modelled after the existing method > assistNodeIsExtendedType > (4) Then in every place assistNodeIsExtendedType is checked and final > types are skipped, check for assistNodeIsImplementedType and skip > annotations. > (5) CompletionParser needs a new method consumeClassHeaderImplements > very similar to consumeClassHeaderExtends which sets the assistNodeParent > to typeDecl if assistNode happens to be an implemented supertype. > I cannot see any objectionable thing in this approach per se. However, I have a few doubts about it - where all do you set the boolean assistNodeIsImplementedType? The extends case was pretty straightforward since it had to be done everywhere, but the implements case is not so simple. There are places where, after implements, annotations are still valid(such as after wildcard). So this has to be taken care of in the fix. Also this new field is going to coexist with the already defined assistNodeIsInterface? How do you differentiate between these two fields? Wont there be cases when both of them will be true (I dont know if it'll have any effect, but its not correct for both to be set true at the same time). Again, in my patch there are places where we set searchFor to IJavaSearchConstants.INTERFACE_AND_ANNOTATION. It should be set to IJavaSearchConstants.INTERFACE in cases where we dont need to look for annotations at all. Although I think CompletionEngine#acceptTypes(scope) will filter off those annotations in your patch as well, won't it be better if we dont put them in the list of the types found at all?
Created attachment 171067 [details] Alternate patch - Take 2 Reconstructed the patch. This also addresses the comment about filtering of annotation types sooner. Ayush, see if this patch address all concerns, Or if there are any loose ends still. For the record, your original proposal patch looks good and I didn't find issues with it. I felt it can be simplified and made more in tune with other fixes already made.
(In reply to comment #6) > Created an attachment (id=171067) [details] > Alternate patch - Take 2 > > Reconstructed the patch. This also addresses the comment about > filtering of annotation types sooner. > > Ayush, see if this patch address all concerns, Or if there are > any loose ends still. Patch looks good. Now i'm confused about which one to keep. Both patches have their own merits - while on the one hand my patch goes along with the design of the boolean flags in CompletionEngine and the new boolean field introduced has a one-to-one relationship with IJavaSearchConstants.INTERFACE (basically it fits nicely with the other boolean flags and doesnt look like an outsider), it does have a lot of changes at many different places, which, as your patch shows, can be avoided. Your patch, on the other hand, looks simplistic and localises changes to a few places, as well as reuses an existing fix, but introduces a boolean field which seems to kind of conflict with the already existing CompletionEngine#assistNodeIsInterface ( since this and assisntNodeIsImplementedType can both be true at the same time, which doesnt make much sense). However, I dont see any point which would make me prefer any fix over the other. I'd veer a bit towards your fix since it has fewer changes. :) What do you think?
(In reply to comment #7) > Your patch, on the other hand, looks simplistic and localises changes to a few I think you meant to say simpler instead of simplistic :) > places, as well as reuses an existing fix, but introduces a boolean field which > seems to kind of conflict with the already existing > CompletionEngine#assistNodeIsInterface ( since this and > assisntNodeIsImplementedType can both be true at the same time, which doesnt > make much sense). I don't think they conflict at all, both can be meaningfully time at the same time - but you can rename this new field to the name you had i.e, assistNodeIsInterfaceExcludingAnnotations if that makes it clearer. > other. I'd veer a bit towards your fix since it has fewer changes. :) > What do you think? Sounds good. Per above, you can change the name of the field to make it clearer.
Created attachment 171247 [details] srikanth's patch with small changes (In reply to comment #8) > I think you meant to say simpler instead of simplistic :) Yes i did mean simpler. :) > Sounds good. Per above, you can change the name of the field to make it > clearer. Fixed this in the new patch, and also introduced another test in CompletionParserTest#testBug310423() to make sure that the parent assist node is set to the type declaration.
(In reply to comment #9) > > Sounds good. Per above, you can change the name of the field to make it > > clearer. > > Fixed this in the new patch I gave it a quick once over and it looks good. There is a typo in the field name though, it reads assistNodeIsInterfaceExcludinAnnotation instead of assistNodeIsInterfaceExcludingAnnotation.
(In reply to comment #11) > I gave it a quick once over and it looks good. There is a typo in the > field name though, it reads assistNodeIsInterfaceExcludinAnnotation > instead of assistNodeIsInterfaceExcludingAnnotation. Ok, this patch will anyways have to be updated for HEAD. So will correct the typo in the updated patch itself. Thanks Srikanth!
Created attachment 172307 [details] patch with corrected typo
Released in HEAD for 3.7M1. Added tests in: CompletionParserTest#testBug310423() CompletionTests_1_5#testBug310423a() CompletionTests_1_5#testBug310423a()
Verified for 3.7 M1 using build I20100802-1800
