You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by em...@apache.org on 2020/07/25 21:46:35 UTC
[groovy] branch GROOVY-9653 created (now 01108c4)
This is an automated email from the ASF dual-hosted git repository.
emilles pushed a change to branch GROOVY-9653
in repository https://gitbox.apache.org/repos/asf/groovy.git.
at 01108c4 GROOVY-9653: SC: improve classgen for multiple setter method candidates
This branch includes the following new commits:
new 01108c4 GROOVY-9653: SC: improve classgen for multiple setter method candidates
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
[groovy] 01/01: GROOVY-9653: SC: improve classgen for multiple
setter method candidates
Posted by em...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
emilles pushed a commit to branch GROOVY-9653
in repository https://gitbox.apache.org/repos/asf/groovy.git
commit 01108c4e709dedec0093466a1464ef813305f4fd
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Sat Jul 25 16:44:52 2020 -0500
GROOVY-9653: SC: improve classgen for multiple setter method candidates
---
.../groovy/classgen/AsmClassGenerator.java | 3 +-
...icTypesBinaryExpressionMultiTypeDispatcher.java | 24 +++---
.../transform/stc/StaticTypeCheckingVisitor.java | 17 +++--
.../stc/FieldsAndPropertiesSTCTest.groovy | 88 +++++++++++++---------
4 files changed, 79 insertions(+), 53 deletions(-)
diff --git a/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java b/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java
index 3277474..4506516 100644
--- a/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java
+++ b/src/main/java/org/codehaus/groovy/classgen/AsmClassGenerator.java
@@ -124,8 +124,8 @@ import java.util.Map;
import java.util.Objects;
import java.util.Optional;
-import static org.apache.groovy.util.BeanUtils.capitalize;
import static org.apache.groovy.ast.tools.ExpressionUtils.isThisOrSuper;
+import static org.apache.groovy.util.BeanUtils.capitalize;
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;
@@ -1311,6 +1311,7 @@ public class AsmClassGenerator extends ClassGenerator {
PropertyExpression pexp = thisPropX(true, variableName);
pexp.getObjectExpression().setSourcePosition(expression);
pexp.getProperty().setSourcePosition(expression);
+ pexp.copyNodeMetaData(expression);
pexp.visit(this);
}
diff --git a/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesBinaryExpressionMultiTypeDispatcher.java b/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesBinaryExpressionMultiTypeDispatcher.java
index b2a6b3c..5286c66 100644
--- a/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesBinaryExpressionMultiTypeDispatcher.java
+++ b/src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesBinaryExpressionMultiTypeDispatcher.java
@@ -47,7 +47,6 @@ import org.codehaus.groovy.syntax.Token;
import org.codehaus.groovy.syntax.TokenUtil;
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;
import org.objectweb.asm.Label;
import org.objectweb.asm.MethodVisitor;
@@ -172,10 +171,20 @@ public class StaticTypesBinaryExpressionMultiTypeDispatcher extends BinaryExpres
@Override
public void evaluateEqual(final BinaryExpression expression, final boolean defineVariable) {
Expression leftExpression = expression.getLeftExpression();
+ Expression rightExpression = expression.getRightExpression();
if (!defineVariable) {
if (leftExpression instanceof PropertyExpression) {
PropertyExpression pexp = (PropertyExpression) leftExpression;
- if (makeSetProperty(
+ MethodNode methodTarget = leftExpression.getNodeMetaData(DIRECT_METHOD_CALL_TARGET);
+ if (methodTarget != null) { // respect StaticTypeCheckingVisitor's setter method selection
+ MethodCallExpression call = callX(pexp.getObjectExpression(), methodTarget.getName(), rightExpression);
+ call.setMethodTarget(methodTarget); // force
+ call.setImplicitThis(pexp.isImplicitThis());
+ call.setSpreadSafe(pexp.isSpreadSafe());
+ call.setSafe(pexp.isSafe());
+ call.visit(controller.getAcg());
+ return;
+ } else if (makeSetProperty(
pexp.getObjectExpression(),
pexp.getProperty(),
expression.getRightExpression(),
@@ -186,16 +195,13 @@ public class StaticTypesBinaryExpressionMultiTypeDispatcher extends BinaryExpres
return;
}
}
- } else {
- Expression rightExpression = expression.getRightExpression();
- if (rightExpression instanceof LambdaExpression || rightExpression instanceof MethodReferenceExpression) {
- rightExpression.putNodeMetaData(INFERRED_FUNCTIONAL_INTERFACE_TYPE, leftExpression.getNodeMetaData(INFERRED_TYPE));
- }
+ // TODO: Handle leftExpression is VariableExpression and has DIRECT_METHOD_CALL_TARGET?
+ } else if (rightExpression instanceof LambdaExpression || rightExpression instanceof MethodReferenceExpression) {
+ rightExpression.putNodeMetaData(INFERRED_FUNCTIONAL_INTERFACE_TYPE, leftExpression.getNodeMetaData(INFERRED_TYPE));
}
// GROOVY-5620: spread-safe operator on LHS is not supported
if (leftExpression instanceof PropertyExpression
- && ((PropertyExpression) leftExpression).isSpreadSafe()
- && StaticTypeCheckingSupport.isAssignment(expression.getOperation().getType())) {
+ && ((PropertyExpression) leftExpression).isSpreadSafe()) {
// rewrite it so that it can be statically compiled
transformSpreadOnLHS(expression);
return;
diff --git a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
index f29513c..b384e56 100644
--- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
+++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
@@ -949,7 +949,7 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
* @param leftExpression left expression of the assignment
* @param rightExpression right expression of the assignment
* @param setterInfo possible setters
- * @return true if type checking passed
+ * @return {@code false} if valid setter found or {@code true} if type checking error created
*/
private boolean ensureValidSetter(final Expression expression, final Expression leftExpression, final Expression rightExpression, final SetterInfo setterInfo) {
// for expressions like foo = { ... }
@@ -985,16 +985,17 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
for (MethodNode setter : setterInfo.setters) {
if (setter == directSetterCandidate) {
leftExpression.putNodeMetaData(DIRECT_METHOD_CALL_TARGET, directSetterCandidate);
+ leftExpression.removeNodeMetaData(INFERRED_TYPE); // clear false assumption
storeType(leftExpression, getType(newRightExpression));
break;
}
}
+ return false;
} else {
- ClassNode firstSetterType = setterInfo.setters.iterator().next().getParameters()[0].getOriginType();
+ ClassNode firstSetterType = setterInfo.setters.get(0).getParameters()[0].getOriginType();
addAssignmentError(firstSetterType, getType(newRightExpression), expression);
return true;
}
- return false;
}
private boolean isCompoundAssignment(final Expression exp) {
@@ -1468,8 +1469,9 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
if (receiverType.isArray() && "length".equals(propertyName)) {
storeType(pexp, int_TYPE);
if (visitor != null) {
- PropertyNode length = new PropertyNode("length", Opcodes.ACC_PUBLIC | Opcodes.ACC_FINAL, int_TYPE, receiverType, null, null, null);
- visitor.visitProperty(length);
+ FieldNode length = new FieldNode("length", Opcodes.ACC_PUBLIC | Opcodes.ACC_FINAL, int_TYPE, receiverType, null);
+ length.setDeclaringClass(receiverType);
+ visitor.visitField(length);
}
return true;
}
@@ -1554,8 +1556,9 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
visitor.visitField(field);
} else {
for (MethodNode setter : setters) {
- ClassNode setterType = setter.getParameters()[0].getOriginType();
- FieldNode virtual = new FieldNode(propertyName, 0, setterType, current, EmptyExpression.INSTANCE);
+ // visiting setter will not infer the property type since return type is void, so visit a dummy field instead
+ FieldNode virtual = new FieldNode(propertyName, 0, setter.getParameters()[0].getOriginType(), current, null);
+ virtual.setDeclaringClass(setter.getDeclaringClass());
visitor.visitField(virtual);
}
}
diff --git a/src/test/groovy/transform/stc/FieldsAndPropertiesSTCTest.groovy b/src/test/groovy/transform/stc/FieldsAndPropertiesSTCTest.groovy
index 5db8505..ed0d16f 100644
--- a/src/test/groovy/transform/stc/FieldsAndPropertiesSTCTest.groovy
+++ b/src/test/groovy/transform/stc/FieldsAndPropertiesSTCTest.groovy
@@ -642,6 +642,26 @@ class FieldsAndPropertiesSTCTest extends StaticTypeCheckingTestCase {
'''
}
+ // GROOVY-9653
+ void testSetterInWithUsingPropertyNotation_DelegateAndOwnerHaveSetter() {
+ assertScript '''
+ class C {
+ final result = new D().with {
+ something = 'value' // ClassCastException: D cannot be cast to C
+ return object
+ }
+ void setSomething(value) { }
+ }
+
+ class D {
+ void setSomething(value) { }
+ Object getObject() { 'works' }
+ }
+
+ assert new C().result == 'works'
+ '''
+ }
+
// GROOVY-6230
void testAttributeWithGetterOfDifferentType() {
assertScript '''import java.awt.Dimension
@@ -778,56 +798,52 @@ import org.codehaus.groovy.ast.stmt.AssertStatement
void testPropertyAssignmentInSubClassAndMultiSetter() {
10.times {
assertScript '''
+ class A {
+ int which
- public class Activity {
- int debug
+ A() {
+ contentView = 42L
+ assert which == 2
+ }
- Activity() {
- contentView = 1
+ void setContentView(Date value) { which = 1 }
+ void setContentView(Long value) { which = 2 }
}
- public void setContentView(Date layoutResID) { debug = 2 }
- public void setContentView(int layoutResID) { debug = 3 }
- }
-
- class MyActivity extends Activity {
- void foo() {
- contentView = 1
- assert debug == 3
- contentView = new Date()
- assert debug == 2
+ class B extends A {
+ void m() {
+ contentView = 42L
+ assert which == 2
+ contentView = new Date()
+ assert which == 1
+ }
}
- }
- new MyActivity().foo()
- '''
+
+ new B().m()
+ '''
}
}
void testPropertyAssignmentInSubClassAndMultiSetterThroughDelegation() {
10.times {
- assertScript '''
-
- public class Activity {
- int debug
+ assertScript '''\
+ class A {
+ int which
- Activity() {
- contentView = 1
+ void setContentView(Date value) { which = 1 }
+ void setContentView(Long value) { which = 2 }
}
- public void setContentView(Date layoutResID) { debug = 2 }
- public void setContentView(int layoutResID) { debug = 3 }
- }
+ class B extends A {
+ }
- class MyActivity extends Activity {
- }
- def activity = new MyActivity()
- activity.with {
- contentView = 1
- assert debug == 3
- contentView = new Date()
- assert debug == 2
- }
- '''
+ new B().with {
+ contentView = 42L
+ assert which == 2
+ contentView = new Date()
+ assert which == 1
+ }
+ '''
}
}