[
https://issues.apache.org/jira/browse/GROOVY-9629?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Eric Milles updated GROOVY-9629:
--------------------------------
Description:
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
{code}
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)
}
...
{code}
In the failed case - this is our method expression
{code}
>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]]]]]] }]]
{code}
In groovy VariableScopeVisitor
{code}
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);
}
{code}
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.
{code}
Boolean isThisCall = (call.getObjectExpression() instanceof VariableExpression
&& ((VariableExpression)call.getObjectExpression()).getName() == "this");
if (isThisCall && call.getMethod() instanceof ConstantExpression) {
//if (call.isImplicitThis() && call.getMethod() instanceof ConstantExpression)
{
{code}
Here is a screenshot of transformed code with the fix:
[https://bertram.d.pr/49lTZd]
was:
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]
> 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
> {code}
> 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)
> }
> ...
> {code}
> In the failed case - this is our method expression
> {code}
> >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]]]]]] }]]
> {code}
> In groovy VariableScopeVisitor
> {code}
> 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);
> }
> {code}
> 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.
> {code}
> Boolean isThisCall = (call.getObjectExpression() instanceof
> VariableExpression &&
> ((VariableExpression)call.getObjectExpression()).getName() == "this");
> if (isThisCall && call.getMethod() instanceof ConstantExpression) {
> //if (call.isImplicitThis() && call.getMethod() instanceof
> ConstantExpression) {
> {code}
> 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)
