groovy
  1. groovy
  2. GROOVY-3971

adding method to class in ASTBuilder not working

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.7-rc-2
    • Fix Version/s: 2.4.0-beta-1
    • Component/s: ast builder
    • Labels:
      None
    • Environment:
      Groovy 1.7-RC-2 on jre1.6.0_04 on Windows Vista SP2
    • Number of attachments :
      1

      Description

      The following code produces class MyClass OK, but running the class produces error:
      Exception in thread "main" java.lang.NoSuchMethodError: main

      Here's the ASTBuilder code I used:

      import static org.objectweb.asm.Opcodes.ACC_PUBLIC
      import static org.objectweb.asm.Opcodes.ACC_STATIC
      import org.codehaus.groovy.ast.builder.AstBuilder
      import org.codehaus.groovy.control.CompilerConfiguration
      import org.codehaus.groovy.control.CompilationUnit
      
      def classes= new AstBuilder().buildFromSpec{
        classNode 'MyClass', ACC_PUBLIC, {
          classNode Object //superclass
          interfaces{
            classNode GroovyObject
          }
          mixins{}
          genericsTypes{}
          method('main', ACC_PUBLIC & ACC_STATIC, Void.class) {
            parameters{
              parameter 'args': String[].class
            }
            exceptions{}
            block{
              methodCall {
                variable "this"
                constant "println"
                argumentList {
                  constant "Hello, world!"
                }
              }
            }
          }
        }
      }
      
      def gcl= new GroovyClassLoader()
      def sec= null
      def ccfg= CompilerConfiguration.DEFAULT
      def cu= new CompilationUnit(ccfg, sec, gcl)
      cu.addClassNode(classes[0])
      cu.compile() 
      

      Also tried this but same result:

      import static org.objectweb.asm.Opcodes.ACC_PUBLIC
      import static org.objectweb.asm.Opcodes.ACC_STATIC
      import org.codehaus.groovy.ast.*
      import java.security.CodeSource
      import org.codehaus.groovy.ast.builder.AstBuilder
      import org.codehaus.groovy.control.CompilerConfiguration
      import org.codehaus.groovy.control.CompilationUnit
      import org.codehaus.groovy.control.CompilePhase
      
      def classes= new AstBuilder().buildFromString(CompilePhase.CONVERSION, """\
      public class MyClass{
        public static void main(String[] args){
          println "Hello, world!"
        }
      }
      """)
      
      def gcl= new GroovyClassLoader()
      def sec= null //new CodeSource()
      def ccfg= CompilerConfiguration.DEFAULT
      def cu= new CompilationUnit(ccfg, sec, gcl)
      cu.addClassNode(classes[1])
      cu.compile()
      

        Activity

        Hide
        Gavin Grover added a comment -

        javap output of class:

        public class MyClass extends java.lang.Object implements groovy.lang.GroovyObject{
            public static java.lang.Long __timeStamp;
            public static java.lang.Long __timeStamp__239_neverHappen1262576568013;
            public MyClass();
            public java.lang.Object this$dist$invoke$2(java.lang.String, java.lang.Object);
            public void this$dist$set$2(java.lang.String, java.lang.Object);
            public java.lang.Object this$dist$get$2(java.lang.String);
            protected groovy.lang.MetaClass $getStaticMetaClass();
            public groovy.lang.MetaClass getMetaClass();
            public void setMetaClass(groovy.lang.MetaClass);
            public java.lang.Object invokeMethod(java.lang.String, java.lang.Object);
            public java.lang.Object getProperty(java.lang.String);
            public void setProperty(java.lang.String, java.lang.Object);
            static {};
            public void super$1$wait();
            public java.lang.String super$1$toString();
            public void super$1$wait(long);
            public void super$1$wait(long, int);
            public void super$1$notify();
            public void super$1$notifyAll();
            public java.lang.Class super$1$getClass();
            public boolean super$1$equals(java.lang.Object);
            public java.lang.Object super$1$clone();
            public int super$1$hashCode();
            public void super$1$finalize();
            static java.lang.Class class$(java.lang.String);
        }
        
        Show
        Gavin Grover added a comment - javap output of class: public class MyClass extends java.lang.Object implements groovy.lang.GroovyObject{ public static java.lang.Long __timeStamp; public static java.lang.Long __timeStamp__239_neverHappen1262576568013; public MyClass(); public java.lang.Object this$dist$invoke$2(java.lang.String, java.lang.Object); public void this$dist$set$2(java.lang.String, java.lang.Object); public java.lang.Object this$dist$get$2(java.lang.String); protected groovy.lang.MetaClass $getStaticMetaClass(); public groovy.lang.MetaClass getMetaClass(); public void setMetaClass(groovy.lang.MetaClass); public java.lang.Object invokeMethod(java.lang.String, java.lang.Object); public java.lang.Object getProperty(java.lang.String); public void setProperty(java.lang.String, java.lang.Object); static {}; public void super$1$wait(); public java.lang.String super$1$toString(); public void super$1$wait(long); public void super$1$wait(long, int); public void super$1$notify(); public void super$1$notifyAll(); public java.lang.Class super$1$getClass(); public boolean super$1$equals(java.lang.Object); public java.lang.Object super$1$clone(); public int super$1$hashCode(); public void super$1$finalize(); static java.lang.Class class$(java.lang.String); }
        Hide
        Roshan Dawrani added a comment -

        For the 2nd example (AstBuilder#buildFromString()), here is the working code:

        import static org.objectweb.asm.Opcodes.ACC_PUBLIC
        import static org.objectweb.asm.Opcodes.ACC_STATIC
        import org.codehaus.groovy.ast.*
        import java.security.CodeSource
        import org.codehaus.groovy.ast.builder.AstBuilder
        import org.codehaus.groovy.control.CompilerConfiguration
        import org.codehaus.groovy.control.CompilationUnit
        import org.codehaus.groovy.control.CompilePhase
        
        def classes = new AstBuilder().buildFromString(CompilePhase.CONVERSION, """\
        public class MyClass{
          public static void main(String[] args){
            println "Roshan here!"
          }
        }
        """)
        def su = classes[1].module.context
        def gcl = new GroovyClassLoader()
        def sec = null //new CodeSource()
        def ccfg = CompilerConfiguration.DEFAULT
        def cu = new CompilationUnit(ccfg, sec, gcl)
        cu.addClassNode(classes[1])
        cu.addSource(su)
        cu.compile()
        
        Show
        Roshan Dawrani added a comment - For the 2nd example (AstBuilder#buildFromString()), here is the working code: import static org.objectweb.asm.Opcodes.ACC_PUBLIC import static org.objectweb.asm.Opcodes.ACC_STATIC import org.codehaus.groovy.ast.* import java.security.CodeSource import org.codehaus.groovy.ast.builder.AstBuilder import org.codehaus.groovy.control.CompilerConfiguration import org.codehaus.groovy.control.CompilationUnit import org.codehaus.groovy.control.CompilePhase def classes = new AstBuilder().buildFromString(CompilePhase.CONVERSION, """\ public class MyClass{ public static void main( String [] args){ println "Roshan here!" } } """) def su = classes[1].module.context def gcl = new GroovyClassLoader() def sec = null // new CodeSource() def ccfg = CompilerConfiguration.DEFAULT def cu = new CompilationUnit(ccfg, sec, gcl) cu.addClassNode(classes[1]) cu.addSource(su) cu.compile()
        Hide
        Roshan Dawrani added a comment -

        Regarding 1st example, the MethodNode (main) is not even getting added to ClassNode (MyClass)'s methods, so no wonder it is not coming in compiled class and then not able to run.

        Show
        Roshan Dawrani added a comment - Regarding 1st example, the MethodNode (main) is not even getting added to ClassNode (MyClass)'s methods, so no wonder it is not coming in compiled class and then not able to run.
        Hide
        Paul King added a comment - - edited

        Slightly tweaked version of Roshan's example:

        import org.codehaus.groovy.ast.*
        import org.codehaus.groovy.ast.builder.AstBuilder
        import org.codehaus.groovy.control.CompilationUnit
        import static org.codehaus.groovy.control.CompilerConfiguration.*
        import static org.codehaus.groovy.control.CompilePhase.*
        
        def classes = new AstBuilder().buildFromString(CONVERSION, """\
        class MyClass {
          static main(String[] args){
            println "Roshan here!"
          }
        }
        """)
        def su = classes[1].module.context
        def gcl = new GroovyClassLoader()
        def cu = new CompilationUnit(DEFAULT, null, gcl)
        cu.addClassNode(classes[1])
        cu.addSource(su)
        cu.compile()
        MyClass.main()
        
        Show
        Paul King added a comment - - edited Slightly tweaked version of Roshan's example: import org.codehaus.groovy.ast.* import org.codehaus.groovy.ast.builder.AstBuilder import org.codehaus.groovy.control.CompilationUnit import static org.codehaus.groovy.control.CompilerConfiguration.* import static org.codehaus.groovy.control.CompilePhase.* def classes = new AstBuilder().buildFromString(CONVERSION, """\ class MyClass { static main( String [] args){ println "Roshan here!" } } """) def su = classes[1].module.context def gcl = new GroovyClassLoader() def cu = new CompilationUnit(DEFAULT, null , gcl) cu.addClassNode(classes[1]) cu.addSource(su) cu.compile() MyClass.main()
        Hide
        Roshan Dawrani added a comment -

        I should have mentioned here for documentation sake what was the change introduced for the 2nd example.

        The issue was that because there was no SourceUnit attached to the CompilationUnit, it was skipping all SourceUnitOperation(s) in the compilation phases - "resolve" being one such skipped operation.

        Because "resolve" was not happening, main method was remaining as "main(LString;)V" instead of "main(Ljava.lang.String;)V" and that's why it was not being recognized as the standard main method.

        Show
        Roshan Dawrani added a comment - I should have mentioned here for documentation sake what was the change introduced for the 2nd example. The issue was that because there was no SourceUnit attached to the CompilationUnit, it was skipping all SourceUnitOperation(s) in the compilation phases - "resolve" being one such skipped operation. Because "resolve" was not happening, main method was remaining as "main(LString;)V" instead of "main(Ljava.lang.String;)V" and that's why it was not being recognized as the standard main method.
        Hide
        Hamlet D'Arcy added a comment -

        I don't think there is any issue with the AST Builder. I wrote a Transformation called "Main" that adds main methods to classes and it works fine. There are two errors in the posted script:
        1: You must use ACC_PUBLIC | ACC_STATIC for the correct modifiers. The & is not correct.
        2: You must use Void.TYPE for the return type, Void.class is not the same.

        I would commit this patch to the Groovy Example directory... what do you think?

        Show
        Hamlet D'Arcy added a comment - I don't think there is any issue with the AST Builder. I wrote a Transformation called "Main" that adds main methods to classes and it works fine. There are two errors in the posted script: 1: You must use ACC_PUBLIC | ACC_STATIC for the correct modifiers. The & is not correct. 2: You must use Void.TYPE for the return type, Void.class is not the same. I would commit this patch to the Groovy Example directory... what do you think?
        Hide
        Roshan Dawrani added a comment -

        Hamlet, isn't the transformation solution different from what is asked for in the example 1 - where classNode and method both are defined in AstBuilder?

        It was noticed that methods defined under AstBuilder (in buildFromSpec) don't get added to the defining ClassNode. Since that has not been addressed, wanted to check if AstBuilder is not supposed to support that.

        Show
        Roshan Dawrani added a comment - Hamlet, isn't the transformation solution different from what is asked for in the example 1 - where classNode and method both are defined in AstBuilder? It was noticed that methods defined under AstBuilder (in buildFromSpec) don't get added to the defining ClassNode. Since that has not been addressed, wanted to check if AstBuilder is not supposed to support that.
        Hide
        Hamlet D'Arcy added a comment -

        Yes, this solution is slightly different because I wanted to make something a little more useful. However, it did work in my testing. The problems were in the pipe character, the return type, and all that CompilationUnit/SourceUnit stuff, not in the AST builder. Maybe I should retest it after work though.

        Show
        Hamlet D'Arcy added a comment - Yes, this solution is slightly different because I wanted to make something a little more useful. However, it did work in my testing. The problems were in the pipe character, the return type, and all that CompilationUnit/SourceUnit stuff, not in the AST builder. Maybe I should retest it after work though.
        Hide
        Roshan Dawrani added a comment -

        No, no, you missed my point. All other things you say are fine (modifier, return type, CompilationUnit/SourceUnit association, etc), except one.

        This example is different from the issue #1 raised here because it is also doing the work of adding the MethodNode to the ClassNode - isn't that something that AstBuilder should also be able to do in its buildFromSpec()?

        Does AstBuilder not support the following (because right now, methods from the builder don't get associated with ClassNode)?

        def classes= new AstBuilder().buildFromSpec{
          classNode 'MyClass', ACC_PUBLIC, {
            ....
            method('main', ACC_PUBLIC & ACC_STATIC, Void.class) {
                ....
            }
          }
        }
        
        Show
        Roshan Dawrani added a comment - No, no, you missed my point. All other things you say are fine (modifier, return type, CompilationUnit/SourceUnit association, etc), except one. This example is different from the issue #1 raised here because it is also doing the work of adding the MethodNode to the ClassNode - isn't that something that AstBuilder should also be able to do in its buildFromSpec()? Does AstBuilder not support the following (because right now, methods from the builder don't get associated with ClassNode)? def classes= new AstBuilder().buildFromSpec{ classNode 'MyClass', ACC_PUBLIC, { .... method('main', ACC_PUBLIC & ACC_STATIC, Void .class) { .... } } }
        Hide
        Paul King added a comment - - edited

        Following on from Roshan's comments, currently, I can only solve the original problem using something like:

        import static org.objectweb.asm.Opcodes.ACC_PUBLIC
        import static org.objectweb.asm.Opcodes.ACC_STATIC
        import static org.codehaus.groovy.control.CompilerConfiguration.*
        import org.codehaus.groovy.ast.builder.AstBuilder
        import org.codehaus.groovy.control.CompilationUnit
        
        def classes = new AstBuilder().buildFromSpec {
          classNode 'HelloClass', ACC_PUBLIC, {
            classNode Object //superclass
            interfaces {
              classNode GroovyObject
            }
            mixins {}
          }
        }
        
        def methods = new AstBuilder().buildFromSpec {
          method('main', ACC_PUBLIC | ACC_STATIC, Void.TYPE) {
            parameters {
              parameter 'args': String[].class
            }
            exceptions{}
            block{
              expression {
                methodCall {
                  variable "this"
                  constant "println"
                  argumentList {
                    constant "Hello, world!"
                  }
                }
              }
            }
          }
        }
        def helloClass = classes[0]
        def mainMethod = methods[0]
        helloClass.addMethod(mainMethod)
        def gcl = new GroovyClassLoader()
        def cu = new CompilationUnit(DEFAULT, null, gcl)
        cu.addClassNode(helloClass)
        cu.compile()
        

        It would be nice to have something like a {{methods

        {...}

        }} node which allowed the two to be combined.

        Show
        Paul King added a comment - - edited Following on from Roshan's comments, currently, I can only solve the original problem using something like: import static org.objectweb.asm.Opcodes.ACC_PUBLIC import static org.objectweb.asm.Opcodes.ACC_STATIC import static org.codehaus.groovy.control.CompilerConfiguration.* import org.codehaus.groovy.ast.builder.AstBuilder import org.codehaus.groovy.control.CompilationUnit def classes = new AstBuilder().buildFromSpec { classNode 'HelloClass', ACC_PUBLIC, { classNode Object //superclass interfaces { classNode GroovyObject } mixins {} } } def methods = new AstBuilder().buildFromSpec { method('main', ACC_PUBLIC | ACC_STATIC, Void .TYPE) { parameters { parameter 'args': String [].class } exceptions{} block{ expression { methodCall { variable " this " constant "println" argumentList { constant "Hello, world!" } } } } } } def helloClass = classes[0] def mainMethod = methods[0] helloClass.addMethod(mainMethod) def gcl = new GroovyClassLoader() def cu = new CompilationUnit(DEFAULT, null , gcl) cu.addClassNode(helloClass) cu.compile() It would be nice to have something like a {{methods {...} }} node which allowed the two to be combined.
        Hide
        Roshan Dawrani added a comment -

        And even on this, without a SourceUnit, all SourceUnitOperation(s) of the compilation are not happening - so no "resolve" is happening - all types have to be explicitly resolved, as in "parameter 'args': String[].class"

        Not sure whether such compile scenarios are supported by AstBuilder or not - once it goes beyond vanilla examples, differences from what compiler does normally may start coming in the way very fast - like ClassNodes not wrapped in a ModuleNode, no association with a SourceUnit, all SourceUnitOperation(s) getting skipped, etc.

        Show
        Roshan Dawrani added a comment - And even on this, without a SourceUnit, all SourceUnitOperation(s) of the compilation are not happening - so no "resolve" is happening - all types have to be explicitly resolved, as in "parameter 'args': String[].class" Not sure whether such compile scenarios are supported by AstBuilder or not - once it goes beyond vanilla examples, differences from what compiler does normally may start coming in the way very fast - like ClassNodes not wrapped in a ModuleNode, no association with a SourceUnit, all SourceUnitOperation(s) getting skipped, etc.
        Hide
        Hamlet D'Arcy added a comment -

        In my opinion, using the "buildFromSpecification" requires the user to know the details of the ASTNode subtypes to use effectively. Similarly, using the SwingBuilder requires the user to know about the concrete Swing types as well. The builders are a nice convenience syntactically, but are not an abstraction hiding the details. Since this is the case, I would recommend not adding convenience methods to the builder. Instead, add the convenience methods to the ASTNode subtypes directly and then add the support for these to the builder. I think the builder should remain a syntatic shortcut over the ASTNodes and not start adding too much Api to it.

        Show
        Hamlet D'Arcy added a comment - In my opinion, using the "buildFromSpecification" requires the user to know the details of the ASTNode subtypes to use effectively. Similarly, using the SwingBuilder requires the user to know about the concrete Swing types as well. The builders are a nice convenience syntactically, but are not an abstraction hiding the details. Since this is the case, I would recommend not adding convenience methods to the builder. Instead, add the convenience methods to the ASTNode subtypes directly and then add the support for these to the builder. I think the builder should remain a syntatic shortcut over the ASTNodes and not start adding too much Api to it.
        Hide
        Hamlet D'Arcy added a comment -

        @Roshan

        Your example should work fine with the AstBuilder. I guess I will have to look into it more. However, I'm still not convinced there is a bug in the AstBuilder. I think the error will come down to not using the API correctly.

        For instance, use the same example and replace all the AstBuilder with direct ASTNode constructors... I will bet that it doesn't work that way either.

        The test case for this needs to be an expected result created from ASTNode subclasses and an actual result created from AstBuilder... then compare the expected ASTNode to the actual ASTNode... all this wiring together and writing a class file is immaterial to the proper functioning of the builder.

        Show
        Hamlet D'Arcy added a comment - @Roshan Your example should work fine with the AstBuilder. I guess I will have to look into it more. However, I'm still not convinced there is a bug in the AstBuilder. I think the error will come down to not using the API correctly. For instance, use the same example and replace all the AstBuilder with direct ASTNode constructors... I will bet that it doesn't work that way either. The test case for this needs to be an expected result created from ASTNode subclasses and an actual result created from AstBuilder... then compare the expected ASTNode to the actual ASTNode... all this wiring together and writing a class file is immaterial to the proper functioning of the builder.
        Hide
        Roshan Dawrani added a comment -

        Well, if I replace all AstBuilder with direct ASTNode constructors, then apart from creating ClassNode and MethodNode using their constructors, I will also have to do ClassNode.addMethod(MethodNode) manually - for the ClassNode to compile into a class that has that method.

        That's the question - is AstBuilder supposed to do that internally or is that the responsibility of the AstBuilder user (as in Paul's example).

        If it is supposed to be AstBuilder's responsibility, then it is a bug in AstBuilder curently. If it is not, then that means the following is not supported:

        new AstBuilder().buildFromSpec{
          classNode 'MyClass', ACC_PUBLIC, {
            ....
            method('main', ACC_PUBLIC & ACC_STATIC, Void.class) {
                ....
            }
          }
        }
        

        Just trying to understand how much AstBuilder supports out-of-the-box.

        Show
        Roshan Dawrani added a comment - Well, if I replace all AstBuilder with direct ASTNode constructors, then apart from creating ClassNode and MethodNode using their constructors, I will also have to do ClassNode.addMethod(MethodNode) manually - for the ClassNode to compile into a class that has that method. That's the question - is AstBuilder supposed to do that internally or is that the responsibility of the AstBuilder user (as in Paul's example). If it is supposed to be AstBuilder's responsibility, then it is a bug in AstBuilder curently. If it is not, then that means the following is not supported: new AstBuilder().buildFromSpec{ classNode 'MyClass', ACC_PUBLIC, { .... method('main', ACC_PUBLIC & ACC_STATIC, Void .class) { .... } } } Just trying to understand how much AstBuilder supports out-of-the-box.
        Hide
        Hamlet D'Arcy added a comment -

        @Roshan

        OK, I understand now.

        My reaction is that AstBuilder should not do this. But I could be convinced either way. How many edge cases like this are there, I wonder? How far in this direction do we go?

        Show
        Hamlet D'Arcy added a comment - @Roshan OK, I understand now. My reaction is that AstBuilder should not do this. But I could be convinced either way. How many edge cases like this are there, I wonder? How far in this direction do we go?
        Hide
        Roshan Dawrani added a comment -

        I am not really sure that these are edge cases. To start with, there are FieldNode (which need the owner ClassNode in their constructor and also need to be added to ClassNode with addField()). Similarly for PropertyNodes (owner ClassNode + addProperty).

        First question to ask is about the purpose of AstSpecificationCompiler - whether it even wants to support the compile scenario as in this JIRA.

        If AstSpecificationCompiler wants to give out ClassNodes that are compilable, then I don't think it can avoid the API - it will have to setup of hierarchy of ASTNodes as the compiler expects.

        Show
        Roshan Dawrani added a comment - I am not really sure that these are edge cases. To start with, there are FieldNode (which need the owner ClassNode in their constructor and also need to be added to ClassNode with addField()). Similarly for PropertyNodes (owner ClassNode + addProperty). First question to ask is about the purpose of AstSpecificationCompiler - whether it even wants to support the compile scenario as in this JIRA. If AstSpecificationCompiler wants to give out ClassNodes that are compilable, then I don't think it can avoid the API - it will have to setup of hierarchy of ASTNodes as the compiler expects.
        Hide
        Hamlet D'Arcy added a comment -

        Yes, I see the point. Perhaps that would be a good improvement.

        Show
        Hamlet D'Arcy added a comment - Yes, I see the point. Perhaps that would be a good improvement.
        Hide
        Guillaume Laforge added a comment -

        I agree here that the various edge case nodes have to be "added" to their respective parents, like method nodes, field nodes, property nodes to their class node parent.
        (are we missing other nodes that need to be attached to their parent?)

        Show
        Guillaume Laforge added a comment - I agree here that the various edge case nodes have to be "added" to their respective parents, like method nodes, field nodes, property nodes to their class node parent. (are we missing other nodes that need to be attached to their parent?)
        Hide
        Hamlet D'Arcy added a comment -

        so, to summarize:
        we do want to auto-add these types to the parent. And this should be a minor improvement/enhancement that is part of a point release (7.1 most likely)

        Show
        Hamlet D'Arcy added a comment - so, to summarize: we do want to auto-add these types to the parent. And this should be a minor improvement/enhancement that is part of a point release (7.1 most likely)
        Hide
        Roshan Dawrani added a comment -

        Auto-wrapping expressions into statements may also be considered (like wrapping a MethodCallExpression into an ExpressionStatement).

        Currently, it has to be done manually as below

        block{
              expression { // we should see if this can be avoided at user level.
                methodCall {
                 ....
                }
              }
            }
        
        Show
        Roshan Dawrani added a comment - Auto-wrapping expressions into statements may also be considered (like wrapping a MethodCallExpression into an ExpressionStatement). Currently, it has to be done manually as below block{ expression { // we should see if this can be avoided at user level. methodCall { .... } } }
        Hide
        Guillaume Laforge added a comment -

        @Hamlet, yes, a must-have minor improvement/enhancement, good for a point release in a month or so
        @Roshan, we not necessarily mandatory, but if there are some places / cases like these ones we can help further simplify, why not!

        Show
        Guillaume Laforge added a comment - @Hamlet, yes, a must-have minor improvement/enhancement, good for a point release in a month or so @Roshan, we not necessarily mandatory, but if there are some places / cases like these ones we can help further simplify, why not!
        Hide
        Hamlet D'Arcy added a comment -

        @Guillaume

        >> If there are some places / cases like these ones we can help further simplify, why not!

        I am a little worried about where this leads. Currently, the best documentation for the buildFromSpec is the ASTNode javadoc. This is good, as it is backed by something thoroughly unit tested and is executable in some sense. If we diverge from a a simple wrapper then we need to add some sort of documentation or risk leaving users confused. But re-document the buildFromSpec then it is a big task and duplicates much of what is there. For me, I would not make these simplifications.

        Show
        Hamlet D'Arcy added a comment - @Guillaume >> If there are some places / cases like these ones we can help further simplify, why not! I am a little worried about where this leads. Currently, the best documentation for the buildFromSpec is the ASTNode javadoc. This is good, as it is backed by something thoroughly unit tested and is executable in some sense. If we diverge from a a simple wrapper then we need to add some sort of documentation or risk leaving users confused. But re-document the buildFromSpec then it is a big task and duplicates much of what is there. For me, I would not make these simplifications.
        Hide
        Guillaume Laforge added a comment -

        @Hamlet, yes, you're certainly right, I guess it may be a bit confusing if the builder doesn't closely follow the AST Groovy generates. So let's not do such shortcuts, and keep the structure the same in the builder and in the ASTNodes.

        Show
        Guillaume Laforge added a comment - @Hamlet, yes, you're certainly right, I guess it may be a bit confusing if the builder doesn't closely follow the AST Groovy generates. So let's not do such shortcuts, and keep the structure the same in the builder and in the ASTNodes.
        Hide
        Roshan Dawrani added a comment -

        Lost the thread a bit in the last couple of comments.

        We are talking about avoiding convenience short-cuts like auto-wrapping methodCall{} in expression {} and sticking 1:1 between build and AST API as far as possible, but we are still good to go on the improvement suggested to add methods/fields/properties, etc and setup the hierarchy of ASTNodes appropriately to make the ClassNode coming out of "buildFromSpec" compilable, right?

        Show
        Roshan Dawrani added a comment - Lost the thread a bit in the last couple of comments. We are talking about avoiding convenience short-cuts like auto-wrapping methodCall{} in expression {} and sticking 1:1 between build and AST API as far as possible, but we are still good to go on the improvement suggested to add methods/fields/properties, etc and setup the hierarchy of ASTNodes appropriately to make the ClassNode coming out of "buildFromSpec" compilable, right?
        Hide
        Hamlet D'Arcy added a comment -

        @Roshan

        That is correct.

        "We are talking about avoiding convenience short-cuts like auto-wrapping methodCall{} in expression {} and sticking 1:1 between build and AST API as far as possible, but we are still good to go on the improvement suggested to add methods/fields/properties, etc and setup the hierarchy of ASTNodes appropriately to make the ClassNode coming out of "buildFromSpec" compilable."

        Show
        Hamlet D'Arcy added a comment - @Roshan That is correct. "We are talking about avoiding convenience short-cuts like auto-wrapping methodCall{} in expression {} and sticking 1:1 between build and AST API as far as possible, but we are still good to go on the improvement suggested to add methods/fields/properties, etc and setup the hierarchy of ASTNodes appropriately to make the ClassNode coming out of "buildFromSpec" compilable."
        Hide
        Paul King added a comment -

        See possible solution: https://github.com/groovy/groovy-core/pull/439

        Current status: Allows method, constructor, property and field nodes to be embedded directly within a classNode spec currently without a wrapper helper method, e.g. "methods

        {...}

        ". This makes creating classes much easier but violates the consistency of the spec in that creating every other node type involves just filling in the information from the constructor arguments. This deviates from that pattern. So, we should consider whether we want that before proceeding.

        Show
        Paul King added a comment - See possible solution: https://github.com/groovy/groovy-core/pull/439 Current status: Allows method, constructor, property and field nodes to be embedded directly within a classNode spec currently without a wrapper helper method, e.g. "methods {...} ". This makes creating classes much easier but violates the consistency of the spec in that creating every other node type involves just filling in the information from the constructor arguments. This deviates from that pattern. So, we should consider whether we want that before proceeding.
        Hide
        Paul King added a comment - - edited

        I note that annotations aren't in the constructor and are checked for and added using addAnnotations (though for methods only). So, I'll adapt the proposed solution to work like that for other cases.

        I also note that annotations can't be processed without a SourceUnit (leads to NPEs), so the above fragments don't support adding annotations in any case.

        Show
        Paul King added a comment - - edited I note that annotations aren't in the constructor and are checked for and added using addAnnotations (though for methods only). So, I'll adapt the proposed solution to work like that for other cases. I also note that annotations can't be processed without a SourceUnit (leads to NPEs), so the above fragments don't support adding annotations in any case.
        Hide
        Paul King added a comment -

        I'll mark as resolved since the example below now runs:

        // groovyjarjarasm import reflects using the embedded Groovy packaged asm jar
        import static groovyjarjarasm.asm.Opcodes.*
        import org.codehaus.groovy.ast.builder.AstBuilder
        import org.codehaus.groovy.control.CompilerConfiguration
        import org.codehaus.groovy.control.CompilationUnit
        
        def classes = new AstBuilder().buildFromSpec{
          classNode 'MyClass', ACC_PUBLIC, {
            classNode Object //superclass
            interfaces {
              classNode GroovyObject
            }
            mixins{}
            genericsTypes{}
            methods {
              method('main', ACC_PUBLIC | ACC_STATIC, Void.TYPE) {
                parameters {
                  parameter 'args': String[].class
                }
                exceptions{}
                block {
                  expression {
                    methodCall {
                      variable "this"
                      constant "println"
                      argumentList {
                        constant "Hello, world!"
                      }
                    }
                  }
                }
              }
            }
          }
        }
        
        def gcl = new GroovyClassLoader()
        def sec = null
        def ccfg = CompilerConfiguration.DEFAULT
        def cu = new CompilationUnit(ccfg, sec, gcl)
        cu.addClassNode(classes[0])
        cu.compile()
        

        Please reopen if you disagree.

        I'll also note that if you add annotations into the example that it will no longer compile due to a missing source unit. I'm unsure how to get that working. I'll create another issue unless another developer can enlighten me as to how that would work.

        I'll also note that I haven't used Hamlet's patch which has an integration test. Someone wanting to add such a test is welcome to harvest the relevant section of that patch.

        Show
        Paul King added a comment - I'll mark as resolved since the example below now runs: // groovyjarjarasm import reflects using the embedded Groovy packaged asm jar import static groovyjarjarasm.asm.Opcodes.* import org.codehaus.groovy.ast.builder.AstBuilder import org.codehaus.groovy.control.CompilerConfiguration import org.codehaus.groovy.control.CompilationUnit def classes = new AstBuilder().buildFromSpec{ classNode 'MyClass', ACC_PUBLIC, { classNode Object //superclass interfaces { classNode GroovyObject } mixins{} genericsTypes{} methods { method('main', ACC_PUBLIC | ACC_STATIC, Void .TYPE) { parameters { parameter 'args': String [].class } exceptions{} block { expression { methodCall { variable " this " constant "println" argumentList { constant "Hello, world!" } } } } } } } } def gcl = new GroovyClassLoader() def sec = null def ccfg = CompilerConfiguration.DEFAULT def cu = new CompilationUnit(ccfg, sec, gcl) cu.addClassNode(classes[0]) cu.compile() Please reopen if you disagree. I'll also note that if you add annotations into the example that it will no longer compile due to a missing source unit. I'm unsure how to get that working. I'll create another issue unless another developer can enlighten me as to how that would work. I'll also note that I haven't used Hamlet's patch which has an integration test. Someone wanting to add such a test is welcome to harvest the relevant section of that patch.
        Hide
        Paul King added a comment -

        Just a followup to the previous comment. Setting a source unit seems to do the trick:

        import static groovyjarjarasm.asm.Opcodes.*
        import org.codehaus.groovy.ast.builder.AstBuilder
        import org.codehaus.groovy.ast.*
        import static org.codehaus.groovy.control.CompilerConfiguration.DEFAULT
        import org.codehaus.groovy.control.*
        
        def classes = new AstBuilder().buildFromSpec{
          classNode 'MyClass', ACC_PUBLIC, {
            classNode Object
            interfaces{ classNode GroovyObject }
            mixins{}
            annotations{ annotation Deprecated }
          }
        }
        
        def gcl = new GroovyClassLoader()
        def cu = new CompilationUnit(DEFAULT, null, gcl)
        def su = new SourceUnit("dummyName", "dummySource", DEFAULT, gcl, null)
        ModuleNode module = new ModuleNode(su)
        module.addClass(classes[0])
        cu.AST.addModule(module)
        cu.compile()
        new GroovyShell().evaluate '''
          assert Class.forName("MyClass").annotations[0].toString() == '@java.lang.Deprecated()'
        '''
        
        Show
        Paul King added a comment - Just a followup to the previous comment. Setting a source unit seems to do the trick: import static groovyjarjarasm.asm.Opcodes.* import org.codehaus.groovy.ast.builder.AstBuilder import org.codehaus.groovy.ast.* import static org.codehaus.groovy.control.CompilerConfiguration.DEFAULT import org.codehaus.groovy.control.* def classes = new AstBuilder().buildFromSpec{ classNode 'MyClass', ACC_PUBLIC, { classNode Object interfaces{ classNode GroovyObject } mixins{} annotations{ annotation Deprecated } } } def gcl = new GroovyClassLoader() def cu = new CompilationUnit(DEFAULT, null , gcl) def su = new SourceUnit( "dummyName" , "dummySource" , DEFAULT, gcl, null ) ModuleNode module = new ModuleNode(su) module.addClass(classes[0]) cu.AST.addModule(module) cu.compile() new GroovyShell().evaluate ''' assert Class .forName( "MyClass" ).annotations[0].toString() == '@java.lang.Deprecated()' '''

          People

          • Assignee:
            Paul King
            Reporter:
            Gavin Grover
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: