Community
Participate
Working Groups
Spawned from bug 155013 comment 18 to comment 24 >>> In case we later find that somebody needs more specific local elements, this >>> could be added later, similar to how we've already done with >>> TypeReferenceMatch#getLocalElement(), etc. . > >> Can't getLocalElement() return the IAnnotation in this case and modify the >> javadoc accordingly? > Annotations can be used on local variable (bug 209778 is an example). In this > case we could not used the local element to report both the local variable and > the annotation. That shouldn't be a problem if getLocalElement() always reports the "most local" of the local elements. Clients could just call getLocalElement() and then walk up the parent chain if they are also interested in parents. E.g. in JDT/UI, we would *not* go up to the ILocalVariable when looking for similar elements to rename along with the type, since we're only interested in references that are in the declared type of the local variable but would like to skip references in the annotations. We currently also consider references in an annotation, which is a bug, IMO. I found the idea quite nice, and I'm sure people dealing with annotations would also be glad to get access to the enclosing IAnnotation directly from a *ReferenceMatch. In summary: 1. generalize the localElement and otherElements properties of TypeReferenceMatch into a new type ReferenceMatch (extends SearchMatch) and let all *ReferenceMatch types extend this new ReferenceMatch 2. return the innermost IAnnotation as local elements of ReferenceMatches
Set target as 3.4 as fixing this bug is required to address bug 155013 comment 8 point 3.a)
Could defer until 3.5 since not super critical. Nice to have though for API completeness. So time permitting for M6.
Bug 209778 should be fixed at the same time as this bug
Line 623 of RenameTypeProcessor: if (match.getLocalElement() instanceof ILocalVariable) { relies on the type of the local element... Change SearchEngine to report the most local element will break it and does not fit the Markus' assumption in this bug description: "That shouldn't be a problem if getLocalElement() always reports the "most local" of the local elements..." I'm currently implementing the prototype of this API change and I can confirm that the '3rd strategy' of refactoring will no longer work at all without a corresponding change in JDT/UI code... Another remark is about the other elements (getOtherElements()). My assumption is that if we change S.E. to report the most local element in other element, we should also do the same for the other elements. Otherwise they would be an inconsistency... So, two questions at this stage: 1) Is it OK to change the existing API behavior and break clients who use it? 2) If answer to 1) is YES, is it OK to also change the behavior of getOtherElements() in the same way (and of course also break clients who use it)?
(In reply to comment #4) > I'm currently implementing the prototype of this API change and I can confirm > that the '3rd strategy' of refactoring will no longer work at all without a > corresponding change in JDT/UI code... Can you give an example where it does not work? I checked this example: import java.util.List; public class Ref { void doA(Ref ref) {} void doB(List<Ref> ref) {} void doC(@Tag(Ref.class) Ref ref) {} void dontD(@Tag(Ref.class) Object ref) {} } @interface Tag { Class value(); } The parameter 'ref' in 'dontD' is currently renamed when I rename type 'Ref' to 'Bla' with updating similar stuff, but it should not be, since the reference to Ref is not in the ILocalVariable but in the IAnnotation. I don't have your implementation to test it, but I think the if (match.getLocalElement() instanceof ILocalVariable) ... should do exactly that. > Another remark is about the other elements (getOtherElements()). My assumption > is that if we change S.E. to report the most local element in other element, we > should also do the same for the other elements. Otherwise they would be an > inconsistency... Yes, see bug 209778 and comment 0 > In summary: > 1. > 1) Is it OK to change the existing API behavior and break clients who use it? I think yes: TypeReferenceMatch.getLocalElement() says "This *may* be a local variable [..] or a type parameter [..]". I would read "may" as "can be an ILV, an ITP, or something else...", bend the API a bit, and extend this to also return an IAnnotation if that happens to be the innermost local element. > 2) If answer to 1) is YES, is it OK to also change the behavior of > getOtherElements() in the same way (and of course also break clients who use > it)? Yes, because Javadoc of getOtherElements() just refers to getLocalElement().
Created attachment 92799 [details] Proposed patch This patch passes all JDT/Core tests and I'm currently running JDT/UI ones. It also fixes bug 209778 and so matches the expected behavior described in comment 5 (e.g. does not rename 'ref' in 'dontD') Markus, Jerome, could you review carefully the changes done in the SearchMatch hierarchy and let me know if the design and the Javadoc description make sense? Thanks in advance
All JDT/UI tests also pass. This confirms the fact that nothing should be broken by this patch :-)
Created attachment 92832 [details] New proposed patch after initial Jerome's review Herea re the Jerome's remarks after a quick initial review of the first patch: - no new constructors - maybe ReferenceMatch should be in internal package - getLocalElement design notes in Javadoc should be @since 3.4 (e.g for FieldReferenceMatch) - setLocalElement's Javadoc makes reference to internal field - same for setOtherElements - ReferenceMatch's doc makes reference to "type reference" We also realized that no annotation can be a local element for fields, local variables and type parameters references. So, I removed the changes done on the corresponding SearchMatch classes. This new patch should address all these points, it passes all JDT/Core Model and DOM tests. I'll run all JDT/UI tests to verify that everything is still OK...
(In reply to comment #8) - design notes still present in TypeReferenceMatch#getLocalElement() - TODO left in MethodReferenceMatch#getLocalElement() - same for PackageReferenceMatch - PackageReferenceMatch#getLocalElement() mentions "method reference"
(In reply to comment #8) > - maybe ReferenceMatch should be in internal package Why? It's a useful abstraction, and introducing the APIs get/setLocalElement() in an internal class is confusing and inconvenient for clients. It also makes e.g. setLocalElement(..) inaccessible for clients of MethodReferenceMatch. > We also realized that no annotation can be a local element for fields, local > variables and type parameters references. So, I removed the changes done on the > corresponding SearchMatch classes. What about references to Num.CONST here? @Num(number= Num.CONST) @interface Num { public static final int CONST= 42; int number(); } Isn't this a FieldReferenceMatch whose getLocalElement() should return the @Num annotation? For LocalVariableReferenceMatch and TypeParameterReferenceMatch, I have not found examples where they are referenced inside an annotation, so making them subclass ReferenceMatch is not necessary for this bug (but might be interesting in the future, e.g. to get the enclosing ILocalVariable when the searched local variable is used in the initializer). Javadocs indeed need some polish. I would be defensive in what you specify for getLocalElement(). The only guarantee should be that the elements are IJavaElements whose getParent() chain eventually leads to the element from getElement(). All the rest should be hints.
Note I'm not sure what is the value for providing setLocalElement(...). Search matches can be passed to several search participants. If one of them is changing the local element, subsequent participants will get unexpected results.
(In reply to comment #11) > Note I'm not sure what is the value for providing setLocalElement(...). All other properties of search matches are settable by clients (including TypeReferenceMatch.setLocalElement(IJavaElement)), so this property should also be settable for consistency. IIRC, this was done to allow clients to create their own search matches or modify existing matches (the Rename refactoring e.g. updates positions in some cases to strip out qualifiers). > Search matches can be passed to several search participants. If one of them is > changing the local element, subsequent participants will get unexpected > results. I don't understand how that could happen. AFAIK, a SearchParticipant only *generates* search matches.
(In reply to comment #12) > (In reply to comment #11) > > Note I'm not sure what is the value for providing setLocalElement(...). > > All other properties of search matches are settable by clients (including > TypeReferenceMatch.setLocalElement(IJavaElement)), so this property should also > be settable for consistency. Actually I think this was a mistake. And no one is using them: we didn't find any reference to set*Element() in Ganymede. > IIRC, this was done to allow clients to create > their own search matches or modify existing matches (the Rename refactoring > e.g. updates positions in some cases to strip out qualifiers). If there were such clients they could have their own subclasses. > > Search matches can be passed to several search participants. If one of them is > > changing the local element, subsequent participants will get unexpected > > results. > > I don't understand how that could happen. AFAIK, a SearchParticipant only > *generates* search matches. Sorry I meant search requestors (e.g. if a search requestor wraps another search requestor). Search matches are the result of an operation. It is completely unexpected to make side effects on such results.
(In reply to comment #13) > Search matches are the result of an operation. It is completely unexpected to > make side effects on such results. But it's convenient and performant (saves us from creating a wrapper for every match object or copy everything into new objects of a subclass). And it's not a problem as long as the requestor is not passed on to others (which it usually is not). Furthermore, it may also make sense for search participants to create their own TypeReferenceMatches, e.g. for some embedded SQL in a String that is known to refer to a Java type name. The SearchMatches are today completely usable in this example, but when clients cannot set all properties, it becomes awkward.
(In reply to comment #14) > > But it's convenient and performant (saves us from creating a wrapper for every > match object or copy everything into new objects of a subclass). And it's not a > problem as long as the requestor is not passed on to others (which it usually > is not). > > Furthermore, it may also make sense for search participants to create their own > TypeReferenceMatches, e.g. for some embedded SQL in a String that is known to > refer to a Java type name. The SearchMatches are today completely usable in > this example, but when clients cannot set all properties, it becomes awkward. > I'm not really sure to understand why you (JDT/UI) are so upset by this setter (setLocalElement(...)) inaccessibility on ReferenceMatch although you currently do not use it on TypeReferenceMatch? Do you plan to use it in the near future?
> I'm not really sure to understand why you (JDT/UI) are so upset by this setter > (setLocalElement(...)) inaccessibility on ReferenceMatch although you currently > do not use it on TypeReferenceMatch? Do you plan to use it in the near future? We have no plans to use setLocalElement(...) right now, but I just like clean and consistent APIs. However, I won't die without it. But I will die if ReferenceMatch is not made API. Without that, a client who wants to find matches in annotations would have to write a sermon of "instanceof *ReferenceMatch" tests and also cast to each subtype of ReferenceMatch just to be able to call getLocalElement().
(In reply to comment #16) > > I'm not really sure to understand why you (JDT/UI) are so upset by this setter > > (setLocalElement(...)) inaccessibility on ReferenceMatch although you currently > > do not use it on TypeReferenceMatch? Do you plan to use it in the near future? > First, I wanted to apologize reading this again. It looks like I should not have to use 'upset' as this term is too vague and could make my phrase tendentious or worst... :-( I just wanted to mean 'annoyed' and should have to use this term instead... I wish I was a native English speaker! <g> > We have no plans to use setLocalElement(...) right now, but I just like clean > and consistent APIs. However, I won't die without it. But I will die if > ReferenceMatch is not made API. Without that, a client who wants to find > matches in annotations would have to write a sermon of "instanceof > *ReferenceMatch" tests and also cast to each subtype of ReferenceMatch just to > be able to call getLocalElement(). > As, we urgently need to focus today on this API, we agreed to move ReferenceMatch as an API class. _All_ *ReferenceMatch classes will inherit from this abstract class but getLocalElement will ever return null for LocalVariableReferenceMatch and TypeParameterReferenceMatch. setLocalElement will not be accessible on the ReferenceMatch API class but only on TypeReferenceMatch for backward compatibility... I hope this would be ok for you and that I can implement this today to let you review it before your long week-end ;-)
> [..] upset [..] No offense taken. I don't care about etiquette here, I only care about APIs ;-) > _All_ *ReferenceMatch classes will inherit from ReferenceMatch [..] This of course poses the problem how to specify the API for LocalVariable-/TypeParameterReferenceMatch#getLocalElement(). I would just let the *ReferenceMatch which really implement #getLocalElement() inherit from ReferenceMatch. The others can easily be added later on. Otherwise, the API would have to tell that it may not work in 3.4 for some targets but would probably be fixed for later releases. Not nice and not too helpful for clients. > I hope this would be ok for you and that I can implement this today to let you > review it before your long week-end ;-) AFAICS in my inbox, my long week-end will not be that long in the end :-/
(In reply to comment #18) > > _All_ *ReferenceMatch classes will inherit from ReferenceMatch [..] > > This of course poses the problem how to specify the API for > LocalVariable-/TypeParameterReferenceMatch#getLocalElement(). I would just let > the *ReferenceMatch which really implement #getLocalElement() inherit from > ReferenceMatch. The others can easily be added later on. > > Otherwise, the API would have to tell that it may not work in 3.4 for some > targets but would probably be fixed for later releases. Not nice and not too > helpful for clients. > We were more thinking about a simple implementation and documentation in ReferenceMatch, something like: public abstract class ReferenceMatch extends SearchMatch { ... /** * Returns the local element of this search match. * * @return <code>null</code> if the search match does not have any local * element. Otherwise, look at the subclass of the concrete search match * to know what Java element could be returned. */ public IJavaElement getLocalElement() { return null; } }
Created attachment 93144 [details] Last proposed patch
Jerome already has kindly reviewed this patch, thanks for the time :-) Markus, I hope you'll have some time to have a look at it before I release it. I'll plan to do it tomorrow evening, except if you do not agree and shout before...
(In reply to comment #20) 1. Some typos in ReferenceMatch.getLocalElement(): - "inner most" -> "inner-most" - "the most usal case" -> "usual" - "<pre> public class X< T extends Test>" -> "X<T extends Test>" 2. InternalReferenceMatch as a superclass of an API class causes awkward API problems: With the patch, an innocent call to TypeReferenceMatch#getLocalElement() is now flagged as "Discouraged access". The best design is not to have inaccessible supertypes of an API class. 3. Issues I found while testing with a breakpoint in NewSearchResultCollector (you can also deal with these in another bug if you want): public class Test { @Tag Test test1, test2, test3; void method() { @Tag Test local= null; @Tag Test local1, local2, local3; } } @interface Tag {} * References to type Tag: Expected (e= getElement(); le= getLocalElement(); oe= getOtherElements()): - 1: e= test1; le= @Tag (in test1), oe= {@Tag (in test2), @Tag (in test3)} => was: oe= null - 2: e= method; le= ILocalVariable local; oe= null => was: le= null - 3: e= method; le= ILocalVariable local1; oe= {ILV local2, ILV local3} => was: le= null oe= null 4. With annotations, there are now also cases where Method-/Field-/PackageReferenceMatch require getOtherElements(). This method should be pulled up into ReferenceMatch. Example: public class TestMethodReference { @Annot(clazz = pack.Test.class) int x, y; } @interface Annot { Class clazz(); } => Search for references to 'clazz' should return a MethodReferenceMatch with e= x; le= @Annot (in x); oe= {@Annot (in y)} => Similar case for FieldReferenceMatch and PackageReferenceMatch (e.g. search for references to 'pack' in example above)
(In reply to comment #22) > 1. Some typos in ReferenceMatch.getLocalElement(): > Fixed. > > 2. InternalReferenceMatch as a superclass of an API class causes awkward API > problems: With the patch, an innocent call to > TypeReferenceMatch#getLocalElement() is now flagged as "Discouraged access". > The best design is not to have inaccessible supertypes of an API class. > Perhaps the solution would be to let getLocalElement() on TypeReferenceMatch which simply invokes the super method. > > 3. Issues I found while testing with a breakpoint in NewSearchResultCollector > (you can also deal with these in another bug if you want): > I'll investigate... > > 4. With annotations, there are now also cases where > Method-/Field-/PackageReferenceMatch require getOtherElements(). This method > should be pulled up into ReferenceMatch. > Example: > > public class TestMethodReference { > @Annot(clazz = pack.Test.class) int x, y; > } > @interface Annot { > Class clazz(); > } > > => Search for references to 'clazz' should return a MethodReferenceMatch with > e= x; le= @Annot (in x); oe= {@Annot (in y)} > > => Similar case for FieldReferenceMatch and PackageReferenceMatch (e.g. search > for references to 'pack' in example above) > We talk about this case with Jerome and we agreed that it was not necessary as it would be the same annotation (even if the parent is different). Why do you need to have the other local variable in this case?
(In reply to comment #23) > Perhaps the solution would be to let getLocalElement() on TypeReferenceMatch > which simply invokes the super method. Would be a workaround if applied to all subclasses of InternalReferenceMatch, yes. Is it worth the hassle, given that the only "advantage" is that the *ReferenceMatch classes have one special property that is just read-only? > > 4. With annotations, there are now also cases where > > Method-/Field-/PackageReferenceMatch require getOtherElements(). [..] > We talk about this case with Jerome and we agreed that it was not necessary as > it would be the same annotation (even if the parent is different). Why do you > need to have the other local variable in this case? I don't need it, but that's probably because I don't write tooling for annotations. The rationale is the same as for TypeReferenceMatch#getOtherElements(): There, we needed ILocalVariables as other elements because that's the only way for clients to get at these ILocalVariables without building an AST. One scenario where this would be useful is if someone needs to collect all local variables that are annotated with "@Annot(clazz= pack.Test.class)" with an explicit "class= *". Without MethodReferenceMatch#getOtherElements(), they would not be able to find out about the variable y that's also annotated. But since the API deadline is approaching quickly, we can also defer point 4.
(In reply to comment #24) > (In reply to comment #23) > > Perhaps the solution would be to let getLocalElement() on TypeReferenceMatch > > which simply invokes the super method. > > Would be a workaround if applied to all subclasses of InternalReferenceMatch, > yes. Is it worth the hassle, given that the only "advantage" is that the > *ReferenceMatch classes have one special property that is just read-only? > That's right and I do not want to use this workaround on all subclasses. So, I do prefer rename the method getLocalElement() on InternalReferenceMatch as localElement(), implement it on ReferenceMatch to return null and finally make getLocalElement() calling this protected method. Then, there's no longer warning while calling getLocalElement() from a TypeReferenceMatch outside JDT/Core. > > > 4. With annotations, there are now also cases where > > > Method-/Field-/PackageReferenceMatch require getOtherElements(). > [..] > I don't need it, but that's probably because I don't write tooling for > annotations. The rationale is the same as for > TypeReferenceMatch#getOtherElements(): There, we needed ILocalVariables as > other elements because that's the only way for clients to get at these > ILocalVariables without building an AST. > > One scenario where this would be useful is if someone needs to collect all > local variables that are annotated with "@Annot(clazz= pack.Test.class)" with > an explicit "class= *". Without MethodReferenceMatch#getOtherElements(), they > would not be able to find out about the variable y that's also annotated. > > But since the API deadline is approaching quickly, we can also defer point 4. > Agreed, let's defer this to 3.5...
(In reply to comment #22) > 3. Issues I found while testing with a breakpoint in NewSearchResultCollector > (you can also deal with these in another bug if you want): > > public class Test { > @Tag Test test1, test2, test3; > void method() { > @Tag Test local= null; > @Tag Test local1, local2, local3; > } > } > @interface Tag {} > > * References to type Tag: > Expected (e= getElement(); le= getLocalElement(); oe= getOtherElements()): > - 1: e= test1; le= @Tag (in test1), oe= {@Tag (in test2), @Tag (in test3)} > => was: oe= null > - 2: e= method; le= ILocalVariable local; oe= null > => was: le= null > - 3: e= method; le= ILocalVariable local1; oe= {ILV local2, ILV local3} > => was: le= null oe= null > Here is the result of my investigation: 1) There's a problem with the annotation visit in the compiler. It currently does not visit the annotation although I guess it should. Philippe, is it OK for you if I modify the traverse(ASTVisitor, BlockScope) methods on Annotation subclasses as follow: MarkerAnnotation: public void traverse(ASTVisitor visitor, BlockScope scope) { if (visitor.visit(this, scope)) { this.type.traverse(visitor, scope); } visitor.endVisit(this, scope); } NormalAnnotation: public void traverse(ASTVisitor visitor, BlockScope scope) { if (visitor.visit(this, scope)) { this.type.traverse(visitor, scope); if (this.memberValuePairs != null) { ... } } visitor.endVisit(this, scope); } SingleMemberAnnotation: public void traverse(ASTVisitor visitor, BlockScope scope) { if (visitor.visit(this, scope)) { this.type.traverse(visitor, scope); if (this.memberValue != null) { this.memberValue.traverse(visitor, scope); } } visitor.endVisit(this, scope); } Null check on this.type does not seem necessary as all constructors senders for these classes provide a non-null value for the type reference... 2) After having fixing the compiler issue + problems with other elements, I get now: * References to type Tag: - 1: e= test1; le= @Tag (in test1), oe= {IF test2, IF test3} - 2: e= method; le= @Tag (in local); oe= null - 3: e= method; le= @Tag (in local1); oe= {ILV local2, ILV local3} Markus, this is slightly different from what you're expected: a) the objects in the other elements array are ILocalVariable not IAnnotation. Report annotations there does not seem necessary as it would be the same one with only a different parent => we can directly report the parents in this array. b) The local element for 2) and 3) is (an Annotation. You should not expect a ILocalVariable as you're searching for reference to the type Tag... Did I miss something here? If you agree with this results, then point 4) can also be fixed in 3.4 :-). Otherwise, revert the change to currently report null and postpone the discussion about this behavior to 3.5...
(In reply to comment #26) > If you agree with this results, then point 4) can also be fixed in 3.4 :-). > Otherwise, revert the change to currently report null and postpone the > discussion about this behavior to 3.5... > Forget this remark, I mixed the points. Point 4) discussion definitely needs to be postponed to 3.5...
Created attachment 93191 [details] Proposed patch fixing comment 22 issues This new patch includes changes described at comment 25 and comment 26. It passes all JDT/Core and JDT/UI tests, but... now fails to fix bug 209778 as well. The key point is other elements returning null or not on comment 22 example 2). There are 3 possibilities for the object returned by getOtherElements() in this peculiar case: 1) return null, then bug 209778 will be fixed :-) 2) return an array of ILocalVaribale (comment 26 point 2)), then bug 209778 needs a change in JDT/UI code to be fixed 3) return an array of IAnnotation, then the fix needs to be done in JDT/Core (not trivial => M7) Personally my preferences order would (obviously) be: first 2), then 1) and 3) at last...
(In reply to comment #28) Now, the protected ReferenceMatch#localElement() is an API method in subclasses of ReferenceMatch, but it should not be. > The key point is other elements returning null or not on comment 22 example 2). Sorry, my examples in comment 22 point 3 were indeed screwed. The local and other references in ex. 2 and 3 should have pointed to IAnnotations, not ILocalVariables. > There are 3 possibilities for the object returned by getOtherElements() in this > peculiar case: > 1) return null, then bug 209778 will be fixed :-) => Good for M6, but would not really fix bug 209778. > 2) return an array of ILocalVaribale (comment 26 point 2)), then bug 209778 > needs > a change in JDT/UI code to be fixed => I wouldn't do this mixture. 'Local' elements are now defined as the inner-most elements, and it would be strange if getOtherElements() would not return inner-most elements now (the existing Javadoc of TypeReferenceMatch explicitly talks about local elements). Better to have null for M6. > 3) return an array of IAnnotation, then the fix needs to be done in JDT/Core > (not trivial => M7) => The way to go for M7.
(In reply to comment #29) > > Now, the protected ReferenceMatch#localElement() is an API method in subclasses > of ReferenceMatch, but it should not be. > I think there's no perfect solution here. With this one, I agree that client can call localElement(). So, I suggest to say that ReferenceMatch is not intended to be subclassed by clients and then, they will get a "Discouraged access" warning telling them they should not use this method if they call it in a subclass of *ReferenceMatch. Would it be acceptable? > > The key point is other elements returning null or not on comment 22 example 2). > > Sorry, my examples in comment 22 point 3 were indeed screwed. The local and > other references in ex. 2 and 3 should have pointed to IAnnotations, not > ILocalVariables. > OK, I'll implement solution 1) for M6 and we see what we will do for M7...
Created attachment 93215 [details] Last proposed patch for M6 This last patch includes the last comments remarks: 1) Warn clients that ReferenceMatch is not intended to be subclassed 2) comment 28 solution 1)
I released the last patch for 3.4M6 in HEAD stream to have it in today's warm-up build. We'll tune it next week if necessary...
Verified for 3.4M6 using build I20080324-1300
