Skip to content

Commit 4ca951d

Browse files
eric-milleseric-milles
committed
GROOVY-7971, GROOVY-8411, GROOVY-8965: logical-or instanceof guards
1 parent cc8bc3e commit 4ca951d

File tree

5 files changed

+109
-46
lines changed

5 files changed

+109
-46
lines changed

‎src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesTypeChooser.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ public ClassNode resolveType(final Expression exp, final ClassNode current) {
4343
var ast = getTarget(exp); // GROOVY-9344, GROOVY-9607, GROOVY-11375
4444
inferredType = ast.getNodeMetaData(StaticTypesMarker.INFERRED_TYPE);
4545
}
46-
if (inferredType != null && !isPrimitiveVoid(inferredType)) {
46+
if (inferredType != null && !isPrimitiveVoid(inferredType) && !inferredType.getName().startsWith("(")) {
4747
return inferredType;
4848
}
4949

‎src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java‎

Lines changed: 94 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,7 @@
266266
import static org.codehaus.groovy.syntax.Types.INTDIV_EQUAL;
267267
import static org.codehaus.groovy.syntax.Types.KEYWORD_IN;
268268
import static org.codehaus.groovy.syntax.Types.KEYWORD_INSTANCEOF;
269+
import static org.codehaus.groovy.syntax.Types.LOGICAL_OR;
269270
import static org.codehaus.groovy.syntax.Types.MINUS_MINUS;
270271
import static org.codehaus.groovy.syntax.Types.MOD;
271272
import static org.codehaus.groovy.syntax.Types.MOD_EQUAL;
@@ -848,10 +849,13 @@ public Expression transform(final Expression expr) {
848849
return;
849850
}
850851

852+
// GROOVY-7971, GROOVY-8965, GROOVY-10096, GROOVY-10702, et al.
853+
if (op == LOGICAL_OR) typeCheckingContext.pushTemporaryTypeInfo();
854+
851855
leftExpression.visit(this);
852856
ClassNode lType = getType(leftExpression);
853857
var setterInfo = removeSetterInfo(leftExpression);
854-
if (setterInfo != null) {
858+
if (setterInfo != null) { assert op != LOGICAL_OR ;
855859
if (ensureValidSetter(expression, leftExpression, rightExpression, setterInfo)) {
856860
return;
857861
}
@@ -860,7 +864,16 @@ public Expression transform(final Expression expr) {
860864
lType = getOriginalDeclarationType(leftExpression);
861865
applyTargetType(lType, rightExpression);
862866
}
863-
rightExpression.visit(this);
867+
if (op != LOGICAL_OR) {
868+
rightExpression.visit(this);
869+
} else {
870+
var lhs = typeCheckingContext.temporaryIfBranchTypeInformation.pop();
871+
typeCheckingContext.pushTemporaryTypeInfo();
872+
rightExpression.visit(this);
873+
874+
var rhs = typeCheckingContext.temporaryIfBranchTypeInformation.pop();
875+
propagateTemporaryTypeInfo(lhs, rhs); // `instanceof` on either side?
876+
}
864877
}
865878

866879
ClassNode rType = isNullConstant(rightExpression)
@@ -973,7 +986,7 @@ && isAssignment(enclosingBinaryExpression.getOperation().getType())) {
973986
} else if (op == KEYWORD_INSTANCEOF) {
974987
pushInstanceOfTypeInfo(leftExpression, rightExpression);
975988
} else if (op == COMPARE_NOT_INSTANCEOF) { // GROOVY-6429, GROOVY-8321, GROOVY-8412, GROOVY-8523, GROOVY-9931
976-
putNotInstanceOfTypeInfo(extractTemporaryTypeInfoKey(leftExpression), Collections.singleton(rightExpression.getType()));
989+
putNotInstanceOfTypeInfo(extractTemporaryTypeInfoKey(leftExpression), Set.of(rightExpression.getType()));
977990
}
978991
if (!isEmptyDeclaration) {
979992
storeType(expression, resultType);
@@ -989,6 +1002,58 @@ && isAssignment(enclosingBinaryExpression.getOperation().getType())) {
9891002
}
9901003
}
9911004

1005+
private void propagateTemporaryTypeInfo(final Map<Object, List<ClassNode>> lhs,
1006+
final Map<Object, List<ClassNode>> rhs) {
1007+
// TODO: deal with (x !instanceof T)
1008+
lhs.keySet().removeIf(k -> k instanceof Object[]);
1009+
rhs.keySet().removeIf(k -> k instanceof Object[]);
1010+
1011+
for (var entry : lhs.entrySet()) {
1012+
if (rhs.containsKey(entry.getKey())) {
1013+
// main case: (x instanceof A || x instanceof B) produces A|B type
1014+
ClassNode t = unionize(entry.getValue(), rhs.get(entry.getKey()));
1015+
1016+
var tti = typeCheckingContext.temporaryIfBranchTypeInformation.peek()
1017+
.computeIfAbsent(entry.getKey(), x -> new LinkedList<>());
1018+
tti.add(t);
1019+
} else if (entry.getKey() instanceof Variable v) {
1020+
// edge case: (x instanceof A || ...) produces A|typeof(x) type
1021+
ClassNode t = unionize(entry.getValue(), List.of(v instanceof ASTNode n ? getType(n) : v.getType()));
1022+
1023+
var tti = typeCheckingContext.temporaryIfBranchTypeInformation.peek()
1024+
.computeIfAbsent(entry.getKey(), x -> new LinkedList<>());
1025+
tti.add(t);
1026+
}
1027+
}
1028+
1029+
rhs.keySet().removeAll(lhs.keySet());
1030+
1031+
for (var entry : rhs.entrySet()) {
1032+
// edge case: (... || x instanceof B) produces typeof(x)|B type
1033+
System.err.println("TODO");
1034+
}
1035+
}
1036+
1037+
private static ClassNode unionize(final List<ClassNode> a, final List<ClassNode> b) {
1038+
List<ClassNode> types = new ArrayList<>(a.size() + b.size());
1039+
for (ClassNode t : a) {
1040+
if (t instanceof UnionTypeClassNode u) {
1041+
Collections.addAll(types, u.getDelegates());
1042+
} else {
1043+
types.add(t);
1044+
}
1045+
}
1046+
for (ClassNode t : b) {
1047+
if (t instanceof UnionTypeClassNode u) {
1048+
Collections.addAll(types, u.getDelegates());
1049+
} else {
1050+
types.add(t);
1051+
}
1052+
}
1053+
assert types.size() > 1;
1054+
return new UnionTypeClassNode(types.toArray(ClassNode[]::new));
1055+
}
1056+
9921057
private void validateResourceInARM(final BinaryExpression expression, final ClassNode lType) {
9931058
if (expression instanceof DeclarationExpression
9941059
&& TryCatchStatement.isResource(expression)
@@ -1170,8 +1235,8 @@ private boolean ensureValidSetter(final Expression expression, final Expression
11701235
}
11711236
addStaticTypeError(message, leftExpression);
11721237
} else {
1173-
ClassNode[] tergetTypes = visibleSetters.stream().map(setterType).toArray(ClassNode[]::new);
1174-
addAssignmentError(tergetTypes.length == 1 ? tergetTypes[0] : new UnionTypeClassNode(tergetTypes), getType(valueExpression), expression);
1238+
ClassNode[] targetTypes = visibleSetters.stream().map(setterType).toArray(ClassNode[]::new);
1239+
addAssignmentError(targetTypes.length == 1 ? targetTypes[0] : new UnionTypeClassNode(targetTypes), getType(valueExpression), expression);
11751240
}
11761241
return true;
11771242
}
@@ -1492,7 +1557,7 @@ protected void checkGroovyConstructorMap(final Expression receiver, final ClassN
14921557
ClassNode valueType = getType(valueExpression);
14931558
BinaryExpression kv = typeCheckingContext.popEnclosingBinaryExpression();
14941559
if (propertyTypes.stream().noneMatch(targetType -> checkCompatibleAssignmentTypes(targetType, getResultType(targetType, ASSIGN, valueType, kv), valueExpression))) {
1495-
ClassNode targetType = propertyTypes.size() == 1 ? propertyTypes.iterator().next() : new UnionTypeClassNode(propertyTypes.toArray(ClassNode.EMPTY_ARRAY));
1560+
ClassNode targetType = propertyTypes.size() == 1 ? propertyTypes.iterator().next() : new UnionTypeClassNode(propertyTypes.toArray(ClassNode[]::new));
14961561
if (!extension.handleIncompatibleAssignment(targetType, valueType, entryExpression)) {
14971562
addAssignmentError(targetType, valueType, entryExpression);
14981563
}
@@ -4005,18 +4070,16 @@ protected List<Receiver<String>> makeOwnerList(final Expression objectExpression
40054070
ClassNode receiver = getType(objectExpression);
40064071
List<Receiver<String>> owners = new ArrayList<>();
40074072
if (typeCheckingContext.delegationMetadata != null
4008-
&& objectExpression instanceof VariableExpression
4009-
&& "owner".equals(((Variable) objectExpression).getName())
4010-
&& /*isNested:*/typeCheckingContext.delegationMetadata.getParent() != null) {
4073+
&& /*isNested:*/typeCheckingContext.delegationMetadata.getParent() != null
4074+
&& objectExpression instanceof VariableExpression v && v.getName().equals("owner")) {
40114075
owners.add(new Receiver<>(receiver, "owner")); // if nested, Closure is the first owner
40124076
List<Receiver<String>> thisType = new ArrayList<>(2); // and the enclosing class is the
40134077
addDelegateReceiver(thisType, makeThis(), null); // end of the closure owner chain
40144078
addReceivers(owners, thisType, typeCheckingContext.delegationMetadata.getParent(), "owner.");
40154079
} else {
4016-
List<ClassNode> temporaryTypes = getTemporaryTypesForExpression(objectExpression);
4017-
int temporaryTypesCount = (temporaryTypes != null ? temporaryTypes.size() : 0);
4018-
if (temporaryTypesCount > 0) { // GROOVY-8965, GROOVY-10180, GROOVY-10668
4019-
owners.add(Receiver.make(lowestUpperBound(temporaryTypes)));
4080+
// GROOVY-8965, GROOVY-10180, GROOVY-10668: `instanceof` type before declared
4081+
for (ClassNode tempType : getTemporaryTypesForExpression(objectExpression)) {
4082+
if (tempType != receiver) owners.add(Receiver.make(tempType));
40204083
}
40214084
if (isClassClassNodeWrappingConcreteType(receiver)) {
40224085
ClassNode staticType = receiver.getGenericsTypes()[0].getType();
@@ -4033,9 +4096,6 @@ protected List<Receiver<String>> makeOwnerList(final Expression objectExpression
40334096
}
40344097
}
40354098
}
4036-
if (temporaryTypesCount > 1 && !(objectExpression instanceof VariableExpression)) {
4037-
owners.add(Receiver.make(new UnionTypeClassNode(temporaryTypes.toArray(ClassNode.EMPTY_ARRAY))));
4038-
}
40394099
}
40404100
return owners;
40414101
}
@@ -4248,16 +4308,22 @@ protected void afterSwitchConditionExpressionVisited(final SwitchStatement state
42484308

42494309
@Override
42504310
protected void afterSwitchCaseStatementsVisited(final SwitchStatement statement) {
4311+
List<ClassNode> caseTypes;
42514312
// GROOVY-8411: if any "case Type:" then "default:" contributes condition type
4252-
if (!statement.getDefaultStatement().isEmpty() && !typeCheckingContext.temporaryIfBranchTypeInformation.peek().isEmpty())
4253-
pushInstanceOfTypeInfo(statement.getExpression(), new ClassExpression(statement.getExpression().getNodeMetaData(TYPE)));
4313+
if (!statement.getDefaultStatement().isEmpty() && (caseTypes = typeCheckingContext.temporaryIfBranchTypeInformation.peek().remove(extractTemporaryTypeInfoKey(statement.getExpression()))) != null)
4314+
pushInstanceOfTypeInfo(statement.getExpression(), new ClassExpression(unionize(caseTypes, List.of((ClassNode) statement.getExpression().getNodeMetaData(TYPE)))));
42544315
}
42554316

42564317
@Override
42574318
public void visitCaseStatement(final CaseStatement statement) {
42584319
Expression selectable = typeCheckingContext.getEnclosingSwitchStatement().getExpression();
42594320
Expression expression = statement.getExpression();
42604321
if (expression instanceof ClassExpression) { // GROOVY-8411: refine the switch type
4322+
var types = typeCheckingContext.temporaryIfBranchTypeInformation.peek().remove(extractTemporaryTypeInfoKey(selectable));
4323+
if (types != null) {
4324+
ClassNode t = unionize(types, List.of(expression.getType()));
4325+
expression = new ClassExpression(t);
4326+
}
42614327
pushInstanceOfTypeInfo(selectable, expression);
42624328
} else if (expression instanceof ClosureExpression) { // GROOVY-9854: propagate the switch type
42634329
ClassNode inf = selectable.getNodeMetaData(TYPE);
@@ -5341,7 +5407,7 @@ private static ClassNode makeSelf(final ClassNode trait) {
53415407
if (!selfTypes.isEmpty()) { // TODO: reduce to the most-specific type(s)
53425408
ClassNode superclass = selfTypes.stream().filter(t -> !t.isInterface()).findFirst().orElse(OBJECT_TYPE);
53435409
selfTypes.remove(superclass); selfTypes.add(trait);
5344-
return new WideningCategories.LowestUpperBoundClassNode("TypesOf$" + trait.getNameWithoutPackage(), superclass, selfTypes.toArray(ClassNode.EMPTY_ARRAY));
5410+
return new WideningCategories.LowestUpperBoundClassNode("TypesOf$" + trait.getNameWithoutPackage(), superclass, selfTypes.toArray(ClassNode[]::new));
53455411
}
53465412
return trait;
53475413
}
@@ -6209,7 +6275,7 @@ protected List<ClassNode> getTemporaryTypesForExpression(final Expression expres
62096275
}
62106276

62116277
private ClassNode getInferredTypeFromTempInfo(final Expression expression, final ClassNode expressionType) {
6212-
if (expression instanceof VariableExpression) {
6278+
if (expression instanceof VariableExpression && !isPrimitiveType(expressionType)) {
62136279
List<ClassNode> tempTypes = getTemporaryTypesForExpression(expression);
62146280
if (!tempTypes.isEmpty()) {
62156281
ClassNode superclass;
@@ -6249,10 +6315,15 @@ private ClassNode getInferredTypeFromTempInfo(final Expression expression, final
62496315
}
62506316

62516317
int typesCount = types.size();
6252-
if (typesCount == 1) {
6318+
if (typesCount == 1 || (typesCount != 0 && types.stream().filter(ClassNode::isInterface).count() == 0)) {
62536319
return types.get(0);
6254-
} else if (typesCount > 1) {
6255-
return new UnionTypeClassNode(types.toArray(ClassNode.EMPTY_ARRAY));
6320+
} else if (typesCount > 1) { // create disjunction type (A&B)
6321+
String names = types.stream().map(StaticTypeCheckingSupport::prettyPrintType).collect(Collectors.joining(" & ", "(", ")"));
6322+
Map<Boolean, List<ClassNode>> ifacesAndClasses = types.stream().collect(Collectors.partitioningBy(ClassNode::isInterface));
6323+
ClassNode sc = !ifacesAndClasses.get(Boolean.FALSE).isEmpty() ? ifacesAndClasses.get(Boolean.FALSE).get(0) : OBJECT_TYPE;
6324+
ClassNode[] is = ifacesAndClasses.get(Boolean.TRUE).toArray(ClassNode[]::new);
6325+
assert !isObjectType(sc) || (is.length > 1) : "expecting specific super class or multiple interfaces";
6326+
return new ClassNode(names, Opcodes.ACC_ABSTRACT, sc, is, null);
62566327
}
62576328
}
62586329
}

‎src/test/groovy/groovy/transform/stc/MethodCallsSTCTest.groovy‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -875,6 +875,17 @@ class MethodCallsSTCTest extends StaticTypeCheckingTestCase {
875875
''',
876876
'Cannot find matching method','#foo(java.util.Date)',
877877
'Cannot find matching method','#bar(java.util.Date)'
878+
879+
shouldFailWithMessages foo + '''
880+
def it = ~/regexp/
881+
if (it !instanceof String) {
882+
it = 123
883+
Integer.toHextString(it)
884+
} else { // cannot be String
885+
foo(it)
886+
}
887+
''',
888+
'Cannot find matching method','#foo(java.util.regex.Pattern)'
878889
}
879890

880891
// GROOVY-5226, GROOVY-11290
@@ -903,16 +914,6 @@ class MethodCallsSTCTest extends StaticTypeCheckingTestCase {
903914
}
904915
}
905916
'''
906-
907-
assertScript foobar + '''
908-
def it = ~/regexp/
909-
if (it !instanceof String) {
910-
it = 123
911-
foo(it)
912-
} else {
913-
bar(it)
914-
}
915-
'''
916917
}
917918

918919
// GROOVY-5226, GROOVY-11290

‎src/test/groovy/groovy/transform/stc/TypeInferenceSTCTest.groovy‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
*/
1919
package groovy.transform.stc
2020

21-
import groovy.test.NotYetImplemented
2221
import org.codehaus.groovy.ast.ClassHelper
2322
import org.codehaus.groovy.ast.ClassNode
2423
import org.codehaus.groovy.ast.MethodNode
@@ -243,7 +242,6 @@ class TypeInferenceSTCTest extends StaticTypeCheckingTestCase {
243242
}
244243

245244
// GROOVY-10096
246-
@NotYetImplemented
247245
void testInstanceOf10() {
248246
shouldFailWithMessages '''
249247
class Foo {
@@ -313,7 +311,7 @@ class TypeInferenceSTCTest extends StaticTypeCheckingTestCase {
313311
// ^ (AbstractCollection<String> & Serializable & ... & Deque)
314312
}
315313
''',
316-
'Cannot call <UnionType:java.util.AbstractCollection','#add(java.lang.String) with arguments [int]'
314+
'Cannot call (java.util.AbstractCollection','#add(java.lang.String) with arguments [int]'
317315
}
318316

319317
// GROOVY-5226
@@ -1249,7 +1247,7 @@ class TypeInferenceSTCTest extends StaticTypeCheckingTestCase {
12491247
case File:
12501248
something.toString()
12511249
default:
1252-
something.getCanonicalName()
1250+
something.canonicalName // TODO: GROOVY-10702
12531251
}
12541252
}
12551253
''',

‎src/test/groovy/org/codehaus/groovy/classgen/asm/sc/TypeInferenceStaticCompileTest.groovy‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,17 +18,10 @@
1818
*/
1919
package org.codehaus.groovy.classgen.asm.sc
2020

21-
import groovy.test.NotYetImplemented
2221
import groovy.transform.stc.TypeInferenceSTCTest
2322

2423
/**
2524
* Unit tests for static compilation : type inference.
2625
*/
27-
class TypeInferenceStaticCompileTest extends TypeInferenceSTCTest implements StaticCompilationTestSupport {
28-
29-
@Override
30-
@NotYetImplemented
31-
void testInstanceOf9() {
32-
super.testInstanceOf9() // GROOVY-7971
33-
}
26+
final class TypeInferenceStaticCompileTest extends TypeInferenceSTCTest implements StaticCompilationTestSupport {
3427
}

0 commit comments

Comments
 (0)