Closure does not have scope precedent over local method

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

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

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.

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

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

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)
    }
    ....
}

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)
    }
    ....
}

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)

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

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.

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

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.