Community
Participate
Working Groups
Hi. There is a subtle problem with how AspectJ handles around advice: Closures are always generated based on the declared type. That means that when I have a base program ... ArrayList a = new ArrayList(); List b = a; a.size(); b.size(); ... and an around advice as follows ... Object around(List l): call(* List.*(..)) && target(l) { List other = new SomeList(l); //SomeList is *not* a subtype of ArrayList return proceed(other); } ... in the around advice that is generated for the match b.size(), everything works fine (the call target is substituted by the other list implementation). However for the call a.size(), I get a ClassCastException, because the type for the around body/closure etc. is ArrayList and not List. It would be great if the code generation could be changed to use the type that was used in the header of the around advice instead (here List).
What about Object around(List l): call(* ArrayList.*(..)) && target(l) "List" is declared and bound, but it's not right to permit replacing an ArrayList with a List in this context. I'd hate to have to evaluate the pointcut to know what to permit. Nor is it clear we should use the topmost declaring type for the method. That would solve your ClassCastException, but a method can be declared by two supertypes (ok, warning/error there) and it can also be hoisted or pushed down the hierarchy between build- and deploy-time and still be binary compatible, but that would leave the closure with the wrong type. I can imagine programmers getting upset that a List was permitted where an ArrayList was expected. e.g., imagine another piece of advice on the same join point that uses the target ArrayList - CCE or advice fails to run, right? So the only way to avoid CCE is to use the declared type when replacing any variables. That means that when you bind on a supertype, you run the risk of CCE when replacing a reference with a supertype reference. That's been the (only, AFAIK) type-safety risk of around advice for a long time.
I remember discussing this with Jim Hugunin many moons ago. For proxying etc. it is attractive to proceed with an alternate target that implements the same interface, but AspectJ only supports proceeding with a target object that is of the same type as the static type of the receiver at the join point. Wes gives some good examples of why this is in his previous comment. We might be able to give better error reporting than a runtime ClassCastException though - a warning if the target proceed argument does not have the required static type for example.
Hmmm, so I have given this a few days of thought. To be honest, first I was stunned because this looked so totally obvious problem with such an obvious solution to me but Wes brought up a nice twist... So if I understand the problem correctly, it boils down to a possible mismatch between the declared type of a variable in an advice definition (in Wes' example "List") and what is stated in the type pattern (here "ArrayList"). As Wes explained, the pointcut... call(* ArrayList.*(..)) && target(l) ... certainly provides an ArrayList and hence, I agree that the code generation should assume this type when "proceeding", because other aspects (clients of the pointcut) could well rely on the fact that l has this concrete type. However, I propose the following, as a clean solution. Assume, I rewrite Wes' aspect as follows: --- pointcut listCall(List l): call(* ArrayList.*(..)) && target(l); Object around(List l): listCall(l) { proceed(new LinkedList(l)); } --- So here we have a *named* pointcut, which on its LHS declares it provides a variable of type List. Hence, clients should not assume anything about the actual subtype of List that will be bound during runtime - despite (!) the fact that the RHs of the pointcut definition reveals that it will actually always be an ArrayList. Consequently, the methods generated for the around advice should only be aware of this type List and the related methods should be generated with this declared type only. This would allow to "proceed" as shown, with another list implementation. Does this sound reasonable to you?
Let's create a simplified List hierarchy to explore this: public interface List<E> { void add(E element); E get(int index); } public class ArrayList<E> implements List<E> { public void add(E element) {...} public E get(int index) { ... } public boolean onlySupportedInArrayList() { ... } } public class LinkedList<E> implements List<E> { public void add(E element) {...} public E get(int index) { ... } public boolean onlySupportedInLinkedList() { ... } } and some code to give rise to a join point: ArrayList al = new ArrayList(); al.onlySupportedInArrayList(); Now if we examine your rewritten aspect: --- pointcut listCall(List l): call(* ArrayList.*(..)) && target(l); Object around(List l): listCall(l) { proceed(new LinkedList(l)); } --- The call to al.onlySupportedInArrayList is matched by the listCall pointcut expression, but the proceed will fail because it is impossible to call onlySupportedInArrayList because this operation is not defined for LinkedList. The only situation in which it is safe to use an alternative target type on proceed is when the join point has a signature in which the declaring type is a super-type of the replacement proceeding type. So: pointcut listCall(List l): call(* List.*(..)) && target(l); Object around(List l): listCall(l) { proceed(new LinkedList(l)); } is theoretically safe, even though the original target may have been an ArrayList. A weaver supporting this would have to determine the least-specific (root) declaring type of the join point shadow, and validate (probably at runtime via an instanceof test) that the replacement target object was an instance of the root declaring type.
Ahh, true. Thanks for pointing that out. So the problem really boils down to a possible mismatch between the declared type in the pointcut header and the type pattern in its body. One idea I had was to simply alter the semantics of target(..) to only match calls which are declared on the type of its argument. But this would be too complicated for most to understand, I suppose, and also it would prevent you from writing pointcuts as... pointcut pc(List l): (call(* LinkedList.size()) || call(* ArrayList.size())) && target(l); ... where one uses the most common supertype not to restrict matching but rather to accommodate either of the two implementations. Hmmm, so this is a real problem then. I wonder how come that nobody has seen this as a serious limitation before? I do. Or is there an idiom to get around this? I even tried to intercept the original call on the concrete type with another around-advice that casts the object to the interface type I want, then proceeds on that in order to provoke an interface-call, but that also did not seem to work.
Documented in implementation.xml,1.10 and semantics.xml,1.31. Still need to investigate differences with annotation-style implementation.
