groovy

Using referenced String constant as value of Annotation causes compile error

Details

  • Type: Bug Bug
  • Status: Open Open
  • Priority: Major Major
  • Resolution: Unresolved
  • Affects Version/s: 1.6-rc-1
  • Fix Version/s: 1.7.x
  • Component/s: Compiler
  • Labels:
    None
  • Environment:
    OSX 10.5.6
    JDK 1.6
  • Number of attachments :
    2

Description

When using a String constant as the value for an annotation groovyc errors out with the following message:

Attribute 'value' should have type java.lang.String but found type java.lang.Object in @SpringBean

I've attached sample code to reproduce. The annotation and PersonDao are regular Java classes, just as was my case when finding the bug. The PersonAction is a Groovy class.

Issue Links

This issue duplicates:
GROOVY-2827 org.codehaus.groovy.ast.expr.PropertyExpression when using external constants in annotations Major Closed

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Roshan Dawrani added a comment -

As per http://www.nabble.com/Annotation-values-in-Groovy.-td20824889.html and looking at the code, it looks like annotation values in groovy can only be constant values (like Strings, classes, numbers, other annotations and enums, etc).

In this case, annotation value itself is a property expression PersonDao.DAO_ID, which is pointing to a static final String but in terms of groovy internals, it is not a constant expression but a property expression.

So, should this case be seen as a defect or as designed?

Show
Roshan Dawrani added a comment - As per http://www.nabble.com/Annotation-values-in-Groovy.-td20824889.html and looking at the code, it looks like annotation values in groovy can only be constant values (like Strings, classes, numbers, other annotations and enums, etc). In this case, annotation value itself is a property expression PersonDao.DAO_ID, which is pointing to a static final String but in terms of groovy internals, it is not a constant expression but a property expression. So, should this case be seen as a defect or as designed?
Hide
Permalink
Paul King added a comment -

Not sure it matters whether we call it a bug or enhancement request. It isn't supported now and seems a very worthwhile (necessary?) enhancement.

Show
Paul King added a comment - Not sure it matters whether we call it a bug or enhancement request. It isn't supported now and seems a very worthwhile (necessary?) enhancement.
Hide
Permalink
Gregg Bolinger added a comment -

I was thinking the same thing as Paul, FWIW

Show
Gregg Bolinger added a comment - I was thinking the same thing as Paul, FWIW
Hide
Permalink
Roshan Dawrani added a comment -

You are right. It does look a worthwhile feature but if it more like an enhancement, then it may not be scheduled for 1.6, right?
Or, enhancements are still being taken up for 1.6?

Show
Roshan Dawrani added a comment - You are right. It does look a worthwhile feature but if it more like an enhancement, then it may not be scheduled for 1.6, right? Or, enhancements are still being taken up for 1.6?
Hide
Permalink
blackdrag blackdrag added a comment -

It can be seen as improvement or as bug. But knowing that the groovy compiler does no form of inlining atm and this issue means we have to add that I would say it should not go into 1.6.0, but 1.6.1 might be ok

Show
blackdrag blackdrag added a comment - It can be seen as improvement or as bug. But knowing that the groovy compiler does no form of inlining atm and this issue means we have to add that I would say it should not go into 1.6.0, but 1.6.1 might be ok
Hide
Permalink
Guillaume Laforge added a comment -

It is a bug, not an improvement
I haven't double checked how it's working in Java, but since the PersonDao.DAO_ID field is a String constant, it should be allowed in Java.
And hence in Groovy.
I wish it could be fixed in 1.6.0, but if it's not, it should really be fixed in 1.6.1.

Show
Guillaume Laforge added a comment - It is a bug, not an improvement I haven't double checked how it's working in Java, but since the PersonDao.DAO_ID field is a String constant, it should be allowed in Java. And hence in Groovy. I wish it could be fixed in 1.6.0, but if it's not, it should really be fixed in 1.6.1.
Hide
Permalink
Roshan Dawrani added a comment -

It works in Java. I had double checked that earlier but then I saw that mailing list thread reply and thought that groovy annotations had some known constraints and were not as flexible as Java.

Show
Roshan Dawrani added a comment - It works in Java. I had double checked that earlier but then I saw that mailing list thread reply and thought that groovy annotations had some known constraints and were not as flexible as Java.
Hide
Permalink
Guillaume Laforge added a comment -

Groovy's annotations should work as much as possible as in Java.
But it doesn't prevent us from providing additional capabilities
But at least, here, I think it should behave just as in Java.

Show
Guillaume Laforge added a comment - Groovy's annotations should work as much as possible as in Java. But it doesn't prevent us from providing additional capabilities But at least, here, I think it should behave just as in Java.
Hide
Permalink
Paul King added a comment -

I think I agree with Guillaume's last comment. But just so that we all understand the implications. Consider this code example:

import java.lang.annotation.*

@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.TYPE)
@interface MyAnnotation {
    String[] groups() default []
}

class Base {
    static final String CONST = 'bing'
    def run() { getClass().annotations[0].groups() }
}

@MyAnnotation(groups = 'foo')
class Child1 extends Base {}

@MyAnnotation(groups = ['bar', 'baz', Base.CONST]) //  *1
class Child2 extends Base {}

assert new Child1().run() == ['foo']
assert new Child2().run() == ['bar', 'baz']

Currently *1 breaks because we are expecting a static String but get a PropertyExpression that would evaluate at runtime to a constant.
The implication if we somehow evaluate this is that we are bypassing any runtime behavior, i.e.
if Base had any MOP in place which changed the value of CONST at runtime, that would be ignored here.
I believe that is the right thing. Just confirming everyone's opinions.

Show
Paul King added a comment - I think I agree with Guillaume's last comment. But just so that we all understand the implications. Consider this code example: 
import java.lang.annotation.*

@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.TYPE)
@interface MyAnnotation {
    String[] groups() default []
}

class Base {
    static final String CONST = 'bing'
    def run() { getClass().annotations[0].groups() }
}

@MyAnnotation(groups = 'foo')
class Child1 extends Base {}

@MyAnnotation(groups = ['bar', 'baz', Base.CONST]) //  *1
class Child2 extends Base {}

assert new Child1().run() == ['foo']
assert new Child2().run() == ['bar', 'baz']
Currently *1 breaks because we are expecting a static String but get a PropertyExpression that would evaluate at runtime to a constant. The implication if we somehow evaluate this is that we are bypassing any runtime behavior, i.e. if Base had any MOP in place which changed the value of CONST at runtime, that would be ignored here. I believe that is the right thing. Just confirming everyone's opinions.
Hide
Permalink
Paul King added a comment -

Probably a duplicate

Show
Paul King added a comment - Probably a duplicate
Hide
Permalink
blackdrag blackdrag added a comment -

isn't that equal to a problem you showed me in Paris? Where we found that we cannot access the constant value through reflection without initializing the class? And where we said that we maybe have to say, that we don't support that particular part? So from the language side it would be good to have that, but from the technical side I don't know how to implement that. As of runtime vs. compile time... I agree with Paul here

Show
blackdrag blackdrag added a comment - isn't that equal to a problem you showed me in Paris? Where we found that we cannot access the constant value through reflection without initializing the class? And where we said that we maybe have to say, that we don't support that particular part? So from the language side it would be good to have that, but from the technical side I don't know how to implement that. As of runtime vs. compile time... I agree with Paul here
Hide
Permalink
Paul King added a comment -

I think the attached patch will cover most cases but there is quite likely a better way to do it. As per the issue Jochen and I discussed in Paris, you can't wait until (say the ExtendedVerifier) and do checking in one spot because by then 'constants' are actually pulled out of 'fields' and placed into a static initializer. This meant that I had to try 'statically resolving' the constants earlier. I did it in ResolveVisitor and StaticImportVisitor. Happy for other suggestions. If I don't get feedback in the next couple of days, I will apply as is and we can tweak later if need be.

Show
Paul King added a comment - I think the attached patch will cover most cases but there is quite likely a better way to do it. As per the issue Jochen and I discussed in Paris, you can't wait until (say the ExtendedVerifier) and do checking in one spot because by then 'constants' are actually pulled out of 'fields' and placed into a static initializer. This meant that I had to try 'statically resolving' the constants earlier. I did it in ResolveVisitor and StaticImportVisitor. Happy for other suggestions. If I don't get feedback in the next couple of days, I will apply as is and we can tweak later if need be.
Hide
Permalink
Paul King added a comment -

If anyone is interested, here is the test case I was using:

import java.lang.annotation.*
import static Constants.BAR

@Retention(RetentionPolicy.RUNTIME)
@Target([ElementType.METHOD, ElementType.FIELD])
@interface Anno {
    String value() default ""
    String[] array() default []
}

class Constants {
    static final String FOO = "foo"
    static final String BAR = "bar"
}

class ClassWithAnnotationUsingConstant {
    @Anno(array = [Constants.FOO, BAR, org.objectweb.asm.Opcodes.INVOKEDYNAMIC_OWNER])
    public annotatedField
}

assert ClassWithAnnotationUsingConstant.getDeclaredField('annotatedField').annotations[0].value() == ['foo', 'bar', "java/lang/dyn/Dynamic"]

Making use of InvokeDynamic parts of ASM 3.2 (locally on my machine) already!

Show
Paul King added a comment - If anyone is interested, here is the test case I was using: 
import java.lang.annotation.*
import static Constants.BAR

@Retention(RetentionPolicy.RUNTIME)
@Target([ElementType.METHOD, ElementType.FIELD])
@interface Anno {
    String value() default ""
    String[] array() default []
}

class Constants {
    static final String FOO = "foo"
    static final String BAR = "bar"
}

class ClassWithAnnotationUsingConstant {
    @Anno(array = [Constants.FOO, BAR, org.objectweb.asm.Opcodes.INVOKEDYNAMIC_OWNER])
    public annotatedField
}

assert ClassWithAnnotationUsingConstant.getDeclaredField('annotatedField').annotations[0].value() == ['foo', 'bar', "java/lang/dyn/Dynamic"]
Making use of InvokeDynamic parts of ASM 3.2 (locally on my machine) already!
Hide
Permalink
Paul King added a comment -

fix and test applied to trunk (pending merge with 1_6_X branch but I will wait a day to let dust settle and make sure no catastrophes)

Show
Paul King added a comment - fix and test applied to trunk (pending merge with 1_6_X branch but I will wait a day to let dust settle and make sure no catastrophes)
Hide
Permalink
blackdrag blackdrag added a comment -

But Paul... am I right assuming that 

import java.lang.annotation.*
import static Constants.BAR

@Retention(RetentionPolicy.RUNTIME)
@Target([ElementType.METHOD, ElementType.FIELD])
@interface Anno {
    int value() default -1
}

class ClassWithAnnotationUsingConstant {
    @Anno(value = Integer.MAX_VALUE)
    public annotatedField
}

assert ClassWithAnnotationUsingConstant.getDeclaredField('annotatedField').annotations[0].value() == Integer.MAX_VALUE

will not work? IMHO the solution you provided looks for a ConstantExpression and if that is there the value can be used. If not, then it is a problem. Assume that in your example Constants is precompiled and you get exactly the same problem, even if it is a groovy class. The solution you gave works only with not precompiled classes or will have unwanted side effects. using Annotation constants is a different thing, because here the constant can be read without executing the static initializer. Because of that for example @Target([ElementType.METHOD, ElementType.FIELD]) works in Groovy.

But I am against a solution that works sometimes under some special occurrences only. So I am also against this fix.

Show
blackdrag blackdrag added a comment - But Paul... am I right assuming that 
import java.lang.annotation.*
import static Constants.BAR

@Retention(RetentionPolicy.RUNTIME)
@Target([ElementType.METHOD, ElementType.FIELD])
@interface Anno {
    int value() default -1
}

class ClassWithAnnotationUsingConstant {
    @Anno(value = Integer.MAX_VALUE)
    public annotatedField
}

assert ClassWithAnnotationUsingConstant.getDeclaredField('annotatedField').annotations[0].value() == Integer.MAX_VALUE
will not work? IMHO the solution you provided looks for a ConstantExpression and if that is there the value can be used. If not, then it is a problem. Assume that in your example Constants is precompiled and you get exactly the same problem, even if it is a groovy class. The solution you gave works only with not precompiled classes or will have unwanted side effects. using Annotation constants is a different thing, because here the constant can be read without executing the static initializer. Because of that for example @Target([ElementType.METHOD, ElementType.FIELD]) works in Groovy. But I am against a solution that works sometimes under some special occurrences only. So I am also against this fix.
Hide
Permalink
Paul King added a comment -

The example you give (at least after removing the import to Constants which isn't around in your example) will work fine and having it work for Java constants was my main motivation but I understand your reservations.

We can't at the moment make it work for constants in pre-compiled Groovy classes but it will for constants in pre-compiled Groovy interfaces. So, sniffing around looking for the constant expressions is a bit of a compromise trying to cater for some of the more common cases.

Can you think of a better approach? I don't think not supporting Java cases is a nice option as existing Java libraries already have such usage patterns when used from Java.

Show
Paul King added a comment - The example you give (at least after removing the import to Constants which isn't around in your example) will work fine and having it work for Java constants was my main motivation but I understand your reservations. We can't at the moment make it work for constants in pre-compiled Groovy classes but it will for constants in pre-compiled Groovy interfaces. So, sniffing around looking for the constant expressions is a bit of a compromise trying to cater for some of the more common cases. Can you think of a better approach? I don't think not supporting Java cases is a nice option as existing Java libraries already have such usage patterns when used from Java.
Hide
Permalink
blackdrag blackdrag added a comment -

if that example is working then I missed out part of your change. Before your patch, if you asked the ClassNode of Integer for the FieldNode MAX_VALUE, you got no initialExpression, so you could not get an constant expression and no value to use... right, I missed out this part: 

try {
                    if (type.isResolved()) {
                        Field field = type.getTypeClass().getField(pe.getPropertyAsString());
                        if (field != null) {
                            return new ConstantExpression(field.get(null));
                        }
                    }
                } catch(Exception e) {/*ignore*/}
If the class has an static initializer, then this code here will cause the static initializer to be executed, else it is impossible to get the value. And I thought we don't want this to happen. Btw, this part of the code forgets to check that the field is static, but you swallow the exception anyway... which is another part I don't like. If we execute the static initializer, then why making things even worse by swallowing any exception that initializer may throw? Then I don't understand your text about interfaces. Normal interfaces are not that much different from normal classes. They have an static initializer too, and that one will get executed as well. Only in case of an annotation it is possible to read the value directly without first executing the static initializer.

Also it is a bit unclear to me why most of your code is in StaticImportVisitor. If I got it right, then you transform ClassNode.Field variant used by a PropertyExpression, even if that is outside of an annotation. If we are going to add such dangerous code, that executes static initializers, then we should not make matters worse and execute it for parts we don't even need... that is unless I misread the code and it is really limited to annotations... If it is limited to annotations, then it should be in an annotations handling visitor or a new visitor. It is no good to just enlarge the existing visitors, especially not the big ones. Your argument that you cannot do it in ExtendedVerifier is one I cannot follow. Sure, there the values are pulled out, but what is so bad for this? Why not change AnnotationVisitor to do exactly that? It is not like the constant will have any value for us in other parts at the moment.

As for precompiled Groovy classes. I don't folow that argument. The code you added will execute the static initializer, thus it will not make any difference between precompiled Groovy code or Java code, regardless of being a class or interface. The only case we can safely support is groovy code we currently compile, which is too limited.

Can I think of a better approach that works in all cases? No. We could try reading the class in bytecode and extract this information from there, but that is lot more complicated and will not work if we don't have the bytecode.. which at runtime is a common case.

Show
blackdrag blackdrag added a comment - if that example is working then I missed out part of your change. Before your patch, if you asked the ClassNode of Integer for the FieldNode MAX_VALUE, you got no initialExpression, so you could not get an constant expression and no value to use... right, I missed out this part: 
try {
                    if (type.isResolved()) {
                        Field field = type.getTypeClass().getField(pe.getPropertyAsString());
                        if (field != null) {
                            return new ConstantExpression(field.get(null));
                        }
                    }
                } catch(Exception e) {/*ignore*/}
If the class has an static initializer, then this code here will cause the static initializer to be executed, else it is impossible to get the value. And I thought we don't want this to happen. Btw, this part of the code forgets to check that the field is static, but you swallow the exception anyway... which is another part I don't like. If we execute the static initializer, then why making things even worse by swallowing any exception that initializer may throw? Then I don't understand your text about interfaces. Normal interfaces are not that much different from normal classes. They have an static initializer too, and that one will get executed as well. Only in case of an annotation it is possible to read the value directly without first executing the static initializer. Also it is a bit unclear to me why most of your code is in StaticImportVisitor. If I got it right, then you transform ClassNode.Field variant used by a PropertyExpression, even if that is outside of an annotation. If we are going to add such dangerous code, that executes static initializers, then we should not make matters worse and execute it for parts we don't even need... that is unless I misread the code and it is really limited to annotations... If it is limited to annotations, then it should be in an annotations handling visitor or a new visitor. It is no good to just enlarge the existing visitors, especially not the big ones. Your argument that you cannot do it in ExtendedVerifier is one I cannot follow. Sure, there the values are pulled out, but what is so bad for this? Why not change AnnotationVisitor to do exactly that? It is not like the constant will have any value for us in other parts at the moment. As for precompiled Groovy classes. I don't folow that argument. The code you added will execute the static initializer, thus it will not make any difference between precompiled Groovy code or Java code, regardless of being a class or interface. The only case we can safely support is groovy code we currently compile, which is too limited. Can I think of a better approach that works in all cases? No. We could try reading the class in bytecode and extract this information from there, but that is lot more complicated and will not work if we don't have the bytecode.. which at runtime is a common case.
Hide
Permalink
Paul King added a comment -

The ClassNode.Field variant is only looked at when inside annotations unless I made a mistake in the code that checks that. So, yes we are executing the static initializer but for what should be only a very small handful of classes. I'll have a think about how to move the code. The reason I put it where I did was because I couldn't get it to work in ExtendedVerifier. I might ask for your assistance later. Do you know what the javac compiler does here? Do you know if it has equivalent code that would cause static initializers to be executed?

Show
Paul King added a comment - The ClassNode.Field variant is only looked at when inside annotations unless I made a mistake in the code that checks that. So, yes we are executing the static initializer but for what should be only a very small handful of classes. I'll have a think about how to move the code. The reason I put it where I did was because I couldn't get it to work in ExtendedVerifier. I might ask for your assistance later. Do you know what the javac compiler does here? Do you know if it has equivalent code that would cause static initializers to be executed?
Hide
Permalink
blackdrag blackdrag added a comment -

javac is a different problem domain, since javac has always the byte or the source code. javac must inline the constant because no other option is allowed. that means javac will read the bytecode, extract the value and then... just found an interesting corner case for javac:

class Constants {
  final static String FOO;
  static {
    FOO = "foo";
  }
}

class ClassWithAnnotationUsingConstant {
    @Anno(value=Constants.FOO)
    public Object annotatedField;
}
This code does not compile. And indeed the inline version is represented different in bytecode. Now that opens a new issue for Groovy. To allow this usage pattern Groovy has to do it the same way, meaning inline. If not then it will make problems even from Java. Sadly reflection does not provide any way to go around this issue. There is only one way, and that will get the static initializer being executed.

Show
blackdrag blackdrag added a comment - javac is a different problem domain, since javac has always the byte or the source code. javac must inline the constant because no other option is allowed. that means javac will read the bytecode, extract the value and then... just found an interesting corner case for javac:
class Constants {
  final static String FOO;
  static {
    FOO = "foo";
  }
}

class ClassWithAnnotationUsingConstant {
    @Anno(value=Constants.FOO)
    public Object annotatedField;
}
This code does not compile. And indeed the inline version is represented different in bytecode. Now that opens a new issue for Groovy. To allow this usage pattern Groovy has to do it the same way, meaning inline. If not then it will make problems even from Java. Sadly reflection does not provide any way to go around this issue. There is only one way, and that will get the static initializer being executed.
Hide
Permalink
Peter Niederwieser added a comment - - edited

Have you come to a conclusion on how to solve this issue? We frequently use String constants in annotations, and it's very awkward that Groovy forces us to inline them.

Show
Peter Niederwieser added a comment - - edited Have you come to a conclusion on how to solve this issue? We frequently use String constants in annotations, and it's very awkward that Groovy forces us to inline them.

People

Vote (9)
Watch (10)

Dates

  • Created:
    Updated:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Codehaus. Try JIRA - bug tracking software for your team.