You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by sh...@apache.org on 2016/02/05 06:13:04 UTC
[1/2] groovy git commit: GROOVY-7705: Add bridge methods for setting
private fields of outer class from inner class or closure (closes #229)
Repository: groovy
Updated Branches:
refs/heads/GROOVY_2_4_X c46974382 -> 10632b46c
GROOVY-7705: Add bridge methods for setting private fields of outer class from inner class or closure (closes #229)
Project: http://git-wip-us.apache.org/repos/asf/groovy/repo
Commit: http://git-wip-us.apache.org/repos/asf/groovy/commit/28a17b32
Tree: http://git-wip-us.apache.org/repos/asf/groovy/tree/28a17b32
Diff: http://git-wip-us.apache.org/repos/asf/groovy/diff/28a17b32
Branch: refs/heads/GROOVY_2_4_X
Commit: 28a17b32ca1ad800917de96de0eaab1b283c9686
Parents: c469743
Author: Shil Sinha <sh...@gmail.com>
Authored: Wed Dec 9 19:23:24 2015 -0500
Committer: Shil Sinha <sh...@apache.org>
Committed: Thu Feb 4 23:58:57 2016 -0500
----------------------------------------------------------------------
...ypesBinaryExpressionMultiTypeDispatcher.java | 57 +++++++
.../sc/StaticCompilationMetadataKeys.java | 3 +-
.../transform/sc/StaticCompilationVisitor.java | 43 +++--
.../VariableExpressionTransformer.java | 3 +
.../stc/StaticTypeCheckingVisitor.java | 25 +--
.../groovy/transform/stc/StaticTypesMarker.java | 1 +
.../FieldsAndPropertiesStaticCompileTest.groovy | 163 +++++++++++++++++++
7 files changed, 273 insertions(+), 22 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/groovy/blob/28a17b32/src/main/org/codehaus/groovy/classgen/asm/sc/StaticTypesBinaryExpressionMultiTypeDispatcher.java
----------------------------------------------------------------------
diff --git a/src/main/org/codehaus/groovy/classgen/asm/sc/StaticTypesBinaryExpressionMultiTypeDispatcher.java b/src/main/org/codehaus/groovy/classgen/asm/sc/StaticTypesBinaryExpressionMultiTypeDispatcher.java
index a1088c4..9025665 100644
--- a/src/main/org/codehaus/groovy/classgen/asm/sc/StaticTypesBinaryExpressionMultiTypeDispatcher.java
+++ b/src/main/org/codehaus/groovy/classgen/asm/sc/StaticTypesBinaryExpressionMultiTypeDispatcher.java
@@ -29,6 +29,7 @@ import org.codehaus.groovy.classgen.asm.*;
import org.codehaus.groovy.runtime.MetaClassHelper;
import org.codehaus.groovy.syntax.Token;
import org.codehaus.groovy.syntax.Types;
+import org.codehaus.groovy.transform.sc.StaticCompilationMetadataKeys;
import org.codehaus.groovy.transform.sc.StaticCompilationVisitor;
import org.codehaus.groovy.transform.stc.StaticTypeCheckingSupport;
import org.codehaus.groovy.transform.stc.StaticTypeCheckingVisitor;
@@ -38,6 +39,7 @@ import org.objectweb.asm.MethodVisitor;
import org.objectweb.asm.Opcodes;
import java.lang.reflect.Modifier;
+import java.util.Map;
import java.util.concurrent.atomic.AtomicInteger;
import static org.codehaus.groovy.ast.ClassHelper.*;
@@ -53,6 +55,7 @@ import static org.codehaus.groovy.transform.sc.StaticCompilationVisitor.*;
public class StaticTypesBinaryExpressionMultiTypeDispatcher extends BinaryExpressionMultiTypeDispatcher implements Opcodes {
private final AtomicInteger labelCounter = new AtomicInteger();
+ private static final MethodNode CLOSURE_GETTHISOBJECT_METHOD = CLOSURE_TYPE.getMethod("getThisObject", new Parameter[0]);
public StaticTypesBinaryExpressionMultiTypeDispatcher(WriterController wc) {
@@ -298,6 +301,60 @@ public class StaticTypesBinaryExpressionMultiTypeDispatcher extends BinaryExpres
call.visit(controller.getAcg());
return true;
}
+ if (isThisExpression && !controller.isInClosure()) {
+ receiverType = controller.getClassNode();
+ }
+ if (makeSetPrivateFieldWithBridgeMethod(receiver, receiverType, property, arguments, safe, spreadSafe, implicitThis)) return true;
+ }
+ return false;
+ }
+
+ @SuppressWarnings("unchecked")
+ private boolean makeSetPrivateFieldWithBridgeMethod(final Expression receiver, final ClassNode receiverType, final String fieldName, final Expression arguments, final boolean safe, final boolean spreadSafe, final boolean implicitThis) {
+ WriterController controller = getController();
+ FieldNode field = receiverType.getField(fieldName);
+ ClassNode outerClass = receiverType.getOuterClass();
+ if (field == null && implicitThis && outerClass != null && !receiverType.isStaticClass()) {
+ Expression pexp;
+ if (controller.isInClosure()) {
+ MethodCallExpression mce = new MethodCallExpression(
+ new VariableExpression("this"),
+ "getThisObject",
+ ArgumentListExpression.EMPTY_ARGUMENTS
+ );
+ mce.putNodeMetaData(StaticTypesMarker.INFERRED_TYPE, controller.getOutermostClass());
+ mce.setImplicitThis(true);
+ mce.setMethodTarget(CLOSURE_GETTHISOBJECT_METHOD);
+ pexp = new CastExpression(controller.getOutermostClass(),mce);
+ } else {
+ pexp = new PropertyExpression(
+ new ClassExpression(outerClass),
+ "this"
+ );
+ ((PropertyExpression)pexp).setImplicitThis(true);
+ }
+ pexp.putNodeMetaData(StaticTypesMarker.INFERRED_TYPE, outerClass);
+ pexp.setSourcePosition(receiver);
+ return makeSetPrivateFieldWithBridgeMethod(pexp, outerClass, fieldName, arguments, safe, spreadSafe, true);
+ }
+ ClassNode classNode = controller.getClassNode();
+ if (field != null && Modifier.isPrivate(field.getModifiers())
+ && (StaticInvocationWriter.isPrivateBridgeMethodsCallAllowed(receiverType, classNode) || StaticInvocationWriter.isPrivateBridgeMethodsCallAllowed(classNode,receiverType))
+ && !receiverType.equals(classNode)) {
+ Map<String, MethodNode> mutators = (Map<String, MethodNode>) receiverType.redirect().getNodeMetaData(StaticCompilationMetadataKeys.PRIVATE_FIELDS_MUTATORS);
+ if (mutators != null) {
+ MethodNode methodNode = mutators.get(fieldName);
+ if (methodNode != null) {
+ MethodCallExpression mce = new MethodCallExpression(receiver, methodNode.getName(),
+ new ArgumentListExpression(field.isStatic()?new ConstantExpression(null):receiver, arguments));
+ mce.setMethodTarget(methodNode);
+ mce.setSafe(safe);
+ mce.setSpreadSafe(spreadSafe);
+ mce.setImplicitThis(implicitThis);
+ mce.visit(controller.getAcg());
+ return true;
+ }
+ }
}
return false;
}
http://git-wip-us.apache.org/repos/asf/groovy/blob/28a17b32/src/main/org/codehaus/groovy/transform/sc/StaticCompilationMetadataKeys.java
----------------------------------------------------------------------
diff --git a/src/main/org/codehaus/groovy/transform/sc/StaticCompilationMetadataKeys.java b/src/main/org/codehaus/groovy/transform/sc/StaticCompilationMetadataKeys.java
index c09ee01..3409368 100644
--- a/src/main/org/codehaus/groovy/transform/sc/StaticCompilationMetadataKeys.java
+++ b/src/main/org/codehaus/groovy/transform/sc/StaticCompilationMetadataKeys.java
@@ -27,7 +27,8 @@ public enum StaticCompilationMetadataKeys {
STATIC_COMPILE_NODE, // used to mark a section of code as to be statically compiled
BINARY_EXP_TARGET, // use to tell which method should be used in a binary expression
PRIVATE_BRIDGE_METHODS, // private bridge methods are methods used by an outer class to access an inner class method
- PRIVATE_FIELDS_ACCESSORS, // private constants methods are methods used by an outer class to access an outer class constant
+ PRIVATE_FIELDS_ACCESSORS, // private fields accessors are methods used by an inner class to access an outer class field
+ PRIVATE_FIELDS_MUTATORS, // private fields mutators are methods used by an inner class to set an outer class field
PROPERTY_OWNER, // the type of the class which owns the property
COMPONENT_TYPE, // for list.property expressions, we need the inferred component type
RECEIVER_OF_DYNAMIC_PROPERTY // if a receiver is the receiver of a dynamic property (for mixed mode compilation)
http://git-wip-us.apache.org/repos/asf/groovy/blob/28a17b32/src/main/org/codehaus/groovy/transform/sc/StaticCompilationVisitor.java
----------------------------------------------------------------------
diff --git a/src/main/org/codehaus/groovy/transform/sc/StaticCompilationVisitor.java b/src/main/org/codehaus/groovy/transform/sc/StaticCompilationVisitor.java
index cf62c0f..dcdbfda 100644
--- a/src/main/org/codehaus/groovy/transform/sc/StaticCompilationVisitor.java
+++ b/src/main/org/codehaus/groovy/transform/sc/StaticCompilationVisitor.java
@@ -27,10 +27,12 @@ import org.codehaus.groovy.ast.stmt.EmptyStatement;
import org.codehaus.groovy.ast.stmt.ExpressionStatement;
import org.codehaus.groovy.ast.stmt.ForStatement;
import org.codehaus.groovy.ast.stmt.Statement;
+import org.codehaus.groovy.ast.tools.GeneralUtils;
import org.codehaus.groovy.classgen.asm.*;
import org.codehaus.groovy.classgen.asm.sc.StaticCompilationMopWriter;
import org.codehaus.groovy.classgen.asm.sc.StaticTypesTypeChooser;
import org.codehaus.groovy.control.SourceUnit;
+import org.codehaus.groovy.syntax.Token;
import org.codehaus.groovy.transform.stc.StaticTypeCheckingSupport;
import org.codehaus.groovy.transform.stc.StaticTypeCheckingVisitor;
import org.codehaus.groovy.transform.stc.StaticTypesMarker;
@@ -165,35 +167,54 @@ public class StaticCompilationVisitor extends StaticTypeCheckingVisitor {
}
/**
- * Adds special accessors for private constants so that inner classes can retrieve them.
+ * Adds special accessors and mutators for private fields so that inner classes can get/set them
*/
@SuppressWarnings("unchecked")
private void addPrivateFieldsAccessors(ClassNode node) {
Set<ASTNode> accessedFields = (Set<ASTNode>) node.getNodeMetaData(StaticTypesMarker.PV_FIELDS_ACCESS);
- if (accessedFields==null) return;
- Map<String, MethodNode> privateConstantAccessors = (Map<String, MethodNode>) node.getNodeMetaData(PRIVATE_FIELDS_ACCESSORS);
- if (privateConstantAccessors!=null) {
+ Set<ASTNode> mutatedFields = (Set<ASTNode>) node.getNodeMetaData(StaticTypesMarker.PV_FIELDS_MUTATION);
+ if (accessedFields == null && mutatedFields == null) return;
+ Map<String, MethodNode> privateFieldAccessors = (Map<String, MethodNode>) node.getNodeMetaData(PRIVATE_FIELDS_ACCESSORS);
+ Map<String, MethodNode> privateFieldMutators = (Map<String, MethodNode>) node.getNodeMetaData(PRIVATE_FIELDS_MUTATORS);
+ if (privateFieldAccessors != null || privateFieldMutators != null) {
// already added
return;
}
int acc = -1;
- privateConstantAccessors = new HashMap<String, MethodNode>();
+ privateFieldAccessors = accessedFields != null ? new HashMap<String, MethodNode>() : null;
+ privateFieldMutators = mutatedFields != null ? new HashMap<String, MethodNode>() : null;
final int access = Opcodes.ACC_STATIC | Opcodes.ACC_PUBLIC | Opcodes.ACC_SYNTHETIC;
for (FieldNode fieldNode : node.getFields()) {
- if (accessedFields.contains(fieldNode)) {
-
+ boolean generateAccessor = accessedFields != null && accessedFields.contains(fieldNode);
+ boolean generateMutator = mutatedFields != null && mutatedFields.contains(fieldNode);
+ if (generateAccessor) {
acc++;
Parameter param = new Parameter(node.getPlainNodeReference(), "$that");
- Expression receiver = fieldNode.isStatic()?new ClassExpression(node):new VariableExpression(param);
+ Expression receiver = fieldNode.isStatic() ? new ClassExpression(node) : new VariableExpression(param);
Statement stmt = new ExpressionStatement(new PropertyExpression(
receiver,
fieldNode.getName()
));
- MethodNode accessor = node.addMethod("pfaccess$"+acc, access, fieldNode.getOriginType(), new Parameter[]{param}, ClassNode.EMPTY_ARRAY, stmt);
- privateConstantAccessors.put(fieldNode.getName(), accessor);
+ MethodNode accessor = node.addMethod("pfaccess$" + acc, access, fieldNode.getOriginType(), new Parameter[]{param}, ClassNode.EMPTY_ARRAY, stmt);
+ privateFieldAccessors.put(fieldNode.getName(), accessor);
+ }
+
+ if (generateMutator) {
+ //increment acc if it hasn't been incremented in the current iteration
+ if (!generateAccessor) acc++;
+ Parameter param = new Parameter(node.getPlainNodeReference(), "$that");
+ Expression receiver = fieldNode.isStatic() ? new ClassExpression(node) : new VariableExpression(param);
+ Parameter value = new Parameter(fieldNode.getOriginType(), "$value");
+ Statement stmt = GeneralUtils.assignS(
+ new PropertyExpression(receiver, fieldNode.getName()),
+ new VariableExpression(value)
+ );
+ MethodNode mutator = node.addMethod("pfaccess$0" + acc, access, fieldNode.getOriginType(), new Parameter[]{param, value}, ClassNode.EMPTY_ARRAY, stmt);
+ privateFieldMutators.put(fieldNode.getName(), mutator);
}
}
- node.setNodeMetaData(PRIVATE_FIELDS_ACCESSORS, privateConstantAccessors);
+ if (privateFieldAccessors != null) node.setNodeMetaData(PRIVATE_FIELDS_ACCESSORS, privateFieldAccessors);
+ if (privateFieldMutators != null) node.setNodeMetaData(PRIVATE_FIELDS_MUTATORS, privateFieldMutators);
}
/**
http://git-wip-us.apache.org/repos/asf/groovy/blob/28a17b32/src/main/org/codehaus/groovy/transform/sc/transformers/VariableExpressionTransformer.java
----------------------------------------------------------------------
diff --git a/src/main/org/codehaus/groovy/transform/sc/transformers/VariableExpressionTransformer.java b/src/main/org/codehaus/groovy/transform/sc/transformers/VariableExpressionTransformer.java
index 8b1d779..84e25eb 100644
--- a/src/main/org/codehaus/groovy/transform/sc/transformers/VariableExpressionTransformer.java
+++ b/src/main/org/codehaus/groovy/transform/sc/transformers/VariableExpressionTransformer.java
@@ -66,6 +66,9 @@ public class VariableExpressionTransformer {
private Expression tryTransformPrivateFieldAccess(VariableExpression expr) {
FieldNode field = expr.getNodeMetaData(StaticTypesMarker.PV_FIELDS_ACCESS);
+ if (field == null) {
+ field = expr.getNodeMetaData(StaticTypesMarker.PV_FIELDS_MUTATION);
+ }
if (field != null) {
// access to a private field from a section of code that normally doesn't have access to it, like a
// closure or an inner class
http://git-wip-us.apache.org/repos/asf/groovy/blob/28a17b32/src/main/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
----------------------------------------------------------------------
diff --git a/src/main/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java b/src/main/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
index 0f08f7e..9e1fae2 100644
--- a/src/main/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
+++ b/src/main/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
@@ -341,13 +341,14 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
}
/**
- * Given a field node, checks if we are calling a private field from an inner class.
+ * Given a field node, checks if we are accessing or setting a private field from an inner class.
*/
- private void checkOrMarkPrivateAccess(Expression source, FieldNode fn) {
+ private void checkOrMarkPrivateAccess(Expression source, FieldNode fn, boolean lhsOfAssignment) {
if (fn!=null && Modifier.isPrivate(fn.getModifiers()) &&
(fn.getDeclaringClass() != typeCheckingContext.getEnclosingClassNode() || typeCheckingContext.getEnclosingClosure()!=null) &&
fn.getDeclaringClass().getModule() == typeCheckingContext.getEnclosingClassNode().getModule()) {
- addPrivateFieldOrMethodAccess(source, fn.getDeclaringClass(), StaticTypesMarker.PV_FIELDS_ACCESS, fn);
+ StaticTypesMarker marker = lhsOfAssignment ? StaticTypesMarker.PV_FIELDS_MUTATION : StaticTypesMarker.PV_FIELDS_ACCESS;
+ addPrivateFieldOrMethodAccess(source, fn.getDeclaringClass(), marker, fn);
}
}
@@ -994,6 +995,11 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
if (!typeCheckMultipleAssignmentAndContinue(leftExpression, rightExpression)) return;
+ if (leftExpression instanceof VariableExpression
+ && ((VariableExpression) leftExpression).getAccessedVariable() instanceof FieldNode) {
+ checkOrMarkPrivateAccess(leftExpression, (FieldNode) ((VariableExpression) leftExpression).getAccessedVariable(), true);
+ }
+
//TODO: need errors for write-only too!
if (addedReadOnlyPropertyError(leftExpression)) return;
@@ -1217,7 +1223,7 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
FieldNode field = current.getDeclaredField(propertyName);
field = allowStaticAccessToMember(field, staticOnly);
- if (storeField(field, isAttributeExpression, pexp, current, visitor, receiver.getData())) return true;
+ if (storeField(field, isAttributeExpression, pexp, current, visitor, receiver.getData(), !readMode)) return true;
PropertyNode propertyNode = current.getProperty(propertyName);
propertyNode = allowStaticAccessToMember(propertyNode, staticOnly);
@@ -1225,7 +1231,7 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
boolean isThisExpression = objectExpression instanceof VariableExpression &&
((VariableExpression) objectExpression).isThisExpression() && objectExpressionType.equals(current);
- if (storeField(field, isThisExpression, pexp, receiver.getType(), visitor, receiver.getData()))
+ if (storeField(field, isThisExpression, pexp, receiver.getType(), visitor, receiver.getData(), !readMode))
return true;
MethodNode getter = current.getGetterMethod("get" + capName);
@@ -1281,7 +1287,7 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
}
foundGetterOrSetter = foundGetterOrSetter || !setters.isEmpty() || getter != null;
- if (storeField(field, true, pexp, current, visitor, receiver.getData())) return true;
+ if (storeField(field, true, pexp, current, visitor, receiver.getData(), !readMode)) return true;
// if the property expression is an attribute expression (o.@attr), then
// we stop now, otherwise we must check the parent class
if (/*!isAttributeExpression && */current.getSuperClass() != null) {
@@ -1438,11 +1444,11 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
storeType(expressionToStoreOn, type);
}
- private boolean storeField(FieldNode field, boolean returnTrueIfFieldExists, PropertyExpression expressionToStoreOn, ClassNode receiver, ClassCodeVisitorSupport visitor, String delegationData) {
+ private boolean storeField(FieldNode field, boolean returnTrueIfFieldExists, PropertyExpression expressionToStoreOn, ClassNode receiver, ClassCodeVisitorSupport visitor, String delegationData, boolean lhsOfAssignment) {
if (field==null || !returnTrueIfFieldExists) return false;
if (visitor != null) visitor.visitField(field);
storeWithResolve(field.getOriginType(), receiver, field.getDeclaringClass(), field.isStatic(), expressionToStoreOn);
- checkOrMarkPrivateAccess(expressionToStoreOn, field);
+ checkOrMarkPrivateAccess(expressionToStoreOn, field, lhsOfAssignment);
if (delegationData!=null) {
expressionToStoreOn.putNodeMetaData(StaticTypesMarker.IMPLICIT_RECEIVER, delegationData);
}
@@ -1459,7 +1465,6 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
return true;
}
-
protected void storeInferredTypeForPropertyExpression(final PropertyExpression pexp, final ClassNode flatInferredType) {
if (pexp.isSpreadSafe()) {
ClassNode list = LIST_TYPE.getPlainNodeReference();
@@ -3930,7 +3935,7 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
if (vexp == VariableExpression.SUPER_EXPRESSION) return makeSuper();
final Variable variable = vexp.getAccessedVariable();
if (variable instanceof FieldNode) {
- checkOrMarkPrivateAccess(vexp, (FieldNode) variable);
+ checkOrMarkPrivateAccess(vexp, (FieldNode) variable, isLHSOfEnclosingAssignment(vexp));
return getType((FieldNode) variable);
}
if (variable != null && variable != vexp && variable instanceof VariableExpression) {
http://git-wip-us.apache.org/repos/asf/groovy/blob/28a17b32/src/main/org/codehaus/groovy/transform/stc/StaticTypesMarker.java
----------------------------------------------------------------------
diff --git a/src/main/org/codehaus/groovy/transform/stc/StaticTypesMarker.java b/src/main/org/codehaus/groovy/transform/stc/StaticTypesMarker.java
index fb3abde..9785433 100644
--- a/src/main/org/codehaus/groovy/transform/stc/StaticTypesMarker.java
+++ b/src/main/org/codehaus/groovy/transform/stc/StaticTypesMarker.java
@@ -34,6 +34,7 @@ public enum StaticTypesMarker {
DELEGATION_METADATA, // used to store the delegation strategy and delegate type of a closure when declared with @DelegatesTo
IMPLICIT_RECEIVER, // if the receiver is implicit but not "this", store the name of the receiver (delegate or owner)
PV_FIELDS_ACCESS, // set of private fields that are accessed from closures or inner classes
+ PV_FIELDS_MUTATION, // set of private fields that are set from closures or inner classes
PV_METHODS_ACCESS, // set of private methods that are accessed from closures or inner classes
DYNAMIC_RESOLUTION, // call recognized by a type checking extension as a dynamic method call
SUPER_MOP_METHOD_REQUIRED // used to store the list of MOP methods that still have to be generated
http://git-wip-us.apache.org/repos/asf/groovy/blob/28a17b32/src/test/org/codehaus/groovy/classgen/asm/sc/FieldsAndPropertiesStaticCompileTest.groovy
----------------------------------------------------------------------
diff --git a/src/test/org/codehaus/groovy/classgen/asm/sc/FieldsAndPropertiesStaticCompileTest.groovy b/src/test/org/codehaus/groovy/classgen/asm/sc/FieldsAndPropertiesStaticCompileTest.groovy
index 61a0216..6af5f13 100644
--- a/src/test/org/codehaus/groovy/classgen/asm/sc/FieldsAndPropertiesStaticCompileTest.groovy
+++ b/src/test/org/codehaus/groovy/classgen/asm/sc/FieldsAndPropertiesStaticCompileTest.groovy
@@ -533,4 +533,167 @@ import org.codehaus.groovy.transform.sc.ListOfExpressionsExpression
foo?.id = 'new'
'''
}
+
+ void testPrivateFieldMutationInClosureUsesBridgeMethod() {
+ try {
+ assertScript '''
+ class Foo {
+ private String s
+ Closure c = { this.s = 'abc' }
+
+ void test() {
+ c()
+ assert s == 'abc'
+ }
+ }
+ new Foo().test()
+ '''
+ } finally {
+ assert astTrees['Foo$_closure1'][1].contains('INVOKESTATIC Foo.pfaccess$00 (LFoo;Ljava/lang/String;)Ljava/lang/String')
+ }
+ }
+
+ void testImplicitPrivateFieldMutationInClosureUsesBridgeMethod() {
+ try {
+ assertScript '''
+ class Foo {
+ private String s
+ Closure c = { s = 'abc' }
+
+ String test() {
+ c()
+ assert s == 'abc'
+ }
+ }
+ new Foo().test()
+ '''
+ } finally {
+ assert astTrees['Foo$_closure1'][1].contains('INVOKESTATIC Foo.pfaccess$00 (LFoo;Ljava/lang/String;)Ljava/lang/String')
+ }
+ }
+
+ void testPrivateStaticFieldMutationInClosureUsesBridgeMethod() {
+ try {
+ assertScript '''
+ class Foo {
+ private static String s
+ Closure c = { s = 'abc' }
+
+ String test() {
+ c()
+ assert s == 'abc'
+ }
+ }
+ new Foo().test()
+ '''
+ } finally {
+ assert astTrees['Foo$_closure1'][1].contains('INVOKESTATIC Foo.pfaccess$00 (LFoo;Ljava/lang/String;)Ljava/lang/String')
+ }
+ }
+
+ void testPrivateFieldMutationInAICUsesBridgeMethod() {
+ try {
+ assertScript '''
+ class A {
+ private int x
+ void test() {
+ def aic = new Runnable() { void run() { A.this.x = 666 } }
+ aic.run()
+ assert x == 666
+ }
+ }
+ new A().test()
+ '''
+ } finally {
+ assert astTrees['A$1'][1].contains('INVOKESTATIC A.pfaccess$00 (LA;I)I')
+ }
+ }
+
+ void testImplicitPrivateFieldMutationInAICUsesBridgeMethod() {
+ try {
+ assertScript '''
+ class A {
+ private int x
+ void test() {
+ def aic = new Runnable() { void run() { x = 666 } }
+ aic.run()
+ assert x == 666
+ }
+ }
+ new A().test()
+ '''
+ } finally {
+ assert astTrees['A$1'][1].contains('INVOKESTATIC A.pfaccess$00 (LA;I)I')
+ }
+ }
+
+ void testPrivateStaticFieldMutationInAICUsesBridgeMethod() {
+ try {
+ assertScript '''
+ class A {
+ private static int x
+ void test() {
+ def aic = new Runnable() { void run() { x = 666 } }
+ aic.run()
+ assert x == 666
+ }
+ }
+ new A().test()
+ '''
+ } finally {
+ assert astTrees['A$1'][1].contains('INVOKESTATIC A.pfaccess$00 (LA;I)I')
+ }
+ }
+
+ void testMultiplePrivateFieldMutatorBridgeMethods() {
+ try {
+ assertScript '''
+ class A {
+ private int x
+ private String y
+ Closure mutate = { x = 1; y = 'abc' }
+
+ void test() {
+ mutate()
+ assert x == 1
+ assert y == 'abc'
+ }
+ }
+ new A().test()
+ '''
+ } finally {
+ assert astTrees['A$_closure1'][1].contains('INVOKESTATIC A.pfaccess$00 (LA;I)I')
+ assert astTrees['A$_closure1'][1].contains('INVOKESTATIC A.pfaccess$01 (LA;Ljava/lang/String;)Ljava/lang/String;')
+ }
+ }
+
+ void testPrivateFieldBridgeMethodsAreGeneratedAsNecessary() {
+ try {
+ assertScript '''
+ class A {
+ private int accessed = 0
+ private String mutated
+ private String accessedAndMutated = ''
+ Closure c = {
+ println accessed
+ mutated = 'abc'
+ println accessedAndMutated
+ accessedAndMutated = 'def'
+ }
+
+ void test() {
+ c()
+ assert mutated == 'abc'
+ assert accessedAndMutated == 'def'
+ }
+ }
+ new A().test()
+ '''
+ } finally {
+ assert !astTrees['A'][1].contains('pfaccess$00') // no mutator bridge method for 'accessed'
+ assert !astTrees['A'][1].contains('pfaccess$1') // no accessor bridge method for 'mutated'
+ assert astTrees['A$_closure1'][1].contains('INVOKESTATIC A.pfaccess$2 (LA;)Ljava/lang/String;')
+ assert astTrees['A$_closure1'][1].contains('INVOKESTATIC A.pfaccess$02 (LA;Ljava/lang/String;)Ljava/lang/String;')
+ }
+ }
}
[2/2] groovy git commit: GROOVY-7627 Property calls are not correctly
type checked if the setter parameter type or getter return type are different
from the underlying field type (closes #139)
Posted by sh...@apache.org.
GROOVY-7627 Property calls are not correctly type checked if the setter parameter type or getter return type are different from the underlying field type
(closes #139)
Project: http://git-wip-us.apache.org/repos/asf/groovy/repo
Commit: http://git-wip-us.apache.org/repos/asf/groovy/commit/10632b46
Tree: http://git-wip-us.apache.org/repos/asf/groovy/tree/10632b46
Diff: http://git-wip-us.apache.org/repos/asf/groovy/diff/10632b46
Branch: refs/heads/GROOVY_2_4_X
Commit: 10632b46cfe13ee13f890c6c6eacd701deaf933d
Parents: 28a17b3
Author: Shil S <sh...@gmail.com>
Authored: Tue Oct 13 03:03:55 2015 -0400
Committer: Shil Sinha <sh...@apache.org>
Committed: Fri Feb 5 00:08:17 2016 -0500
----------------------------------------------------------------------
.../asm/sc/StaticTypesCallSiteWriter.java | 2 +-
.../stc/StaticTypeCheckingVisitor.java | 21 +++---
.../stc/FieldsAndPropertiesSTCTest.groovy | 74 ++++++++++++++++++++
3 files changed, 88 insertions(+), 9 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/groovy/blob/10632b46/src/main/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java
----------------------------------------------------------------------
diff --git a/src/main/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java b/src/main/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java
index 526d001..8d54036 100644
--- a/src/main/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java
+++ b/src/main/org/codehaus/groovy/classgen/asm/sc/StaticTypesCallSiteWriter.java
@@ -471,7 +471,7 @@ public class StaticTypesCallSiteWriter extends CallSiteWriter implements Opcodes
// and that one references the other, the getters for properties have not been
// generated by the compiler yet (generated by the Verifier)
PropertyNode propertyNode = receiverType.getProperty(methodName);
- if (propertyNode!=null) {
+ if (getterNode == null && propertyNode != null) {
// it is possible to use a getter
String prefix = "get";
if (boolean_TYPE.equals(propertyNode.getOriginType())) {
http://git-wip-us.apache.org/repos/asf/groovy/blob/10632b46/src/main/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
----------------------------------------------------------------------
diff --git a/src/main/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java b/src/main/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
index 9e1fae2..c789e64 100644
--- a/src/main/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
+++ b/src/main/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
@@ -1225,12 +1225,10 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
field = allowStaticAccessToMember(field, staticOnly);
if (storeField(field, isAttributeExpression, pexp, current, visitor, receiver.getData(), !readMode)) return true;
- PropertyNode propertyNode = current.getProperty(propertyName);
- propertyNode = allowStaticAccessToMember(propertyNode, staticOnly);
- if (storeProperty(propertyNode, pexp, current, visitor, receiver.getData())) return true;
+ boolean isThisExpression = objectExpression instanceof VariableExpression
+ && ((VariableExpression) objectExpression).isThisExpression()
+ && objectExpressionType.equals(current);
- boolean isThisExpression = objectExpression instanceof VariableExpression &&
- ((VariableExpression) objectExpression).isThisExpression() && objectExpressionType.equals(current);
if (storeField(field, isThisExpression, pexp, receiver.getType(), visitor, receiver.getData(), !readMode))
return true;
@@ -1246,7 +1244,12 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
// need to visit even if we only look for a setters for compatibility
if (visitor != null && getter != null) visitor.visitMethod(getter);
- if (readMode) {
+ PropertyNode propertyNode = current.getProperty(propertyName);
+ propertyNode = allowStaticAccessToMember(propertyNode, staticOnly);
+ //prefer explicit getter or setter over property if receiver is not 'this'
+ boolean checkGetterOrSetter = !isThisExpression || propertyNode == null;
+
+ if (readMode && checkGetterOrSetter) {
if (getter != null) {
ClassNode cn = inferReturnTypeGenerics(current, getter, ArgumentListExpression.EMPTY_ARGUMENTS);
storeInferredTypeForPropertyExpression(pexp, cn);
@@ -1256,7 +1259,7 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
pexp.putNodeMetaData(StaticTypesMarker.IMPLICIT_RECEIVER, delegationData);
return true;
}
- } else {
+ } else if (!readMode && checkGetterOrSetter) {
if (!setters.isEmpty()) {
if (visitor != null) {
if (field != null) {
@@ -1281,12 +1284,14 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
pexp.putNodeMetaData(StaticTypesMarker.IMPLICIT_RECEIVER, delegationData);
}
return true;
- } else if (getter != null) {
+ } else if (getter != null && propertyNode == null) {
pexp.putNodeMetaData(StaticTypesMarker.READONLY_PROPERTY, true);
}
}
foundGetterOrSetter = foundGetterOrSetter || !setters.isEmpty() || getter != null;
+ if (storeProperty(propertyNode, pexp, current, visitor, receiver.getData())) return true;
+
if (storeField(field, true, pexp, current, visitor, receiver.getData(), !readMode)) return true;
// if the property expression is an attribute expression (o.@attr), then
// we stop now, otherwise we must check the parent class
http://git-wip-us.apache.org/repos/asf/groovy/blob/10632b46/src/test/groovy/transform/stc/FieldsAndPropertiesSTCTest.groovy
----------------------------------------------------------------------
diff --git a/src/test/groovy/transform/stc/FieldsAndPropertiesSTCTest.groovy b/src/test/groovy/transform/stc/FieldsAndPropertiesSTCTest.groovy
index 90ce986..a99b27a 100644
--- a/src/test/groovy/transform/stc/FieldsAndPropertiesSTCTest.groovy
+++ b/src/test/groovy/transform/stc/FieldsAndPropertiesSTCTest.groovy
@@ -739,6 +739,80 @@ import org.codehaus.groovy.ast.stmt.AssertStatement
'''
}
+ void testPropertyStyleSetterArgShouldBeCheckedAgainstParamType() {
+ shouldFailWithMessages '''
+ class Foo {
+ Bar bar;
+
+ void setBar(int x) {
+ this.bar = new Bar(x: x)
+ }
+ }
+
+ class Bar {
+ int x
+ }
+
+ Foo foo = new Foo()
+ foo.bar = new Bar()
+ ''', 'Cannot assign value of type Bar to variable of type int'
+
+ assertScript '''
+ class Foo {
+ Bar bar;
+
+ void setBar(int x) {
+ this.bar = new Bar(x: x)
+ }
+ }
+
+ class Bar {
+ int x
+ }
+
+ Foo foo = new Foo()
+ foo.bar = 1
+ assert foo.bar.x == 1
+ '''
+ }
+
+ void testPropertyStyleGetterUsageShouldBeCheckedAgainstReturnType() {
+ shouldFailWithMessages '''
+ class Foo {
+ Bar bar;
+
+ int getBar() {
+ bar.x
+ }
+ }
+
+ class Bar {
+ int x
+ }
+
+ Foo foo = new Foo(bar: new Bar(x: 1))
+ Bar bar = foo.bar
+ ''', 'Cannot assign value of type int to variable of type Bar'
+
+ assertScript '''
+ class Foo {
+ Bar bar;
+
+ int getBar() {
+ bar.x
+ }
+ }
+
+ class Bar {
+ int x
+ }
+
+ Foo foo = new Foo(bar: new Bar(x: 1))
+ int x = foo.bar
+ assert x == 1
+ '''
+ }
+
public static interface InterfaceWithField {
String boo = "I don't fancy fields in interfaces"
}