Bug 128664 - [annotations] Incorrect/asymmetric semantics of inherited annotation for interface vs. class
Summary: [annotations] Incorrect/asymmetric semantics of inherited annotation for inte...
Status: NEW
Alias: None
Product: AspectJ
Classification: Tools
Component: Compiler (show other bugs)
Version: DEVELOPMENT   Edit
Hardware: PC Windows XP
: P3 normal (vote)
Target Milestone: ---   Edit
Assignee: aspectj inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2006-02-20 10:31 EST by Ramnivas Laddad CLA
Modified: 2013-06-24 11:05 EDT (History)
1 user (show)

See Also:


Attachments
patch for matcher (75.28 KB, patch)
2008-12-09 14:25 EST, Andrew Clement CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ramnivas Laddad CLA 2006-02-20 10:31:36 EST
Pointcut of the form "execution((@InheritedAnnotation *).new(..))" matches
ctors of classes extending a class with the @InheritedAnnotation, but
not of implementing an interface with the same annotation.

Note that changing type pattern to "(@InheritedAnnotation *)+" doesn't change the situation.

(BTW, I got VerfiyError with the same code after some incremental compilation,
      but couldn't reproduce despite making several attempts.)

Here is the test case that illustrates the problem:

import java.lang.annotation.Inherited;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;

import junit.framework.TestCase;

public class InheritedAnnotationPointcutTest extends TestCase {
	public void testCtorExecutionForInterfaceInheritingClass() {
		MonitorMyAnnotationExecution.aspectOf().executionCount = 0;
		new ClassImplementingInterfaceWithInheritedAnnotation();
		assertEquals(1, MonitorMyAnnotationExecution.aspectOf().executionCount);
	}

	public void testCtorExecutionForClassExtendingClass() {
		MonitorMyAnnotationExecution.aspectOf().executionCount = 0;
		new ClassExtendingClassWithInheritedAnnotation();
		// Expecting 2, one for derived and one for base class ctor execution
		assertEquals(2, MonitorMyAnnotationExecution.aspectOf().executionCount);
	}
}

@Retention(RetentionPolicy.RUNTIME)
@Inherited
@interface InheritedAnnotation {
}

@InheritedAnnotation
interface InterfaceWithInheritedAnnotation {
}

class ClassImplementingInterfaceWithInheritedAnnotation implements InterfaceWithInheritedAnnotation {
}

@InheritedAnnotation
class ClassWithInheritedAnnotation {
}

class ClassExtendingClassWithInheritedAnnotation extends ClassWithInheritedAnnotation {
}

aspect MonitorMyAnnotationExecution {
	int executionCount;
	
	before() : execution((@InheritedAnnotation *)+.new(..)) {
		executionCount++;
	}
}
Comment 1 Andrew Clement CLA 2006-02-20 14:51:23 EST
From the javadoc for java.lang.annotation.Inherited:

---
(@Inherited) Indicates that an annotation type is automatically inherited. If an Inherited meta-annotation is present on an annotation type declaration, and the user queries the annotation type on a class declaration, and the class declaration has no annotation for this type, then the class's superclass will automatically be queried for the annotation type. This process will be repeated until an annotation for this type is found, or the top of the class hierarchy (Object) is reached. If no superclass has an annotation for this type, then the query will indicate that the class in question has no such annotation.

Note that this meta-annotation type has no effect if the annotated type is used to annotate anything other than a class. Note also that this meta-annotation only causes annotations to be inherited from superclasses; annotations on implemented interfaces have no effect. 

---
The last sentence seems to suggest we are doing the right thing I think...
Comment 2 Ramnivas Laddad CLA 2006-02-20 15:54:37 EST
I did not know about difference in treatment for interface-inherited
annotations vs. class-inherited annotations. Weird, but clearly stated. 
I guess they wanted to avoid issues with multiply-inherited annotations 
(conflicting annotations and properties from different interfaces 
implemented by a class, for example).

It seems, however, that the version with a '+' i.e. 
"execution((@InheritedAnnotation *)+.new(..)))" should make the tests pass
(it doesn't), since the "@InheritedAnnotation *" should match the 
InterfaceWithInheritedAnnotation type, making the pointcut should be logically equivalent to "execution(InterfaceWithInheritedAnnotation+.new(..))".
Comment 3 Andrew Clement CLA 2006-03-21 05:52:31 EST
i think the '+' case ramnivas indicates in his last paragraph ought to work.
Comment 4 Andrew Clement CLA 2007-10-23 12:15:11 EDT
yikes - is this still broke?
Comment 5 Andrew Clement CLA 2008-12-04 12:25:50 EST
First problem here is that the + is lost on resolution of the pattern:

(@InheritedAnnotation *)+

- that is because the * is considered to match everything so we 'forget' the +, even though the annotation is also there.  If we change it such that the + is persisted, then we currently end up matching far too much - argh!

Comment 6 Andrew Clement CLA 2008-12-04 14:45:29 EST
OK, I have a working solution.  It also triggered another testcase from ajc150.xml to fail - turns out it was inadvertently testing something like this but ignoring that the matching failed (it was only testing the parsing). 

the bad news is that it affects the serialized form (we need to persist that there was a + on the end).  This means increasing the version number of all the attributes.  That will mean a bunch of user problems where they upgrade to 1.6.3 but fail to do so everywhere - so they'll build code with 1.6.3 then attempt to use it with a 1.6.2 weaver and it will fail.  I'm currently weighing up the cost of that vs fixing this now.  I do have another bug whose fix also needs a version change - so it might be worth doing it now.
Comment 7 Andrew Clement CLA 2008-12-09 14:25:37 EST
Created attachment 119950 [details]
patch for matcher

this is the patch to address this.  It modifies the serialized form (but doesnt have the right version check guards on that code yet).
Comment 8 Andrew Clement CLA 2008-12-09 14:26:45 EST
not changing the serialized form for 1.6.3 - so deferring this bug.
Comment 9 Andrew Clement CLA 2013-06-24 11:05:07 EDT
unsetting the target field which is currently set for something already released