groovy
  1. groovy
  2. GROOVY-5009

Problem dispatching array as argument to a closure

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.2
    • Fix Version/s: 2.1.0-rc-1, 1.8.9, 2.0.7
    • Component/s: None
    • Labels:
      None
    • Number of attachments :
      0

      Description

      myscript.groovy
      ExpandoMetaClass.enableGlobally()
      
      Object[] myObjectArray = ['a', 'b'] as Object[]
      
      closure = {
      	println 'closure running...'
      }
      	
      closure(myObjectArray)
      
      code $ groovy -version
      Groovy Version: 1.8.2 JVM: 1.6.0_26
      code $ groovy myscript.groovy 
      Caught: groovy.lang.MissingMethodException: No signature of method: myscript$_run_closure1.doCall() is applicable for argument types: (java.lang.String, java.lang.String) values: [a, b]
      Possible solutions: doCall(), doCall(java.lang.Object), call(), call([Ljava.lang.Object;), call(java.lang.Object), findAll()
      groovy.lang.MissingMethodException: No signature of method: myscript$_run_closure1.doCall() is applicable for argument types: (java.lang.String, java.lang.String) values: [a, b]
      Possible solutions: doCall(), doCall(java.lang.Object), call(), call([Ljava.lang.Object;), call(java.lang.Object), findAll()
      	at myscript.run(myscript.groovy:9)
      

      I have tested with versions all the way back to 1.7.0 and the exception is thrown for all the versions I tested.

      If I remove the ExpandoMetaClass.enableGlobally(), the exception goes away.

      I think this is related to http://jira.grails.org/browse/GRAILS-8002, though I am not sure why that problem just showed up in Grails 2.0-M2. I have not been able to reproduce it with Grails 1.3.7.

        Activity

        Hide
        Roshan Dawrani added a comment -

        Looked a bit into it and obviously the problem is related to how ClosureMetaClass(CMC) and ExpandoMetaClass choose the method to invoke.

        1) The ClosureMetaClass chooses the method to invoke via StandardClosureChooser as it finds the following two methods burnt into the class and restricts its method selection to these two going just by the number of arguments (and selects "doCall(Object)" when "closure(myObjectArray)" call is made:

        - public Object doCall()
        - public Object doCall(Object it)
        

        2) On the other hand, the EMC makes its choice from among the following methods and chooses Closure.call(Object... args) as a more exact match:

        - public Object groovy.lang.Closure.call()
        - public Object groovy.lang.Closure.call(java.lang.Object[])
        - public Object groovy.lang.Closure.call(java.lang.Object)
        

        I think the difference in behavior is arising because groovy.lang.Closure has an extra "call(Object...)" and its counterpart "doCall(Object...)" is missing in closure's bytecode. Shouln't these two sets of methods be in sync always? Once they are, we should have consistent behavior one way or another, whether CMC is used, or EMC.

        Show
        Roshan Dawrani added a comment - Looked a bit into it and obviously the problem is related to how ClosureMetaClass(CMC) and ExpandoMetaClass choose the method to invoke. 1) The ClosureMetaClass chooses the method to invoke via StandardClosureChooser as it finds the following two methods burnt into the class and restricts its method selection to these two going just by the number of arguments (and selects "doCall(Object)" when "closure(myObjectArray)" call is made: - public Object doCall() - public Object doCall(Object it) 2) On the other hand, the EMC makes its choice from among the following methods and chooses Closure.call(Object... args) as a more exact match: - public Object groovy.lang.Closure.call() - public Object groovy.lang.Closure.call(java.lang.Object[]) - public Object groovy.lang.Closure.call(java.lang.Object) I think the difference in behavior is arising because groovy.lang.Closure has an extra "call(Object...)" and its counterpart "doCall(Object...)" is missing in closure's bytecode. Shouln't these two sets of methods be in sync always? Once they are, we should have consistent behavior one way or another, whether CMC is used, or EMC.
        Hide
        Roshan Dawrani added a comment -

        Apart from the above mentioned inconsistency on Groovy side, there is another inconsistency on Java side that is in play - related to how the direct dispatch in it differs from a reflection based dispatch:

        import java.lang.reflect.Method;
        
        public class TryInJava {
            public static void main(String[] args) throws Exception {
                Object[] strs = new Object[] {"a", "b"};
                Object[] inputs = new Object[] {strs};
                Method m = Foo.class.getMethod("call", Object[].class);
                Foo foo = new Foo();
                foo.call(inputs); // this prints "1" as the inputs array reaches method "call" wrapped properly - as {{"a", "b"}}.
                m.invoke(foo, inputs); // this prints "2" as now the inputs array reaches method "call" unwrapped - as {"a", "b"}.
            }
        }
        
        
        class Foo {
            public void call(Object[] inputs) {
                System.out.println(inputs.length);
            }
        }
        

        Since we cannot remove call(Object... args)/call(Object[]) from public API of groovy.lang.Closure and since Groovy has to do a reflection based call, removing the Groovy side inconsistency may result in failure to dispatch the array to a closure in both CMC and EMC cases - unless we then treat it as some special case.

        Show
        Roshan Dawrani added a comment - Apart from the above mentioned inconsistency on Groovy side, there is another inconsistency on Java side that is in play - related to how the direct dispatch in it differs from a reflection based dispatch: import java.lang.reflect.Method; public class TryInJava { public static void main( String [] args) throws Exception { Object [] strs = new Object [] { "a" , "b" }; Object [] inputs = new Object [] {strs}; Method m = Foo.class.getMethod( "call" , Object [].class); Foo foo = new Foo(); foo.call(inputs); // this prints "1" as the inputs array reaches method "call" wrapped properly - as {{ "a" , "b" }}. m.invoke(foo, inputs); // this prints "2" as now the inputs array reaches method "call" unwrapped - as { "a" , "b" }. } } class Foo { public void call( Object [] inputs) { System .out.println(inputs.length); } } Since we cannot remove call(Object... args)/call(Object[]) from public API of groovy.lang.Closure and since Groovy has to do a reflection based call, removing the Groovy side inconsistency may result in failure to dispatch the array to a closure in both CMC and EMC cases - unless we then treat it as some special case.
        Hide
        blackdrag blackdrag added a comment -

        I think the solution is to make EMC doCall instead of call

        Show
        blackdrag blackdrag added a comment - I think the solution is to make EMC doCall instead of call
        Hide
        blackdrag blackdrag added a comment -

        call is intended to be a kind of dispatcher method and actually behaving as it should, even though that makes no sense if called directly in Grovy

        Show
        blackdrag blackdrag added a comment - call is intended to be a kind of dispatcher method and actually behaving as it should, even though that makes no sense if called directly in Grovy
        Hide
        Graeme Rocher added a comment -

        Can we prioritize a fix on this? I would like to get it tested against Grails as I'm concerned about what other regressions could be caused by making a change in behavior like this. In particular I'm concerned about meta-programming logic in EMC and whether it will be impacted by this change.

        For example what will be the impact on methodMissing logic like this which currently works:

        https://github.com/SpringSource/grails-data-mapping/blob/44031e133d3f7d3c93f575c38b4038bee246292c/grails-datastore-gorm/src/main/groovy/org/grails/datastore/gorm/GormStaticApi.groovy#L102

        Anyway regardless I think it is important we get this change in soon so we can do some appropriate testing

        Show
        Graeme Rocher added a comment - Can we prioritize a fix on this? I would like to get it tested against Grails as I'm concerned about what other regressions could be caused by making a change in behavior like this. In particular I'm concerned about meta-programming logic in EMC and whether it will be impacted by this change. For example what will be the impact on methodMissing logic like this which currently works: https://github.com/SpringSource/grails-data-mapping/blob/44031e133d3f7d3c93f575c38b4038bee246292c/grails-datastore-gorm/src/main/groovy/org/grails/datastore/gorm/GormStaticApi.groovy#L102 Anyway regardless I think it is important we get this change in soon so we can do some appropriate testing
        Hide
        blackdrag blackdrag added a comment -

        should be fixed now

        Show
        blackdrag blackdrag added a comment - should be fixed now
        Hide
        Graeme Rocher added a comment -

        This still seems to be an issue. Code to reproduce:

        ExpandoMetaClass.enableGlobally()
        def args = [1, 2] as Object[]
        def c = {}
        c(args)
        

        Exception:

        groovy.lang.MissingMethodException: No signature of method: ConsoleScript6$_run_closure1.doCall() is applicable for argument types: (java.lang.Integer, java.lang.Integer) values: [1, 2]
        Possible solutions: doCall(), doCall(java.lang.Object), call(), call([Ljava.lang.Object;), call(java.lang.Object), findAll()
        	at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.unwrap(ScriptBytecodeAdapter.java:55)
        	at groovy.lang.Closure$call.call(Unknown Source)
        	at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:42)
        	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:108)
        	at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:116)
        	at ConsoleScript6.run(ConsoleScript6:4)
        	at groovy.lang.GroovyShell.runScriptOrMainOrTestOrRunnable(GroovyShell.java:266)
        	
        
        Show
        Graeme Rocher added a comment - This still seems to be an issue. Code to reproduce: ExpandoMetaClass.enableGlobally() def args = [1, 2] as Object [] def c = {} c(args) Exception: groovy.lang.MissingMethodException: No signature of method: ConsoleScript6$_run_closure1.doCall() is applicable for argument types: (java.lang. Integer , java.lang. Integer ) values: [1, 2] Possible solutions: doCall(), doCall(java.lang. Object ), call(), call([Ljava.lang. Object ;), call(java.lang. Object ), findAll() at org.codehaus.groovy.runtime.ScriptBytecodeAdapter.unwrap(ScriptBytecodeAdapter.java:55) at groovy.lang.Closure$call.call(Unknown Source) at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:42) at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:108) at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:116) at ConsoleScript6.run(ConsoleScript6:4) at groovy.lang.GroovyShell.runScriptOrMainOrTestOrRunnable(GroovyShell.java:266)
        Hide
        Jeff Scott Brown added a comment -

        This looks like it may be a regression introduced in 1.8.1.

        doit.groovy
        ExpandoMetaClass.enableGlobally()
        List l = new ArrayList()
        l << ([1, 2] as Object[])
        
        l.each {
        	println "it is ${it}"
        }
        

        With 1.8.0...

        grv $ groovy -version
        Groovy Version: 1.8.0 JVM: 1.6.0_26
        grv $ groovy doit.groovy 
        it is [1, 2]
        grv $ 
        

        With 1.8.1...

        grv $ groovy -version
        gGroovy Version: 1.8.1 JVM: 1.6.0_26
        grv $ groovy doit.groovy 
        Caught: groovy.lang.MissingMethodException: No signature of method: doit$_run_closure1.doCall() is applicable for argument types: (java.lang.Integer, java.lang.Integer) values: [1, 2]
        Possible solutions: doCall(), doCall(java.lang.Object), call(), call([Ljava.lang.Object;), call(java.lang.Object), findAll()
        groovy.lang.MissingMethodException: No signature of method: doit$_run_closure1.doCall() is applicable for argument types: (java.lang.Integer, java.lang.Integer) values: [1, 2]
        Possible solutions: doCall(), doCall(java.lang.Object), call(), call([Ljava.lang.Object;), call(java.lang.Object), findAll()
        	at doit.run(doit.groovy:5)
        grv $ 
        

        With 1.8.2...

        grv $ groovy -version
        grGroovy Version: 1.8.2 JVM: 1.6.0_26
        ogrv $ groovy doit.groovy 
        Caught: groovy.lang.MissingMethodException: No signature of method: doit$_run_closure1.doCall() is applicable for argument types: (java.lang.Integer, java.lang.Integer) values: [1, 2]
        Possible solutions: doCall(), doCall(java.lang.Object), call(), call([Ljava.lang.Object;), call(java.lang.Object), findAll()
        groovy.lang.MissingMethodException: No signature of method: doit$_run_closure1.doCall() is applicable for argument types: (java.lang.Integer, java.lang.Integer) values: [1, 2]
        Possible solutions: doCall(), doCall(java.lang.Object), call(), call([Ljava.lang.Object;), call(java.lang.Object), findAll()
        	at doit.run(doit.groovy:5)
        grv $ 
        
        Show
        Jeff Scott Brown added a comment - This looks like it may be a regression introduced in 1.8.1. doit.groovy ExpandoMetaClass.enableGlobally() List l = new ArrayList() l << ([1, 2] as Object []) l.each { println "it is ${it}" } With 1.8.0... grv $ groovy -version Groovy Version: 1.8.0 JVM: 1.6.0_26 grv $ groovy doit.groovy it is [1, 2] grv $ With 1.8.1... grv $ groovy -version gGroovy Version: 1.8.1 JVM: 1.6.0_26 grv $ groovy doit.groovy Caught: groovy.lang.MissingMethodException: No signature of method: doit$_run_closure1.doCall() is applicable for argument types: (java.lang.Integer, java.lang.Integer) values: [1, 2] Possible solutions: doCall(), doCall(java.lang.Object), call(), call([Ljava.lang.Object;), call(java.lang.Object), findAll() groovy.lang.MissingMethodException: No signature of method: doit$_run_closure1.doCall() is applicable for argument types: (java.lang.Integer, java.lang.Integer) values: [1, 2] Possible solutions: doCall(), doCall(java.lang.Object), call(), call([Ljava.lang.Object;), call(java.lang.Object), findAll() at doit.run(doit.groovy:5) grv $ With 1.8.2... grv $ groovy -version grGroovy Version: 1.8.2 JVM: 1.6.0_26 ogrv $ groovy doit.groovy Caught: groovy.lang.MissingMethodException: No signature of method: doit$_run_closure1.doCall() is applicable for argument types: (java.lang.Integer, java.lang.Integer) values: [1, 2] Possible solutions: doCall(), doCall(java.lang.Object), call(), call([Ljava.lang.Object;), call(java.lang.Object), findAll() groovy.lang.MissingMethodException: No signature of method: doit$_run_closure1.doCall() is applicable for argument types: (java.lang.Integer, java.lang.Integer) values: [1, 2] Possible solutions: doCall(), doCall(java.lang.Object), call(), call([Ljava.lang.Object;), call(java.lang.Object), findAll() at doit.run(doit.groovy:5) grv $
        Hide
        blackdrag blackdrag added a comment -

        the original example of this issue still holds, but is not ot be fixed for 1.8 or 1.7 and is not o be fixed for Grails as well

        Show
        blackdrag blackdrag added a comment - the original example of this issue still holds, but is not ot be fixed for 1.8 or 1.7 and is not o be fixed for Grails as well
        Hide
        Graeme Rocher added a comment -

        Any update on this issue?

        Show
        Graeme Rocher added a comment - Any update on this issue?
        Hide
        Graeme Rocher added a comment -

        Guys are there any plans to fix this?

        Show
        Graeme Rocher added a comment - Guys are there any plans to fix this?
        Hide
        blackdrag blackdrag added a comment - - edited

        The problem is how to fix this. We cannot fix this without breaking code at another place. The call(Object...) could be called with call('a','b') or with def x=['a','b'] as Object[]; call(x). For the call method they are the same, but for the doCall methods they are not!

        For example... let us say we add a doCall(Object...) method, then either we would let it do what call does and in the end have the same problem and nothing fixed, or we change the type of the implicit it to be an Object[] instead of being an Object. The disadvantages of this are the semantic change for the much implicit it parameter, which will break a lot of code, and that you will then never get a MissingMethodException from the closure unless you gave in the type yourself. And even then it won't be always correct... assuming you have cl=

        {a,b,->...}

        , and then do cl('a','b') you expect this to work. So far so good... but cl(x), with x being the Object[] will still also call that one

        The other possible way would be to do what ClosureMetaClass does and not even think about calling the call method, but instead directing to doCall. the result for me: 11 failures and 401 errors in the build.

        Show
        blackdrag blackdrag added a comment - - edited The problem is how to fix this. We cannot fix this without breaking code at another place. The call(Object...) could be called with call('a','b') or with def x= ['a','b'] as Object[]; call(x). For the call method they are the same, but for the doCall methods they are not! For example... let us say we add a doCall(Object...) method, then either we would let it do what call does and in the end have the same problem and nothing fixed, or we change the type of the implicit it to be an Object[] instead of being an Object. The disadvantages of this are the semantic change for the much implicit it parameter, which will break a lot of code, and that you will then never get a MissingMethodException from the closure unless you gave in the type yourself. And even then it won't be always correct... assuming you have cl= {a,b,->...} , and then do cl('a','b') you expect this to work. So far so good... but cl(x), with x being the Object[] will still also call that one The other possible way would be to do what ClosureMetaClass does and not even think about calling the call method, but instead directing to doCall. the result for me: 11 failures and 401 errors in the build.
        Hide
        blackdrag blackdrag added a comment -

        this is reported as fixed in 1.8.3

        Show
        blackdrag blackdrag added a comment - this is reported as fixed in 1.8.3
        Hide
        Jeff Scott Brown added a comment -
        ~ $ groovy -version
        Groovy Version: 1.8.8 JVM: 1.6.0_37 Vendor: Apple Inc. OS: Mac OS X
        ~ $ 
        ~ $ cat doit.groovy 
        ExpandoMetaClass.enableGlobally()
        
        Object[] myObjectArray = ['a', 'b'] as Object[]
        
        closure = {
            println 'closure running...'
        }
        
        closure(myObjectArray)
        
        ~ $ 
        ~ $ groovy doit.groovy 
        Caught: groovy.lang.MissingMethodException: No signature of method: doit$_run_closure1.doCall() is applicable for argument types: (java.lang.String, java.lang.String) values: [a, b]
        Possible solutions: doCall(), doCall(java.lang.Object), call(), call([Ljava.lang.Object;), call(java.lang.Object), findAll()
        groovy.lang.MissingMethodException: No signature of method: doit$_run_closure1.doCall() is applicable for argument types: (java.lang.String, java.lang.String) values: [a, b]
        Possible solutions: doCall(), doCall(java.lang.Object), call(), call([Ljava.lang.Object;), call(java.lang.Object), findAll()
        	at doit.run(doit.groovy:9)
        ~ $ 
        
        Show
        Jeff Scott Brown added a comment - ~ $ groovy -version Groovy Version: 1.8.8 JVM: 1.6.0_37 Vendor: Apple Inc. OS: Mac OS X ~ $ ~ $ cat doit.groovy ExpandoMetaClass.enableGlobally() Object[] myObjectArray = ['a', 'b'] as Object[] closure = { println 'closure running...' } closure(myObjectArray) ~ $ ~ $ groovy doit.groovy Caught: groovy.lang.MissingMethodException: No signature of method: doit$_run_closure1.doCall() is applicable for argument types: (java.lang.String, java.lang.String) values: [a, b] Possible solutions: doCall(), doCall(java.lang.Object), call(), call([Ljava.lang.Object;), call(java.lang.Object), findAll() groovy.lang.MissingMethodException: No signature of method: doit$_run_closure1.doCall() is applicable for argument types: (java.lang.String, java.lang.String) values: [a, b] Possible solutions: doCall(), doCall(java.lang.Object), call(), call([Ljava.lang.Object;), call(java.lang.Object), findAll() at doit.run(doit.groovy:9) ~ $
        Hide
        blackdrag blackdrag added a comment -

        reopen for now till I verified it is really fixed in 1.8.9

        Show
        blackdrag blackdrag added a comment - reopen for now till I verified it is really fixed in 1.8.9
        Hide
        Jeff Scott Brown added a comment -

        Should we remove 1.8.3 from the "Fix Version" field?

        Show
        Jeff Scott Brown added a comment - Should we remove 1.8.3 from the "Fix Version" field?
        Hide
        blackdrag blackdrag added a comment -

        looks like I made a fix, but then did not apply it to 2.0.x and 1.8.x

        Show
        blackdrag blackdrag added a comment - looks like I made a fix, but then did not apply it to 2.0.x and 1.8.x
        Hide
        blackdrag blackdrag added a comment -

        I found the fix I did back then and that it is indeed in master and 2.1.0-RC-x, but is missing on 1.8.x and 2.0.x. So I pushed the change there as well and close the issue with that

        Show
        blackdrag blackdrag added a comment - I found the fix I did back then and that it is indeed in master and 2.1.0-RC-x, but is missing on 1.8.x and 2.0.x. So I pushed the change there as well and close the issue with that

          People

          • Assignee:
            blackdrag blackdrag
            Reporter:
            Jeff Scott Brown
          • Votes:
            9 Vote for this issue
            Watchers:
            9 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: