Comment 1 Andrew Clement 2008-09-05 17:35:01 EDT

try and find some time to think about this before 1.6.2.  If all we do is implement Simone's approach but generating the code for it, I think it isn't worth it - and it doesn't cover all cases, as described.  So there needs to be something clever we can do by being in control of weaving (we are not going to analyse hierarchies).

Comment 2 Simone Gianni 2008-09-07 10:38:50 EDT

What we need for this to work is basically this :
  new C();  // A call in code
 
  ...

  public C() { // The C constructor
    interceptingAspect.weEnteredHere(C.class);
    super();
    otherCode();
    interceptingAspect.weAreExitingHere(C.class);
  }

He could save the C.class upon first call, and only execute the advices upon call to a matching exit point. 

The problem is we cannot write code before super(), so we will have no way to save our "entry point".

The only other way to analyze the current flow, is using the stack.

The simplest "usage" of the stack is reading and analyzing the stack frame, but I agree that it's expensive to make a proper analysis of such poor data.

Another way I can think of is rather creative, but still it's possible : it is based on the fact that a call to another this() is an alternative to the implicit (or explicit) call to super(). No matter what, super() will be called before any local code execution, but calling other this() constructors we can put stuff in the stack in the form of parameters.

AspectJ could create synthetic constructors wrapping real ones, and pass the entry point class up in the chain as a parameter.

For example, given the class A as on the mail, it could be rewritten :

public class A {
  public A() {
    this(A.class);
  }

  protected A(Class entry) {
    // Code that was previously in the main constructor
    interceptingAspect.check(entry, A.class);
  }
}

Similarly B can be rewritten as :

public class B extends A {
  public B() {
    this(B.class);
  }

  protected B(Class entry) {
    super(entry);
    // Code that was previously in the main constructor
    interceptingAspect.check(entry, B.class);
  }
}

This way, we are storing the entry class in the stack itself (passing it as a parameter) and the check to trigger the joinpoint is a simple equality check.

To implement this, AspectJ should :
 - Find classes which needs this kind of weaving (as normal)
 - Find all constructors in those classes (as normal for a A+.new(..) pointcut)
 - Create new synthetic constructors having the same signatures but with a Class parameter added.
 - Rewrite origianl constructors so that they will simply call the new one, adding the current object class as last parameter.
 - The new constructors will contain the bytecode from the original ones.
 - This bytecode must be rewritten only to do the following :
  - The call to "super(..)" or "this(..)" must be rewritten to add the Class parameter.
  - At the end the code to check the joinpoint must be added (as always)

I have no idea if this is at all "compatible" with current bytecode manipulation performed by AspectJ (I think technically it is, I don't know what implications it could have). While I know for sure AspectJ creates synthetic fields and methods, I've never seen it creating synthetic constructors to wrap real ones.

If this is a possible solutions, I can see two "special" situations :
 - Partial hierarchy weaving, limited upwards (like for example, B and C are exposed to weaver, A isn't) : should not be a big deal, B would be weaved as expected but the call to super(args) would call the normal super(args), without adding the new parameter. This "special" situation is no special at all : all classes extends Object, and Object is not exposed to the weaver.
 - Partial hierarchy weaving, limited downwards (like A and B weaved, C is not) : no problem at all, since only B is exposed to the weaver, the aspect is applied only on B. Also this one is no special at all, it is what happens to any execution(A+.something()) pointcut.

I converted the example on the email "weaving manually" the A, B and C classes as explained here, also adding new parameters to constructors, and it works. But my relatively short experience on AspectJ and bytecode manipulation is probably preventing me to see what the real problem with this approach could be.

Comment 3 Simone Gianni 2008-09-07 11:02:53 EDT

Another problem could be the semantics of this new joinpoint. 

First of all this would be a "wraps nothing" joinpoint, cause it identifies a place in the execution flow "before returning from the constructor which was originally called by the client code". So, specifying after, before or around produces same results, cause proceed() leads to nothing (except other advices? see later).

Also, relation with other pointcuts may be a problem. "before returning" seems to mean "after executing any other after advice on the constructor". This IMHO is correct : aspects advicing after(A+.new()) are probably contributing to object initialization, and this joinpoint is here to guarantee that we are executing after the object has been completely initialized. 

Another thing to discuss is wether this pointcut is applied to classes, to specific constructors, or is combinable with an execution() pointcut to specify which constructors we are interested in.

Suppose it is called completeInit, we can have 2+1 forms :

// Form 1
pointcut MyClassInited() : completeInit(MyClass+);

// Form 1 + specification
pointcut MyClassSpecificallyInited2() : execution(MyClass+.new(String)) && completeInit(MyClass+);

// Form 2
pointcut MyClassSpecificallyInited1() : completeInit(MyClass+.new(String));


Since I think of this joinpoint as a specialized version of an execution() pointcut (as opposed to using a call pointcut, due to limitations to weaver exposition), I would go with form 2.

Also, using this kind of joinpoint with a complete and determined class pattern is nothing more than an execution(). In fact, completeInit(MySubClass.new(..)) is completely replaceable with an after on execution(MySubClass.new(..)) .. only when a class pattern which includes a hierarchy is involved this joinpoint has any meaning.

In the case where the implementation follows my previous comment on this issue, knowing which constructors we want to advice changes the weaving algorithm just in the last part : adding or not adding the check code for advice execution. In fact, trying to weave only involved constructors, leads to the need to follow possible execution paths .. which are not many, cause there can be no conditional branches, but still are a problem cause they propagate to super() calls.

Comment 4 Andrew Clement 2008-09-09 01:21:49 EDT

Yes, my initial thought had been it would be implemented via delegating to a generated constructor.  Ensuring the generated artifacts don't interfere with existing join points that a user can see in the code is a pain, but would be necessary.

I agree with all the points raised.

Introducing a new join-point shouldn't be taken lightly - we haven't done it for many years.  The two synchronization related join points I created about 18months ago are still only in existence when activated via -X.

Given the likely implementation and its complexity, I don't think this is going to happen soon.  I would move this out to 1.6.3 but I don't have bugzilla rights to create 1.6.3 right now.

Comment 5 Simone Gianni 2008-09-09 09:54:34 EDT

public aspect CheckingWithSimplestAspectJEver {

    pointcut constructor(A inst) : execution(A+.new(..)) && this(inst);
   
    after(A inst) : constructor(inst) {
        if
(inst.getClass().equals(thisJoinPointStaticPart.getSourceLocation().getWithinType()))

           System.out.println("Done with " +
inst.getClass().getSimpleName());
    }
}


I still feel that this should be somehow a primitive in aspectj, and not an explicit if the user has to write, but it's rather simple to do it anyway :) 

Comment 6 Andrew Clement 2013-06-24 11:02:06 EDT

unsetting the target field which is currently set for something already released