MutableClassInfo?

[kabirkhan]

Looking at using ClassInfo in AOP
http://www.jboss.com/index.html?module=bb&op=viewtopic&t=148135
http://www.jboss.com/index.html?module=bb&op=viewtopic&p=4199716#4199716

We will initially go from using two abstractions (reflect and javassist) to using three abstractions (reflect, javassist and ClassInfo). The reflect stuff can be got rid of, since the initial focus is on getting rid of duplicated code in our pointcut matchers. However, we will still end up with ClassInfo (for reading classes) and raw javassist (for modifying classes). I'm toying with the idea of being able to just use ClassInfo everywhere. Would there be any interest in MutableClassInfo/-MethodInfo etc. interfaces backed by javassist? The javassist versions of ClassInfo could implement those interfaces.

[stalep]

as we've stated in the aop design forum (and here) is that the current state of jboss-reflect is too limited for aop to use at all. a well designed MutableClassInfo can change that though.
from what i can see it has to support the following:

- the possibility to create a classobjectrepresentation from a byte[]/File/Url
- be able to change/add/remove fields/methods/constructors/classes similiar to what javassist support
- do finegrained operations/checks ala annotation visibility (which is only possible with javassist and not java.lang.reflect afaik)
- be able to get a classobjectrepresentations (ala ClassFile in javassist) from an existing ClassInfo object
- support scoped classpools
++?

[stalep]

if we could design us away from the usage of javassist.bytecode.ClassFile and perhaps include that into MutableClassInfo we would get away with only using the MCI object in aop.
from what i can see now there is only one place where we use ClassFile for something else than getting annotation metadata and that is in org.jboss.aop.standalone.Compiler where its used to find classname from a byte[]. this can be done with CtClass too, but i guess that doing it with ClassFile is more efficient...?

[alesj]

as we've stated in the aop design forum (and here) is that the current state of jboss-reflect is too limited for aop to use at all. a well designed MutableClassInfo can change that though.

What about if we just added 'mutable' wrapper/delegate.
e.g. you would actually create your own ClassInfo impl based on existing JavassistCI:
Extra stuff would be held in this mutable wrapper/delegate.
When invoked, first check extra stuff if it exists, else delegate.

- the possibility to create a classobjectrepresentation from a byte[]/File/Url

What exactly do you mean here?
e.g. ClassInfo ci = factory.getClassInfo(byte[]/File/Url); ?

- be able to change/add/remove fields/methods/constructors/classes similiar to what javassist support

How do you do this in your prototype?
I guess a single place where you know the aop rules would be enough?
e.g. AOP impl of TypeInfoFactory overridding instantiate method

- do finegrained operations/checks ala annotation visibility (which is only possible with javassist and not java.lang.reflect afaik)

Annotation visibility?

- be able to get a classobjectrepresentations (ala ClassFile in javassist) from an existing ClassInfo object

Why?
This is/shoule be impl detail.

- support scoped classpools

Yup.

Adrian, what would happen now if we used Javassist impl of TypeInfo?
Probably CtClass not found for some non 'default' classes?

[adrian.brock]

"alesj" wrote:

Adrian, what would happen now if we used Javassist impl of TypeInfo?
Probably CtClass not found for some non 'default' classes?

Its not complete.

* The generics stuff isn't done because javassist didn't support generics when I
originally wrote it.
* There's an issue with the method reflection for non-public methods/fields when not using
Sun's JDK.

[alesj]

Its not complete.

* The generics stuff isn't done because javassist didn't support generics when I
originally wrote it.
* There's an issue with the method reflection for non-public methods/fields when not using
Sun's JDK.

I meant more about wrt scoped classpools.

But I guess this shouldn't be a problem to add/impl,
as our current scoped classpool impl is simply a map of CL,Pool pairs
and all TypeInfoFactory::get methods have a means to get CL.

It's probably just the case of delaying class loading as late as possible
plus changing how we cache things with non-introspection TypeInfoFactory impls.
e.g. Class == CL + classname

[adrian.brock]

"alesj" wrote:

as we've stated in the aop design forum (and here) is that the current state of jboss-reflect is too limited for aop to use at all. a well designed MutableClassInfo can change that though.

What about if we just added 'mutable' wrapper/delegate.
e.g. you would actually create your own ClassInfo impl based on existing JavassistCI:
Extra stuff would be held in this mutable wrapper/delegate.
When invoked, first check extra stuff if it exists, else delegate.

No. It should evolve into a one-stop shop for reflection and byte-code
manipulation implemented on top of javassist (or any other byte code tool).
That's the purpose of the abstraction.

The original idea was that ClassInfo would be able to represent classes before
they are loaded. The code was originally ported by Bill from AOP.

Having additional functionality to add/change methods, etc.
should also be a part of this api.

That additional functionality obviously wouldn't work if you just chose to use
the basic introspection implementation as now which is "read-only".

But to make it properly useful it needs deeper integration with the classloading
structure, see below.

- the possibility to create a classobjectrepresentation from a byte[]/File/Url

What exactly do you mean here?
e.g. ClassInfo ci = factory.getClassInfo(byte[]/File/Url); ?

This just lets you create a ClassInfo from the bytecode.
But for this to work properly it needs to have a understanding as to which
classloader the class/bytecode should be assigned.

- be able to change/add/remove fields/methods/constructors/classes similiar to what javassist support

How do you do this in your prototype?
I guess a single place where you know the aop rules would be enough?
e.g. AOP impl of TypeInfoFactory overridding instantiate method

See above. It just needs a new interface to say the Javassist*Info
implements the Mutable behaviour.

- do finegrained operations/checks ala annotation visibility (which is only possible with javassist and not java.lang.reflect afaik)

Annotation visibility?

Not sure why you need this. I assume you mean being able to look
at annotations that are not Runtime visible? Is that really a good idea?

- be able to get a classobjectrepresentations (ala ClassFile in javassist) from an existing ClassInfo object

Why?
This is/shoule be impl detail.

Unless you mean being able to get the resulting byte code from the manipulation
so you can defineClass() on it I don't think it is a good idea.

Allowing access to implementation details is not a good idea,
e.g. the deprecated TypeInfo.getClass() should not be there.

Instead we need to extend the abstraction to make this unnecessary.
This could include some helper methods to defineClass() from a ClassInfo.

- support scoped classpools

Yup.

Nope/Yup. The Scoped classpools are wrong. They don't understand the classloading
rules properly - except in JBoss4, although they are better than what we have now.
See the discussions in the aop forum I had repeatedly last year with Kabir.

SUMMARY

Besides completing the generics stuff in the javassist class info
so we can use it, the TypeInfo needs additional "optional" interfaces for mutable
behaviour.

But the real work is making aop/classinfo properly
integrate with the classloading structure.

Additionally as Ales and I discussed in Slovenia, the MDR introspection stuff
(Class annotations) and a number of other places
should also be using the ClassInfo stuff.

Especially when it comes to the more efficient method/field reflection stuff I wrote
in the JavassistClassInfo.

[adrian.brock]

"alesj" wrote:

But I guess this shouldn't be a problem to add/impl,
as our current scoped classpool impl is simply a map of CL,Pool pairs
and all TypeInfoFactory::get methods have a means to get CL.

The scoped classpool isn't that though. It works at the "domain" level
assuming all peer classloaders are visible to each other and that they all
export everything.

It's probably just the case of delaying class loading as late as possible
plus changing how we cache things with non-introspection TypeInfoFactory impls.
e.g. Class == CL + classname

If everybody used the ClassInfo stuff there should be no real classloading at all
until it is really needed, i.e. objects constructed.
And all the reflection would be optimized (faster and stored only once)
using the javassist implementation.

[stalep]

This just lets you create a ClassInfo from the bytecode.
But for this to work properly it needs to have a understanding as to which
classloader the class/bytecode should be assigned.

yes, well yes and no. when creating a CtClass object yes, when creating a ClassFile object then no. as mentioned ClassFile is a impl detail, but we need the functionality it brings. - but if we can implement what we currently need from ClassFile to *ClassInfo* then no.
eg, atm we can:

ClassFile cf = new ClassFile( new DataInputStream(new FileInputStream(file)));

- do finegrained operations/checks ala annotation visibility (which is only possible with javassist and not java.lang.reflect afaik)

Not sure why you need this. I assume you mean being able to look
at annotations that are not Runtime visible? Is that really a good idea?

we use it a lot in aop so yes, we need it.

Unless you mean being able to get the resulting byte code from the manipulation
so you can defineClass() on it I don't think it is a good idea.

Allowing access to implementation details is not a good idea,
e.g. the deprecated TypeInfo.getClass() should not be there.

Instead we need to extend the abstraction to make this unnecessary.
This could include some helper methods to defineClass() from a ClassInfo.

i was not thinking of getting the .class. i was thinking of something like CtClass.getClassFile() since atm that is the only way we can get info like annotation visibility, but if we can implement to get that from TypeInfo/etc its not needed.

for now we havent added any methods to add/change methods/fields/etc to ClassInfo, we just changed the factory to not use the .class representation when creating the ClassInfo object to use the CtClass instead. - which i guess it was meant to anyway if im understanding adrian correct.

[kabirkhan]

Nope/Yup. The Scoped classpools are wrong. They don't understand the classloading

The new classpools I am working on will

[kabirkhan]

SUMMARY

Besides completing the generics stuff in the javassist class info
so we can use it, the TypeInfo needs additional "optional" interfaces for mutable
behaviour.

Cool, that is exactly what I had in mind when originally talking to Ales about this.

But the real work is making aop/classinfo properly
integrate with the classloading structure.

This is WIP, in AOP I am reimplementing the classpools
http://www.jboss.com/index.html?module=bb&op=viewtopic&p=4201653#4201653
I have hacked around the missing features that will be introduced by JBCL-78 for now, once jboss-cl 2.0.2 is released I'm good. I am more or less done with this, but need to add logging and to test this out in AS.

For ClassInfo, the underlying ClassPool that is used by XXXInfo will need to be the same as the ClassPool that is used by AOP, so some centralized repository of ClassPools will probably be needed. For AOP this is done by https://svn.jboss.org/repos/jbossas/projects/aop/trunk/aop/src/main/java/org/jboss/aop/classpool/AOPClassPoolRepository.java, but maybe this should be moved into jboss-reflect?

[kabirkhan]

- do finegrained operations/checks ala annotation visibility (which is only possible with javassist and not java.lang.reflect afaik)

Annotation visibility?

Not sure why you need this. I assume you mean being able to look
at annotations that are not Runtime visible? Is that really a good idea?

When I took over the project it supported pointcut matching on both Runtime visible and invisible annotations out of the box. Checking the invisible ones proved to be a pretty big performance hit, so we disabled the checking of those by default. In case this screwed up things for our users we added the ability to turn on checking of chosen invisible annotations by specifying

-Djboss.aop.invisible.annotations=org.blah.Annotation1,org.blah.Annotation2

or

-Djboss.aop.invisible.annotations=*

AFAIK nobody has complained about this/asked how to turn it on, although it is mentioned in the reference manual. So, either we remove support for that in 2.1.0, or have an alternative mechanism to provide for that

[stalep]

is it ok if i do some changes to jboss-reflect (mostly thinking of JavassistTypeInfo)?
i wont change any of the interfaces untill we've discussed further, but without changing JavassistTypeInfo i cant get anywhere with the prototype in aop.

[alesj]

Fine with me,
as long as you follow the directions set on this forum' posts discussion,
which don't seem to be a light change/impl.

As I don't wanna diverse into temp impl vs. real/end impl vs. snapshot.

As I talked to Kabir, I wanted to leave this open for discussion,
as about who's gonna do what/how/when, at the eventual f2f meeting.

[stalep]

ok, after some days trying different approaches for the mutable design here is some of the first ideas (ive removed the javadoc to reduce the size of the code):

public interface MutableClassInfo extends ClassInfo
{

 MutableMethodInfo createMutableMethod(Body body);

 MutableMethodInfo createMutableMethod(ModifierInfo modifier, String name,
 String[] parameters, String[] exceptions);

 MutableMethodInfo createMutableMethod(ModifierInfo modifier, String name, Body body,
 String[] parameters, String[] exceptions);

 MutableMethodInfo createMutableMethod(ModifierInfo modifier, String name,
 MutableClassInfo[] parameters, MutableClassInfo[] exceptions);

 MutableMethodInfo createMutableMethod(ModifierInfo modifier, String name, Body body,
 MutableClassInfo[] parameters, MutableClassInfo[] exceptions);


 MutableConstructorInfo createMutableConstructor(Body body);

 MutableConstructorInfo createMutableConstructor(ModifierInfo modifier, String[] parameters,
 String[] exceptions);

 MutableConstructorInfo createMutableConstructor(ModifierInfo modifier, Body body,
 String[] parameters, String[] exceptions);

 void addMethod(MutableMethodInfo mmi);

 void removeMethod(MutableMethodInfo mmi);

 void addConstructor(MutableConstructorInfo mci);

 void removeConstructor(MutableConstructorInfo mci);

}

public interface MutableMethodInfo extends MethodInfo
{

 void setModifier(ModifierInfo mi);

 void setReturnType(String returnType);

 void setReturnType(MutableClassInfo returnType);

 void setName(String name);

 void setBody(Body body);

 void setParameters(String[] parameters);

 void setParameters(MutableClassInfo[] parameters);

 void setExceptions(String[] exceptions);

 void setExceptions(MutableClassInfo[] exceptions);

 /**
 * TODO: something similar to CtBehavior.instrument(...)
 *
 * @param mmc
 */
 void executeCommand(MutableMethodInfoCommand mmc);
}

public interface MutableConstructorInfo extends ConstructorInfo
{

 void setModifier(ModifierInfo mi);

 void setBody(Body body);

 void setParameters(String[] parameters);

 void setParameters(MutableClassInfo[] parameters);

 void setExceptions(String[] exceptions);

 void setExceptions(MutableClassInfo[] exceptions);

}

public interface Body
{
 String getBody();
}
// There are three implementations of AbstractJavassistBody
// Default, InsertBefore, InsertAfter (to reflect javassist api)
public abstract class AbstractJavassistBody implements Body
{
 String body;

 AbstractJavassistBody(String body)
 {
 this.body = body;
 }
 public String getBody()
 {
 return body;
 }

 abstract void createBody(CtBehavior behaviour);
}

The idea is that JavassistTypeInfo now will implement MutableClassInfo instead of ClassInfo. The usage of the Class parameter needs to be removed, internal methods using it will just use the CtClass object. The question is the method getType(), can we just remove it altogether? -its been marked as deprecated.

JavassistMethodInfo will implement MutableMethodInfo and JavassistConstructorInfo will implement MutableConstructorInfo the same way as JTI. Is there a reason why the JMI constructor is public? if not it should be set to package protected.

We need functionality to get the method signature from a method. This could be implemented in a utilclass, but i would rather see it added to the MethodInfo interface.
We might also need a method like ClassInfo.getClassLoader(), but i guess Kabir/Adrian know more about that.

I havent listed up any exceptions yet. There are a few exceptions, but they all extend RuntimeException so they are not listed. I have no big objections of having them extend Exception instead...

Design for fields will be posted later, but they will follow the same idea.
Javassist have a lot of functionality to create new methods/constructors/etc that i dont think we need to support directly in MCI, but rather create some util classes to do that. Im thinking of classes/methods like: CtNewMethod.copy/abstractMethod/getter/setter/delegator/etc.