Community
Participate
Working Groups
Some users are declaring public ITDs with annotations on and then, post weaving, another tool is consuming the annotations on the members created in the target type. We don't currently copy the annotations from the ITD that exists in the aspect to the public member created in the target type - this needs fixing.
Fixed the case for public ITD'd methods onto a class. I have not tested/fixed these other 'interesting' cases: - public ITD method with no annotation that has declare @method annotation on it. - public ITD field with annotation on it. - public ITD field with no annotation that has declare @field annotation on it. - public ITD method with annotation on it which is made onto an interface. - public abstract ITD method with annotations on it. I also wonder if there are bugs lurking in our matching on ITDs using annotation patterns when the ITDs are either abstract or made onto an interface - a quick look seems to suggest 'holder' methods for the annotations aren't being produced by the compiler, which would cause the annotations to disappear... I don't propose to look at these cases for M3 but will do before 1.5.0 final.
Created attachment 22786 [details] tests for the above five cases All five cases currently fail... in the first case the annotation can by seen with runtime "@annotation(a)" matching in a pointcut, but not through java.lang.reflect.
fixed case 4...
Why only *public*? If there is an ITD for a methods -- of any scope -- with annotations, I would expect those annotations to exist on the target. (But I will admit that just now I have only found a use case for public scope.)
public ITD members are the only ones added 'as-is' to the target type - if you write public void A.m() then type A will get a public member 'm'. Other ITD forms with 'default' or 'private' visibility are added as mangled members that really shouldn't be getting looked-at by tools searching for members - these members have names starting ajc$ indicating they are artifacts of the AJ compiler, effectively they are synthetic members, we just haven't tagged them as such yet. There could be use cases for the annotations to get copied across in those cases - but I can't think of any. I raised this bug to cover the cases that I think do have reasonable use cases right now, doesn't mean we wont fix the others later... Copying annotations around is not free, it impacts performance of the compiler as annotations have to be discovered - all the other information about a member is captured in its signature. If there is a reason to copy them, then we will.
fixed case 5, broken case 4.. can't remember how I got case 4 working previously
case 4 and 5 are now both working...
2,4 and 5 working.. just 1 and 3 to go
hmm... my fix for case2 causes annotations to be put on twice if it's a non-ITD field.. I think that in order for case2 to be fixed properly I'll need to fix a "case 6" - public non-ITD field with declare @field annotation on it
Now it looks like there are about 26 cases, not even looking at static fields and methods (that would double the number of cases again). By annotated I mean it has an annotation on it rather than declared remotely on it. By ITD-on-itself I mean an aspect declaring an ITD on itself. 1. public method with declare @method 2. public annotated method 3. public ITD method with declare @method 4. public annotated ITD method 5. public ITD-on-itself method with declare @method 6. public annotated ITD-on-itself method 7. public method on an Interface with declare @method 8. public annotated method on an Interface 9. public ITD method onto an Interface with declare @method 10. public annotated ITD method onto an Interface 11. public abstract method with declare @method 12. public abstract annotated method 13. public abstract ITD method with declare @method 14. public abstract annotated ITD method 15. public abstract ITD-on-itself method with declare @method 16. public abstract annotated ITD-on-itself method 17. public abstract method on an Interface with declare @method 18. public abstract annotated method on an Interface 19. public abstract ITD method onto an Interface with declare @method 20. public abstract annotated ITD method onto an Interface 21. public field with declare @field 22. public annotated field 23. public ITD field with declare @field 24. public annotated ITD field 25. public ITD-on-itself field with declare @field 26. public annotated ITD-on-itself field
No wait, missed a few.. now 29 cases 1. public method with declare @method 2. public method on the aspect that declares @method on it 3. public annotated method 4. public ITD method with declare @method 5. public annotated ITD method 6. public ITD-on-itself method with declare @method 7. public annotated ITD-on-itself method 8. public method on an Interface with declare @method 9. public annotated method on an Interface 10. public ITD method onto an Interface with declare @method 11. public annotated ITD method onto an Interface 12. public abstract method with declare @method 13. public abstract method on the aspect that declares @method on it 14. public abstract annotated method 15. public abstract ITD method with declare @method 16. public abstract annotated ITD method 17. public abstract ITD-on-itself method with declare @method 18. public abstract annotated ITD-on-itself method 19. public abstract method on an Interface with declare @method 20. public abstract annotated method on an Interface 21. public abstract ITD method onto an Interface with declare @method 22. public abstract annotated ITD method onto an Interface 23. public field with declare @field 24. public field on the aspect that declares @field on it 25. public annotated field 26. public ITD field with declare @field 27. public annotated ITD field 28. public ITD-on-itself field with declare @field 29. public annotated ITD-on-itself field
Sorry (little fib) to have started you on this crusade Andy, but you seem to be having fun ;-)
Created attachment 23871 [details] tests for the 29 cases 11 fail in my workspace at the moment (suprisingly few!)
Created attachment 24121 [details] fixes all the cases for fields
I've now fixed all of the cases for fields and methods apart from cases 6 and 17 (which are pretty obscure).. I'm going to be working on these two now...
Created attachment 24382 [details] fixes all 29 cases
I've now committed the testcase patch - it isnt wired into the full suite yet.
Patch for the fix includes changes to: LazyClassGen AjcMemberMaker NewMethodTypeMunger NewConstructorTypeMunger BcelClassWeaver BcelTypeMunger I've now done: LazyClassGen patched - added 'replaceField()' AjcMemberMaker patched - added comments on inter**() methods to better explain how they hang together. NewConstructorTypeMunger - commented out the 'getDispatchMethod()' - not used NewMethodTypeMunger - changed the name of getDispatchMethod() to better reflect what it actually returns (now it is getInterMethodBody()) still to do: BcelClassWeaver BcelTypeMunger
Hmmm. Running the new tests with no fix, 21 fail. Applying the patches to the two remaining files causes those two modified files to no longer compile. I had to put in some modifications of my own to get them to compile again - and even then it brings the number of failures down to 6, not to 0. I'm not sure what is missing from the patch? It seems odd that the only patch required to make them work was for weaver - as I seem to need to change org.aspectj.ajdt.core because of some of the changes?
These two comments also concern me from the patch: // ajh02: stupid method added. // this method is only needed because it's still looking on the old annotation // holder (the interMethodBody) when doing pointcut matching for annotated // methods. // ajh02: this is kind of stupid. Because we still need the annotations on the // interMethodBody for pointcut matching to work we have to put them there as // well as on the interMethodDispatch. Really the code should be changed so // that the pointcut matching code looks at the intermethod dispatch // rather than the intermethodbody I thought we had modified all the logic throughout the system to look on the generated dispatch method for the annotations? Although I don't necessarily mind the annotations being on both the dispatch method and the actual body method - we really should be using the dispatch method to find the annotations (or match them), since the body method is not always created (abstract ITDs).
yep, those comments concern me too. I have no idea why the patch doesn't seem to work for you... When you're not busy we should discuss this and try to work out whats different between our workspaces
Created attachment 25399 [details] fixes all 29 cases again this patch has been synchronised with the latest AspectJ
Created attachment 25739 [details] fix which doesn't use interMethodBody this fix doesn't use the interMethodBody as an annotation holder
Fix checked in (shock horror) - waiting on build.
Fix available on AJ downloads page. For anyone feeling brave and attempting to modify current AJDT builds to plug in this AJ dev build - be aware that we have seen some incompatibilities with recent dev builds and AJDT so it may misbehave.
Created attachment 36976 [details] Test Case for 1.5.0 brakes described use cases 8 and 9
This testcase breaks use cases 8 and 9 under 1.5.0. Please reopen. // source 1 @Retention(RetentionPolicy.RUNTIME) @Target(ElementType.METHOD) public @interface TestAnnotation { public boolean value() default true; } // source 2 public aspect TestAspect { declare parents: TestClass implements TestInterface; // this also does not work (even when removing annotation in the following ITD) // declare @method: public void TestInterface.foo(): @TestAnnotation; @TestAnnotation public void TestInterface.foo() { System.err.println("foo"); } } // source 3 public interface TestInterface { public void foo(); } // source 4 public class TestClass { public static void main(String[] args) throws Exception { // returns null System.err.println(TestClass.class.getMethod("foo").getAnnotation(TestAnnotation.class)); } }
not quite the same as cases 8 and 9 since they have testcases and the testcases pass. Reopening for investigation of these new problems.
The new case covered is related to providing a *default implementation* for an interface method via ITD and expecting the class getting the default implementation to also get the annotation. The annotation copying code we have to get the annotation from the ITD in the aspect to the affected target class had to be copied from where it existed for munging new interface methods to where we munge topmost implementors. testcases and fixes checked in (both the 'normal' case and the case where the annotation is added via declare)
extra fixes available in latest AJ build: Date of build: 03/28/2006 13:39:48
