Bug 95529 - concrete-aspect
Summary: concrete-aspect
Status: RESOLVED FIXED
Alias: None
Product: AspectJ
Classification: Tools
Component: Compiler (show other bugs)
Version: DEVELOPMENT   Edit
Hardware: All All
: P2 enhancement (vote)
Target Milestone: 1.5.0RC1   Edit
Assignee: Alexandre Vasseur CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-05-17 05:53 EDT by Alexandre Vasseur CLA
Modified: 2005-11-12 13:53 EST (History)
1 user (show)

See Also:


Attachments
Testcase for abstract LTW aspect and harness improvements (7.23 KB, application/octet-stream)
2005-10-28 08:30 EDT, Matthew Webster CLA
no flags Details
Patch for loadtime, weaver, ... (8.51 KB, application/octet-stream)
2005-11-02 12:40 EST, Matthew Webster CLA
no flags Details
Updated patch (8.39 KB, application/octet-stream)
2005-11-11 07:50 EST, Matthew Webster CLA
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandre Vasseur CLA 2005-05-17 05:53:16 EDT
for M4

note: need to think some if we want to jit the concrete aspect, or if its not
that needed ie that the shadow munger API allow us to do what we need (perhaps
not easy to deal with perClause and uniqueness enforcement of concrete-aspect
name, but perhaps a burden as well to deal with jit since then no bytecode can
be grabbed from file system ie may confuse the bcel repository and resolvedTypeX
and alike)

Ideas Andy ? I am more in favor of bytecode gen the concrete aspect there.
(that further means the underlying infra needs to support define class callbacks
like "acceptClass" thing used for ajc$Closure and alike when running LTW. I am
ok with that)
Comment 1 Adrian Colyer CLA 2005-08-26 10:32:05 EDT
marking as "enhancement" to make it easier to distinguish between new function
to be implemented in M4 and bugs to be fixed.
Comment 2 Adrian Colyer CLA 2005-09-27 05:49:16 EDT
Raising to P2 to ensure we resolve this in the run-up to AJ5 RC1
Comment 3 Alexandre Vasseur CLA 2005-10-24 08:47:19 EDT
Some notes:

@Aspect
abstract class MyA {
  @Before("pc()")
  .... {..}

  @Pointcut  // see here, nothing defined ie default "" in annotation
  abstract void pc(); // must be abstract for @Pointcut() to be allowed
}

// note: If AJC + run: as we have an abstract aspect, the before advice is not
// applied

Scenario A
----------

aop.xml contains:
<aspect name="MyA"/> // ie refers an abstract aspect
--> what should happen?
I think it is an error, and we report it as such

Scenario B
----------
aop.xml contains:
<aspect name="MyA_Real" extends="MyA">
  <pointcut name="pc" expression="..."/>
</aspect>
--> checks as follow:
- MyA is abstract
- Every abstract poincut from MyA is defined in XML <pointcut elements
- Only void and (noargs) method are allowed
(ie not: <pointcut name="pc(a)" expression="... && target(a)" />
 with lookup of binding for void pc(SomeType x) etc)
[that means only simple pointcut can be xml-defined] 


comments?
Comment 4 Alexandre Vasseur CLA 2005-10-25 05:29:51 EDT
impl done

some notes:
concrete aspect is JIT generated, using @AspectJ style
@Aspect // ie per from super
public class My extends Asbtract {
   @Pointcut(.... the expr from xml)
   mods.. void pc() {} 
   //mods must obey OO rules here
}

this ones is then passed thru the munger for aspectof method to be added
(this does NOT follow the regular process - which assumes as configured weaver,
as we are precisely doing the config at that stage).

doc updated with sample
Comment 5 Alexandre Vasseur CLA 2005-10-25 05:31:11 EDT
spotted that decl precedence cannot be late defined.
added some xml for that (that again is turned to an @AJ annotation)

<concrete-aspect.... precedence="...">
...

Comment 6 Alexandre Vasseur CLA 2005-10-25 06:01:09 EDT
impl commited
work pending on ajdt core to not complain on asbtract @Pointcut and do some
checks (abstract ()V method only)
Comment 7 Alexandre Vasseur CLA 2005-10-25 09:11:16 EDT
all done
TODO = test on error reporting
Comment 8 Matthew Webster CLA 2005-10-26 14:14:29 EDT
This looks great however:
-	 Declaring an abstract aspect is quite valid because it may have an 
effect even if not made concrete e.g. ITDs (see 
http://dev.eclipse.org/mhonarc/lists/aspectj-users/msg04545.html). I am 
working on a testcase.
-	The support for precedence has not been fully specified. The user 
should be able to define it without having to extend an existing aspect as can 
be done with the AspectJ syntax. There is an enhancement request for this: 
https://bugs.eclipse.org/bugs/show_bug.cgi?id=106504
-	Testing error messages will be much easier using the new LTW support 
in the harness. I have started the work and will post a patch when I have one. 
We can reuse all the expected message handling that exists for the “compile” 
task.
Comment 9 Matthew Webster CLA 2005-10-28 08:30:15 EDT
Created attachment 28914 [details]
Testcase for abstract LTW aspect and harness improvements

This started as a simple LTW ITD testcase but needed several small changes to
the harness and weaving adaptor to get consistent message handling. It also
contains a test for empty aop.xml files.
Comment 10 Matthew Webster CLA 2005-11-02 12:40:06 EST
Created attachment 29186 [details]
Patch for loadtime, weaver, ...

1. Improved test for empty aop.xml
2. Avoid creating duplicate world/weaver in WeavingAdaptor.init()
Comment 11 Andrew Clement CLA 2005-11-08 09:06:15 EST
Do we need to integrate Matthews patches here?
Comment 12 Matthew Webster CLA 2005-11-08 12:12:39 EST
Now that bug 95516 is complete we need to remove the warning about abstract 
aspects being declared in aop.xml.
Comment 13 Matthew Webster CLA 2005-11-11 07:50:29 EST
Created attachment 29771 [details]
Updated patch

1. Test weaving adaptor disabled and message issued when no aop.xml found
2. Test declaration and side effect of an abstract aspect
Comment 14 Andrew Clement CLA 2005-11-11 08:51:42 EST
as per discussion on call yesterday, new patch integrated. will close when build
available.
Comment 15 Andrew Clement CLA 2005-11-12 13:53:18 EST
code available in latest dev build.