Groovy
  1. Groovy
  2. GROOVY-3284

Call behavior on enums is inconsistent

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.5.7
    • Fix Version/s: 1.6.1, 1.5.8, 1.7-beta-1
    • Component/s: None
    • Labels:
      None
    • Environment:
      XP, cygwin, JRE 1.6.0

      Description

      A call method on an enum can be used within a for loop, but not otherwise. This is kind of odd, and it might be better to disable the for loop behavior if it's not reasonable to fix in general.

       
      
      enum Foo {
          A({ println("A") }),
          B({ println("B") })
          Foo(Closure c) {
            call = c
          }
          final Closure call
      }
      
      // works
      for (f in Foo) {
         f()
      }
      
      // works
      Foo.A.call()
      
      // doesn't work
      Foo.A()
      
      // doesn't work
      a=Foo.A
      a()
      

        Activity

        Hide
        Roshan Dawrani added a comment -

        I don't think if

        Foo.A() and

        a=Foo.A
        a()

        are even supposed to work as per groovy syntax.

        If call is a member of enum Foo of type Closure as shown in the JIRA example, then what is supposed to work is:

        Foo.A.call() and Foo.A.call.call() (same as f =

        {..}

        and calling it as f() and f.call()), and

        a=Foo.A
        a.call() and a.call.call()

        Show
        Roshan Dawrani added a comment - I don't think if Foo.A() and a=Foo.A a() are even supposed to work as per groovy syntax. If call is a member of enum Foo of type Closure as shown in the JIRA example, then what is supposed to work is: Foo.A.call() and Foo.A.call.call() (same as f = {..} and calling it as f() and f.call()), and a=Foo.A a.call() and a.call.call()
        Hide
        Roshan Dawrani added a comment -

        A call like Foo.A() is not supposed to work because Foo.A is of type Foo and not Closure.

        Show
        Roshan Dawrani added a comment - A call like Foo.A() is not supposed to work because Foo.A is of type Foo and not Closure.
        Hide
        Jochen Theodorou added a comment -

        you can't say it like that.... x() is supposed to resolve to x.call(), but this is a undocumented feature as we are not entirely sure if we really want to support this.

        Show
        Jochen Theodorou added a comment - you can't say it like that.... x() is supposed to resolve to x.call(), but this is a undocumented feature as we are not entirely sure if we really want to support this.
        Hide
        Roshan Dawrani added a comment -

        x() is supposed to resolve to x.call() even if x is not of type Closure? Irrespective of type of x?

        Show
        Roshan Dawrani added a comment - x() is supposed to resolve to x.call() even if x is not of type Closure? Irrespective of type of x?
        Hide
        Jochen Theodorou added a comment -

        kind of... as I said, it is not really specified. for sure I can say, that call() is to be made even if x is no Closure. I suspect in the MetaClass code where we test the field we test only for closures. A change here needs some experiments, because it may influence other parts.

        Show
        Jochen Theodorou added a comment - kind of... as I said, it is not really specified. for sure I can say, that call() is to be made even if x is no Closure. I suspect in the MetaClass code where we test the field we test only for closures. A change here needs some experiments, because it may influence other parts.
        Hide
        Roshan Dawrani added a comment -

        I have readied the partial fix for supporting Foo.A() resolving to Foo.A.call() in the example given in the JIRA.

        I am working on the 2nd half of the issue but I got a little confused by the previous comments.

        So, this is something that needs to be supported, right? Or, there is some unsurity about it?

        Show
        Roshan Dawrani added a comment - I have readied the partial fix for supporting Foo.A() resolving to Foo.A.call() in the example given in the JIRA. I am working on the 2nd half of the issue but I got a little confused by the previous comments. So, this is something that needs to be supported, right? Or, there is some unsurity about it?
        Hide
        Roshan Dawrani added a comment -

        For the 2nd issue reported, it works if it is used as

        Case 1) when a is not a dynamic variable
        def z = Foo.A 
        z() // prints A
        z = Foo.B
        z() // prints B
        

        but not if

        Case 2) when a is a dynamic variable
        a=Foo.A
        a()
        

        Is the code in case 1 above an acceptable workaround?

        Show
        Roshan Dawrani added a comment - For the 2nd issue reported, it works if it is used as Case 1) when a is not a dynamic variable def z = Foo.A z() // prints A z = Foo.B z() // prints B but not if Case 2) when a is a dynamic variable a=Foo.A a() Is the code in case 1 above an acceptable workaround?
        Hide
        Jochen Theodorou added a comment -

        I guess case 1 is what is implemented now.. case 2 will try to call the method named "a" and will then see that there is no such method, then it will get the property value a and see if it is a Closure, which it is not. So the call fails. The point that is open is, if we should look only at the property value. If we would simply try to execute the call method regardless the type, then it would work. I think a fix should go into that direction to complete the work the compiler does for local variables.

        Show
        Jochen Theodorou added a comment - I guess case 1 is what is implemented now.. case 2 will try to call the method named "a" and will then see that there is no such method, then it will get the property value a and see if it is a Closure, which it is not. So the call fails. The point that is open is, if we should look only at the property value. If we would simply try to execute the call method regardless the type, then it would work. I think a fix should go into that direction to complete the work the compiler does for local variables.
        Hide
        Roshan Dawrani added a comment -

        Ok, I will look into the issue 2 with the supplied information.

        For now, I have applied the partial fix to solve the issue 1.

        Now, Foo.A() gets resolved to Foo.A.call() for enum constant Foo.A.

        Before the fix, the target object for the method call A() was class Foo and that's why Foo.A() was getting treated like a static call A() on class Foo, and it was failing because that method wasn't there.

        Now, for Foo.A(), target is the enum constant Foo.A and it is like Foo.A() == <Foo.A>() == Foo.A.call().

        Show
        Roshan Dawrani added a comment - Ok, I will look into the issue 2 with the supplied information. For now, I have applied the partial fix to solve the issue 1. Now, Foo.A() gets resolved to Foo.A.call() for enum constant Foo.A. Before the fix, the target object for the method call A() was class Foo and that's why Foo.A() was getting treated like a static call A() on class Foo, and it was failing because that method wasn't there. Now, for Foo.A(), target is the enum constant Foo.A and it is like Foo.A() == <Foo.A>() == Foo.A.call().
        Hide
        Jochen Theodorou added a comment -

        I shouldn't have deleted the first part of my comment saying something like "adding a call transformation depending on the type of the compiler is a no go". Frankly I would say you should undo the commit. It goes not in the wrong direction and if case 2 is solved, then there is no need for your case1 fix too. I deleted that part of my comment because I thought that is the current state without your patch.

        If Foo.A() is seen as a static method call, then what is wrong with that? Doesn't the normal method call logic look for a propoerty? Shouldn't A be a propoerty/field of Foo? Doesn't that mean that case 2 will solve case 1 automatically?

        Show
        Jochen Theodorou added a comment - I shouldn't have deleted the first part of my comment saying something like "adding a call transformation depending on the type of the compiler is a no go". Frankly I would say you should undo the commit. It goes not in the wrong direction and if case 2 is solved, then there is no need for your case1 fix too. I deleted that part of my comment because I thought that is the current state without your patch. If Foo.A() is seen as a static method call, then what is wrong with that? Doesn't the normal method call logic look for a propoerty? Shouldn't A be a propoerty/field of Foo? Doesn't that mean that case 2 will solve case 1 automatically?
        Hide
        Roshan Dawrani added a comment -

        Before I go the undo commit path, just wanted to clear if there is some confusion among case 1/2 and issue 1/2 -

        So, there are two issues reported:

        For the enum:

        enum Foo {
            A({ println("A") })
            Foo(Closure c) {
              call = c
            }
            final Closure call
        }
        

        Issue 1: Foo.A() does not get resolved to Foo.A.call(). - Right now, this is what I have tried to resolve. So, if enum Foo as a field named A, which is of type Foo (which meets the condition required for enum constants) and there is method expression like Foo.A(), then the expression gets modified to Foo.A.call(). Is that not correct? Do you mean to say that if enum Foo has a static method A() as well as an enum constant A, then there will be a problem? If that is the case, you mean static call thing is more preferable and is direction it should go in?

        Issue 2: This issue says if we do
        a = Foo.A
        a(), then it does not work.

        For this I said, that without any changes done for this issue, if we instead do
        def a = Foo.A
        a() - then it works and had checked if this workaround was accpetable.

        I can still undo the commit but I thought I should make sure that there is no confusion between issue 1/2 and issue 2's case 1/case 2.

        Show
        Roshan Dawrani added a comment - Before I go the undo commit path, just wanted to clear if there is some confusion among case 1/2 and issue 1/2 - So, there are two issues reported: For the enum: enum Foo { A({ println( "A" ) }) Foo(Closure c) { call = c } final Closure call } Issue 1: Foo.A() does not get resolved to Foo.A.call(). - Right now, this is what I have tried to resolve. So, if enum Foo as a field named A, which is of type Foo (which meets the condition required for enum constants) and there is method expression like Foo.A(), then the expression gets modified to Foo.A.call(). Is that not correct? Do you mean to say that if enum Foo has a static method A() as well as an enum constant A, then there will be a problem? If that is the case, you mean static call thing is more preferable and is direction it should go in? Issue 2: This issue says if we do a = Foo.A a(), then it does not work. For this I said, that without any changes done for this issue, if we instead do def a = Foo.A a() - then it works and had checked if this workaround was accpetable. I can still undo the commit but I thought I should make sure that there is no confusion between issue 1/2 and issue 2's case 1/case 2.
        Hide
        Roshan Dawrani added a comment -

        I will appreciate if you can tell in what kind of enum related scenarios, the fix would interfere.

        I will try it your way anyway for the part-2 that is remaining.

        Thanks.

        Show
        Roshan Dawrani added a comment - I will appreciate if you can tell in what kind of enum related scenarios, the fix would interfere. I will try it your way anyway for the part-2 that is remaining. Thanks.
        Hide
        Jochen Theodorou added a comment -

        for your issue 1: the static method supersedes the call convention. So yes.. if there is a static method A, then it should be called instead of doing Foo.A.call()

        for issue 2: I think this is a workaround, but why letting have them define a local variable just for that? It would be better to then simply write Foo.A.call() then instead, or not? For DSLs it is better if it works without the workaround.

        If you are going to do the fix as I suggested, then the potential impact is unclear to me. I would probably put that not into 1.5.x or 1.6.x, but only into 1.7, because of that.

        Show
        Jochen Theodorou added a comment - for your issue 1: the static method supersedes the call convention. So yes.. if there is a static method A, then it should be called instead of doing Foo.A.call() for issue 2: I think this is a workaround, but why letting have them define a local variable just for that? It would be better to then simply write Foo.A.call() then instead, or not? For DSLs it is better if it works without the workaround. If you are going to do the fix as I suggested, then the potential impact is unclear to me. I would probably put that not into 1.5.x or 1.6.x, but only into 1.7, because of that.
        Hide
        Roshan Dawrani added a comment -

        Ok, I will first undo my changes from the 3 branches and then try to fix it as you have suggested.

        And then if I am solve it that way, I will have the patch reviewed first and do the changes only on 1.7.

        Show
        Roshan Dawrani added a comment - Ok, I will first undo my changes from the 3 branches and then try to fix it as you have suggested. And then if I am solve it that way, I will have the patch reviewed first and do the changes only on 1.7.
        Hide
        Jochen Theodorou added a comment -

        ok

        Show
        Jochen Theodorou added a comment - ok
        Hide
        Roshan Dawrani added a comment -

        Undid the changes made earlier.

        Show
        Roshan Dawrani added a comment - Undid the changes made earlier.
        Hide
        Roshan Dawrani added a comment -

        Resolved.

        Applied the new, reviewed fix on the branches 1.5.x, 1.6.x and trunk.

        Show
        Roshan Dawrani added a comment - Resolved. Applied the new, reviewed fix on the branches 1.5.x, 1.6.x and trunk.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development