Fixes for Bug 80249: Order of types passed to compiler determines weaving behavior

6 files changed, 110 insertions, 17 deletions

diff --git a/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/AjLookupEnvironment.java b/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/AjLookupEnvironment.java
index 325f5ed27..da778a54e 100644
--- a/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/AjLookupEnvironment.java
+++ b/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/AjLookupEnvironment.java
@@ -24,15 +24,6 @@ import org.aspectj.ajdt.internal.compiler.ast.AspectDeclaration;
 import org.aspectj.ajdt.internal.compiler.ast.PointcutDeclaration;
 import org.aspectj.bridge.IMessage;
 import org.aspectj.bridge.WeaveMessage;
-import org.aspectj.weaver.AsmRelationshipProvider;
-import org.aspectj.weaver.ConcreteTypeMunger;
-import org.aspectj.weaver.ResolvedTypeMunger;
-import org.aspectj.weaver.ResolvedTypeX;
-import org.aspectj.weaver.TypeX;
-import org.aspectj.weaver.WeaverStateInfo;
-import org.aspectj.weaver.World;
-import org.aspectj.weaver.bcel.LazyClassGen;
-import org.aspectj.weaver.patterns.DeclareParents;
 import org.aspectj.org.eclipse.jdt.core.compiler.CharOperation;
 import org.aspectj.org.eclipse.jdt.internal.compiler.ast.AbstractMethodDeclaration;
 import org.aspectj.org.eclipse.jdt.internal.compiler.ast.TypeDeclaration;
@@ -48,6 +39,15 @@ import org.aspectj.org.eclipse.jdt.internal.compiler.lookup.PackageBinding;
 import org.aspectj.org.eclipse.jdt.internal.compiler.lookup.ReferenceBinding;
 import org.aspectj.org.eclipse.jdt.internal.compiler.lookup.SourceTypeBinding;
 import org.aspectj.org.eclipse.jdt.internal.compiler.problem.ProblemReporter;
+import org.aspectj.weaver.AsmRelationshipProvider;
+import org.aspectj.weaver.ConcreteTypeMunger;
+import org.aspectj.weaver.ResolvedTypeMunger;
+import org.aspectj.weaver.ResolvedTypeX;
+import org.aspectj.weaver.TypeX;
+import org.aspectj.weaver.WeaverStateInfo;
+import org.aspectj.weaver.World;
+import org.aspectj.weaver.bcel.LazyClassGen;
+import org.aspectj.weaver.patterns.DeclareParents;
 
 /**
  * Overrides the default eclipse LookupEnvironment for two purposes.
@@ -120,11 +120,46 @@ public class AjLookupEnvironment extends LookupEnvironment {
 		Collection declareParents = factory.getDeclareParents();
 
 		doPendingWeaves();
+		
+		// We now have some list of types to process, and we are about to apply the type mungers.
+		// There can be situations where the order of types passed to the compiler causes the
+		// output from the compiler to vary - THIS IS BAD.  For example, if we have class A
+		// and class B extends A.  Also, an aspect that 'declare parents: A+ implements Serializable'
+		// then depending on whether we see A first, we may or may not make B serializable.
+		
+		// The fix is to process them in the right order, ensuring that for a type we process its 
+		// supertypes and superinterfaces first.  This algorithm may have problems with:
+		// - partial hierarchies (e.g. suppose types A,B,C are in a hierarchy and A and C are to be woven but not B)
+		// - weaving that brings new types in for processing (see pendingTypesToWeave.add() calls) after we thought
+		//   we had the full list.
+		// 
+		// but these aren't common cases (he bravely said...)
+		boolean typeProcessingOrderIsImportant = declareParents.size()>0;
+		
+		if (typeProcessingOrderIsImportant) {
+			List typesToProcess = new ArrayList();
+			for (int i=lastCompletedUnitIndex+1; i<=lastUnitIndex; i++) {
+				CompilationUnitScope cus = units[i].scope;
+				SourceTypeBinding[] stbs = cus.topLevelTypes;
+				for (int j=0; j<stbs.length; j++) {
+					SourceTypeBinding stb = stbs[j];
+					typesToProcess.add(stb);
+				}
+			}
 
-		for (int i = lastCompletedUnitIndex + 1; i <= lastUnitIndex; i++) {
-			weaveInterTypeDeclarations(units[i].scope, typeMungers, declareParents);
+			while (typesToProcess.size()>0) {
+				// A side effect of weaveIntertypes() is that the processed type is removed from the collection
+				weaveIntertypes(typesToProcess,(SourceTypeBinding)typesToProcess.get(0),typeMungers,declareParents);
+			}
+		
+		} else {
+			// Order isn't important
+			for (int i = lastCompletedUnitIndex + 1; i <= lastUnitIndex; i++) {
+				//System.err.println("Working on "+new String(units[i].getFileName()));
+				weaveInterTypeDeclarations(units[i].scope, typeMungers, declareParents);
+			}
 		}
-        
+		
         for (int i = lastCompletedUnitIndex + 1; i <= lastUnitIndex; i++) {
             SourceTypeBinding[] b = units[i].scope.topLevelTypes;
             for (int j = 0; j < b.length; j++) {
@@ -146,6 +181,35 @@ public class AjLookupEnvironment extends LookupEnvironment {
 		stepCompleted = BUILD_FIELDS_AND_METHODS;
 		lastCompletedUnitIndex = lastUnitIndex;
 	}
+	
+	/**
+	 * Weave the parents and intertype decls into a given type.  This method looks at the
+	 * supertype and superinterfaces for the specified type and recurses to weave those first
+	 * if they are in the full list of types we are going to process during this compile... it stops recursing
+	 * the first time it hits a type we aren't going to process during this compile.  This could cause problems 
+	 * if you supply 'pieces' of a hierarchy, i.e. the bottom and the top, but not the middle - but what the hell
+	 * are you doing if you do that?
+	 */
+	private void weaveIntertypes(List typesToProcess,SourceTypeBinding typeToWeave,Collection typeMungers,Collection declareParents) {
+		// Look at the supertype first
+	    ReferenceBinding superType = typeToWeave.superclass();
+	    //System.err.println("Been asked to weave "+new String(typeToWeave.getFileName()));
+	    if (typesToProcess.contains(superType) && superType instanceof SourceTypeBinding) {
+	    	//System.err.println("Recursing to supertype "+new String(superType.getFileName()));
+	    	weaveIntertypes(typesToProcess,(SourceTypeBinding)superType,typeMungers,declareParents);
+	    }
+	    // Then look at the superinterface list
+		ReferenceBinding[] interfaceTypes = typeToWeave.superInterfaces();
+	    for (int i = 0; i < interfaceTypes.length; i++) {
+	    	ReferenceBinding binding = interfaceTypes[i];
+	    	if (typesToProcess.contains(binding) && binding instanceof SourceTypeBinding) {
+		    	//System.err.println("Recursing to superinterface "+new String(binding.getFileName()));
+	    		weaveIntertypes(typesToProcess,(SourceTypeBinding)binding,typeMungers,declareParents);
+	    	}
+		}
+	    weaveInterTypeDeclarations(typeToWeave,typeMungers,declareParents,false);
+	    typesToProcess.remove(typeToWeave);
+	}
 
 	private void doPendingWeaves() {
 		for (Iterator i = pendingTypesToWeave.iterator(); i.hasNext(); ) {
@@ -350,7 +414,7 @@ public class AjLookupEnvironment extends LookupEnvironment {
 			}
 		}
 	}
-
+	
 	private void doDeclareParents(DeclareParents declareParents, SourceTypeBinding sourceType) {
 		List newParents = declareParents.findMatchingNewParents(factory.fromEclipse(sourceType));
 		if (!newParents.isEmpty()) {
diff --git a/tests/decp/A.java b/tests/decp/A.java
new file mode 100644
index 000000000..f7e8cc3b9
--- /dev/null
+++ b/tests/decp/A.java
@@ -0,0 +1,2 @@
+public class A {
+}
diff --git a/tests/decp/AspectX.java b/tests/decp/AspectX.java
new file mode 100644
index 000000000..9fb992f50
--- /dev/null
+++ b/tests/decp/AspectX.java
@@ -0,0 +1,3 @@
+public aspect AspectX {
+  declare parents: A+ implements java.io.Serializable;
+}
diff --git a/tests/decp/B.java b/tests/decp/B.java
new file mode 100644
index 000000000..6a96c2e00
--- /dev/null
+++ b/tests/decp/B.java
@@ -0,0 +1,12 @@
+public class B extends A {
+	
+	public static void main(String[] args) {
+		B b = new B();
+		if (b.getClass().getInterfaces().length>0) {
+			throw new RuntimeException("B should not implement any interfaces: "+b.getClass().getInterfaces()[0].toString());
+		}
+		if (!(b instanceof java.io.Serializable)) {
+			throw new RuntimeException("B should be serializable! Inherited from A");
+		}
+	}
+}
diff --git a/tests/src/org/aspectj/systemtest/ajc150/Ajc150Tests.java b/tests/src/org/aspectj/systemtest/ajc150/Ajc150Tests.java
index e79edf0ba..25d4e8f80 100644
--- a/tests/src/org/aspectj/systemtest/ajc150/Ajc150Tests.java
+++ b/tests/src/org/aspectj/systemtest/ajc150/Ajc150Tests.java
@@ -26,7 +26,11 @@ public class Ajc150Tests extends org.aspectj.testing.XMLBasedAjcTestCase {
     return new File("../tests/src/org/aspectj/systemtest/ajc150/ajc150.xml");
   }
 
-  public void test() {
-  	// placeholder for the first test...
+  public void test_typeProcessingOrderWhenDeclareParents() {
+	try {
+  		runTest("Order of types passed to compiler determines weaving behavior");
+	} finally {
+  		System.err.println(ajc.getLastCompilationResult().getStandardError());
+	}
   }
 }
\ No newline at end of file
diff --git a/tests/src/org/aspectj/systemtest/ajc150/ajc150-tests.xml b/tests/src/org/aspectj/systemtest/ajc150/ajc150-tests.xml
index b770e0de2..f2023d6c9 100644
--- a/tests/src/org/aspectj/systemtest/ajc150/ajc150-tests.xml
+++ b/tests/src/org/aspectj/systemtest/ajc150/ajc150-tests.xml
@@ -1,7 +1,15 @@
-<!-- AspectJ v1.5.0 Tests: Bridge attribute on methods generated to support generics -->
-    
+<!-- AspectJ v1.5.0 Tests -->
+
+   
     <ajc-test dir="java5/bridgeMethods" pr="72766" title="Ignore bridge methods">
         <compile files="AspectX.aj" inpath="testcode.jar" options="-showWeaveInfo">
           <message kind="warning" line="7" text="pointcut did not match on the method call to a bridge method."/>
         </compile>
     </ajc-test>
+
+    <ajc-test dir="decp" pr="80249" title="Order of types passed to compiler determines weaving behavior">
+        <compile files="A.java,B.java,AspectX.java"/>
+        <run class="B"/>
+        <compile files="B.java,A.java,AspectX.java"/>
+        <run class="B"/>
+    </ajc-test>
\ No newline at end of file