[
https://issues.apache.org/jira/browse/GROOVY-9629?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18060828#comment-18060828
]
James Fredley commented on GROOVY-9629:
---------------------------------------
See: https://github.com/apache/grails-core/pull/15448
> 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.20.10#820010)
