Community
Participate
Working Groups
Follow up of bug 333919. See bug 333919 comment 2 and bug 333919 comment 3.
Jay, please investigate.
In the case of binary method, it seems fairly straight forward since IBinaryMethod#getParameterAnnotations already have the required implementation and can be used in the new API implementation. But for source types, it might require some additional work. Looking at what can be done.
A general "ILocalVariable[] getParameters()" in IMethod also wouldn't hurt. We already have an analogous "ITypeParameter[] getTypeParameters()". However, I don't have a concrete use case for the additional information (source ranges, flags) right now, so a specialized API is also OK for me if the general getParameters() is too costly.
Created attachment 188592 [details] Draft patch API added in IMethod. The patch also contains the implementation + tests. Will update after running all existing tests. --------- /** * Returns an array of annotations for the parameter at the given index. * * @param index the index or order of the method parameter. * @return the array of annotations for the specified parameter * @throws JavaModelException * @since 3.7 */ IAnnotation[] getParameterAnnotations(int index) throws JavaModelException; ---------------- I don't know if it would be better if the API returns all annotations instead of a specific parameter's. Markus/Olivier, what do you think?
Created attachment 188599 [details] Updated patch The previous patch had a bug. This one fixes it.
Created attachment 188610 [details] Hack for JavaElement view I quickly tested the updated patch by hacking the JavaElement view [1]. With the hack, annotations show up as children of PARAMETER NAMES. I saw that most of the queries don't work and the annotation element does not exist. Furthermore, the API with the index parameter looks odd. No other IJavaElement has such an API. Is there anything speaking against "ILocalVariable[] getParameters()"? You have to create the ILocalVariables anyway because they are the parents of the annotations. And this API would perfectly fit the existing story. [1] repo: :pserver:anonymous@dev.eclipse.org:/cvsroot/eclipse module: jdt-ui-home/plugins/org.eclipse.jdt.jeview
(In reply to comment #6) > Is there anything speaking against "ILocalVariable[] getParameters()"? I was wary of making ILocalVariable part of an API since ILocalVariable has been labeled as pseudo-element as per the API doc. "In particular such a pseudo-element should not be used as a handle. For example its name range won't be updated if the underlying source changes." Looking at the code, we don't seem to bother about local variables being part of the element hierarchy. But if we intend to make it part of an API, we will have to fix that.
Or how about the API just returning an array of strings, just like IMethod#getParameterNames and IMethod#getParameterTypes?
> Looking at the code, we don't seem to bother about local variables being part > of the element hierarchy. But if we intend to make it part of an API, we will > have to fix that. Definitely no. Just look at IMethod#getTypeParameters() an make it analogous. In hindsight, IParent#getChildren() was not a great API, since clients (e.g. UI) tend to rely on the current set of children, and not all clients deal gracefully with new elements added to the children. That's why we didn't extend the getChildren() in the past, but added separate queries for new kinds of children. > Or how about the API just returning an array of strings, just like > IMethod#getParameterNames and IMethod#getParameterTypes? Why do you think clients are only interested in the annotation names but not in the member-value pairs (or the source positions)?
As Markus, I agree that this API should retrieve the full flavor annotations. Not just the names. We need the equivalent of ((IAnnotatable)method).getAnnotations(), but in a way that makes it easier to be retrieved than using the workaround suggested in bug 333919 comment 2.
Created attachment 188895 [details] Modified patch This patch seem to be better. The parameter name, when selected in the editor, appears as a LocalVariable in the JavaElements view. And the annotations do appear under parameter names. I am not sure what queries I need to test here. The patch also includes the modified hack to org.eclipse.jdt.jeview.views.JavaElement. Markus, please let me what other tests I can use to check the completeness of the fix. As for your request for API returning ILocalVariable themselves, I feel it might involve more effort.
I found several issues with the patch in binary mode. I am looking at it. Jay, I'll ask you to review a patch later today or tomorrow.
(In reply to comment #11) > As for your request for API returning ILocalVariable themselves, I > feel it might involve more effort. I don't understand that. You need to have fully resolved ILocalVariables anyway, since these are the parent elements of the annotations. So I don't see the additional effort. On the other hand, an API that can return IAnnotations but does not give direct access to all parameter ILocalVariables looks pretty lame.
(In reply to comment #13) > I don't understand that. You need to have fully resolved ILocalVariables > anyway, since these are the parent elements of the annotations. So I don't see > the additional effort. On the other hand, an API that can return IAnnotations > but does not give direct access to all parameter ILocalVariables looks pretty > lame. BinaryMethod don't have ILocalVariables. So I don't think how we can have an API that is exposing ILocalVariable on IMethod that would work for source and binary methods.
Created attachment 189128 [details] Proposed fix + regression tests I haven't checked the way it works with the ASTView, but regression tests look good. I also believe that for consistency with other methods defined on IMethod an empty array should be returned when the method has no parameters. Jay, please review the changes.
> Markus, please let me what other tests I can use to check the completeness of the fix. Focus the JavaElement View on the local variable (using first toolbar button). Then navigate down to the annotation and up to the method and compare the annotations on the local var with the annotations from the new API: - select two items and choose context menu > Compare with Each Other... - compare their children - compare the info in the Properties view
> BinaryMethod don't have ILocalVariables Only BinaryMethods without source attachments. A problem that is bleeding through in the last patch is that the parent element of a parameter annotation in a BinaryMethod is the BinaryMethod, not the local variable. This contradicts the general contract of getParent(), and it is inconsistent with the correct parent I see when I look at the annotations of an ILocalVariable from a BinaryMethod with source attachment). [This is hopefully obsolete now:] I checked the new proposed API for consistency with other APIs, and I didn't find any other jdt.core APIs which return a [][] (except for char[][]). If for any reason we need to go with the getParameterAnnotations() API, then the indexed API would be better, since it would match IMethodBinding#getParameterAnnotations(int).
(In reply to comment #17) > Only BinaryMethods without source attachments. A problem that is bleeding > through in the last patch is that the parent element of a parameter annotation > in a BinaryMethod is the BinaryMethod, not the local variable. I don't see how we can come up with an API that would be different if the binary methods has source attachment or not. > This contradicts the general contract of getParent(), and it is inconsistent > with the correct parent I see when I look at the annotations of an > ILocalVariable from a BinaryMethod with source attachment). Please let me know how you do this. > [This is hopefully obsolete now:] I checked the new proposed API for > consistency with other APIs, and I didn't find any other jdt.core APIs which > return a [][] (except for char[][]). If for any reason we need to go with the > getParameterAnnotations() API, then the indexed API would be better, since it > would match IMethodBinding#getParameterAnnotations(int). All methods to retrieve annotations right now are returning empty arrays. In this case we should go with the indexed version. At least it would be consistent for both types of methods: source and binary. What would be an API that returns local variables for binary methods without source attachment ? Before we do more work on this, we need to agree on the API contract, otherwise it is a waste of time.
(In reply to comment #18) > > This contradicts the general contract of getParent(), and it is inconsistent > > with the correct parent I see when I look at the annotations of an > > ILocalVariable from a BinaryMethod with source attachment). > Please let me know how you do this. - have the JavaElement view installed - open a binary class with source attachments and select the name of an annotated parameter - open the JavaElement view - click the leftmost view toolbar button (codeSelect) - expand the ANNOTATIONS node and check the children of the first annotation
> What would be an API that returns local variables for binary methods without > source attachment ? See comment 3. Like for ITypeParameters of binary members without source attachments, the source range and name range would of course be invalid, and getFlags() can probably not return "final" if source is not available.
(In reply to comment #20) > - have the JavaElement view installed > - open a binary class with source attachments and select the name of an > annotated parameter > - open the JavaElement view > - click the leftmost view toolbar button (codeSelect) > - expand the ANNOTATIONS node and check the children of the first annotation This is the local variable defined by the SelectionEngine. I'll see how I can "insert" such local variables inside the current patch. When using directly the IMethod, a binary method doesn't use its source attachment to return parameter annotations. I'll see what I can do. But as you say, the modifiers/positions of this local variable would be wrong for a binary method even if there is a source attachment.
(In reply to comment #21) > But as you say, the modifiers/positions of this local variable would be wrong > for a binary method even if there is a source attachment. I only said the ranges can't be available if there's no source. If source is available, the similar IType#getTypeParameters() resolves correct name/source ranges, but the bounds are from the class file. I would expect the same from ILocalVariable (ranges from source, but flags and annotations from class file).
Created attachment 189195 [details] Proposed fix + regression tests new patch that defines the following API: ILocalVariable[] getParameters() on org.eclipse.jdt.core.IMethod. Markus, let me know if this meets all your criteria.
Created attachment 189196 [details] Proposed change for JavaElementView code Updated change to match the new API.
(In reply to comment #22) > I only said the ranges can't be available if there's no source. If source is > available, the similar IType#getTypeParameters() resolves correct name/source > ranges, but the bounds are from the class file. I would expect the same from > ILocalVariable (ranges from source, but flags and annotations from class file). This only works if the corresponding elements exists in the model. By definition this is not true for local variable element.
(In reply to comment #24) Looks pretty good. Apart from the missing source ranges in binaries with source, I found a problem with the following path in the JE view: BinaryMethod -> PARAMETERS -> LocalVariable -> ANNOTATIONS -> Annotation -> PARENT: The parent is the BinaryMethod, but it should be the LocalVariable. Looks like the problem is that BinaryMethod#getAnnotations(..) calls Util.getAnnotation(this, ...), but it should pass the new local variable instead of 'this'.
Created attachment 189202 [details] Updated patch for JavaElement View Source methods are good. For binary methods with source, getParameters()[0].getFlags() does not include "final", but the element from codeSelect also extracts this from source.
Created attachment 189233 [details] Proposed fix + regression tests I think this patch handles all remaining issues. Local variables from binary with source attachment has source range, name ranges and flags. Markus, please give it a try. If ok, I'll release tomorrow.
Parent chains are good now and parameters in binaries with source also have correct source ranges. Only the annotations on such parameters are still missing their source range. I saw that the annotation offsets are already available in SourceMapper#enterAbstractMethod(MethodInfo) in methodInfo.annotations. They should also be stored and later returned in SourceMapper#get*Range(..). Copy/paste error in SourceMapper#enterAbstractMethod(MethodInfo): Second "// type parameters" comment is wrong. The new constructor of LocalVariable is a bit strange, since the only possible argument to the "IAnnotation[] annotations" parameter is Annotation.NO_ANNOTATIONS. I would remove the new constructor and just use the old one (with "null" as argument).
(In reply to comment #29) > The new constructor of LocalVariable is a bit strange, since the only possible > argument to the "IAnnotation[] annotations" parameter is > Annotation.NO_ANNOTATIONS. I would remove the new constructor and just use the > old one (with "null" as argument). Passing null will require another method call to convert it to Annotation.NO_ANNOTATIONS. So I plan to keep the new constructor.
Olivier showed me that existing binary annotations (on method declaration) also don't have any positions, so I don't think this is an important feature any more. Feel free to forget this request or move it to RESOLVED/LATER. Go for the last patch, although I would still remove the second LocalVariable constructor to make the code more readable, unless we have numbers that prove the performance impact. I'd say a decent JIT should inline that method invocation anyway.
Created attachment 189390 [details] Proposed fix + regression tests
Released for 3.7M6.
Verified for 3.7M6 using build I20110307-0800.
Reverified for 3.7 RC0 using Build id: I20110428-0848
