From fab59b5d20ee3ad5d49920c4e9fe785f58820614 Mon Sep 17 00:00:00 2001 From: Andy Clement Date: Mon, 31 Jan 2022 15:47:27 -0800 Subject: [PATCH] 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 --- .../aspectj/weaver/patterns/IfPointcut.java | 38 +++++++++-- tests/bugs198/github_120/C.java | 64 +++++++++++++++++++ tests/bugs198/github_120/D.java | 33 ++++++++++ tests/bugs198/github_122/E.java | 30 +++++++++ .../systemtest/ajc198/Ajc198TestsJava.java | 45 ++++++++----- .../org/aspectj/systemtest/ajc198/ajc198.xml | 31 +++++++++ 6 files changed, 220 insertions(+), 21 deletions(-) create mode 100644 tests/bugs198/github_120/C.java create mode 100644 tests/bugs198/github_120/D.java create mode 100644 tests/bugs198/github_122/E.java 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 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + -- 2.39.5