Community
Participate
Working Groups
I have the following program: aspect Simple { pointcut interesting(A instance): execution(* getString(..)) && this (instance); Object around(A instance) : interesting(instance) { System.err.println("around advice"); C c = new C(); return proceed(c); } public static void main(String[] argv) { B b = new B(); C c = new C(); System.out.println(b.getString()); System.out.println(c.getString()); } } interface A { public String getString(); } class B implements A { String s = "hello from B"; public String getString() { return s; } } class C implements A { String s = "hello from C"; public String getString() { return s; } } It compiles but it won't run. I (we) were thinking that because the around advice signature takes an 'A' then we could proceed with an instance of any type implementing A. In the case above although the advice matches on the execution of a method on B - we attempt to create a 'C' (which also implements A) and proceed with that. The class cast exception is because some of the extracted methods in the generated code have parameters of type 'B' rather than 'A'. Here is the relevant code from a decompiled B.class: class B implements A { B() { s = "hello from B"; } public String getString() { return (String)getString_aroundBody1$advice(this, Simple.aspectOf(), this, null); } static final String getString_aroundBody0(B b) { return b.s; } static final Object getString_aroundBody1$advice(Simple this, A instance, AroundClosure ajc_aroundClosure, C c) { System.err.println("around advice"); C c1 = new C(); C c2 = c; C c3 = c1; return getString_aroundBody0((B)c3); } String s; } The classcastexception comes out at the point we try and cast C to a B ready for consumption by getString_aroundBody0(B b). The code that does this seems to be BcelShadow.createMethodGen(). Should this even work? Should we be constructing getString_aroundBody0() taking an A rather than a B?
Ignore the decompiled code - the decompilers seem to have trouble consuming the generated around advice method - the signature is all wrong ?!? This doesn't mean there isn't a problem, just that its gonna be hard to investigate!
Adrian and I have been discussing what to do... It seems the getString() in our example is extracted into a method called something like: static String getString_adviceBody0(B b) {...} And it is invoked from the advice method with: getString_adviceBody0((B)c); ============ In fact when proceeding on C we'd like something like this: The method is extracted to static String getString_adviceBody<UNIQUE_ACROSS_TYPES_ID>(A a) {...} and the advice calls c.getString_adviceBody<UNIQUE_ACROSS_TYPES_ID>(c); Notice the extracted form of the method now takes the same parameter as was indicated in the around advice declaration (i.e. A, the common interface type) Notice that now, although the extracted method is static, we invoke it on 'c' which allows us to in fact invoke the variant that exists in the C class rather than the one in the B class. If we call it in this way there is no need to type cast the argument to the extracted method. Finally, because just using a counter on the end of extracted method names could cause problems in the case of two pieces of advice applying to one file and only one piece applying to another - we use a name suffix that relates to the around advice declaration. In our example, this would mean that at the point the extracted method is called, it is called in 'C' rather than 'B' and so returns the right result. of course, Adrian and I are not experts on compilers or type binding so we haven't a clue if any of this would work ;)
There are a number of problems with the scheme just described. Chief of which is that there is no virtual dispatch for static methods. So the method: String getString_adviceBody<UNIQUE_ACROSS_TYPES_ID>(A a) {...} would have to be non-static. Secondly, every implementor of the interface would need to be woven to add the getString_adviceBody method (since you don't know which implementor will be dispatched to in the proceed). Some of these implementors may not even be matched by the pointcut associated with the around advice - so what would trigger their munging? It therefore seems that the dispatch has to happen to the actual orginal method as declared on the interface (getString() in this case), and the around advice body would then need to be smart enough to know not to reapply itself the second time round. But this is a big change, and can it be done in a way that doesn't hurt performance for all the other uses of around advice? And what happens in the case when the target implementor is itself the recipient of other advice - do all the precedence rules work out ok?? Note also that although it doesn't give a class cast exception at runtime, proceeding with a subtype of the type receiving the advice (e.g. if you change the sample program to make C extend B) gives the wrong results as B's implementation of getString() will execute rather than C's. The programming and semantics guide are kind of silent on exactly what should happen when you start changing a variable in a proceed argument list that is bound to the target() (or this()) at a join point. It is a very powerful and useful thing to be able to do. Since I can't see an easy implementation right now, it's tempting to document an implementation limitation that you can only proceed with the same concrete type - but that cuts out at least one important use case that I can't easily see another way of implementing: namely using around advice to delegate execution of all methods in an interface to another implementor, without resorting to reflection or listing each method in the interface individually. If I can control all the call sites, I might be able to get close with dynamic proxies (http://java.sun.com/j2se/1. 3/docs/guide/reflection/proxy.html), but if I need to advise executions... bring on the gurus....
This code is behaving according to the type model for around advice that I believe Erik and I both have in our heads. However, I can't find this documented anywhere in the language spec, so I'm cc'ing Erik to keep me honest. Joinpoint shadows have a static type signature that is independent of any advice on them. So, in your example, the execution joinpoint on B has the static signature this and target: B args: () This static signature is used a lot for optimizing matching and residues. The one place that it can be visible is with proceed and the value returned from around. The rule for proceed is that you must call it with arguments that correspond to the static type of the corresponding joinpoint shadow or you'll get a runtime type error. Having this rule makes it a lot easier to think about complicated cases. For example, what happens if A doesn't contain a getString method? What if your advice was on this(instance) && get(* s)? Should it still be possible to substitute a C for a B? Where would the control-flow pick up in that case? The fact that you can write this code and you get runtime type errors instead of compile-time errors is the one known hole in AspectJ's type system. I've heard there are papers showing it's not possible to do any better, but all I know is this is the best we could come up with at the time. There are several small ways to improve the error messages from this situation that are worth doing as time permits. One is to modify the casts that are inserted to instead check the type and throw a special IncompatibleClassForProceed exception. Another important change is to add an xlint warning for cases where such casts are inserted, and to detect cases like this one where the cast is guaranteed to fail and signal them more aggressively. For the particular example you show here you can solve this problem by modifying your advice to read: Object around(A instance) : interesting(instance) { System.err.println("around advice"); C c = new C(); return c.getString(); } Obviously this isn't a completely general solution if you have too many wildcards in play. The current static type of joinpoint shadows is a relatively simple place to be. It might be possible to come up with a more aggressive type model for proceed that enables this kind of change, as well as reducing the corresponding restrictions on the values returned from around advice. However, I don't see how that could be done in time for 1.2.
No plans to change the current implementation at the moment...
LATER/REMIND bugs are being automatically reopened as P5 because the LATER and REMIND resolutions are deprecated.
