Skip to content

Commit

Permalink
cast implicit property and fix types of within-closure this conversion
Browse files Browse the repository at this point in the history
  • Loading branch information
@eric-milles
eric-milles committed Sep 17, 2024
1 parent 77dc80a commit 168bb9b
Show file tree
Hide file tree
Showing 6 changed files with 83 additions and 103 deletions.
22 changes: 8 additions & 14 deletions src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -896,18 +896,6 @@ public void visitLambdaExpression(final LambdaExpression expression) {
controller.getLambdaWriter().writeLambda(expression);
}

/**
* Loads either this object or if we're inside a closure then load the top level owner
*/
protected void loadThisOrOwner() {
ClassNode classNode = controller.getClassNode();
if (classNode.getOuterClass() == null) {
loadThis(VariableExpression.THIS_EXPRESSION);
} else {
fieldX(classNode.getDeclaredField("owner")).visit(this);
}
}

/**
* Generates byte code for constants.
*
Expand Down Expand Up @@ -1417,11 +1405,17 @@ public void visitVariableExpression(final VariableExpression expression) {
if (variable != null) {
controller.getOperandStack().loadOrStoreVariable(variable, expression.isUseReferenceDirectly());
} else {
PropertyExpression pexp = thisPropX(true, expression.getName());
PropertyExpression pexp = thisPropX(/*implicit-this*/true, expression.getName());
pexp.getProperty().setSourcePosition(expression);
pexp.setType(expression.getType());
pexp.copyNodeMetaData(expression);
pexp.visit(this);

if (!compileStack.isLHS() && !expression.isDynamicTyped()) {
ClassNode variableType = controller.getTypeChooser()
.resolveType(expression, controller.getClassNode());
controller.getOperandStack().doGroovyCast(variableType);
}
}

if (!compileStack.isLHS()) {
Expand Down Expand Up @@ -2375,7 +2369,7 @@ private void loadThis(final VariableExpression thisOrSuper) {
OperandStack operandStack = controller.getOperandStack();
if (controller.isInGeneratedFunction() && !controller.getCompileStack().isImplicitThis()) {
mv.visitMethodInsn(INVOKEVIRTUAL, "groovy/lang/Closure", "getThisObject", "()Ljava/lang/Object;", false);
ClassNode expectedType = controller.getTypeChooser().resolveType(thisOrSuper, controller.getOutermostClass());
ClassNode expectedType = controller.getTypeChooser().resolveType(thisOrSuper, controller.getThisType() );
if (!isObjectType(expectedType) && !isPrimitiveType(expectedType)) {
BytecodeHelper.doCast(mv, expectedType);
operandStack.push(expectedType);
Expand Down
88 changes: 42 additions & 46 deletions src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticInvocationWriter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,12 @@
import static org.apache.groovy.ast.tools.ExpressionUtils.isSuperExpression;
import static org.apache.groovy.ast.tools.ExpressionUtils.isThisOrSuper;
import static org.codehaus.groovy.ast.ClassHelper.isGStringType;
import static org.codehaus.groovy.ast.ClassHelper.isGeneratedFunction;
import static org.codehaus.groovy.ast.ClassHelper.isObjectType;
import static org.codehaus.groovy.ast.ClassHelper.isPrimitiveVoid;
import static org.codehaus.groovy.ast.ClassHelper.isStringType;
import static org.codehaus.groovy.ast.tools.GeneralUtils.args;
import static org.codehaus.groovy.ast.tools.GeneralUtils.attrX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.callX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.classX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.constX;
Expand All @@ -91,6 +93,7 @@
import static org.codehaus.groovy.ast.tools.GeneralUtils.propX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.stmt;
import static org.codehaus.groovy.ast.tools.GeneralUtils.varX;
import static org.codehaus.groovy.ast.tools.GenericsUtils.makeClassSafe0;
import static org.codehaus.groovy.transform.trait.Traits.isTrait;
import static org.objectweb.asm.Opcodes.ACONST_NULL;
import static org.objectweb.asm.Opcodes.ALOAD;
Expand Down Expand Up @@ -213,6 +216,33 @@ public void writeSpecialConstructorCall(final ConstructorCallExpression call) {
controller.getCompileStack().pop();
}

private Expression thisObjectExpression(final ClassNode source, final ClassNode target) {
ClassNode thisType = source;
while (isGeneratedFunction(thisType)) {
thisType = thisType.getOuterClass();
}
Expression thisExpr;
if (isTrait(thisType.getOuterClass())) {
thisExpr = varX("thisObject"); // GROOVY-7242, GROOVY-8127
} else if (controller.isStaticContext()) {
thisExpr = varX("thisObject", makeClassSafe0(ClassHelper.CLASS_Type, thisType.asGenericsType()));
} else {
thisExpr = varX("thisObject", thisType);
// adjust for multiple levels of nesting
while (!thisType.isDerivedFrom(target) && !thisType.implementsInterface(target)) {
FieldNode thisZero = thisType.getDeclaredField("this$0");
if (thisZero != null) {
thisExpr = attrX(thisExpr, "this$0");
thisExpr.setType(thisZero.getType());
thisType = thisType.getOuterClass();
if (thisType != null) continue;
}
break;
}
}
return thisExpr;
}

/**
* Attempts to make a direct method call on a bridge method, if it exists.
*/
Expand All @@ -238,32 +268,22 @@ protected boolean tryBridgeMethod(final MethodNode target, final Expression rece
lookupClassNode = target.getDeclaringClass().redirect();
}
Map<MethodNode, MethodNode> bridges = lookupClassNode.getNodeMetaData(StaticCompilationMetadataKeys.PRIVATE_BRIDGE_METHODS);
MethodNode bridge = bridges == null ? null : bridges.get(target);
MethodNode bridge = (bridges == null ? null : bridges.get(target));
if (bridge != null) {
Expression fixedReceiver = receiver;
Expression newReceiver = receiver;
if (implicitThis) {
if (!controller.isInGeneratedFunction()) {
if (!thisClass.isDerivedFrom(lookupClassNode))
fixedReceiver = propX(classX(lookupClassNode), "this");
newReceiver = propX(classX(lookupClassNode), "this");
} else if (thisClass != null) {
ClassNode current = thisClass.getOuterClass();
fixedReceiver = varX("thisObject", current);
// adjust for multiple levels of nesting if needed
while (current.getOuterClass() != null && !lookupClassNode.equals(current)) {
FieldNode thisField = current.getField("this$0");
current = current.getOuterClass();
if (thisField != null) {
fixedReceiver = propX(fixedReceiver, "this$0");
fixedReceiver.setType(current);
}
}
newReceiver = thisObjectExpression(thisClass, lookupClassNode);
}
}
ArgumentListExpression newArgs = args(target.isStatic() ? nullX() : fixedReceiver);
ArgumentListExpression newArguments = args(target.isStatic() ? nullX() : newReceiver);
for (Expression expression : args.getExpressions()) {
newArgs.addExpression(expression);
newArguments.addExpression(expression);
}
return writeDirectMethodCall(bridge, implicitThis, fixedReceiver, newArgs);
return writeDirectMethodCall(bridge, implicitThis, newReceiver, newArguments);
}
return false;
}
Expand All @@ -284,22 +304,10 @@ protected boolean writeDirectMethodCall(final MethodNode target, final boolean i
List<Expression> argumentList = new ArrayList<>();
if (emn.isStaticExtension()) {
argumentList.add(nullX());
} else if (!isThisOrSuper(receiver) || !controller.isInGeneratedFunction()) {
argumentList.add(receiver);
} else {
Expression fixedReceiver = null;
if (isThisOrSuper(receiver) && classNode.getOuterClass() != null && controller.isInGeneratedFunction()) {
ClassNode current = classNode.getOuterClass();
fixedReceiver = varX("thisObject", current);
// adjust for multiple levels of nesting if needed
while (current.getOuterClass() != null && !classNode.equals(current)) {
FieldNode thisField = current.getField("this$0");
current = current.getOuterClass();
if (thisField != null) {
fixedReceiver = propX(fixedReceiver, "this$0");
fixedReceiver.setType(current);
}
}
}
argumentList.add(fixedReceiver != null ? fixedReceiver : receiver);
argumentList.add(thisObjectExpression(classNode, target.getDeclaringClass()));
}
argumentList.addAll(args.getExpressions());
loadArguments(argumentList, parameters);
Expand Down Expand Up @@ -367,20 +375,8 @@ protected boolean writeDirectMethodCall(final MethodNode target, final boolean i
&& controller.isInGeneratedFunction()
&& !classNode.isDerivedFrom(target.getDeclaringClass())
&& !classNode.implementsInterface(target.getDeclaringClass())) {
ClassNode thisType = controller.getThisType();
if (isTrait(thisType.getOuterClass())) thisType = ClassHelper.dynamicType(); // GROOVY-7242

fixedReceiver = varX("thisObject", thisType);
// account for multiple levels of inner types
while (thisType.getOuterClass() != null && !target.getDeclaringClass().equals(thisType)) {
FieldNode thisField = thisType.getField("this$0");
thisType = thisType.getOuterClass();
if (thisField != null) {
fixedReceiver = propX(fixedReceiver, "this$0");
fixedReceiver.setType(thisType);
fixedImplicitThis = false;
}
}
fixedReceiver = thisObjectExpression(classNode, target.getDeclaringClass());
if (!(fixedReceiver instanceof VariableExpression)) fixedImplicitThis = false;
}
}
if (receiver != null && !isSuperExpression(receiver) && !isClassWithSuper(receiver)) {
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -476,7 +476,7 @@ private boolean makeGetPropertyWithGetter(final Expression receiver, final Class
}
// check outer class
if (implicitThis && receiverType instanceof InnerClassNode && !receiverType.isStaticClass()) {
if (makeGetPropertyWithGetter(receiver, receiverType.getOuterClass(), propertyName, safe, implicitThis)) {
if (makeGetPropertyWithGetter(receiver, receiverType.getOuterClass(), propertyName, safe, implicitThis)) {
return true;
}
}
Expand Down Expand Up @@ -524,8 +524,8 @@ boolean makeGetField(final Expression receiver, final ClassNode receiverType, fi
}
mv.visitLabel(skip);
}
operandStack.replace(resultType);
}
operandStack.replace(resultType);
return true;
}
return false;
Expand Down
19 changes: 9 additions & 10 deletions src/spec/test/builder/ObjectGraphBuilderTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,11 @@ import asciidoctor.Utils
import groovy.test.GroovyTestCase

/**
* Tests for ObjectGraphBuilder. The tests directly in this file are specific
* to ObjectGraphBuilder. Functionality in common with other Builders
* is tested in the parent class.
*/
class ObjectGraphBuilderTest extends GroovyTestCase {
* Tests for ObjectGraphBuilder. The tests directly in this file are specific
* to ObjectGraphBuilder. Functionality in common with other Builders
* is tested in the parent class.
*/
final class ObjectGraphBuilderTest extends GroovyTestCase {

void testBuilder() {
assertScript '''// tag::domain_classes[]
Expand Down Expand Up @@ -78,7 +78,6 @@ assert employee.address instanceof Address
'''
}


void testBuildImmutableFailure() {
def err = shouldFail '''
package com.acme
Expand Down Expand Up @@ -133,12 +132,12 @@ builder.newInstanceResolver = { Class klazz, Map attributes ->
}
// end::newinstanceresolver[]
def person = builder.person(name:'Jon', age:17)
'''
}

void testId() {
assertScript '''package com.acme
assertScript '''
package com.acme
class Company {
String name
Expand Down Expand Up @@ -179,6 +178,6 @@ assert e1.name == 'Duke'
assert e2.name == 'John'
assert e1.address.line1 == '123 Groovy Rd'
assert e2.address.line1 == '123 Groovy Rd'
'''
'''
}
}
}
33 changes: 13 additions & 20 deletions src/test/groovy/bugs/Groovy8446.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,19 +18,16 @@
*/
package groovy.bugs

import groovy.transform.CompileStatic
import org.junit.Test

import static groovy.test.GroovyAssert.assertScript
import static groovy.test.GroovyAssert.shouldFail

@CompileStatic
final class Groovy8446 {

private GroovyShell shell = new GroovyShell()

@Test
void testVoidArray0() {
shell.evaluate '''
assertScript '''
class C {
Void[] m() {}
}
Expand All @@ -40,25 +37,21 @@ final class Groovy8446 {

@Test
void testVoidArray1() {
def err = shouldFail {
shell.evaluate '''
class C {
void[] m() {}
}
'''
}
assert err =~ /void\[\] is an invalid type/ || err =~ /Unexpected input: '\('/
def err = shouldFail '''
class C {
void[] m() {}
}
'''
assert err =~ /void\[\] is an invalid type|Unexpected input: '\('/
}

@Test
void testVoidArray2() {
def err = shouldFail {
shell.evaluate '''
class C {
def meth(void... args) {}
}
'''
}
def err = shouldFail '''
class C {
def meth(void... args) {}
}
'''
assert err =~ /void\[\] is an invalid type|void is not allowed here/
}
}
20 changes: 9 additions & 11 deletions src/test/groovy/transform/stc/ClosuresSTCTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -628,30 +628,28 @@ class ClosuresSTCTest extends StaticTypeCheckingTestCase {
void testRecurseClosureCallAsMethod() {
assertScript '''
Closure<Integer> cl
cl = { int x -> x == 0 ? x : 1+cl(x-1) }
cl = { int x -> x == 0 ? x : 1 + cl(x-1) }
'''
}

void testFibClosureCallAsMethod() {
assertScript '''
Closure<Integer> fib
fib = { int x-> x<1?x:fib(x-1)+fib(x-2) }
fib(2)
fib = { int x -> x<2 ? x : fib(x-1) + fib(x-2) }
assert fib(2) == 1
'''
}

void testFibClosureCallAsMethodFromWithinClass() {
assertScript '''
class FibUtil {
private Closure<Integer> fibo
FibUtil() {
fibo = { int x-> x<1?x:fibo(x-1)+fibo(x-2) }
class Fibonacci {
private static final Closure<Integer> f
static {
f = { int x -> x<2 ? x : f(x-1) + f(x-2) }
}
int fib(int n) { fibo(n) }
static int of(int n) { f(n) }
}
FibUtil fib = new FibUtil()
fib.fib(2)
assert Fibonacci.of(2) == 1
'''
}

Expand Down

0 comments on commit 168bb9b

Please sign in to comment.