groovy

Method overriding with ExpandoMetaClass is partially broken

Details

  • Type: Bug Bug
  • Status: Closed 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
  • Number of attachments :
    1

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

Ascending order - Click to sort in descending order
Hide
Permalink
blackdrag blackdrag 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
blackdrag blackdrag 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
Permalink
Guillaume Laforge 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 Laforge 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
Permalink
Guillaume Laforge 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 Laforge 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.
Hide
Permalink
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
Hide
Permalink
Graeme Rocher added a comment -

Reproducible example http://groovyconsole.appspot.com/script/440004

Show
Graeme Rocher added a comment - Reproducible example http://groovyconsole.appspot.com/script/440004
Hide
Permalink
Guillaume Laforge added a comment -

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

Show
Guillaume Laforge added a comment - Exactly what I needed to test my patch. And indeed, that fixes it.

People

  • Assignee:
    Guillaume Laforge
    Reporter:
    Graeme Rocher
Vote (0)
Watch (1)

Dates

  • Created:
    Updated:
    Resolved: