groovy
  1. groovy
  2. GROOVY-4339

Compiler first allows an annotation on a statement, then fails with a weird error message

    Details

    • Type: Bug Bug
    • Status: Open Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: 3.0
    • Component/s: class generator
    • Labels:
      None
    • Number of attachments :
      0

      Description

      int i
      
      @Deprecated
      i = 2
      

      Snippet above fails with

      _.groovy: 4: The current scope already contains a variable of the name i
       @ line 4, column 1.
         i = 2
      

      It should better say that an annotation is not allowed/handled where it is specified.

        Activity

        Hide
        Paul King added a comment - - edited

        Yes, the annotation (currently) acts as a type placeholder, effectively being the equivalent of def i = 2, which isn't always what would be desired. For an annotation that could appear in both places, I am not sure how we could easily distinguish between the two cases in a backwards compatible way.

        Show
        Paul King added a comment - - edited Yes, the annotation (currently) acts as a type placeholder, effectively being the equivalent of def i = 2 , which isn't always what would be desired. For an annotation that could appear in both places, I am not sure how we could easily distinguish between the two cases in a backwards compatible way.
        Hide
        Roshan Dawrani added a comment - - edited
        @AnyAnnotation
        i = 2
        

        is equivalent of def i = 2?

        Ok, if you say that fixing it will be breaking people's existing code using this feature.

        Fixing any bug is backward-incompatible. For a forward-looking project, first attempt should be to see whether something looks right/wrong, or desirable/undesirable.

        class Test {
            def foo() {
                int i
        
                @Deprecated
                i = 2 /* would anyone really be declaring variables like this - no def, no type, not even in a script? */
            }
        }
        
        Show
        Roshan Dawrani added a comment - - edited @AnyAnnotation i = 2 is equivalent of def i = 2 ? Ok, if you say that fixing it will be breaking people's existing code using this feature. Fixing any bug is backward-incompatible. For a forward-looking project, first attempt should be to see whether something looks right/wrong, or desirable/undesirable. class Test { def foo() { int i @Deprecated i = 2 /* would anyone really be declaring variables like this - no def, no type, not even in a script? */ } }
        Hide
        Guillaume Laforge added a comment -

        Yes:

        @AnyAnnotation
        i = 2
        

        is equivalent to

        @AnyAnnotation def i = 2
        

        So what do you suggest?
        That an annotation on a declaration mandate a type or def?

        Show
        Guillaume Laforge added a comment - Yes: @AnyAnnotation i = 2 is equivalent to @AnyAnnotation def i = 2 So what do you suggest? That an annotation on a declaration mandate a type or def?
        Hide
        Roshan Dawrani added a comment - - edited
        @AnyAnnotation
        i = 2
        

        If you insist on seeing the above as a "declaration", then it's a different thing.

        I was surprised to see that even in absence of absolutely any kind of modifier - def, static, final, some_type, volatile, etc, etc, it can be seen as a declaration.

        Show
        Roshan Dawrani added a comment - - edited @AnyAnnotation i = 2 If you insist on seeing the above as a "declaration", then it's a different thing. I was surprised to see that even in absence of absolutely any kind of modifier - def, static, final, some_type, volatile, etc, etc, it can be seen as a declaration.
        Hide
        Guillaume Laforge added a comment -

        It's not that I "insist" on see the above as a "declaration". But it currently is a declaration
        It doesn't mean that it's what it should be ultimately!
        I personally don't like this aspect of Groovy, and I'd prefer to have at least def or a type, and then the annotation before that.
        But as Paul says, that's a breaking change to fix that issue.
        So if we want to do something like that, we should be very careful.
        The problem with our current syntax is that we can't annotate a statement like an assignment, since Groovy sees that as a declaration.
        I'd actually like us to be a bit more strict with what Groovy allows here: for instance, def String foo is allowed, although either def or string should be used, but not both, but I've seen lots of code like that
        I'd be interested in also hearing others' view on this.
        I'd love to enforce a more idiomatic Groovy syntax in this area. I bet you're along that line of thinking too?
        If we're doing that, though, it'll have to be for 1.8 only, not for 1.7, as it's a notable breaking change – even if easy to fix.

        Show
        Guillaume Laforge added a comment - It's not that I "insist" on see the above as a "declaration". But it currently is a declaration It doesn't mean that it's what it should be ultimately! I personally don't like this aspect of Groovy, and I'd prefer to have at least def or a type, and then the annotation before that. But as Paul says, that's a breaking change to fix that issue. So if we want to do something like that, we should be very careful. The problem with our current syntax is that we can't annotate a statement like an assignment, since Groovy sees that as a declaration. I'd actually like us to be a bit more strict with what Groovy allows here: for instance, def String foo is allowed, although either def or string should be used, but not both, but I've seen lots of code like that I'd be interested in also hearing others' view on this. I'd love to enforce a more idiomatic Groovy syntax in this area. I bet you're along that line of thinking too? If we're doing that, though, it'll have to be for 1.8 only, not for 1.7, as it's a notable breaking change – even if easy to fix.
        Hide
        Roshan Dawrani added a comment - - edited

        I know it currently IS a declaration. I have seen the AST. The question raised is whether it should be.

        1.7 or 1.8 or 2.0 or easiness of the fix is also not the point here. The point is whether it is ok for the compiler to be so hyper-lenient that conflicting "def String s" is also ok and "@ann s" is also ok.

        Step 1 is to decide whether to see it as a faulty behavior or not. Release planning comes much later. No point in talking about both at the same time

        Show
        Roshan Dawrani added a comment - - edited I know it currently IS a declaration. I have seen the AST. The question raised is whether it should be. 1.7 or 1.8 or 2.0 or easiness of the fix is also not the point here. The point is whether it is ok for the compiler to be so hyper-lenient that conflicting "def String s" is also ok and "@ann s" is also ok. Step 1 is to decide whether to see it as a faulty behavior or not. Release planning comes much later. No point in talking about both at the same time
        Hide
        Guillaume Laforge added a comment -

        I already expressed my views here.
        I'd prefer making things more consistent, always requiring def or a type, but not both at the same time.

        Show
        Guillaume Laforge added a comment - I already expressed my views here. I'd prefer making things more consistent, always requiring def or a type, but not both at the same time.
        Hide
        Paul King added a comment -

        For me it isn't a yes/no answer that this is always a bug as is. Perhaps if the annotation can only be placed on a field or local variable, then the compiler should treat it as an implicit def as it does now. We definitely need to do some changes once annotations which appear in other places can be created or if we should choose to support such annotations now.

        Show
        Paul King added a comment - For me it isn't a yes/no answer that this is always a bug as is. Perhaps if the annotation can only be placed on a field or local variable, then the compiler should treat it as an implicit def as it does now. We definitely need to do some changes once annotations which appear in other places can be created or if we should choose to support such annotations now.
        Hide
        Roshan Dawrani added a comment -

        It is not true that only if an annotation can be placed on a field or local variable, the compiler treats it as an implicit def.

        import org.codehaus.groovy.transform.*
        
        @GroovyASTTransformation
        i = 10
        

        Even the above is fine - when GroovyASTTransformation is only meant for type level and nothing else - but that would be another bug.

        Show
        Roshan Dawrani added a comment - It is not true that only if an annotation can be placed on a field or local variable, the compiler treats it as an implicit def. import org.codehaus.groovy.transform.* @GroovyASTTransformation i = 10 Even the above is fine - when GroovyASTTransformation is only meant for type level and nothing else - but that would be another bug.
        Hide
        Paul King added a comment - - edited

        Yes, it would be good to have that fixed too. That particular example is nasty albeit somewhat benign. It leaves the TYPE annotation in the AST because it has RUNTIME retention but there is no easy way to get at it at runtime anyway!

        Show
        Paul King added a comment - - edited Yes, it would be good to have that fixed too. That particular example is nasty albeit somewhat benign. It leaves the TYPE annotation in the AST because it has RUNTIME retention but there is no easy way to get at it at runtime anyway!
        Hide
        Roshan Dawrani added a comment -

        Seems like a bunch of issues there in that case?

        1) the main one - whether @AnyAnn i should declare i or need def/some type/some modifier for it to be flagged a declaration?

        2) annotation target checking - @GroovyASTTransformation'a allowed targets are only TYPE, but it goes anywhere. @GroovyASTTransformation i = 10 has no target checking.

        3) as you say, if we want to support annotations at other places, maybe ones with retention-policy RUNTIME should be handled carefully.

        Show
        Roshan Dawrani added a comment - Seems like a bunch of issues there in that case? 1) the main one - whether @AnyAnn i should declare i or need def/some type/some modifier for it to be flagged a declaration? 2) annotation target checking - @GroovyASTTransformation'a allowed targets are only TYPE, but it goes anywhere. @GroovyASTTransformation i = 10 has no target checking. 3) as you say, if we want to support annotations at other places, maybe ones with retention-policy RUNTIME should be handled carefully.
        Hide
        Paul King added a comment -

        Two additional issues sounds good to me. Even though some of these aspects have been discussed before, I don't recall either (2) or (3) having specific Jira issues.

        Show
        Paul King added a comment - Two additional issues sounds good to me. Even though some of these aspects have been discussed before, I don't recall either (2) or (3) having specific Jira issues.
        Hide
        blackdrag blackdrag added a comment -

        The examples you have discussed till now are all centered about scripts, buttry to think about it in the context of a class. Afaik that is the only context this is used like this. But of course once the user knows, the user may choose to use it in a script context as well. The class context is also where it is possible t get the information if the rentention policy is set accordingly. Anyway...

        If we, at one time, really want to support annotations evrywhere, then this is in the way for the script context. There is no JIRA for this, but we discussed it several times and seem to want to have it. So I agree with Roshan on this one...

        For (1)... I can imagine Doug Lea wanting something like a @UnchangedFromHere annotation on a variable, without declaration.

        All in all I am in favor for no longer allowing this. Arguably we can allow it in the class context.

        Show
        blackdrag blackdrag added a comment - The examples you have discussed till now are all centered about scripts, buttry to think about it in the context of a class. Afaik that is the only context this is used like this. But of course once the user knows, the user may choose to use it in a script context as well. The class context is also where it is possible t get the information if the rentention policy is set accordingly. Anyway... If we, at one time, really want to support annotations evrywhere, then this is in the way for the script context. There is no JIRA for this, but we discussed it several times and seem to want to have it. So I agree with Roshan on this one... For (1)... I can imagine Doug Lea wanting something like a @UnchangedFromHere annotation on a variable, without declaration. All in all I am in favor for no longer allowing this. Arguably we can allow it in the class context.
        Hide
        Paul King added a comment -

        Some interesting (2008 vintage) presentations:

        http://www.cs.rice.edu/~mgricken/research/xajavac/download/annotation-poster-2008.pdf
        http://www.cs.rice.edu/~mgricken/research/xajavac/download/Java%20Annotations%20for%20Types%20and%20Expressions.ppt

        The ppt has a summary of the new annotation targets in JSR-308.
        Of particular interest is some proposed extensions beyond JSR-308, e.g.:

        @Contained {
          // block annotation
          // block of code that does not spawn async. tasks
        }
        int i = -5;
        int j = @AlwaysPositive (i*i); // paren. expression annotation
        

        And also some suggestions for removing ambiguities that arise when applying annotations to more targets.

        Show
        Paul King added a comment - Some interesting (2008 vintage) presentations: http://www.cs.rice.edu/~mgricken/research/xajavac/download/annotation-poster-2008.pdf http://www.cs.rice.edu/~mgricken/research/xajavac/download/Java%20Annotations%20for%20Types%20and%20Expressions.ppt The ppt has a summary of the new annotation targets in JSR-308. Of particular interest is some proposed extensions beyond JSR-308, e.g.: @Contained { // block annotation // block of code that does not spawn async. tasks } int i = -5; int j = @AlwaysPositive (i*i); // paren. expression annotation And also some suggestions for removing ambiguities that arise when applying annotations to more targets.
        Hide
        Paul King added a comment - - edited

        Also, an example which currently runs which we can use to discuss which bits (if any) we would like to make no longer valid:

        import java.lang.annotation.*
        import groovy.transform.*
        
        @Retention(RetentionPolicy.SOURCE)
        @Target(ElementType.LOCAL_VARIABLE)
        public @interface NonNegative {}
        
        @Retention(RetentionPolicy.SOURCE)
        // ElementType.BINARY_EXPRESSION is hypothetical extension
        @Target([ElementType.LOCAL_VARIABLE/*, ElementType.BINARY_EXPRESSION*/])
        public @interface Trace {}
        
        class Foo {
          def items = []
          def bar() {
            @Lazy a = 1
            @PackageScope b = 2
            @NonNegative c = 3
            @Trace d = 4
            //@Trace d = 5
            items += [a, b, c, d]
          }
        }
        
        @Lazy u = 6
        @PackageScope v = 7
        @ScriptField w = 8
        @NonNegative x = 9
        @Trace y = 10
        //@Trace y = 11
        @Grab('com.google.collections:google-collections:1.0')
        z = new com.google.common.collect.HashBiMap(a:1)
        
        assert u == 6
        assert v == 7
        assert w == 8
        assert x == 9
        assert y == 10 // 11
        assert z.inverse()[1] == 'a'
        
        def f = new Foo()
        f.bar()
        assert f.items == [1, 2, 3, 4 /* 5 */]
        

        Notes:

        • ElementType.BINARY_EXPRESSION is just one possible suggestion. Might be better to be more specific, e.g. just on assignment or more general, e.g. on any expression.
        • @Trace is made up and intended to log or print output similar to what PowerAssert gives us but without comparing two sides - just the result of the expression
        • @NonNegative is made up here but JSR-305 and many other packages already have similar annotations. It might indicate that a local variable or field will only contain zero or positive values. It might be used for optimisation or runtime validation checks.
        • @PackageScope is a bit of an outlier - shouldn't be controversial, it is highlighting that we aren't picking up that it is placed on a LOCAL_VARIABLE instead of a FIELD
        • @Lazy is also a bit odd. Hard to tell what it might mean for a local variable but in theory we might have some AST transforms that are applicable to LOCAL_VARIABLEs.
        • @Grab currently works on LOCAL_VARIABLEs (among other things).
        Show
        Paul King added a comment - - edited Also, an example which currently runs which we can use to discuss which bits (if any) we would like to make no longer valid: import java.lang.annotation.* import groovy.transform.* @Retention(RetentionPolicy.SOURCE) @Target(ElementType.LOCAL_VARIABLE) public @ interface NonNegative {} @Retention(RetentionPolicy.SOURCE) // ElementType.BINARY_EXPRESSION is hypothetical extension @Target([ElementType.LOCAL_VARIABLE/*, ElementType.BINARY_EXPRESSION*/]) public @ interface Trace {} class Foo { def items = [] def bar() { @Lazy a = 1 @PackageScope b = 2 @NonNegative c = 3 @Trace d = 4 //@Trace d = 5 items += [a, b, c, d] } } @Lazy u = 6 @PackageScope v = 7 @ScriptField w = 8 @NonNegative x = 9 @Trace y = 10 //@Trace y = 11 @Grab('com.google.collections:google-collections:1.0') z = new com.google.common.collect.HashBiMap(a:1) assert u == 6 assert v == 7 assert w == 8 assert x == 9 assert y == 10 // 11 assert z.inverse()[1] == 'a' def f = new Foo() f.bar() assert f.items == [1, 2, 3, 4 /* 5 */] Notes: ElementType.BINARY_EXPRESSION is just one possible suggestion. Might be better to be more specific, e.g. just on assignment or more general, e.g. on any expression. @Trace is made up and intended to log or print output similar to what PowerAssert gives us but without comparing two sides - just the result of the expression @NonNegative is made up here but JSR-305 and many other packages already have similar annotations. It might indicate that a local variable or field will only contain zero or positive values. It might be used for optimisation or runtime validation checks. @PackageScope is a bit of an outlier - shouldn't be controversial, it is highlighting that we aren't picking up that it is placed on a LOCAL_VARIABLE instead of a FIELD @Lazy is also a bit odd. Hard to tell what it might mean for a local variable but in theory we might have some AST transforms that are applicable to LOCAL_VARIABLEs. @Grab currently works on LOCAL_VARIABLEs (among other things).
        Hide
        blackdrag blackdrag added a comment -

        I changed the fix target of this to 2.0 with the intention to make an annotation on an assignment not be a declaration. Since that is a breaking change the only scope this can be done is Groovy 2.0

        Show
        blackdrag blackdrag added a comment - I changed the fix target of this to 2.0 with the intention to make an annotation on an assignment not be a declaration. Since that is a breaking change the only scope this can be done is Groovy 2.0
        blackdrag blackdrag made changes -
        Field Original Value New Value
        Assignee Jochen Theodorou [ blackdrag ]
        Fix Version/s 2.0 [ 13489 ]
        Affects Version/s 1.8-beta-1 [ 16013 ]

          People

          • Assignee:
            Unassigned
            Reporter:
            Roshan Dawrani
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated: