Community
Participate
Working Groups
/* Example to illustrate a problem with variable binding and (||) in pointcuts. When run, this program produces java.lang.ClassCastException immediately after the call to "foo". The reason is that the instance tests inherent in the pointcut are done separately from the variable binding. Decompiled, the code produced for the relevant call to "foo" is as follows: -------------------------------------------------- DisjunctVarBinding.foo(r5, r4); label_0: { if (r5 instanceof B == false) { if (r4 instanceof B == false) { break label_0; } } IfPointcut.aspectOf().ajc$afterReturning$IfPointcut$26d(r5, (B) r4); } -------------------------------------------------- It should however read something like this, using the instance tests to determine the appropriate variable binding -------------------------------------------------- DisjunctVarBinding.foo(r5,r4); if (r4 instanceof B) IfPointcut.aspectOf().ajc$afterReturning$IfPointcut$26d(r5, (B)r4) else if (r5 instanceof A) IfPointcut.aspectOf().ajc$afterReturning$IfPointcut$26d(r4,(B)r5) -------------------------------------------------- */ class A { void m() { System.out.println("A"); } } class B extends A { void m() { System.out.println("B"); } } aspect IfPointcut { after(A a, B b) returning: call(* foo(*,*)) && (args(b,a) || args(a,b)) { System.out.println("Woven"); } } public class DisjunctVarBinding { public static void foo(A a, A b) { a.m(); b.m(); } public static void main(String[] args) { A a = new A(); B b = new B(); foo(b,a); } }
Thanks for the clear and simple bug report. This looks like a valid bug. The current architecture (in org.aspectj.weaver.patterns) keeps the process of matching and binding in Pointcuts separate. I believe that the correct fix to this bug in the near-term is to make the program below generate a weave-time error explaining that the bindings for a and b COULD BE ambiguous when applied to call(DisjunctVarBinding.foo(A, A)). This is a potentially valid concern, i.e. what order should the arguments be passed in to the advice for foo(new B (), new B())? The short-term error checking should be added either to ExposedState.set or to callers to ensure that there isn't already a conflicting binding recorded for the variable. Adding this error checking is important and should be done either for 1.2final or shortly thereafter. In the longer term (1.3 or later) we should investigate better support for this kind of potential ambiguity not being an error.
It would be a pity to rule out this kind of thing in the longer term. All that's needed is a well-defined meaning so that no ambiguities arise. Here is a possible semantics that determines the binding order. Think of each pointcut as being shorthand for a "disjunctive normal form" [that is a series of "||" separated expressions, each of which are primitives combined with "&&": (P1 && P2 ... && Pn) || ... || (Q1 && Q2 && ... && Qm) ] Now we can describe the intended pointcut matching by the following rules: 1. Conjunctions (&&) work by simple evaluation from left-to-right. The evaluation shortcuts when a conjunct doesn't match. Note that for expressions that solely consists of primitives and (&&), the result of the match, if one exists at all, is uniquely determined. 2. For disjunctions (||), it is easy to understand what happens for disjunctive normal forms. Try each of the alternatives in left-to-right order, and pick the first one that succeeds. If you don't have a DNF, then you can make it so by applying the following four rules exhaustively: !(P||Q) => !P && !Q !(P&&Q) => !P || !Q (P || Q) && R => (P&&R) || (Q && R) R && (P ||Q) => (R&&P) || (R && Q), provided R does not contain (||) The proviso in the last rule guarantees that the order of the disjuncts in the result is well-defined. 3. The static checks are also with respect to the DNF: * no variable may be bound twice in the same disjunct * each disjunct must bind the same set of variables * that bound set of variables must be a superset of the variable declarations of the pointcut Of course the implementation does not need to use the DNF internally, but it must do the re-binding as stipulated by DNF. This corresponds nicely to what would happen in a language like Prolog [alright, I've spent quite a few years in declarative language research, and I like to think of the pointcut notation as a very pure logical language :-)]. Below are a few examples to clarify the above rules further. EXAMPLES All are matched against foo(2,3): 1. calls to "foo" where the first argument is 2: pointcut example(int x) : call(void foo(int,int)) && args(x,..) && if(x==2) Note in this example, there is something to be said for having x local to the pointcut, and not exposed as an argument, but AspectJ syntax doesn't allow us to do that. I'll leave the pointcut header out in subsequent examples. 2. calls to foo where either the first or second argument is 3: call(* foo(..)) && (args(x,..) || args(..,x)) && if(x==3) => ((call(* foo(..)) && args(x,..)) || (call (* foo(..)) && args(..,x))) && if(x==3) => call(* foo(..)) && args(x,..) && if(x==3) || call(* foo(..)) && args(..,x) && if(x==3) Upon matching against foo(2,3), the first disjunct fails, but the second one succeeds. 3. Bind x to the first or second argument of foo, and bind y to the first or second argument of foo. However, we don't want the binding (x==2 && y==2) call(* foo(..)) && (args(x,..) || args(..,x)) && (args(y,..) || args(..,y)) && if(!(x==2 && y==2)) => (call(* foo(..)) && args(x,..) && args(y,..) && if !(x==2 && y==2)) || (call(* foo(..)) && args(x,..) && args(..,y) && if !(x==2 && y==2)) || (call(* foo(..)) && args(..,x) && args(y,..) && if !(x==2 && y==2)) || (call(* foo(..)) && args(..,x) && args(..,y) && if !(x==2 && y==2)) So when matched against foo(2,3) this will succeed upon the second alternative with (x=2 and y=3). This example also illustrates why it is necessary to limit the rules for producing the DNF. If we allowed the rule R && (P || Q)=> (R && P) || (R && Q), provided R does not contain (||) but ignored the proviso, we could obtain the DNF (call(* foo(..)) && args(x,..) && args(y,..) && if !(x==2 && y==2)) || (call(* foo(..)) && args(..,x) && args(y,..) && if !(x==2 && y==2)) || (call(* foo(..)) && args(x,..) && args(..,y) && if !(x==2 && y==2)) || (call(* foo(..)) && args(..,x) && args(..,y) && if !(x==2 && y==2)) and when that is matched against foo(2,3), we get the solution (x=3 and y=2).It is worthwhile to compare the two above expressions in DNF closely: note that in the first, we fix the value of x, and then run through all the possible values of y. By contrast, in the second expression, y is so to speak in the `outer loop', and for each choice of y, we run through all the values of x. We chose the first alternative because it appears to be more intuitive, corresponding to a left-to-right evaluation order. Finally, note that the above semantics could easily be amended to take into account the presence of side-effects, if that was desired.
I'm implementing Jims proposed short term solution for AspectJ 1.2 which polices an attempt to bind to a formal more than once. What makes putting out a nice error quite tricky is that at the point where we can determine we are attempting to bind for a second time, we don't have all the right state around. As currently implemented, my fix gives this for the test program: C:\Eclipse\212\eclipse\aspectj_ws2\tests\bugs\DisjunctVarBinding.java:18 error Cannot bind more than once to a formal. Failed to bind on type:B (args(b,a) || args(a,b)) { ^^^^^^^ C:\Eclipse\212\eclipse\aspectj_ws2\tests\bugs\DisjunctVarBinding.java:18 error Cannot bind more than once to a formal. Failed to bind on type:A (args(b,a) || args(a,b)) { ^^^^^^^ The state I have around is: - The source location of the offending args() pcd. - The shadow 'foo(a,b)' - The BindingTypePattern which gives me the type that cannot be bound and the index of the formal against which we are trying to bind it. What I *dont* appear to have around is the name of the entry in args, so I don't know that it is 'a' or 'b' because earlier on in the process they were converted to BinaryTypePatterns for the right type. I could add state to BinaryTypePattern to remember any originating 'name' that was supplied in args but that is a larger change for this unusual error case.
Fix checked in. Fix in AspectJ1.2rc2. But we should make this bug an enhancement request for the future rather than closing so we can investigate this case not being an error.
Intended fix for 1.2 checked in, moving to enhancement request.
Hi all. I was wondering if this bug is ever going to be properly addressed. It has been open for about four years now. The reason for why I am asking is that at this year's AOSD conference there seemed to be some confusion about what the actual AspectJ semantics is w.r.t. such bindings. Actually pretty much everybody seemed to agree that conceptually abc implements the correct semantics. However, the fact that this bug has been open and unfixed in ajc for such a long time makes some people believe that the semantics that ajc implements would somehow be the "correct" one. My personal opinion is that it's really about time to get rid of those implementation specific peculiarities of AspectJ (in either implementation), at least at all places where they can actually be avoided. Otherwise I see AspectJ end up in the same state in which C ended up, containing an extreme amount of serious flaws, just because its semantics was mostly defined by what the (flawed) compiler was doing. I think that's a bad state of the art. Just my 2 cents...
Unfortunately we have other priorities and enhancements like this never make it to the top of the list for implementing. I agree differing semantics between implementations is less than ideal - feel free to submit a patch and I will look to integrate it.
