Groovy
  1. Groovy
  2. GROOVY-4069

Custom constructors added via metaclass are sometimes not cleaned up

    Details

    • Type: Bug Bug
    • Status: 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

      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

        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
        Jochen Theodorou 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
        Jochen Theodorou 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:

              Development