adding method to class in ASTBuilder not working

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);
}

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()

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.

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()

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.

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?

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.

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.

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) {
        ....
    }
  }
}

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.

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.

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.

@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.

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.

@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?

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.

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

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?)

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)

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 {
         ....
        }
      }
    }

@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!

@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.

@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.

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?

@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."