Summary: | [@AspectJ] @AJ around advice does not behave like code style | ||
---|---|---|---|
Product: | [Tools] AspectJ | Reporter: | Andrew Clement <aclement> |
Component: | Compiler | Assignee: | aspectj inbox <aspectj-inbox> |
Status: | RESOLVED FIXED | QA Contact: | |
Severity: | critical | ||
Priority: | P2 | ||
Version: | DEVELOPMENT | ||
Target Milestone: | 1.5.2 | ||
Hardware: | PC | ||
OS: | Windows XP | ||
Whiteboard: |
Description
Andrew Clement
2006-02-02 06:29:17 EST
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. |