Roshan Dawrani
added a comment - 05/Jan/10 2:19 PM 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.

Roshan Dawrani
added a comment - 06/Jan/10 12:11 AM 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 D'Arcy
added a comment - 06/Jan/10 1:33 AM 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.

Roshan Dawrani
added a comment - 06/Jan/10 1:45 AM 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.

Hamlet D'Arcy
added a comment - 06/Jan/10 1:55 AM 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.

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

Roshan Dawrani
added a comment - 06/Jan/10 2:02 AM 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) {
....
}
}
}

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.

Roshan Dawrani
added a comment - 06/Jan/10 2:58 AM 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.

Hamlet D'Arcy
added a comment - 06/Jan/10 4:19 AM 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.

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.

Hamlet D'Arcy
added a comment - 06/Jan/10 4:25 AM @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:

Roshan Dawrani
added a comment - 06/Jan/10 4:50 AM 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.

Hamlet D'Arcy
added a comment - 06/Jan/10 5:08 AM @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.

Roshan Dawrani
added a comment - 06/Jan/10 5:23 AM 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.

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

Guillaume Laforge
added a comment - 06/Jan/10 6:40 AM 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?)

Hamlet D'Arcy
added a comment - 06/Jan/10 10:59 AM 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)

Roshan Dawrani
added a comment - 06/Jan/10 11:10 AM 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 Laforge
added a comment - 06/Jan/10 12:05 PM @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!

>> 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 D'Arcy
added a comment - 07/Jan/10 6:40 AM @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.

Guillaume Laforge
added a comment - 07/Jan/10 4:02 PM @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.

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 Dawrani
added a comment - 07/Jan/10 8:49 PM 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?

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

Hamlet D'Arcy
added a comment - 08/Jan/10 1:35 AM @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."

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.

Paul King
added a comment - 08/Jun/14 5:27 AM 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.

Paul King
added a comment - 08/Jun/14 2:51 PM - 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.

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.