fix for pr107953, @AfterThrowing with no formal

5 files changed, 147 insertions, 35 deletions

diff --git a/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/ast/ValidateAtAspectJAnnotationsVisitor.java b/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/ast/ValidateAtAspectJAnnotationsVisitor.java
index 68502b43c..2d1785797 100644
--- a/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/ast/ValidateAtAspectJAnnotationsVisitor.java
+++ b/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/ast/ValidateAtAspectJAnnotationsVisitor.java
@@ -260,6 +260,8 @@ public class ValidateAtAspectJAnnotationsVisitor extends ASTVisitor {
 	 * 1) Advice must be public
 	 * 2) Advice must have a void return type if not around advice
 	 * 3) Advice must not have any other @AspectJ annotations
+	 * 4) After throwing advice must declare the thrown formal
+	 * 5) After returning advice must declare the returning formal
 	 */
 	private void validateAdvice(MethodDeclaration methodDeclaration) {
 		
@@ -290,6 +292,42 @@ public class ValidateAtAspectJAnnotationsVisitor extends ASTVisitor {
 		if (ajAnnotations.adviceKind != AdviceKind.Around) {
 			ensureVoidReturnType(methodDeclaration);
 		}	  
+		
+		if (ajAnnotations.adviceKind == AdviceKind.AfterThrowing) {
+			int[] throwingLocation = new int[2];
+			String thrownFormal = getStringLiteralFor("throwing",ajAnnotations.adviceAnnotation,throwingLocation);
+			if (thrownFormal != null) {
+				Argument[] arguments = methodDeclaration.arguments;
+				if (arguments != null && arguments.length > 0) {
+					Argument lastArgument = arguments[arguments.length - 1];
+					if (!thrownFormal.equals(new String(lastArgument.name))) {
+						methodDeclaration.scope.problemReporter()
+							.signalError(methodDeclaration.sourceStart,methodDeclaration.sourceEnd,"throwing formal '" + thrownFormal + "' must be declared as the last parameter in the advice signature");
+					}
+				} else {
+					methodDeclaration.scope.problemReporter()
+					.signalError(methodDeclaration.sourceStart,methodDeclaration.sourceEnd,"throwing formal '" + thrownFormal + "' must be declared as the last parameter in the advice signature");
+				}
+			}
+		}
+		
+		if (ajAnnotations.adviceKind == AdviceKind.AfterReturning) {
+			int[] throwingLocation = new int[2];
+			String returningFormal = getStringLiteralFor("returning",ajAnnotations.adviceAnnotation,throwingLocation);
+			if (returningFormal != null) {
+				Argument[] arguments = methodDeclaration.arguments;
+				if (arguments != null && arguments.length > 0) {
+					Argument lastArgument = arguments[arguments.length - 1];
+					if (!returningFormal.equals(new String(lastArgument.name))) {
+						methodDeclaration.scope.problemReporter()
+							.signalError(methodDeclaration.sourceStart,methodDeclaration.sourceEnd,"returning formal '" + returningFormal + "' must be declared as the last parameter in the advice signature");
+					}
+				} else {
+					methodDeclaration.scope.problemReporter()
+					.signalError(methodDeclaration.sourceStart,methodDeclaration.sourceEnd,"returning formal '" + returningFormal + "' must be declared as the last parameter in the advice signature");
+				}
+			}
+		}
 
 		resolveAndSetPointcut(methodDeclaration, ajAnnotations.adviceAnnotation);
 		
diff --git a/weaver/src/org/aspectj/weaver/WeaverMessages.java b/weaver/src/org/aspectj/weaver/WeaverMessages.java
index b49155f49..40ef2e9a3 100644
--- a/weaver/src/org/aspectj/weaver/WeaverMessages.java
+++ b/weaver/src/org/aspectj/weaver/WeaverMessages.java
@@ -148,6 +148,10 @@ public class WeaverMessages {
 	
 	public static final String HAS_MEMBER_NOT_ENABLED="hasMemberNotEnabled";
 	
+	// @AspectJ
+	public static final String RETURNING_FORMAL_NOT_DECLARED_IN_ADVICE = "returningFormalNotDeclaredInAdvice";
+	public static final String THROWN_FORMAL_NOT_DECLARED_IN_ADVICE = "thrownFormalNotDeclaredInAdvice";
+	
 	public static String format(String key) {
 		return bundle.getString(key);
 	}
diff --git a/weaver/src/org/aspectj/weaver/bcel/AtAjAttributes.java b/weaver/src/org/aspectj/weaver/bcel/AtAjAttributes.java
index ef8c26497..2896d9dc6 100644
--- a/weaver/src/org/aspectj/weaver/bcel/AtAjAttributes.java
+++ b/weaver/src/org/aspectj/weaver/bcel/AtAjAttributes.java
@@ -46,6 +46,7 @@ import org.aspectj.weaver.ReferenceType;
 import org.aspectj.weaver.ResolvedPointcutDefinition;
 import org.aspectj.weaver.ResolvedType;
 import org.aspectj.weaver.UnresolvedType;
+import org.aspectj.weaver.WeaverMessages;
 import org.aspectj.weaver.patterns.AndPointcut;
 import org.aspectj.weaver.patterns.DeclareErrorOrWarning;
 import org.aspectj.weaver.patterns.DeclarePrecedence;
@@ -341,26 +342,43 @@ public class AtAjAttributes {
         boolean hasAtAspectJAnnotationMustReturnVoid = false;
         for (int i = 0; i < attributes.length; i++) {
             Attribute attribute = attributes[i];
-            if (acceptAttribute(attribute)) {
-                RuntimeAnnotations rvs = (RuntimeAnnotations) attribute;
-                hasAtAspectJAnnotationMustReturnVoid = hasAtAspectJAnnotationMustReturnVoid || handleBeforeAnnotation(
-                        rvs, struct, preResolvedPointcut
-                );
-                hasAtAspectJAnnotationMustReturnVoid = hasAtAspectJAnnotationMustReturnVoid || handleAfterAnnotation(
-                        rvs, struct, preResolvedPointcut
-                );
-                hasAtAspectJAnnotationMustReturnVoid = hasAtAspectJAnnotationMustReturnVoid || handleAfterReturningAnnotation(
-                        rvs, struct, preResolvedPointcut
-                );
-                hasAtAspectJAnnotationMustReturnVoid = hasAtAspectJAnnotationMustReturnVoid || handleAfterThrowingAnnotation(
-                        rvs, struct, preResolvedPointcut
-                );
-                hasAtAspectJAnnotation = hasAtAspectJAnnotation || handleAroundAnnotation(
-                        rvs, struct, preResolvedPointcut
-                );
-                // there can only be one RuntimeVisible bytecode attribute
-                break;
-            }
+            try {
+				if (acceptAttribute(attribute)) {
+				    RuntimeAnnotations rvs = (RuntimeAnnotations) attribute;
+				    hasAtAspectJAnnotationMustReturnVoid = hasAtAspectJAnnotationMustReturnVoid || handleBeforeAnnotation(
+				            rvs, struct, preResolvedPointcut
+				    );
+				    hasAtAspectJAnnotationMustReturnVoid = hasAtAspectJAnnotationMustReturnVoid || handleAfterAnnotation(
+				            rvs, struct, preResolvedPointcut
+				    );
+				    hasAtAspectJAnnotationMustReturnVoid = hasAtAspectJAnnotationMustReturnVoid || handleAfterReturningAnnotation(
+				            rvs, struct, preResolvedPointcut, bMethod
+				    );
+				    hasAtAspectJAnnotationMustReturnVoid = hasAtAspectJAnnotationMustReturnVoid || handleAfterThrowingAnnotation(
+				            rvs, struct, preResolvedPointcut, bMethod
+				    );
+				    hasAtAspectJAnnotation = hasAtAspectJAnnotation || handleAroundAnnotation(
+				            rvs, struct, preResolvedPointcut
+				    );
+				    // there can only be one RuntimeVisible bytecode attribute
+				    break;
+				}
+			} catch (ReturningFormalNotDeclaredInAdviceSignatureException e) {
+				msgHandler.handleMessage(
+					new Message(
+							WeaverMessages.format(WeaverMessages.RETURNING_FORMAL_NOT_DECLARED_IN_ADVICE,e.getFormalName()),
+							IMessage.ERROR,
+							null,
+							bMethod.getSourceLocation())
+				);
+			} catch (ThrownFormalNotDeclaredInAdviceSignatureException e) {
+				msgHandler.handleMessage(
+						new Message(
+								WeaverMessages.format(WeaverMessages.THROWN_FORMAL_NOT_DECLARED_IN_ADVICE,e.getFormalName()),
+								IMessage.ERROR,
+								null,
+								bMethod.getSourceLocation())
+					);			}
         }
         hasAtAspectJAnnotation = hasAtAspectJAnnotation || hasAtAspectJAnnotationMustReturnVoid;
 
@@ -402,8 +420,7 @@ public class AtAjAttributes {
             );
             ;// go ahead
         }
-
-
+        
         return struct.ajAttributes;
     }
 
@@ -664,7 +681,13 @@ public class AtAjAttributes {
      * @param struct
      * @return true if found
      */
-    private static boolean handleAfterReturningAnnotation(RuntimeAnnotations runtimeAnnotations, AjAttributeMethodStruct struct, ResolvedPointcutDefinition preResolvedPointcut) {
+    private static boolean handleAfterReturningAnnotation(
+    		RuntimeAnnotations runtimeAnnotations, 
+    		AjAttributeMethodStruct struct, 
+    		ResolvedPointcutDefinition preResolvedPointcut,
+    		BcelMethod owningMethod) 
+    throws ReturningFormalNotDeclaredInAdviceSignatureException
+    {
         Annotation after = getAnnotation(runtimeAnnotations, AjcMemberMaker.AFTERRETURNING_ANNOTATION);
         if (after != null) {
             ElementNameValuePair annValue = getAnnotationElement(after, VALUE);
@@ -689,8 +712,16 @@ public class AtAjAttributes {
             }
             if (annReturned != null) {
                 returned = annReturned.getValue().stringifyValue();
-                if (isNullOrEmpty(returned))
+                if (isNullOrEmpty(returned)) {
                     returned = null;
+                } else {
+                   	// check that thrownFormal exists as the last parameter in the advice
+                	String[] pNames = owningMethod.getParameterNames();
+                	if (pNames == null || pNames.length == 0 || !pNames[pNames.length -1].equals(returned)) {
+                		throw new ReturningFormalNotDeclaredInAdviceSignatureException(returned);
+                	}
+                	
+                }
             }
 
             // this/target/args binding
@@ -748,16 +779,22 @@ public class AtAjAttributes {
      * @param struct
      * @return true if found
      */
-    private static boolean handleAfterThrowingAnnotation(RuntimeAnnotations runtimeAnnotations, AjAttributeMethodStruct struct, ResolvedPointcutDefinition preResolvedPointcut) {
+    private static boolean handleAfterThrowingAnnotation(
+    		RuntimeAnnotations runtimeAnnotations, 
+    		AjAttributeMethodStruct struct, 
+    		ResolvedPointcutDefinition preResolvedPointcut, 
+    		BcelMethod owningMethod)
+    throws ThrownFormalNotDeclaredInAdviceSignatureException
+    {
         Annotation after = getAnnotation(runtimeAnnotations, AjcMemberMaker.AFTERTHROWING_ANNOTATION);
         if (after != null) {
             ElementNameValuePair annValue = getAnnotationElement(after, VALUE);
             ElementNameValuePair annPointcut = getAnnotationElement(after, POINTCUT);
-            ElementNameValuePair annThrowned = getAnnotationElement(after, THROWING);
+            ElementNameValuePair annThrown = getAnnotationElement(after, THROWING);
 
             // extract the pointcut and throwned type/binding - do some checks
             String pointcut = null;
-            String throwned = null;
+            String thrownFormal = null;
             if ((annValue != null && annPointcut != null) || (annValue == null && annPointcut == null)) {
                 reportError("@AfterThrowing: either 'value' or 'poincut' must be provided, not both", struct);
                 return false;
@@ -771,17 +808,24 @@ public class AtAjAttributes {
                 reportError("@AfterThrowing: either 'value' or 'poincut' must be provided, not both", struct);
                 return false;
             }
-            if (annThrowned != null) {
-                throwned = annThrowned.getValue().stringifyValue();
-                if (isNullOrEmpty(throwned))
-                    throwned = null;
+            if (annThrown != null) {
+                thrownFormal = annThrown.getValue().stringifyValue();
+                if (isNullOrEmpty(thrownFormal)) {
+                    thrownFormal = null;
+                } else {
+                	// check that thrownFormal exists as the last parameter in the advice
+                	String[] pNames = owningMethod.getParameterNames();
+                	if (pNames == null || pNames.length == 0 || !pNames[pNames.length -1].equals(thrownFormal)) {
+                		throw new ThrownFormalNotDeclaredInAdviceSignatureException(thrownFormal);
+                	}
+                }
             }
 
             // this/target/args binding
             // exclude the throwned binding from the pointcut binding since it is an extraArg binding
             FormalBinding[] bindings = new org.aspectj.weaver.patterns.FormalBinding[0];
             try {
-                bindings = (throwned == null ? extractBindings(struct) : extractBindings(struct, throwned));
+                bindings = (thrownFormal == null ? extractBindings(struct) : extractBindings(struct, thrownFormal));
             } catch (UnreadableDebugInfoException unreadableDebugInfoException) {
                 return false;
             }
@@ -795,7 +839,7 @@ public class AtAjAttributes {
             int extraArgument = extractExtraArgument(struct.method);
 
             // return binding
-            if (throwned != null) {
+            if (thrownFormal != null) {
                 extraArgument |= Advice.ExtraArgument;
             }
 
@@ -1457,4 +1501,26 @@ public class AtAjAttributes {
             return node;
         }
     }
+    
+    static class ThrownFormalNotDeclaredInAdviceSignatureException extends Exception {
+    	
+    	private String formalName;
+    	
+    	public ThrownFormalNotDeclaredInAdviceSignatureException(String formalName) {
+    		this.formalName = formalName;
+    	}
+    	
+    	public String getFormalName() { return formalName; }
+    }
+    
+    static class ReturningFormalNotDeclaredInAdviceSignatureException extends Exception {
+    	
+    	private String formalName;
+    	
+    	public ReturningFormalNotDeclaredInAdviceSignatureException(String formalName) {
+    		this.formalName = formalName;
+    	}
+    	
+    	public String getFormalName() { return formalName; }
+    }
 }
diff --git a/weaver/src/org/aspectj/weaver/bcel/BcelMethod.java b/weaver/src/org/aspectj/weaver/bcel/BcelMethod.java
index 89f6d67ea..7bdf3af71 100644
--- a/weaver/src/org/aspectj/weaver/bcel/BcelMethod.java
+++ b/weaver/src/org/aspectj/weaver/bcel/BcelMethod.java
@@ -68,8 +68,8 @@ final class BcelMethod extends ResolvedMemberImpl {
 		this.sourceContext = declaringType.getResolvedTypeX().getSourceContext();
 		this.world = declaringType.getResolvedTypeX().getWorld();
 		this.bcelObjectType = declaringType;
-		unpackAjAttributes(world);
 		unpackJavaAttributes();
+		unpackAjAttributes(world);
 	}
 
 	// ----
diff --git a/weaver/src/org/aspectj/weaver/weaver-messages.properties b/weaver/src/org/aspectj/weaver/weaver-messages.properties
index 56e94c5ae..d896687d0 100644
--- a/weaver/src/org/aspectj/weaver/weaver-messages.properties
+++ b/weaver/src/org/aspectj/weaver/weaver-messages.properties
@@ -157,4 +157,8 @@ noParameterizedDeclaringTypesInExecution=can't use parameterized type patterns f
 noParameterizedDeclaringTypesInCall=can't use parameterized type patterns for the declaring type of a call pointcut expression (use the raw type instead)
 noRawTypePointcutReferences=cannot use a raw type reference to refer to a pointcut in a generic type (use a parameterized reference instead)
 
-hasMemberNotEnabled=the type pattern {0} can only be used when the -XhasMember option is set
\ No newline at end of file
+hasMemberNotEnabled=the type pattern {0} can only be used when the -XhasMember option is set
+
+# @AspectJ
+returningFormalNotDeclaredInAdvice=the last parameter of this advice must be named ''{0}'' to bind the returning value
+thrownFormalNotDeclaredInAdvice=the last parameter of this advice must be named ''{0}'' and be of a subtype of Throwable
\ No newline at end of file