groovy
  1. groovy
  2. GROOVY-3422

unable to reference one static closure from another without class name prefix

    Details

    • Type: Bug Bug
    • Status: Open Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.6
    • Fix Version/s: 3.0
    • Component/s: Compiler
    • Labels:
      None
    • Environment:
      groovy 1.6 java 1.6
    • Testcase included:
      yes
    • Number of attachments :
      1

      Description

      I've hit an issue in groovy 1.6 where one statically defined closure can't seem to access another static closure in the same class without prefixing the actual name of the class on the call to the closure.

      This test case demonstrates the issue:

      #! /usr/bin/env groovy

      class Foo {
      static first =

      { val -> "first $val" }

      // works as expected
      static second =

      { Foo.first( "second" ) }

      // doesn't work, apparently "first" is not in scope, is this expected behavior?
      static third =

      { first( "third" ) }

      }

      assert "first second" == Foo.second() // OK in 1.6
      assert "first third" == Foo.third() // blows up in 1.6!

        Activity

        Hide
        blackdrag blackdrag added a comment -

        Roshan, I am assigning this issue to you, because I think you recently resolved something compareable. If you are not ok with that, then assign the issue to me.

        Show
        blackdrag blackdrag added a comment - Roshan, I am assigning this issue to you, because I think you recently resolved something compareable. If you are not ok with that, then assign the issue to me.
        Hide
        Roshan Dawrani added a comment -

        Jochen, any initial opinion/input on whether it is better to try to resolve this unqualified call to static closure at compile-time or at run-time (meta-class infrastructure)?

        Show
        Roshan Dawrani added a comment - Jochen, any initial opinion/input on whether it is better to try to resolve this unqualified call to static closure at compile-time or at run-time (meta-class infrastructure)?
        Hide
        blackdrag blackdrag added a comment -

        the same program in a non static variant works, so I would say it should work both times in the same style.

        Show
        blackdrag blackdrag added a comment - the same program in a non static variant works, so I would say it should work both times in the same style.
        Hide
        Roshan Dawrani added a comment -

        Hi Jochen, I am a little tied up in my job for some days and am not able to give it sufficient time. So, I am assigning it back to you for now but I will try it as soon as I can. I don't want to hold it with me unnecessarily if it can be resolved much faster by you in the meanwhile.

        rgds,
        Roshan

        Show
        Roshan Dawrani added a comment - Hi Jochen, I am a little tied up in my job for some days and am not able to give it sufficient time. So, I am assigning it back to you for now but I will try it as soon as I can. I don't want to hold it with me unnecessarily if it can be resolved much faster by you in the meanwhile. rgds, Roshan
        Hide
        Roshan Dawrani added a comment -

        Hi,
        I am attaching a patch that fixes the reported issue. Right now, the patch is based on 1.5.x branch. I wanted to have it reviewed before also trying the same approach on 1.6.x and trunk.

        So, basically the fix is that when a closure's delegate/owner is a class, it was only looking for static meta method. After the patch, if a static meta method is not found, it checks if it has a static closure property of the same name and if it finds it, it maps the call to that closure's doCall() method and also changes the object (on which this doCall() should be called) to the closure instance.

        Please let me know if the fix looks ok. The patch has a test case and all existing tests have gone through.

        rgds,
        Roshan

        Show
        Roshan Dawrani added a comment - Hi, I am attaching a patch that fixes the reported issue. Right now, the patch is based on 1.5.x branch. I wanted to have it reviewed before also trying the same approach on 1.6.x and trunk. So, basically the fix is that when a closure's delegate/owner is a class, it was only looking for static meta method. After the patch, if a static meta method is not found, it checks if it has a static closure property of the same name and if it finds it, it maps the call to that closure's doCall() method and also changes the object (on which this doCall() should be called) to the closure instance. Please let me know if the fix looks ok. The patch has a test case and all existing tests have gone through. rgds, Roshan
        Hide
        Roshan Dawrani added a comment -

        Jochen, did you get a chance to review the patch?

        Shall I go ahead using it to fix the issue?

        Show
        Roshan Dawrani added a comment - Jochen, did you get a chance to review the patch? Shall I go ahead using it to fix the issue?
        Hide
        blackdrag blackdrag added a comment -

        I don't like that array solution very much... Since we do a static call can't we use the declaration class of the meta method to set the call class?

        Show
        blackdrag blackdrag added a comment - I don't like that array solution very much... Since we do a static call can't we use the declaration class of the meta method to set the call class?
        Hide
        Roshan Dawrani added a comment -

        I am not sure about that. I can't confirm as I don't have the source in front of me right now. But mapping of the call to static closure is being done to doCall() of the closure instance. So that closure instance is being put into the array element in being returned in this particular scenario. It is not the call class that is needed, it is the call object, which I don't think can be retrieved from the MetaMethod, can it?

        Yes, the array solution does not look very clean. I can introduce a small helper class to combine the meta method and the call object and return them together. I think it is private method anyway.

        Show
        Roshan Dawrani added a comment - I am not sure about that. I can't confirm as I don't have the source in front of me right now. But mapping of the call to static closure is being done to doCall() of the closure instance. So that closure instance is being put into the array element in being returned in this particular scenario. It is not the call class that is needed, it is the call object, which I don't think can be retrieved from the MetaMethod, can it? Yes, the array solution does not look very clean. I can introduce a small helper class to combine the meta method and the call object and return them together. I think it is private method anyway.
        Hide
        blackdrag blackdrag added a comment -

        oh, you are right about the call object.. A helper class is not needed, if it is only to wrap the result.But I prefer to that you return the array instead of giving it in from outside. The it is at last clear that more than one value is returned. The array values do need comments of course and one more thing.. I think MetaClassImpl is duplicating the logic, so you should look there too. Best is if you repeat the test with using MetaClassImpl as MetaClass for the Closure

        Show
        blackdrag blackdrag added a comment - oh, you are right about the call object.. A helper class is not needed, if it is only to wrap the result.But I prefer to that you return the array instead of giving it in from outside. The it is at last clear that more than one value is returned. The array values do need comments of course and one more thing.. I think MetaClassImpl is duplicating the logic, so you should look there too. Best is if you repeat the test with using MetaClassImpl as MetaClass for the Closure
        Hide
        Roshan Dawrani added a comment -

        All is clear and I will look at the duplicated logic that you have pointed to.

        But I did not understand what you mean by "repeat the test with using MetaClassImpl as MetaClass for the Closure". Can you explain that a bit please? Thanks.

        Show
        Roshan Dawrani added a comment - All is clear and I will look at the duplicated logic that you have pointed to. But I did not understand what you mean by "repeat the test with using MetaClassImpl as MetaClass for the Closure". Can you explain that a bit please? Thanks.
        Hide
        blackdrag blackdrag added a comment -

        the default meta class of a closure is ClosureMetaClass. In case that ExpandoMetaClass#enableGlobally() is called the closures will not get ClosureMetaClass as default, they will get ExpandoMetaClass as default. And that one is based on MetaClassImpl. so as the metaclass system is as of now tis means that ClosureMetaClass and MetaClassImpl will have to have both logic to handle closures. ClosureMetaClass is optimizing same cases, MetaClassImpl is not. So I suggest you do closure.metaClass = ... for every closure you test and then run the test again

        Show
        blackdrag blackdrag added a comment - the default meta class of a closure is ClosureMetaClass. In case that ExpandoMetaClass#enableGlobally() is called the closures will not get ClosureMetaClass as default, they will get ExpandoMetaClass as default. And that one is based on MetaClassImpl. so as the metaclass system is as of now tis means that ClosureMetaClass and MetaClassImpl will have to have both logic to handle closures. ClosureMetaClass is optimizing same cases, MetaClassImpl is not. So I suggest you do closure.metaClass = ... for every closure you test and then run the test again
        Hide
        blackdrag blackdrag added a comment -

        The issue has the problematic point, that closure needs to do a property lookup from within method lookup on something else than itself. This is something possibly influencing the MOP, thus I set a new fix version. For example we have to discuss if that should be done, and what helper methods should be used if it is done. Currently it is not done at all here

        Show
        blackdrag blackdrag added a comment - The issue has the problematic point, that closure needs to do a property lookup from within method lookup on something else than itself. This is something possibly influencing the MOP, thus I set a new fix version. For example we have to discuss if that should be done, and what helper methods should be used if it is done. Currently it is not done at all here

          People

          • Assignee:
            blackdrag blackdrag
            Reporter:
            Ted Naleid
          • Votes:
            1 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated: