Preserve ordering of declare annotation when removing and adding annotations

Issue: 407739

org.aspectj.matcher/src/org/aspectj/weaver/CrosscuttingMembers.java

@@ -16,6 +16,7 @@ import java.util.Collection;
 import java.util.HashSet;
 import java.util.Hashtable;
 import java.util.Iterator;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -58,9 +59,9 @@ public class CrosscuttingMembers {
 	private List<Declare> declareDominates = new ArrayList<Declare>(4);
 
 	// These are like declare parents type mungers
-	private Set<DeclareAnnotation> declareAnnotationsOnType = new HashSet<DeclareAnnotation>();
-	private Set<DeclareAnnotation> declareAnnotationsOnField = new HashSet<DeclareAnnotation>();
-	private Set<DeclareAnnotation> declareAnnotationsOnMethods = new HashSet<DeclareAnnotation>();
+	private Set<DeclareAnnotation> declareAnnotationsOnType = new LinkedHashSet<DeclareAnnotation>();
+	private Set<DeclareAnnotation> declareAnnotationsOnField = new LinkedHashSet<DeclareAnnotation>();
+	private Set<DeclareAnnotation> declareAnnotationsOnMethods = new LinkedHashSet<DeclareAnnotation>();
 	// declareAnnotationsOnMethods includes constructors too
 
 	private Set<DeclareTypeErrorOrWarning> declareTypeEow = new HashSet<DeclareTypeErrorOrWarning>();

org.aspectj.matcher/src/org/aspectj/weaver/CrosscuttingMembersSet.java

@@ -18,6 +18,7 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
@@ -286,10 +287,12 @@ public class CrosscuttingMembersSet {
 		return declareParents;
 	}
 
-	// DECAT Merge multiple together
+	/**
+	 * @return an amalgamation of the declare @type statements.
+	 */
 	public List<DeclareAnnotation> getDeclareAnnotationOnTypes() {
 		if (declareAnnotationOnTypes == null) {
-			Set<DeclareAnnotation> ret = new HashSet<DeclareAnnotation>();
+			Set<DeclareAnnotation> ret = new LinkedHashSet<DeclareAnnotation>();
 			for (Iterator<CrosscuttingMembers> i = members.values().iterator(); i.hasNext();) {
 				ret.addAll(i.next().getDeclareAnnotationOnTypes());
 			}
@@ -299,9 +302,12 @@ public class CrosscuttingMembersSet {
 		return declareAnnotationOnTypes;
 	}
 
+	/**
+	 * @return an amalgamation of the declare @field statements.
+	 */
 	public List<DeclareAnnotation> getDeclareAnnotationOnFields() {
 		if (declareAnnotationOnFields == null) {
-			Set<DeclareAnnotation> ret = new HashSet<DeclareAnnotation>();
+			Set<DeclareAnnotation> ret = new LinkedHashSet<DeclareAnnotation>();
 			for (Iterator<CrosscuttingMembers> i = members.values().iterator(); i.hasNext();) {
 				ret.addAll(i.next().getDeclareAnnotationOnFields());
 			}
@@ -312,11 +318,11 @@ public class CrosscuttingMembersSet {
 	}
 
 	/**
-	 * Return an amalgamation of the declare @method/@constructor statements.
+	 * @return an amalgamation of the declare @method/@constructor statements.
 	 */
 	public List<DeclareAnnotation> getDeclareAnnotationOnMethods() {
 		if (declareAnnotationOnMethods == null) {
-			Set<DeclareAnnotation> ret = new HashSet<DeclareAnnotation>();
+			Set<DeclareAnnotation> ret = new LinkedHashSet<DeclareAnnotation>();
 			for (Iterator<CrosscuttingMembers> i = members.values().iterator(); i.hasNext();) {
 				ret.addAll(i.next().getDeclareAnnotationOnMethods());
 			}

tests/bugs173/pr407739/Aspect.java

@@ -0,0 +1,6 @@
+public aspect Aspect {
+
+	declare @field: * Hello.dummy : -@MyAnnotation;
+	declare @field: * Hello.dummy : @MyAnnotation(dummy2 = "korte");
+
+}

tests/bugs173/pr407739/Hello.java

@@ -0,0 +1,9 @@
+public class Hello {
+
+	@MyAnnotation(dummy1 = "alma")
+	private String dummy;
+
+    public static void main(String []argv) throws Exception {
+      System.out.print(Hello.class.getDeclaredField("dummy").getDeclaredAnnotations()[0]);
+    }
+}

tests/bugs173/pr407739/MyAnnotation.java

@@ -0,0 +1,13 @@
+
+import static java.lang.annotation.ElementType.FIELD;
+import static java.lang.annotation.ElementType.METHOD;
+import java.lang.annotation.Retention;
+import static java.lang.annotation.RetentionPolicy.RUNTIME;
+import java.lang.annotation.Target;
+
+@Target({METHOD, FIELD})
+@Retention(RUNTIME)
+public @interface MyAnnotation {
+	String dummy1() default "";
+	String dummy2() default "";
+}

tests/bugs173/pr407966/Aspect.aj

@@ -0,0 +1,5 @@
+public aspect Aspect {
+        void around(): call(* Def.def(..)) {
+                System.out.println("aspect");
+        }
+}

tests/bugs173/pr407966/Def.java

@@ -0,0 +1,8 @@
+class Clazz<T> {
+}
+
+public class Def {
+        public static void def(Clazz<?>[] c) {
+                System.out.println("def");
+        }
+}

tests/bugs173/pr407966/Use.java

@@ -0,0 +1,5 @@
+public class Use {
+        public static void main(String[] argv) {
+                Def.def(null);
+        }
+}

tests/bugs173/pr408014/Code.java

@@ -0,0 +1,6 @@
+
+class Code implements Foo.Intface {}
+
+class Foo {
+  interface Intface {}
+}

tests/bugs173/pr408014/IIdentifiable.java2

@@ -0,0 +1,5 @@
+interface IIdentifiable {
+    UUID getPlatformId();
+    void setPlatformId(UUID id);
+}
+

tests/bugs173/pr408014/IdentifiableMixin.java

@@ -0,0 +1,24 @@
+import org.aspectj.lang.annotation.*;
+
+public class IdentifiableMixin implements MyAspect.IIdentifiable {
+	
+	private String id;
+	
+	public String getPlatformId() {
+		return id;
+	}
+	
+}
+
+@Aspect
+class MyAspect {
+  
+	public interface IIdentifiable {
+		String getPlatformId();
+	}
+
+    @DeclareMixin("!is(InterfaceType) && !is(EnumType)")
+    public static IIdentifiable createIIdentifiable() {
+        return new IdentifiableMixin();
+    }
+}

tests/bugs173/pr408014/MyAspect.java

@@ -0,0 +1,17 @@
+import org.aspectj.lang.annotation.*;
+
+@Aspect
+class MyAspect {
+  
+	public interface IIdentifiable {
+		UUID getPlatformId();
+		void setPlatformId(UUID id);
+	}
+
+    @DeclareMixin("!is(InterfaceType) && !is(EnumType)")
+    public static IIdentifiable createIIdentifiable() {
+        return new IdentifiableMixin();
+    }
+}
+
+class UUID {}

tests/bugs173/pr408014/UUID.java

@@ -0,0 +1 @@
+class UUID {}

tests/src/org/aspectj/systemtest/ajc173/Ajc173Tests.java

@@ -1,5 +1,5 @@
 /*******************************************************************************
- * Copyright (c) 2012 Contributors
+ * Copyright (c) 2013 Contributors
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
  * which accompanies this distribution, and is available at
@@ -24,6 +24,18 @@ import org.aspectj.testing.XMLBasedAjcTestCase;
  */
 public class Ajc173Tests extends org.aspectj.testing.XMLBasedAjcTestCase {
 
+	public void testAddRemoveAnnos_407739() throws Exception {
+		runTest("add remove annos");
+	}
+	
+//	public void testOrdering_407966() throws Exception {
+//		runTest("ordering");
+//	}
+//	
+//	public void testInnerInterfaceMixin_408014() throws Exception {
+//		runTest("inner interface mixin");
+//	}
+	
 	public void testClassAnnoValue_405016_1() throws Exception {
 		// test that class literals allowed
 		runTest("class anno value 1");
@@ -57,189 +69,6 @@ public class Ajc173Tests extends org.aspectj.testing.XMLBasedAjcTestCase {
 		assertEquals(1,ags.length);
 		assertEquals("LFoo;",ags[0].getTypeSignature());
 	}
-	
-	// still broken!
-//	public void testDeclareAnnoOnItd2() throws Exception {
-//		runTest("declare anno on itd 2");
-//		JavaClass jc = getClassFrom(ajc.getSandboxDirectory(),"C");
-//		Method m = getMethodStartsWith(jc, "getName");
-//		assertNotNull(m);
-//		AnnotationGen[] ags = m.getAnnotations();
-//		for (int i=0;i<ags.length;i++) {
-//			System.out.println(ags[i]);
-//		}
-//		assertEquals(1,ags.length);
-//		assertEquals("LFoo;",ags[0].getTypeSignature());
-//	}
-	
-//	// if the test is failing because the classes won't run, remove the run blocks from the ajc172.xml entry and re-run to check signatures.
-//	public void testSignatures_pr394535() throws Exception {
-//		runTest("signatures");
-//		
-//		JavaClass jc = getClassFrom(ajc.getSandboxDirectory(),"Bug2$ClassA2"); // the working one
-//		String sss = jc.getSignatureAttribute().getSignature();
-//		assertEquals("<T::LBug2$Interface12;:LBug2$Interface22;>Ljava/lang/Object;Ljava/io/Serializable;", sss);
-//		
-//		jc = getClassFrom(ajc.getSandboxDirectory(),"Bug$ClassA");
-//		sss = jc.getSignatureAttribute().getSignature();
-//		assertEquals("<T::LBug$Interface1;:LBug$Interface2;>Ljava/lang/Object;Ljava/io/Serializable;", sss);
-//	}
-//	
-//	// extends
-//	public void testPSignatures_pr399590() throws Exception {
-//		runTest("p signatures 1");
-//		JavaClass jc = getClassFrom(ajc.getSandboxDirectory(),"Cage");
-//		String sss = jc.getSignatureAttribute().getSignature();
-//		assertEquals("<T:LAnimal<+LCage<TT;>;>;>LBar;", sss);
-//		jc = getClassFrom(ajc.getSandboxDirectory(),"Cage2");
-//		sss = jc.getSignatureAttribute().getSignature();
-//		assertEquals("<T:LAnimal2<+LCage2<TT;>;>;>LBar2;Ljava/io/Serializable;", sss);
-//	}
-//	
-//	// extends two classes
-//	public void testPSignatures_pr399590_2() throws Exception {
-//		runTest("p signatures 2");
-//		JavaClass jc = getClassFrom(ajc.getSandboxDirectory(),"Cage");
-//		String sss = jc.getSignatureAttribute().getSignature();
-//		assertEquals("<T:LAnimal<+LCage<TT;LIntf;>;LIntf;>;Q:Ljava/lang/Object;>LBar;", sss);
-//		jc = getClassFrom(ajc.getSandboxDirectory(),"Cage2");
-//		sss = jc.getSignatureAttribute().getSignature();
-//		assertEquals("<T:LAnimal2<+LCage2<TT;LIntf2;>;LIntf2;>;Q:Ljava/lang/Object;>LBar2;Ljava/io/Serializable;", sss);
-//	}
-//	
-//	// super
-//	public void testPSignatures_pr399590_3() throws Exception {
-//		runTest("p signatures 3");
-//		JavaClass jc = getClassFrom(ajc.getSandboxDirectory(),"Cage");
-//		String sss = jc.getSignatureAttribute().getSignature();
-//		assertEquals("<T:LAnimal<-LXXX<TT;>;>;>LBar;", sss);
-//		jc = getClassFrom(ajc.getSandboxDirectory(),"Cage2");
-//		sss = jc.getSignatureAttribute().getSignature();
-//		assertEquals("<T:LAnimal2<-LXXX2<TT;>;>;>LBar2;Ljava/io/Serializable;", sss);
-//	}
-//
-//	// super
-//	public void testPSignatures_pr399590_4() throws Exception {
-//		runTest("p signatures 4");
-//		JavaClass jc = getClassFrom(ajc.getSandboxDirectory(),"Cage");
-//		String sss = jc.getSignatureAttribute().getSignature();
-//		assertEquals("<T:LAnimal<-LXXX<TT;>;LYYY;>;>LBar;", sss);
-//		jc = getClassFrom(ajc.getSandboxDirectory(),"Cage2");
-//		sss = jc.getSignatureAttribute().getSignature();
-//		assertEquals("<T:LAnimal2<-LXXX2<TT;>;LYYY2;>;>LBar2;Ljava/io/Serializable;", sss);
-//	}
-//
-//	// unbound
-//	public void testPSignatures_pr399590_5() throws Exception {
-//		runTest("p signatures 5");
-//		JavaClass jc = getClassFrom(ajc.getSandboxDirectory(),"Cage");
-//		String sss = jc.getSignatureAttribute().getSignature();
-//		assertEquals("<T:LAnimal<*>;>LBar;", sss);
-//		jc = getClassFrom(ajc.getSandboxDirectory(),"Cage2");
-//		sss = jc.getSignatureAttribute().getSignature();
-//		assertEquals("<T:LAnimal2<*>;>LBar2;Ljava/io/Serializable;", sss);
-//	}
-//
-//	public void testIfPointcutNames_pr398246() throws Exception {
-//		runTest("if pointcut names");
-//		JavaClass jc = getClassFrom(ajc.getSandboxDirectory(), "X");
-//		Method m = getMethodStartsWith(jc, "ajc$if");
-//		assertEquals("ajc$if$andy", m.getName());
-//	}
-//
-//	public void testIfPointcutNames_pr398246_2() throws Exception {
-//		runTest("if pointcut names 2");
-//		JavaClass jc = getClassFrom(ajc.getSandboxDirectory(), "X");
-//		Method m = getMethodStartsWith(jc, "ajc$if");
-//		assertEquals("ajc$if$fred", m.getName());
-//	}
-//
-//	// fully qualified annotation name is used
-//	public void testIfPointcutNames_pr398246_3() throws Exception {
-//		runTest("if pointcut names 3");
-//		JavaClass jc = getClassFrom(ajc.getSandboxDirectory(), "X");
-//		Method m = getMethodStartsWith(jc, "ajc$if");
-//		assertEquals("ajc$if$barney", m.getName());
-//	}
-//
-//	// compiling a class later than the initial build - does it pick up the
-//	// right if clause name?
-//	public void testIfPointcutNames_pr398246_4() throws Exception {
-//		runTest("if pointcut names 4");
-//		JavaClass jc = getClassFrom(ajc.getSandboxDirectory(), "X");
-//		Method m = getMethodStartsWith(jc, "ajc$if");
-//		assertEquals("ajc$if$sid", m.getName());
-//	}
-//
-//	// new style generated names
-//	public void testIfPointcutNames_pr398246_5() throws Exception {
-//		runTest("if pointcut names 5");
-//		JavaClass jc = getClassFrom(ajc.getSandboxDirectory(), "X");
-//		Method m = getMethodStartsWith(jc, "ajc$if");
-//		assertEquals("ajc$if$ac0cb804", m.getName());
-//
-//		jc = getClassFrom(ajc.getSandboxDirectory(), "X2");
-//		m = getMethodStartsWith(jc, "ajc$if");
-//		assertEquals("ajc$if$ac0cb804", m.getName());
-//	}
-//
-//	// new style generated names - multiple ifs in one pointcut
-//	public void testIfPointcutNames_pr398246_6() throws Exception {
-//		runTest("if pointcut names 6");
-//		JavaClass jc = getClassFrom(ajc.getSandboxDirectory(), "X");
-//		Method m = getMethodStartsWith(jc, "ajc$if",1);
-//		assertEquals("ajc$if$aac93da8", m.getName());
-//		m = getMethodStartsWith(jc, "ajc$if",2);
-//		assertEquals("ajc$if$1$ae5e778a", m.getName());
-//	}
-//
-//	// new style generated names - multiple ifs in one advice
-//	public void testIfPointcutNames_pr398246_7() throws Exception {
-//		runTest("if pointcut names 7");
-//		JavaClass jc = getClassFrom(ajc.getSandboxDirectory(), "X");
-//		Method m = getMethodStartsWith(jc, "ajc$if",1);
-//		assertEquals("ajc$if$1$ac0607c", m.getName());
-//		m = getMethodStartsWith(jc, "ajc$if",2);
-//		assertEquals("ajc$if$1$1$4d4baf36", m.getName());
-//	}
-//
-//	public void testOptionalAspects_pr398588() {
-//		runTest("optional aspects");
-//	}
-//
-//	public void testInconsistentClassFile_pr389750() {
-//		runTest("inconsistent class file");
-//	}
-//
-//	public void testInconsistentClassFile_pr389750_2() {
-//		runTest("inconsistent class file 2");
-//	}
-//
-//	public void testInconsistentClassFile_pr389750_3() {
-//		runTest("inconsistent class file 3");
-//	}
-//
-//	public void testInconsistentClassFile_pr389750_4() {
-//		runTest("inconsistent class file 4");
-//	}
-//
-//	public void testAnnotationValueError_pr389752_1() {
-//		runTest("annotation value error 1");
-//	}
-//
-//	public void testAnnotationValueError_pr389752_2() {
-//		runTest("annotation value error 2");
-//	}
-//
-//	// this needs some cleverness to fix... the annotation value is parsed as a
-//	// string and then not checked
-//	// to see if the user is accidentally supplying, for example, an enum value.
-//	// Due to the use of strings, it
-//	// is hard to check. The verification code might go here:
-//	// WildAnnotationTypePattern, line 205 (the string case)
-//	// public void testAnnotationValueError_pr389752_3() {
-//	// runTest("annotation value error 3");
-//	// }
 
 	// ---
 

tests/src/org/aspectj/systemtest/ajc173/ajc173.xml

@@ -2,6 +2,28 @@
 
 <suite>
 
+	<ajc-test dir="bugs173/pr407739" title="add remove annos">
+		<compile files="MyAnnotation.java Hello.java Aspect.java" options="-1.5 -showWeaveInfo">
+			<message kind="weave" text="'private String dummy [RuntimeVisibleAnnotations]' of type 'Hello' (Hello.java) has had @MyAnnotation field annotation removed by 'Aspect' (Aspect.java:3)"/>
+			<message kind="weave" text="'private String dummy [RuntimeVisibleAnnotations]' of type 'Hello' (Hello.java) is annotated with @MyAnnotation(dummy2 = &quot;korte&quot;) field annotation from 'Aspect' (Aspect.java:4)"/>		
+		</compile>
+		<run class="Hello">
+		<stdout>
+		<line text="@MyAnnotation(dummy2=korte, dummy1=)"/>
+		</stdout>
+		</run>
+	</ajc-test>
+	
+	<ajc-test dir="bugs173/pr408014" title="inner interface mixin">
+		<compile files="IdentifiableMixin.java" options="-1.5 -showWeaveInfo">		
+		</compile>
+	</ajc-test>
+	
+	<ajc-test dir="bugs173/pr407966" title="ordering">
+		<compile files="Aspect.aj Use.java Def.java" options="-1.5 -showWeaveInfo">
+		</compile>
+	</ajc-test>
+
 	<ajc-test dir="bugs173/pr407494" title="inner names">
 		<compile files="A.java" options="-1.5 -showWeaveInfo">		
 			<message kind="weave" text="Join point 'staticinitialization(void a.b.c.A$B.&lt;clinit&gt;())' in Type 'a.b.c.A$B' (A.java:4) advised by before advice from 'a.b.c.X' (A.java:13)"/>

weaver/src/org/aspectj/weaver/bcel/BcelClassWeaver.java

@@ -1264,6 +1264,26 @@ class BcelClassWeaver implements IClassWeaver {
 		return false;
 	}
 
+	/**
+	 * Remove an annotation from the supplied array, if it is in there.
+	 */
+	private AnnotationAJ[] removeFromAnnotationsArray(AnnotationAJ[] annotations,AnnotationAJ annotation) {
+		for (int i=0;i<annotations.length;i++) {
+			if (annotations[i] != null && annotation.getTypeName().equals(annotations[i].getTypeName())) {
+				// Remove it!
+				AnnotationAJ[] newArray = new AnnotationAJ[annotations.length-1];
+				int index=0;
+				for (int j=0;j<annotations.length;j++) {
+					if (j!=i) {
+						newArray[index++]=annotations[j];
+					}
+				}
+				return newArray;
+			}
+		}
+		return annotations;
+	}
+
 	// BUGWARNING not getting enough warnings out on declare @field ? There is a potential problem here with warnings not
 	// coming out - this will occur if they are created on the second iteration round this loop.
 	// We currently deactivate error reporting for the second time round. A possible solution is to record what annotations
@@ -1320,6 +1340,7 @@ class BcelClassWeaver implements IClassWeaver {
 									AsmRelationshipProvider.addDeclareAnnotationFieldRelationship(world.getModelAsAsmManager(),
 											decaf.getSourceLocation(), clazz.getName(), field, true);
 									reportFieldAnnotationWeavingMessage(clazz, field, decaf, true);
+									dontAddMeTwice = removeFromAnnotationsArray(dontAddMeTwice, annotation);
 								} else {
 									worthRetrying.add(decaf);
 								}