126167: Fix for @Around problems...

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

  * Contributors: 
  *     Xerox/PARC     initial implementation 
  *    Alex Vasseur    wired up for @AJ proceeding
+ *    Andy Clement 23-06-06 added extras for @AJ
  * ******************************************************************/
 
 
 
 public abstract class AroundClosure {
     protected Object[] state;
+
+    // Records with the related joinpoint has a this or a target and whether 
+    // either of them are bound in the pointcut.  Set in the 'link' call made
+    // at each matching join point... (see pr126167)
+    // bit6 being 1 means the flags haven't been initialized
+    protected int bitflags = 0x100000; 
     protected Object[] preInitializationState;
 
     public AroundClosure() {
     	this.state = state;
     }
     
-    
+    public int getFlags() {return bitflags;}
 
     public Object[] getState() {
       return state;
         jp.set$AroundClosure(this);
         return jp;
     }
+
+    /**
+     * This method is called to implicitly associate the closure with the joinpoint
+     * as required for @AJ aspect proceed()
+     */
+    public ProceedingJoinPoint linkClosureAndJoinPoint(int flags) {
+        //TODO is this cast safe ?
+        ProceedingJoinPoint jp = (ProceedingJoinPoint)state[state.length-1];
+        jp.set$AroundClosure(this);
+        this.bitflags = flags;
+        return jp;
+    }
 }

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

         if (arc == null)
             return null;
         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();
+        	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();
-            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];
-                }
+            
+            // 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);
+            firstArgumentIndexIntoState+=(hasTarget&&!thisTargetTheSame?1:0);
+            if (hasThis) {
+            	if (bindsThis) {
+            		// replace [0] (this)
+	            	firstArgumentIndexIntoAdviceBindings=1;
+	            	state[0]=adviceBindings[0];
+            	} else {
+            		// leave state[0] alone, its OK
+            	}
+            }
+            if (hasTarget) {
+            	if (bindsTarget) {
+                  if (thisTargetTheSame) {
+                	  // this and target are the same so replace state[0]
+		        	  firstArgumentIndexIntoAdviceBindings=1+(bindsThis?1:0);
+		        	  state[0]=adviceBindings[(bindsThis?1:0)];
+		          } else {
+		        	  // need to replace the target, and it is different to this, whether
+		        	  // that means replacing state[0] or state[1] depends on whether
+		        	  // the join point has a this
+		        	  firstArgumentIndexIntoAdviceBindings=(hasThis?1:0)+1;
+		        	  state[hasThis?1:0]=adviceBindings[hasThis?1:0]; 	  
+		          }
+            	} else {
+            		// 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);
         }
     }
-
+    
 
 }

tests/src/org/aspectj/systemtest/ajc151/AtAroundTests.java

  * @author AndyClement
  *
  */
-
-
 public class AtAroundTests extends XMLBasedAjcTestCase {
 
-  public void testCodeBasic() {       runTest("code style - basic"); }
-  public void testAtBasic()   { runTest("annotation style - basic"); }
+  public void testCodeBasic()       { runTest("code style - basic"); }
+  public void testAtBasicNoInline() { runTest("annotation style - basic - noinline"); }
+  public void testAtBasic()         { runTest("annotation style - basic"); }
   
+  public void testCodeBindingTarget()       { runTest("code style - correct usage, binding and passing same target for call"); }
+  public void testAtBindingTargetNoInline() { runTest("annotation style - correct usage, binding and passing same target for call - noinline"); }
+  public void testAtBindingTarget()         { runTest("annotation style - correct usage, binding and passing same target for call"); }
+
+  public void testCodeBindingTarget2()       { runTest("code style - correct usage, binding and passing new target for call"); }
+  public void testAtBindingTargetNoInline2() { runTest("annotation style - correct usage, binding and passing new target for call - noinline"); }
+  public void testAtBindingTarget2()         { runTest("annotation style - correct usage, binding and passing new target for call"); }
+
   public void testCodeErrorCase1() {     runTest("code style - forget to pass target");}
   // Don't think we can report correct errors for @AJ as the parameters are specified as an object array
-  // public void testAtErrorCase1() { runTest("annotation style - forget to pass target");}
+  //public void testAtErrorCase1() { runTest("annotation style - forget to pass target");}
+
+  public void testCodeBindThisCallChangeProceed()       { runTest("code style - bind this on call - change on proceed - no effect");} 
+  public void testAtBindThisCallChangeProceedNoInline() { runTest("annotation style - bind this on call - change on proceed - no effect - noinline");}
+  public void testAtBindThisCallChangeProceed()         { runTest("annotation style - bind this on call - change on proceed - no effect");}
+
+  public void testCodeBindThisExecutionChangeProceed()       { runTest("code style - bind this on execution - change on proceed - works");}
+  public void testAtBindThisExecutionChangeProceedNoInline() { runTest("annotation style - bind this on execution - change on proceed - works - noinline");}
+  public void testAtBindThisExecutionChangeProceed()         { runTest("annotation style - bind this on execution - change on proceed - works");}
+
+  public void testCodeBindBothExecutionChangeProceed()       { runTest("code style - bind this and target on execution - change on proceed - works");}
+  public void testAtBindBothExecutionChangeProceedNoInline() { runTest("annotation style - bind this and target on execution - change on proceed - works - noinline");}
+  public void testAtBindBothExecutionChangeProceed()         { runTest("annotation style - bind this and target on execution - change on proceed - works");}
 
   public void testCodeErrorCase2() {     runTest("code style - incorrect arg types");}
   // Don't think we can report correct errors for @AJ as the parameters are specified as an object array
   // public void testAtErrorCase2() { runTest("annotation style - incorrect arg types");}
-  
-  public void testCodeBindingTarget() {       runTest("code style - correct usage, binding and passing new target for call"); }
-  public void testAtBindingTarget()   { runTest("annotation style - correct usage, binding and passing new target for call"); }
 
-  public void testCodeChangingTarget() {       runTest("code style - changing target for call"); }
-  public void testAtChangingTarget()   { runTest("annotation style - changing target for call"); }
-  
   public void testCodeChangingTargetDifferingOrder() {       runTest("code style - changing target for call - reverse order"); }
   // @AJ cant cope with the changing of the order of arguments bound and passed through proceed
   //public void testAtChangingTargetDifferingOrder()   { runTest("annotation style - changing target for call - reverse order"); }
- 
-  public void testCodeBindThisCallChangeProceed() {     runTest("code style - bind this on call - change on proceed - no effect");} 
-  //public void testAtBindThisCallChangeProceed() { runTest("annotation style - bind this on call - change on proceed - no effect");}
+
+  public void testCodeBindBothCallChangeProceed()       { runTest("code style - bind this and target on call - change on proceed - works");}
+  public void testAtBindBothCallChangeProceedNoInline() { runTest("annotation style - bind this and target on call - change on proceed - works - noinline");}
+  public void testAtBindBothCallChangeProceed()         { runTest("annotation style - bind this and target on call - change on proceed - works");}
   
-  public void testCodeBindThisExecutionChangeProceed() {     runTest("code style - bind this on execution - change on proceed - works");}
-  //public void testAtBindThisExecutionChangeProceed() { runTest("annotation style - bind this on execution - change on proceed - works");}
+  public void testBreakingIt1() { runTest("breaking it - one");}
+  public void testBreakingIt2() { runTest("breaking it - two");}
+  
+  public void testBugCase1() { runTest("bug case one");}
+  public void testBugCase2() { runTest("bug case two");}
+  public void testMultipleArgs() { runTest("multiple args");}
   
-  public void testCodeBindBothExecutionChangeProceed() { runTest("code style - bind this and target on execution - change on proceed - works");}
-  //public void testAtBindBothExecutionChangeProceed() { runTest("annotation style - bind this and target on execution - change on proceed - works");}
- 
-  public void testCodeBindBothCallChangeProceed() { runTest("code style - bind this and target on call - change on proceed - works");}
-  //public void testAtBindBothCallChangeProceed() { runTest("annotation style - bind this and target on call - change on proceed - works");}
-
   public void testCodeSubsetArguments() {     runTest("code style - works with subset of arguments in advice");}
+  // cant do this for annotation style
   //public void testAtSubsetArguments() { runTest("annotation style - works with subset of arguments in advice");}
-
-  
   
-  //
+  // ---
   public static Test suite() {
     return XMLBasedAjcTestCase.loadSuite(AtAroundTests.class);
   }

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

 		range.insert(entryInstructions, Range.InsideBefore);
 	}
     
-    public void weaveAroundInline(
-    	BcelAdvice munger,
-    	boolean hasDynamicTest)
-	{
 		/* Implementation notes:
 		 * 
 		 * AroundInline still extracts the instructions of the original shadow into 
 		 * new method for the advice can also be re-lined.  We are not doing that
 		 * presently.
 		 */
+    public void weaveAroundInline(BcelAdvice munger,boolean hasDynamicTest) {
 		 
 		// !!! THIS BLOCK OF CODE SHOULD BE IN A METHOD CALLED weaveAround(...);
         Member mungerSig = munger.getSignature();
 
         // specific test for @AJ proceedInInners
         if (munger.getConcreteAspect().isAnnotationStyleAspect()) {
-            // if we can't find one proceed()
-            // we suspect that the call is happening in an inner class
-            // so we don't inline it.
+            // if we can't find one proceed() we suspect that the call 
+        	// is happening in an inner class so we don't inline it.
             // Note: for code style, this is done at Aspect compilation time.
             boolean canSeeProceedPassedToOther = false;
             InstructionHandle curr = adviceMethod.getBody().getStart();
 		final InstructionFactory fact = getFactory();
 		
 		// now generate the aroundBody method
+		// eg. "private static final void method_aroundBody0(M, M, String, org.aspectj.lang.JoinPoint)"
         LazyMethodGen extractedMethod = 
         	extractMethod(
-        		NameMangler.aroundCallbackMethodName(
-        			getSignature(),
-        			getEnclosingClass()),
+        		NameMangler.aroundCallbackMethodName(getSignature(),getEnclosingClass()),
 				Modifier.PRIVATE,
-				munger
-            );
-        			
+				munger);
         			
         // now extract the advice into its own method
         String adviceMethodName =
-			NameMangler.aroundCallbackMethodName(
-							getSignature(),
-							getEnclosingClass()) + "$advice";
+			NameMangler.aroundCallbackMethodName(getSignature(),getEnclosingClass()) + "$advice";
         
-		List argVarList = new ArrayList();
-		List proceedVarList = new ArrayList();
+		List argVarList      = new ArrayList();
+		List proceedVarList  = new ArrayList();
 		int extraParamOffset = 0;
 		
 		// Create the extra parameters that are needed for passing to proceed
 		// This code is very similar to that found in makeCallToCallback and should
 		// be rationalized in the future
+		
 		if (thisVar != null) {
 			argVarList.add(thisVar);
 			proceedVarList.add(new BcelVar(thisVar.getType(), extraParamOffset));
     /**
      * ATAJ Handle the inlining for @AJ aspects
      *
-     * @param fact
-     * @param callbackMethod
-     * @param munger
-     * @param localAdviceMethod
-     * @param argVarList
-     * @param isProceedWithArgs
-     * @return
      */
     private InstructionList getRedoneProceedCallForAnnotationStyle(
         InstructionFactory fact,
     {
         // Notes:
         // proceedingjp is on stack (since user was calling pjp.proceed(...)
-        // the boxed args to proceed() are on stack as well (Object[]) unless
-        // the call is to pjp.proceed(<noarg>)
 
         // new Object[]{new Integer(argAdvice1-1)};// arg of proceed
         // call to proceed(..) is NOT made
         // int res = .. from original code
 
         //Note: we just don't care about the proceed map etc
+    	// (we would need to care if we allow repositioning of arguments in advice signature)
 
         InstructionList ret = new InstructionList();
 
         // store the Object[] array on stack if proceed with args
         if (isProceedWithArgs) {
+        	
+        	// STORE the Object[] into a local variable
             Type objectArrayType = Type.getType("[Ljava/lang/Object;");
             int localProceedArgArray = localAdviceMethod.allocateLocal(objectArrayType);
             ret.append(InstructionFactory.createStore(objectArrayType, localProceedArgArray));
 
+            // STORE the ProceedingJoinPoint instance into a local variable
             Type proceedingJpType = Type.getType("Lorg/aspectj/lang/ProceedingJoinPoint;");
             int localJp = localAdviceMethod.allocateLocal(proceedingJpType);
             ret.append(InstructionFactory.createStore(proceedingJpType, localJp));
             // TODO do we want to try catch ClassCast and AOOBE exception ?
 
             // special logic when withincode is static or not
-            int startIndex = 0;
-            if (thisVar != null) {
-                startIndex = 1;
-                //TODO this logic is actually depending on target as well - test me
-                ret.append(new ALOAD(0));//thisVar
+            
+            // This next bit of code probably makes more sense if you read its implementation for
+            // weaveAroundClosure() - see JoinPointImpl.proceed(Object[]).  Basically depending
+            // on whether the join point has a this/target and whether the pointcut binds this/target
+            // then the arguments to the 'new' proceed call need to be reorganized. (pr126167)
+        	boolean relatedPointcutBindsThis = bindsThis(munger);
+        	boolean relatedPointcutBindsTarget = bindsTarget(munger);
+        	boolean targetIsSameAsThis = getKind().isTargetSameAsThis();
+        	
+        	// two numbers can differ because a pointcut may bind both this/target and yet at the
+        	// join point this and target are the same (eg. call)
+            int indexIntoObjectArrayForArguments=0;
+            int indexIntoCallbackMethodForArguments = 0;
+            if (hasThis()) {
+            	if (relatedPointcutBindsThis) {
+            		if (!(relatedPointcutBindsTarget && targetIsSameAsThis)) {
+	            		// they have supplied new this as first entry in object array
+	            		 ret.append(InstructionFactory.createLoad(objectArrayType, localProceedArgArray));
+	                     ret.append(Utility.createConstant(fact, 0));
+	                     ret.append(InstructionFactory.createArrayLoad(Type.OBJECT));
+	                     ret.append(Utility.createConversion(fact,Type.OBJECT,callbackMethod.getArgumentTypes()[0]));
+	                     indexIntoCallbackMethodForArguments++;
+            		 }
+                    indexIntoObjectArrayForArguments=1;
+            	} else {
+            		// use local variable 0 (which is 'this' for a non-static method)
+            		ret.append(new ALOAD(0));
+                	indexIntoCallbackMethodForArguments++;
+            	}
             }
-//    	    if (bindsThisOrTarget(munger.getPointcut())) {                
             
-            for (int i = startIndex, len=callbackMethod.getArgumentTypes().length; i < len; i++) {
+            if (hasTarget()) {
+            	if (relatedPointcutBindsTarget) {
+            		if (getKind().isTargetSameAsThis()) {
+            			 ret.append(InstructionFactory.createLoad(objectArrayType, localProceedArgArray));
+                         ret.append(Utility.createConstant(fact, relatedPointcutBindsThis?1:0));
+                         ret.append(InstructionFactory.createArrayLoad(Type.OBJECT));
+                         ret.append(Utility.createConversion(fact,Type.OBJECT,callbackMethod.getArgumentTypes()[0]));
+                         indexIntoObjectArrayForArguments++;
+                         indexIntoCallbackMethodForArguments++;
+            		} else {
+            			 int position =(hasThis()&& relatedPointcutBindsThis?1:0);
+	           			 ret.append(InstructionFactory.createLoad(objectArrayType, localProceedArgArray));
+	                     ret.append(Utility.createConstant(fact, position));
+	                     ret.append(InstructionFactory.createArrayLoad(Type.OBJECT));
+	                     ret.append(Utility.createConversion(fact,Type.OBJECT,callbackMethod.getArgumentTypes()[position]));
+	                     indexIntoObjectArrayForArguments=position+1;
+	             		 indexIntoCallbackMethodForArguments++;
+            		}
+            	} else {
+                	if (getKind().isTargetSameAsThis()) {
+                		//ret.append(new ALOAD(0));
+                	} else {
+                		ret.append(InstructionFactory.createLoad(localAdviceMethod.getArgumentTypes()[0],hasThis()?1:0));
+                		indexIntoCallbackMethodForArguments++;
+                	}
+            	}
+            }
+            
+            
+            for (int i = indexIntoCallbackMethodForArguments, len=callbackMethod.getArgumentTypes().length; i < len; i++) {
                 Type stateType = callbackMethod.getArgumentTypes()[i];
                 BcelWorld.fromBcel(stateType).resolve(world);
                 if ("Lorg/aspectj/lang/JoinPoint;".equals(stateType.getSignature())) {
                     ret.append(new ALOAD(localJp));// from localAdvice signature
                 } else {
                     ret.append(InstructionFactory.createLoad(objectArrayType, localProceedArgArray));
-                    ret.append(Utility.createConstant(fact, i-startIndex));
+                    ret.append(Utility.createConstant(fact, i-indexIntoCallbackMethodForArguments +indexIntoObjectArrayForArguments));
                     ret.append(InstructionFactory.createArrayLoad(Type.OBJECT));
                     ret.append(Utility.createConversion(
                             fact,
                     ));
                 }
             }
+
         } else {
             Type proceedingJpType = Type.getType("Lorg/aspectj/lang/ProceedingJoinPoint;");
             int localJp = localAdviceMethod.allocateLocal(proceedingJpType);
 //        return ret;
     }
 
-    public void weaveAroundClosure(BcelAdvice munger, boolean hasDynamicTest) {
+    private boolean bindsThis(BcelAdvice munger) {
+    	UsesThisVisitor utv = new UsesThisVisitor();
+    	munger.getPointcut().accept(utv, null);
+		return utv.usesThis;
+	}
+
+    private boolean bindsTarget(BcelAdvice munger) {
+    	UsesTargetVisitor utv = new UsesTargetVisitor();
+    	munger.getPointcut().accept(utv, null);
+		return utv.usesTarget;
+	}
+    
+    private static class UsesThisVisitor extends IdentityPointcutVisitor {
+        boolean usesThis = false;
+        
+        public Object visit(ThisOrTargetPointcut node, Object data) {
+        	if (node.isThis() && node.isBinding()) usesThis=true;
+        	return node;
+        }
+
+        public Object visit(AndPointcut node, Object data) {
+            if (!usesThis) node.getLeft().accept(this, data);
+            if (!usesThis) node.getRight().accept(this, data);
+            return node;
+        }
+
+        public Object visit(NotPointcut node, Object data) {
+            if (!usesThis) node.getNegatedPointcut().accept(this, data);
+            return node;
+        }
+
+        public Object visit(OrPointcut node, Object data) {
+            if (!usesThis) node.getLeft().accept(this, data);
+            if (!usesThis) node.getRight().accept(this, data);
+            return node;
+        }
+    }
+    
+
+    private static class UsesTargetVisitor extends IdentityPointcutVisitor {
+        boolean usesTarget = false;
+        
+        public Object visit(ThisOrTargetPointcut node, Object data) {
+        	if (!node.isThis() && node.isBinding()) usesTarget=true;
+        	return node;
+        }
+
+        public Object visit(AndPointcut node, Object data) {
+            if (!usesTarget) node.getLeft().accept(this, data);
+            if (!usesTarget) node.getRight().accept(this, data);
+            return node;
+        }
+
+        public Object visit(NotPointcut node, Object data) {
+            if (!usesTarget) node.getNegatedPointcut().accept(this, data);
+            return node;
+        }
+
+        public Object visit(OrPointcut node, Object data) {
+            if (!usesTarget) node.getLeft().accept(this, data);
+            if (!usesTarget) node.getRight().accept(this, data);
+            return node;
+        }
+    }
+
+	public void weaveAroundClosure(BcelAdvice munger, boolean hasDynamicTest) {
     	InstructionFactory fact = getFactory();
 
 		enclosingMethod.setCanInline(false);
 			}
 		}
 
+		// initialize the bit flags for this shadow
+		int bitflags                                 =0x000000;
+		if (getKind().isTargetSameAsThis()) bitflags|=0x010000;
+		if (hasThis())                      bitflags|=0x001000;
+		if (bindsThis(munger))              bitflags|=0x000100;
+		if (hasTarget())                    bitflags|=0x000010;
+		if (bindsTarget(munger))            bitflags|=0x000001;
+		
         // ATAJ for @AJ aspect we need to link the closure with the joinpoint instance
         if (munger.getConcreteAspect()!=null && munger.getConcreteAspect().isAnnotationStyleAspect() 
            && munger.getDeclaringAspect()!=null && munger.getDeclaringAspect().resolve(world).isAnnotationStyleAspect()) {
+        	// stick the bitflags on the stack and call the variant of linkClosureAndJoinPoint that takes an int
+        	closureInstantiation.append(fact.createConstant(new Integer(bitflags)));
             closureInstantiation.append(Utility.createInvoke(
                     getFactory(),
                     getWorld(),
                             UnresolvedType.forName("org.aspectj.runtime.internal.AroundClosure"),
                             Modifier.PUBLIC,
                             "linkClosureAndJoinPoint",
-                            "()Lorg/aspectj/lang/ProceedingJoinPoint;"
+                            "(I)Lorg/aspectj/lang/ProceedingJoinPoint;"
                             )
             ));
         }