1 2 Previous Next 27 Replies Latest reply on Apr 30, 2010 10:14 AM by kabirkhan

    JBREFLECT-5 - Implementing generics in JavassistClassInfo

    kabirkhan

      I have had a first stab at implementing generics in the Javassist version of ClassInfo and committed it against http://jira.jboss.org/jira/browse/JBREFLECT-5.

       

      I have created Javassist versions of the tests that were not there before: JavassistGenericClassUnitTestCase and JavassistGenericInterfaceUnitTestCase. They are just subclasses of the abstract base classes used by the Introspection versions of these tests. They pass, but I don't think that they are good enough for the following reasons:

       

      1) They only test plain generics such as Collection<String>, while more complex things such as Collection<Map<String, List<V>>> are not tested.

       

      2) Wildcards and bounds are not tested, but I am not sure if we should support those or not?

       

      3) When accessing generics from the return type of a method, e.g.

       

           Collection<String> getStringCollection(){}
      
      

      we get hold of this method using normal reflection (A) and construct the TypeInfo for that using the ParameterizedType (B1+2) for which I extended the Javassist TypeInfo factory:

         private void assertComponentType(String methodName, Class<?> expected) throws Exception
         {
            Method method = ClassInfoGenericClassTest.class.getMethod(methodName, (Class[]) null); //A
            Type type = method.getGenericReturnType(); //B1
            assertComponentType(type, expected);
         }
      
         private void assertComponentType(Type type, Class<?> expected) throws Exception
         {
            TypeInfoFactory factory = getTypeInfoFactory();
            TypeInfo typeInfo = factory.getTypeInfo(type); //B2
            ClassInfo classInfo = assertInstanceOf(typeInfo, ClassInfo.class);
            assertTrue(classInfo.isCollection());
            TypeInfo expectedInfo = factory.getTypeInfo(expected);
            assertEquals(expectedInfo, classInfo.getComponentType());
         }
      
      However, that is probably not what will happen in real life, we would have a ClassInfo, get hold of the MethodInfo in question, and then get the return type which should contain the actualTypeArguments. This needs implementing and is not being tested at the moment, and the same goes for parameters and field types.
      4) Flavia mentioned that for the component type tests we might have something like
      interface WeirdMap<V, K> implements WeirdMap<K, V>
       
      
      This is kind of tested but needs fleshing out more
      What I have do so far satisfies what the tests require, but in case somebody looks at this while I look at the aop failures in AS trunk, feel free to rework and change what I have done, it is by no means the final solution although I think it proves that the stuff Chiba did for the Signature attribute works well and we can figure out things from that.
      A few points worth remembering:
      a) I have implemented a JavassistParameterizedClassInfo (stolen from the introspection implementation) which handles things like Collection<String>. This basically delegates to the "raw" ClassInfo for Collection, augmented with extra information regarding the <String> bit. These are currently not cached and are created on every access.
      b) JavassistClassInfo should cache the values of isMap() and isCollection()
        • 1. Re: JBREFLECT-5 - Implementing generics in JavassistClassInfo
          alesj

          1) It should be easy to add some tests for this, right?

          Perhaps we use some on-the-fly generics creation (which Javassist has, afaik), and test this for some reasonable depth.

           

          2) Some simple tests would be nice.

           

          3) How do you get the generic Type then?

           

          4) What are the issues / pitfalls here?

           

          Like I said, while implementing this, other depending projects are the best test env.

          e.g. Kernel, CL, Deployers and of course the whole AS

          * -Dorg.jboss.reflect.spi.TypeInfoFactory=org.jboss.reflect.plugins.javassist.JavassistTypeInfoFactory

          • 2. Re: JBREFLECT-5 - Implementing generics in JavassistClassInfo
            kabirkhan

            alesj wrote:

             

            1) It should be easy to add some tests for this, right?

            Perhaps we use some on-the-fly generics creation (which Javassist has, afaik), and test this for some reasonable depth.

            Yes

            alesj wrote:

            3) How do you get the generic Type then?


            Not sure I understand the question. I've just worked with what was provided, but we need some tests testing the scenario I outlined. We would need to check the Signature attribute of the method and then construct the parameterized ClassInfo as was done for getGenericSuperClass().

             

            alesj wrote:

             

            4) What are the issues / pitfalls here?

            The main thing is knowing the index of the type parameter we are looking for in the parent class, then getting the type argument using that index used when extending that class and matching that up with the type parameters of the child class. This needs doing for the whole hierarchy. This has been done, but needs some better tests

            alesj wrote:

             

            Like I said, while implementing this, other depending projects are the best test env.

            e.g. Kernel, CL, Deployers and of course the whole AS

            * -Dorg.jboss.reflect.spi.TypeInfoFactory=org.jboss.reflect.plugins.javassist.JavassistTypeInfoFactory

            I'll try that

            • 3. Re: JBREFLECT-5 - Implementing generics in JavassistClassInfo
              kabirkhan

              kabir.khan@jboss.com wrote:

               

               

              3) When accessing generics from the return type of a method, e.g.

               

                   Collection<String> getStringCollection(){}
               
              

              we get hold of this method using normal reflection (A) and construct the TypeInfo for that using the ParameterizedType (B1+2) for which I extended the Javassist TypeInfo factory:

                 private void assertComponentType(String methodName, Class<?> expected) throws Exception
                 {
                    Method method = ClassInfoGenericClassTest.class.getMethod(methodName, (Class[]) null); //A
                    Type type = method.getGenericReturnType(); //B1
                    assertComponentType(type, expected);
                 }
               
                 private void assertComponentType(Type type, Class<?> expected) throws Exception
                 {
                    TypeInfoFactory factory = getTypeInfoFactory();
                    TypeInfo typeInfo = factory.getTypeInfo(type); //B2
                    ClassInfo classInfo = assertInstanceOf(typeInfo, ClassInfo.class);
                    assertTrue(classInfo.isCollection());
               
                    TypeInfo expectedInfo = factory.getTypeInfo(expected);
                    assertEquals(expectedInfo, classInfo.getComponentType());
                 }
              
              However, that is probably not what will happen in real life, we would have a ClassInfo, get hold of the MethodInfo in question, and then get the return type which should contain the actualTypeArguments. This needs implementing and is not being tested at the moment, and the same goes for parameters and field types.

              Looking properly, I think what is there at the moment is correct. In reflect we have these additional methods:

               

              Type[] Method.getGenericExceptionTypes()

              Type[] Constructor.getGenericExceptionTypes()

              Type[] Method.getGenericParameterTypes()

              Type[] Constructor.getGenericParameterTypes()

              Type Method.getGenericReturnType()

              Type Field.getGenericType()

              The normal non-generic methods (as getReturnType()) simply return a Class. So, I think that MethodInfo.getReturnType() should just return a plain TypeInfo as it does at the moment.
              I'll concentrate on 1,2 and 4 for now, and see if 3 is needed, although I don't think so since it is not present in the reflect implementation either.
              • 4. Re: JBREFLECT-5 - Implementing generics in JavassistClassInfo
                kabirkhan
                1) and 4) have been done.
                A few points worth remembering:
                a) I have implemented a JavassistParameterizedClassInfo (stolen from the introspection implementation) which handles things like Collection<String>. This basically delegates to the "raw" ClassInfo for Collection, augmented with extra information regarding the <String> bit. These are currently not cached and are created on every access.

                I have not added caching yet, and adding this simple test passes with the introspection implementation, but fails with the Javassist implementation

                 

                   public Comparable<String> signatureCachedParameterizedClassInfo()
                   {
                      return null;
                   }
                   
                   public void testCachedParameterizedClassInfo() throws Throwable
                   {
                      Type type = getGenericReturnType("signatureCachedParameterizedClassInfo");
                      TypeInfo typeInfo1 = getTypeInfoFactory().getTypeInfo(type);
                      TypeInfo typeInfo2 = getTypeInfoFactory().getTypeInfo(getGenericReturnType("signatureCachedParameterizedClassInfo"));
                      assertEquals(typeInfo1, typeInfo2);
                      assertSame(typeInfo1, typeInfo2);  // <-- FAILS
                
                      //Also check the results of repeated calls to getGenericSuperClass/-Interfaces() etc.
                   }
                
                 
                

                 

                Is this object equality a requirement? My fear is that working out the key will be costly

                 

                A few points worth remembering:
                b) JavassistClassInfo should cache the values of isMap() and isCollection()

                This has been done.

                • 5. Re: JBREFLECT-5 - Implementing generics in JavassistClassInfo
                  kabirkhan

                  kabir.khan@jboss.com wrote:

                   

                  2) Wildcards and bounds are not tested, but I am not sure if we should support those or not?

                   

                  Note to self: When I get round to this, I should check both interface and class bounds

                  • 6. Re: JBREFLECT-5 - Implementing generics in JavassistClassInfo
                    alesj

                    I have not added caching yet, and adding this simple test passes with the introspection implementation, but fails with the Javassist implementation

                     

                       public Comparable<String> signatureCachedParameterizedClassInfo()
                       {
                          return null;
                       }
                       
                       public void testCachedParameterizedClassInfo() throws Throwable
                       {
                          Type type = getGenericReturnType("signatureCachedParameterizedClassInfo");
                          TypeInfo typeInfo1 = getTypeInfoFactory().getTypeInfo(type);
                          TypeInfo typeInfo2 = getTypeInfoFactory().getTypeInfo(getGenericReturnType("signatureCachedParameterizedClassInfo"));
                          assertEquals(typeInfo1, typeInfo2);
                          assertSame(typeInfo1, typeInfo2);  // <-- FAILS
                     
                          //Also check the results of repeated calls to getGenericSuperClass/-Interfaces() etc.
                       }

                     

                    Is this object equality a requirement? My fear is that working out the key will be costly

                    My first answer is yes, but you can convince we otherwise. ;-)

                     

                    As all of the stuff in Reflect works this way, why is this here a problem?

                    • 7. Re: JBREFLECT-5 - Implementing generics in JavassistClassInfo
                      kabirkhan

                      alesj wrote:

                       

                      As all of the stuff in Reflect works this way, why is this here a problem?

                      Not really a problem, I just wanted to check if the object equality is required. It seems to work that way normally, so I'll do that for this instead as well.

                      • 8. Re: JBREFLECT-5 - Implementing generics in JavassistClassInfo
                        kabirkhan

                        kabir.khan@jboss.com wrote:

                         

                        2) Wildcards and bounds are not tested, but I am not sure if we should support those or not?


                        It looks like they should be supported, since the following quick test passes with the Introspection implementation, but needs some work in the Javassist implementation:

                         

                           public static Collection<?> signatureCollectionUnboundedWildcard() 
                           {
                              return null;
                           }
                           public void testComponentTypeCollectionUnboundedWildcard() throws Throwable
                           {
                              assertComponentType("signatureCollectionUnboundedWildcard", Object.class);
                           }
                        

                         

                        I'll write some proper tests for this and implement in Javassist

                        • 9. Re: JBREFLECT-5 - Implementing generics in JavassistClassInfo
                          kabirkhan

                          One thing I have been doing which might be unnecessary is to wrap things with wildcards with a delegating class, e.g.:

                           

                          /**
                           * Type info for a wildcard type
                           * 
                           * @author <a href="kabir.khan@jboss.com">Kabir Khan</a>
                           * @version $Revision: 1.1 $
                           */
                          public class WildcardTypeInfo extends DelegateClassInfo
                          {
                             private static final long serialVersionUID = 1L;
                          
                             private final String name;
                          
                             public WildcardTypeInfo(String name, ClassInfo delegate)
                             {
                                super(delegate);
                                this.name = name;
                             }
                          
                             @Override
                             public void toShortString(JBossStringBuilder buffer)
                             {
                                buffer.append(name);
                             }
                          
                             @Override
                             protected void toString(JBossStringBuilder buffer)
                             {
                                buffer.append("name=").append(name);
                             }
                          }
                          
                          

                           

                          So if you try to get the typeInfo for Class<? extends some.Thing> we end up with a ParameterizedClassInfo for Class which contains one type argument. This type argument is for some.Thing. The original introspection implemetation would just use that directly. What I am doing now is to

                          get the TypeInfo for some.Thing (1) and decorate that with a WildcardTypeInfo that delegates to 1, but has the name '? extends some.Thing' which is ONLY used in these two cases:

                          • invoking toString().
                          • looking up the WildcardTypeInfo in the cache
                          This has been done for both implementations.

                           

                          The reason I am unsure if this is needed after all is that I am currently implementing TypeVariables, e.g.:

                           

                          public <T extends some.Thing> T signatureMethod()
                          

                          If I were to wrap it, I would end up with 1 and wrapping that with a TypeVariableTypeInfo which overrides toString with 'T extends some.Thing'. The problem is that if we also have

                           

                          public <U extends some.Thing> T signatureMethod2()
                          public <V extends some.Thing> T signatureMethod3()
                          
                          
                          We end up with 3 different cached versions with a different name used for toString(), which all delegate to 1.
                          The only reason I did this was to have nice output when calling toString(), so I am wondering if that is really necessary?
                          • 10. Re: JBREFLECT-5 - Implementing generics in JavassistClassInfo
                            kabirkhan

                            In case I was not clear, if I have

                             

                            <T extends some.Thing> sig1()

                            <U extends some.Thing> sig2()

                            <V extends some.Thing> sig3()

                            Do I
                            1) Use some.Thing's plain TypeInfo
                            2) Wrap some.Thing's plain TypeInfo with something whose only function really is "pretty-printing", of these
                            a) Use a separate wrapper for T, U, V etc.
                            b) Use the wildcard wrapper for them, so we end up with '? extends some.Thing' no matter if we are printing T, U or V
                            2b kind of seems pointless, and 2a too fine-grained, so maybe I should just go with 1?
                            • 11. Re: JBREFLECT-5 - Implementing generics in JavassistClassInfo
                              alesj

                              Do I
                              1) Use some.Thing's plain TypeInfo
                              2) Wrap some.Thing's plain TypeInfo with something whose only function really is "pretty-printing", of these
                              a) Use a separate wrapper for T, U, V etc.
                              b) Use the wildcard wrapper for them, so we end up with '? extends some.Thing' no matter if we are printing T, U or V
                              2b kind of seems pointless, and 2a too fine-grained, so maybe I should just go with 1?

                              1 seems the only logical choice.

                               

                              Is this only for Javassist impl -- hence how is this handled in Introspection,

                              or is this missing in both impls?

                              • 12. Re: JBREFLECT-5 - Implementing generics in JavassistClassInfo
                                kabirkhan
                                1 was how it was being done in introspection, but then I "improved it" in both implementations since it was not really tested properly. I will make both behave as 1.
                                • 13. Re: JBREFLECT-5 - Implementing generics in JavassistClassInfo
                                  kabirkhan

                                  I am trying out the javassist implementation in kernel now. The first stumbling block is this

                                   

                                  java.lang.RuntimeException: Unable to create a KernelInitializer based on the specified KernelConfig

                                  at org.jboss.kernel.KernelFactory.createKernelInitializer(KernelFactory.java:156)

                                  at org.jboss.kernel.KernelFactory.assembleNewKernel(KernelFactory.java:99)

                                  at org.jboss.kernel.KernelFactory.newInstance(KernelFactory.java:67)

                                  at org.jboss.kernel.plugins.bootstrap.AbstractBootstrap.bootstrap(AbstractBootstrap.java:114)

                                  at org.jboss.kernel.plugins.bootstrap.AbstractBootstrap.run(AbstractBootstrap.java:83)

                                  at org.jboss.test.kernel.bootstrap.test.BootstrapTestCase.testBasicBootstrap(BootstrapTestCase.java:76)

                                  at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)

                                  at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)

                                  at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)

                                  at java.lang.reflect.Method.invoke(Method.java:597)

                                  at junit.framework.TestCase.runTest(TestCase.java:168)

                                  at junit.framework.TestCase.runBare(TestCase.java:134)

                                  at junit.framework.TestResult$1.protect(TestResult.java:110)

                                  at junit.framework.TestResult.runProtected(TestResult.java:128)

                                  at junit.framework.TestResult.run(TestResult.java:113)

                                  at junit.framework.TestCase.run(TestCase.java:124)

                                  at junit.framework.TestSuite.runTest(TestSuite.java:232)

                                  at junit.framework.TestSuite.run(TestSuite.java:227)

                                  at junit.extensions.TestDecorator.basicRun(TestDecorator.java:24)

                                  at junit.extensions.TestSetup$1.protect(TestSetup.java:23)

                                  at junit.framework.TestResult.runProtected(TestResult.java:128)

                                  at junit.extensions.TestSetup.run(TestSetup.java:27)

                                  at org.junit.internal.runners.JUnit38ClassRunner.run(JUnit38ClassRunner.java:81)

                                  at org.eclipse.jdt.internal.junit4.runner.JUnit4TestReference.run(JUnit4TestReference.java:46)

                                  at org.eclipse.jdt.internal.junit.runner.TestExecution.run(TestExecution.java:38)

                                  at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:467)

                                  at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.runTests(RemoteTestRunner.java:683)

                                  at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.run(RemoteTestRunner.java:390)

                                  at org.eclipse.jdt.internal.junit.runner.RemoteTestRunner.main(RemoteTestRunner.java:197)

                                  Caused by: java.security.AccessControlException: access denied (java.lang.RuntimePermission getClassLoader)

                                  at java.security.AccessControlContext.checkPermission(AccessControlContext.java:323)

                                  at java.security.AccessController.checkPermission(AccessController.java:546)

                                  at java.lang.SecurityManager.checkPermission(SecurityManager.java:532)

                                  at org.jboss.reflect.plugins.javassist.JavassistTypeInfo.getClassLoader(JavassistTypeInfo.java:211)

                                  at org.jboss.classadapter.plugins.BasicClassAdapter.getClassLoader(BasicClassAdapter.java:70)

                                  at org.jboss.beans.info.plugins.AbstractBeanInfoFactory.getBeanInfo(AbstractBeanInfoFactory.java:136)

                                  at org.jboss.beans.info.plugins.AbstractBeanInfoFactory.getBeanInfo(AbstractBeanInfoFactory.java:124)

                                  at org.jboss.config.plugins.AbstractConfiguration.getBeanInfo(AbstractConfiguration.java:69)

                                  at org.jboss.kernel.plugins.config.AbstractKernelConfig.getBeanInfo(AbstractKernelConfig.java:65)

                                  at org.jboss.kernel.plugins.config.property.PropertyKernelConfig.getImplementation(PropertyKernelConfig.java:184)

                                  at org.jboss.kernel.plugins.config.property.PropertyKernelConfig.createKernelInitializer(PropertyKernelConfig.java:121)

                                  at org.jboss.kernel.KernelFactory.createKernelInitializer(KernelFactory.java:150)

                                  ... 28 more

                                   

                                  JavassistTypeInfo:

                                   

                                     /** The get classloader permission */
                                     private static final RuntimePermission GET_CLASSLOADER_PERMISSION = new RuntimePermission("getClassLoader");
                                  ..
                                     public ClassLoader getClassLoader()
                                     {
                                        // looks like Javassist ClassPool::getClassLoader
                                        // doesn't check for security, so we should
                                        SecurityManager sm = System.getSecurityManager();
                                        if (sm != null)
                                           sm.checkPermission(GET_CLASSLOADER_PERMISSION);
                                        return JavassistUtil.getClassLoader(ctClass);
                                     }
                                   
                                  

                                   

                                  In the introspection implementation this simply delegates to Class.getClassLoader()

                                   

                                      public ClassLoader getClassLoader() {
                                          ClassLoader cl = getClassLoader0();
                                          if (cl == null)
                                              return null;
                                          SecurityManager sm = System.getSecurityManager();
                                          if (sm != null) {
                                              ClassLoader ccl = ClassLoader.getCallerClassLoader();
                                              if (ccl != null && ccl != cl && !cl.isAncestor(ccl)) {
                                                  sm.checkPermission(SecurityConstants.GET_CLASSLOADER_PERMISSION);
                                              }
                                          }
                                          return cl;
                                      }
                                  

                                   

                                  getClassLoader() gets called quite a bit, and I am unsure how best to handle this?

                                  • 14. Re: JBREFLECT-5 - Implementing generics in JavassistClassInfo
                                    alesj

                                    getClassLoader() gets called quite a bit, and I am unsure how best to handle this?

                                    What about if we simply mock what Class::getClassLoader is doing, but based on Javassist API?

                                    1 2 Previous Next