Community
Participate
Working Groups
Note: This could be just a doc thing, where we need to document the limitations that annotation style around advice has compared to code style. A user raised an issue on the list, here is He looked at this documentation: http://www.eclipse.org/aspectj/doc/released/adk15notebook/ataspectj-pcadvice.html and was looking how to use around advice and ProceedingJoinPoint. I just tried the same aspect too: import org.aspectj.lang.*; import org.aspectj.lang.annotation.*; @Aspect public class ProceedAspect { @Pointcut("call(* setAge(..)) && args(i)") void setAge(int i) {} @Around("setAge(i)") public Object twiceAsOld(ProceedingJoinPoint thisJoinPoint, int i) { System.err.println("advice running"); return thisJoinPoint.proceed(new Object[]{i*2}); } } public class Foo { int a; public void setAge(int i) { System.err.println("Setting age to "+i); a=i; } public static void main(String[]argv) { new Foo().setAge(5); } } > java Foo advice running Exception in thread "main" java.lang.ClassCastException: java.lang.Integer at Foo.setAge_aroundBody1$advice(Foo.java:112) at Foo.main(Foo.java:8) Before it will work, you need to supply the target in the proceed call array in the advice: @Around("setAge(i)") public Object twiceAsOld(ProceedingJoinPoint thisJoinPoint, int i) { return thisJoinPoint.proceed(new Object[]{thisJoinPoint.getTarget(),i*2}); //using Java 5 autoboxing } >java Foo advice running Setting age to 10 Maybe thats just something to document... but then let's change the pointcut from call to execution: @Aspect public class ProceedAspect { @Pointcut("execution(* setAge(..)) && args(i)") void setAge(int i) {} @Around("setAge(i)") public Object twiceAsOld(ProceedingJoinPoint thisJoinPoint, int i) { System.err.println("advice running"); return thisJoinPoint.proceed(new Object[]{thisJoinPoint.getTarget(),i*2}); } } >java Foo Exception in thread "main" java.lang.ClassCastException: Foo can not be converted to int at org.aspectj.runtime.internal.Conversions.intValue(Conversions.java:57) at Foo.setAge_aroundBody1$advice(Foo.java:113) at Foo.setAge(Foo.java:1) at Foo.main(Foo.java:11) oh ... let's now remove that getTarget() call we put in the advice: >java Foo advice running Setting age to 10 I know many of the problems here are that we can't do compile time type checking of the object array passed to proceed in the source - we would have to generate sophisticated code to handle it at runtime. There are numerous combinations, all failing in different ways so I tried to rationalise it and coded 11 testcases in code style and @AJ style to see what happens. It seems in all but the most basic cases, we get into trouble. I'm particularly worried about cases like those above where the code in the advice depends directly on the matched join point kind... Anyway, the tests are all checked in, see AtAroundTests.
I've also run into a number of situations where proceeding with arguments in @AJ style does not behave correctly. This needs to be addressed for 1.5.2.
just a crazy thought (must be the heat stroke I got on my run...) - if replacing the target type for the proceed, perhaps we should have a second proceed method on PJP, so we have proceed(Object[]) and proceed(Object newtarget,Object[]) ... just wondering if that might make processing easier - it will be more explicit in the advice what the programmer is trying to achieve...
I'm trying to decide the best thing to do here. At the moment it looks like you have to supply the target if the joinpoint you match on has a 'this' regardless of whether the pointcut related to your advice uses 'this()'. The explosion in the example code in the first example in this bug are (i believe) consequences of this/target being equivalent at the execution join point. It would seem to make sense that the new this/target have to be supplied if they are used in the pointcut (i.e. you might be wanting to replace them). I thought I liked the idea of the two argument proceed() for these kinds of cases as it will allow us some rudimentary checking in the compiler - but is it unnecessarily coupling the pointcut and the advice? For example (pseudocode... half code style, have @AJ style...) pointcut p(Object o,int i): call(* something(..)) && this(o) && args(i); void around(Object o,int i): p(o,i) { proceed(o,37); } the two arg variant is only in use there because p() uses this() - if p() was changed to: pointcut p(Object o,int i): call(* something(..)) && args(o,i); then proceed() taking two arguments is an error - we have tightly coupled the pointcut and the advice. This suggests the single Object array form of proceed is still the best thing to do. The decision then is whether the arguments to the proceed are based on what is available at the joinpoint or based on what was bound in the pointcut. From the developers notebook, the intent seems to have been the latter, but based on the current implementation it is the former.
Adrian - what particular use case do you think causes problems? I have fixed all the problems I've highlighted in this bug and examples in the developers notebook, is there something else? As I mentioned, sensible rules are: 1. if you bind 'this()' you must supply it (the same or unchanged) as the first entry in the object array 2. if you bind 'target()' you must supply it (the same or unchanged) as the next entry in the object array 3. you need to then supply all the arguments expected at the join point. Subsetting the arguments (like with code style) can be 'banned' (it may work, it may not but its unreliable) and reordering their sequence because you reordered them in the advice signature can be 'banned' (bunch of missing infrastructure). I think that gives us a sensible definition of how to use it... any more elaborate and we may end up being forced to generate all the tedious bytecode that verifies the object array at runtime and produces nice errors...
all committed. Doc changes are in, 30 new testcases. I think all the original confusion and complexity arose because of this: - whether you had to supply this/target in the object array to proceed was based on whether the matched join point had a this/target and whether this/target were the same (they are for execution jps for example). It made no difference whether you bound them or not in your pointcut... If you didn't know these rules, you might be failing to supply the extra parameter when it was needed or supplying an extra one when it wasnt required, causing the rest of the arguments you supply to be out of step with what is expected at the join point... even worse, if you matched a 'selection' of join points that did or did not have a this/target, it'd be tough to get the proceed call correct It's much more predictable now!
done all I plan to do with this for 1.5.2 - future problems we can cover with new bugs.
