Community
Participate
Working Groups
Given the following aspect: package pkg; public aspect A { pointcut p() : call(* pkg.*.*(..)); before() : p() { } } Adding a whitespace (or anything else in the aspect) and saving results in an incremental build and all crosscutting information is lost. Everything is regainded after a full build. This is a regression from AspectJ 1.5.0 and could be related to bug 133532.
Created attachment 37494 [details] failing testcase patch Apply this patch to the tests project.
interesting...interesting... I picked up this bug as I thought it might be related to what people are reporting on the list, and it could be... but let me explain what is happening: In the testcase Helen attaches, it checks for weave info messages. Now on the incremental build where nothing has changed, we dont expect to get a weave info message as no new weaving has occurred. So I had to instead look at the structure model since after the incremental build that should still contain relationships for what has been woven. And it doesnt... why is that? Well ... on starting the incremental build we discover that an aspect has changed and in the 'orrible structure model repair code, we remove the aspect from the hierarchy, we *ALSO* remove all relationships that had the aspect as an endpoint... of course this means the relationship showing class C is advised is removed. In the old days this was fine as on recompilation we would recreate the relationships ... but the advancements in 1.5.1 mean we don't re-add the relationships, so it looks like C is no longer advised...
Hmmm... So I have started modifying the structure repair code. It seems so far that a new algorithm like this will work: - Only remove the nodes for deleted files at the start of a build, and remove all relationships with these removed nodes at one of their endpoints (unlike today where we also remove nodes for 'modified' files) Then, for files that are going to be compiled, the new node structure built for them via EclipseFactory.finishedCompilationUnit() (which calls buildStructureForCompilationUnit) will replace any existing structure for that type, so they don't need to be explicitly deleted. (note, the uses pointcut relationship is added at this point, not during weaving like all the other relationships *sigh*) then, just prior to weaving a file we need to go in and remove all relationships where the *TARGET* of the relationship is in the file we are about to weave, because they will be added again during weaving (this means we won't damage relationships on other files that are affected by nodes in the modified aspect). Although the infrastructure will work 'by accident' without this change, it isn't correct - without the change we will just not add duplicate relationships, but we won't do anything to tidy up any that no longer exist). Now - this involves, for any target, working out the type it is 'in' (which programelement). If we try and do this then we discover there are glitches here and there, in test 121384 something peculiar happens finding out the containing type for an 'aspect declarations' relationship target as it seems to be targetting the first line of a file rather than the type declaration line? We could blindly assume this relationship needs deleting because the target is wierd, but it would be better to stop the wierd relationship getting created in the first place. Once that algorithm is in and debugged, then we hit the problem with source locations. We have now said (quite sensibly) that newline type whitespace in a file isn't a structural change to it, however it *can* affect the source location/offset for an entity (for example, advice may move to be on a different line on an incremental compile). Now, how do we repair the structure model in this case? the 'advised by' relationships on the affected elements will have the old source location... It is perhaps time we moved to a more robust form of handle for the elements of a file, something not tied to the position of the element, but tied to all the information we know can't change about an element, so something like the handles AJDT uses: a method: =TJP Example/src<tjp{Demo.java[Demo~main an aspect: =TJP Example/src<tjp*GetInfo.aj}GetInfo some advice: =TJP Example/src<tjp*GetInfo.aj}GetInfo&around an ITD: =Bean Example/src<bean*BoundPoint.aj}BoundPoint)Point.hasListeners)QString; the 2 declare parents in an aspect: =Bean Example/src<bean*BoundPoint.aj}BoundPoint`declare parents!2 a pointcut: =Spacewar Example/src<spacewar*Ship.aj[Ship+helmCommandsCut+QShip; a method call: =Bean Example/src<bean{Demo.java[Demo~main~\\[QString;?method-call(void bean.Point.setX(int))!37!0!0!0!I what about 2 pieces of before advice with the same signature and the same pointcut (unusual...) - how would their handles differ? I think the second would have !2 at the end handles of that form won't be different simply for white space changes, they will only be different if a structural change *has* occurred in the file. The prefix (the project name) may need changing to be the config file perhaps (build.lst is the default I think). Because all that change is not trivial, the short term fix would be to go back to the old days where any aspect change is considered a serious change - it may perform slowly when you make a change to an aspect, but the structure model is guaranteed correct and it seems to me we would be better off with something slow and correct rather than fast and unreliable. If we do go back to this way of working, by changing replaceWith to return true - then a few tests fail (for the reasons you'd expect - they are testing how many files got compiled after incremental compiles) testPr125405(org.aspectj.systemtest.incremental.tools.MultiProjectIncrementalTests) testPr129163_2(org.aspectj.systemtest.incremental.tools.MultiProjectIncrementalTests) testPr129163_3(org.aspectj.systemtest.incremental.tools.MultiProjectIncrementalTests) testPr131505(org.aspectj.systemtest.incremental.tools.MultiProjectIncrementalTests) testPr134541(org.aspectj.systemtest.incremental.tools.MultiProjectIncrementalTests)
with the hack to always see an Aspect change (even a whitespace one) as a need to full build, some AJDT tests fail - there is some quirk in AJ that it thinks files on disk have changed when they clearly havent. possibly could be fixed by putting a delay in between steps of the AJDT tests but has prompted me to try and do the right solution instead. So, new solution is a variation of the algorithm described at the beginning of the last comment (turned out not to be as simple as i initially thought, will scribble it down when I get more time). And to resolve the problem of handles being tied to source locations, I've included source locations in the equals comparisons for mungers, meaning a change in position of the munger will be seen as a change requiring a reweave. With a little fiddling this is working OK but I decided to play a little with declare warning instead of advice and that has caused me some concerns ... possibly leading me into the problem Ramnivas is seeing with advice vanishing on incremental compiles. Basically, my workspace is in a bit of a state, I'll get it all in as soon as I can. At the moment, the AJ tree has the 'hack' in (replaceWith always returns true) - I will reverse that when I put these changes in.
fixes are all committed. Some new tests have been added to MultiProjectIncrementalTests to verify the model is consistent post compile.
yey! fix available - we should cover the handle changes in a separate enh request.
Just as a note, bug 141730 has been raised to cover the handle changes suggested.
