333274: more tests and fixes: nested @around advice with proceed

org.aspectj.matcher/src/main/java/org/aspectj/weaver/Advice.java

@@ -83,6 +83,10 @@ public static Advice makePerTypeWithinEntry(World world, Pointcut p, ResolvedTyp
 		ret.concreteAspect = inAspect;
 		return ret;
 	}
+
+	public boolean isAroundAdvice() {
+		return attribute.getKind() == AdviceKind.Around;
+	}
 
 	public static Advice makeSoftener(World world, Pointcut entry, TypePattern exceptionType, ResolvedType inAspect,
 			IHasSourceLocation loc) {

org.aspectj.matcher/src/main/java/org/aspectj/weaver/Shadow.java

@@ -46,7 +46,8 @@ public abstract class Shadow {
 	private ResolvedMember resolvedSignature;
 	protected final Shadow enclosingShadow;
 	protected List<ShadowMunger> mungers = Collections.emptyList();
-
+	protected boolean needAroundClosureStacking = false;
+
 	public int shadowId = nextShadowID++; // every time we build a shadow, it gets a new id
 
 	// ----
@@ -628,6 +629,20 @@ protected void prepareForMungers() {
 	/** Actually implement the (non-empty) mungers associated with this shadow */
 	private void implementMungers() {
 		World world = getIWorld(); 
+		needAroundClosureStacking = false;
+		int annotationStyleWithAroundAndProceedCount = 0;
+		for (ShadowMunger munger: mungers) {
+			if (munger.getDeclaringType()!= null && 
+				munger.getDeclaringType().isAnnotationStyleAspect() &&
+				munger.isAroundAdvice() &&
+				munger.bindsProceedingJoinPoint()) {
+				annotationStyleWithAroundAndProceedCount++;
+				if (annotationStyleWithAroundAndProceedCount>1) {
+					needAroundClosureStacking = true;
+					break;
+				}
+			}
+		}
 		for (ShadowMunger munger : mungers) {
 			if (munger.implementOn(this)) {
 				world.reportMatch(munger, this);

org.aspectj.matcher/src/main/java/org/aspectj/weaver/ShadowMunger.java

@@ -303,5 +303,13 @@ public void write(CompressingDataOutputStream stream) throws IOException {
 	// }
 	// newShadowMunger.binaryFile = null;
 	// }
+
+	public boolean bindsProceedingJoinPoint() {
+		return false;
+	}
+
+	public boolean isAroundAdvice() {
+		return false;
+	}
 
 }

runtime/src/main/java/org/aspectj/lang/ProceedingJoinPoint.java

@@ -29,6 +29,19 @@ public interface ProceedingJoinPoint extends JoinPoint {
      */
     void set$AroundClosure(AroundClosure arc);
 
+    /**
+     * The joinpoint needs to know about its closure so that proceed can delegate to closure.run().
+     * This internal method should not be called directly, and won't be visible to the end-user when
+     * packed in a jar (synthetic method). This should maintain a stack of closures as multiple around
+     * advice with proceed are targeting a joinpoint and the stack will need to be unwound when
+     * exiting nested advice. Passing a non null arc indicates a push, passing null indicates a pop.
+     *
+     * @param arc the around closure to associate with this joinpoint
+     */
+     default void stack$AroundClosure(AroundClosure arc) {
+    	 throw new UnsupportedOperationException();
+     }
+
     /**
      * Proceed with the next advice or target method invocation
      *

runtime/src/main/java/org/aspectj/runtime/internal/AroundClosure.java

@@ -65,6 +65,21 @@ public ProceedingJoinPoint linkClosureAndJoinPoint() {
         return jp;
     }
 
+    /**
+     * This method is called to implicitly associate the closure with the joinpoint
+     * as required for @AJ aspect proceed()
+     * 
+     * @param flags indicating whether this/target found at joinpoint and bound
+     * @return the associated ProceedingJoinPoint
+     */
+    public ProceedingJoinPoint linkStackClosureAndJoinPoint(int flags) {
+        //TODO is this cast safe ?
+        ProceedingJoinPoint jp = (ProceedingJoinPoint)state[state.length-1];
+        jp.stack$AroundClosure(this);
+        this.bitflags = flags;
+        return jp;
+    }
+
     /**
      * This method is called to implicitly associate the closure with the joinpoint
      * as required for @AJ aspect proceed()
@@ -79,4 +94,10 @@ public ProceedingJoinPoint linkClosureAndJoinPoint(int flags) {
         this.bitflags = flags;
         return jp;
     }
+
+    public void unlink() {
+    	ProceedingJoinPoint jp = (ProceedingJoinPoint)state[state.length-1];
+    	jp.stack$AroundClosure(null);
+    }
+
 }

runtime/src/main/java/org/aspectj/runtime/reflect/JoinPointImpl.java

@@ -13,6 +13,8 @@
 
 package org.aspectj.runtime.reflect;
 
+import java.util.Stack;
+
 import org.aspectj.lang.JoinPoint;
 import org.aspectj.lang.ProceedingJoinPoint;
 import org.aspectj.lang.Signature;
@@ -134,47 +136,73 @@ public final String toLongString() {
 		return staticPart.toLongString();
 	}
 
-	// To proceed we need a closure to proceed on
-	private AroundClosure arc;
+	// To proceed we need a closure to proceed on. Generated code
+	// will either be using arc or arcs but not both. arcs being non-null
+	// indicates it is in use (even if an empty stack)
+	private AroundClosure arc = null;	
+	private Stack<AroundClosure> arcs = null;
 
 	public void set$AroundClosure(AroundClosure arc) {
 		this.arc = arc;
 	}
 
+ 	public void stack$AroundClosure(AroundClosure arc) {
+		// If input parameter arc is null this is the 'unlink' call from AroundClosure
+		if (arcs == null) {
+			arcs = new Stack<AroundClosure>();
+		}
+		if (arc==null) {
+			this.arcs.pop();
+		} else {
+			this.arcs.push(arc);
+		}
+ 	}
+
 	public Object proceed() throws Throwable {
 		// when called from a before advice, but be a no-op
-		if (arc == null)
-			return null;
-		else
-			return arc.run(arc.getState());
+		if (arcs == null) {
+			if (arc == null) {
+				return null;
+			} else {
+				return arc.run(arc.getState());
+			}
+		} else {
+			return arcs.peek().run(arcs.peek().getState());			
+		}
 	}
 
 	public Object proceed(Object[] adviceBindings) throws Throwable {
 		// when called from a before advice, but be a no-op
-		if (arc == null)
+		AroundClosure ac = null;
+		if (arcs == null) {
+			ac = arc;
+		} else {
+			ac = arcs.peek();
+		}
+
+		if (ac == null) {
 			return null;
-		else {
-
+		} else {
 			// Based on the bit flags in the AroundClosure we can determine what to
 			// expect in the adviceBindings array. We may or may not be expecting
 			// the first value to be a new this or a new target... (see pr126167)
-			int flags = arc.getFlags();
+			int flags = ac.getFlags();
 			boolean unset = (flags & 0x100000) != 0;
 			boolean thisTargetTheSame = (flags & 0x010000) != 0;
 			boolean hasThis = (flags & 0x001000) != 0;
 			boolean bindsThis = (flags & 0x000100) != 0;
 			boolean hasTarget = (flags & 0x000010) != 0;
 			boolean bindsTarget = (flags & 0x000001) != 0;
-
+	
 			// state is always consistent with caller?,callee?,formals...,jp
-			Object[] state = arc.getState();
-
+			Object[] state = ac.getState();
+	
 			// these next two numbers can differ because some join points have a this and
 			// target that are the same (eg. call) - and yet you can bind this and target
 			// separately.
-
+	
 			// In the state array, [0] may be this, [1] may be target
-
+	
 			int firstArgumentIndexIntoAdviceBindings = 0;
 			int firstArgumentIndexIntoState = 0;
 			firstArgumentIndexIntoState += (hasThis ? 1 : 0);
@@ -202,8 +230,8 @@ public Object proceed(Object[] adviceBindings) throws Throwable {
 						// This previous variant doesn't seem to cope with only binding target at a joinpoint
 						// which has both this and target. It forces you to supply this even if you didn't bind
 						// it.
-//						firstArgumentIndexIntoAdviceBindings = (hasThis ? 1 : 0) + 1;
-//						state[hasThis ? 1 : 0] = adviceBindings[hasThis ? 1 : 0];
+	//						firstArgumentIndexIntoAdviceBindings = (hasThis ? 1 : 0) + 1;
+	//						state[hasThis ? 1 : 0] = adviceBindings[hasThis ? 1 : 0];
 
 						int targetPositionInAdviceBindings = (hasThis && bindsThis) ? 1 : 0;
 						firstArgumentIndexIntoAdviceBindings = ((hasThis&&bindsThis)?1:0)+((hasTarget&&bindsTarget&&!thisTargetTheSame)?1:0);
@@ -213,20 +241,20 @@ public Object proceed(Object[] adviceBindings) throws Throwable {
 					// leave state[0]/state[1] alone, they are OK
 				}
 			}
-
+	
 			// copy the rest across
 			for (int i = firstArgumentIndexIntoAdviceBindings; i < adviceBindings.length; i++) {
 				state[firstArgumentIndexIntoState + (i - firstArgumentIndexIntoAdviceBindings)] = adviceBindings[i];
 			}
-
+	
 			// old code that did this, didnt allow this/target overriding
 			// for (int i = state.length-2; i >= 0; i--) {
 			// int formalIndex = (adviceBindings.length - 1) - (state.length-2) + i;
 			// if (formalIndex >= 0 && formalIndex < adviceBindings.length) {
 			// state[i] = adviceBindings[formalIndex];
 			// }
 			// }
-			return arc.run(state);
+			return ac.run(state);
 		}
 	}
 

tests/src/test/java/org/aspectj/systemtest/ajc1611/Ajc1611Tests.java

@@ -103,17 +103,17 @@ public void testDeclareField_328840() {
 		runTest("pr328840");
 	}
 
-	// public void testAnnoStyleAdviceChain_333274() {
-	// runTest("anno style advice chain");
-	// }
-	//
-	// public void testAnnoStyleAdviceChain_333274_2() {
-	// runTest("code style advice chain");
-	// }
-	//
-	// public void testAnnoStyleAdviceChain_333274_3() {
-	// runTest("code style advice chain - no inline");
-	// }
+	 public void testAnnoStyleAdviceChain_333274() {
+		 runTest("anno style advice chain");
+	 }
+
+	 public void testAnnoStyleAdviceChain_333274_2() {
+		 runTest("code style advice chain");
+	 }
+
+	 public void testAnnoStyleAdviceChain_333274_3() {
+		 runTest("code style advice chain - no inline");
+	 }
 
 	// ---
 

tests/src/test/java/org/aspectj/systemtest/ajc193/Ajc193Tests.java

@@ -5,7 +5,7 @@
  * which accompanies this distribution, and is available at
  * http://www.eclipse.org/legal/epl-v10.html
  *******************************************************************************/
-package org.aspectj.systemtest.ajc193;
+package org.aspectj.systemtest.ajc193; 
 
 import java.io.File;
 
@@ -18,9 +18,13 @@
 
 /**
  * @author Andy Clement
- */
+ */ 
 public class Ajc193Tests extends XMLBasedAjcTestCaseForJava10OrLater {
 
+	public void testNestedAroundProceed() {
+		runTest("nested around proceed");
+	}
+
 	public void testDeclareMixinOverweavingControl() {
 		runTest("overweaving decm - control");
 	}

tests/src/test/resources/org/aspectj/systemtest/ajc1611/ajc1611.xml

@@ -117,7 +117,7 @@
   </ajc-test>
 
   <ajc-test dir="bugs1611/pr333274" title="code style advice chain">
-  <compile files="ma2/Annotation1.java  ma2/aspect1/Aspect1.java  ma2/aspect3/Aspect3.java ma2/Main.java ma2/Precedence.java" options="-1.5 "/>
+  <compile files="ma2/Annotation1.java  ma2/aspect1/Aspect1.java  ma2/aspect3/Aspect3.java ma2/Main.java ma2/Precedence.java" options="-1.5 -XnoInline"/>
   <run class="ma2.Main">
   <stdout>
   <line text="&gt;In Aspect1"/>

tests/src/test/resources/org/aspectj/systemtest/ajc190/ajc190.xml

@@ -77,8 +77,8 @@
   </ajc-test>
 
   <ajc-test dir="bugs190/modules/fff" title="compile module including aspects">
-    <compile files="module-info.java pkg/Demo.java otherpkg/Azpect.java" modulepath="$runtime" outjar="demomodule.jar" options="-1.9"/>
-    <run modulepath="$runtime:demomodule.jar" module="demo/pkg.Demo">
+    <compile files="module-info.java pkg/Demo.java otherpkg/Azpect.java" modulepath="$runtimemodule" outjar="demomodule.jar" options="-1.9"/>
+    <run modulepath="$runtimemodule:demomodule.jar" module="demo/pkg.Demo">
       <stdout>
       <line text="Azpect running"/>
       <line text="Demo running"/>

tests/src/test/resources/org/aspectj/systemtest/ajc193/ajc193.xml

@@ -2,6 +2,42 @@
 
 <suite>
 
+    <ajc-test dir="bugs193/333274" vm="1.8" title="nested around proceed">
+        <compile files="ma/aspect2/Aspect2.java,ma/aspect2/Annotation2.java,ma/aspect3/Aspect3.java,ma/aspect3/Annotation3.java,ma/Precedence.java,ma/Main.java,ma/aspect1/Aspect1.java,ma/aspect1/Annotation1.java" options="-showWeaveInfo -1.8 -XnoInline">
+	        <message kind="weave" text="Join point 'method-execution(int ma.Main$Dummy.retryTranslateAndTimeLimited())' in Type 'ma.Main$Dummy' (Main.java:16) advised by around advice from 'ma.aspect3.Aspect3' (Aspect3.java:11)"/>
+	        <message kind="weave" text="Join point 'method-execution(int ma.Main$Dummy.retryTranslateAndTimeLimited())' in Type 'ma.Main$Dummy' (Main.java:16) advised by around advice from 'ma.aspect2.Aspect2' (Aspect2.java:11)"/>
+	        <message kind="weave" text="Join point 'method-execution(int ma.Main$Dummy.retryTranslateAndTimeLimited())' in Type 'ma.Main$Dummy' (Main.java:16) advised by around advice from 'ma.aspect1.Aspect1' (Aspect1.java:12)"/>
+        </compile>
+        <!-- 
+        >In Aspect1
+>In Aspect2
+>In Aspect3
+Method call
+<In Aspect3
+<In Aspect2
+=In Aspect1
+Method call
+<In Aspect1
+ -->
+        <run class="ma.Main">
+	        <stdout>
+	        <line text="&gt;In Aspect1"/>
+	        <line text="&gt;In Aspect2"/>
+	        <line text="&gt;In Aspect3"/>
+	        <line text="Method call"/>
+	        <line text="&lt;In Aspect3"/>
+	        <line text="&lt;In Aspect2"/>
+	        <line text="=In Aspect1"/>
+	        <line text="&gt;In Aspect2"/>
+	        <line text="&gt;In Aspect3"/>
+	        <line text="Method call"/>
+	        <line text="&lt;In Aspect3"/>
+	        <line text="&lt;In Aspect2"/>
+			<line text="&lt;In Aspect1"/>
+	        </stdout>
+        </run>
+    </ajc-test>
+
     <ajc-test dir="bugs193/543657" vm="1.8" title="overweaving decm - control">
         <compile files="MoodIndicator.java,Code1.java" options="-showWeaveInfo -1.8">
 	        <message kind="weave" text="Mixing interface 'MoodIndicator$Moody' (MoodIndicator.java) into type 'Code1' (Code1.java)"/>

weaver/src/main/java/org/aspectj/weaver/bcel/BcelAdvice.java

@@ -76,7 +76,17 @@ public BcelAdvice(AjAttribute.AdviceAttribute attribute, Pointcut pointcut, Memb
 		super(attribute, pointcut, simplify(attribute.getKind(), adviceSignature));
 		this.concreteAspect = concreteAspect;
 	}
-
+
+	public boolean bindsProceedingJoinPoint() {
+		UnresolvedType[] parameterTypes = signature.getParameterTypes();
+		for (int i=0;i<parameterTypes.length;i++) {
+			if (parameterTypes[i].equals(UnresolvedType.PROCEEDING_JOINPOINT)) {
+				return true;
+			}
+		}
+		return false;
+	}
+
 	/**
 	 * A heavyweight BcelMethod object is only required for around advice that will be inlined. For other kinds of advice it is
 	 * possible to save some space.
@@ -603,6 +613,7 @@ public InstructionList getAdviceArgSetup(BcelShadow shadow, BcelVar extraVar, In
 							} else {
 								previousIsClosure = true;
 								il.append(closureInstantiation.copy());
+								shadow.closureVarInitialized = true;
 							}
 						}
 					} else if ("Lorg/aspectj/lang/JoinPoint$StaticPart;".equals(getSignature().getParameterTypes()[i]