]> source.dussan.org Git - aspectj.git/commitdiff
Support for @RequiredTypes and abstract aspects
authorAndy Clement <aclement@gopivotal.com>
Tue, 21 Oct 2014 20:19:19 +0000 (13:19 -0700)
committerAndy Clement <aclement@gopivotal.com>
Tue, 21 Oct 2014 20:19:19 +0000 (13:19 -0700)
12 files changed:
docs/dist/doc/README-183.html [new file with mode: 0644]
docs/dist/doc/index.html
org.aspectj.ajdt.core/src/org/aspectj/ajdt/internal/compiler/lookup/EclipseSourceType.java
org.aspectj.matcher/src/org/aspectj/weaver/CrosscuttingMembersSet.java
runtime/testsrc/org/aspectj/runtime/reflect/RuntimePerformanceTest.java
tests/bugs183/436653/abstract/A.java [new file with mode: 0644]
tests/bugs183/436653/abstract/AA.java [new file with mode: 0644]
tests/bugs183/436653/abstract/Code.java [new file with mode: 0644]
tests/bugs183/436653/abstract/X.java [new file with mode: 0644]
tests/src/org/aspectj/systemtest/ajc183/Ajc183Tests.java
tests/src/org/aspectj/systemtest/ajc183/ajc183.xml
weaver/src/org/aspectj/weaver/ltw/LTWWorld.java

diff --git a/docs/dist/doc/README-183.html b/docs/dist/doc/README-183.html
new file mode 100644 (file)
index 0000000..5b97481
--- /dev/null
@@ -0,0 +1,114 @@
+<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 3.2 Final//EN">
+<html> <head>
+<title>AspectJ 1.8.3 Readme</title>
+<style type="text/css">
+<!--
+  P   { margin-left:  20px; }
+  PRE { margin-left:  20px; }
+  LI  { margin-left:  20px; }
+  H4  { margin-left:  20px; }
+  H3  { margin-left:  10px; }
+-->
+</style>
+</head>
+
+<body>
+<div align="right"><small>
+&copy; Copyright 2014 Contributors.
+All rights reserved.
+</small></div>
+
+<h1>AspectJ 1.8.3 Readme</h1>
+
+<p>The full list of resolved issues in 1.8.3 is available 
+<a href="https://bugs.eclipse.org/bugs/buglist.cgi?query_format=advanced;bug_status=RESOLVED;bug_status=VERIFIED;bug_status=CLOSED;product=AspectJ;target_milestone=1.8.3;">here</a></h2>.</p>
+
+<ul>
+<li>1.8.3 available 22-Oct-2014
+</ul>
+
+<h2>Notable changes</h2>
+
+<h3>Conditional aspect activation with <tt>@RequiredTypes</tt> - <a href="https://bugs.eclipse.org/bugs/show_bug.cgi?id=436653">Issue 436653</a></h3>
+
+<p>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 <tt>"can't find type"</tt> messages and currently requires the user to add unnecessary entries to their
+build dependencies just to keep the unused aspects happy.
+</p>
+<p>With AspectJ 1.8.3 it is now possible to express a constraint on an aspect. The <tt>@RequiredTypes</tt>
+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.
+</p>
+<p>Example:</p>
+<pre><code>import org.aspectj.lang.annotation.*;
+
+@RequiredTypes("com.foo.Bar")
+public aspect Foo {
+  before(): execution(@com.foo.Bar * *(..)) {}
+}
+</code></pre>
+
+<p>
+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 <tt>'com.foo.Bar'</tt> 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 <tt>"can't find type"</tt> messages.
+</p>
+
+<h3>cflow and the pre-initialization joinpoint changes due to Java 7 verifier modifications - <a href="https://bugs.eclipse.org/bugs/show_bug.cgi?id=443477">Issue 443477</a></h3>
+
+<p>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:
+
+<pre><code>execution(* foo(..)) && cflow(within(Bar))</code></pre>
+
+<p>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:</p>
+
+<pre><code>execution(* foo(..)) && cflow(execution(* Bar.*(..))</code></pre>
+<p>or</p>
+<pre><code>execution(* foo(..)) && cflow(within(Bar) && execution(* *(..)))</code></pre>
+
+<p>
+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 :)</p>
+
+<h3>around advice and lambdas - <a href="https://bugs.eclipse.org/bugs/show_bug.cgi?id=445395">Issue 445395</a></h3>
+
+<p>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 <tt>-XnoInline</tt>).</p>
+
+<p>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.</p>
+
+<p>
+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.
+</p>
+
+<!-- ============================== -->  
+</body>
+</html>
index 64d8b8ec716f86f7c80b6d46f0eefeb8d07e5386..3b3558b9cee10394af8ec8a1d9b6084f32a57005 100644 (file)
 <tr> <td>README's
      </td>
      <td>Changes and porting guide for AspectJ 
+        <a href="README-183.html">1.8.2</a>,
         <a href="README-182.html">1.8.2</a>,
         <a href="README-181.html">1.8.1</a>,
         <a href="README-180.html">1.8.0</a>,
index 9b04c6d1fb7aec988dd5f876ad83b4c7967e8fbb..dc82d4aac54259af1a66e24442da406a6b60bdcd 100644 (file)
@@ -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) {
index 3937830cdc9dfaed3d6401b48149dc840bd9abf3..969f96ece4a62d483a280afd73df7649580fb8c5 100644 (file)
@@ -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 */<ResolvedType, CrosscuttingMembers> members = new HashMap<ResolvedType, CrosscuttingMembers>();
+       private final Map<ResolvedType, CrosscuttingMembers> members = new HashMap<ResolvedType, CrosscuttingMembers>();
 
        // List of things to be verified once the type system is 'complete'
-       private transient List /* IVerificationRequired */<IVerificationRequired> verificationList = null;
+       private transient List<IVerificationRequired> verificationList = null;
 
        private List<ShadowMunger> shadowMungers = null;
        private List<ConcreteTypeMunger> 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<ResolvedType> knownAspects = members.keySet();
                Set<ResolvedType> toBeReplaced = new HashSet<ResolvedType>();
                for (Iterator<ResolvedType> 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);
                        }
                }
index ac487539a5d89e721edca5d2110406c5e4de0947..25959aebcfc95ac4319b751c70e88f75e775b073 100644 (file)
@@ -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 (file)
index 0000000..9f4b93d
--- /dev/null
@@ -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 (file)
index 0000000..eceb74d
--- /dev/null
@@ -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 (file)
index 0000000..f956f53
--- /dev/null
@@ -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 (file)
index 0000000..e0450f4
--- /dev/null
@@ -0,0 +1,8 @@
+import org.aspectj.lang.annotation.*;
+
+//@RequiredTypes("A")
+aspect X extends AA {
+  @SuppressAjWarnings("adviceDidNotMatch")
+  before(): execution(* *(..)) { System.out.println("X.before"); }
+}
+
index bb033b6dbbdb05bdf9971dac7dd21a57403b595c..db6878e157e1641f856b29141860fcefe5fca3f2 100644 (file)
@@ -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");
        }
index c24987f6bbff491a983d7a923af289340aefc56b..e1c40ae72a465116796da050388782921c97297a 100644 (file)
                </stdout>
            </run>
     </ajc-test>
+
+       <ajc-test dir="bugs183/436653/abstract" title="suppress type not found - abstract 2">
+               <compile options="-1.8 -Xlint:ignore" files="A.java" outjar="codeA.jar"/>
+               <compile options="-1.8" files="X.java AA.java" classpath="codeA.jar" outjar="aspects.jar"/>
+               <compile options="-1.8" files="Code.java" aspectpath="aspects.jar"/>
+               <run class="Code">
+               <stdout>
+               <!--
+                These don't come out because AA is missing required type A in the last compile step
+               <line text="X.before"/>
+               <line text="X.before"/>
+               -->
+               <line text="Code.m()"/>
+               </stdout>
+               </run>
+    </ajc-test>
+    
+       <ajc-test dir="bugs183/436653/abstract" title="suppress type not found - abstract 1">
+               <compile options="-1.8 -Xlint:ignore" files="A.java" outjar="codeA.jar"/>
+               <compile options="-1.8" files="AA.java X.java" classpath="codeA.jar" outjar="aspects.jar"/>
+               <compile options="-1.8" files="Code.java" aspectpath="aspects.jar"/>
+               <run class="Code">
+               <stdout>
+               <!--
+                These don't come out because AA is missing required type A in the last compile step.
+                Difference between this test and the previous one is that this is a different order
+                of aspects (addOrReplaceAspects called with the abstract aspect first here).
+               <line text="X.before"/>
+               <line text="X.before"/>
+               -->
+               <line text="Code.m()"/>
+               </stdout>
+               </run>
+    </ajc-test>
     
        <ajc-test dir="bugs183/436653" title="suppress type not found 4">
                <compile options="-1.8 -Xlint:ignore" files="X.java" outjar="aspects.jar"/>
index eff7a5999e6d7a0011f00763b8a770e2aa4bc8df..49185a2fb2f1ab7ea692732d3fe2dc312b3a9245 100644 (file)
@@ -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
                        }