Bug 86818 - Consider extending type pattern matching to support hasmethod(), hasfield()
Summary: Consider extending type pattern matching to support hasmethod(), hasfield()
Status: ASSIGNED
Alias: None
Product: AspectJ
Classification: Tools
Component: Compiler (show other bugs)
Version: unspecified   Edit
Hardware: PC Windows XP
: P2 enhancement with 33 votes (vote)
Target Milestone: ---   Edit
Assignee: Adrian Colyer CLA
QA Contact:
URL:
Whiteboard:
Keywords:
: 239089 (view as bug list)
Depends on:
Blocks:
 
Reported: 2005-02-28 07:33 EST by Adrian Colyer CLA
Modified: 2013-06-24 11:07 EDT (History)
9 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Adrian Colyer CLA 2005-02-28 07:33:06 EST
Consider the use cases: 

1) every type that has a field with a certain property (eg. type, or
annotation), should have a mixin interface used by the aspect to manage that
property.

(From Michael Nascimento) :
"I wanted to introduce a mixin in any class containing a method with a
certain annotation. This is the case for genesis (
https://genesis.dev.java.net , still using AW 1.1, going to migrate to
AW 2.0 next week ) CommandResolver interface and its default
implementation. The idea is any class that had a method annotated with
@Remotable/@Transactional had this interface introduced in order to
support two core genesis features, transparent remoting and
transactional support."

2) Any type defining a method setFoo(Foo) should have a Foo instance DI'd into
it after it is initialised.

Both of these use cases are easily dealt with by extending AspectJ's type
pattern language to support hasfield() and hasmethod() type pattern 'designators'. 

The syntax would be:

hasfield(AnnotationPattern? FieldTypePattern NamePattern)

hasmethod(AnnotationPattern? ReturnTypePattern NamePattern(TypePatternList))

Use case 1 above is now addressed by:

declare parents: hasfield(@SomeAnnotation * *) implements MyMixin;

Use case 2 is addressed by:

after(Object obj) returning : execution(hasmethod(void setFoo(Foo)).new(..)) &&
this(obj) {
   // call setFoo on 'this' reflectively
}

=============

Note, this would be the first time we have introduced type pattern designators.
It might make sense to do this at the same time as supporting a type pattern
'language' - i.e. add composition (have it already) and abstraction to type
patterns. This would be a much bigger change of course. It would require a means
to declare named type patterns (just as we declare pointcuts today), to compose
type patterns (we already have this), and to use a named type pattern in place
of an anonymous type pattern at any point in the AspectJ grammar that a type
pattern is accepted.

For example:

aspect Foo {

  // tpatterns have visiblity just like pointcuts, may be declared in classes?
 private tpattern requiresMixinSupport :
    hasmethod(@Remotable * *.*(..)) || hasmethod(@Transactional * *.*(..));

  // need some way to indicate type pattern name rather than type name, a 
  // prefix char is the obvious - most have meanings that are already taken.
  // can't use $ (which I prefer) because it is legal in Java identifiers.
 declare parents: &requiresMixinSupport implements MyMixin;
 
}
Comment 1 Adrian Colyer CLA 2005-02-28 07:33:26 EST
oops, should have made this severity 'enh'...
Comment 2 Michael Nascimento CLA 2005-08-08 23:46:40 EDT
I'd like to point out that we cannot migrate genesis to AJ until it supports 
something like hasmethod()/hasfield(). This is, for sure, the most critical 
feature missing in AJ for us.
Comment 3 Adrian Colyer CLA 2005-08-17 15:17:25 EDT
This feature isn't going to make it into AspectJ 1.5.0 - all of the work on generics has created too much 
schedule pressure. We've also pushed the current pointcut language just about as far as we can with 
the additions for annotations, generics, varargs, and other Java 5 features. Further extensions need to 
be considered as part of a thorough review of the pointcut language direction. AspectJ *will* end up 
with more complete support for type patterns (including hasMethod() / hasField() or their equivalents) 
in a release beyond 1.5.0. 

In the interim, if your need is to associate state and behaviour on a per-instance basis with objects that 
have methods with certain annotations you may be able to make a design using perthis() work. For 
example:

/**
  * one instance created for each object in which we encounter a transactional or remotable
  * method execution join point.
  */
pubic aspect CommandResolution(perthis(remotableOrTransactional()) {

  pointcut remotableOrTransactional() : 
     execution(@Remotable * *(..)) || execution(@Transactional * *(..));

   private int x;  // state particular to the remotable / transactional object

   // any advice particular to the remotable / transactional object  here

}

now if at any point in your program you need to get hold of the CommandResolution aspect for a given 
object you can ask:

CommandResolution.hasAspect(someObject)  // returns true if an aspect instance has been created
CommandResolution.aspectOf(someObject) // returns the aspect instance

Comment 4 Michael Nascimento CLA 2005-08-19 17:52:47 EDT
The main idea would be to introduce a mixin in a class with a certain method or 
field, so this wouldn't help a lot.

I wonder if the problem is the work required to implement the feature or if you 
are unsure about how/if this feature should be implemented.
Comment 5 Adrian Colyer CLA 2005-08-20 07:42:12 EDT
the mechanics of the basic implementation (matching types based on the presence
of a method/field meeting certain criteria) is easy given all the signature
matching code we already have.

the thing I worry about is compound effect on the complexity of the pointcut /
type pattern language of all the additions. Pushing into type pattern
designators is ideally done at the same time as providing abstraction and
composition for type patterns (as per my earlier comments in this enhancement
request). Getting the language design right for that is a bigger task (and the
implementation cost is also greater of course).

On the basic type patterns hasmethod and hasfield though... should the semantics
be has method (including inherited), has declared method (members declared in
type under consideration only), or has visible method (includes inherited, but
not private members of supertypes)? I favour the last of these three I think.

I'm interested to hear comments from the others on cc whether or not this
feature is sufficiently attractive to put in ahead of the full type pattern
abstraction capability. I don't think it comprises the future design in any way
by doing it now.

Comment 6 Adrian Colyer CLA 2005-08-20 08:17:28 EDT
actually.... there may be a way.

if we allowed hasmethod / hasfield as type patterns only in declare parents and
declare @type (a restriction that could be lifted at some point in the future
should we so decide) then we don't have to deal with things like:

execution(hasfield(@Foo * *) hasmethod(* someMethod(int, int)).(String,
hasfield(* amount)))

and yet all such things are still expressable by using the declare parents or
declare @type forms to add either a marker interface or annotation to matching
types so that regular pointcut expressions can pick out those types.

this design would be doable in AJ 5 timeframe i should think...
Comment 7 Adrian Colyer CLA 2005-08-20 14:20:36 EDT
...and then I thought some more about it. The implementation is trivial if you
say that in a declare parents / declare @type statement hasmethod and hasfield
do not take into account ITDs - but that seems very unsatisfactory since it
means that moving a method out of a type and into an ITD (or vice-versa) could
have unintended consequences and makes ITD members second-class citizens.

So ITD members must be taken into account, but consider the statements (in the
same or multiple aspects)

declare parents : hasmethod(* foo()) implements X;

public void Y.foo() {}

as a result of which Y should implement X. Currently we do all operations on the
type hierarchy before any ITD operations. This algorithm fails in the above
situation. A simple switch of order to perform ITD processing ahead of declare
parents looks attractive but means that we can't perform checks for illegal
overrides at the time we first process the ITDs.  So we would need to change to
an evaluation order something like this:

* process all ITDs, w/out illegal override checks
* process all declare @method / @field / @constructor  (can affect
hasmethod/field matching)
* process declare parents and declare @type as we do today, from super to sub
and with iteration for interaction between declarations
* now do illegal override checks for all ITDd members 
* and now.... the decp / dec @type processing may have changed the matching of
the declare @method / ... statements, which in turn may change the decp matching
once more - eurgh!

there's some serious cycle detection / resolution thinking to be done before
such an algorithm could be safe. Introducing an interaction between member
signatures and type hierarchy processing adds a lot of additional complexity.
Comment 8 Michael Nascimento CLA 2005-08-23 12:27:51 EDT
About this:

> On the basic type patterns hasmethod and hasfield though... should the 
> semantics be has method (including inherited), has declared method (members 
> declared in type under consideration only), or has visible method (includes 
> inherited, but not private members of supertypes)? I favour the last of these 
> three I think.

I agree the default behaviour should be the last one mentioned, but 
constructions including such as hasmethod(private @Remotable *.*(..)) should  
work as well.

I also agree the last proposed use case (about declare @type statement 
hasmethod and hasfield taking into account ITDs) is needed for this feature to 
be implemented. In fact, these constructs wouldn't be very useful for genesis 
it that wasn't the case.
Comment 9 Adrian Colyer CLA 2005-08-24 11:27:24 EDT
I spent some time thinking about stable rules for managing declare statements
with interactions whilst on the train today. I think the following algorithm has
most of the properties we are after:

1) do all ITDs
2) sort declares into List<Set<Declares>> with the declares in each
set being of the same precedence, and the list being in precedence
order, highest first.
3) within each Set<Declares> :
   a) match all declares (but don't do anything) to find the set that
   currently match, record those that did not match
   b) process all matched declares, from supertype to subtype
   c) if the set of unmatched declares is not empty, repeat (3) with
   the unmatched set as argument iff at least one declare did match during this 
   round of processing

the latest AspectJ build from the download page supports a -XhasMember option,
which if enabled allows you to use hasmember and hasfield within declare parents
and declare @type, subject to the restriction discussed in this report that it
doesn't currently match ITDs. The syntax is simply
has(method|field)(SignaturePattern), where SignaturePattern accepts anything you
can write in a call / execution / get / set pointcut. It's not documented
anywhere at this point in time since it really needs the full algorithm above to
be implemented before it can be unleashed on the world at large, but should be
useful already for some use cases and for experimentation. Test suite is in
tests/org.aspectj.systemtest.ajc150.HasMember, with the actual source files
compiled by the tests in tests/hasmember
Comment 10 Adrian Colyer CLA 2005-08-26 11:18:26 EDT
the algorithm in this bug needs considering in the M4 timeframe regardless of
hasmethod/field (and if we get it right, that facility can be fully enabled)
Comment 11 Adrian Colyer CLA 2005-10-28 06:11:53 EDT
For 1.5.0 we should document the behaviour that we have implemented as an X option. The full solution 
will be deferred until 1.5.1.
Comment 12 Adrian Colyer CLA 2005-11-03 12:30:04 EST
A refinement to the algorithm in step 3:

1) do all ITDs
2) sort declares into List<Set<Declares>> with the declares in each
set being of the same precedence, and the list being in precedence
order, highest first.
3) within each Set<Declares> :
   a) match all declares (but don't do anything) to find the set that
   currently match, record those that did not match
   b) process all matched declares, from supertype to subtype
   c) match all of these (previously matched) declares again (without doing anything of course). If any
      of them no longer match we have an ambiguity situation (which must be based on a negation 
     - something like "&& !Foo+" in a pattern. An ambiguity that has not been resolved by dec 
     precedence (which this is) should result in an error message
   c) if the set of unmatched declares is not empty, repeat (3) with
   the unmatched set as argument iff at least one declare did match during this 
   round of processing
Comment 13 Adrian Colyer CLA 2005-11-27 13:46:26 EST
I've decided not to document the X option either at this point in time. The whole show can get addressed in one swoop in 1.5.1.
Comment 14 Ron Bodkin CLA 2006-08-17 17:23:11 EDT
Any plans to implement this for 1.5.3?

I discovered another use case that will probably interest you Adrian. It would be valuable to optimize the performance of load-time weaving with Spring 2.0. You wisely decided to remove the <exclude within="org.springframework..*"/> clause from Spring 2.0's META-INF/aop.xml (I'm sure glad you did that).

A technique that I've found useful to improve performance while being open for extension is to use inclusions instead. For the configurable annotation consuming aspects you could add something like this to let the aop.xml file be extensible yet it would still optimize load-time weaving performance:
<weaver>
  <include within="(@Configurable *)+"/>
  <include within="(@Transactional *)+"/>
  <include within="hasmethod((@Transactional *) * *(..))"/>
</weaver>

While you'd think evaluating hasmethod would be similar in performance to weaving, I've seen a lot of overhead in weaving in my test cases that can be avoided with include tests like this... 

However, I can't test this on a 1.5.3 dev build. When I try to use
  pointcut hasMethod() : hasmethod(@Marker * *(..));

I get 

Syntax error on token ")", "name pattern" expected
Comment 15 Ron Bodkin CLA 2006-08-17 17:27:12 EDT
Sorry make that
    pointcut hasMethod() : within(hasmethod(@Marker * *(..)));
                                           ^
Which gives:

Syntax error on token "(", ")" expected
Comment 16 Andrew Clement CLA 2006-10-25 04:23:56 EDT
not working on this for a point-point release - moving to 1.6
Comment 17 Ramnivas Laddad CLA 2006-11-30 01:01:35 EST
Just wanted to add another use case I just implemented for the Spring framework: http://opensource.atlassian.com/projects/spring/browse/SPR-2896. The use of hasmethod() is really useful here (tried it, and it works well). Of course, since the hasmethod() is still experimental, the fix that uses it has been moved to a comment.
Comment 18 Andrew Clement CLA 2007-12-07 09:39:32 EST
This had a lot of votes - and I have finished the full implementation.  However, it drastically changes the internal weaving loops related to the use of:

- intertype declarations
- declare parents
- declare annotation

Because we haven't had an AspectJ release for a while since 1.5.3 I wanted to get 1.5.4 out without introducing such a drastic change that may have destabilising effects on the robust 1.5 code stream.

I will get this into an early driver of 1.6.0 instead - which won't be far off.
Comment 19 Andrew Clement CLA 2008-01-22 13:52:26 EST
implementation ready to go but didn't make it into 1.6.0m1 - that already had too much change in it!
Comment 20 Avatar CLA 2008-07-01 08:29:03 EDT
*** Bug 239089 has been marked as a duplicate of this bug. ***
Comment 21 Andrew Clement CLA 2013-06-24 11:07:26 EDT
unsetting the target field which is currently set for something already released