Uploaded image for project: 'Groovy'
  1. Groovy
  2. GROOVY-7772

Class.&instanceMethod had better to have same meaning of Class::instanceMethod of Java8

    Details

    • Type: Improvement
    • Status: Reopened
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.4.6
    • Fix Version/s: 2.6.0-alpha-1
    • Component/s: groovy-runtime
    • Labels:
      None

      Description

      Groovy's operator .& for method is similar functionality to Java8's method reference operator ::.

      No. lhs rhs meaing of Groovy's .& (Closure) meaning of java8's :: (FunctionalInterface)
      1 instance instanceMethod { ..args -> instance.instanceMethod(args) same as groovy
      2 Class staticMethod { ..args -> Class.staticMethod(args) same as groovy
      3 instance staticMethod ERROR groovy.lang.MissingMethodException: Error same as groovy (compile error)
      4 Class instanceMethod error Function<RetType,Class,Args..>, where method instance method of Class which is declared as ```RetType instanceMethod(Args..) {...}

      ```. In other words it is interpreted as a function which takes LHS Class as the first parameter which additionally inserted to the method.)

      IMHO, i'd like to propose to change the No 4 pattern semantics of groovy same as Java 8 's. Because:

      • You can write:
        ["a,b,c"].collect ( String.&toUpperCase )
        

        instaed of

        ["a,b,c"].collect { it.toUpperCase() }
        
      • Can have correspond operator to java8's ::. which is understandablea and needed for Java programmers.

        Activity

        Hide
        blackdrag Jochen Theodorou added a comment - - edited

        I think I don't understand pattern number 3 actually... I would have thought you are talking about something like this:

        class X {
          static foo(){}
        }
        
        def x = new X()
        def m = x.&foo
        m.call()
        

        but this does not fail, not dynamic compiled and not static compiled. The meaning of x.&y() is in all cases always x.y(), be x a class, y a static method or not and combinations of it. Which is why pattern 4 fails. What happens if we see x.&y(args...) like a method taking X and args..., for which we curry (in the Groovy sense) the first argument with x? In other words: x.&y becomes

        {xarg, args... -> xarg.y(args)}

        .curry( x ), then all cases work like before, because that is actually what we are doing (kind of). A more Java-like version would be to have

        {xarg, args... -> if (y is static) X.y(args) else xarg.y(args)}

        .curry( x ). Then all 4 cases would work.

        Show
        blackdrag Jochen Theodorou added a comment - - edited I think I don't understand pattern number 3 actually... I would have thought you are talking about something like this: class X { static foo(){} } def x = new X() def m = x.&foo m.call() but this does not fail, not dynamic compiled and not static compiled. The meaning of x.&y() is in all cases always x.y(), be x a class, y a static method or not and combinations of it. Which is why pattern 4 fails. What happens if we see x.&y(args...) like a method taking X and args..., for which we curry (in the Groovy sense) the first argument with x? In other words: x.&y becomes {xarg, args... -> xarg.y(args)} .curry( x ), then all cases work like before, because that is actually what we are doing (kind of). A more Java-like version would be to have {xarg, args... -> if (y is static) X.y(args) else xarg.y(args)} .curry( x ). Then all 4 cases would work.
        Hide
        uehaj UEHARA Junji added a comment - - edited

        Sorry No 3 is wrong. It should be:

        3 instance staticMethod { ..args -> instance.staticMethod(args) } (edit) Compile time error(invalid method reference)

        You mentioned semantics with closure and curry(), I think that is:

        x.&y
          == if x instanceof Class && y is_not_static_method then
                {xarg, args... -> xarg.y(args)}
             else
                {xarg, args... -> xarg.y(args)}.curry(x)
        

        for confirmation,

        "abc".&toUpperCase() // Instance.&instanceMethod (No 1)
          == {xarg, args... -> xarg.toUpperCase(args)}.curry("abc")()
          == {args... -> "abc".toUpperCase(args)}()
          == {->"abc".toUpperCase()}()
          == "abc".toUpperCase()
          == "ABC"
        
        Integer.&parseInt("123") // Class.&staticMethod (No 2)
          == {xarg, args... -> xarg.parseInt(args)}.curry(Integer)("123")
          == {args -> Integer.parseInt(args)}("123")
          == Integer.parseInt("123")
          == 123
        
        3.&parseInt("123") // Instance.&staticMethod (No 3)
          == {xarg, args... -> xarg.parseInt(args)}.curry(3)("123")
          == {args... -> 3.parseInt(args)}("123")
          == 3.parseInt("123")
          == 123
        
        String.&toUpperCase("abc") // Class.&instanceMethod (No 4)
          == {xarg, args... -> xarg.toUpperCase(args)}("abc")
          == "abc".toUpperCase()
          == "ABC"  // this is My proposal behavior, Current groovy treat this as error.
        
        Show
        uehaj UEHARA Junji added a comment - - edited Sorry No 3 is wrong. It should be: 3 instance staticMethod { ..args -> instance.staticMethod(args) } (edit) Compile time error(invalid method reference) You mentioned semantics with closure and curry(), I think that is: x.&y == if x instanceof Class && y is_not_static_method then {xarg, args... -> xarg.y(args)} else {xarg, args... -> xarg.y(args)}.curry(x) for confirmation, "abc" .&toUpperCase() // Instance.&instanceMethod (No 1) == {xarg, args... -> xarg.toUpperCase(args)}.curry( "abc" )() == {args... -> "abc" .toUpperCase(args)}() == {-> "abc" .toUpperCase()}() == "abc" .toUpperCase() == "ABC" Integer .&parseInt( "123" ) // Class .&staticMethod (No 2) == {xarg, args... -> xarg.parseInt(args)}.curry( Integer )( "123" ) == {args -> Integer .parseInt(args)}( "123" ) == Integer .parseInt( "123" ) == 123 3.&parseInt( "123" ) // Instance.&staticMethod (No 3) == {xarg, args... -> xarg.parseInt(args)}.curry(3)( "123" ) == {args... -> 3.parseInt(args)}( "123" ) == 3.parseInt( "123" ) == 123 String .&toUpperCase( "abc" ) // Class .&instanceMethod (No 4) == {xarg, args... -> xarg.toUpperCase(args)}( "abc" ) == "abc" .toUpperCase() == "ABC" // this is My proposal behavior, Current groovy treat this as error.
        Hide
        blackdrag Jochen Theodorou added a comment - - edited

        I would actually like to keep the outside face the same, meaning we create a

        {xarg, args... ->}

        .curry( x ) every time. Instead I would like to change the logic inside... but using your logic:

        {xarg, args... ->
          if x instanceof Class && y is_not_static_method then
               args[0].y(args[1..n])
          else
              xarg.y(args)
        }
        Show
        blackdrag Jochen Theodorou added a comment - - edited I would actually like to keep the outside face the same, meaning we create a {xarg, args... ->} .curry( x ) every time. Instead I would like to change the logic inside... but using your logic: {xarg, args... -> if x instanceof Class && y is_not_static_method then args[0].y(args[1..n]) else xarg.y(args) }
        Hide
        uehaj UEHARA Junji added a comment - - edited

        If so, y also might better to be extracted to outside.

        // Definition
        x.&y
         == {xarg, yarg, args... ->
          if xarg instanceof Class && yarg is_not_static_method_on(xarg) then
               args[0].yarg(args[1..n])
          else
               xarg.yarg(args)
         }.curry(x,y)
        
        

        then,

        "abc".&toUpperCase()() // Instance instanceMethod (No 1)
          == {xarg, yarg, args... ->
          if xarg instanceof Class && yarg is_not_static_method then
               args[0].yarg(args[1..n])
          else
               xarg.yarg(args)
          }.curry("abc","toUpperCase"")()
          == {args... ->
          if "abc" instanceof Class && "toUpperCase" is_not_static_method then
               args[0].toUpperCase(args[1..n])
          else
               "abc".toUpperCase(args)
          }()
          == {args... ->
               "abc".toUpperCase(args)
          }()
          == "abc".toUpperCase()
          == "ABC"
        
        
        String.&toUpperCase()("abc") // Class instanceMethod (No 4)
          == {xarg, args... ->
          if xarg instanceof Class && y is_not_static_method then
               args[0].y(args[1..n])
          else
               xarg.y(args)
          }.curry(x)("abc")
          == {xarg, args... ->
          if xarg instanceof Class && "toUpperCase" is_not_static_method then
               args[0].toUpperCase(args[1..n])
          else
               xarg.toUpperCase(args)
          }.curry(String)("abc")
          == {args... ->
          if String instanceof Class && "toUpperCase" is_not_static_method then
               args[0].toUpperCase(args[1..n])
          else
               String.toUpperCase(args)
          }("abc")
          == {args... ->
             args[0].toUpperCase(args[1..n])
          }("abc")
          == "abc".toUpperCase()
          == "ABC"
        
        Show
        uehaj UEHARA Junji added a comment - - edited If so, y also might better to be extracted to outside. // Definition x.&y == {xarg, yarg, args... -> if xarg instanceof Class && yarg is_not_static_method_on(xarg) then args[0].yarg(args[1..n]) else xarg.yarg(args) }.curry(x,y) then, "abc" .&toUpperCase()() // Instance instanceMethod (No 1) == {xarg, yarg, args... -> if xarg instanceof Class && yarg is_not_static_method then args[0].yarg(args[1..n]) else xarg.yarg(args) }.curry( "abc" , "toUpperCase" ")() == {args... -> if "abc" instanceof Class && "toUpperCase" is_not_static_method then args[0].toUpperCase(args[1..n]) else "abc" .toUpperCase(args) }() == {args... -> "abc" .toUpperCase(args) }() == "abc" .toUpperCase() == "ABC" String .&toUpperCase()( "abc" ) // Class instanceMethod (No 4) == {xarg, args... -> if xarg instanceof Class && y is_not_static_method then args[0].y(args[1..n]) else xarg.y(args) }.curry(x)( "abc" ) == {xarg, args... -> if xarg instanceof Class && "toUpperCase" is_not_static_method then args[0].toUpperCase(args[1..n]) else xarg.toUpperCase(args) }.curry( String )( "abc" ) == {args... -> if String instanceof Class && "toUpperCase" is_not_static_method then args[0].toUpperCase(args[1..n]) else String .toUpperCase(args) }( "abc" ) == {args... -> args[0].toUpperCase(args[1..n]) }( "abc" ) == "abc" .toUpperCase() == "ABC"
        Hide
        blackdrag Jochen Theodorou added a comment -

        what do you mean by extracted to the outside for y? You mean as String or do you mean as some kind of method object? Because the later we cannot really do. As for a String... that is what the current MethodClosure does already, the static method check is done inside MethodClosure then.

        Show
        blackdrag Jochen Theodorou added a comment - what do you mean by extracted to the outside for y? You mean as String or do you mean as some kind of method object? Because the later we cannot really do. As for a String... that is what the current MethodClosure does already, the static method check is done inside MethodClosure then.
        Hide
        uehaj UEHARA Junji added a comment -

        I mean y as String.
        In Java8, Class::instanceMethod is not mean the instance method is actually method of the Class.

        class P {
            public String method(){return "P.method";}
        }
        class C extends P {
             @Override
             public String method(){return "C.method";}
        }
        :
            public static String test5(Function<? super C,String> func, C c) {
                return func.apply(c); // c.method()
            }
        :
                System.out.println(test5(P::method, new C())); // => "C.method".
        

        So whet we need for y is only name of the method, not method object.

        Show
        uehaj UEHARA Junji added a comment - I mean y as String. In Java8, Class::instanceMethod is not mean the instance method is actually method of the Class. class P { public String method(){ return "P.method" ;} } class C extends P { @Override public String method(){ return "C.method" ;} } : public static String test5(Function<? super C, String > func, C c) { return func.apply(c); // c.method() } : System .out.println(test5(P::method, new C())); // => "C.method" . So whet we need for y is only name of the method, not method object.
        Hide
        blackdrag Jochen Theodorou added a comment -

        Sure, I was probably not explaining cleanly enough... of course it is a virtual call if it is a virtual method. On the JVM it is actually really difficult to call P#method from outside, without meaning C#method

        Show
        blackdrag Jochen Theodorou added a comment - Sure, I was probably not explaining cleanly enough... of course it is a virtual call if it is a virtual method. On the JVM it is actually really difficult to call P#method from outside, without meaning C#method
        Hide
        githubbot ASF GitHub Bot added a comment -

        GitHub user uehaj opened a pull request:

        https://github.com/apache/groovy/pull/287

        Javalike method pointer groovy 7772

        This is a change for method pointer semantics in groovy discussed in following issue.
        https://issues.apache.org/jira/browse/GROOVY-7772

        ref.
        http://docs.groovy-lang.org/latest/html/documentation/index.html#method-pointer-operator
        https://docs.oracle.com/javase/tutorial/java/javaOO/methodreferences.html#type

        You can merge this pull request into a Git repository by running:

        $ git pull https://github.com/uehaj/groovy javalike-method-pointer-GROOVY-7772

        Alternatively you can review and apply these changes as the patch at:

        https://github.com/apache/groovy/pull/287.patch

        To close this pull request, make a commit to your master/trunk branch
        with (at least) the following in the commit message:

        This closes #287


        commit 0308f6fe44ba354ba3eaf61d45f924895d09483d
        Author: UEHARA Junji <uehaj@jggug.org>
        Date: 2016-03-07T21:18:54Z

        GROOVY-7772: Change method pointer like Java 8

        commit f2ab90ea1ee0960d2a87b0feb536c4134841eb82
        Author: UEHARA Junji <uehaj@jggug.org>
        Date: 2016-03-08T20:00:44Z

        GROOVY-7772: Add test cases.

        commit 280a27b6b4715d153e01258670dda5131ebed187
        Author: UEHARA Junji <uehaj@jggug.org>
        Date: 2016-03-08T20:28:32Z

        GROOVY-7772: Handle too few arguments as missing method exception.


        Show
        githubbot ASF GitHub Bot added a comment - GitHub user uehaj opened a pull request: https://github.com/apache/groovy/pull/287 Javalike method pointer groovy 7772 This is a change for method pointer semantics in groovy discussed in following issue. https://issues.apache.org/jira/browse/GROOVY-7772 ref. http://docs.groovy-lang.org/latest/html/documentation/index.html#method-pointer-operator https://docs.oracle.com/javase/tutorial/java/javaOO/methodreferences.html#type You can merge this pull request into a Git repository by running: $ git pull https://github.com/uehaj/groovy javalike-method-pointer- GROOVY-7772 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/groovy/pull/287.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #287 commit 0308f6fe44ba354ba3eaf61d45f924895d09483d Author: UEHARA Junji <uehaj@jggug.org> Date: 2016-03-07T21:18:54Z GROOVY-7772 : Change method pointer like Java 8 commit f2ab90ea1ee0960d2a87b0feb536c4134841eb82 Author: UEHARA Junji <uehaj@jggug.org> Date: 2016-03-08T20:00:44Z GROOVY-7772 : Add test cases. commit 280a27b6b4715d153e01258670dda5131ebed187 Author: UEHARA Junji <uehaj@jggug.org> Date: 2016-03-08T20:28:32Z GROOVY-7772 : Handle too few arguments as missing method exception.
        Hide
        uehaj UEHARA Junji added a comment -

        I made PR for this change as a reference. Would you review it?

        Show
        uehaj UEHARA Junji added a comment - I made PR for this change as a reference. Would you review it?
        Hide
        blackdrag Jochen Theodorou added a comment -

        I think there is one more point to clear:

        class X {
          def m() {1}
          def m(int i) {i+1}
        }
        
        def ref = new X().&m
        assert ref() == 1
        assert ref(1) == 2
        assert ref.parameterTypes == [int]

        As you can imagine code like this would not be possible in Java. It also means. The implementation guarantees, that the prameterTypes will be corresponding to the longest signature. If multiple signatures of the same length exists, then a random one might be taken. In similar fashion this imposes a problem for determining if we do dispatch on a static method or not in your code. Imagine for example m() being static and m(int) not, and the reference X.&m. The code will make a setup for isStaticMethod=false and owner being a Class, prameterTypes will become [X,int], a call X.&m(1) will incorrectly not work according to the logic in doCall.
        Actually the class has another big problem not caused by this change. If we do a call to the MethodClosure from Groovy, the meta class logic will be used. But if we do the call from Java, the logic in doCall will be used. They have to stay in sync, to show the same effect, which is bad. Good would be one version using the other. Now for efficiency reasons we have sometimes direct calls from call to doCall. So at this point I would normally suggest making the class final and remove the doCall method. call, goes to doCall dynamically, so the logic in the meta class would still work out... but this would be a breaking change.. So instead I would let doCall throw an Exception.
        Then of course call should not be specified by MethodClosure at all. And of course the decision if we do a dispatch based on the static method or not, with an instance or a Class, must be completely in the MetaClassImpl part, you already touched. But instead of blindly invoking, you will need to get the MetaMethod and check. for the special case, to blindly invoke otherwise. This of course also means MethodClosure would not have a isStaticMethod property as well

        Show
        blackdrag Jochen Theodorou added a comment - I think there is one more point to clear: class X { def m() {1} def m( int i) {i+1} } def ref = new X().&m assert ref() == 1 assert ref(1) == 2 assert ref.parameterTypes == [ int ] As you can imagine code like this would not be possible in Java. It also means. The implementation guarantees, that the prameterTypes will be corresponding to the longest signature. If multiple signatures of the same length exists, then a random one might be taken. In similar fashion this imposes a problem for determining if we do dispatch on a static method or not in your code. Imagine for example m() being static and m(int) not, and the reference X.&m. The code will make a setup for isStaticMethod=false and owner being a Class, prameterTypes will become [X,int] , a call X.&m(1) will incorrectly not work according to the logic in doCall. Actually the class has another big problem not caused by this change. If we do a call to the MethodClosure from Groovy, the meta class logic will be used. But if we do the call from Java, the logic in doCall will be used. They have to stay in sync, to show the same effect, which is bad. Good would be one version using the other. Now for efficiency reasons we have sometimes direct calls from call to doCall. So at this point I would normally suggest making the class final and remove the doCall method. call, goes to doCall dynamically, so the logic in the meta class would still work out... but this would be a breaking change.. So instead I would let doCall throw an Exception. Then of course call should not be specified by MethodClosure at all. And of course the decision if we do a dispatch based on the static method or not, with an instance or a Class, must be completely in the MetaClassImpl part, you already touched. But instead of blindly invoking, you will need to get the MetaMethod and check. for the special case, to blindly invoke otherwise. This of course also means MethodClosure would not have a isStaticMethod property as well
        Hide
        uehaj UEHARA Junji added a comment -

        Thanks for consideration. I don't have confidence that I correct understand your point,
        But I started to think this proposal introduces a breaking change.
        Probably it would be better introduce Java 8 compatible :: operator in Groovy instead of extending/changing semantics of .& operator.
        But I don't know still remains metaClass problem with :: operator in Groovy.

        Show
        uehaj UEHARA Junji added a comment - Thanks for consideration. I don't have confidence that I correct understand your point, But I started to think this proposal introduces a breaking change. Probably it would be better introduce Java 8 compatible :: operator in Groovy instead of extending/changing semantics of .& operator. But I don't know still remains metaClass problem with :: operator in Groovy.
        Hide
        blackdrag Jochen Theodorou added a comment -

        let's concentrate on one point first, overloads...

                for(MetaMethod m : methods) {
                    if (m.getParameterTypes().length > maximumNumberOfParameters) {
                        Class[] pt = m.getNativeParameterTypes();
                        maximumNumberOfParameters = pt.length;
                        parameterTypes = pt;
                        isStaticMethod = m.isStatic();
                    }
                }
        

        What happens if there are two methods of the same length? maximumNumberOfParameters will have the right information still, but parameterTypes might be one or the other, same for isStaticMethod. You can't be sure which one is taken, since InvokerHelper.getMetaClass(clazz).respondsTo(owner, method); does not guarantee any order. This means that information is unstable. So if you have two methods with the same number of parameters and the same name, You, as user, cannot be sure which of those two will be taken into consideration for your isStaticMethod based logic. You can see that already a bit when you look at line 63 in MethodClosure in your PR. There you adjust the parameter types, basically making it more for instance methods. But just before you had a loop selecting on the length of these parameter types, and there static and non-static was equal. Why is the non-static version not automatically longer in the first loop already?
        This then leads to line 77. If we cannot be sure that the partial method selection through the logic before was right, then it is most probably wrong to build on top of that information and do as if it was right. You see that parameterTypes plays absolutely no role in doCall. But isStaticMethod is equally bad. So instead of initializing the MethodClosure with this information.
        I think we have to get the information new... That means get the metaclass, ask it for a instance MetaMethod that fits the given arguments, change the signature to the static version and ask for a fitting static method. If only one of them exists, we have a candidate for our call. If both exist, then we can call either the first or the second... or we produce a MethodSelectionException. Of course there might be more than one method that could respond in both cases... those are probably error cases as well

        Show
        blackdrag Jochen Theodorou added a comment - let's concentrate on one point first, overloads... for (MetaMethod m : methods) { if (m.getParameterTypes().length > maximumNumberOfParameters) { Class [] pt = m.getNativeParameterTypes(); maximumNumberOfParameters = pt.length; parameterTypes = pt; isStaticMethod = m.isStatic(); } } What happens if there are two methods of the same length? maximumNumberOfParameters will have the right information still, but parameterTypes might be one or the other, same for isStaticMethod. You can't be sure which one is taken, since InvokerHelper.getMetaClass(clazz).respondsTo(owner, method); does not guarantee any order. This means that information is unstable. So if you have two methods with the same number of parameters and the same name, You, as user, cannot be sure which of those two will be taken into consideration for your isStaticMethod based logic. You can see that already a bit when you look at line 63 in MethodClosure in your PR. There you adjust the parameter types, basically making it more for instance methods. But just before you had a loop selecting on the length of these parameter types, and there static and non-static was equal. Why is the non-static version not automatically longer in the first loop already? This then leads to line 77. If we cannot be sure that the partial method selection through the logic before was right, then it is most probably wrong to build on top of that information and do as if it was right. You see that parameterTypes plays absolutely no role in doCall. But isStaticMethod is equally bad. So instead of initializing the MethodClosure with this information. I think we have to get the information new... That means get the metaclass, ask it for a instance MetaMethod that fits the given arguments, change the signature to the static version and ask for a fitting static method. If only one of them exists, we have a candidate for our call. If both exist, then we can call either the first or the second... or we produce a MethodSelectionException. Of course there might be more than one method that could respond in both cases... those are probably error cases as well
        Hide
        daniel_sun Daniel Sun added a comment -

        Jochen, the issue has been fixed in parrot branch

        Show
        daniel_sun Daniel Sun added a comment - Jochen, the issue has been fixed in parrot branch
        Hide
        daniel_sun Daniel Sun added a comment -

        Fixed in the parrot branch

        Show
        daniel_sun Daniel Sun added a comment - Fixed in the parrot branch
        Hide
        paulk Paul King added a comment -

        I'll temporarily re-open this until we understand current status a bit better. My goal will be to close again once we have created a new issue probably to document the current status.

        Show
        paulk Paul King added a comment - I'll temporarily re-open this until we understand current status a bit better. My goal will be to close again once we have created a new issue probably to document the current status.

          People

          • Assignee:
            daniel_sun Daniel Sun
            Reporter:
            uehaj UEHARA Junji
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:

              Development