groovy
  1. groovy
  2. GROOVY-3675

MetaClassImpl invokes call() on binding variable even if it is not a Closure

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 1.6.4
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Number of attachments :
      0

      Description

      The following:

      foo = 5
      foo()
      

      results in:

      Caught: groovy.lang.MissingMethodException: No signature of method: java.lang.Integer.call() is applicable for argument types: () values: []
      	at MyScript.run(MyScript.groovy:2)
      

      This is bad for at least two reasons:

      • The error message is hard to understand (should be: No signature of method: MyScript.foo() is applicable for argument types: () values: [])
      • invokeMethod and methodMissing don't get a chance to intercept the method call

      Likely solution: Change line 1093 of MetaClassImpl.java to: if(bindingVar instanceof Closure) {

        Issue Links

          Activity

          Hide
          Peter Niederwieser added a comment -

          I just noticed that the logic to call a closure in the binding is duplicated (MetaClassImpl and Script.invokeMethod). Can one of the two occurrences be removed?

          Show
          Peter Niederwieser added a comment - I just noticed that the logic to call a closure in the binding is duplicated (MetaClassImpl and Script.invokeMethod). Can one of the two occurrences be removed?
          Hide
          Roshan Dawrani added a comment -

          I am not sure about the binding variable logic in Script.invokeMethod but the change to MetaClassImpl was done by me as part of GROOVY-3284, where it was mentioned that "for sure I can say, that call() is to be made even if x is no Closure".

          Reproduce here some portion of the comments from GROOVY-3284

          • A call like Foo.A() is not supposed to work because Foo.A is of type Foo and not Closure.
          • you can't say it like that.... x() is supposed to resolve to x.call(), but this is a undocumented feature as we are not entirely sure if we really want to support this.
          • x() is supposed to resolve to x.call() even if x is not of type Closure? Irrespective of type of x?
          • kind of... as I said, it is not really specified. for sure I can say, that call() is to be made even if x is no Closure. I suspect in the MetaClass code where we test the field we test only for closures. A change here needs some experiments, because it may influence other parts.
          Show
          Roshan Dawrani added a comment - I am not sure about the binding variable logic in Script.invokeMethod but the change to MetaClassImpl was done by me as part of GROOVY-3284 , where it was mentioned that "for sure I can say, that call() is to be made even if x is no Closure". Reproduce here some portion of the comments from GROOVY-3284 A call like Foo.A() is not supposed to work because Foo.A is of type Foo and not Closure. you can't say it like that.... x() is supposed to resolve to x.call(), but this is a undocumented feature as we are not entirely sure if we really want to support this. x() is supposed to resolve to x.call() even if x is not of type Closure? Irrespective of type of x? kind of... as I said, it is not really specified. for sure I can say, that call() is to be made even if x is no Closure. I suspect in the MetaClass code where we test the field we test only for closures. A change here needs some experiments, because it may influence other parts.
          Hide
          blackdrag blackdrag added a comment -

          let us change the example....

          def foo = 5
          foo()

          do you still expect the methodmissingexception here? What if foo is a field? What if foo has no int, but a closure? What if a call() method has been added to int?

          Show
          blackdrag blackdrag added a comment - let us change the example.... def foo = 5 foo() do you still expect the methodmissingexception here? What if foo is a field? What if foo has no int, but a closure? What if a call() method has been added to int?
          Hide
          Peter Niederwieser added a comment -

          What I expect above all is consistent behavior. Therefore the duplication in MetaClassImpl and Script is dubious. When a call goes through MetaClassImpl, you get one behavior, and when it doesn't (e.g. because it goes through ClosureMetaClass), then you get another one. IMHO this needs to be fixed.

          Show
          Peter Niederwieser added a comment - What I expect above all is consistent behavior. Therefore the duplication in MetaClassImpl and Script is dubious. When a call goes through MetaClassImpl, you get one behavior, and when it doesn't (e.g. because it goes through ClosureMetaClass), then you get another one. IMHO this needs to be fixed.
          Hide
          Roshan Dawrani added a comment -

          That sounds reasonable but the first thing is to decide which of the two behaviors is correct.

          Show
          Roshan Dawrani added a comment - That sounds reasonable but the first thing is to decide which of the two behaviors is correct.
          Hide
          Peter Niederwieser added a comment -

          Unless the undocumented "resolve to x.call()" feature is made official, it seems logical to only invoke call() for closures. But I think we need Jochen's input here.

          Show
          Peter Niederwieser added a comment - Unless the undocumented "resolve to x.call()" feature is made official, it seems logical to only invoke call() for closures. But I think we need Jochen's input here.
          Hide
          blackdrag blackdrag added a comment -

          the logic for f() is:
          (1) if there is a local varibale f, do f.call().
          (2) if there is a method f(), invoke it
          (3) if there is a property f, invoke f.call()

          is this not consistent or is that problematic?

          Show
          blackdrag blackdrag added a comment - the logic for f() is: (1) if there is a local varibale f, do f.call(). (2) if there is a method f(), invoke it (3) if there is a property f, invoke f.call() is this not consistent or is that problematic?
          Hide
          Peter Niederwieser added a comment -

          The inconsistency is the following: If the call goes through MetaClassImpl, the binding variable wins over Script.invokeMethod(). If it doesn't (e.g. because it goes through ClosureMetaClass), then it's the other way around.

          Show
          Peter Niederwieser added a comment - The inconsistency is the following: If the call goes through MetaClassImpl, the binding variable wins over Script.invokeMethod(). If it doesn't (e.g. because it goes through ClosureMetaClass), then it's the other way around.
          Hide
          blackdrag blackdrag added a comment -

          so it is not about an existing method or variable, but for the case it does not exist... ok...

          (1) if there is a local varibale f, do f.call().
          (2) if there is a method f(), invoke it
          (3) if there is a property f, invoke f.call()
          (4) call invokeMethod()/methodMissing

          Accroding to this scheme it is ok for the script variable to win over invokeMethod.

          Show
          blackdrag blackdrag added a comment - so it is not about an existing method or variable, but for the case it does not exist... ok... (1) if there is a local varibale f, do f.call(). (2) if there is a method f(), invoke it (3) if there is a property f, invoke f.call() (4) call invokeMethod()/methodMissing Accroding to this scheme it is ok for the script variable to win over invokeMethod.
          Hide
          Peter Niederwieser added a comment -

          >so it is not about an existing method or variable, but for the case it does not exist... ok...
          It's about the race between an existing script variable and Script.invokeMethod().

          >Accroding to this scheme it is ok for the script variable to win over invokeMethod.
          I don't see script variables mentioned in your logic, and intuitively I'd put them after invokeMethod(), just like the author of Script.invokeMethod(). Anyway, if a script variable should win over invokeMethod(), then the implementation of Script.invokeMethod() must change. Because right now, Script.invokeMethod() wins over a script variable as soon as the call occurs from within a closure (and for all other calls that don't go through MetaClassImpl).

          Show
          Peter Niederwieser added a comment - >so it is not about an existing method or variable, but for the case it does not exist... ok... It's about the race between an existing script variable and Script.invokeMethod(). >Accroding to this scheme it is ok for the script variable to win over invokeMethod. I don't see script variables mentioned in your logic, and intuitively I'd put them after invokeMethod(), just like the author of Script.invokeMethod(). Anyway, if a script variable should win over invokeMethod(), then the implementation of Script.invokeMethod() must change. Because right now, Script.invokeMethod() wins over a script variable as soon as the call occurs from within a closure (and for all other calls that don't go through MetaClassImpl).
          Hide
          blackdrag blackdrag added a comment -

          Script variables are properties, so this goes in (3).

          Show
          blackdrag blackdrag added a comment - Script variables are properties, so this goes in (3).
          Hide
          Peter Niederwieser added a comment -

          OK, then let's change Script.invokeMethod().

          Show
          Peter Niederwieser added a comment - OK, then let's change Script.invokeMethod().
          Hide
          blackdrag blackdrag added a comment -

          After rereading the issue I think we are in the state that we can close the issue already. If

          (1) if there is a local varibale f, do f.call().
          (2) if there is a method f(), invoke it
          (3) if there is a property f, invoke f.call()
          (4) call invokeMethod()/methodMissing

          is acceptable, then the important point here is that f is found in step (3) in the binding. Script's invokeMethod is not asked, because that would have happened in (4), but because a property was found we do the call for it which fails, because Integer does not provide a call() method in this example. What if it would provide one? Wouldn't it be expected to be called? The only way to fix it in the way that you may think is right is to react to the missing method there.

          If the method call is in a Closure, then yes, it behaves different. And that is the reason I don't close this issue. But this is because of the way the MOP is designed. So this will become a target for 2.0 instead

          Show
          blackdrag blackdrag added a comment - After rereading the issue I think we are in the state that we can close the issue already. If (1) if there is a local varibale f, do f.call(). (2) if there is a method f(), invoke it (3) if there is a property f, invoke f.call() (4) call invokeMethod()/methodMissing is acceptable, then the important point here is that f is found in step (3) in the binding. Script's invokeMethod is not asked, because that would have happened in (4), but because a property was found we do the call for it which fails, because Integer does not provide a call() method in this example. What if it would provide one? Wouldn't it be expected to be called? The only way to fix it in the way that you may think is right is to react to the missing method there. If the method call is in a Closure, then yes, it behaves different. And that is the reason I don't close this issue. But this is because of the way the MOP is designed. So this will become a target for 2.0 instead
          blackdrag blackdrag made changes -
          Field Original Value New Value
          Link This issue relates to GROOVY-2503 [ GROOVY-2503 ]
          Peter Niederwieser made changes -
          Assignee Peter Niederwieser [ pniederw ]
          blackdrag blackdrag made changes -
          Status Open [ 1 ] Closed [ 6 ]
          Assignee blackdrag blackdrag [ blackdrag ]
          Resolution Won't Fix [ 2 ]

            People

            • Assignee:
              blackdrag blackdrag
              Reporter:
              Peter Niederwieser
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: