Bug 243224 - [plan] [matching] Incorrect ambiguity error reported for complex pointcut
Summary: [plan] [matching] Incorrect ambiguity error reported for complex pointcut
Status: NEW
Alias: None
Product: AspectJ
Classification: Tools
Component: Compiler (show other bugs)
Version: DEVELOPMENT   Edit
Hardware: PC Windows XP
: P2 major (vote)
Target Milestone: ---   Edit
Assignee: aspectj inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-08-05 15:46 EDT by charles CLA
Modified: 2013-06-24 11:01 EDT (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description charles CLA 2008-08-05 15:46:14 EDT
I got the following error. This stack trace seems to be new in bugzilla so I report it. I can't reproduce for now since it involves multiple big projects. I hope some comments can help me narrow down the concrete cases.
java.lang.NullPointerException
at org.aspectj.weaver.bcel.BcelWeaver.raiseAmbiguityInDisjunctionError(BcelWeaver.java:859)
at org.aspectj.weaver.bcel.BcelWeaver.validateOrBranch(BcelWeaver.java:700)
at org.aspectj.weaver.bcel.BcelWeaver.validateBindings(BcelWeaver.java:660)
at org.aspectj.weaver.bcel.BcelWeaver.rewritePointcuts(BcelWeaver.java:598)
at org.aspectj.weaver.bcel.BcelWeaver.prepareForWeave(BcelWeaver.java:494)
at org.aspect ... oBuildJob.run(AutoBuildJob.java:235)
at org.eclipse.core.internal.jobs.Worker.run(Worker.java:55)

Compile error: NullPointerException thrown: null
Comment 1 Andrew Clement CLA 2008-08-05 16:06:58 EDT
Errors in that code have been reported before.  For example:

https://bugs.eclipse.org/bugs/show_bug.cgi?id=162657

but that is closed as fixed right now.  It could be the same thing - are you definetly using the latest compiler Charles?

In AspectJ1.6.1 line 859 is a comment line in BcelWeaver, whereas in AspectJ1.6.0 it was a line that would NPE due to a null source location (as described in 162657).
Comment 2 charles CLA 2008-08-05 19:49:41 EDT
Yes. I'm using the latest Dev. build. I can reproduce it now, but with my entire project settings. I haven't been able to isolate a small case. I can give you all the projects that I'm currently working on. A change to one of the files can trigger this bug. It has something to do with defining "execution-based" joinpoints in the super aspect and reusing this definition for pointcut composition in the child aspect, however, combining with other joinpoint definitions that are "call" based. I change all joinpoints to be "execution-based" and this error disappears. Let me know if you are interested in looking at this issue. I prefer not to attaching the entire source tree in the bugzilla. I can give them to you in private.
Comment 3 Andrew Clement CLA 2008-08-07 00:51:43 EDT
Hi Charles, if you email me the project setup then I will take a look.
Comment 4 Andrew Clement CLA 2008-08-13 16:45:34 EDT
hair raising stuff. 15Meg project with an error somewhere inside :)

Here is the pointcut that causes us trouble:

((((((!within(aolib.adstm.PrismAdapter+) && execution(public * aolib.adstm.ITransactional+.get*(..))) && this(BindingTypePattern(ITr, 0))) && persingleton(STM)) && !concretecflow(org.aspectj.runtime.internal.CFlowCounter STM.ajc$cflowCounter$0)) || (((call(* *..*.DMI.get*(..)) && target(BindingTypePattern(ITr, 0))) && persingleton(STM)) && !concretecflow(org.aspectj.runtime.internal.CFlowCounter STM.ajc$cflowCounter$0))) || (((call(* *..*.DMC.get*(..)) && target(BindingTypePattern(ITr, 0))) && persingleton(STM)) && !concretecflow(org.aspectj.runtime.internal.CFlowCounter STM.ajc$cflowCounter$0)))

When validating the pointcut to check bindings are correct in all branches we run the code BcelWeaver.validateOrBranch().  Because Or nodes just have a left and right (rather than more than 2 nodes), the case above results in a top level OR with a left branch that is another OR and a right branch.  If I summarize the above 3 piece pointcut like this:

((execution&&this OR call&&target) OR (call&&target))

when validating the OR between execution and call, we are fine that this() is used in one branch and target() in the other because call and execution can never match the same pointcut.  However, when we validate the top level OR the left branch and right branch can match the same join points (the left can match call/execution and the right call).  And the code breaks down... it doesn't handle this situation and sees a this() being used in the left branch whilst a target() is used in the right branch and because they can apparently match at the same join point, the error is raised.

trying to think of a fix that doesn't involve churning out a ton of new code..

Comment 5 Andrew Clement CLA 2008-08-13 17:27:35 EDT
I'm afraid the simplest thing for you to do to progress is split the pointcut so execution and call aren't mixed up together in the end result.  I know that may make your code a bit messy unfortunately... but it'll get you going until this is fixed properly.

The NullPointer happens for the same reason as in bug 162657, in that the source context is lost during rewrite - this is just another case of that.  I've fixed this case so at least there is no NPE now and the (incorrect) message about ambiguous binding comes out cleanly.

Here is the entire problem compressed into a small test program:

aspect Crashit {
  before(Object s): (
     (
      (!within(A*) && execution(* f*(..)) && this(s)) ||
      (call(* q*(..)) && target(s)))
     || call(* p*(..)) && target(s)
                    ) && !cflow(execution(* t*(..))) {}
}

And the error we see now that the npe is fixed:

C:\temp\canoworms\Crashit.java:5 [error] ambiguous binding of parameter(s) s across '||' in pointcut
Comment 6 charles CLA 2008-08-14 01:21:49 EDT
The workaround is, like you said, to consistently use the same kind of joinpoint definitions. It is not a blocker for us. Thanks a lot for figuring this out. 
Comment 7 Andrew Clement CLA 2008-12-01 16:15:15 EST
wont get to this in 1.6.3
Comment 8 Andrew Clement CLA 2013-06-24 11:01:49 EDT
unsetting the target field which is currently set for something already released