Bug 345172 - Unexpected advice behaviour within ITD
Summary: Unexpected advice behaviour within ITD
Status: NEW
Alias: None
Product: AspectJ
Classification: Tools
Component: Library (show other bugs)
Version: 1.6.11   Edit
Hardware: PC Windows 7
: P3 blocker (vote)
Target Milestone: ---   Edit
Assignee: aspectj inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-09 13:13 EDT by Max Meier CLA
Modified: 2013-06-24 11:02 EDT (History)
1 user (show)

See Also:


Attachments
aspect file (647 bytes, application/octet-stream)
2011-05-09 13:14 EDT, Max Meier CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Max Meier CLA 2011-05-09 13:13:06 EDT
Hello,

there is an unexpected advice behaviour in the following aspect:

import static java.lang.annotation.ElementType.*;
import java.lang.annotation.*;

public aspect InterType {

  @Retention(RetentionPolicy.RUNTIME)
  @Target({METHOD})
  public @interface MyAnnotation {
  }
  
  public static aspect AroundMethod {
    Object around() : execution(@MyAnnotation * * (..)) {
      return proceed();
    }   
  }
  
  public interface InterTypeIfc {}
 
  // (1)
  @MyAnnotation
  public void InterTypeIfc.m1(int p1) {}
  
  // (2)
  public void InterTypeIfc.m1(int p1, int p2) {}
  
  // (3)
  @MyAnnotation
  public void m1(int p1) {}

  // (4)
  public void m1(int p1, int p2) {}

}

(1,3) correctly adviced by AroundMethod aspect
(4)   correctly not advices by the aspect 
(2)   adviced by AroundMethod aspect --- what i'm not expecting and don't want

Is there any error in the pointcut: execution(@MyAnnotation * * (..)).

Environment: 

Eclipse 3.6 
Eclipse AspectJ Development Tools
Version: 2.1.2.e36x-20110307-1000
AspectJ version: 1.6.11.20110304135300
Comment 1 Max Meier CLA 2011-05-09 13:14:30 EDT
Created attachment 195106 [details]
aspect file
Comment 2 Andrew Clement CLA 2011-05-09 18:44:29 EDT
thanks for the test program.  I've fixed this, although only the case you are hitting.  In your case you have two methods with the same name, only the first of which is annotated, which triggers the problem.  The search code that looks for methods was obviously just using the name rather than enough information to find the right method.  I've now extended it to also check the arity, and so this testcase is fine.  That does mean if you have two methods with the same number of parameters (but differing types) this problem can still happen, so I am not closing the bug for now.  

Comparing the parameters isn't entirely straightforward due to generics, I'll tackle that another day.
Comment 3 Max Meier CLA 2011-05-10 00:23:31 EDT
Great! How can I test your changes?

(In reply to comment #2)
> thanks for the test program.  I've fixed this, although only the case you are
> hitting.  In your case you have two methods with the same name, only the first
> of which is annotated, which triggers the problem.  The search code that looks
> for methods was obviously just using the name rather than enough information to
> find the right method.  I've now extended it to also check the arity, and so
> this testcase is fine.  That does mean if you have two methods with the same
> number of parameters (but differing types) this problem can still happen, so I
> am not closing the bug for now.  
> 
> Comparing the parameters isn't entirely straightforward due to generics, I'll
> tackle that another day.
Comment 4 Andrew Clement CLA 2011-05-10 11:48:30 EDT
I just put a new copy of AspectJ into AJDT - an AJDT build later today should contain them, you can install a dev build using the usual update site: http://download.eclipse.org/tools/ajdt/36/dev/update
Comment 5 Max Meier CLA 2011-05-11 05:39:47 EDT
I tested the new version and it works for the simple case, but reality knows it better. For my real project there are methods with same parameter count. So there it fails again.

I ask myself, whether the approach with the arity of the parameters is the right way. In this special use case only the presence of the annotation should relevant for weaving, count and type of the method parameters should not.

The problem seams only occur with inter-type declaration. With "normal" aspect methods it works fine, case (3) and (4). Is there any difference between those?

Any further idea?
Comment 6 Andrew Clement CLA 2011-05-11 14:12:56 EDT
Yep, ITDs are very different to regular methods.  There is a point where we have the 'fake' method that represents the method that will exist on the target and we have to hunt for the 'real method' that is holding the annotations.  This search was initially only considering the name - and so always hitting the annotated one.  I changed it to also consider parameter count, so now it finds the right one in the testcase supplied, but will fail as I described for same arity but different types.

I will fix the general case and consider parameter types in the search, but I doubt I'll get to that in the next week or so.
Comment 7 Andrew Clement CLA 2011-05-12 15:56:27 EDT
ok, i added more checking.  It will now check the parameter types.  However, the algorithm will still break down if you generics are involved (e.g. the parameter type is a type variable)
Comment 8 Andrew Clement CLA 2013-06-24 11:02:31 EDT
unsetting the target field which is currently set for something already released