From 0df90e175b316d5a8e6189c2d89725cd8afd7bd3 Mon Sep 17 00:00:00 2001 From: Andy Clement Date: Tue, 21 Oct 2014 13:19:19 -0700 Subject: [PATCH] Support for @RequiredTypes and abstract aspects --- docs/dist/doc/README-183.html | 114 ++++++++++++++++++ docs/dist/doc/index.html | 1 + .../compiler/lookup/EclipseSourceType.java | 5 + .../weaver/CrosscuttingMembersSet.java | 50 ++++++-- .../reflect/RuntimePerformanceTest.java | 2 +- tests/bugs183/436653/abstract/A.java | 1 + tests/bugs183/436653/abstract/AA.java | 5 + tests/bugs183/436653/abstract/Code.java | 8 ++ tests/bugs183/436653/abstract/X.java | 8 ++ .../systemtest/ajc183/Ajc183Tests.java | 8 ++ .../org/aspectj/systemtest/ajc183/ajc183.xml | 34 ++++++ .../src/org/aspectj/weaver/ltw/LTWWorld.java | 8 +- 12 files changed, 228 insertions(+), 16 deletions(-) create mode 100644 docs/dist/doc/README-183.html create mode 100644 tests/bugs183/436653/abstract/A.java create mode 100644 tests/bugs183/436653/abstract/AA.java create mode 100644 tests/bugs183/436653/abstract/Code.java create mode 100644 tests/bugs183/436653/abstract/X.java diff --git a/docs/dist/doc/README-183.html b/docs/dist/doc/README-183.html new file mode 100644 index 000000000..5b97481d8 --- /dev/null +++ b/docs/dist/doc/README-183.html @@ -0,0 +1,114 @@ + + +AspectJ 1.8.3 Readme + + + + +
+© Copyright 2014 Contributors. +All rights reserved. +
+ +

AspectJ 1.8.3 Readme

+ +

The full list of resolved issues in 1.8.3 is available +here.

+ + + +

Notable changes

+ +

Conditional aspect activation with @RequiredTypes - Issue 436653

+ +

AspectJ is sometimes used to create aspect libraries. These libraries contain a number of aspects often covering +a variety of domains. The library might typically be available as a jar and contains a single aop.xml file that +names all the aspects. The library is then consumed by some application. +However, the application may not need to use all those aspects +but because they are listed in the aop.xml they will be 'active'. Now the pointcuts in those unused aspects +may not match anything in the application and could be considered harmless but the pointcuts and the aspects +themselves may have references to types in other libraries that the application does not have around. This can lead +to unhelpful "can't find type" messages and currently requires the user to add unnecessary entries to their +build dependencies just to keep the unused aspects happy. +

+

With AspectJ 1.8.3 it is now possible to express a constraint on an aspect. The @RequiredTypes +annotation specifies one or more fully qualified types that must be discoverable on the classpath in +order for the aspect to activate. Using this there is no need to add those extraneous dependencies to +an applications build classpath. +

+

Example:

+
import org.aspectj.lang.annotation.*;
+
+@RequiredTypes("com.foo.Bar")
+public aspect Foo {
+  before(): execution(@com.foo.Bar * *(..)) {}
+}
+
+ +

+If the above aspect is listed in an aop.xml for loadtime weaving or passed on the aspectpath for +compile time weaving, if the type 'com.foo.Bar' is not accessible on the classpath then the +aspect will be turned off and the pointcut will have no effect. There will be no attempt made to +match it and so no unhelpful "can't find type" messages. +

+ +

cflow and the pre-initialization joinpoint changes due to Java 7 verifier modifications - Issue 443477

+ +

There has been a change in the Java7 verifier in a recent patch release of Java7 (update 67) that causes +a verify error for usage of a particular AspectJ construct. The problem occurs if you are using +cflow and it hits the preinitialization join point. The pattern of code generated in that case causes +the verifyerror. In this release of AspectJ we have taken the 'quick' approach to solving this, namely +to avoid advising preinitialization with the cflow construct. This problem appears to come up +when the aspect is non-optimal anyway and hitting preinitialization was never really intended by the +pointcut writer. For example: + +

execution(* foo(..)) && cflow(within(Bar))
+ +

The use of cflow and within there will actually hit *a lot* of joinpoints, many of which the user probably didn't mean to. +It feels like we actually need a warning to indicate the pointcut is probably suboptimal. What the user probably +meant was something more like this:

+ +
execution(* foo(..)) && cflow(execution(* Bar.*(..))
+

or

+
execution(* foo(..)) && cflow(within(Bar) && execution(* *(..)))
+ +

+But even if they did want the less optimal form of cflow there still seems little use in applying it to +pre-initialization - that is your cue to raise an AspectJ bug with a realistic use case inside that proves this +an invalid assumption :)

+ +

around advice and lambdas - Issue 445395

+ +

For optimal performance, where possible, AspectJ tries to inline around advice when it applies +at a joinpoint. There are few characteristics of a joinpoint match that can prevent this but we +do try to inline where we can (the inlining can be manually turned off via -XnoInline).

+ +

Inlining of around advice basically means copying the advice instructions into the target class. This causes +a problem when the advice uses lambdas. Lambda usage is currently implemented in java compilers by generating +invokedynamic bytecode instructions that reference bootstrap methods created in the class and a helper method +generated in the class containing the lambda code. When the invokedynamic is encountered at runtime, some magic +happens and the bootstrap method is used to generate a class on the fly that calls the particular lambda method. +All this 'extra stuff' breaks the basic inlining algorithm that simply copies the advice bytecode into the target. +Effectively the inlining process needs to become much more sophisticated and copy the bootstrap methods and +the lambda helper methods, avoiding clashes with existing bootstrap/helpers in the target.

+ +

+Prior to AspectJ 1.8.3 when the inlining failed you would get a horrible class cast exception that mentions +constant pool entries (because the bootstrap method hadn't been copied over to the target). Temporarily in +1.8.3 we are turning off inlining of around advice containing lambdas, which will at least avoid the failure, +with the longer term goal of improving the inlining process to do all the necessary extra work. +

+ + + + diff --git a/docs/dist/doc/index.html b/docs/dist/doc/index.html index 64d8b8ec7..3b3558b9c 100644 --- a/docs/dist/doc/index.html +++ b/docs/dist/doc/index.html @@ -138,6 +138,7 @@ README's Changes and porting guide for AspectJ + 1.8.2, 1.8.2, 1.8.1, 1.8.0, diff --git a/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/EclipseSourceType.java b/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/EclipseSourceType.java index 9b04c6d1f..dc82d4aac 100644 --- a/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/EclipseSourceType.java +++ b/org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/EclipseSourceType.java @@ -818,6 +818,11 @@ public class EclipseSourceType extends AbstractReferenceTypeDelegate { AnnotationValue array = new ArrayAnnotationValue(arrayValues); AnnotationNameValuePair anvp = new AnnotationNameValuePair(new String(mvp.name), array); annotationAJ.addNameValuePair(anvp); + } else if (mvp.value instanceof Literal) { + AnnotationValue av = generateElementValue(mvp.value, + ((Literal) mvp.value).resolvedType); + AnnotationNameValuePair anvp = new AnnotationNameValuePair(new String(mvp.name), av); + annotationAJ.addNameValuePair(anvp); } else { MethodBinding methodBinding = mvp.binding; if (methodBinding == null) { diff --git a/org.aspectj.matcher/src/org/aspectj/weaver/CrosscuttingMembersSet.java b/org.aspectj.matcher/src/org/aspectj/weaver/CrosscuttingMembersSet.java index 3937830cd..969f96ece 100644 --- a/org.aspectj.matcher/src/org/aspectj/weaver/CrosscuttingMembersSet.java +++ b/org.aspectj.matcher/src/org/aspectj/weaver/CrosscuttingMembersSet.java @@ -23,14 +23,14 @@ import java.util.List; import java.util.Map; import java.util.Set; +import org.aspectj.bridge.IMessage; +import org.aspectj.bridge.MessageUtil; import org.aspectj.weaver.patterns.Declare; import org.aspectj.weaver.patterns.DeclareAnnotation; import org.aspectj.weaver.patterns.DeclareParents; import org.aspectj.weaver.patterns.DeclareSoft; import org.aspectj.weaver.patterns.DeclareTypeErrorOrWarning; import org.aspectj.weaver.patterns.IVerificationRequired; -import org.aspectj.weaver.tools.Trace; -import org.aspectj.weaver.tools.TraceFactory; /** * This holds on to all CrosscuttingMembers for a world. It handles management of change. @@ -40,15 +40,13 @@ import org.aspectj.weaver.tools.TraceFactory; */ public class CrosscuttingMembersSet { - private static Trace trace = TraceFactory.getTraceFactory().getTrace(CrosscuttingMembersSet.class); - private transient World world; // FIXME AV - ? we may need a sequencedHashMap there to ensure source based precedence for @AJ advice - private final Map /* ResolvedType (the aspect) > CrosscuttingMembers */ members = new HashMap(); + private final Map members = new HashMap(); // List of things to be verified once the type system is 'complete' - private transient List /* IVerificationRequired */ verificationList = null; + private transient List verificationList = null; private List shadowMungers = null; private List typeMungers = null; @@ -69,17 +67,46 @@ public class CrosscuttingMembersSet { public boolean addOrReplaceAspect(ResolvedType aspectType) { return addOrReplaceAspect(aspectType, true); } + + /** + * Check if any parent aspects of the supplied aspect have unresolved dependencies (and so + * should cause this aspect to be turned off). + * @param aspectType the aspect whose parents should be checked + * @return true if this aspect should be excluded because of a parents' missing dependencies + */ + private boolean excludeDueToParentAspectHavingUnresolvedDependency(ResolvedType aspectType) { + ResolvedType parent = aspectType.getSuperclass(); + boolean excludeDueToParent = false; + while (parent != null) { + if (parent.isAspect() && parent.isAbstract() && world.hasUnsatisfiedDependency(parent)) { + if (!world.getMessageHandler().isIgnoring(IMessage.INFO)) { + world.getMessageHandler().handleMessage( + MessageUtil.info("deactivating aspect '" + aspectType.getName() + "' as the parent aspect '"+parent.getName()+ + "' has unsatisfied dependencies")); + } + excludeDueToParent = true; + } + parent = parent.getSuperclass(); + } + return excludeDueToParent; + } /** * @return whether or not that was a change to the global signature XXX for efficiency we will need a richer representation than * this */ public boolean addOrReplaceAspect(ResolvedType aspectType, boolean inWeavingPhase) { - - if (!world.isAspectIncluded(aspectType) || world.hasUnsatisfiedDependency(aspectType)) { + if (!world.isAspectIncluded(aspectType)) { return false; } - + if (world.hasUnsatisfiedDependency(aspectType)) { + return false; + } + // Abstract super aspects might have unsatisfied dependencies + if (excludeDueToParentAspectHavingUnresolvedDependency(aspectType)) { + return false; + } + boolean change = false; CrosscuttingMembers xcut = members.get(aspectType); if (xcut == null) { @@ -108,14 +135,15 @@ public class CrosscuttingMembersSet { return change; } - + private boolean addOrReplaceDescendantsOf(ResolvedType aspectType, boolean inWeavePhase) { // System.err.println("Looking at descendants of "+aspectType.getName()); Set knownAspects = members.keySet(); Set toBeReplaced = new HashSet(); for (Iterator it = knownAspects.iterator(); it.hasNext();) { ResolvedType candidateDescendant = it.next(); - if ((candidateDescendant != aspectType) && (aspectType.isAssignableFrom(candidateDescendant))) { + // allowMissing = true - if something is missing, it really probably is not a descendant + if ((candidateDescendant != aspectType) && (aspectType.isAssignableFrom(candidateDescendant, true))) { toBeReplaced.add(candidateDescendant); } } diff --git a/runtime/testsrc/org/aspectj/runtime/reflect/RuntimePerformanceTest.java b/runtime/testsrc/org/aspectj/runtime/reflect/RuntimePerformanceTest.java index ac487539a..25959aebc 100644 --- a/runtime/testsrc/org/aspectj/runtime/reflect/RuntimePerformanceTest.java +++ b/runtime/testsrc/org/aspectj/runtime/reflect/RuntimePerformanceTest.java @@ -78,7 +78,7 @@ public class RuntimePerformanceTest extends TestCase { long ratio = (EXPECTED_RATIO*noCache/cache); System.out.println("ratio=" + ratio); - assertTrue("Using cache should be " + EXPECTED_RATIO + " times faster: " + ratio,(ratio > EXPECTED_RATIO)); + assertTrue("Using cache should be " + EXPECTED_RATIO + " times faster: " + ratio,(ratio >= EXPECTED_RATIO)); } private long invokeSignatureToString (Signature sig, long iterations) { diff --git a/tests/bugs183/436653/abstract/A.java b/tests/bugs183/436653/abstract/A.java new file mode 100644 index 000000000..9f4b93d84 --- /dev/null +++ b/tests/bugs183/436653/abstract/A.java @@ -0,0 +1 @@ +public class A {} diff --git a/tests/bugs183/436653/abstract/AA.java b/tests/bugs183/436653/abstract/AA.java new file mode 100644 index 000000000..eceb74d76 --- /dev/null +++ b/tests/bugs183/436653/abstract/AA.java @@ -0,0 +1,5 @@ +import org.aspectj.lang.annotation.*; + +@RequiredTypes("A") +abstract aspect AA { +} diff --git a/tests/bugs183/436653/abstract/Code.java b/tests/bugs183/436653/abstract/Code.java new file mode 100644 index 000000000..f956f53cf --- /dev/null +++ b/tests/bugs183/436653/abstract/Code.java @@ -0,0 +1,8 @@ +public class Code { + public static void main(String []argv) { + new Code().m(); + } + public void m() { + System.out.println("Code.m()"); + } +} diff --git a/tests/bugs183/436653/abstract/X.java b/tests/bugs183/436653/abstract/X.java new file mode 100644 index 000000000..e0450f424 --- /dev/null +++ b/tests/bugs183/436653/abstract/X.java @@ -0,0 +1,8 @@ +import org.aspectj.lang.annotation.*; + +//@RequiredTypes("A") +aspect X extends AA { + @SuppressAjWarnings("adviceDidNotMatch") + before(): execution(* *(..)) { System.out.println("X.before"); } +} + diff --git a/tests/src/org/aspectj/systemtest/ajc183/Ajc183Tests.java b/tests/src/org/aspectj/systemtest/ajc183/Ajc183Tests.java index bb033b6db..db6878e15 100644 --- a/tests/src/org/aspectj/systemtest/ajc183/Ajc183Tests.java +++ b/tests/src/org/aspectj/systemtest/ajc183/Ajc183Tests.java @@ -25,6 +25,14 @@ public class Ajc183Tests extends org.aspectj.testing.XMLBasedAjcTestCase { runTest("super calls"); } + public void testSuppressTypeNotFoundAbstract_436653_2() { + runTest("suppress type not found - abstract 2"); + } + + public void testSuppressTypeNotFoundAbstract_436653_1() { + runTest("suppress type not found - abstract 1"); + } + public void testSuppressTypeNotFound_436653() { runTest("suppress type not found"); } diff --git a/tests/src/org/aspectj/systemtest/ajc183/ajc183.xml b/tests/src/org/aspectj/systemtest/ajc183/ajc183.xml index c24987f6b..e1c40ae72 100644 --- a/tests/src/org/aspectj/systemtest/ajc183/ajc183.xml +++ b/tests/src/org/aspectj/systemtest/ajc183/ajc183.xml @@ -10,6 +10,40 @@ + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/weaver/src/org/aspectj/weaver/ltw/LTWWorld.java b/weaver/src/org/aspectj/weaver/ltw/LTWWorld.java index eff7a5999..49185a2fb 100644 --- a/weaver/src/org/aspectj/weaver/ltw/LTWWorld.java +++ b/weaver/src/org/aspectj/weaver/ltw/LTWWorld.java @@ -176,8 +176,8 @@ public class LTWWorld extends BcelWorld implements IReflectionWorld { if (concurrentMapClass != null) { try { return (Map) concurrentMapClass.newInstance(); - } catch (InstantiationException _) { - } catch (IllegalAccessException _) { + } catch (InstantiationException ie) { + } catch (IllegalAccessException iae) { } // fall through if exceptions } @@ -191,9 +191,9 @@ public class LTWWorld extends BcelWorld implements IReflectionWorld { for (int i = 0; i < betterChoices.length; i++) { try { return Class.forName(betterChoices[i]); - } catch (ClassNotFoundException _) { + } catch (ClassNotFoundException cnfe) { // try the next one - } catch (SecurityException _) { + } catch (SecurityException se) { // you get one of these if you dare to try to load an undefined class in a // package starting with java like java.util.concurrent } -- 2.39.5