Groovy
  1. Groovy
  2. GROOVY-4720

Method overriding with ExpandoMetaClass is partially broken

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.8-rc-4, 1.9-beta-1, 1.7.11
    • Component/s: None
    • Labels:
      None

      Description

      There is a regression whereby you cannot override methods using ExpandoMetaClass.

      The reason is that ClosureMetaMethod.createMethodList creates an anonymous inner class of type MetaMethod and adds it to the returned List<MetaMethod> that are to be registered

      Later the isNonRealMethod(MetaMethod) check in MetaMethodIndex does this check:

          private boolean isNonRealMethod(MetaMethod method) {
              return method instanceof NewInstanceMetaMethod ||
                      method instanceof NewStaticMetaMethod ||
                      method instanceof ClosureMetaMethod ||
                      method instanceof GeneratedMetaMethod ||
                      method instanceof ClosureStaticMetaMethod ||
                      method instanceof MixinInstanceMetaMethod;
          }
      

      Since the anonymous inner MetaMethod defined in ClosureMetaMethod is not an instance of any of these types then the method is never registered in the MetaMethodIndex:

                          if (methodC == matchC) {
                              if (isNonRealMethod(method)) {
                                  list.set(found, method);
                              }
      

        Activity

        Graeme Rocher created issue -
        Hide
        Jochen Theodorou added a comment -

        Graeme, if I understand you right, then the change you are speaking of is from Dez 11 2008 and a reaction to GROOVY-2951. That means that every 1.7 version has this bug, since this one was for 1.7 beta1. You said it was an regression, but really since back then? The changes in MetaMethodIndex go back to even July 15 2008 and the class was not touched since then. Can you provide a test case?

        Show
        Jochen Theodorou added a comment - Graeme, if I understand you right, then the change you are speaking of is from Dez 11 2008 and a reaction to GROOVY-2951 . That means that every 1.7 version has this bug, since this one was for 1.7 beta1. You said it was an regression, but really since back then? The changes in MetaMethodIndex go back to even July 15 2008 and the class was not touched since then. Can you provide a test case?
        Hide
        Guillaume Delcroix added a comment -

        Graeme, any update on this one? How did you produce the problem? Do you have some sample we could have a look at?

        Show
        Guillaume Delcroix added a comment - Graeme, any update on this one? How did you produce the problem? Do you have some sample we could have a look at?
        Hide
        Guillaume Delcroix added a comment -

        Without further explanations on how to reproduce the issue, it's difficult to figure out what to do, and how to check it's the proper thing to do, despite having had several persons trying to reproduce the issue for several hours without any luck so far.
        Anyhow, according to the explanations of the JIRA issue, if we follow the idea of creating a special MetaMethod class for use in ClosureMetaMethod#createMethodList() instead of the anonymous inner class, and then referencing that class in the instanceof checks of MetaMethodIndex#isNonRealMethod(), then all tests still pass.
        So I've created such a patch, and am attaching it here.
        It would be good if you could test it, and better if you could provide a test case showing the problem.

        Show
        Guillaume Delcroix added a comment - Without further explanations on how to reproduce the issue, it's difficult to figure out what to do, and how to check it's the proper thing to do, despite having had several persons trying to reproduce the issue for several hours without any luck so far. Anyhow, according to the explanations of the JIRA issue, if we follow the idea of creating a special MetaMethod class for use in ClosureMetaMethod#createMethodList() instead of the anonymous inner class, and then referencing that class in the instanceof checks of MetaMethodIndex#isNonRealMethod(), then all tests still pass. So I've created such a patch, and am attaching it here. It would be good if you could test it, and better if you could provide a test case showing the problem.
        Guillaume Delcroix made changes -
        Field Original Value New Value
        Attachment GROOVY-4720__Method_overriding_with_ExpandoMetaClass_is_partially_broken.patch [ 54254 ]
        Guillaume Delcroix made changes -
        Fix Version/s 1.8-rc-3 [ 17228 ]
        Fix Version/s 1.8-rc-4 [ 17245 ]
        Hide
        Graeme Rocher added a comment -

        Jochen/Guillaume - The regression is not new and I have a workaround so maybe it can be downgraded from blocker. I uncovered the regression as part of the new GORM/NoSQL work.

        I will try out the patch Guillaume and feedback asap, although the regression is old I'm sure we can agree this is the wrong behavior and should be fixed

        Show
        Graeme Rocher added a comment - Jochen/Guillaume - The regression is not new and I have a workaround so maybe it can be downgraded from blocker. I uncovered the regression as part of the new GORM/NoSQL work. I will try out the patch Guillaume and feedback asap, although the regression is old I'm sure we can agree this is the wrong behavior and should be fixed
        Guillaume Delcroix made changes -
        Priority Blocker [ 1 ] Critical [ 2 ]
        Hide
        Graeme Rocher added a comment -
        Show
        Graeme Rocher added a comment - Reproducible example http://groovyconsole.appspot.com/script/440004
        Hide
        Guillaume Delcroix added a comment -

        Exactly what I needed to test my patch.
        And indeed, that fixes it.

        Show
        Guillaume Delcroix added a comment - Exactly what I needed to test my patch. And indeed, that fixes it.
        Guillaume Delcroix made changes -
        Assignee Guillaume Laforge [ guillaume ]
        Guillaume Delcroix made changes -
        Fix Version/s 1.9-beta-1 [ 17153 ]
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Fix Version/s 1.7.11 [ 17244 ]
        Paul King made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Mark Thomas made changes -
        Project Import Sun Apr 05 13:32:57 UTC 2015 [ 1428240777691 ]
        Mark Thomas made changes -
        Workflow jira [ 12733650 ] Default workflow, editable Closed status [ 12745461 ]
        Mark Thomas made changes -
        Project Import Mon Apr 06 02:11:23 UTC 2015 [ 1428286283443 ]
        Mark Thomas made changes -
        Workflow jira [ 12973439 ] Default workflow, editable Closed status [ 12980670 ]
        Transition Time In Source Status Execution Times Last Executer Last Execution Date
        Open Open Resolved Resolved
        13d 4h 34m 1 Guillaume Delcroix 24/Mar/11 09:31
        Resolved Resolved Closed Closed
        20d 5h 2m 1 Paul King 13/Apr/11 15:33

          People

          • Assignee:
            Guillaume Delcroix
            Reporter:
            Graeme Rocher
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development