jira.codehaus.org

  • Log In Access more options
    • Online Help
    • Keyboard Shortcuts
    • About JIRA
    • JIRA Credits
    • What?s New
  • Dashboards Access more options (Alt+d)
  • Projects Access more options (Alt+p)
  • Issues Access more options (Alt+i)
  • groovy
  • GROOVY-4118

JavaStubGenerator doesn't generate annotations available in Groovy code

  • Log In
  • Views
    • XML
    • Word
    • Printable

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.7.1
  • Fix Version/s: 1.7.3, 1.8-beta-1
  • Component/s: class generator
  • Labels:
    None

Description

Looking at org.codehaus.groovy.tools.javac.JavaStubGenerator (http://goo.gl/ob23) I see it doesn't generate annotations that are originally available in Groovy sources.

I think it causes those issues later:

  • http://jira.codehaus.org/browse/GMAVEN-68 - "GMaven: generateStubs generates stubs without original Javadocs or annotations"
  • http://jira.codehaus.org/browse/GMAVEN-4 - "GMaven: Stub generation should generate annotations"

I'm trying to use AnnoMojo annotations (http://goo.gl/rbRw) when developing my MOJOs in Groovy.
GMaven's "generateStubs" goal doesn't produce Java sources with original AnnoMojo annotations

  • Options
    • Sort By Name
    • Sort By Date
    • Ascending
    • Descending
    • Download All

Attachments

  1. Text File
    groovy4118_JavaStubGenerator_annotations_B.patch
    09/Jun/10 12:05 AM
    4 kB
    Paul King
  2. Text File
    groovy4118_JavaStubGenerator_annotations.patch
    06/Jun/10 11:36 PM
    3 kB
    Paul King

Issue Links

is related to

Bug - A problem which impairs or prevents the functions of the product. GMAVEN-68 generateStubs generates stubs without original Javadocs or annotations

  • Major - Major loss of function.
  • Open - The issue is open and ready for the assignee to start work on it.

Bug - A problem which impairs or prevents the functions of the product. GMAVEN-75 Stub generation issues

  • Major - Major loss of function.
  • Closed - The issue is considered finished, the resolution is correct. Issues which are not closed can be reopened.

Activity

Ascending order - Click to sort in descending order
  • All
  • Comments
  • Work Log
  • History
  • Activity
Hide
Permalink
Paul King added a comment - 06/Jun/10 11:36 PM

Potential patch

Show
Paul King added a comment - 06/Jun/10 11:36 PM Potential patch
Hide
Permalink
Paul King added a comment - 06/Jun/10 11:38 PM

Jochen, let me know if there are any issues with patch otherwise I will apply in a few days.

Show
Paul King added a comment - 06/Jun/10 11:38 PM Jochen, let me know if there are any issues with patch otherwise I will apply in a few days.
Hide
Permalink
blackdrag blackdrag added a comment - 07/Jun/10 2:10 AM

the patch seems to be only allowing for numbers and constants that are ten interpreted as String. Missing are classes and enums (both clear name, the same as for numbers)

Show
blackdrag blackdrag added a comment - 07/Jun/10 2:10 AM the patch seems to be only allowing for numbers and constants that are ten interpreted as String. Missing are classes and enums (both clear name, the same as for numbers)
Hide
Permalink
blackdrag blackdrag added a comment - 07/Jun/10 2:11 AM

ah yes... another part would be to print the annotations only if the "java5" field is true.

Show
blackdrag blackdrag added a comment - 07/Jun/10 2:11 AM ah yes... another part would be to print the annotations only if the "java5" field is true.
Hide
Permalink
Paul King added a comment - 07/Jun/10 5:36 AM

Yes, both very good points. I might have to leave those changes and some kind of test to someone else unless I get a good Internet connection in San Fran and get time to look again.

Show
Paul King added a comment - 07/Jun/10 5:36 AM Yes, both very good points. I might have to leave those changes and some kind of test to someone else unless I get a good Internet connection in San Fran and get time to look again.
Hide
Permalink
Paul King added a comment - 09/Jun/10 12:05 AM

revised patch addressing Jochen's raised points

Show
Paul King added a comment - 09/Jun/10 12:05 AM revised patch addressing Jochen's raised points
Hide
Permalink
Paul King added a comment - 10/Jun/10 1:44 PM

So the one thing that I am still unsure of is whether we need to add back in the default imports which I commented out in genImports a few weeks back - everything else is fully qualified but I don't that is the case with Annotation constants.

Show
Paul King added a comment - 10/Jun/10 1:44 PM So the one thing that I am still unsure of is whether we need to add back in the default imports which I commented out in genImports a few weeks back - everything else is fully qualified but I don't that is the case with Annotation constants.
Hide
Permalink
blackdrag blackdrag added a comment - 11/Jun/10 3:21 AM

If the fully qualified class name would be used, then there is no need for an annotaton. Since there seems to be a class resolving phase I think that it is not needed

Show
blackdrag blackdrag added a comment - 11/Jun/10 3:21 AM If the fully qualified class name would be used, then there is no need for an annotaton. Since there seems to be a class resolving phase I think that it is not needed
Hide
Permalink
Paul King added a comment - 14/Jun/10 1:45 AM

For the Resolving Phase to occur before we print out the Java stub annotation info, I moved the stubGenerator visitor from CONVERSION to SEMANTIC_ANALYSIS phase. I still need to create a bunch of tests covering all of the cases.

Show
Paul King added a comment - 14/Jun/10 1:45 AM For the Resolving Phase to occur before we print out the Java stub annotation info, I moved the stubGenerator visitor from CONVERSION to SEMANTIC_ANALYSIS phase. I still need to create a bunch of tests covering all of the cases.
Hide
Permalink
Paul King added a comment - 14/Jun/10 1:52 AM

A quick check for import aliasing was also added. Again tests pending but works locally for me for the testcase given in GMAVEN-75.

Show
Paul King added a comment - 14/Jun/10 1:52 AM A quick check for import aliasing was also added. Again tests pending but works locally for me for the testcase given in GMAVEN-75.
Hide
Permalink
Guillaume Laforge added a comment - 14/Jun/10 11:34 AM

Paul made some changes in this area, please reopen the issue if it's still not working as expected.

Show
Guillaume Laforge added a comment - 14/Jun/10 11:34 AM Paul made some changes in this area, please reopen the issue if it's still not working as expected.
Hide
Permalink
Evgeny Goldin added a comment - 16/Jun/10 6:22 AM - edited

Just tried it with Groovy 1.7.3.
Annotations put on properties are not generated.

"src/main/scripts/com/clearforest/plugins/springbatch/SpringBatchMojo.groovy":

package com.clearforest.plugins.springbatch

import org.jfrog.maven.annomojo.annotations.MojoPhase
import org.jfrog.maven.annomojo.annotations.MojoGoal
import org.codehaus.gmaven.mojo.GroovyMojo
import org.jfrog.maven.annomojo.annotations.MojoParameter

/**
 * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 * Spring Batch invoker
 * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 */
@MojoGoal( "run" )
@MojoPhase( "install" )
class SpringBatchMojo extends GroovyMojo
{

    @MojoParameter ( required = true )
    String sth

    void execute ()
    {
        System.out.println( "[[[[[[[[$sth]]]]]]]]]]]" );
    }
}

generates the following code at "target/generated-sources/groovy-stubs/main/com/clearforest/plugins/springbatch/SpringBatchMojo.java":

package com.clearforest.plugins.springbatch;

import org.jfrog.maven.annomojo.annotations.MojoGoal;
import org.codehaus.gmaven.mojo.GroovyMojo;
import org.jfrog.maven.annomojo.annotations.MojoParameter;
import org.jfrog.maven.annomojo.annotations.MojoPhase;

@MojoGoal(value="run") @MojoPhase(value="install") public class SpringBatchMojo
  extends org.codehaus.gmaven.mojo.GroovyMojo  implements
    groovy.lang.GroovyObject {
public SpringBatchMojo
() {}
public  groovy.lang.MetaClass getMetaClass() { return (groovy.lang.MetaClass)null;}
public  void setMetaClass(groovy.lang.MetaClass mc) { }
public  java.lang.Object invokeMethod(java.lang.String method, java.lang.Object arguments) { return null;}
public  java.lang.Object getProperty(java.lang.String property) { return null;}
public  void setProperty(java.lang.String property, java.lang.Object value) { }
public  String getSth() { return (String)null;}
public  void setSth(String value) { }
public  void execute() { }
protected  groovy.lang.MetaClass $getStaticMetaClass() { return (groovy.lang.MetaClass)null;}
}

@MojoParameter annotation isn't generated

Show
Evgeny Goldin added a comment - 16/Jun/10 6:22 AM - edited Just tried it with Groovy 1.7.3. Annotations put on properties are not generated. "src/main/scripts/com/clearforest/plugins/springbatch/SpringBatchMojo.groovy":
package com.clearforest.plugins.springbatch

import org.jfrog.maven.annomojo.annotations.MojoPhase
import org.jfrog.maven.annomojo.annotations.MojoGoal
import org.codehaus.gmaven.mojo.GroovyMojo
import org.jfrog.maven.annomojo.annotations.MojoParameter

/**
 * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 * Spring Batch invoker
 * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 */
@MojoGoal( "run" )
@MojoPhase( "install" )
class SpringBatchMojo extends GroovyMojo
{

    @MojoParameter ( required = true )
    String sth

    void execute ()
    {
        System.out.println( "[[[[[[[[$sth]]]]]]]]]]]" );
    }
}
generates the following code at "target/generated-sources/groovy-stubs/main/com/clearforest/plugins/springbatch/SpringBatchMojo.java":
package com.clearforest.plugins.springbatch;

import org.jfrog.maven.annomojo.annotations.MojoGoal;
import org.codehaus.gmaven.mojo.GroovyMojo;
import org.jfrog.maven.annomojo.annotations.MojoParameter;
import org.jfrog.maven.annomojo.annotations.MojoPhase;

@MojoGoal(value="run") @MojoPhase(value="install") public class SpringBatchMojo
  extends org.codehaus.gmaven.mojo.GroovyMojo  implements
    groovy.lang.GroovyObject {
public SpringBatchMojo
() {}
public  groovy.lang.MetaClass getMetaClass() { return (groovy.lang.MetaClass)null;}
public  void setMetaClass(groovy.lang.MetaClass mc) { }
public  java.lang.Object invokeMethod(java.lang.String method, java.lang.Object arguments) { return null;}
public  java.lang.Object getProperty(java.lang.String property) { return null;}
public  void setProperty(java.lang.String property, java.lang.Object value) { }
public  String getSth() { return (String)null;}
public  void setSth(String value) { }
public  void execute() { }
protected  groovy.lang.MetaClass $getStaticMetaClass() { return (groovy.lang.MetaClass)null;}
}
@MojoParameter annotation isn't generated
Hide
Permalink
Guillaume Laforge added a comment - 16/Jun/10 6:27 AM

By the way, note that sth is not a field, but a property.

Show
Guillaume Laforge added a comment - 16/Jun/10 6:27 AM By the way, note that sth is not a field, but a property.
Hide
Permalink
Evgeny Goldin added a comment - 16/Jun/10 6:40 AM

Yeah, right, I mean properties, sorry

Show
Evgeny Goldin added a comment - 16/Jun/10 6:40 AM Yeah, right, I mean properties, sorry
Hide
Permalink
Roshan Dawrani added a comment - 16/Jun/10 7:34 AM

So, how do we handle properties here? Normally the stub generator doesn't print-out the private fields. A property becomes (a private field + getter/setter methods).

In properties case, do we start printing out the code for wrapped private fields also?

Not sure if it will make sense to print the annotation provided on the property on the getter/setter methods.

Show
Roshan Dawrani added a comment - 16/Jun/10 7:34 AM So, how do we handle properties here? Normally the stub generator doesn't print-out the private fields. A property becomes (a private field + getter/setter methods). In properties case, do we start printing out the code for wrapped private fields also? Not sure if it will make sense to print the annotation provided on the property on the getter/setter methods.
Hide
Permalink
Evgeny Goldin added a comment - 16/Jun/10 7:49 AM

My use case are Groovy MOJOs and I try to use AnnoMojo (http://goo.gl/rbRw) for Java5 annotations.
That's the reason for annotation properties, they become plugin configurations later

Show
Evgeny Goldin added a comment - 16/Jun/10 7:49 AM My use case are Groovy MOJOs and I try to use AnnoMojo (http://goo.gl/rbRw) for Java5 annotations. That's the reason for annotation properties, they become plugin configurations later
Hide
Permalink
Roshan Dawrani added a comment - 16/Jun/10 7:59 AM

Don't know about MOJOs. So, a question.

From your use case point of view, if "String sth" internally becomes "private String sth + public getSth()/setSth()" - will it help for the plugin configuration later if the annotation remained on the private field or moved on to accessor methods?

Show
Roshan Dawrani added a comment - 16/Jun/10 7:59 AM Don't know about MOJOs. So, a question. From your use case point of view, if "String sth" internally becomes "private String sth + public getSth()/setSth()" - will it help for the plugin configuration later if the annotation remained on the private field or moved on to accessor methods?
Hide
Permalink
Roshan Dawrani added a comment - 16/Jun/10 8:06 AM

Not sure whether it's an option for you or not, but a change from

@MojoParameter ( required = true )
String sth

to

@MojoParameter ( required = true )
public String sth

will make sure that you get the following in the output

@org.jfrog.maven.annomojo.annotations.MojoParameter(required=true) public java.lang.String sth;
Show
Roshan Dawrani added a comment - 16/Jun/10 8:06 AM Not sure whether it's an option for you or not, but a change from
@MojoParameter ( required = true )
String sth
to
@MojoParameter ( required = true )
public String sth
will make sure that you get the following in the output
@org.jfrog.maven.annomojo.annotations.MojoParameter(required=true) public java.lang.String sth;
Hide
Permalink
Evgeny Goldin added a comment - 16/Jun/10 9:08 AM

Yeap! It worked. Thanks a lot )

Show
Evgeny Goldin added a comment - 16/Jun/10 9:08 AM Yeap! It worked. Thanks a lot )
Hide
Permalink
Evgeny Goldin added a comment - 16/Jun/10 9:10 AM

Is this requirement for "public" modifier is something that I'll always need to use or it may be relaxed in the following versions?

Show
Evgeny Goldin added a comment - 16/Jun/10 9:10 AM Is this requirement for "public" modifier is something that I'll always need to use or it may be relaxed in the following versions?
Hide
Permalink
Roshan Dawrani added a comment - 16/Jun/10 9:10 AM

So, shall we close it back?

Show
Roshan Dawrani added a comment - 16/Jun/10 9:10 AM So, shall we close it back?
Hide
Permalink
Roshan Dawrani added a comment - 16/Jun/10 9:18 AM

Instead of a direct answer, there are a few indirect things there to be told:

1) "String sth" is a property in groovy code that gets changed internally to "private String sth + getSth()/setSth()"

2) "public String sth" is a field in groovy code and it remains unchanged as a field.

3) Stub generators currently generates annotations only for non-private fields and not for properties.

Going forward:

4) The stub generator may start doing something for properties as well - not sure at this point what that will be - whether the annotation will go on to stick to private field or accessors.

5) It is highly unlikely that property "String sth" will later translate to "public String sth" field instead of (private field + accessors).

Show
Roshan Dawrani added a comment - 16/Jun/10 9:18 AM Instead of a direct answer, there are a few indirect things there to be told: 1) "String sth" is a property in groovy code that gets changed internally to "private String sth + getSth()/setSth()" 2) "public String sth" is a field in groovy code and it remains unchanged as a field. 3) Stub generators currently generates annotations only for non-private fields and not for properties. Going forward: 4) The stub generator may start doing something for properties as well - not sure at this point what that will be - whether the annotation will go on to stick to private field or accessors. 5) It is highly unlikely that property "String sth" will later translate to "public String sth" field instead of (private field + accessors).
Hide
Permalink
Evgeny Goldin added a comment - 16/Jun/10 9:25 AM

Ok, I see, thanks for the explanation. In this case the issue can be closed again, I suppose.
I've just updated 3 of our Maven plugins to use AnnoMojo annotations instead of Javadoc ones, we'll see in a couple of days if everything is Ok.
But from what I saw, the generation works Ok now ..

Many thanks again. We were waiting for a long time to make this switch.

Show
Evgeny Goldin added a comment - 16/Jun/10 9:25 AM Ok, I see, thanks for the explanation. In this case the issue can be closed again, I suppose. I've just updated 3 of our Maven plugins to use AnnoMojo annotations instead of Javadoc ones, we'll see in a couple of days if everything is Ok. But from what I saw, the generation works Ok now .. Many thanks again. We were waiting for a long time to make this switch.
Hide
Permalink
Roshan Dawrani added a comment - 16/Jun/10 9:31 AM

You are welcome. Forwarding "many thanks" to Paul for the original implementation

Show
Roshan Dawrani added a comment - 16/Jun/10 9:31 AM You are welcome. Forwarding "many thanks" to Paul for the original implementation
Hide
Permalink
Gregory Fouquet added a comment - 23/Aug/10 10:33 AM

It seems to me annotation properties of array type are still not handled correctly. For example, with Groovy 1.7.3 and 1.7.4 :

// ArrayAnnotation.java
public @interface ArrayAnnotation {
String[] value() default {};
}

// AnnotatedClass.groovy
@ArrayAnnotation(["foo", "bar"])
class AnnotatedClass { }

generates the stub :

@ArrayAnnotation(value=null)
public class AnnotatedClass extends java.lang.Object implements groovy.lang.GroovyObject {
// irrelevent stuff removed
}
Show
Gregory Fouquet added a comment - 23/Aug/10 10:33 AM It seems to me annotation properties of array type are still not handled correctly. For example, with Groovy 1.7.3 and 1.7.4 :
// ArrayAnnotation.java
public @interface ArrayAnnotation {
String[] value() default {};
}

// AnnotatedClass.groovy
@ArrayAnnotation(["foo", "bar"])
class AnnotatedClass { }
generates the stub :
@ArrayAnnotation(value=null)
public class AnnotatedClass extends java.lang.Object implements groovy.lang.GroovyObject {
// irrelevent stuff removed
}
Hide
Permalink
Paul King added a comment - 23/Aug/10 4:59 PM

Yes, there are a few known limitations of the current stub generator - one of them being that we can't easily resolve constant expressions for the general case (at least not without significant overheads) and we handle constant-looking things as best we can by hand-coded "sniffing" of such expressions - and arrays/lists are not in the list of what we currently look for - but could be.

Does the above cause you particular problems? Obviously the real constants will be in the Groovy byte code just not in the stubs which are only short-lived.

In any case, probably worth a new issue to handle arrays/lists as that may not be too hard.

Show
Paul King added a comment - 23/Aug/10 4:59 PM Yes, there are a few known limitations of the current stub generator - one of them being that we can't easily resolve constant expressions for the general case (at least not without significant overheads) and we handle constant-looking things as best we can by hand-coded "sniffing" of such expressions - and arrays/lists are not in the list of what we currently look for - but could be. Does the above cause you particular problems? Obviously the real constants will be in the Groovy byte code just not in the stubs which are only short-lived. In any case, probably worth a new issue to handle arrays/lists as that may not be too hard.
Hide
Permalink
Gregory Fouquet added a comment - 24/Aug/10 7:48 AM

You are right, it turns out I can successfully deactivate stub generation for the offending classes (they are unit tests). Thanks for your insight.

Shall I file a new issue anyway ?

Show
Gregory Fouquet added a comment - 24/Aug/10 7:48 AM You are right, it turns out I can successfully deactivate stub generation for the offending classes (they are unit tests). Thanks for your insight. Shall I file a new issue anyway ?
Hide
Permalink
Paul King added a comment - 24/Aug/10 4:48 PM

Might be good to add. We can always close it if there is no interest or problems arise in solving it easily.

Show
Paul King added a comment - 24/Aug/10 4:48 PM Might be good to add. We can always close it if there is no interest or problems arise in solving it easily.
Hide
Permalink
Gregory Fouquet added a comment - 06/Sep/10 8:49 AM

With some delay, I opened issue GROOVY-4394

Show
Gregory Fouquet added a comment - 06/Sep/10 8:49 AM With some delay, I opened issue GROOVY-4394

People

  • Assignee:
    Paul King
    Reporter:
    Evgeny Goldin
Vote (0)
Watch (2)

Dates

  • Created:
    20/Mar/10 3:48 PM
    Updated:
    06/Sep/10 8:49 AM
    Resolved:
    16/Jun/10 9:31 AM
  • Atlassian JIRA (v5.0.4#731-sha1:3aa7374)
  • Report a problem
  • Powered by a free Atlassian JIRA open source license for Codehaus. Try JIRA - bug tracking software for your team.