Community
Participate
Working Groups
I'm using Eclipse 3.2 and AJDT 1.4. When I write a simple class and an simple aspect like: public class MyClass { int x; public int getX() { return x; } public void setX(int x) { this.x = x; } public static void main(String[] args) { MyClass m = new MyClass(); m.setX(10); System.out.println(m.getX()); } } public aspect MyAspect { pointcut mypointcut(): execution(* getX()) && !within(MyAspect); int around(): mypointcut() { int w = proceed() + 3; return w; } } When I start the program I'm getting the result 13. If I change the +3 to +4 with "Build autmatically" activated, save the aspect and execute the program, it still results in 13. If I deactivate Build Automatically, clean the project and build the project manually, the next run results in 14. Yours, Steffen
Hi, I have been able to reporduce this bug, but not an AJDT bug so should be passed over to AJ. Event Trace: 10:32:25 Build kind = AUTOBUILD 10:32:25 Project=lo, kind of build requested=Incremental AspectJ compilation 10:32:25 build: Examined delta - source file changes in required project lo 10:32:25 Preparing for build: planning to be an incremental build 10:32:25 Starting incremental compilation loop 1 of possibly 5 10:32:25 Timer event: 219ms: Time to first compiled message 10:32:25 Timer event: 234ms: Time to first woven message 10:32:25 AspectJ reports build successful, build was: INCREMENTAL 10:32:25 AJDE Callback: finish. Was full build: false 10:32:25 Timer event: 297ms: Total time spent in AJDE 10:32:25 Timer event: 0ms: Create element map (2 rels in project: lo) 10:32:25 Types affected during build = 1 10:32:25 Timer event: 0ms: Add markers (2 markers) 10:32:25 Timer event: 344ms: Total time spent in AJBuilder.build()
Assigning to AJ since we do an incremental build but the new value hasn't been picked up.
It's not so much that a build is not being triggered, since as you can see in the following verbose output: =========================================================================================== 14:30:43 Build kind = AUTOBUILD 14:30:43 Project=pr154054, kind of build requested=Incremental AspectJ compilation 14:30:43 build: Examined delta - source file changes in required project pr154054 14:30:43 Preparing for build: planning to be an incremental build 14:30:43 Starting incremental compilation loop 1 of possibly 5 14:30:43 AJC: compiling source files 14:30:43 Timer event: 180ms: Time to first compiled message 14:30:43 AJC: compiled: C:\eclipse\workspaces\aspectj\runtime-workspace\pr154054\src\pkg\MyAspect.aj 14:30:43 AJC: processing reweavable state 14:30:43 AJC: adding type mungers 14:30:43 Timer event: 200ms: Time to first woven message 14:30:43 AJC: woven aspect pkg.MyAspect (from C:\eclipse\workspaces\aspectj\runtime-workspace\pr154054\src\pkg\MyAspect.aj) 14:30:43 AspectJ reports build successful, build was: INCREMENTAL 14:30:43 AJDE Callback: finish. Was full build: false 14:30:44 Timer event: 300ms: Total time spent in AJDE 14:30:44 Timer event: 30ms: Create element map (2 rels in project: pr154054) 14:30:44 Types affected during build = 1 14:30:44 Timer event: 0ms: Add markers (2 markers) 14:30:44 Timer event: 381ms: Total time spent in AJBuilder.build() ========================================================================== AspectJ is compiling and reweaving the aspect, it's more a case of the new value not being picked up. The reason for this is because its around advice and the advice is being inlined. If you turn inlining off then everything works as expected. As does the case if you consequently make a change to MyClass which causes an incremental build of MyClass.
Also see bug 83890 which anticipated this problem ... but doesn't offer a fix ;)
A possible solution could be to improve the equals methods so we pick up that there has been a change, however, this could get messy....
Created attachment 49817 [details] failing testcase Apply this patch to the tests project. This patch rewrites the supplied test code as an incremental test to fit within the testsuite.
the suggestion in comment #5 is the right solution - i too believe the messiness could be an issue (that's why we haven't already fixed it...) but it's worth prototyping that fix to see how truly messy it is.
I believe I have a prototype fix for this bug - it isn't optimized and definitely needs tidying up. The fix is to implement equals/hashCode in BcelMethod, Method and Code. The equals method in Code uses a modified toString() which doesn't include the LineNumberTable (since this changes if a whitespace is inserted with the consequence being a full build rather than an inc one - not what we want). The issue is obviously whether to use toString() or not since this is fairly expensive. Plus Code.equals is called a fair amount. These changes cause three testcase failures: 1.MultiprojectIncrementalTests.testSwappingAdviceAndHandles_pr141730() - this failed because we used to expect a full build and with the changes we get an inc one. Change was to check for inc build instead 2.MultiprojectIncrementalTests.testInitializerCountForJDTLikeHandleProvider_pr141730() - this failed because we were banking on a full build and the changes we used no longer forced a full build. Fix was to force a full build. 3. AtAjSyntaxTest.testIfPointcut2() - failed because the output comparison was different and the output was dependent on the ordering of the execution of if() pointcuts. Fix was to make the test more robust. Also, note that the originally raised problem could have been fixed by checking the equality of 'byte[] code' in Code, however, this doesn't cover the case when it's not an int, for example a string is updated.
A different approach is to deal with the comparison of inlined around advice shadowmungers in CrosscuttingMemebers.replaceWith(..) differently than other shadowmungers. Here, rather than using Set equality, implement our own "equivalence". For example, two inlined around advice shadowmungers are considered equivalent if their signatures have the same byte[] code and their ConstantPools are the same. This allows for different line numbers (which consequently doesn't force a full build on whitespace changes). Implementing it this way only requires "equivalent" methods in CrosscuttingMembers and BcelMethod. It also doesn't require any changes to the testcases (although I've kept the changes to AtAjSyntaxTest.testIfPointcut2() which makes it more robust).
Created attachment 50455 [details] failing testcase and fix described in comment #9 This zip contains two patches: 1. pr154054-tests2.txt: apply to the tests project. Contains previously attached testcase, plus test where a String changes, plus the changes to the AtAjSyntaxTest mentioned previously 2. pr154054-weaver.txt: apply to the weaver project. Contains the fix detailed in comment #9
i like the idea of having the fix up at that level, but I was a bit surprised to see it relying on toString() of the constant pool for the method when doing the comparison. Constant pools can be large and it isn't cached, so the toString() is recalculated each time a comparison occurs - however I know this is only for around advice so maybe that isn't such a problem. But it seems to me that the constant pool can change without the around advice changing in any way, will that not cause us to full build? The constant pool is shared by all the members in the class, suppose I make this incremental change to an aspect: BEFORE: aspect Foo { before(): execution(* *(..)) { System.out.println("abc");} void around(): execution(* *(..)) { proceed();} } AFTER: aspect Foo { before(): execution(* *(..)) { System.out.println("def");} void around(): execution(* *(..)) { proceed();} } The fix here will cause a full build because the constant pool has changed - really you just want to consider the constant pool entries referenced by that piece of around advice.
Created attachment 50687 [details] testcases and fix including testcase in comment #11 Attached zip file contains three patches: * pr154054-bcel-builder.txt: apply to the bcel-builder project. This adds a "getCodeString()" method to Code which returns the same as toString(true) except for the exception table and attribute information. This method is then used within BcelMethod.isEquivalentTo(Object) to check for equivalence. * pr154054-tests.txt: apply to the tests project. This is the same as the previous tests patch with the addition of the testcase provided in comment #11 * pr154054-weaver.txt: apply to the weaver project. This is a modification of the previously attached weaver patch. It uses the Code.getCodeString() method in BcelMethod.isEquivalentTo(Object) rather than working out if the Code.getCode() are equal and using equality of the constant pool.
So close ... but still not quite there. Because the exception types and ranges are not included in the code string, you fail to notice this as a change requiring a full build: BEFORE: aspect Foo { void around(): execution(* *(..)) { try { proceed(); } catch (Exception e) { } } } AFTER: aspect Foo { void around(): execution(* *(..)) { try { proceed(); } catch (Throwable e) { } } } So - I had to augment the code string method, I've also added a testcase for this situation. (A future problem could be with noticing when the name of a local variable has changed - if we don't notice then debugging may not work as expected BUT that doesnt matter until we properly sort out debugging of inlined around advice ;) So I don't propose we cover it in the this bug) shame that crosscuttingmembers knows about bcel - would be nice to remove that at some point (I'd rather keep BcelMethod package protected) - but don't have time to address all this at the mo.
fixes available iplog
Updated org.aspectj/bundles.
