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

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

    [ https://issues.apache.org/jira/browse/GROOVY-9629?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17154021#comment-17154021 ] 

Eric Milles commented on GROOVY-9629:
-------------------------------------

{{MethodCallExpression}} defaults its {{implicitThis}} to true because the implication is that when you have "foo.bar()" you are resolving "foo" as "this.foo".  If you intend a different meaning, then you can call {{setImplicitThis(false)}} on any {{MethodCallExpression}} you create.

When you have "this.bar()" the {{implicitThis}} can tell you if "this" was explicitly given in the source or implied, as in "bar()".

> 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: 2.4.x, 2.5.x, 3.0.4
>            Reporter: Eric Helgeson
>            Priority: Major
>
> 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)