Community
Participate
Working Groups
When an annotation can only target one kind of thing (e.g., a type) but is used to modify something else in a pointcut (e.g., a method), it would be nice if the compiler emitted an error, since the two situations can be confusingly similar: call(@Nice * *(..)) // method call((@Nice *) *(..)) // return type --------------------------------------------- import java.lang.annotation.ElementType; import java.lang.annotation.Retention; import java.lang.annotation.RetentionPolicy; import java.lang.annotation.Target; public class AnnotationTypePatternMistakes { @Retention(RetentionPolicy.RUNTIME) @Target({ElementType.TYPE}) @interface I{} @I() class C{} static aspect A { // would like a CE here - I only on types pointcut pc() : execution(@I * *(..)); declare error : pc() : "hi"; } }
take a look... start at WildTypePattern.resolveBindings() which converts all patterns to concrete ExactTypePatterns in situations like this..
The fix has to go in to SignaturePattern.resolveBindings(..) rather than lower down because in the case of the annotation pattern being non-null i.e you have a pointcut of the form @MyAnnotation * *(..) then you need to know the fact that you're interested in methods when you check whether @MyAnnotation has an @Target annotation which is ElementType.METHOD (if no @Target is specified then the default is that it applies to everything). There is then the cases for incorrect annotation types in the other positions within the signature pattern, for example * (@MyAnnotation *).*(..) - declaring type * *(@MyAnnotation *) - parameter type (@MyAnnotation *) *(..) - return type * *.*(..) throws (@MyAnnotation *) - throws pattern (required) * *.*(..) throws !(@MyAnnotation *) - throws pattern (disallowed) In these cases, all that needs to be checked is whether the patternNode has a @Target annotation on it which is either not the default or not ElementType.TYPE (as these are the only ones allowed). Again, I believe that the logic should be contained in the SignaturePattern class rather than lower down because the different implementations of resolveBindings(..) can be called from various places and placing a check on the targetKind might result in an xlint warning being recorded when it shouldn't. Unfortuntately, I can't think of a testcase for this :-( The overall fix is to mirror the implementation of working out whether runtime retention is there by adding a method "String getTargetType()" to the ReferenceTypeDelegate interface and the corresponding implementations in the four classes which implement this (excluding the AbstractReferenceTypeDelegate class). Then, in SignaturePattern.resolveBindings(..) after each PatternNode has been resolved, check whether it has the correct target kind. The drawback of putting the fix in SignaturePattern is that not all pointcuts are resolved via this route, for example handler(@TypeAnnotation *) however, I'm leaving the implementation for these scenarios until someone raises it as a bug since the proposed fix covers the scenario under which this bug was raised. I believe though, a similar implementation to the one proposed here would need to be applied in this case.
Created attachment 30135 [details] testcase patch Apply this patch to the tests project. Note that this fix means that with testAtField_WrongTargetSource() we now get two xlint messages which has meant that updates have had to be made to the corresponding place in ajc150.xml.
Created attachment 30136 [details] zip containing fix This zip contains two patches which make up the proposed fix for this bug: * pr115252-org-aspectj-ajdt-core-patch.txt - apply to the org.aspectj.ajdt.core project * pr115252-weaver-patch.txt - apply to the weaver project
The previously attached patches need to be ammended for a couple of reasons. Firstly, the scenario when an annotation has more than one @Target annotations isn't covered (which requires updates to both the tests and proposed fixes). Secondly, it is nicer to deal with AnnotationTargetKind's (which is a newly created TypeSafeEnum which lives better in its own class rather than as an inner class) rather than Strings.
Created attachment 30220 [details] updated testcase patch Updated testcase patch including one extra test which has annotations that have more than one @Target annotation. Apply patch to the tests project.
Created attachment 30222 [details] zip containing updated proposed fix Zip containing updated proposed fix as discussed in comment #5. This zip contains two patches which make up the proposed fix for this bug: * pr115252-org-aspectj-ajdt-core-patch2.txt - apply to the org.aspectj.ajdt.core project * pr115252-weaver-patch2.txt - apply to the weaver project
patches integrated - thanks Helen. Waiting on build.
fix available.
