Groovy
  1. Groovy
  2. GROOVY-3069

Closure does not have scope precedent over local method

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5.6
    • Fix Version/s: 1.6-rc-1, 1.5.8, 1.7-beta-1
    • Component/s: syntax
    • Labels:
      None
    • Environment:
      Microsoft Windows XP [Version 5.1.2600]
      Java(TM) 2 Runtime Environment, Standard Edition (build 1.5.0_04-b05)
      Groovy Version: 1.5.6 JVM: 1.5.0_04-b05

      Description

      Looking at this SSCCE, the local method selectSql gets called rather than the closure, when the closure should have scope preference. Please find below reference to the discussion about this issue on the groovy-user mailing-list.

      public class Tester {
      private String selectSql()

      {return 'local method'}

      public String returnSql(Closure selectSql)

      { return selectSql() }

      }

      Tester t = new Tester()
      println t.returnSql()

      { 'date,name,value' }

      Reference: http://www.nabble.com/Closure-does-not-have-scope-precedent-over-local-method--td19724860.html#a19724860

      1. 3069_v15x.diff
        1 kB
        Roshan Dawrani
      2. 3069_v16x.diff
        1 kB
        Roshan Dawrani
      3. 3069_v17x.diff
        1 kB
        Roshan Dawrani
      4. Groovy3069Bug.groovy
        0.6 kB
        Roshan Dawrani
      5. Ver2_3069Patches.zip
        3 kB
        Roshan Dawrani
      6. Ver2_Groovy3069Bug.groovy
        1 kB
        Roshan Dawrani

        Activity

        Hide
        Roshan Dawrani added a comment -

        The issue found in all 1.5.8, 1.6-RC-1, and 1.7-beta-1 branches.

        Please find attached the fix for all 3 versions.

        The modification has been made to AsmClassGenerator -> visitMethodCallExpression().

        If it now sees a closure variable on the compile stack (an argument or a local variable) of the same name as a method on the class, the closure is given precedence over the method on the class.

        Hope it helps.
        Roshan

        Show
        Roshan Dawrani added a comment - The issue found in all 1.5.8, 1.6-RC-1, and 1.7-beta-1 branches. Please find attached the fix for all 3 versions. The modification has been made to AsmClassGenerator -> visitMethodCallExpression(). If it now sees a closure variable on the compile stack (an argument or a local variable) of the same name as a method on the class, the closure is given precedence over the method on the class. Hope it helps. Roshan
        Hide
        Roshan Dawrani added a comment -

        Attaching the test case for the issue. It didn't allow more than 3 attachments last time

        Show
        Roshan Dawrani added a comment - Attaching the test case for the issue. It didn't allow more than 3 attachments last time
        Hide
        Roshan Dawrani added a comment -

        Simplified the "if" conditions that are used to identify this scenario when a local closure variable needs higher predecence. re-attached the re-tested patches.

        Show
        Roshan Dawrani added a comment - Simplified the "if" conditions that are used to identify this scenario when a local closure variable needs higher predecence. re-attached the re-tested patches.
        Hide
        Jochen Theodorou added a comment -

        I would remove the "&& compileStack.getVariable(methodName).getType().equals(ClassHelper.CLOSURE_TYPE)", because we do not depend on the type of the variable, since an untyped variable may still be a closure. Let us simply assume that it is allowed. And another small thing about code style.. please use "} else

        {" and "}

        else if". I know we are not really consistent with keeping that style, but we should try

        Show
        Jochen Theodorou added a comment - I would remove the "&& compileStack.getVariable(methodName).getType().equals(ClassHelper.CLOSURE_TYPE)", because we do not depend on the type of the variable, since an untyped variable may still be a closure. Let us simply assume that it is allowed. And another small thing about code style.. please use "} else {" and "} else if". I know we are not really consistent with keeping that style, but we should try
        Hide
        Roshan Dawrani added a comment -

        Attaching version 2 of the patches and the test case incorporating the review comments.

        The patches now allow a local closure variable to take predence over class methods even without the Closure type explicitly specified on the local variable.

        There were 2 places in the existing code base which were impacted because of the change in semantics being introduced by this fix. They have also been modified.

        def create(select) {
            ....
            if (select) {
                // after the fix, the following call would consider "select" as a closure and do a invokeClosure
                // but in some existing cases in groovy code base, even though the same variable name
                // is being used, value passed is a boolean or string and following select(..) call
                // is intended to call an instance method. At these 2 impacted places, I have changed
                // the parameter name to avoid conflict.
        
                select(i)
            }
            ....
        }
        
        Show
        Roshan Dawrani added a comment - Attaching version 2 of the patches and the test case incorporating the review comments. The patches now allow a local closure variable to take predence over class methods even without the Closure type explicitly specified on the local variable. There were 2 places in the existing code base which were impacted because of the change in semantics being introduced by this fix. They have also been modified. def create(select) { .... if (select) { // after the fix, the following call would consider "select" as a closure and do a invokeClosure // but in some existing cases in groovy code base, even though the same variable name // is being used, value passed is a boolean or string and following select(..) call // is intended to call an instance method. At these 2 impacted places, I have changed // the parameter name to avoid conflict. select(i) } .... }
        Hide
        Roshan Dawrani added a comment -

        I think instead of ignoring the type check completely in ACG, it may be better if we use the type if provided (local variable type != Object).

        That way, in code like below, "select" won't be mapped as a invokeClosure call on the "boolean" variable "select", which will be the case of type is totally removed from the new ACG check conditions for this fix.

        def create(boolean select) {
            ....
            if (select) {
                select(i)
            }
            ....
        }
        
        Show
        Roshan Dawrani added a comment - I think instead of ignoring the type check completely in ACG, it may be better if we use the type if provided (local variable type != Object). That way, in code like below, "select " won't be mapped as a invokeClosure call on the "boolean" variable "select", which will be the case of type is totally removed from the new ACG check conditions for this fix. def create( boolean select) { .... if (select) { select(i) } .... }
        Hide
        Jochen Theodorou added a comment -

        no, we should keep consistency and a boolean is there no different from Object or Closure. Also the user may have used a category to add a call method to Boolean, which would cause select(i) to be executed successfully. (Note: select is boolean, not Boolean, but that does not matter for the category)

        Show
        Jochen Theodorou added a comment - no, we should keep consistency and a boolean is there no different from Object or Closure. Also the user may have used a category to add a call method to Boolean, which would cause select(i) to be executed successfully. (Note: select is boolean, not Boolean, but that does not matter for the category)
        Hide
        Jochen Theodorou added a comment -

        when looking at the patch, then we have "call.isImplicitThis() && isThisExpression && compileStack.containsVariable(methodName)" and we have in the if before "methodName != null && isThisExpression && isFieldOrVariable(methodName) && !classNode.hasPossibleMethod(methodName, arguments))", also the body is nearly the same... so I think those two should be merged.. the equal parts are "methodName != null && isThisExpression", then I suggest to add "&& (compileStack.containsVariable(name) || (classNode.getDeclaredField(name) != null; && !classNode.hasPossibleMethod(methodName, arguments)))" I would also suggest to then remove isFieldOrVariable because the method is no longer used then... and since the if part becomes so long I would also suggest to put that whole thing in its own method

        Show
        Jochen Theodorou added a comment - when looking at the patch, then we have "call.isImplicitThis() && isThisExpression && compileStack.containsVariable(methodName)" and we have in the if before "methodName != null && isThisExpression && isFieldOrVariable(methodName) && !classNode.hasPossibleMethod(methodName, arguments))", also the body is nearly the same... so I think those two should be merged.. the equal parts are "methodName != null && isThisExpression", then I suggest to add "&& (compileStack.containsVariable(name) || (classNode.getDeclaredField(name) != null; && !classNode.hasPossibleMethod(methodName, arguments)))" I would also suggest to then remove isFieldOrVariable because the method is no longer used then... and since the if part becomes so long I would also suggest to put that whole thing in its own method
        Hide
        Roshan Dawrani added a comment -

        Actually I also thought of merging the conditions like that but there is one more patch I had worked on (for GROOVY-3156) and that also added 2 conditions to the same if condition in visitMethodCallExpression (to see if method call is not an explicit this call inside a closure), making it an even longer list of conditions.

        I am not sure fully but GROOVY-3156 may still need isFieldOrVariable() to be there. At least the patch had worked with it present.

        If you can look at GROOVY-3156 (it is related to GROOVY-2849, whose fix has been discussed on JIRA), may be fix for these 2 issues can be looked at/tested together and then whatever conditions are still applicable in that long list, can be moved to their own method.

        Show
        Roshan Dawrani added a comment - Actually I also thought of merging the conditions like that but there is one more patch I had worked on (for GROOVY-3156 ) and that also added 2 conditions to the same if condition in visitMethodCallExpression (to see if method call is not an explicit this call inside a closure), making it an even longer list of conditions. I am not sure fully but GROOVY-3156 may still need isFieldOrVariable() to be there. At least the patch had worked with it present. If you can look at GROOVY-3156 (it is related to GROOVY-2849 , whose fix has been discussed on JIRA), may be fix for these 2 issues can be looked at/tested together and then whatever conditions are still applicable in that long list, can be moved to their own method.
        Hide
        Jochen Theodorou added a comment -

        I come more and more to the conclusion that ACG is not the right place to do that. ACG should concentrate on the bytecode generation part and should ideally do only minimal other things. Finding if a method call is actually a call() method on a variable is not really part of that business.Things like these should be done in VariableScopeVisitor instead. Meaning the MethodCallExpression should have been transformed there to use the call method and a VariableExpression. And msot important the VariableExpression should have been linked to the defining Variable

        Show
        Jochen Theodorou added a comment - I come more and more to the conclusion that ACG is not the right place to do that. ACG should concentrate on the bytecode generation part and should ideally do only minimal other things. Finding if a method call is actually a call() method on a variable is not really part of that business.Things like these should be done in VariableScopeVisitor instead. Meaning the MethodCallExpression should have been transformed there to use the call method and a VariableExpression. And msot important the VariableExpression should have been linked to the defining Variable
        Hide
        Roshan Dawrani added a comment -

        I think it is a good idea to not clutter ACG with too many things and move some of it back to VariableScopeVaisitor.

        However, there are 4 issues right now with patches that are tested but are ACG based (visitMethodCallExpression() / visitAttributeOrProperty()), so I just wanted to bring up a suggestion.

        How about going ahead with the current patches to fix the functional issues raised in these 4 bugs and open another JIRA to do the internal re-organization to move things out of ACG to wherever it makes more sense. That way, the ACG clean-up can be done in isolation and if there are more such places in ACG that require such clean-up, they can be taken up too (done in isolation, it will be free of the context of all these 4 issues)

        Either way, I am fine with ACG being simplified and would like to give it a try but I wanted to check if it makes sense to you that we first get rid of these 4 bugs functionally and then do the re-organization internally in isolation.

        Show
        Roshan Dawrani added a comment - I think it is a good idea to not clutter ACG with too many things and move some of it back to VariableScopeVaisitor. However, there are 4 issues right now with patches that are tested but are ACG based (visitMethodCallExpression() / visitAttributeOrProperty()), so I just wanted to bring up a suggestion. How about going ahead with the current patches to fix the functional issues raised in these 4 bugs and open another JIRA to do the internal re-organization to move things out of ACG to wherever it makes more sense. That way, the ACG clean-up can be done in isolation and if there are more such places in ACG that require such clean-up, they can be taken up too (done in isolation, it will be free of the context of all these 4 issues) Either way, I am fine with ACG being simplified and would like to give it a try but I wanted to check if it makes sense to you that we first get rid of these 4 bugs functionally and then do the re-organization internally in isolation.
        Hide
        Jochen Theodorou added a comment -

        I merged your fix to 1.5.8 and made also some modification... I moved part of the fix to VariableScopeVisitor, which will now generate the method call for local variables. As a result ACG will no longer have to check if we are in a closure or if should do call() on a local variable.

        Show
        Jochen Theodorou added a comment - I merged your fix to 1.5.8 and made also some modification... I moved part of the fix to VariableScopeVisitor, which will now generate the method call for local variables. As a result ACG will no longer have to check if we are in a closure or if should do call() on a local variable.

          People

          • Assignee:
            Jochen Theodorou
            Reporter:
            Roshan Dawrani
          • Votes:
            1 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development