Community
Participate
Working Groups
Build Identifier: 20110615-0604 The change method signature refactoring introduces a compilation error. Reproducible: Always Steps to Reproduce: 1. Create the classes package p2; public class C { } package p1; import p2.*; public class B extends A { long k(){ return 0; } } package p2; public class A { long k(){ return 1; } public long m(){ return new A().k(); } } 2. Apply the change method signature refactoring to add a "int" parameter to B.k package p1; import p2.*; public class B extends A { long k(int i){ return 0; } } package p2; public class C { } package p2; public class A { long k(){ return 1; } public long m(){ return new A().k(0); } } 3. The resulting program does not compile
This is a bug in the search engine: A search for references to p1.B.k() should not find a match in p2.A.m(), since the package-visible p2.A.k() is *not* overridden by p1.B.k().
Satyam, please investigate.
Created attachment 211840 [details] proposed patch for this bug I found that the root cause is probably in org.eclipse.jdt.core/search/org/eclipse/jdt/internal/core/search/matching/MethodLocator.java isVirtualInvoke. I may not have fixed it in the best way. Basically checking if the method is private or if it is default and not in the same package then it is not a virtual invoke
Samrat, Thanks for the patch. The patch contains the changes required to fix the problem and that it is correct. It doesn't take care of null declaration qualification, which could be optional during search. Do you want to fix this completely? If so, please try to fix this and try to get the testcase too. Otherwise, I could take this to completeness shortly. Thanks again.
(In reply to comment #4) > Samrat, Thanks for the patch. The patch contains the changes required to fix > the problem and that it is correct. It doesn't take care of null declaration > qualification, which could be optional during search. > > Do you want to fix this completely? If so, please try to fix this and try to > get the testcase too. Otherwise, I could take this to completeness shortly. > > Thanks again. Satyam, can you please clarify a bit more. I am not sure if understand you correctly when you say "It doesn't take care of null declaration qualification, which could be optional during search."
(In reply to comment #5) > Satyam, can you please clarify a bit more. I am not sure if understand you > correctly when you say "It doesn't take care of null declaration > qualification, which could be optional during search." If you do a Java search for references to <Type>.<Method>(), without qualifying <Type> at all, the this.pattern.declaringQualification could be null and there could be problems with the current patch.
(In reply to comment #6) > (In reply to comment #5) > > Satyam, can you please clarify a bit more. I am not sure if understand you > > correctly when you say "It doesn't take care of null declaration > > qualification, which could be optional during search." > If you do a Java search for references to <Type>.<Method>(), without qualifying > <Type> at all, the this.pattern.declaringQualification could be null and there > could be problems with the current patch. I think eclipse is not going in that path. If you do Java search for method references without specifying the type, mustResolve in MatchLocator.reportMatching is always set to false and hence MethodLocator.resolveLevel is never called. If you still think the patch needs tweaking please go ahead :)
(In reply to comment #7) > (In reply to comment #6) > > (In reply to comment #5) > > > Satyam, can you please clarify a bit more. I am not sure if understand you > > > correctly when you say "It doesn't take care of null declaration > > > qualification, which could be optional during search." > > If you do a Java search for references to <Type>.<Method>(), without qualifying > > <Type> at all, the this.pattern.declaringQualification could be null and there > > could be problems with the current patch. > > I think eclipse is not going in that path. If you do Java search for method > references without specifying the type, mustResolve in > MatchLocator.reportMatching is always set to false and hence > MethodLocator.resolveLevel is never called. If you still think the patch needs > tweaking please go ahead :) Actually mentioning the type partially would run into this path. Fixing this seems to get more involved. Will attach a patch shortly.
Created attachment 213744 [details] Proposed patch
Ayush, Can you review?
(In reply to comment #10) > Ayush, Can you review? I have a few comments: - copyrights need to be added to MatchLocator. - In testBug357547c, A.k() - looks like u intended to make it default but made it public - testBug357547b passes without the fix. Also, even if I change k() to public in A and B, it still does not return me any references. Does p*B.k() not include p1.B.k()? TO test more I created two more classes pp.ppB and pp.ppA with other things same as comment 0. Search for refs to p*B.k() should atleast return the call in ppA.m(), no? - i think the condition added i.e. this.pattern.focus != null is always false in all the tests, which basically wipes out the effect of the other two conditions adjoing it. Is there a test case when focus is not null? - Probably an existing problem: comment 0 example - right click on k() declared in B and search for declarations yields both B.k() and A.k(). However, search pattern is p1.B.k(). Why should A.k() be shown. When i search for p1.B.k from "java search" dialog i get only B.k() which looks correct. - [unrelated] I get a warning on B.k() from comment 0 - The method B.k() does not override the inherited method from A since it is private to a different package. Quick fix offered is 'change visibility of A.k to default'. This is wrong. Will file a new UI bug for this.
(In reply to comment #11) Hi Ayush, Thanks for the review. > I have a few comments: > - copyrights need to be added to MatchLocator. There is not change in MatchLocator. Are you talking about something else? > > - In testBug357547c, A.k() - looks like u intended to make it default but made > it public That is true. I will change it. > > - testBug357547b passes without the fix. Also, even if I change k() to public > in A and B, it still does not return me any references. Does p*B.k() not > include p1.B.k()? TO test more I created two more classes pp.ppB and pp.ppA > with other things same as comment 0. Search for refs to p*B.k() should atleast > return the call in ppA.m(), no? There looks like some other problem too. I will find that out. search for p*.B.k() works good. > > - i think the condition added i.e. this.pattern.focus != null is always false > in all the tests, which basically wipes out the effect of the other two > conditions adjoing it. Is there a test case when focus is not null? testBug357547a is for that. > > - Probably an existing problem: comment 0 example - right click on k() declared > in B and search for declarations yields both B.k() and A.k(). However, search > pattern is p1.B.k(). Why should A.k() be shown. When i search for p1.B.k from > "java search" dialog i get only B.k() which looks correct. We should look at the package even in case of declarations. I will look at it. However, we intentionally report the B.k() adding some flavours to the search results. > - [unrelated] I get a warning on B.k() from comment 0 - The method B.k() does > not override the inherited method from A since it is private to a different > package. Quick fix offered is 'change visibility of A.k to default'. This is > wrong. Will file a new UI bug for this. I don't think this is wrong.
(In reply to comment #12) > (In reply to comment #11) > Hi Ayush, Thanks for the review. > > > I have a few comments: > > - copyrights need to be added to MatchLocator. > There is not change in MatchLocator. Are you talking about something else? Sorry I meant MethodLocator, where Samrat had submitted the initial fix. > > > > - i think the condition added i.e. this.pattern.focus != null is always false > > in all the tests, which basically wipes out the effect of the other two > > conditions adjoing it. Is there a test case when focus is not null? > testBug357547a is for that. Oops yes I missed it > > - [unrelated] I get a warning on B.k() from comment 0 - The method B.k() does > > not override the inherited method from A since it is private to a different > > package. Quick fix offered is 'change visibility of A.k to default'. This is > > wrong. Will file a new UI bug for this. > I don't think this is wrong. Why not? The quick fix does not fix the problem, so its bogus, right?
(In reply to comment #13) > Sorry I meant MethodLocator, where Samrat had submitted the initial fix. Samrat, can you give your copyright message. I would like to add in the file. > > > - [unrelated] I get a warning on B.k() from comment 0 - The method B.k() does > > > not override the inherited method from A since it is private to a different > > > package. Quick fix offered is 'change visibility of A.k to default'. This is > > > wrong. Will file a new UI bug for this. > > I don't think this is wrong. > Why not? The quick fix does not fix the problem, so its bogus, right? You are right. I didn't look at the message properly. It is a bug.
Created attachment 214284 [details] Updated patch Updated according to Ayush's comments. - Made the change for search for declarations also to take care of the default, when a search pattern is used. Added a test for this. - Updated the tests (testBug357547b and testBug357547c) accoding to the comments. - Didn't fix the 'p*B' type reference problem mentioned for testBug357547b, as it is a separate issue. Ayush, please have a look at this. Thanks.
(In reply to comment #14) > (In reply to comment #13) > > Sorry I meant MethodLocator, where Samrat had submitted the initial fix. > Samrat, can you give your copyright message. I would like to add in the file. Samrat, the format is this: Your Name <email@example.com> - Bug Title - https://bugs.eclipse.org/BUG_NUMBER > > > > - [unrelated] I get a warning on B.k() from comment 0 - The method B.k() does > > > > not override the inherited method from A since it is private to a different > > > > package. Quick fix offered is 'change visibility of A.k to default'. This is > > > > wrong. Will file a new UI bug for this. Opened bug 377243
(In reply to comment #15) > Created attachment 214284 [details] > Updated patch > > Updated according to Ayush's comments. > - Made the change for search for declarations also to take care of the default, > when a search pattern is used. Added a test for this. > - Updated the tests (testBug357547b and testBug357547c) accoding to the > comments. > - Didn't fix the 'p*B' type reference problem mentioned for testBug357547b, as > it is a separate issue. > > Ayush, please have a look at this. Thanks. Wrong patch.. will update with the new patch shortly.
Created attachment 214290 [details] Updated patch Correct Patch as for comment 15
(In reply to comment #18) > Created attachment 214290 [details] > Updated patch > > Correct Patch as for comment 15 Hi Satyam here is my copyright message. Samrat Dhillon samrat.dhillon@gmail.com - Search for method references is returning methods as overriden even if the superclass's method is only package-visible - https://bugs.eclipse.org/357547
+1. The two remaining issues can be handled in separate bugs: - p*B.k() problem - the second last point in comment 11
(In reply to comment #20) > +1. > The two remaining issues can be handled in separate bugs: > - p*B.k() problem > - the second last point in comment 11 Thanks Ayush for the review. Released this patch on master via commit 4f8b41427e5710e37012dc18560acc142566864b and commit 474e0db6559546cf5ac1476d3c4eb203e5f714e7
(In reply to comment #20) > +1. > The two remaining issues can be handled in separate bugs: > - p*B.k() problem I get the desired result if I use p*.B.k() - note the '.' after the '*'
The fix for the bug as reported in comment #0 has been verified with build I20120429-1800
