groovy

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: 1.8.x, 2.x
  • 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

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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

People

  • Assignee:
    blackdrag blackdrag
    Reporter:
    Ted Naleid
Vote (1)
Watch (2)

Dates

  • Created:
    Updated: