fix for Bugzilla Bug 51929

   Advice calling protected super method causing java.lang.VerifyError 'Bad access to protected data' 
Also expanded test to cover protected field access as well as methods

Fix required getting the correct receiver type for both field access and method
calls to correspond to Java's complicated rules for accessing protected
members (JLSv2 6.6.2 and mentioned in passing in JVMv2 5.4.4)

8 files changed, 63 insertions, 35 deletions

diff --git a/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/ast/AccessForInlineVisitor.java b/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/ast/AccessForInlineVisitor.java
index d4af2905c..c913040fe 100644
--- a/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/ast/AccessForInlineVisitor.java
+++ b/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/ast/AccessForInlineVisitor.java
@@ -35,12 +35,14 @@ import org.eclipse.jdt.internal.compiler.ast.QualifiedTypeReference;
 import org.eclipse.jdt.internal.compiler.ast.SingleNameReference;
 import org.eclipse.jdt.internal.compiler.ast.SingleTypeReference;
 import org.eclipse.jdt.internal.compiler.ast.ThisReference;
+import org.eclipse.jdt.internal.compiler.ast.TypeDeclaration;
 import org.eclipse.jdt.internal.compiler.lookup.ArrayBinding;
 import org.eclipse.jdt.internal.compiler.lookup.BlockScope;
 import org.eclipse.jdt.internal.compiler.lookup.FieldBinding;
 import org.eclipse.jdt.internal.compiler.lookup.MethodBinding;
 import org.eclipse.jdt.internal.compiler.lookup.ReferenceBinding;
 import org.eclipse.jdt.internal.compiler.lookup.TypeBinding;
+import org.eclipse.jdt.internal.compiler.lookup.VariableBinding;
 
 /**
  * Walks the body of around advice
@@ -75,29 +77,35 @@ public class AccessForInlineVisitor extends ASTVisitor {
 	
 	public void endVisit(SingleNameReference ref, BlockScope scope) {
 		if (ref.binding instanceof FieldBinding) {
-			ref.binding = getAccessibleField((FieldBinding)ref.binding);
+			ref.binding = getAccessibleField((FieldBinding)ref.binding, ref.actualReceiverType);
 		}
 	}
 
 	public void endVisit(QualifiedNameReference ref, BlockScope scope) {
-		//System.err.println("qref: " + ref + ", " + ref.binding.getClass().getName() + ", " + ref.codegenBinding.getClass().getName());
-		//System.err.println("   others: " + Arrays.asList(ref.otherBindings));
 		if (ref.binding instanceof FieldBinding) {
-			ref.binding = getAccessibleField((FieldBinding)ref.binding);
+			ref.binding = getAccessibleField((FieldBinding)ref.binding, ref.actualReceiverType);
 		}
-		if (ref.otherBindings != null) {
+		if (ref.otherBindings != null && ref.otherBindings.length > 0) {
+			TypeBinding receiverType;
+			if (ref.binding instanceof FieldBinding) {
+				receiverType = ((FieldBinding)ref.binding).type;
+			} else if (ref.binding instanceof VariableBinding) {
+				receiverType = ((VariableBinding)ref.binding).type;
+			} else {
+				//!!! understand and fix this case later
+				receiverType = ref.otherBindings[0].declaringClass;
+			}
+			
 			for (int i=0, len=ref.otherBindings.length; i < len; i++) {
-				if (ref.otherBindings[i] instanceof FieldBinding) {
-					ref.otherBindings[i] = getAccessibleField((FieldBinding)ref.otherBindings[i]);
-				}
+				FieldBinding binding = ref.otherBindings[i];
+				ref.otherBindings[i] = getAccessibleField(binding, receiverType);
+				receiverType = binding.type;
 			}
 		}
 	}
 
 	public void endVisit(FieldReference ref, BlockScope scope) {
-		if (ref.binding instanceof FieldBinding) {
-			ref.binding = getAccessibleField((FieldBinding)ref.binding);
-		}
+		ref.binding = getAccessibleField(ref.binding, ref.receiverType);
 	}
 	public void endVisit(MessageSend send, BlockScope scope) {
 		if (send instanceof Proceed) return;
@@ -105,10 +113,10 @@ public class AccessForInlineVisitor extends ASTVisitor {
 		
 		if (send.isSuperAccess() && !send.binding.isStatic()) {
 			send.receiver = new ThisReference(send.sourceStart, send.sourceEnd);
-			MethodBinding superAccessBinding = getSuperAccessMethod((MethodBinding)send.binding);
+			MethodBinding superAccessBinding = getSuperAccessMethod(send.binding);
 			AstUtil.replaceMethodBinding(send, superAccessBinding);
 		} else if (!isPublic(send.binding)) {
-			send.syntheticAccessor = getAccessibleMethod((MethodBinding)send.binding);
+			send.syntheticAccessor = getAccessibleMethod(send.binding, send.receiverType);
 		}
 	}
 	public void endVisit(AllocationExpression send, BlockScope scope) {
@@ -132,11 +140,11 @@ public class AccessForInlineVisitor extends ASTVisitor {
 		makePublic(ref.resolvedType); //getTypeBinding(scope));  //??? might be trouble
 	}
 	
-	private FieldBinding getAccessibleField(FieldBinding binding) {
+	private FieldBinding getAccessibleField(FieldBinding binding, TypeBinding receiverType) {
 		//System.err.println("checking field: " + binding);
 		if (!binding.isValidBinding()) return binding;
 		
-		makePublic(binding.declaringClass);
+		makePublic(receiverType);
 		if (isPublic(binding)) return binding;
 		if (binding instanceof PrivilegedFieldBinding) return binding;
 		if (binding instanceof InterTypeFieldBinding) return binding;
@@ -145,9 +153,9 @@ public class AccessForInlineVisitor extends ASTVisitor {
 			binding.modifiers = AstUtil.makePackageVisible(binding.modifiers);
 		}
 		
-		ResolvedMember m = EclipseFactory.makeResolvedMember(binding);
+		ResolvedMember m = EclipseFactory.makeResolvedMember(binding, receiverType);
 		if (inAspect.accessForInline.containsKey(m)) return (FieldBinding)inAspect.accessForInline.get(m);
-		FieldBinding ret = new InlineAccessFieldBinding(inAspect, binding);
+		FieldBinding ret = new InlineAccessFieldBinding(inAspect, binding, m);
 		
 		//System.err.println("   made accessor: " + ret);
 		
@@ -155,10 +163,10 @@ public class AccessForInlineVisitor extends ASTVisitor {
 		return ret;
 	}
 	
-	private MethodBinding getAccessibleMethod(MethodBinding binding) {
+	private MethodBinding getAccessibleMethod(MethodBinding binding, TypeBinding receiverType) {
 		if (!binding.isValidBinding()) return binding;
 		
-		makePublic(binding.declaringClass);  //???
+		makePublic(receiverType);  //???
 		if (isPublic(binding)) return binding;
 		if (binding instanceof InterTypeMethodBinding) return binding;
 
@@ -167,7 +175,7 @@ public class AccessForInlineVisitor extends ASTVisitor {
 		}
 
 		
-		ResolvedMember m = EclipseFactory.makeResolvedMember(binding);
+		ResolvedMember m = EclipseFactory.makeResolvedMember(binding, receiverType);
 		if (inAspect.accessForInline.containsKey(m)) return (MethodBinding)inAspect.accessForInline.get(m);
 		MethodBinding ret = world.makeMethodBinding(
 			AjcMemberMaker.inlineAccessMethodForMethod(inAspect.typeX, m)
@@ -228,4 +236,11 @@ public class AccessForInlineVisitor extends ASTVisitor {
 		isInlinable = false;
 	}
 
+	public boolean visit(
+		TypeDeclaration localTypeDeclaration,
+		BlockScope scope) {
+		// we don't want to transform any local anonymous classes as they won't be inlined
+		return false;
+	}
+
 }
diff --git a/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/EclipseFactory.java b/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/EclipseFactory.java
index 1a4e71fe0..e75cee9b3 100644
--- a/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/EclipseFactory.java
+++ b/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/EclipseFactory.java
@@ -182,10 +182,14 @@ public class EclipseFactory {
 	}
 	
 	public static ResolvedMember makeResolvedMember(MethodBinding binding) {
+		return makeResolvedMember(binding, binding.declaringClass);
+	}
+
+	public static ResolvedMember makeResolvedMember(MethodBinding binding, TypeBinding declaringType) {
 		//System.err.println("member for: " + binding + ", " + new String(binding.declaringClass.sourceName));
 		ResolvedMember ret =  new ResolvedMember(
 			binding.isConstructor() ? Member.CONSTRUCTOR : Member.METHOD,
-			fromBinding(binding.declaringClass),
+			fromBinding(declaringType),
 			binding.modifiers,
 			fromBinding(binding.returnType),
 			new String(binding.selector),
@@ -195,9 +199,13 @@ public class EclipseFactory {
 	}
 
 	public static ResolvedMember makeResolvedMember(FieldBinding binding) {
+		return makeResolvedMember(binding, binding.declaringClass);
+	}
+	
+	public static ResolvedMember makeResolvedMember(FieldBinding binding, TypeBinding receiverType) {
 		return new ResolvedMember(
 			Member.FIELD,
-			fromBinding(binding.declaringClass),
+			fromBinding(receiverType),
 			binding.modifiers,
 			fromBinding(binding.type),
 			new String(binding.name),
diff --git a/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/InlineAccessFieldBinding.java b/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/InlineAccessFieldBinding.java
index f0b844903..006cb19c1 100644
--- a/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/InlineAccessFieldBinding.java
+++ b/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/InlineAccessFieldBinding.java
@@ -15,6 +15,7 @@ package org.aspectj.ajdt.internal.compiler.lookup;
 
 import org.aspectj.ajdt.internal.compiler.ast.AspectDeclaration;
 import org.aspectj.weaver.AjcMemberMaker;
+import org.aspectj.weaver.ResolvedMember;
 import org.eclipse.jdt.internal.compiler.ast.ASTNode;
 import org.eclipse.jdt.internal.compiler.lookup.FieldBinding;
 import org.eclipse.jdt.internal.compiler.lookup.InvocationSite;
@@ -40,17 +41,17 @@ public class InlineAccessFieldBinding extends FieldBinding {
 	
 	public FieldBinding baseField;
 	
-	public InlineAccessFieldBinding(AspectDeclaration inAspect, FieldBinding baseField) {
+	public InlineAccessFieldBinding(AspectDeclaration inAspect, FieldBinding baseField, ResolvedMember resolvedField) {
 		super(baseField, baseField.declaringClass);
 
 		this.reader = new SimpleSyntheticAccessMethodBinding(
 			inAspect.factory.makeMethodBinding(
 				AjcMemberMaker.inlineAccessMethodForFieldGet(
-					inAspect.typeX, EclipseFactory.makeResolvedMember(baseField)
+					inAspect.typeX, resolvedField
 			)));
 		this.writer = new SimpleSyntheticAccessMethodBinding(inAspect.factory.makeMethodBinding(
 				AjcMemberMaker.inlineAccessMethodForFieldSet(
-					inAspect.typeX, EclipseFactory.makeResolvedMember(baseField)
+					inAspect.typeX, resolvedField
 			)));
 			
 		this.constant = ASTNode.NotAConstant;
diff --git a/tests/ajcTests.xml b/tests/ajcTests.xml
index 4f68d8e00..ded12e25e 100644
--- a/tests/ajcTests.xml
+++ b/tests/ajcTests.xml
@@ -7172,4 +7172,10 @@
         <compile files="TraceWithInnerV2.aj"/>
         <run class="Main"/>
     </ajc-test>
+    
+	<ajc-test dir="bugs/protectedvf"
+		title="mail list VerifyError with protected access">
+		<compile files="main/Driver.java,main/p2/AbstractTest.aj,main/p1/ConcreteTest.aj"/>
+		<run class="main.Driver"/>
+	</ajc-test>
 </suite>
diff --git a/tests/ajcTestsFailing.xml b/tests/ajcTestsFailing.xml
index 33a3a5d14..cd8ba3e47 100644
--- a/tests/ajcTestsFailing.xml
+++ b/tests/ajcTestsFailing.xml
@@ -133,12 +133,6 @@
         <run class="InterfaceInitializerOrder"/>
     </ajc-test>
 
-	<ajc-test dir="bugs/protectedvf"
-		title="mail list VerifyError with protected access">
-		<compile files="main/Driver.java,main/p2/AbstractTest.aj,main/p1/ConcreteTest.aj"/>
-		<run class="main.Driver"/>
-		</ajc-test>
-
 	<ajc-test dir="bugs/fieldsOnInterfaces"
 		pr="52107"
 		title="declare String field on interface">
diff --git a/tests/bugs/protectedvf/main/Driver.java b/tests/bugs/protectedvf/main/Driver.java
index 6590760eb..5ca85cd32 100644
--- a/tests/bugs/protectedvf/main/Driver.java
+++ b/tests/bugs/protectedvf/main/Driver.java
@@ -9,10 +9,10 @@ public class Driver{
 	}
 
 	private void doOtherStuff() {
-		System.out.println("doing other stuff");
+		//System.out.println("doing other stuff");
 	}
 
 	private void doStuff() {
-		System.out.println("doing stuff");
+		//System.out.println("doing stuff");
 	}
 }
diff --git a/tests/bugs/protectedvf/main/p1/ConcreteTest.aj b/tests/bugs/protectedvf/main/p1/ConcreteTest.aj
index be044e544..26f213b0c 100644
--- a/tests/bugs/protectedvf/main/p1/ConcreteTest.aj
+++ b/tests/bugs/protectedvf/main/p1/ConcreteTest.aj
@@ -10,14 +10,16 @@ final aspect ConcreteTest extends AbstractTest {
 	protected pointcut pc2(): execution(* Driver.doOtherStuff());

 

 	Object around(): pc2() {

-		System.out.println("adding to the other stuff");

+		//System.out.println("adding to the other stuff");

 		/*If we comment out the next line we don't get a verify error.*/

+		ConcreteTest ct = this;

+		System.out.println("test: " + s + ", " + this.s + ", " + ct.s);

 		System.out.println("The value of the field when replacing is " + getField());

 		return proceed();

 	}

 

 	protected void hook() {

-		/*This doesn't cause a verify error seemably because the advice calling it is in AbstractTest*/

+		/*This doesn't cause a verify error because this code is not inlined*/

 		System.out.println("The value of the field is " + getField());

 	}

 }
\ No newline at end of file
diff --git a/tests/bugs/protectedvf/main/p2/AbstractTest.aj b/tests/bugs/protectedvf/main/p2/AbstractTest.aj
index 0c8b040c4..748af744d 100644
--- a/tests/bugs/protectedvf/main/p2/AbstractTest.aj
+++ b/tests/bugs/protectedvf/main/p2/AbstractTest.aj
@@ -3,11 +3,13 @@ package main.p2;
 public abstract aspect AbstractTest {

 

 	private int field;

+	protected String s = "test";

 

 	protected abstract pointcut pc();

 

 	Object around(): pc() {

 		this.field++;

+		s += "-1";

 		hook();

 		return proceed();

 	}