[jira] [Created] (GROOVY-9629) MethodCallExpression - implicitThis is always set to true

["Eric Helgeson (Jira)" <ji...@apache.org>]

Eric Helgeson created GROOVY-9629:
-------------------------------------

             Summary: MethodCallExpression - implicitThis is always set to true
                 Key: GROOVY-9629
                 URL: https://issues.apache.org/jira/browse/GROOVY-9629
             Project: Groovy
          Issue Type: Bug
    Affects Versions: 3.0.4, 2.4.x, 2.5.x
            Reporter: Eric Helgeson


Original issue in Grails [https://github.com/grails/grails-core/issues/11464] - Method with @Transactional and where query with local variable causes exception

 

Using Grails @Transactional AST we noticed an issue where Groovy would incorrectly believe the LHS of a where {} expression was a local variable. Brian Wheeler(bwheeler) traced it into groovy setting implicitThis to true when it should not be:

From: https://github.com/grails/grails-data-mapping/blob/a08f482152c7174ce0b95211530516f77d67cebe/grails-datastore-gorm/src/main/groovy/org/grails/datastore/gorm/transform/AbstractMethodDecoratingTransformation.groovy#L292
```
protected MethodNode moveOriginalCodeToNewMethod(MethodNode methodNode, String renamedMethodName, Parameter[] newParameters, ClassNode classNode, SourceUnit source, Map<String, ClassNode> genericsSpec) {
...
VariableScopeVisitor scopeVisitor = new VariableScopeVisitor(new SourceUnit("dummy", "dummy", source.getConfiguration(), source.getClassLoader(), new ErrorCollector(source.getConfiguration())))

if (methodNode == null) {
 scopeVisitor.visitClass(classNode)
 } else {
 scopeVisitor.prepareVisit(classNode)
 scopeVisitor.visitMethod(renamedMethodNode)
 }
...
```
In the failed case - this is our method expression
>org.codehaus.groovy.ast.expr.MethodCallExpression@d9f41[object: org.codehaus.groovy.ast.expr.VariableExpression@d9f41[variable: delegate] method: ConstantExpression[friend] arguments: org.codehaus.groovy.ast.expr.ArgumentListExpression@d9f41[org.codehaus.groovy.ast.expr.ClosureExpression@d9f41[]\{ org.codehaus.groovy.ast.stmt.BlockStatement@d9f41[org.codehaus.groovy.ast.stmt.ExpressionStatement@d9f41[expression:org.codehaus.groovy.ast.expr.MethodCallExpression@d9f41[object: org.codehaus.groovy.ast.expr.VariableExpression@d9f41[variable: this] method: ConstantExpression[eq] arguments: org.codehaus.groovy.ast.expr.ArgumentListExpression@d9f41[ConstantExpression[id], org.codehaus.groovy.ast.expr.PropertyExpression@15d157[object: org.codehaus.groovy.ast.expr.VariableExpression@15d023[variable: friend] property: ConstantExpression[id]]]]]] }]]


In groovy VariableScopeVisitor
```
public void visitMethodCallExpression(MethodCallExpression call) {
 if (call.isImplicitThis() && call.getMethod() instanceof ConstantExpression) {
 ConstantExpression methodNameConstant = (ConstantExpression) call.getMethod();
 Object value = methodNameConstant.getText();

if (!(value instanceof String)) {
 throw new GroovyBugError("tried to make a method call with a non-String constant method name.");
 }

String methodName = (String) value;
 Variable v = checkVariableNameForDeclaration(methodName, call);
 if (v != null && !(v instanceof DynamicVariable)) {
 checkVariableContextAccess(v, call);
 }

if (v instanceof VariableExpression || v instanceof Parameter) {
 VariableExpression object = new VariableExpression(v);
 object.setSourcePosition(methodNameConstant);
 call.setObjectExpression(object);
 ConstantExpression method = new ConstantExpression("call");
 method.setSourcePosition(methodNameConstant); // important for GROOVY-4344
 call.setImplicitThis(false);
 call.setMethod(method);
 }

}
 super.visitMethodCallExpression(call);
 }
```

In MethodCallExpression - implicitThis is always set to true in the constructor so this if statement will kick in on a method call with a ConstantExpression method.
You can see in this case the expression is "delegate". The groovy test cases this is usually "this".
In the checkVariableNameForDeclaration call - the scope is walked up looking for the variable - in our case "friend".
Friend is found 2 scopes up in the method call - and is a VariableExpression - so it triggers the changing of "delegate.friend" into "friend.call" - which is not valid and results in a no such method exception.

In our working example the checkVariableNameForDeclaration call the variable friend is not found because it does not conflict and the response is a DynamicVariable - which prevents the code from falling into the "call" swap out and thus working.

I doubt this is the right 'fix' - but if I change the if statement to check the method for 'this' explicitly - then the test cases pass, and it solves this situation. I am figuring that MethodCallExpression should properly figure out when to set implicitThis = true or not - or maybe enhance the logic around when to swap a method call into a '.call' here.

```
Boolean isThisCall = (call.getObjectExpression() instanceof VariableExpression && ((VariableExpression)call.getObjectExpression()).getName() == "this");
 if (isThisCall && call.getMethod() instanceof ConstantExpression) {
 //if (call.isImplicitThis() && call.getMethod() instanceof ConstantExpression) {
```

Here is a screenshot of transformed code with the fix:
[https://bertram.d.pr/49lTZd] 

 



--
This message was sent by Atlassian Jira
(v8.3.4#803005)