groovy
  1. groovy
  2. GROOVY-4069

Custom constructors added via metaclass are sometimes not cleaned up

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.7.1
    • Fix Version/s: 1.7.2, 1.8-beta-1
    • Component/s: None
    • Labels:
      None
    • Testcase included:
      yes
    • Number of attachments :
      2

      Description

      There seems to be an issue with constructors added via metaclass and cleanup:

      class MetaClassMagicTests extends GroovyTestCase {
          void testIt() {
              ExpandoMetaClass.enableGlobally()
              
              // Child.metaClass // uncomment and everything works
              
              // Save the old meta class
              def oldMetaClass = Parent.metaClass
              
              // Install a new one
              def emc = new ExpandoMetaClass(Parent, true, true)
              emc.initialize()
              GroovySystem.metaClassRegistry.setMetaClass(Parent, emc)
              
              // Install a map based constructor
              emc.constructor = { Map m -> Parent.newInstance() }
              
              // Parent constructor is used, all good
              assert new Child([:]) instanceof Parent
              
              // Reinstate the old meta class
              GroovySystem.metaClassRegistry.removeMetaClass(Parent) 
              GroovySystem.metaClassRegistry.setMetaClass(Parent, oldMetaClass)
      
              // This fails, this calls the custom constructor from above and returns an instance of Parent
              assert new Child([:]) instanceof Child
          }
          
      }
      
      class Parent { def a }
      class Child extends Parent { def b }
      
      1. 4069_v18x_Patch.txt
        8 kB
        Roshan Dawrani
      2. V2_4069_v18x_Patch.txt
        6 kB
        Roshan Dawrani

        Issue Links

          Activity

          Hide
          Roshan Dawrani added a comment -

          Isn't it the Child metaClass that you should be manipulating instead of Parent's?

          The following code goes through:

          class MetaClassMagicTests extends GroovyTestCase {
              void testIt() {
                  ExpandoMetaClass.enableGlobally()
                  
                  def oldMetaClass = Child.metaClass
                  
                  def emc = new ExpandoMetaClass(Child, true, true)
                  emc.initialize()
                  GroovySystem.metaClassRegistry.setMetaClass(Child, emc)
                  
                  emc.constructor = { Map m -> Child.newInstance() }
                  
                  assert new Child([:]) instanceof Parent
                  
                  GroovySystem.metaClassRegistry.removeMetaClass(Child) 
                  GroovySystem.metaClassRegistry.setMetaClass(Child, oldMetaClass)
          
                  assert new Child([:]) instanceof Child
              }
              
          }
          
          class Parent { def a }
          class Child extends Parent { def b }
          
          Show
          Roshan Dawrani added a comment - Isn't it the Child metaClass that you should be manipulating instead of Parent's? The following code goes through: class MetaClassMagicTests extends GroovyTestCase { void testIt() { ExpandoMetaClass.enableGlobally() def oldMetaClass = Child.metaClass def emc = new ExpandoMetaClass(Child, true , true ) emc.initialize() GroovySystem.metaClassRegistry.setMetaClass(Child, emc) emc.constructor = { Map m -> Child.newInstance() } assert new Child([:]) instanceof Parent GroovySystem.metaClassRegistry.removeMetaClass(Child) GroovySystem.metaClassRegistry.setMetaClass(Child, oldMetaClass) assert new Child([:]) instanceof Child } } class Parent { def a } class Child extends Parent { def b }
          Hide
          Luke Daley added a comment -

          No, the point is that after I have removed the constructor from the parent's metaclass, it is still called when constructing the child. This is definitely unexpected.

          Note the...

          // Child.metaClass // uncomment and everything works
          

          line. If that get's executed then everything works, which seems like that if the Child metaClass is initialised before diddling with the Parent's metaClass you get the right behaviour.

          Show
          Luke Daley added a comment - No, the point is that after I have removed the constructor from the parent's metaclass, it is still called when constructing the child. This is definitely unexpected. Note the... // Child.metaClass // uncomment and everything works line. If that get's executed then everything works, which seems like that if the Child metaClass is initialised before diddling with the Parent's metaClass you get the right behaviour.
          Hide
          Roshan Dawrani added a comment - - edited

          While your point about inconsistency is right, I think your asserts convey the wrong expectations (as far as I understand metaClass thing). I think when you add a constructor through Parent EMC, "new Child()" should not get that constructor.

          So, the following should be the behavior:

          ExpandoMetaClass.enableGlobally()
          
          def oldMetaClass = Parent.metaClass
          
          def emc = new ExpandoMetaClass(Parent, true, true)
          emc.initialize()
          GroovySystem.metaClassRegistry.setMetaClass(Parent, emc)
          
          emc.constructor = { Map m -> Parent.newInstance() }
          
          /* here also you should get a new Child because you have manipulated Parent MC */
          assert new Child([:]) instanceof Child
          
          GroovySystem.metaClassRegistry.removeMetaClass(Parent) 
          GroovySystem.metaClassRegistry.setMetaClass(Parent, oldMetaClass)
          
          assert new Child([:]) instanceof Child
          
          class Parent { def a }
          class Child extends Parent { def b }
          

          For the behavior that you are trying, you should manipulate Child MC.

          Can someone please verify the behavior - one way or another? Thanks.

          Show
          Roshan Dawrani added a comment - - edited While your point about inconsistency is right, I think your asserts convey the wrong expectations (as far as I understand metaClass thing). I think when you add a constructor through Parent EMC, "new Child()" should not get that constructor. So, the following should be the behavior: ExpandoMetaClass.enableGlobally() def oldMetaClass = Parent.metaClass def emc = new ExpandoMetaClass(Parent, true , true ) emc.initialize() GroovySystem.metaClassRegistry.setMetaClass(Parent, emc) emc.constructor = { Map m -> Parent.newInstance() } /* here also you should get a new Child because you have manipulated Parent MC */ assert new Child([:]) instanceof Child GroovySystem.metaClassRegistry.removeMetaClass(Parent) GroovySystem.metaClassRegistry.setMetaClass(Parent, oldMetaClass) assert new Child([:]) instanceof Child class Parent { def a } class Child extends Parent { def b } For the behavior that you are trying, you should manipulate Child MC. Can someone please verify the behavior - one way or another? Thanks.
          Hide
          Roshan Dawrani added a comment -

          Attaching a patch for review based on my understanding of the behavior in this scenario.

          Basically the change made in the patch is that EMC should not create a constructor site if the type <init> has been invoked on does not match the type it finds <init> on at runtime. That way when "Parent" defines a new constructor through EMC, "new Child()" will not match it.

          Tests are added to verify the behavior when you manipulate the MC of Parent and of Child and variations "ChildX.metaClass" commented/uncommented that lead to the inconsistent behavior.

          Show
          Roshan Dawrani added a comment - Attaching a patch for review based on my understanding of the behavior in this scenario. Basically the change made in the patch is that EMC should not create a constructor site if the type <init> has been invoked on does not match the type it finds <init> on at runtime. That way when "Parent" defines a new constructor through EMC, "new Child()" will not match it. Tests are added to verify the behavior when you manipulate the MC of Parent and of Child and variations "ChildX.metaClass" commented/uncommented that lead to the inconsistent behavior.
          Hide
          blackdrag blackdrag added a comment -

          hmm... is the type parameter really needed? I assume the declaring class of a method for a method added by closure will always not be the same as the current class. So I wonder if it wouldn't be enough to change

          (type == null) || (method.getDeclaringClass().getTheClass().equals(type)
          

          to

          method.getDeclaringClass().getTheClass().equals(getTheClass())
          

          As I understand it, this will always false if the method is added through a closure... or is getTheType() returning something else than what "type" would get?

          Show
          blackdrag blackdrag added a comment - hmm... is the type parameter really needed? I assume the declaring class of a method for a method added by closure will always not be the same as the current class. So I wonder if it wouldn't be enough to change (type == null ) || (method.getDeclaringClass().getTheClass().equals(type) to method.getDeclaringClass().getTheClass().equals(getTheClass()) As I understand it, this will always false if the method is added through a closure... or is getTheType() returning something else than what "type" would get?
          Hide
          Roshan Dawrani added a comment -

          Thanks for the feedback, Jochen. I didn't notice that I could get the type from MC#getTheClass() as well.

          I have made that change and I am attaching V2 of the patch here.

          Please let me know if you see any other issue.

          Show
          Roshan Dawrani added a comment - Thanks for the feedback, Jochen. I didn't notice that I could get the type from MC#getTheClass() as well. I have made that change and I am attaching V2 of the patch here. Please let me know if you see any other issue.
          Hide
          Roshan Dawrani added a comment -

          Fixed

          Show
          Roshan Dawrani added a comment - Fixed

            People

            • Assignee:
              Roshan Dawrani
              Reporter:
              Luke Daley
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: