Community
Participate
Working Groups
20060110 A new types IResolvedAnnotation and IResolvedMemberValuePair has been added to the AST. It basically represents a new kind of binding that group the bindings of an annotation and the agruments of an annotation. IMO this might be handy, but doesn't really fit in the existing style of the AST. The existing bindings are enough tor represent the information required. e.g. annotation.resolveTypeBinding() gives you directly access to the binding of the annotation. To simplify the access to the bindings of the attributes, why not add a method annotation.resolveAnnotationArguments() : IMethodBinding[] and for the values a API annotation.resolveAnnotationValues() : Object[] (I'm not sure if the annotation values really help much here, they are only in vary rare cases configured) resulting in no new types.
Oops, I didn't realize what's the main reason for this new type: To give access to the annotations when having a IBinding (IBinding.getAnnotations()). It's strange that this is not a IBinding, but I guess adding a new binding might be highly breaking. I also think that IResolvedMemberValuePair could be folded in IResolvedAnnotation. All information about the value name can be accessed through the methods of the annotations ITypeBinding (IResolvedAnnotation.getAnnotationType().getDeclaredMethods())
A couple of additional comments: - all types in the DOM that provide resolved information currently have binding it their names. IMO even if this class can't be made a binding we should put binding into its name. - the pair Annotation definition and annotation reference has IMO similarities with generic and parameterized types. For generic/parameterized types all information is available through ITypeBinding. What was the underlying reason for coming up with a new class IResolvedAnnotation and not "merging" it into the current type binding. - if we want to have this class separately from bindings then we should think about creating an interface above binding which is then implemented by IResolvedAnnotation as well. Otherwise clients can't deal with all resolved items in a DOM polymorphical anymore. Thoughts ?
More comments: (1) The identity and equality relationships between two IResolvedAnnotations should be documented (same for IResolvedMemberValuePair). (2) IMethodBinding's new query IResolvedAnnotation[] getParameterAnnotations(int paramIndex) does not fit the style of the rest of the API. It answers a question about an IVariableBinding, and it's strange that this is offered on IMethodBinding. If the query is really necessary on IMethodBinding, it should rather be IResolvedAnnotation[][] getParameterAnnotations(). A cleaner approach would be to offer IVariableBinding[] getParameters() on IMethodBinding, from where clients can easily ask IVariableBinding#getAnnotations() for each parameter.
(In reply to comment #3) > A cleaner approach would be to offer > IVariableBinding[] getParameters() > on IMethodBinding, from where clients can easily ask > IVariableBinding#getAnnotations() for each parameter. The problem with this solution is that there is no such thing as getParameters() on IMethodBinding that would return IVariableBinding[]. The reason for that is that the method binding doesn't know anything about the parameters' names. IVariableBinding would need to provide getName(), ... A method binding simply knows about the parameter types.
(In reply to comment #2) > - all types in the DOM that provide resolved information currently have > binding it their names. IMO even if this class can't be made a binding we > should put binding into its name. I never liked this idea of having a binding for a name. The name itself means nothing. The binding only has a meaning according to the context. So it is a good thing to retrieve the binding using the name's parent. > - the pair Annotation definition and annotation reference has IMO similarities > with generic and parameterized types. For generic/parameterized types > all information is available through ITypeBinding. What was the underlying > reason for coming up with a new class IResolvedAnnotation and not "merging" > it into the current type binding. Are you suggesting to move all the methods from IResolvedAnnotation to ITypeBinding? > - if we want to have this class separately from bindings then we should think > about creating an interface above binding which is then implemented by > IResolvedAnnotation as well. Otherwise clients can't deal with all resolved > items in a DOM polymorphical anymore. Would not it be an API breakage to do so? Tim, any comment regarding the second question?
>> - the pair Annotation definition and annotation reference has IMO similarities >> with generic and parameterized types. For generic/parameterized types >> all information is available through ITypeBinding. What was the underlying >> reason for coming up with a new class IResolvedAnnotation and not "merging" >> it into the current type binding. >Are you suggesting to move all the methods from IResolvedAnnotation to >ITypeBinding? Something other than the type binding is needed because the values are needed. It is also neccessary to couple the values to the the member. This seems like the logical representation to me.
In reply to comment #5: 1.) May be a misunderstanding: my suggestion is to name the class something like IAnnotationReferenceBinding instead of IResolvedAnnotation to make clear it is an type from the binding world. 2.) Yes, one of my first thought was "Why isn't this a ITypeBinding". It could have a method isAnnotationReference. If true then the analogous methods currently available on IResolvedAnnotation have a meaningful value. I compared this with generic/parameterized types since (although both are declaration) the second is also an "instantiation" of the first. 3.) I don't think so. Added a new superclass is IMO not an API breakage. It is binary compatible but not source comaptible.
Resolved annotations are NOT type bindings, they denote some sort of references to an annotation type, with attributes. The parallel with parameterized types doesn't work as one cannot use an annotation ref as variable type. So in essence, it is not the same. I agree, though, that current naming is confusing. I would suggest: IAnnotationBinding since it is: - a binding - any binding is resolved intrinsically
Internally, I use "attribute" for memberPairValue. Is this something which would help decoding the API ?
We discussed a lot here in the JDT.UI team: Here are our findings: - We agree with Philipps comment 9: this new construct is not related to ITypeBinding. IAnnotationBinding is a good name. Here are our findings and an additional, more lightweigth alternative: a.) we call it a binding (IAnnotationBinding): it won't extend IBinding for compatibility reasons but behaves like one: - identity - a unique key - only there if 100% complete (no bindings for unresolved cases) - mapping from binding to ASTNode (CompilationUnit.findDeclaringNode(binding)) - mapping from nodes to binding (Annotation.resolveAnnotationBinding()) - binding should also provide the containing node (IAnnotationBinding.getAnnotatedBinding), similar like an IMethodBinding.getDeclaringType - if e.g 2 methods are annotated with @X, these 2 annotations have different bindings (reason: CompilationUnit.findDeclaringNode(binding) must return the correct ASTNode) If IAnnotationBinding has a IResolvedMemberValuePair, then also IResolvedMemberValuePair should be a binding, with the same properties b.) we only treat it as a attibute of binding. Similar to IBinding.getModifiers(), you just get some information from a binding (in int), but it isn't one. Name could be 'IAnnotationDescription' - no mapping API from IAnnotationDescription to ASTNode (no CompilationUnit.findDeclaringNode(annotDesc). Instead: CompilationUnit.findDeclaringNode(annotDesc.getAnnotatedBinding()) and find the annotation on the returned ASTNode) - no other way to get that IAnnotationDescription than from the declaring binding (IBinding.getAnnotations(), but no 'annotation.resolveAnnotation()'. Instead clients can use: annotation.resolveTypeBinding().getAnnotaion(name) - idendity not required, no key solution a.) is more or less what we have now: Except of the name, there is no key, you can't access the parent, IResolvedMemberValuePair doesn't follow identity rules, you can't find a IResolvedMemberValuePair in an AST. It also seems to be possible that its parts can be null (APT does a null check after IResolvedAnnotation.getAnnotationType). the weaknesses of this solution are: it's called and behaves like a binding but can't be one (for compatibility reasons): Ugly duplicated APIs: CompilationUnit.findDeclaringNode(IResolvedAnnotation resolvedAnnotation) Annotation.resolveAnnotationBinding -> AST inconsistencies solution b.) is less intrusive but more work for the user to map from an IAnnotationDescription to a ASTNode. We are not aware of a need for this however. As an additional proposal, valid for both alternatives: Merge IResolvedMemberValuePair in IAnnotationBinding IAnnotationBinding getAnnotationType() : ITypeBinding getValues() : Object getValue(key, useDefault) : Object The values are returned in the same order as getAnnotationType().getDeclaredMethods() The name and 'isDefault' can all be accessed through the IMethodBinding getValue(name) : Object is a convenience method to access an value for a given key. 'useDefault' decides if the default value or 'null' will be returned if the annotation has not defined a value.
Why IAnnotationBinding cannot extend IBinding ? When you say "for compatibility reason", I'm not sure to understand with 'what' we have to be compatible with since this is being introduced in 3.2.
I would also consider these to implement IBinding (since they are bindings).
Everybody who assumes that there are exactly 4 diffenrent binding kinds will fail, e.g. in switch statement. There are several of these cases also in our code. I'd be ready to update our code, but I think this change would have to be sold as breaking API change in 3.2. IBinding: /** * Returns the kind of bindings this is. * * @return one of the kind constants: * <code>PACKAGE</code>, * <code>TYPE</code>, * <code>VARIABLE</code>, * or <code>METHOD</code>. */ public int getKind();
How different is that from 3.1 additions for param types, wildcards, type parameters etc... ?
For all these changes we introduced the new AST level (AST.JLS3) so that clients have to migrate.
(In reply to comment #13) > Everybody who assumes that there are exactly 4 diffenrent binding kinds will > fail, e.g. in switch statement. > There are several of these cases also in our code. > I'd be ready to update our code, but I think this change would have to be sold > as breaking API change in 3.2. I don't see how this would be an API breakage. If we add new constants, the existing users would not miss any new or old bindings. They would still treat all the existing bindings. The new constants would point to instances of the new binding types. The only problem I am seeing is for user who add a default case statement in switch statement. Those default could not be reached. But since this wasn't specified before, this is not really an API breakage. The problem is more for users that used a default for the last sets of bindings. For example: switch(binding.getKind()) { case IBinding.PACKAGE : ...; break; case IBinding.TYPE : ....; break; case IBinding.VARIABLE : ... break; default: // Assuming IBinding.METHOD .... } We could document this one. You are the one that requested a more consistent model. So this looks like a fair trade-off.
If everybody agrees with Olivier's opinion, JDT would be glad to update their code to not make sure nobody makes any assumptions of the possible bindings kinds. Jim, what's your opinion here?
I also agree with Olivier. Note we had the same dilema in the Java model when adding new IJavaElements. But experience shows that clients are doing the right thing since nobody reported a problem because of these new IJavaElements.
I agree with comment #13 that this must be considered a breaking API change. But I also agree with comment #16 that the real risk of breaking existing clients is low. So I'd be ok with making this API change even though it is breaking (there would need to be a 3.2 migration guide entry explaining the change and its impact). While updating the spec for IBinding.getKind() to include the new constant, the spec could warn clients that additional binding types might be added in the future, so that they can program defensively.
Just to make sure we agree on the implications of these new IAnnotationBindings: I would expect that e.g. SimpleName#resolveBinding() would then return such an IAnnotationBinding for the name of an Annotation node. And if IMemberValuePair is not merged into IAnnotationBinding, then we should have MemberValuePair#resolveMemberValuePair() returning an IMemberValuePairBinding, etc. But I think Martin's proposal to drop IMemberValuePairBinding would be a cleaner API anyway.
Regarding Markus' comment 20 about SimpleName#resolveBinding() on the annotation name: For compatibility reasons I would not change this behaviour, but let it return the type binding of the annotation type. To get the annotation binding use Annotation.resolveAnnotationBinding() We have a similar case for the constructor invocation: the binding on the name is the type binding. 'ConstructorInvocation.resolveConstructorBinding' gives you the method binding.
Created attachment 34415 [details] Patch for jdt.core projects
Created attachment 34416 [details] Patch for jdt.apt projects
I everyone agrees with these changes, we need to coordinate with APT to release the 2 patches at the same time so as to not break a build. Jess please let me know when it is a good time for you. Idealy we would like to do it before the end of the week so that we're ready for the M5 builds.
May I suggest the following: rename 'Annotation.resolveAnnotation' to 'Annotation.resolveAnnotationBinding' This would be conistent with all other resolve methods, they always are called resolveXXBinding CompilationUnit.findDeclaringNode(IAnnotationBinding annotationBinding) not required (it is covered by CompilationUnit.findDeclaringNode(IBinding binding)) getAllMemberValuePairs must not create new elements every time. Bindings must be idendity comparable I still suggest to simplify and get rid of IMemberValuePairBinding. The annotation type returned with getAnnotationType gives you all the information you need about member value pairs (name and default). IAnnotationBinding getAnnotationType() : ITypeBinding getValues() : Object[] null if value is inherited from default getValue(key, useDefault) : Object
Jerome - your patch to APT looks good. You're welcome to apply it directly to APT for the M5 build, or I can do so once you've made the change to jdt.core. Anytime today or tomorrow would be great.
Thanks for the suggestions Martin. However I still think we should keep IMemberPairValueBinding to be consistent since we have the AST node MemberPairValue. I'll provides new patches shortly.
Created attachment 34435 [details] Improved patch for jdt.core projects
Created attachment 34436 [details] Improved patch for jdt.apt projects
Commited both patches to HEAD.
You should add this to the javadoc of IBinding#getJavaElement(): * For annotations, this method returns the Java element of the annotation type. And there are still some "missing links" - MemberValuePair#resolveMemberValuePairBinding(): IMemberValuePairBinding - IMemberValuePairBinding#getDeclaringAnnotation(): IAnnotationBinding - IAnnotationBinding#getDeclaringBinding(): IBinding CompilationUnit#findDeclaringNode(IBinding) promises to return a MemberValuePair for a member value pair binding. What can clients expect if they pass an IMemberValuePairBinding for a default value or for a SingleMemberAnnotation's value?
re:- MemberValuePair#resolveMemberValuePairBinding(): IMemberValuePairBinding Sounds fair. re:- IMemberValuePairBinding#getDeclaringAnnotation(): IAnnotationBinding An annotation is homogenous to a paramType or a message send. Arguments are not connected to the parent container. So I wouldn't add this one. re:- IAnnotationBinding#getDeclaringBinding(): IBinding What is that ? Isn't it a dup of #getAnnotationType() ?
Thanks for your comment Markus. - Updated getJavaElement() spec as suggested. - Added MemberValuePair#resolveMemberValuePairBinding() - Updated CompilationUnit#findDeclaringNode(IBinding) spec to say that null is returned in the default value and single member cases
> re:- IMemberValuePairBinding#getDeclaringAnnotation(): IAnnotationBinding > An annotation is homogenous to a paramType or a message send. Arguments are > not connected to the parent container. So I wouldn't add this one. I disagree. An IMemberValuePairBinding has a unique parent, and there should be a query to get that parent. We already have this for all other containment relations, namely ITypeBinding#getPackage() and IType/Variable/MethodBinding#getDeclaringClass/Method(). typeArgument -> parameterizedType and parameterType -> methodBinding are not one-to-one mappings, and therefore it's clear that there's no link to a parent. > re:- IAnnotationBinding#getDeclaringBinding(): IBinding > What is that ? Isn't it a dup of #getAnnotationType() ? No, like IType/Variable/MethodBinding#getDeclaringClass/Method(), this would return the parent binding that has this IAnnotationBinding attached, i.e binding.getAnnotations() would contain the receiver.
Sorry, I think I accidentally removed the target milestone - resetting to 3.2 M5. I filed bug 127462 for the enhancement requests from comments 32 and 34.
Verified for 3.2 M5 using build I20060214-0010.
