Community
Participate
Working Groups
This was highlighted when running the AJDT core tests with the latest aspectj. The failing test (AJBuilderTest.testIncrementalBuildWithSrcFolder) uses the TJP Example, adds a comment to the Demo.java file and saves. It then checks that an incremental build happened. With the latest AspectJ this fails since a full build occurs instead. Unfortunately this test only fails when run as part of AJBuilderTest rather than on its own. The reason this is happening is the change to the logic in AjCompilerAdapter.weave() line 288. If there have been changes marked against the weaver and we're not a batch build then we used to just add all the classes to the list of classes to weave and continue. What now happens is that we force a full build because we believe an aspect has changed. The reason we believe that an aspect has changed comes from the call to AjState.getModifiedFiles(long lastBuildTime). Here we iterate over the files in the build config and check whether they were last modified before or after the lastBuildTime. The logic is: long modTime = file.lastModified(); // need to add 1000 since lastModTime is only accurate to a second on some (all?) platforms if (modTime + 1000 > lastBuildTime) { ret.add(file); } In the failing case, modTime < lastBuildTime but modTime + 1000 > lastBuildTime for the aspect. Therefore, it's added to the list of modified files and the test fails. When run on its own the aspect returns modTime + 1000 > lastBuildTime so we only think we've modified a class - which is correct. Clearly, removing the "+ 1000" fixes the problem and running the AJ test suite against this causes no failures. However, this doesn't fix the problem that the "+1000" was put in to fix. Doing the same thing and running the AJDT core test suite on my linux box does occasionally cause a failure with one of the tests - we didn't notice that the file had changed because of the timestamp, didn't do a build and consequently the test failed. This again is not recreatable on its own and if you add print statments. This is not a new bug since I've run the ajdt test suite against the AJ 1.5.0 release and even though we're saying we want to reweave the world we're not doing a full build. Therefore there could have been cases where we were weaving everything when we needn't have - the changes to go back to source if an aspect changed have just hightlighted the problem. Finally, I have been unable as yet to write a filing testcase for the AJ testsuite for this bug.
Another thing I found out..... As mentioned when I raised the bug, the reason for the full build is that the weaver is asked if it should reweave the world. This returns true and we're not a batch build so we force a full build. The code is: weaver.prepareForWeave(); if (weaver.needToReweaveWorld()) { if (!isBatchCompile) addAllKnownClassesToWeaveList(); } In the call to weaver.prepareForWeave() we go through the list of added classes (which includes both the class and the aspect). If its an aspect then we set the needToReweaveWorld field using the following: needToReweaveWorld |= xcutSet.addOrReplaceAspect(type); This returns true even though it probably shouldn't. Following this through, the "true" comes from CrosscuttingMembers.repace(CrosscuttingMembers). In here, the perClause, shadowMungers and typeMungers of the two crosscuttingmembers are checked for equality (amoung other things). However, these all return that they're not equal - the question is why they should do this considering nothing has changed. Looking further, there's no implementation of the equals method in PerClause, plus shadowMungers and typeMungers are using the List equals methods. Just ignoring these three checks causes the tests to pass.
Implementing the equals and hashCode methods in PerClause (which should probably be moved down into the subclasses) and changing the shadowMungers and typeMungers fields in CrosscuttingMembers to be Sets rather than Lists (so the Set equals method is used) means that all AJDT tests pass. Unfortunately, moving this over to AspectJ causes several test failures: Tests which fail because they're expecting a particular order in sysout ----------------------------------------------------------------------- * testAnnotatedITDFs_pr114005_1 (in 150 testsuite) * testAnnotatedITDFs_pr114005_2 (in 150 testsuite) * XXXJoinPoint,java - part of testRunThemAllWithJavacCompiledAndLTW * AfterXTest.java - part of testRunThemAllWithJavacCompiledAndLTW There are also some failures in the incremental testsuite: ---------------------------------------------------------- * testPr125405 - this expected 2 compiles and 1 weave, however is now getting 1 compile and 1 weave (as it was before the go back to source changes were made). This test consists of one aspect which is made generic and then back again. * testPr113257 - this expected a full build however its now an incremental build (as it was before the go back to source changes were made). This tests consists of an aspect and class which are built using a full build. An aspect which does nothing is then added. This now results in an incremental build * testPr123612 - this expected a full build however its now an incremental build (as it was before the go back to source changes were made). This test consists of one file which contains an aspect, interface and class. Originally the aspect has a declare @type statement. This statement is then removed which now causes an incremental build. Looking into testPr123612 - the reason we're now returning that we shouldn't reweave the world (and consequently not forcing a full build) regardless of the fact that the aspect has changed is possibly due to the call to addCrosscuttingStructures(..) in AjLookupEnvironment.collectAllITDsAndDeclares(). This calls down to the CrosscuttingMembers.replaceWith(..) method and notices that the list of declareAnnotationsOnType is different (note that the typeMungers aren't different), however, the next time this method is called via the weaver.prepareForWeave() method this list seems to have been reset as there's nothing in the old list of declareAnnotationsOnType and we return that there are no changes. The reason this was previously causing a full build (after the back to source changes) was that the two perclauses were always deemed to be not equal.
testPr123612: ------------- To clarify....after the declare @ type has been commented out in the testcase and we're rebuilding, the first time we enter CrosscuttingMembers.replaceWith(..) "this" has the declare @type recorded against it, but "other" doesn't. Since these don't match, we return that there is a change, however, the "other" declare @type value (namely no value) replaces the "this" one. This is why when we come to see if there are any changes in order to check whether we need to reweave the world we return that there aren't any. testPr113257: ------------- Although we've added an aspect, "needToReweaveWorld" is still false because it's just been added so comparing the crosscuttingMembers returns that nothing has changed. testPr125405: ------------- Each time the crosscuttingMembers are compared in the CrosscuttingMembers.replaceWith(..) method we return that nothing has changed. This is both when this is called via AjLookupEnvironment.collectAllITDsAndDeclares() and weaver.prepareForWeave(). Although with the implementation of equals in PerClause and the use of set equality rather than list equality on the surface we're returning the same as before the "back to source" changes, namely that we're doing an incremental build and that we're only weaving one thing, underneath we're actually doing something different. Because checking the equality of the perClauses always returned that something had changed we always thought that we would reweave the world. This used to be that we then added all classes to the list of classes to reweave, so although we didn't rebuild, we did weave everything. This no longer happens - instead we just weave those things which have changed.
Note.....Since the test for pr123612 contains all its types within the same file, all three classes are rewoven. As soon as this is split up into different files, only the aspect is rewoven.
Making the change to keep the shadowMungers and typeMungers in a list within CrosscuttingMembers and only convert to a set when we want to check for equality within replaceWith(..) means that the only failing tests are now the incremental ones.
Ok....an update on progress before I forget what I've done... The AspectJ tests have been fixed by keeping a record of whether or not we've ever found any changes when calling CrosscuttingMembers.replaceWith(..). This has been implemented by adding a field in CrosscuttingMembersSet called "changedSinceLastReset". This is updated if something has changed from within addOrReplaceAspect() and is reset before the call to collectAllITDsAndDeclares(..) in AjLookupEnvironment.completeTypeBindings(). Unfortuntately, although all the AspectJ tests now pass, this causes 4 failures within the AJDT core tests. The failures are all that an incremental build is expected but a full build happens. The scenarios are: 1. A comment is added to a class file which results in a full build even though no aspect has been changed. This is partly a timing problem since the aspect is added to the list of files which have changed only when all tests are run together. This is the same scenario for two of the failing tests (one is with a src folder and one isn't) 2. There are two projects where A depends on B and the contents of a method in B is changed which has no references to anything else. This results in a full build of A. 3. There are two projects where A depends on B and the contents of a method in B is changed which is referenced by something in A. This results in a full build of A. The reference is in a class not an aspect. Looking into scenario 1. the shadowmungers and typemungers have changed the first time replaceWith is called on the incremental build. The recorded list contains two entries and the current list contains nothing. Therefore, the recorded list is replaced and we remember that things have changed. The next time replaceWith is called (when we're preparing to weave), the recorded list is nothing (as expected since it changed last time) but the current list has the same to entries that we had to begin with. This is the same for both the shadowmungers and typemungers lists. Looking into this further, this is because there is cflow in the pointcut. This means that these typemungers are created from shadowmungers and so haven't been created the first time around but have been by the second time around. Therefore, it seems that the first time we enter we don't care about the shadowmungers. By adding an extra boolean flag to the method signature of replaceWith(..) saying whether or not we care about the shadowmungers or typemungers still does not solve the problem. This is because various equals methods have either not been implemented correctly or are not implemented at all. Fixing this in Advice, MemberImpl, BcelTypeMunger and ResolvedTypeMunger does fix the problem in scenario 1. Note, in its current state, this is a short term solution as we really should check the typemungers on both calls to replaceWith. There should be a flag set against each of them saying whether or not we should care about them (we don't care if they are created via a shadowmunger) and then check each of the ones we care about.
The remaining failing tests are different. The steps the test seems to be taking are as follows: - test starts - create project bug99133b - full build of project B occurs where we notice that we need to reweave the world and that we're a batchCompile - full build successful - create project bug99133a - full build of project bug99133a occurs where we notice that we need to reweave the world and that we're a batch compile - full build successful - full build of project bug99133a occurs again....notice that we need to reweave the world and that we're a batch compile - full build successful - we add sysout to the method body of a method within a class C.java in project bug99133b - nothing else has changed - incremental build of bug99133b is tried - we notice that nothing has changed and consequently that we don't need to reweave the world - this incremental build is successful - we then try to incrementally build project bug99133a - in AjState.prepareForNextBuild() we get the AjState for outputlocation C:\eclipse_installations\ajdt\eclipse31\junit-workbench-workspace\bug99133b\bin - we decide that there have in fact been structural changes to A1.class (which is the aspect within bug99133b that hasn't been touched at all) - we retry building bug99133a as a full build. - test fails because we do a full build rather than an incremental one Unfortunately, this test doesn't always fail, although at least it fails more times than it passes :-) Also, I haven't yet been able to write a failing test to fit within the AspectJ testsuite.
Running these failing tests agaist the 1.5.0 Aspectj returns that there are no changes so continues with an incremental build.
Further info.... The reason we're returning that there are structural changes is that within AjState.hasStructuralChanges(..) the ClassFileReader is reporting that there are two fields: FieldInfo with name ajc$initFailureCause and FieldInfo with name ajc$perSingletonInstance, whereas the CompactStructureRepresentation is reporting that there aren't any fields. This comparison returns that structural changes have happened and so the aspect A1 is added to the list of structuralChangesSinceLastFullBuild. This is all coming from the AjState.noteResult(InterimCompilationResult) method with the aspect having been recorded as the unwoven classfile. Previously (AJ 1.5.0), the unwoven classfile that is recorded against the interimCompilationResult is the class which has changed - C1. In the new case (post AspectJ 1.5.0) there is no record that the last thing built was C1...its all in terms of A1. The problem is coming from what we record at the end of the first incremental build (after we've incrementally built the class file C1 which has changed). Somewhere down the line I noticed that we do correctly record that there are no structural changes in the class file C1, we're just not doing the correct thing with the aspect. If we didn't record that anything structural had changed in the aspect then I believe we would be ok since we would then continue with an incremental build.
This sounds like you're getting close to a solution. I'm not sure I fully understand your last comment Helen, but I do get the gist of it (and I certainly understand noteResult and the compact structure representation stuff). It looks like we might be able to crack this tomorrow if we teamed up on it. Look out for me on 'sametime' and maybe we can have an impromptu call and get this nailed... Thks, A.
The 'fields' part of this problem is because at the time the compact structure is created, we ask the EclipseSourceType for its declared fields and getDeclaredFields() fills in its result 'on demand'. Unfortunately someone called getDeclaredFields() earlier, when there were none, then the code generators in the AspectDeclaration added the ajc$perSingletonInstance and the ajc$initFailureCause, but they are never accessible through getDeclaredFields(). The fix for this appears to be that EclipseSourceType.getDeclaredFields() also checks if the number of the fields on the backing binding has changed since it last worked out its answer, if it has then recalculate our answer. Of course, all this never used to be a problem because we operated on the bytecode produced at the end of compilation when all the fields have been correctly stored in the class file and we reparsed it to determine what fields there were. After fixing fields, naturally methods is also broken - but its more serious this time as the methods set is built from the declaration.methods array, when our new methods (ajclinit, aspectOf, hasAspect) have been created as binding entries and not methoddeclarations...
(capture of an IM discussion) I wondered about a different approach: instead of making sure we always see the generated members what if we ignored them during the comparison? for fields, I almost managed to convince myself that was safe last night it does make the 'has it changed' method more expensive, since we can't just compare lengths first ...but we only do that for the small # of things that we think have changed since the last increment, so that's not so bad ie. we filter the collections to take out ajc$*, aspectOf, hasAspect Since we are interested in structural changes from the perspective of the user program code, and these things are generated, they can never affect that.
we may then have problems with clinit and init - possibly they can be 'ignored' too ... maybe <init> if it is the default one and we're in an aspect.
Filtering out the fields which start with "ajc$" and those methods whose name contains either "aspectOf", "hasAspect", "clinit" or "ajc$" means that the AJDT core tests now all pass.
As expected migrating the changes over to AspectJ causes AJ test failures :-( The implementation of the various equals methods is causing two test failures: * Ajc150Tests.testGenericAspects_pr115237 - this is now failing with a "NoAspectBoundException" * MultiProjectIncrementalTests.testPr114875 - this was expecting a full build but is now returning that nothing has changed. In the case of the multiproject test I believe this is reasonable and the test should be changed to check for an incremental build. The scenario is that there is an @AJ aspect called Base which is built (full build). A new @AJ aspect is added which again forces a full build (as expected). However, the final step is to include an exact copy of the original Base aspect. We now (through the implementation of the various equals methods) think that these are the same thing and so dont go back to source and do a full build. As for the NoAspectBoundException.......
The NoAspectBoundsException is coming from the implementation of equals and hashcode in ResolvedTypeMunger.
The failing testcase is: public class pr115237 { public static void main(String[] args) { C c = new C(); c.go(); A a = A.aspectOf(c); B b = B.aspectOf(c); <------- causing NoAspectBoundsException } static class C { void go() {} } abstract static aspect AA pertarget(pc()) { abstract pointcut pc(); before() : pc() { System.out.println("go()"); } } static aspect A extends AA { pointcut pc() : call(void C.go()); } abstract static aspect BB<T> pertarget(pc()) { abstract pointcut pc(); before() : pc() { System.out.println("go()"); } } static aspect B extends BB<C> { pointcut pc() : call(void C.go()); } } The implementation of equals in ResolvedTypeMunger is returning true in all cases when it used to return false when it was using Objects's equals method.
The fix is to move the equals implementation from ResolvedTypeMunger down into its subtypes. With this there all are no AJ test failures. Its now a case of splitting all these changes into consumable patches....
Created attachment 35752 [details] zip containing first part of fix The attached zip contains two patches: * pr129163-weaver-patch-perClauseEquals.txt - apply to the weaver project. This patch adds equals and hashCode methods to the different perClause classes * pr129163-test-patch-perClauseEquals.txt - apply to the tests project. This patch makes changes to three tests. The first is legitimate in that we now compile once rather than twice, however, the second two comment out the check for a full build. This should be reinstated when other patches relating to this bug are applied and has only been included so that all the changes required for this bug can be applied incrementally.
Created attachment 35754 [details] better test patch than last time This is a better patch for the tests project than the one contained in the zip file. The two tests which have been ammended will fail when the other changes are in (shadowmunger/typemunger checking) which will remind us to switch them back.
first patches in for perclause hashcode/equals
Created attachment 35757 [details] patch containing fix to AjState Apply patch to the org.aspectj.ajdt.core project. This patch contains the changes required to AjState and EclipseSourceType - see comment #14
Created attachment 35759 [details] zip containing patches for maintaining a record about whether anything has changed This zip contains three patches: * pr129163-tests-patch-maintainRecord.txt - apply to the tests project. This patch returns the two multiproject incremental tests back to checking for a full build rather than an incremental one (reverses the previous test patch) * pr129163-ajdt-patch-maintainRecord.txt - apply to the org.aspectj.ajdt.core project. * pr129163-weaver-patch-maintainRecord.txt - apply to the weaver project. This fixes the problems described in comment #2 to comment #6
patch from comment 22 integrated.
patch from comment 23 integrated
Created attachment 35826 [details] patch containing implementation of remaining equals/hashCode Apply this patch to the weaver project. This patch contains the remaining implemenations of equals and hashCode.
With all these patches applied there are two remaining failing AJDT tests: AJBuidler.testIncrementalBuildWithSrcFolder and AJBuilder.testIncrementalBuildWithoutSrcFolder. These are failing because the typeMungers are being compared and we're coming to the conclusion that things have changed when they haven't.
patch from comment 26 integrated ... one to go
Created attachment 35841 [details] patch for checking if we really care about the typemungers Apply this patch to the weaver project. This patch contains the work for checking the typeMungers. If we don't care about the shadowmungers then we remove those type mungers which are created to help with the implementation of shadowmungers from the sets we compare in CrosscuttingMembers.replaceWith(..). Otherwise we include them. Hopefully thats the last patch since now both the AJDT and AspectJ tests pass :-)
patch from comment 29 committed. that should be it - the AJDT containing this is going to behave very very differently ... but hopefully in a good way.
Unfortunately with the latest AspectJ which contains all the attached patches one of the AJDT visual tests fails with the following NPE: java.lang.NullPointerException at org.aspectj.ajdt.internal.compiler.WeaverMessageHandler.findReferenceContextFor(WeaverMessageHandler.java:188) at org.aspectj.ajdt.internal.compiler.WeaverMessageHandler.handleMessage(WeaverMessageHandler.java:105) at org.aspectj.weaver.Lint$Kind.signal(Lint.java:273) at org.aspectj.weaver.patterns.WildTypePattern.resolveBindingsForMissingType(WildTypePattern.java:817) at org.aspectj.weaver.patterns.WildTypePattern.resolveBindingsFromFullyQualifiedTypeName(WildTypePattern.java:696) at org.aspectj.weaver.patterns.WildTypePattern.resolveBindings(WildTypePattern.java:622) at org.aspectj.weaver.patterns.SignaturePattern.resolveBindings(SignaturePattern.java:82) at org.aspectj.weaver.patterns.KindedPointcut.resolveBindings(KindedPointcut.java:259) at org.aspectj.weaver.patterns.Pointcut.resolve(Pointcut.java:194) at org.aspectj.ajdt.internal.compiler.ast.PointcutDesignator.finishResolveTypes(PointcutDesignator.java:84) at org.aspectj.ajdt.internal.compiler.ast.PointcutDeclaration.resolveStatements(PointcutDeclaration.java:209) at org.aspectj.org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration.resolve(AbstractMethodDeclaration.java:400) at org.aspectj.ajdt.internal.compiler.ast.PointcutDeclaration.resolvePointcut(PointcutDeclaration.java:190) at org.aspectj.ajdt.internal.compiler.lookup.AjLookupEnvironment.resolvePointcutDeclarations(AjLookupEnvironment.java:422) at org.aspectj.ajdt.internal.compiler.lookup.AjLookupEnvironment.completeTypeBindings(AjLookupEnvironment.java:234) at org.aspectj.org.eclipse.jdt.internal.compiler.Compiler.beginToCompile(Compiler.java:301) at org.aspectj.org.eclipse.jdt.internal.compiler.Compiler.compile(Compiler.java:315) at org.aspectj.ajdt.internal.core.builder.AjBuildManager.performCompilation(AjBuildManager.java:843) at org.aspectj.ajdt.internal.core.builder.AjBuildManager.doBuild(AjBuildManager.java:268) at org.aspectj.ajdt.internal.core.builder.AjBuildManager.incrementalBuild(AjBuildManager.java:170) at org.aspectj.ajde.internal.CompilerAdapter.compile(CompilerAdapter.java:117) at org.aspectj.ajde.internal.AspectJBuildManager$CompilerThread.run(AspectJBuildManager.java:191) NullPointerException thrown: null I have, however, been able to recreate this as an AspectJ testcase.
Created attachment 35899 [details] failing testcase which produces NPE Apply this patch to the tests project.
The NPE is occuring because during the incremental build (in the initialisation of AjCompilerAdapter) we reset the compiler instance within WeaverMessageHandler to be null. Consequently, when we come to calculate the warning an NPE occurs when we try to ask the compiler instance for the unitsToProcess. Just as a note, the failing testcase adds the following pointcut: pointcut p2(): call(* File.*(..)); without adding an import for File. We fail whilst trying to report the warning.
Created attachment 35905 [details] improved failing testcase which shows NPE Apply to the tests project. This test improves on the last one supplied by checking the warning output.
Created attachment 35906 [details] patch containing fix for NPE Apply this patch to the org.aspectj.ajdt.core project. This patch contains the fix for the NPE. Rather than passing null through when the compiler is reset, pass through the new compiler instance.
comment 34,35 patches committed
With all these changes there is one remaining failing test: an XReference visual test. Looking into this I believe its a failure in the xref view updating logic. Strangely, this test is also failing for me now when I run it against the existing code in the 1.3 ajdt brach. What's happening is that an aspect is opened in the editor which populates the xref view. Then we navigate to the line containing the declare warning statement. We then uncheck "link with editor" in the xref view, comment out the declare warning statement and save. This forces a build which forces an update of the xref view which should remove the declare warning relationships. However, it doesn't do this. This is because the xref view doesn't register that the last thing that was selected before it decided to uncheck "link with editor" was the aspect (coming from navigating to the line of the declare warning statement). It therefore thinks that the last thing that was selected was the package declaration. Consequently, when it comes to calculate whether anythings changed, it thinks that we've selected something different and so doesn't update the view. When following these steps manually in a runtime workbench everything works as expected. Therefore I believe this is a timing issue with the xref visual test. I've raised AJDT bug 131090 to cover this and to make the testcase more robust. All this means that all tests are now passing :-)
Wahey.....aspectj has been checked into the AJDT 1.3 branch. Therefore, closing this bug as fixed.
what a monster... I do hope memory usage is better in Aj ;) thanks for persisting with this bug Helen.
it's a new record for most commented aspectj bug (or so I believe)! Thanks Helen for perserving with this. The change in AjState that kicked all this off was only 5-10 lines of code! Here's hoping that AJDT is significantly improved after all this effort - it should certainly be noticeable.
Created attachment 36053 [details] patch containing tests which follow the AJDT test failures Apply this patch to the tests project. I'm really not sure why I couldn't write AJ tests which were failing in the same way as the AJDT ones when I last tried......I tried again this morning and succeeded without too much problem. Hmmm...it would have been much easier if I'd managed to do that first time around.....Oh well :-( This patch contains two tests which follow the two failing scenarios that were seen in AJDT. Firstly, the scenario which was mentioned when this bug was raised, and secondly the one mentioned in comment #6 and comment #7. The tests fail against a 'pre-any fixes for this bug' aspectj workspace and then behave exactly as the AJDT ones when the same steps are applied that are detailed in this bug.
