groovy
  1. groovy
  2. GROOVY-3069

Closure does not have scope precedent over local method

    Details

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

      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_Groovy3069Bug.groovy
        1 kB
        Roshan Dawrani

        Activity

        Hide
        blackdrag blackdrag 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
        blackdrag blackdrag 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
        blackdrag blackdrag 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
        blackdrag blackdrag 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
        blackdrag blackdrag 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
        blackdrag blackdrag 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:
            blackdrag blackdrag
            Reporter:
            Roshan Dawrani
          • Votes:
            1 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: