Bug 246125 - [plan] [split] Split matching and weaving
Summary: [plan] [split] Split matching and weaving
Status: RESOLVED FIXED
Alias: None
Product: AspectJ
Classification: Tools
Component: Compiler (show other bugs)
Version: DEVELOPMENT   Edit
Hardware: PC Mac OS X - Carbon (unsup.)
: P3 normal (vote)
Target Milestone: 1.6.3   Edit
Assignee: aspectj inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 265029
  Show dependency tree
 
Reported: 2008-09-03 14:29 EDT by Andrew Clement CLA
Modified: 2009-02-16 11:30 EST (History)
1 user (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-09-03 14:29:54 EDT
 
Comment 1 Andrew Clement CLA 2008-09-04 17:30:31 EDT
For the matcher to be a reusable entity it needs to be separable from the bcel implementation of the interfaces used in the matching process (member/etc).

Currently, because it is all in one project, the line has blurred over time and throughout the org.aspectj.weaver.patterns package are references to the org.aspectj.weaver.bcel types.  For example instanceof checks for BcelAdvice, where 'Advice' would have done.

The simplest way to proceed in teasing them apart initially is change the visibility of some of the bcel types to package only, then they cannot be accessed by the patterns package and so it must change to use the abstract types.

I've started with BcelAdvice and it worked well, reducing (in some cases removing) instanceof/casts and promoting methods/state from BcelAdvice to where it ought to belong, in the abstract Advice declaration.

I'll carry on with this route for a bit then attempt to write a testcase that uses the pattern parsing and matching code, with no reflection world or bcel world.
Comment 2 Andrew Clement CLA 2008-09-04 17:42:22 EDT
worked ok for another couple of types too, but BcelShadow - well there are some tests in the main tests folder that reference it and i think they should be able to test it.  The lazyTjpOptimization flag they are testing isn't really a piece of Shadow state, it is peculiar to bytecode and BcelShadow.  So although I've fixed uses where I can I had to make it public again so it was visible to the testcode.

I've also looked in the org.aspectj.weaver.patterns test folder in the weaver project.  There are some tests based on reflection there and the rest use a bcelworld.  I need some that use a new type of World.  I could split the weaver in two. rename the org.aspectj.weaver.patterns package to org.aspectj.patterns and push all the bcel related material into a project that depends on this new project that encapsulates pattern parsing and matching.  Can't think of a good name for it at the moment.
Comment 3 Andrew Clement CLA 2008-09-05 12:31:01 EDT
i think the new module may get called patterns right now, or maybe matching.

I've finally addressed the hideous AnnotationX type which was always a wrapper that delegated to an eclipsey or bcel annotation underneath.  I've moved it to a proper interface/abstract-impl/subclasses model.  I think it'll be faster and more reliable than the horrible code there previously.  (and it will enable me to break the weaver a little further apart) - just running the tests now
Comment 4 Andrew Clement CLA 2008-09-18 12:24:49 EDT
New module is called 'matcher'

A first pass at the split is done, where matcher contains no bcel related code and uses a simple reflection world to test itself.

initial aspectjmatcher.jar was 900k but that included runtime.jar contents too. I've now split that out so no dependency from matcher on runtime.  Reduced to 793k, but there is more tangling to deal with.  The structure model is tangled up with the matcher, which is what I'll look into next.  Also want to pull out the typesystem abstraction into a new package so a new typesystem provider (eg. based on asm or ITypes) can more easily work out what they need to do.

For now, I might sort out the tests to check I have the right split there, rename some packages and perhaps commit.  All tests are passing.
Comment 5 Michael Scharf CLA 2008-10-06 10:18:07 EDT
Could the new matcher be used to match non java artifacts? 
   http://dev.eclipse.org/mhonarc/lists/aspectj-users/msg09789.html
I would like to use the aspectj pointcut syntax to match non java methods (methods that are dynamically called).
Comment 6 Andrew Clement CLA 2008-10-06 12:11:34 EDT
A reusable matcher will be able to match upon any bunch of 'stuff' that can be represented as something 'like Java' (ie there are types and these types have members, some of which are fields and some of which are methods).

The primary goal here is to enable matching based on JDTs JavaElement view of the world - this would mean matching can be done without compilation of source to bytecode.  It may do what you want, if you can represent your information at the right level of abstraction for the matcher to operate against.
Comment 7 Andrew Clement CLA 2008-10-20 14:31:56 EDT
ok - doing this properly now.  In very slow steps so I can revert more easily when a refactoring goes awry.

First, I'm going to pull up the generic signature parsing code that we wrote for AspectJ5.  Right now it lives in bcel when it really is just about taking strings apart (that happen to be generic signatures).  The matcher needs it but does not want a bcel dependency.  So moving it to the util project.  The tests that can move there are also moved but some higher level tests that used a repository to load some types are 'promoted' to the weaver tests area since that is allowed to have the bcel dependency.  The weaver project is then refactored to use the util versions of signature parsing.

Committed.
Comment 8 Andrew Clement CLA 2008-10-20 14:47:19 EDT
org.aspectj.matcher module created in CVS, and integrated into build scripts - currently tiny (only contains the manifest).

weaver dependency on matcher created.
Comment 9 Andrew Clement CLA 2008-10-20 14:50:28 EDT
trying to remove dependency from types in org.aspectj.weaver to anything structure model related (since matching doesnt require a structure model).  So far this is moving the model helper utils to a new weaver subpackage (org.aspectj.weaver.model).  Seems to mean the following have references to the model from org.aspectj.weaver:

Checker, Shadow, ShadowMunger
Comment 10 Andrew Clement CLA 2008-10-20 17:55:21 EDT
Shadow and ShadowMunger are tied to both the crossreference reporting and the structure model relationship construction.  A reusable matcher may not need either of these so both need moving down from the reusable matcher types.  Moving them down to bcel is perhaps moving them too far since other worlds may want to have this information reported but for now lets push that stuff down to the bcel level.  What we have to do is delegate reporting of information to the world then let a world decide what it wants to do rather that hard coding it into the mungers.  I have done this and now BcelWorld will report a message, add a crossreference relationship and an entry in the structure model.  I would probably now expect the reflection world tests to break as I think they may be using a crossreference mechanism in places.  We will see.

In ShadowMunger there was also the logic to fault in a binary aspect into the structure model.  Clearly it should not live there so it has been moved to the AsmRelationshipProvider (still perhaps not its final resting place but at least it is a structure model related type).  This removes the dependencies from Shadow/ShadowMunger onto structure model types.
Comment 11 Andrew Clement CLA 2008-10-20 17:56:14 EDT
Did same thing for match reporting from Checker that I did for the mungers - no dependency on asm from the org.aspectj.weaver package now.
Comment 12 Andrew Clement CLA 2008-10-20 19:35:11 EDT
Removed dependency on AsmManager from World. 
Removed BcelObjectType dependency from ReferenceType.
Added matcher dependency to other modules that need it.
Comment 13 Andrew Clement CLA 2008-10-21 13:12:22 EDT
New methods on the WeavingSupport interface to remove direct dependencies:

- BcelCflowAccessVar in ConcreteCflowPointcut
- BcelAccessForInlineMunger in PerObject, PerSingleton, PerTypeWithin

JoinPoint was used in IfPointcut but it turned out to be redundant code.  i can't (right now) see why a matcher would need reference to the runtime jar contents.

getting there.
Comment 14 Andrew Clement CLA 2008-10-21 13:24:43 EDT
current intended package makeup for matcher (will be renamed oa.matcher when stable):

oa.weaver
oa.weaver.ast
oa.weaver.patterns
oa.weaver.reflect
oa.weaver.tools < weavingadapter class will remain in bcel from this package
oa.weaver.internal.tools

Latest problems with that:

PerCflow - BcelAccessForInlineMunger reference 
  - fixed with new WeavingSupport method

PointcutParser touches oaweaverbcel.AtAjAttribtues 
  - promoted internal BindingScope type to top level type in oa.weaver

IWeaveRequestor references UnwovenClassFile 
  - IUnwovenClassFile interface created

ICrossReferenceHandler references IRelationship.Kind 
  - made it string with a helper in IRelationship.Kind.getKindFor(String)
  - this will likely impact AJDT (I think)

AjcMemberMaker refers to JoinPoint 
  - removed dependency, signatures for types it needed are directly coded

The oa.weaver.reflect stuff comes across to provide the lightweight implementation of a reflection world - we need a world for testing anyway so using the reflection one makes sense.  The test refactoring is quite tough since they are currently mixed up (tests for bcelworld/reflectionworld) when ideally there should be a test of <anykindofworld>tests that are extended with bcel and reflection subclasses.  I'll worry about tests once the packages are moved across.  I think the last thing to do will be renaming to oa.matcher, that should be done after the test refactoring.
Comment 15 Andrew Clement CLA 2008-10-21 14:53:10 EDT
Next:

Shadow - references JoinPoint 
  - copied string constants from JoinPoint into Shadow class

WeaverStateInfo - references BcelTypeMunger
  - new class TemporaryTypeMunger - ugly as hell, ought to look at again later
Comment 16 Andrew Clement CLA 2008-10-21 15:49:34 EDT
it now compiles with those packages moved into org.aspectj.matcher

but then I discovered on building it that lib/bcel was referenced from matcher.  If I remove that dependency it breaks because the WeakClassLoaderReference type depends on the bcel ClassLoaderReference type (so that we can pass our reference down into bcel to be used).  I need to break the dependency.

Almost straightforward to break except the ReflectionWorld for 1.5 uses a bcel annotation finder and needs a WeakClassLoaderReference it can pass to bcel.  Not quite sure what impact this has though.  At the moment the 1.5 related reflection world is in the separate weaver-5 project which I have not split into a matcher/weaver.  This means all the tests and infrastructure that will be in the org.aspectj.matcher will be non 1.5 specific - is that a problematic limitation? hmm
Comment 17 Andrew Clement CLA 2008-10-22 01:48:39 EDT
I've just made the split.  All tests were passing so now the weaver packages have been moved to the org.aspectj.matcher module.  Remaining tasks:

- copy/move tests from weaver into org.aspectj.matcher
- rename org.aspectj.matcher packages
- check what damage it causes AJDT
- test inside Spring
Comment 18 Andrew Clement CLA 2008-10-22 13:48:06 EDT
starting to move the tests.  This is more complex than just a simple drag-n-drop of the test classes into the new module though.  Many of the tests are tied to testing a bcel world when in fact they would be valid for any implementation of World (Reflection/Bcel/JDT) and so in the move I create a common base test case (CommonXXX) and then in the matcher module I create the reflection derivative and in the weaver module I create the Bcel derivative.
Comment 19 Andrew Clement CLA 2008-10-28 13:20:19 EDT
Lots of headaches moving the tests.  The biggest problem is that we try to support Java 1.4 and Java 1.5.  The reflection delegate for 1.5 (that supports annotations) uses bcel and AjType (from aspectjrt.jar) and so if I cause the matcher to depend on 1.5 reflection support then I pick up bcel and aspectjrt.jar and include them in the matcher.jar (noooooo...)  So all I will do for now is not have any 1.5 related tests in matcher and leave them where they are in the weaver.
Comment 20 Andrew Clement CLA 2008-10-28 16:35:10 EDT
first pass at moving tests is in.  Build is broken right now though (classpath) so I'll get that working again now before proceeding.
Comment 21 Andrew Clement CLA 2008-12-09 14:34:46 EST
closing bug as first pass at this is done.  There is an:

org.aspectj.matcher.jar 

in the distribution that includes the first cut of code.  Issues it probably still has:

- mutator methods defined on what should be immutable objects...
- not completely specified interface methods where the existing bcel and reflection world implementations may disagree.
- still a bit of weaving 'knowledge' in the matcher.  It can't actually weave but some of the logic for determining runtime residue for pointcuts is still left in there (so the jar could be smaller).
- not enough of the tests brought up from weaver/weaver5 into the matcher