Community
Participate
Working Groups
Using 3.6M6 but I guess this issue exists since day 1 (i.e. 3.1). While investigating bug 305870, I discovered a strange behavior for the Search Engine. Using a pattern created with the search for constant: ANNOTATION_TYPE and limited to: REFERENCES, I still get all type references in the returned matches... See bug 306196 comment 3 for steps to be able to use the Java Search for search a query... For example on the simple following test case: @MyAnnot public class TestAnnot { String test; void foo(String str) { this.test = str; } } @interface MyAnnot {} I get a match on @MyAnnot as expected, but also 2 matches on String which is definitely not an annotation reference!
It's the same issue for references to Enum: public class TestEnum { String foo(MyEnum e) { switch (e) { case ONE: return "1"; case TWO: return "2"; default: return "-1"; } } } enum MyEnum { ONE, TWO } I get two matches: one on String (unexpected) and one on MyEnum (expected)
It's definitely a bug as the doc says both for ENUM_TYPE and ANNOTATION_TYPE that these constants are more restrictive than TYPE... And the REFERENCES doc says that it "... Can be used in conjunction with any of the nature of searched elements so as to better narrow down the search." Note that there's also the same problem with CLASS_AND_ENUM, CLASS_AND_INTERFACE and INTERFACE_AND_ANNOTATION. It looks like these constants were well implemented for DECLARATIONS but not for REFERENCES...
Created attachment 163225 [details] Proposed patch Frederic, can you review this patch?
(In reply to comment #3) > Created an attachment (id=163225) [details] > Proposed patch > > Frederic, can you review this patch? Really nice patch Satyam :-) However, I think you should not return IMPOSSIBLE_MATCH when the binding is not valid. I'm not 100% but it seems to me that in the following example: class X { Zork x; // Zork is not defined ! } Search engine won't return any match, although I would expect a potential match whatever the class suffix would have been set...
Created attachment 163366 [details] Proposed Patch This patch is taking care of missing references. It reports POTENTIAL_MATCH in those case.
(In reply to comment #5) > Created an attachment (id=163366) [details] > Proposed Patch > > This patch is taking care of missing references. It reports POTENTIAL_MATCH in > those case. Not sure to understand the last switch in the resolveLevelForType(TypeBinding) method of TypeReferenceLocator. What do you intend to filter here? Also, I did not see additional tests for potential matches. I would expect at least one...
> Not sure to understand the last switch in the resolveLevelForType(TypeBinding) > method of TypeReferenceLocator. What do you intend to filter here? I want to return the level as it is, if the typeSuffix is class or type. Otherwise, I want to stop at INACCURATE_MATCH. Returning the level as it is should not be a problem for type. For class, MissedTypeBinding()#isClass() was returning true and hence, I have included all class types. I don't have a strong argument for including class suffix. > Also, I did not see additional tests for potential matches. I would expect at > least one... I have included this in testBug306223a()
(In reply to comment #7) > > Not sure to understand the last switch in the resolveLevelForType(TypeBinding) > > method of TypeReferenceLocator. What do you intend to filter here? > I want to return the level as it is, if the typeSuffix is class or type. > Otherwise, I want to stop at INACCURATE_MATCH. > Returning the level as it is should not be a problem for type. For class, > MissedTypeBinding()#isClass() was returning true and hence, I have included all > class types. I don't have a strong argument for including class suffix. > I still do not understand what is the real need here. IMO, as soon as the binding is not valid, then we should get an INACCURATE_MATCH, hence the switch seems not necessary to me, but I may be wrong... Could you add a specific test case for this part of code and which helps me to understand what I'm missing here... > > Also, I did not see additional tests for potential matches. I would expect at > > least one... > I have included this in testBug306223a() OK, I didn't see it, thanks Could you change the package name for all tests? I noticed that testBug306223a() and testBug306223b() are located in package b306223, and testBug306223c() is located in package b306223b. This is a little bit confusing...
1. Without this patch, search("Zork", TYPE, REFERENCES,...) for the below test case will return an EXACT_MATCH. class X { Zork x; // Zork is not defined ! } I am trying to retain that behavior and I feel we should. 2. In addition to TYPE, I am also making other class suffixes to return EXACT_MATCH. I am not very particular about the behavior for this. I would need your advice on this. 3. search("abc", ANNOTATION, REFERENCES, ...) for the above test case should not return any match which could probably return an INACCURATE_MATCH if I just return if binding is not valid. May be I should add a test for this.
I probably am mistaken -- we probably won't end up in that scenario. I am trying to see if it is really needed.
Created attachment 163475 [details] Proposed patch Now, I understand that we will not end up in the scenario 3, that I have mentioned in my comment 9. Hence, I removed the last switch. However, the scenario 1 is valid and I am taking care of this in this patch -- testBug73336() of JavaSearchBugsTests and some other tests fail if I don't take care of TYPE. As I am not really comfortable with the scenario 2, I have ignored 2 in this patch. Fixed the test as you had mentioned.
(In reply to comment #9) > 1. Without this patch, search("Zork", TYPE, REFERENCES,...) for the below test > case will return an EXACT_MATCH. > class X { > Zork x; // Zork is not defined ! > } > I am trying to retain that behavior and I feel we should. > Yes, I agree > 2. In addition to TYPE, I am also making other class suffixes to return > EXACT_MATCH. I am not very particular about the behavior for this. I would > need your advice on this. > As soon as the "limit to" flag is more precise than TYPE, I would expect a potential match. It's a missing type, the compiler have no clue about what kind of type it is, hence the more appropriate answer is that it may be a valid match but the compiler does not know... I tested it and no match is returned with your patch, I think this must be changed and a specific test case added... > 3. search("abc", ANNOTATION, REFERENCES, ...) for the above test case should > not return any match which could probably return an INACCURATE_MATCH if I just > return if binding is not valid. > May be I should add a test for this. > I also expect no match for this request... and this is what happens with your patch, hence the only thing to do is to add a specific test case.
Frederic, I think you missed my comments 10 and 11. Yes, I completely agree with your comments in 12 and hence the patch that I have included in 11 is inline with those. Please review.
Created attachment 163511 [details] Proposed patch There is a small typo in the previous patch. Please use this patch.
(In reply to comment #13) > Frederic, > I think you missed my comments 10 and 11. Yes, I completely agree with your > comments in 12 and hence the patch that I have included in 11 is inline with > those. Please review. No, I didn't miss those comments, my answer was taken all of them into account... 1) Nothing more to say as we agreed on the fix. But, IMO, it's not a good idea to have modified the first test to verify this behavior. Please also add a specific test case. Then if a regression occurs, it would be easier to see in which area it happened! 2) Using your previous patch, no match was returned while searching for "*" ANNOTATIONS, REFERENCES although your new patch now correctly returns a potential match. So, it was more than a typo you fixed in the new one ;-)... Hence, *please*, add a specific test case for this! 3) Here what I said for this point > I also expect no match for this request... and this is what happens with your > patch, hence the only thing to do is to add a specific test case. So, please add this specific test case... The tests of your patch only have "*" for the tested pattern strings... To summarize, the fix looks good in the last patch, but it just misses 3 additional tests using the small snippet: class X { Zork x; // Zork is not defined ! } TIA
Created attachment 163536 [details] Additional tests (In reply to comment #15) Sorry, I also made a mix... In fact I realized that the change you did in testBug30223a() covers the point 2), hence the two tests I was asking to add for 1) and 2) are the same... But, IMO, having more tests than group them in only one is always better, hence I propose these 4 additional simple tests to be sure of the S.E. behavior when TYPE and ANNOTATION_TYPE "Search for" flags are used in combination of REFERENCES "Limit to" flag...
(In reply to comment #15) > So, it was more than a typo you fixed in the new one ;-)... The '!' was fixing for the validBinding() check :)
Created attachment 163599 [details] Proposed patch Frederic, Thanks for the tests. I have included those tests in this patch.
(In reply to comment #18) > Created an attachment (id=163599) [details] > Proposed patch > > Frederic, Thanks for the tests. I have included those tests in this patch. Released for 3.6M7 in HEAD stream
Verified for 3.6M7 using I20100425-2000
