Bug 254606 - Configurable weaveinfo message inserts and expressing match constraints
Summary: Configurable weaveinfo message inserts and expressing match constraints
Status: NEW
Alias: None
Product: AspectJ
Classification: Tools
Component: Compiler (show other bugs)
Version: DEVELOPMENT   Edit
Hardware: PC Windows Vista
: P3 enhancement (vote)
Target Milestone: ---   Edit
Assignee: aspectj inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-11-07 10:48 EST by Andrew Clement CLA
Modified: 2013-06-24 11:07 EDT (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Clement CLA 2008-11-07 10:48:48 EST
Weaveinfo messages are nice... but it would be great if they could include more about the actual advice that is matching.  For example in a testcase I have 10 pieces of advice with various pointcuts and a comment next to each indicating whether they should match (or not).  When running a compile I just get a stream of weaveinfos that don't include the info from the comment - its just a list of line numbers for the matched join point and the advice, this is quite unhelpful.  Instead of:

before(): execution(* *(..)) && args(Foo) { // match exactly once with no runtime test
}

I'd like to write:

@MatchCheck("1","should match with no runtime test")
before(): execution(* *(..)) && args(Foo) {
}

The weaver would verify the match count constraint and the extra text would be included in the weave info message (or possibly in an error message if the constraint is violated)

A count constraint obviously does not make sense for all situations (eg. a trace aspect) but it would be nice to police some advice targeted at specific locations:

@MatchCheck(">0")  // must match somewhere
@MatchCheck("1")   // must match one location
@MatchCheck("37")  // must match 37 locations

If the match count varies from the constraint we get a warning from the weaver.

Taking this further.  Whether there is allowed to be a runtime test *could* be part of the annotations:

@MatchCheck("1",runtimetestallowed=false)

Possibly even naming an advice and expressing counts relative to other advices:

@MatchCheck(adviceName="entryTrace","#exitTrace")
before(): execution(* *(..))

@MatchCheck(adviceName="exitTrace","#entryTrace")
after(): execution(* *(..))

expressing that there should be the same numbers of entry and exit matches.

Writing testcases would be made so much easier with this, and if weaving a third party jar and picking up a new version it would help identify the weaving was proceeding in the same way it used to.

I'm not wedded to the attribute names, values and annotation name here, just thinking out loud.  I might also want to tag advice as 'tell me about the matches for this advice regardless of the other weaver options set'.  

Deow can of course allow particular messages to come out where certain pointcuts match but they don't allow for runtime tests and require duplication of pointcuts alongside the applicable advice.
Comment 1 Ramnivas Laddad CLA 2008-11-11 22:10:39 EST
I like the overall idea. I see a few issues, possible alternatives, and an additional use case:

Issues:
======
1. Weavers could come at many stages and we will need to somehow flag the "now do the march check" stage. For example, in state 1, an aspect library may be compiled with perhaps no matches. In stage 2, it may be woven into an application with some matches. In the final stage, an LTW may come into the mix to weave application server code with many more matches.
2. The annotation may need to have user extensions. For example, I may want to express that transaction advice match must not select any method in a class marked with @Repository annotation (almost real example from Spring applications). In such cases, I may need to provide an expression as a part of annotation value.

A possible alternative:
======================
(Note that class and method names are just for facilitating discussion)

If AspectJ could introduce a listener API, say WeaveListener, that can be set in a manner similar to -XmessageHandlerClass. The WeaveLister type can have methods such as: onAdviceMatch(MatchInfo mi). The MatchInfo may contain, an Advice element and matching info (matched element, runtime checks etc.). Advice could have methods such as getAnnotations().
   
A listener could then combine annotations such as @MatchCheck with the MatchInfo object. Developers could use any other annotations if they so choose and include whatever information they need (say, an expression), just use @AdviceName, or not use any annotation at all.

Additional use case:
===================
Such infrastructure may be used for integration tests to check if a advice matched the expected methods--something that every AspectJ developer asks at some point!

First, I could implement WeaveListener to serialize all MatchInfo into a file, then load that data in a test, and make assertions over that information.

class TransactionManagementAspectMatchingTest extends AbstractAspectMatchTest {
    WeaveData wd;
    protected void setUp() {
        wd = loadWeaveData("my-file.wd");
    }

    public void testTransactionAdviceDoesntMatchRepository() {
       // get advice that is annotated with @AdviceName
       MatchInfo mi = wd.getMatchingInfoByAdviceName("txStart");
       JoinPoint.StaticPart[] jpsps = mi.getMatchedJoinPointShadows();
       for (JoinPoint.StaticPart jpsp : jpsps) {
          String typeString = jpsp.getSignature().getDeclaringTypeName();
          Class type = Class.forName(typeString);
          if (type.getAnnotation(Repository.class) != null) {
             fail();
          }
       }
    }
}

If such a listener can be implemented, this will greatly help integration testing.
Comment 2 Andrew Eisenberg CLA 2008-11-11 22:35:58 EST
I like the idea as well.  I see some benefits for tooling, particularly LTW tooling.

A WeaveListener can be added by the eclipse debugger and can (for example):

1. Add gutter markers for the shadows of joinpoints woven at load time.  Currently, there is a lack of editor support for LTW.  This might address it.  Alternatively, the weave need not happen at load time, but rather, the weaver can do a weave and throw away the result, while the WeaveListener keeps track of the matches as they occur.

2. Set weaving breakpoints (either when weaving occurs, or at locations that get woven).  Would be nice to be able to set a breakpoint whenever a deow is reached, or when a pointcut is matched.
Comment 3 Neale Upstone CLA 2009-03-30 13:28:20 EDT
The gutter markers depend on bug 268309.  Looks like I'd better get the test cases for that patch sorted.
Comment 4 Andrew Clement CLA 2013-06-24 11:07:46 EDT
unsetting the target field which is currently set for something already released