Improve annotation style if pointcut handling

This fixes:
- negating annotation style if() pointcuts doesn't work
- annotation style if() pointcut not able to use a binding
  that is not exposed

Fixes #120,#122

6 files changed, 220 insertions, 21 deletions

diff --git a/org.aspectj.matcher/src/main/java/org/aspectj/weaver/patterns/IfPointcut.java b/org.aspectj.matcher/src/main/java/org/aspectj/weaver/patterns/IfPointcut.java
index 015e94f89..3b4295996 100644
--- a/org.aspectj.matcher/src/main/java/org/aspectj/weaver/patterns/IfPointcut.java
+++ b/org.aspectj.matcher/src/main/java/org/aspectj/weaver/patterns/IfPointcut.java
@@ -85,11 +85,13 @@ public class IfPointcut extends Pointcut {
 
 	@Override
 	protected FuzzyBoolean matchInternal(Shadow shadow) {
-		if ((extraParameterFlags & Advice.ConstantReference) != 0) {
-			if ((extraParameterFlags & Advice.ConstantValue) != 0) {
-				return FuzzyBoolean.YES;
-			} else {
-				return FuzzyBoolean.NO;
+		if (extraParameterFlags != -1) { // make sure it isn't annotation style if relying on these values
+			if ((extraParameterFlags & Advice.ConstantReference) != 0) {
+				if ((extraParameterFlags & Advice.ConstantValue) != 0) {
+					return FuzzyBoolean.YES;
+				} else {
+					return FuzzyBoolean.NO;   
+				}
 			}
 		}
 		// ??? this is not maximally efficient
@@ -271,9 +273,33 @@ public class IfPointcut extends Pointcut {
 					} else if (AjcMemberMaker.TYPEX_ENCLOSINGSTATICJOINPOINT.getSignature().equals(argSignature)) {
 						args.add(shadow.getThisEnclosingJoinPointStaticPartVar());
 					} else {
-						if (state.size() == 0 || currentStateIndex > state.size()) {
+
+						if (state.size() == 0 || currentStateIndex > state.size()) { // if 'we have nothing else to bind from in the state object' 
 							String[] paramNames = testMethod.getParameterNames();
 							StringBuilder errorParameter = new StringBuilder();
+							
+							// Support a single special situation: where the if() pointcut takes a parameter bound elsewhere
+							// in the pointcut but the advice does not bind it. For example:
+							//
+							// @Pointcut("this(o) && if()") public static boolean method(Foo f) { return f.isTrue();}
+							// @Before("method(*)") public void beforeAdvice() {}
+
+							// The condition above is effectively saying 'if we have nothing to bind'
+							if ( (i+1) == testMethod.getParameterTypes().length) { // If there is just one more to bind
+								// As with code style, recurse just to get the variable binding information
+								ExposedState myState = new ExposedState(baseArgsCount);
+								myState.setConcreteAspect(state.getConcreteAspect());
+								residueSource.findResidue(shadow, myState);
+								if (myState.size()==1) {
+									// Treat that as the parameter value the if pointcut needs
+									Var v = myState.get(0);
+									args.add(v);
+									ret = Test.makeAnd(ret,
+											Test.makeInstanceof(v, testMethod.getParameterTypes()[i].resolve(shadow.getIWorld())));
+									continue;
+								}
+							}
+							
 							if (paramNames != null) {
 								errorParameter.append(testMethod.getParameterTypes()[i].getName()).append(" ");
 								errorParameter.append(paramNames[i]);
diff --git a/tests/bugs198/github_120/C.java b/tests/bugs198/github_120/C.java
new file mode 100644
index 000000000..4af57af24
--- /dev/null
+++ b/tests/bugs198/github_120/C.java
@@ -0,0 +1,64 @@
+import org.aspectj.lang.annotation.Aspect;
+import org.aspectj.lang.annotation.Before;
+import org.aspectj.lang.annotation.Pointcut;
+
+/**
+ * This test is exploring situations where an if() pointcut is used with a parameter
+ * and yet a reference pointcut referring to it is not binding the parameter.
+ */
+public class C {
+	
+	int i;
+
+	C(int i) {
+		this.i = i;
+	}
+	
+	public static void main(String []argv) {
+		new C(1).run();
+	}
+
+	public void run() {
+		System.out.println("C.run() executing");
+	}
+	
+	public String toString() {
+		return "C("+i+")";
+	}
+
+}
+
+@Aspect
+abstract class Azpect1 {
+
+	@Pointcut("if(false)")
+	public void isCondition() {}
+	
+	@Before("isCondition() && execution(* C.run(..))")
+	public void beforeAdvice() {
+		System.out.println("Azpect1.beforeAdvice executing");
+	}
+	
+}
+
+@Aspect
+class Azpect2 extends Azpect1 {
+	@Pointcut("check(*)")
+	public void isCondition() { }
+	
+	@Pointcut("this(c) && if()")
+	public static boolean check(C c) {
+		System.out.println("check if() pointcut running on "+c.toString());
+		return true;
+	}
+}
+//
+//abstract aspect A {
+//	pointcut isCondition(): if(false);
+//	before(): isCondition() && execution(* C.run(..)) { System.out.println("A.before"); }
+//}
+//
+//aspect B extends A {
+//	pointcut isCondition(): check(*);
+//	pointcut check(Object o): this(o) && if(o.toString().equals("abc"));
+//}
\ No newline at end of file
diff --git a/tests/bugs198/github_120/D.java b/tests/bugs198/github_120/D.java
new file mode 100644
index 000000000..2ecdaa574
--- /dev/null
+++ b/tests/bugs198/github_120/D.java
@@ -0,0 +1,33 @@
+import org.aspectj.lang.annotation.Aspect;
+import org.aspectj.lang.annotation.Before;
+import org.aspectj.lang.annotation.Pointcut;
+
+/**
+ * This test is exploring situations where an if() pointcut is used with a parameter
+ * and yet a reference pointcut referring to it is not binding the parameter.
+ */
+public class D {
+	
+
+	public static void main(String []argv) {
+		new D().run();
+	}
+
+	public void run() {
+		System.out.println("D.run() executing");
+	}
+	
+	public boolean isTrue() {
+		return true;
+	}
+
+}
+
+@Aspect class Azpect {
+
+	@Pointcut("this(d) && if()") public static boolean method(D d) { return d.isTrue(); }
+
+	@Before("method(*) && execution(* D.run(..))") public void beforeAdvice() {
+		System.out.println("advice running");
+	}
+}
\ No newline at end of file
diff --git a/tests/bugs198/github_122/E.java b/tests/bugs198/github_122/E.java
new file mode 100644
index 000000000..29c818285
--- /dev/null
+++ b/tests/bugs198/github_122/E.java
@@ -0,0 +1,30 @@
+import org.aspectj.lang.annotation.Aspect;
+import org.aspectj.lang.annotation.Before;
+import org.aspectj.lang.annotation.Pointcut;
+
+/**
+ * Test !if() pointcuts
+ */
+public class E {
+
+	public static void main(String []argv) {
+		new E().run();
+	}
+
+	public void run() {
+		System.out.println("E.run() executing");
+	}
+
+}
+
+@Aspect class Azpect {
+
+	@Pointcut("!bar()") 
+	public static void foo() {}
+	
+	@Pointcut("if()") public static boolean bar() { return false; }
+
+	@Before("foo() && execution(* E.run(..))") public void beforeAdvice() {
+		System.out.println("advice running");
+	}
+}
\ No newline at end of file
diff --git a/tests/src/test/java/org/aspectj/systemtest/ajc198/Ajc198TestsJava.java b/tests/src/test/java/org/aspectj/systemtest/ajc198/Ajc198TestsJava.java
index 6ee1ddef7..45a3b1d62 100644
--- a/tests/src/test/java/org/aspectj/systemtest/ajc198/Ajc198TestsJava.java
+++ b/tests/src/test/java/org/aspectj/systemtest/ajc198/Ajc198TestsJava.java
@@ -19,23 +19,26 @@ public class Ajc198TestsJava extends XMLBasedAjcTestCaseForJava17OrLater {
 
 	public void testSealedClassWithLegalSubclasses() {
 		runTest("sealed class with legal subclasses");
-		// TODO: replace 0 by Constants.PREVIEW_MINOR_VERSION after no longer using EA build, but final JDK version
-		checkVersion("Employee", Constants.MAJOR_17, 0 /*Constants.PREVIEW_MINOR_VERSION*/);
-		checkVersion("Manager", Constants.MAJOR_17, 0 /*Constants.PREVIEW_MINOR_VERSION*/);
+		// TODO: replace 0 by Constants.PREVIEW_MINOR_VERSION after no longer using EA
+		// build, but final JDK version
+		checkVersion("Employee", Constants.MAJOR_17, 0 /* Constants.PREVIEW_MINOR_VERSION */);
+		checkVersion("Manager", Constants.MAJOR_17, 0 /* Constants.PREVIEW_MINOR_VERSION */);
 	}
 
 	public void testSealedClassWithIllegalSubclass() {
 		runTest("sealed class with illegal subclass");
-		// TODO: replace 0 by Constants.PREVIEW_MINOR_VERSION after no longer using EA build, but final JDK version
-		checkVersion("Person", Constants.MAJOR_17, 0 /*Constants.PREVIEW_MINOR_VERSION*/);
+		// TODO: replace 0 by Constants.PREVIEW_MINOR_VERSION after no longer using EA
+		// build, but final JDK version
+		checkVersion("Person", Constants.MAJOR_17, 0 /* Constants.PREVIEW_MINOR_VERSION */);
 	}
 
 	public void testWeaveSealedClass() {
 		runTest("weave sealed class");
-		// TODO: replace 0 by Constants.PREVIEW_MINOR_VERSION after no longer using EA build, but final JDK version
-		checkVersion("PersonAspect", Constants.MAJOR_17, 0 /*Constants.PREVIEW_MINOR_VERSION*/);
+		// TODO: replace 0 by Constants.PREVIEW_MINOR_VERSION after no longer using EA
+		// build, but final JDK version
+		checkVersion("PersonAspect", Constants.MAJOR_17, 0 /* Constants.PREVIEW_MINOR_VERSION */);
 	}
-	
+
 	public void testAnnotationStyleSpecialIfClauses() {
 		runTest("annotation style A");
 	}
@@ -44,13 +47,25 @@ public class Ajc198TestsJava extends XMLBasedAjcTestCaseForJava17OrLater {
 		runTest("annotation style B");
 	}
 
-  public static Test suite() {
-    return XMLBasedAjcTestCase.loadSuite(Ajc198TestsJava.class);
-  }
+	public void testAnnotationStyleSpecialIfClauses2_gh120() {
+		runTest("annotation style C");
+	}
 
-  @Override
-  protected java.net.URL getSpecFile() {
-    return getClassResource("ajc198.xml");
-  }
+	public void testAnnotationStyleSpecialIfClauses3_gh120() {
+		runTest("annotation style D");
+	}
+
+	public void testAnnotationStyleNegatedIf_gh122() {
+		runTest("annotation style negated if");
+	}
+
+	public static Test suite() {
+		return XMLBasedAjcTestCase.loadSuite(Ajc198TestsJava.class);
+	}
+
+	@Override
+	protected java.net.URL getSpecFile() {
+		return getClassResource("ajc198.xml");
+	}
 
 }
diff --git a/tests/src/test/resources/org/aspectj/systemtest/ajc198/ajc198.xml b/tests/src/test/resources/org/aspectj/systemtest/ajc198/ajc198.xml
index 64ba56bd9..9874736dd 100644
--- a/tests/src/test/resources/org/aspectj/systemtest/ajc198/ajc198.xml
+++ b/tests/src/test/resources/org/aspectj/systemtest/ajc198/ajc198.xml
@@ -122,4 +122,35 @@
 		</run>
 	</ajc-test>
 
+	<ajc-test dir="bugs198/github_120" title="annotation style C">
+		<compile files="C.java" options="-1.5"/>
+		<run class="C">
+			<stdout>
+				<line text="check if() pointcut running on C(1)"/>
+				<line text="Azpect1.beforeAdvice executing"/>
+				<line text="C.run() executing"/>
+			</stdout>
+		</run>
+	</ajc-test>
+
+	<ajc-test dir="bugs198/github_120" title="annotation style D">
+		<compile files="D.java" options="-1.5"/>
+		<run class="D">
+			<stdout>
+				<line text="advice running"/>
+				<line text="D.run() executing"/>
+			</stdout>
+		</run>
+	</ajc-test>
+
+	<ajc-test dir="bugs198/github_122" title="annotation style negated if">
+		<compile files="E.java" options="-1.5"/>
+		<run class="E">
+			<stdout>
+				<line text="advice running"/>
+				<line text="E.run() executing"/>
+			</stdout>
+		</run>
+	</ajc-test>
+
 </suite>