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/06/14 12:42:18 UTC
[groovy] branch GROOVY-9344 updated: GROOVY-9344,
GROOVY-9516: track assign in closure like if/else branch
This is an automated email from the ASF dual-hosted git repository.
emilles pushed a commit to branch GROOVY-9344
in repository https://gitbox.apache.org/repos/asf/groovy.git
The following commit(s) were added to refs/heads/GROOVY-9344 by this push:
new 5e1af87 GROOVY-9344, GROOVY-9516: track assign in closure like if/else branch
5e1af87 is described below
commit 5e1af87425a952079de667029a2003d126d5e7e2
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Sun Jun 14 07:31:52 2020 -0500
GROOVY-9344, GROOVY-9516: track assign in closure like if/else branch
---
.../transform/stc/StaticTypeCheckingVisitor.java | 35 +++++++--------
.../groovy/transform/stc/ClosuresSTCTest.groovy | 50 ++++++++++++++--------
.../asm/sc/StaticCompileFlowTypingTest.groovy | 15 +++----
3 files changed, 55 insertions(+), 45 deletions(-)
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 067ab02..84e7e9c 100644
--- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
+++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
@@ -96,7 +96,6 @@ import org.codehaus.groovy.ast.stmt.TryCatchStatement;
import org.codehaus.groovy.ast.stmt.WhileStatement;
import org.codehaus.groovy.ast.tools.GenericsUtils;
import org.codehaus.groovy.ast.tools.WideningCategories;
-import org.codehaus.groovy.ast.tools.WideningCategories.LowestUpperBoundClassNode;
import org.codehaus.groovy.classgen.ReturnAdder;
import org.codehaus.groovy.classgen.asm.InvocationWriter;
import org.codehaus.groovy.control.CompilationUnit;
@@ -787,11 +786,8 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
}
// GROOVY-5874: if left expression is a closure shared variable, a second pass should be done
- if (leftExpression instanceof VariableExpression) {
- VariableExpression leftVar = (VariableExpression) leftExpression;
- if (leftVar.isClosureSharedVariable()) {
- typeCheckingContext.secondPassExpressions.add(new SecondPassExpression<>(expression));
- }
+ if (leftExpression instanceof VariableExpression && ((VariableExpression) leftExpression).isClosureSharedVariable()) {
+ typeCheckingContext.secondPassExpressions.add(new SecondPassExpression(expression));
}
boolean isAssignment = isAssignment(expression.getOperation().getType());
@@ -1922,13 +1918,12 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
}
protected boolean isSecondPassNeededForControlStructure(final Map<VariableExpression, ClassNode> varOrigType, final Map<VariableExpression, List<ClassNode>> oldTracker) {
- Map<VariableExpression, ClassNode> assignedVars = popAssignmentTracking(oldTracker);
- for (Map.Entry<VariableExpression, ClassNode> entry : assignedVars.entrySet()) {
+ for (Map.Entry<VariableExpression, ClassNode> entry : popAssignmentTracking(oldTracker).entrySet()) {
Variable key = findTargetVariable(entry.getKey());
- if (key instanceof VariableExpression) {
+ if (key instanceof VariableExpression && varOrigType.containsKey(key)) {
ClassNode origType = varOrigType.get(key);
ClassNode newType = entry.getValue();
- if (varOrigType.containsKey(key) && (!newType.equals(origType))) {
+ if (!newType.equals(origType)) {
return true;
}
}
@@ -2381,12 +2376,16 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
protected void restoreVariableExpressionMetadata(final Map<VariableExpression, Map<StaticTypesMarker, Object>> typesBeforeVisit) {
if (typesBeforeVisit != null) {
for (Map.Entry<VariableExpression, Map<StaticTypesMarker, Object>> entry : typesBeforeVisit.entrySet()) {
- VariableExpression ve = entry.getKey();
- Map<StaticTypesMarker, Object> metadata = entry.getValue();
for (StaticTypesMarker marker : StaticTypesMarker.values()) {
- ve.removeNodeMetaData(marker);
- Object value = metadata.get(marker);
- if (value != null) ve.setNodeMetaData(marker, value);
+ // GROOVY-9344, GROOVY-9516
+ if (marker == INFERRED_TYPE) continue;
+
+ Object value = entry.getValue().get(marker);
+ if (value == null) {
+ entry.getKey().removeNodeMetaData(marker);
+ } else {
+ entry.getKey().putNodeMetaData(marker, value);
+ }
}
}
}
@@ -3383,7 +3382,7 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
}
}
if (mn.isEmpty() && typeCheckingContext.getEnclosingClosure() != null && args.length == 0) {
- // add special handling of getDelegate() and getOwner()
+ // add special handling of "delegate", "owner", and "this" in a closure
switch (name) {
case "getDelegate":
mn = Collections.singletonList(GET_DELEGATE);
@@ -3589,7 +3588,7 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
nodes.add(arg);
}
}
- return new LowestUpperBoundClassNode(returnType.getName() + "Composed", OBJECT_TYPE, nodes.toArray(ClassNode.EMPTY_ARRAY));
+ return new WideningCategories.LowestUpperBoundClassNode(returnType.getName() + "Composed", OBJECT_TYPE, nodes.toArray(ClassNode.EMPTY_ARRAY));
}
}
return returnType;
@@ -5475,8 +5474,6 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport {
if (expression instanceof BinaryExpression) {
Expression left = ((BinaryExpression) expression).getLeftExpression();
if (left instanceof VariableExpression) {
- // should always be the case
- // this should always be the case, but adding a test is safer
Variable target = findTargetVariable((VariableExpression) left);
if (target instanceof VariableExpression) {
VariableExpression var = (VariableExpression) target;
diff --git a/src/test/groovy/transform/stc/ClosuresSTCTest.groovy b/src/test/groovy/transform/stc/ClosuresSTCTest.groovy
index 108376d..e01e306 100644
--- a/src/test/groovy/transform/stc/ClosuresSTCTest.groovy
+++ b/src/test/groovy/transform/stc/ClosuresSTCTest.groovy
@@ -35,10 +35,8 @@ class ClosuresSTCTest extends StaticTypeCheckingTestCase {
void testClosureWithoutArgumentsExplicit() {
// GROOVY-9079: no params to statically type check but shouldn't get NPE
assertScript '''
- import groovy.transform.CompileStatic
import java.util.concurrent.Callable
- @CompileStatic
String makeFoo() {
Callable<String> call = { -> 'foo' }
call()
@@ -127,20 +125,36 @@ class ClosuresSTCTest extends StaticTypeCheckingTestCase {
'''
}
- void testClosureShouldNotChangeInferredType() {
+ void testClosureSharedVariable1() {
assertScript '''
def x = '123';
{ -> x = new StringBuffer() }
- x.charAt(0)
+ x.charAt(0) // available in String and StringBuffer
'''
}
- void testClosureSharedVariableWithIncompatibleType() {
+ void testClosureSharedVariable2() {
shouldFailWithMessages '''
def x = '123';
- { -> x = 1 }
- x.charAt(0)
- ''', 'A closure shared variable [x] has been assigned with various types and the method [charAt(int)] does not exist in the lowest upper bound'
+ { -> x = 123 }
+ x.charAt(0) // available in String but not available in Integer
+ ''', 'Cannot find matching method java.io.Serializable or java.lang.Comparable#charAt(int)'
+ }
+
+ // GROOVY-9516
+ void testClosureSharedVariable3() {
+ shouldFailWithMessages '''
+ class A {}
+ class B extends A { def m() {} }
+ class C extends A {}
+
+ void test() {
+ def x = new B();
+ { -> x = new C() }();
+ def c = x
+ c.m()
+ }
+ ''', 'Cannot find matching method A#m()'
}
void testClosureCallAsAMethod() {
@@ -186,7 +200,7 @@ class ClosuresSTCTest extends StaticTypeCheckingTestCase {
fib.fib(2)
'''
}
-
+
void testClosureRecursionWithoutClosureTypeArgument() {
shouldFailWithMessages '''
Closure fib
@@ -232,6 +246,7 @@ class ClosuresSTCTest extends StaticTypeCheckingTestCase {
}
'''
}
+
// a case in Grails
void testShouldNotThrowClosureSharedVariableError2() {
assertScript '''
@@ -306,9 +321,9 @@ class ClosuresSTCTest extends StaticTypeCheckingTestCase {
}
''', 'Cannot find matching method'
}
-
- //GROOVY-6189
- void testSAMsInMethodSelection(){
+
+ // GROOVY-6189
+ void testSAMsInMethodSelection() {
// simple direct case
assertScript """
interface MySAM {
@@ -317,7 +332,7 @@ class ClosuresSTCTest extends StaticTypeCheckingTestCase {
def foo(MySAM sam) {sam.someMethod()}
assert foo {1} == 1
"""
-
+
// overloads with classes implemented by Closure
["java.util.concurrent.Callable", "Object", "Closure", "GroovyObjectSupport", "Cloneable", "Runnable", "GroovyCallable", "Serializable", "GroovyObject"].each {
className ->
@@ -331,7 +346,7 @@ class ClosuresSTCTest extends StaticTypeCheckingTestCase {
"""
}
}
-
+
void testSAMVariable() {
assertScript """
interface SAM { def foo(); }
@@ -354,7 +369,7 @@ class ClosuresSTCTest extends StaticTypeCheckingTestCase {
assert s.accept(1) == -1
"""
}
-
+
void testSAMProperty() {
assertScript """
interface SAM { def foo(); }
@@ -365,7 +380,7 @@ class ClosuresSTCTest extends StaticTypeCheckingTestCase {
assert x.s.foo() == 1
"""
}
-
+
void testSAMAttribute() {
assertScript """
interface SAM { def foo(); }
@@ -446,7 +461,7 @@ class ClosuresSTCTest extends StaticTypeCheckingTestCase {
}
''', 'Reference to method is ambiguous. Cannot choose between'
}
-
+
void testSAMType() {
assertScript """
interface Foo {int foo()}
@@ -537,4 +552,3 @@ class ClosuresSTCTest extends StaticTypeCheckingTestCase {
'''
}
}
-
diff --git a/src/test/org/codehaus/groovy/classgen/asm/sc/StaticCompileFlowTypingTest.groovy b/src/test/org/codehaus/groovy/classgen/asm/sc/StaticCompileFlowTypingTest.groovy
index 86632f8..4b48fe4 100644
--- a/src/test/org/codehaus/groovy/classgen/asm/sc/StaticCompileFlowTypingTest.groovy
+++ b/src/test/org/codehaus/groovy/classgen/asm/sc/StaticCompileFlowTypingTest.groovy
@@ -18,7 +18,6 @@
*/
package org.codehaus.groovy.classgen.asm.sc
-import groovy.test.NotYetImplemented
import groovy.transform.CompileStatic
import org.junit.Test
@@ -51,10 +50,10 @@ final class StaticCompileFlowTypingTest {
@groovy.transform.CompileStatic
String m() {
- def var = new A()
+ def x = new A()
def c = { ->
- var = new B()
- var.class.simpleName
+ x = new B()
+ x.class.simpleName
}
c()
}
@@ -62,7 +61,7 @@ final class StaticCompileFlowTypingTest {
'''
}
- @NotYetImplemented @Test // GROOVY-9344
+ @Test // GROOVY-9344
void testFlowTyping3() {
assertScript '''
class A {}
@@ -70,12 +69,12 @@ final class StaticCompileFlowTypingTest {
@groovy.transform.CompileStatic
String m() {
- def var = new A()
+ def x = new A()
def c = { ->
- var = new B()
+ x = new B()
}
c()
- var.class.simpleName
+ x.class.simpleName
}
assert m() == 'B'
'''