Comment 1 Wes Isberg 2005-12-08 12:20:12 EST

btw, discussion started in bug 119013.

To repeat, I believe the multiple-signatures rule is correct for overridden method-execution wrt declaring type, but not wrt declared exceptions.  But that also means you can't specify the overriding method-execution's of a method-execution specified using throws.  For call join points, it raises the question whether to match using the supertype throws clause when a subtype reference is used; the user might expect otherwise, but it's not correct.  (e.g., the enclosing method would not have catch or declare that it throws the checked exception)

----------------------------- pseudo-code for the call case...
  pointcut callex(): call(void Super.*(..) throws Exception);
  pointcut callnoex(): call(void Super.*(..));
  ...
  class Super {
     protected void m() throws Exception {}
  }
  class Sub extends Super {
     protected void m() {}
  }
  void someMethod() {
     Sub sub = new Sub();
     Super sup = sub;
     sub.m(); // callnoex, but not callex
     try {
       sup.m(); // callnoex and callex
     } catch (Exception e {}
  }
-----------------------------
(Even if the proposed semantics are adopted, this is likely to be treated as an implementation limitation in the near term.)

Comment 2 Andrew Clement 2005-12-12 09:40:10 EST

note - this is a change in behaviour since AspectJ1.2.1 (I've just tested it) - I suspect accidental.

Comment 3 Andrew Clement 2005-12-12 10:23:01 EST

There is a testcase for this behaviour in the suite already:

Line56 in SignaturePatternTestCase.testThrowsMatch().

The line was changed during the 1.5.0 timeframe, I believe to fit in with the changes to signature matching.  This change at SignaturePattern, line 383:

from

  if (!throwsPattern.matches(aMethod.getExceptions(), world)) 
    return FuzzyBoolean.MAYBE;

to
  if (!throwsPattern.matches(aMethod.getExceptions(), world)) 
    return FuzzyBoolean.NO;

enables the 1.2.1 behaviour for signature matching and the testcase I mentioned earlier needs changing to the form it had in 1.2.1.

Returning MAYBE means 'and take a look at signature above us' - returning NO says 'give up now'.

I need Adrian to comment on this.

Comment 4 Adrian Colyer 2005-12-13 05:47:06 EST

I've been thinking about this for the last hour or so. I believe there is a bug here, but it's not the one that is reported.

There are three parts of a signature that may legally vary up and down an inheritance hierarchy:
* the throws clause
* the return type
* the annotations

AspectJ 5 has a simple rule that a signature matching pcd (call, execution in this case) matches if the join point has a signature that is exactly matched by the signature pattern. This works consistently for throws, covariance, and annotations. By narrowing or widening the declaring type pattern, this allows a user to write a pointcut that matches any variation they care about. I'm very loathe to "special case" the throws pattern. If we did so, we'd continually have to explain the exception to the rule, and the somewhat counter-intuitive behaviour of expressions like:

execution(String MyInterface.getName() throws Exception)

which would no longer guarantee to match the execution of any implementation of the interface method. (I think you should be able to take the full signature as it is in the declaration, express that in a signature pattern, and have it match). 

In other words, Wes wrote "While this tracks the multiple-signature model for the
execution join point, it's not correct to say that the method will throw the
exceptions declared by the method it overrides, unless it explicitly declares
them, so it seems like a bug."  But matching like this does /not/ say that the method will throw the exceptions declared by the method it overrides - it says that the join point has a signature that matches. 

However....

AspectJ 5 also introduces a distinction between the signature*s* of a join point, and the subject of a join point. For execution, the subject being the method that actually gets to execute.  @annotation, and the returning(xxx) pattern in after returning. Using after throwing advice, the declared thrown exception type (if any) is also matching on the subject of the join point.

So 

after() throwing(Ex1 ex1) : execution(* *(..) throws Ex1) {
   ...
}

should *not* be showing as a match in the structure model (and should not be weaving NestedClassNeither.m for example) because the subject at the join point cannot throw the given checked exception.

In summary: if after throwing specifies a checked exception in the throwing clause, then this constrains join point matching to only those join points were the subject can throw the given checked exception.

Comment 5 Andrew Clement 2005-12-13 08:39:43 EST

I've implemented the fix for constraining matches if an exception is specified in the throwing() clause.  Here is a sample program that is effected:

public class InheritedThrows {

    static aspect A {
        after() throwing(Ex1 a): execution(* *.*(..) throws Ex1) {}
    }

    public static class Ex1 extends Exception {}

    public static class Ex2 extends Exception {}

    public interface MyInterface {
        public void m() throws Ex1, Ex2;
    }

    private static class NestedClass1 implements MyInterface {
        public void m() throws Ex1 {} // MATCHES HERE
    }

    private static class NestedClass2 implements MyInterface {
        public void m() throws Ex2 {}
    }

    private static class NestedClassBoth implements MyInterface {
        public void m() throws Ex1, Ex2 {}  // MATCHES HERE
    }

    private static class NestedClassNeither implements MyInterface {
        public void m() {}
    }
}

without the fix, the advice matches all four m() methods.  This only applies to checkedexceptions specified in the throwing() clause.

Comment 6 Wes Isberg 2005-12-13 15:25:34 EST

I think we're ok on declaring and return type, but not annotations or throws.

> I think you should be able to take the full signature as
> it is in the declaration, express that in a signature pattern, 
> and have it match.

I respectfully disagree.  That's putting our implementation ahead of Java semantics. The multiple-signature implementation does the right thing wrt declaring type and perhaps return type, but not annotations or throws, which are not inherited even when the method is overridden.  The goal for our semantics is not consistency with our own constructs, but with the Java language as users use it. 

As you suggest, there are six relevant parts to the signature for the method-call and method-execution join points:
- name
- parameters
- declaring type
- return type
- throws
- annotation

Only the first two are part of the JLS-defined method signature, but all are subject to JLS rules wrt overriding, such that the first four can be said to be inherited when overriding.*

Strictly speaking, the declaring type of the supertype should not match a method declared in the subtype.  However, we made it do so because users expect this and it is useful.  They expect this because such methods override the supertype declarations, and are called instead of the supertype.  It is useful because it means you don't have to respecify the pointcut for each implementation.  In retrospect, it might have been better to require users to use + to include any subtypes.

For covariant return types returning a narrower type, the narrower type would not strictly-speaking be matched by the wider type (i.e., it would not match if subtype-narrow-returning method did not override the supertype-wider-returning method).  However, it seems harmless in practice since (like declaring type) this is always true.  And as with declaring type, it avoids requiring users to use + for subtypes, but that would do the same thing.

Method annotations are not inherited (class annotations can be). So the multiple-signature rule matches on the supertype annotation, but the subject method does not have that annotation.  I would think that if "execution(@Me *(..))" matches at a join point, then so would "@annotation(Me)"  But this is not true in the current implementation.

Similarly, throws clauses are not inherited.  I would expect that if "call(* *(..) throws Exception)" matches, then the join point must be in a code block that either catches or throws Exception, but that's not true (e.g., so there is no reason to soften the exception). I would also expect for "execution(void m() throws Exception)" that the subject method  would in fact declare that it throws the Exception.  Further, it seems weird that "execution(* *(..) throws Exception)" matches when the method is explicitly declared not to throw Exception.  I can't think off-hand how I would tell someone to work around this when their pointcut picks out a method-execution they don't want.

I don't mind saying that this is our implementation and we're going to stick with it.  (I personally wish we had instead required + for return and declaring type.)  But we're going to have to document its weirdness either way, because the "clean" rule about multiple signatures results in counter-intuitive matching, and may prevent users from being able to pick out certain join points.

wrt subject of after-returning, when the return type acts as part of the pointcut, it violates the central dogma (join points <- pointcuts <- advice), so I've never liked this.  I've suggested pointcuts "returns(TypePattern|TypeVariable)" and "throws(..)" by way of replacement, but the current form is so useful and well-understood... Anyway, that means I'm not fond of having more situations where after-returning binding makes a difference.

* wrt what users expect from Java ...
The JLS is I think careful not to say that the supertype declaration is "the" declaration or that the subtype declaration is the definition or implementation.  Per the JLS, the declared method is the subject, and the subject method overrides supertype methods with the same signature (the method signature is defined in the JLS 8.4.2 explicitly to not include throws clauses (and method annotations)).  It further constrains the exceptions that may be thrown and the result types that may be declared.  So our notion of "signature" as applied
to method-execution and method-call join points is clearly broader (and more restrictive) than what the JLS uses for methods, esp. wrt overriding.  It's in throws and annotations that our definition overreaches.

------------------------------------------------------ more experiments
package bugs;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

import org.aspectj.lang.JoinPoint;

public aspect OverriddenMethodDeclarations {
	// not inherited
	@Retention(RetentionPolicy.RUNTIME)
	@Target(ElementType.METHOD)	
	@interface Me{}
	
	static class C {
		@Me()
		void m() throws Exception {}
	}
	static class D extends C{
		void m() {}
	}
	static class E {
		D d(){return null;}
		C c(){return null;}
		static aspect A {
			declare warning: execution(C E.*()) : "C E.*()";
			declare warning: execution(D E.*()) : "D E.*()";
		}
	}
	public static void main(String[] args) {
		C c = new C();
		D d = new D();
		C cd = d;
		try {c.m();} catch (Exception e) {}
		try {cd.m();} catch (Exception e) {}
		d.m();
	}
	static aspect A {
		static void log(JoinPoint jp, Object o) {
			System.out.println("" + jp + ": " + o);
		}
		pointcut scope() : within(OverriddenMethodDeclarations);
		pointcut execMe() :execution(@Me void m()) && scope();
		pointcut execEx() :execution(void m() throws Exception) && scope();
		pointcut execAnyEx() :execution(* *(..) throws Exception) && scope();
		pointcut callEx() :call(void m() throws Exception) && scope();
		declare warning : execMe() : "aa @Me void m()";
		declare warning : execEx() : "aa void m() throws Exception";
		declare warning : execAnyEx() : "aa * *(..) throws Exception";
		declare warning : callEx() : "aa call void m() throws Exception";
		before(Me me) : @annotation(me) && execMe() {
			log(thisJoinPoint, "execMe[" + me + "]");
		}
		before() : execEx() {
			log(thisJoinPoint, "execEx");
		}
	}
}

Comment 7 Adrian Colyer 2005-12-13 15:56:35 EST

doh. 

apologies to wes (thanks for coming back) and to andy (who implemented my proposal). one hour of thinking after 5 hours of jetlag clearly isn't enough.

If you look at my own documentation on this issue here: http://www.eclipse.org/aspectj/doc/next/adk15notebook/join-point-modifiers.html, I clearly state that throws, annotations, and visibility modifiers should be matched based on the subject. So I agree with my (non-jetlagged self) and disagree with my (jetlagged) self. 

did you tag that set of changes in any way andy??????

time to dig out the code again....

Comment 8 Adrian Colyer 2005-12-13 17:19:28 EST

fix in tree (as per AJDK notebook). See bugs150/pr119749.aj and the corresponding specification:

    <ajc-test dir="bugs150" title="modifier overrides">
       <compile files="pr119749.aj" options="-1.5">
        	<message kind="warning" line="26" text="C E.*()"/>
        	<message kind="warning" line="25" text="D E.*()"/>
        	<message kind="warning" line="17" text="aa @Me void m()"/>
        	<message kind="warning" line="17" text="aa void m() throws Exception"/>
        	<message kind="warning" line="17" text="aa * *(..) throws Exception"/>
        	<message kind="warning" line="37" text="aa call void m() throws Exception"/>
        	<message kind="warning" line="38" text="aa call void m() throws Exception"/>
        </compile>
        <run class="pr119749">
            <stdout>
             <line text="execution(void pr119749.C.m()): execMe[@pr119749$Me()]"/>
             <line text="execution(void pr119749.C.m()): execEx"/>
            </stdout>
        </run>
    </ajc-test>

Comment 9 Adrian Colyer 2005-12-14 07:05:03 EST

fix available