Comment 1 Helen Beeken 2006-01-27 09:32:52 EST

Created attachment 33701 [details]
patch containing testcase

Patch containing multiproject incremental testcase - apply to the tests project.

I've tried reproducing this problem and have created a testcase which throws a BCExcpetion at the same line which is reported here - the stack trace is:

org.aspectj.weaver.BCException
at org.aspectj.weaver.bcel.BcelWeaver.weave(BcelWeaver.java:1095)
at org.aspectj.ajdt.internal.compiler.AjCompilerAdapter.weave(AjCompilerAdapter.java:300)
at org.aspectj.ajdt.internal.compiler.AjCompilerAdapter.afterCompiling(AjCompilerAdapter.java:178)
at org.aspectj.ajdt.internal.compiler.CompilerAdapter.ajc$afterReturning$org_aspectj_ajdt_internal_compiler_CompilerAdapter$2$f9cc9ca0(CompilerAdapter.aj:70)
at org.aspectj.org.eclipse.jdt.internal.compiler.Compiler.compile(Compiler.java:367)
at org.aspectj.ajdt.internal.core.builder.AjBuildManager.performCompilation(AjBuildManager.java:811)
at org.aspectj.ajdt.internal.core.builder.AjBuildManager.doBuild(AjBuildManager.java:254)
at org.aspectj.ajdt.internal.core.builder.AjBuildManager.incrementalBuild(AjBuildManager.java:163)
at org.aspectj.ajde.internal.CompilerAdapter.compile(CompilerAdapter.java:117)
at org.aspectj.ajde.internal.AspectJBuildManager$CompilerThread.run(AspectJBuildManager.java:191)

BCException thrown: Can't find bcel delegate for pack.C2 type=class org.aspectj.weaver.ReferenceType
when weaving classes 
when weaving 
when incrementally building BuildConfig[C:\eclipse_installations\aspectj\eclipse\runtime-workspace-20050923-testcaseInvestigation\.metadata\.plugins\org.eclipse.ajdt.core\test2.generated.lst] #Files=4

since 8 lines have been added to the beginning of the BcelWeaver class between the time this bug was raised and now. 

I'm not sure if the testcase follows exactly the same scenario as that reported. The steps the testcase takes are:

 Stage 1: Compile the 4 files, pack.A2 extends pack.A1 (aspects) where
          A2 uses a protected field in A1 and pack.C2 extends pack.C1 
          (classes) where C2 uses a protected field in C1
 Stage 2: make the field private in class C1 ==> compile errors in C2
 Stage 3: make the field private in aspect A1 whilst there's the compile
          error <<-- this causes the BCException

As you can see from this it was necessary for there to be compilation errors in order to produce the BCException, plus I didn't have to delete the file (although deleting the file did produce the same error).

Could you confirm whether this scenario is similar to the one when you originally saw the problem?

Comment 2 Helen Beeken 2006-02-10 08:49:03 EST

The BCException is occuring only if the error is introduced during an incremental build (doing a full build before making the change to the aspect doesn't show this problem). This is because, in the case of a full build, each compilation unit is asked whether it has errors within AjCompilerAdapter.afterProcessing(..). If any return true then the "reportedErrors" field is set to true. Consequently in the AjCompilerAdapter.afterCompiling(..) method there is a check for whether any errors have been reported and if they have then we don't bother to do a weave. In the incremental case, we only check whether there are any errors against the changed compilation unit (in the test scenario in comment #1 this is pack.C1) which returns false. Therefore, when we come to decide whether to weave or not, "reportedErrors" is false, even though there is an error against the subclass pack.C2. Going on to do a weave when there are reported errors leads to the reported BCException because the BcelObjectType for pack.C2 is an EclipseSourceType rather than a BcelObjectType.

Comment 3 Helen Beeken 2006-02-10 11:28:51 EST

Expanding on comment #2.....this is the flow of what's happening:

Stage 1: compile all aspects and classes - there are no compilation errors and this is a full build.

Stage 2: make the field private in class C1. This forces an incremental build of C1. No errors are reported so we weave C1 (there is only C1 in the list of resultsPendingWeave). We then do an incremental build of C2 (since this is a subclass of C1). There are errors reported against C2 (as expected) so we finish without weaving.

Stage 3: make the field private in aspect A1. This forces an incremental build of both A1 and C1 (C1 is in the list of binary files to compile held in the AjState instance). There are no errors reported against the compilation of A1 or C1, therefore we decide to weave. Because we're weaving A1 we need to reweave the world and so all known class files are added to the weave list. This includes the class file for C2 which has an error reported against it. Consequently when we come to weave we go through each one of these classes in turn and when we get to C2 the BCException is thrown.

A possible fix could be to only add those classes which don't have compiler errors (since this would mirror the behaviour of a full build). However, this breaks another incremental test (testPr113531) which has two aspects A and A1 where A1 sets a protected field in A. This field is then made private and there is a resultant compile error against A1. A new aspect is then added whilst there is a compile error against A1, however, this error is then lost.

Comment 4 Helen Beeken 2006-02-13 03:55:37 EST

Another possible fix is to mirror the fix for bug 113531. This bug dealt with the case when there was an error against an aspect when an incremental build took place. The fix was to get assert that if there was an error then the delegate would be an EclipseSourceType in which case this can be ignored without weaving as the error will be reported later. The current bug is the same problem, just with an error against a class. Therefore, a fix would be to put similar logic into the loop where the non-aspects are woven in BcelWeaver.weave(..).

Comment 5 Helen Beeken 2006-02-13 05:17:45 EST

Created attachment 34559 [details]
zip containing testcase and fix

The attached zip file contains two patches:

* pr119882-tests-patch2.txt - apply to the tests project. This differs from the previously attached test by adding an extra assert that the error message is still there.
* pr119882-weaver-patch.txt - apply to the weaver project. This contains the fix described in comment #4.

Unfortunately, after synchronizing with the latest code in HEAD, this bug disappears (or at least the bug that this testcase recreates). This is due to the changes with comment "go 'back to source' on incremental aspect change".  However, since the proposed fix mirrors the behaviour of when an aspect has compile errors, I believe it should probably still be applied.

Comment 6 Andrew Clement 2006-02-13 05:51:24 EST

i agree - i've committed the fix.  there are probably other bugs we have that are also fixed now with this change...

Comment 7 Helen Beeken 2006-02-13 06:04:42 EST

*** Bug 124813 has been marked as a duplicate of this bug. ***

Comment 8 Helen Beeken 2006-02-13 06:05:19 EST

*** Bug 122096 has been marked as a duplicate of this bug. ***

Comment 9 Helen Beeken 2006-02-13 06:07:12 EST

*** Bug 118859 has been marked as a duplicate of this bug. ***

Comment 10 Helen Beeken 2006-03-10 11:03:13 EST

Closing this bug as fixed as the patch has been integrated into AspectJ. Moreover,  AJDT 1.3.1.20060309111525 for Eclipse 3.1 includes an AspectJ which contains this fix and the next build of AJDT for Eclipse 3.2M5 will also contain the fix.