groovy
  1. groovy
  2. GROOVY-2925

ExpandoMetaClass sometimes, but sometimes MetaClassImpl

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Critical Critical
    • Resolution: Won't Fix
    • Affects Version/s: 1.6-beta-1
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Testcase included:
      yes
    • Number of attachments :
      0

      Description

      The type of a new instance of a class depends on whether the metaClass property on its class has been accessed before. That is just not right.

      groovy> class A {}
      groovy> println A.metaClass
      groovy> def a = new A ()
      groovy> println a.metaClass
      groovy> println A.metaClass

      groovy.lang.ExpandoMetaClass@b05c54[class A]
      groovy.lang.ExpandoMetaClass@b05c54[class A]
      groovy.lang.ExpandoMetaClass@b05c54[class A]
      groovy> class A {}
      groovy> //println A.metaClass
      groovy> def a = new A ()
      groovy> println a.metaClass
      groovy> println A.metaClass

      groovy.lang.MetaClassImpl@a7eb57[class A]
      groovy.lang.ExpandoMetaClass@2e22ed[class A]

        Activity

        Hide
        blackdrag blackdrag added a comment -

        this is at the moment by design. If you make a new instance of a GroovyObject, then this instace caches the metaClass once you ask for it. That means, if the MetaClass was not changed before you ask for the MetaClass, then the instance will have the new MetaClass. Else it will have the old one. This has been the case since early we have per instance MetaClasses in Groovy... so the only fix would be to remove per instance MetaClasses... I don't expect you will get a majority for this.

        Show
        blackdrag blackdrag added a comment - this is at the moment by design. If you make a new instance of a GroovyObject, then this instace caches the metaClass once you ask for it. That means, if the MetaClass was not changed before you ask for the MetaClass, then the instance will have the new MetaClass. Else it will have the old one. This has been the case since early we have per instance MetaClasses in Groovy... so the only fix would be to remove per instance MetaClasses... I don't expect you will get a majority for this.
        Hide
        Robert Fischer added a comment -

        Why is the instance metaClass property sometime of type ExpandoMetaClass and sometimes of type MetaClassImpl? That's the problem here – not the existence of per-instance metaclasses.

        Show
        Robert Fischer added a comment - Why is the instance metaClass property sometime of type ExpandoMetaClass and sometimes of type MetaClassImpl? That's the problem here – not the existence of per-instance metaclasses.
        Hide
        blackdrag blackdrag added a comment -

        ok, let us go through the code step-by-step:

        class A {}
        println A.metaClass

        this call DefaultGroovyMethods#getMetaClass(Class), which will set an ExpandoMetaClass for A, and this will eb the default fro A from here on. So, when you create a new instance and ask for the metaClass property of the instance, then it will return the current default, which is ExpandoMetaClass. Thus

        def a = new A ()
        println a.metaClass
        println A.metaClass

        will then print ExpandoMetaClass. Now in the code

        class A {}

        A will have the normal default MetaClass, which is MetaclassImpl. and since you didn't change the class yet, a

        def a = new A ()
        println a.metaClass

        will return MetaClassImpl. At this point a.metaClass will always return MetaClassImpl, unless you set the value to a new one. But the default for A can still be changed, which you do with:

        println A.metaClass
        

        So this will again print ExnapdoMetaClass. If you would now execute this:

        println a.metaClass
        def b = new A ()
        println b.metaClass

        you will still get MetaClassImpl for a, but ExpandoMetaClass for b. I copuld go even further and a.metaClass to null, which will cause a.metaClass to get the current default again, and then a.metaClass would return ExpandoMetaClass too.

        Show
        blackdrag blackdrag added a comment - ok, let us go through the code step-by-step: class A {} println A.metaClass this call DefaultGroovyMethods#getMetaClass(Class), which will set an ExpandoMetaClass for A, and this will eb the default fro A from here on. So, when you create a new instance and ask for the metaClass property of the instance, then it will return the current default, which is ExpandoMetaClass. Thus def a = new A () println a.metaClass println A.metaClass will then print ExpandoMetaClass. Now in the code class A {} A will have the normal default MetaClass, which is MetaclassImpl. and since you didn't change the class yet, a def a = new A () println a.metaClass will return MetaClassImpl. At this point a.metaClass will always return MetaClassImpl, unless you set the value to a new one. But the default for A can still be changed, which you do with: println A.metaClass So this will again print ExnapdoMetaClass. If you would now execute this: println a.metaClass def b = new A () println b.metaClass you will still get MetaClassImpl for a, but ExpandoMetaClass for b. I copuld go even further and a.metaClass to null, which will cause a.metaClass to get the current default again, and then a.metaClass would return ExpandoMetaClass too.
        Hide
        Robert Fischer added a comment -

        I understand that's what it's doing. What I don't understand is why that's acceptable.

        Basically, that means that if you want to do instance-level metaClass mangling based on the ExpandoMetaClass, you have to do: "A.metaClass" before "def a = new A()" or you have no guaranties as to the type of "a.metaClass". Which, I suppose, is a just fine work-around assuming you know what type your "A" is or can control where it's being instantiated.

        Why is MetaClassImpl exposed at all? Why don't all classes have a single ExpandoMetaClass (which could be generated through a CacheMap[1]), which is cloned as part of the constructor call or on the first access to a.metaClass or whatever?

        I encountered this bug, BTW, while working on a blog post [2].

        [1] http://code.google.com/p/jconch/wiki/CacheMapExamples
        [2] http://enfranchisedmind.com/blog/2008/06/24/functional-metaprogramming-ruby-groovy/ – see right above "Let The System Handle The Clean-Up"

        Show
        Robert Fischer added a comment - I understand that's what it's doing. What I don't understand is why that's acceptable. Basically, that means that if you want to do instance-level metaClass mangling based on the ExpandoMetaClass, you have to do: "A.metaClass" before "def a = new A()" or you have no guaranties as to the type of "a.metaClass". Which, I suppose, is a just fine work-around assuming you know what type your "A" is or can control where it's being instantiated. Why is MetaClassImpl exposed at all? Why don't all classes have a single ExpandoMetaClass (which could be generated through a CacheMap [1] ), which is cloned as part of the constructor call or on the first access to a.metaClass or whatever? I encountered this bug, BTW, while working on a blog post [2] . [1] http://code.google.com/p/jconch/wiki/CacheMapExamples [2] http://enfranchisedmind.com/blog/2008/06/24/functional-metaprogramming-ruby-groovy/ – see right above "Let The System Handle The Clean-Up"
        Hide
        blackdrag blackdrag added a comment -

        look at it from another point of view... what do you say when you set a.metaClass to something custom and a later A.metaClass would change this to ExpandoMetaClass? As to why MetaClassImpl is exposed... Well it is a class in groovy.lang. Groovy allows you to completely replace the meta class. That is unlike most other language, where you can add or remove something through specified interfaces, but as we do it, you could for example even change the method dispatching. So while our way is much more powerful, it has its glitches. and as we can replace the MetaClass completely it makes not that much sense to expect that the MetaClass is constant. In 2.0 we are planing to change all this, as we are aware of the glitches and dislike them too. The first step for 2.0 will be to make ExpandoMetaClass the default, which will reduce the need to replace the MetaClass. But let me modify your example... let us assume we use A.metaClass to add a method to the MetaClass, then we create an instance of A and I guess we should all expect that this instance will have the new method as well. Now let us assume we have the class B, then create an instance of B named b and then add a method to B.metaClass... what do you expect now? Should b have that or not? If you think of b having its own metaClass right after b was created, then b should not have the method. If you think that B.metaClass is there to make global changes, then it should have it. But these two are different models already. A third model would be to not to have a per instance MetaClass at all... but ok, now let us assume we have per instance meta classes and that you can replace a meta class completely. Then would you assume that B.metaClass = foo will actually overwrite the MetaClass for the b instance as well?

        Anyway, I forgot to mention, that there isa simple way of ensuring that all MetaClasses will be ExpandoMetaClass and that is by ExpandoMetaClass.enableGlobally()...see http://groovy.codehaus.org/ExpandoMetaClass With this method (if it is called before any MetaClass action is done) you can ensure that.

        Show
        blackdrag blackdrag added a comment - look at it from another point of view... what do you say when you set a.metaClass to something custom and a later A.metaClass would change this to ExpandoMetaClass? As to why MetaClassImpl is exposed... Well it is a class in groovy.lang. Groovy allows you to completely replace the meta class. That is unlike most other language, where you can add or remove something through specified interfaces, but as we do it, you could for example even change the method dispatching. So while our way is much more powerful, it has its glitches. and as we can replace the MetaClass completely it makes not that much sense to expect that the MetaClass is constant. In 2.0 we are planing to change all this, as we are aware of the glitches and dislike them too. The first step for 2.0 will be to make ExpandoMetaClass the default, which will reduce the need to replace the MetaClass. But let me modify your example... let us assume we use A.metaClass to add a method to the MetaClass, then we create an instance of A and I guess we should all expect that this instance will have the new method as well. Now let us assume we have the class B, then create an instance of B named b and then add a method to B.metaClass... what do you expect now? Should b have that or not? If you think of b having its own metaClass right after b was created, then b should not have the method. If you think that B.metaClass is there to make global changes, then it should have it. But these two are different models already. A third model would be to not to have a per instance MetaClass at all... but ok, now let us assume we have per instance meta classes and that you can replace a meta class completely. Then would you assume that B.metaClass = foo will actually overwrite the MetaClass for the b instance as well? Anyway, I forgot to mention, that there isa simple way of ensuring that all MetaClasses will be ExpandoMetaClass and that is by ExpandoMetaClass.enableGlobally()...see http://groovy.codehaus.org/ExpandoMetaClass With this method (if it is called before any MetaClass action is done) you can ensure that.
        Hide
        Robert Fischer added a comment -

        When I asked "why is MetaClassImpl exposed at all?", I meant to ask, "Why isn't ExpandoMetaClass already the default, if it's going to be dropped onto the class as soon as the static #metaClass property is accessed anyway?"

        > "what do you say when you set a.metaClass to something custom and a later A.metaClass would change this to ExpandoMetaClass?"
        I'd say that A.metaClass shouldn't be changing it to ExpandoMetaClass, it already should have been an ExpandoMetaClass. And I'd say property accessors shouldn't have side effects.

        The issue of whether global changes to the static metaClass impact and instances is totally tangential to this conversation. Unsurprisingly, I've got opinions there, but we'll leave that to a more appropriate forum.

        Sounds like I'm best off calling ExpandoMetaClass.enableGlobally() before trying to do any metaprogramming in Groovy. Either that, or just holding off until Groovy 2, particularly if things are going to change substantially at that point.

        Show
        Robert Fischer added a comment - When I asked "why is MetaClassImpl exposed at all?", I meant to ask, "Why isn't ExpandoMetaClass already the default, if it's going to be dropped onto the class as soon as the static #metaClass property is accessed anyway?" > "what do you say when you set a.metaClass to something custom and a later A.metaClass would change this to ExpandoMetaClass?" I'd say that A.metaClass shouldn't be changing it to ExpandoMetaClass, it already should have been an ExpandoMetaClass. And I'd say property accessors shouldn't have side effects. The issue of whether global changes to the static metaClass impact and instances is totally tangential to this conversation. Unsurprisingly, I've got opinions there, but we'll leave that to a more appropriate forum. Sounds like I'm best off calling ExpandoMetaClass.enableGlobally() before trying to do any metaprogramming in Groovy. Either that, or just holding off until Groovy 2, particularly if things are going to change substantially at that point.
        Hide
        Robert Fischer added a comment -

        Let me give a very specific example as to the problem:

        I want to write a method that attaches a new method to an object. Something like:
        def tag(a) {
        a.metaClass.tagged <<

        { true }

        }
        (As a more realistic example, see my blog, where I try and add a property to an instance.)

        The problem is that whether or not that code will explode depends entirely on whether or not the instance I passed in has had it's static metaClass property accessor called. And I won't know if it passes or fails until runtime. So it's very possible that a debug statement in some far away piece of code (which statically interrogates the metaClass) might make this code OK, and bypassing/removing said debug statement would make this code explode.

        That kind of awkward code dependency is a major issue. Glad to hear the Groovy team is looking into it for the Groovy 2 release.

        Show
        Robert Fischer added a comment - Let me give a very specific example as to the problem: I want to write a method that attaches a new method to an object. Something like: def tag(a) { a.metaClass.tagged << { true } } (As a more realistic example, see my blog, where I try and add a property to an instance.) The problem is that whether or not that code will explode depends entirely on whether or not the instance I passed in has had it's static metaClass property accessor called. And I won't know if it passes or fails until runtime. So it's very possible that a debug statement in some far away piece of code (which statically interrogates the metaClass) might make this code OK, and bypassing/removing said debug statement would make this code explode. That kind of awkward code dependency is a major issue. Glad to hear the Groovy team is looking into it for the Groovy 2 release.
        Hide
        blackdrag blackdrag added a comment -

        when you say "property accessors shouldn't have side effects", then believe me, I can feel with you. I am more than unlucky with that "solution". But there is much code out there depending on that, and it is actually better, than what we had before. So it was an improvement, even if it is no good solution. I suggest you go with what Grails does and that is using ExpandoMetaClass.enableGlobally().

        about your code example... I guess you got something wrong.. as soon as you do for example "println a.metaClass"the property will not be null. And this property is not a static one, it is a property of the object. So if you need to ensure that you can add the method/property, you have to check the MetaClass first:

        def tag (a) {
          if (a.metaClass.getClass()==MetaClassImpl) {
            ExpandoMetaClass.enableGlobally()
            a.metaClass = null
          }
        
          if (a.metaClass instanceof ExpandoMetaclass) {
            a.metaClass.tagged << { true }
          } else{
            throw new RuntimeException("the tag object has an unexpected metaclass: $a.metaclass")
          }
        }

        What this code does, is to check first if the object you want to tag uses MetaClassImpl, and if so an ExpandoMetaClass will be used. This code makes no instanceof check,because MetaClassImpl is most probably the base of all meta classes out there... well probably besides ProxyMetaClass. Anyway, th code will reset the metaClass for the object, and in the second if it will check for ExpandoMetaClass and then execute the action. If the metaClass is something else, then an Exception will be thrown. By using a closure for the action, you can easily make a library function out of this.

        Show
        blackdrag blackdrag added a comment - when you say "property accessors shouldn't have side effects", then believe me, I can feel with you. I am more than unlucky with that "solution". But there is much code out there depending on that, and it is actually better, than what we had before. So it was an improvement, even if it is no good solution. I suggest you go with what Grails does and that is using ExpandoMetaClass.enableGlobally(). about your code example... I guess you got something wrong.. as soon as you do for example "println a.metaClass"the property will not be null. And this property is not a static one, it is a property of the object. So if you need to ensure that you can add the method/property, you have to check the MetaClass first: def tag (a) { if (a.metaClass.getClass()==MetaClassImpl) { ExpandoMetaClass.enableGlobally() a.metaClass = null } if (a.metaClass instanceof ExpandoMetaclass) { a.metaClass.tagged << { true } } else { throw new RuntimeException( "the tag object has an unexpected metaclass: $a.metaclass" ) } } What this code does, is to check first if the object you want to tag uses MetaClassImpl, and if so an ExpandoMetaClass will be used. This code makes no instanceof check,because MetaClassImpl is most probably the base of all meta classes out there... well probably besides ProxyMetaClass. Anyway, th code will reset the metaClass for the object, and in the second if it will check for ExpandoMetaClass and then execute the action. If the metaClass is something else, then an Exception will be thrown. By using a closure for the action, you can easily make a library function out of this.
        Hide
        Scott Vlaminck added a comment -

        I voted for this issue for the reason you both mentioned in your last comments. My concern is that the metaclass is changed based on the property accessor. I would be comfortable with the functionality if it were changed through the setter or through ExpandoMetaClass.enableGlobally(), but changing it based on access to the get method is troublesome.

        Show
        Scott Vlaminck added a comment - I voted for this issue for the reason you both mentioned in your last comments. My concern is that the metaclass is changed based on the property accessor. I would be comfortable with the functionality if it were changed through the setter or through ExpandoMetaClass.enableGlobally(), but changing it based on access to the get method is troublesome.
        Hide
        blackdrag blackdrag added a comment -

        Scott, if the object has a metaClass already, then using the property on the class won't change that. the per instance metaClass is only changed from MetaClassImpl if set to null, or not yet used and the default MetaClass registered for the class is no longer MetaClassImpl. So once it has a Metaclass the property accessor won't change it.

        Show
        blackdrag blackdrag added a comment - Scott, if the object has a metaClass already, then using the property on the class won't change that. the per instance metaClass is only changed from MetaClassImpl if set to null, or not yet used and the default MetaClass registered for the class is no longer MetaClassImpl. So once it has a Metaclass the property accessor won't change it.
        Hide
        Scott Vlaminck added a comment -

        Jochen, I understand that, I was just emphasizing Robert's and your point that "property accessors shouldn't have side effects."

        Show
        Scott Vlaminck added a comment - Jochen, I understand that, I was just emphasizing Robert's and your point that "property accessors shouldn't have side effects."
        Hide
        Marc Palmer added a comment -

        Guys this is very painful. There has to be a better way. We spent ages on this problem over the last week. We wanted to cry. ExpandoMetaClass.enableGlobally() works but... one shouldn't have to! I thought it was the new default for Groovy.

        Show
        Marc Palmer added a comment - Guys this is very painful. There has to be a better way. We spent ages on this problem over the last week. We wanted to cry. ExpandoMetaClass.enableGlobally() works but... one shouldn't have to! I thought it was the new default for Groovy.
        Hide
        blackdrag blackdrag added a comment -

        We intend to use a variant of EMC as default for Groovy 2.0, but not for 1.6 or 1.7

        Show
        blackdrag blackdrag added a comment - We intend to use a variant of EMC as default for Groovy 2.0, but not for 1.6 or 1.7
        Hide
        Robert Fischer added a comment -

        My standing recommendation is to put "ExpandoMetaClass.enableGlobally()" at the beginning of any Groovy script where you do EMC work. It's like the first four words of any Perl script – "use static; use warnings;"

        Show
        Robert Fischer added a comment - My standing recommendation is to put "ExpandoMetaClass.enableGlobally()" at the beginning of any Groovy script where you do EMC work. It's like the first four words of any Perl script – "use static; use warnings;"

          People

          • Assignee:
            blackdrag blackdrag
            Reporter:
            Robert Fischer
          • Votes:
            2 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: