Comment 1 Alexandre Vasseur 2005-06-01 10:33:01 EDT

mh, a lot to write actually before hacking that...

the weaved model is like:

weavedClass {
  transient xx_mixin_1 = new Aspect$Mixin();//
  // impl all Mixin ' interface methods using delegation
  void doSomeFromMixinInterface() {
    xx_mixin_1.doSomeFromMixinInterface();
  }
}

Some notes:
1/ the definition does not give a very good control on that, which makes it
rather poor. f.e. in AW we provide:
- custom container thru some mixinOf hence can plug any kind of IoC to
instantiate the mixin instance
- transient or not control
- perclass, singleton, or per instance model
- passing "this" to the mixin (per instance instance) or "class" (per class
class) to the mixin constructor if user wants

2/ what about AJDT stuff
From what I see there are NewMethodTypeMunger and alike for ITD. The
implementation here will require slightly different things, and a fairly deep
level of bytecode work is required to instantiate properly the xx_mixin_<i> (or
whatever mangled name) mixin instances when the weavedClass as more than one
ctor (hence here the mixinOf approach)
That perhaps means some changes in the ASM model to report that properly as well..

3/ what about lang.reflect thing ?
I can see for now a TODO but obviously there will be 2 different models behind
for code style or @style.

4/ last details
Given 2/ there might be some issues with
- proper name mangling and filtering to hide added shadows (since ITD are weaved
first, should be visible method jp, but mixinOf and x_mixin_<i> field set / get
are probably something we want to hide (or ? and what about ctor call of mixin
class etc depending on mixinOf impl.)
- reweavable state (that does contains the type munger list but again this one
is rather different)


Comments welcome. I not eager to implement something if we end up with no
support in AJDT or an impl that is too limited for the users.

Comment 2 Adrian Colyer 2005-08-26 10:31:07 EDT

marking as "enhancement" to make it easier to distinguish between new function
yet to be designed/implemented and bugs that we need to fix.

Comment 3 Adrian Colyer 2005-09-27 05:48:18 EDT

raising to P2 for run in to AJ 5 RC1

Comment 4 Alexandre Vasseur 2005-10-12 06:24:54 EDT

Note 1 (to update doc soon)
will support:

@Aspect
class A {
   @DeclareParents("type pattern")
   SomeInterfaceType  someName;
}

for declare parents implements semantics

as this avoid mandatory use of aspect nested interface (which is bad)

currently working on my box

Comment 5 Alexandre Vasseur 2005-10-12 08:39:23 EDT

[from email]
hi there

some thoughts on @decp.

For marker interface (no impl.) I am now using a field within the
aspect, whose type is the marker interface (ie not a nested interface
in the aspect). This because it decouples the aspect from the marker
interface (1) and because grabbing nested type from a type is perhaps
not the easiest way (2).
So it looks like
@Aspect class A {
   @DeclareParents(typepattern)
   MarkerIntf stuff;
}
It s ugly as we have an unused field - but this is the general deal
with @AJ (f.e. pointcut).
(see docs - it differs from what is in the doc)

now for the ITD that have an implementation we had a proposal (see
docs). But I don't like the fact that we cannot control the way the
mixin gets created (actually we will have to store it somewhere in a
synthetic aspect field, wich sends us back to weaving aspects themself
(same as aspectof), or to gen. on the fly a factory class to host the
mixin field etc. (long time messy in AW).
Instead I argue this syntax:
@Aspect class A {
    @DeclareParents(types = typepattern)
    public static Intf stuff = new IntfImplementation(.....);//stuff
can be IoC - you control it
    // public static is mandatory
    // Intf mandatory to be an interface - which is the ITD contract
}
// the weaved code will simply do delegation for all methods of Intf
Target { // introduced(args) { return A.stuff.introduced(args); }  }

That said it 's tricky to distinguish between decp marker intf. (field
not initialized) and decp with implementation (public static field,
initialized), so we could argue for adding another annotation hence
@DeclareImplements(...) //marker
and @DeclareParents(...) // with implementation


what do you say ??
Side note: I wish we could use A.aspectOf(...) to access the ITD
instance instead of enforcing a public static field, but that would
mean that sometime (depends on binding) you may get
NoAspectBoundException. This is actually sort of contextual ITDs - but
this doesn't seems to exist so far ;-) and I have no use case for that
one - though that is interesting (and can be done later if we want to
support non public static field @DeclareParents)

Comment 6 Alexandre Vasseur 2005-10-14 12:31:04 EDT

ok no matter what you say this is now implemented on my box and fairly neet for
IoC the mixin instance - which should be a very nice replacement for AW mixin
container we had.

I'll polish and commit next week.

Comment 7 Alexandre Vasseur 2005-10-17 10:14:30 EDT

all committed

TODO:
- update docs
- test ASM model / AJDT behavior

Comment 8 Alexandre Vasseur 2005-10-17 10:17:28 EDT

TODO
- test reweavable behavior

Comment 9 Alexandre Vasseur 2005-10-21 09:09:50 EDT

done docs, -showWeaveInfo, reweavable
can be closed at next shipment

Comment 10 Patrik Beno 2005-11-03 07:59:44 EST

I am sorry to say that but this implementation is not very useful because mixin
does not have any link to the target instance.

I.e. in mixin (implementation of the interface) we cannot access the object we
are mixed in...

There should be 1:1 association between the mixin and target instances.

I guess weaved class should look like this:

class Target$$weaved implements SomeIface {

	transient SomeIface iface = TheAspect.create((Target) this);

	String something;
	void doSomething(int howLong) {
	}

	public void run() {
		iface.run();
	}

	public boolean isSuccess() {
		return iface.isSuccess();
	}

	public String sayHello(String name) {
		return iface.sayHello(name);
	}
}

// related code to provide full context:

class Target {
	String something;
	void doSomething(int howLong) {
	}
}

interface SomeIface {
	void run();
	boolean isSuccess();
	String sayHello(String name);
}

@Aspect
class TheAspect {
	@DeclareParents("Target")
	static public SomeIface create(Target target) {
		return new Mixin(target);
	}
}

class Mixin implements SomeIface {
	private Target target;

	public Mixin(Target target) {
		this.target = target;
	}

	public void run() {
		System.out.println(target);
	}

	public boolean isSuccess() {
		return true;
	}

	public String sayHello(String name) {
		return String.format("Hello, %s, from %s", name, target.getClass());
	}
}

Comment 11 Alexandre Vasseur 2005-11-03 08:33:43 EST

I agree with the need for mixin impl. to know about its target. We had
MixinContainer in AspectWerkz for that purpose.

Can't you use some dependency injection for that?

Comment 12 Patrik Beno 2005-11-07 03:50:49 EST

(In reply to comment #11)
> I agree with the need for mixin impl. to know about its target. We had
> MixinContainer in AspectWerkz for that purpose.
> 
> Can't you use some dependency injection for that?

I think we cannot.

Mixin and target are loosely coupled. Target is does not even know that it
subject to mixin operation...

What's up with AspectJ mixins? 
Any plans to support mixins properly?
How hard would it be to get it done right?

thnx.

Comment 13 Alexandre Vasseur 2005-11-07 04:09:24 EST

please write down here any idea you may have for impl.
Note that we need to respect javac compilation rules for @AspectJ mixin

In the code you posted, Target is instrumented so the "Mixin" code is irrelevant:

class Target {
	String something;
	void doSomething(int howLong) {
	}
}

interface SomeIface {
	void run();
	boolean isSuccess();
	String sayHello(String name);
}

@Aspect
class TheAspect {
	@DeclareParents("Target")
	static public SomeIface create(Target target) {
		return new Mixin(target);
	}
}

Means you want:

class Target implements SomeIFace {
	String something;
	void doSomething(int howLong) {
	}
	void run() {
           TheAspect.create(this).run();
        } 
	boolean isSuccess() {
           TheAspect.create(this).run();
        }
	String sayHello(String name) {
            TheAspect.create(this).sayHello(name);
        }
}

Obviously you'll need something else than "create(Target)"
Also you introduce a coupling between the @DeclareParents type pattern and the
create(...) argument. This would require compiler checking, else it would be a 
create(Object o)

Comment 14 Patrik Beno 2005-11-07 05:14:24 EST

(In reply to comment #13)
> please write down here any idea you may have for impl.
> Note that we need to respect javac compilation rules for @AspectJ mixin

ok. once again, much simpler :-)

interface Iface {
   void doSomething();
}

class Mixin implements Iface {
   Object target;
   Mixin(Object target) { ... }
   // ... some impl
}


@Aspect
class TheAspect {
   @DeclareParents("Target")
   static public SomeIface create(Object target) {
      return new Mixin(target);
   }
}

... means I want:

class Target$Woven implements Iface {
   transient Mixin mixin = TheAspect.create(this);

   public void doSomething() {
      mixin.doSomething();
   }
}


SUMMARY

- no need for compiler checks, leave it up to mixin implementor (though I can
image some sort of compiler support but its not mandatory. So signature is:
Aspect.create(Object target)

- mixin instance is created only once when target is created. not in every mixin
call...

- ther is no coupling you mention at all, I used it only for demonstration
purposes (to make more readable/comprehensible)

clear enough? 
any other questions?

Comment 15 Patrik Beno 2005-11-07 05:22:00 EST

sorry, I was a bit too hurry to post this.

ERRATA:

interface Iface {
   void doSomething();
}

@Aspect
class TheAspect {
   @DeclareParents("Target") // or whatever else
   static public Iface create(Object target) {
      return new Mixin(target); // create some instance, link it with target,
type cast it, aspect provider is responsible for all this
   }
}

... means I want:

class Target$Woven implements Iface {
   transient Iface mixin = TheAspect.create(this);

   public void doSomething() {
      mixin.doSomething();
   }
}

hope now its ok...

Comment 16 Alexandre Vasseur 2005-11-07 05:32:24 EST

you make a deliberate choice that the field hosting the mixin is transient. This
can be debatable and can lead to several issues with Serialization (a lot of
users in AW have reported specific needs around that).
F.e. serialize a Target instance and you end up with NPE... 

FYI in AW we had transient controlled by the user
do you need that? what for? 

Also the assumption
transient Mixin mixin = TheAspect.create(this);
cannot be that easily mirrored in bytecode. What will happen is that all
constructor body of Target will be be appended with this field assignment.
Now when the constructor calls a 'this(...)' this requires extra heavy logic to
avoid multiple create(..) call, or a mixin==null check
One of our user then reported that the status of the Target instance (ie this)
is then unclear in some cases. F.e. when this(..) delegation happen, the mixin
will be instantiated based on the state resulting from the 'this(..)' call only
and not the one from the current constructor invoked.


I am willing to do at best to fit user needs - especially because I know this is
far from what I had implemented in AW (f.e. we had implemented static mixin as
well). Obviously the current implementation avoids all those burden and "simple"
is not as simple as it looks like sometimes. It all depends on your use case and
what you do with the feature.

Comments welcome.

Comment 17 Patrik Beno 2005-11-07 05:54:06 EST

(In reply to comment #16)
> you make a deliberate choice that the field hosting the mixin is transient. This
> can be debatable and can lead to several issues with Serialization (a lot of

> FYI in AW we had transient controlled by the user
> do you need that? what for? 

yes it should be configurable
by default, for mixins to be completely transparent, fields introduced by mixins
should be transient so that they do not change the model of the target.

we need transient-by-default for mixin transparency, as mentioned above

however, configurable is fine, i would welcome that :-)

> Also the assumption
> transient Mixin mixin = TheAspect.create(this);
> cannot be that easily mirrored in bytecode. What will happen is that all

you are absolutely right, i did not realize this...
we would need to copy this to every constructor

i hope it is feasible (meaning your architecture allows it)

i would insert following snippet into every constructor:
   if (mixin == null) mixin = TheAspect.create(this);

or maybe better, wrap it in separate method:

// constructor
Target() {
   initMixin();
   // ... rest 
}

// introduced method
private void initMixin() {
   if (mixin == null) mixin = TheAspect.create(this);
}

this would be fine, wouldn't it?
not so extra heavy logic...

this is what we primarily need very much.
...will think about the rest...

thnx

Comment 18 Eclipse Webmaster 2009-08-30 02:51:17 EDT

LATER/REMIND bugs are being automatically reopened as P5 because the LATER and REMIND resolutions are deprecated.