This is an automated email from the ASF dual-hosted git repository. emilles pushed a commit to branch GROOVY-7971alt in repository https://gitbox.apache.org/repos/asf/groovy.git
commit 703deab59def63a6fbfeae7da931ed43026ec843 Author: Eric Milles <[email protected]> AuthorDate: Wed Dec 10 10:12:13 2025 -0600 GROOVY-7971, GROOVY-8411, GROOVY-8965: logical-or `instanceof` guards --- .../transform/stc/StaticTypeCheckingVisitor.java | 117 +++++++++++++++++---- .../groovy/transform/stc/MethodCallsSTCTest.groovy | 21 ++-- .../transform/stc/TypeInferenceSTCTest.groovy | 6 +- 3 files changed, 107 insertions(+), 37 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 bf4451ecae..e1713fb2e5 100644 --- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java +++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java @@ -266,6 +266,7 @@ import static org.codehaus.groovy.syntax.Types.INTDIV; import static org.codehaus.groovy.syntax.Types.INTDIV_EQUAL; import static org.codehaus.groovy.syntax.Types.KEYWORD_IN; import static org.codehaus.groovy.syntax.Types.KEYWORD_INSTANCEOF; +import static org.codehaus.groovy.syntax.Types.LOGICAL_OR; import static org.codehaus.groovy.syntax.Types.MINUS_MINUS; import static org.codehaus.groovy.syntax.Types.MOD; import static org.codehaus.groovy.syntax.Types.MOD_EQUAL; @@ -848,10 +849,13 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { return; } + // GROOVY-7971, GROOVY-8965, GROOVY-10096, GROOVY-10702, et al. + if (op == LOGICAL_OR) typeCheckingContext.pushTemporaryTypeInfo(); + leftExpression.visit(this); ClassNode lType = getType(leftExpression); var setterInfo = removeSetterInfo(leftExpression); - if (setterInfo != null) { + if (setterInfo != null) { assert op != LOGICAL_OR ; if (ensureValidSetter(expression, leftExpression, rightExpression, setterInfo)) { return; } @@ -860,7 +864,16 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { lType = getOriginalDeclarationType(leftExpression); applyTargetType(lType, rightExpression); } - rightExpression.visit(this); + if (op != LOGICAL_OR) { + rightExpression.visit(this); + } else { + var lhs = typeCheckingContext.temporaryIfBranchTypeInformation.pop(); + typeCheckingContext.pushTemporaryTypeInfo(); + rightExpression.visit(this); + + var rhs = typeCheckingContext.temporaryIfBranchTypeInformation.pop(); + propagateTemporaryTypeInfo(lhs, rhs); // `instanceof` on either side? + } } ClassNode rType = isNullConstant(rightExpression) @@ -973,7 +986,7 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { } else if (op == KEYWORD_INSTANCEOF) { pushInstanceOfTypeInfo(leftExpression, rightExpression); } else if (op == COMPARE_NOT_INSTANCEOF) { // GROOVY-6429, GROOVY-8321, GROOVY-8412, GROOVY-8523, GROOVY-9931 - putNotInstanceOfTypeInfo(extractTemporaryTypeInfoKey(leftExpression), Collections.singleton(rightExpression.getType())); + putNotInstanceOfTypeInfo(extractTemporaryTypeInfoKey(leftExpression), Set.of(rightExpression.getType())); } if (!isEmptyDeclaration) { storeType(expression, resultType); @@ -989,6 +1002,58 @@ public class StaticTypeCheckingVisitor extends ClassCodeVisitorSupport { } } + private void propagateTemporaryTypeInfo(final Map<Object, List<ClassNode>> lhs, + final Map<Object, List<ClassNode>> rhs) { + // TODO: deal with (x !instanceof T) + lhs.keySet().removeIf(k -> k instanceof Object[]); + rhs.keySet().removeIf(k -> k instanceof Object[]); + + for (var entry : lhs.entrySet()) { + if (rhs.containsKey(entry.getKey())) { + // main case: (x instanceof A || x instanceof B) produces A|B type + ClassNode t = unionize(entry.getValue(), rhs.get(entry.getKey())); + + var tti = typeCheckingContext.temporaryIfBranchTypeInformation.peek() + .computeIfAbsent(entry.getKey(), x -> new LinkedList<>()); + tti.add(t); + } else if (entry.getKey() instanceof Variable v) { + // edge case: (x instanceof A || ...) produces A|typeof(x) type + ClassNode t = unionize(entry.getValue(), List.of(v instanceof ASTNode n ? getType(n) : v.getType())); + + var tti = typeCheckingContext.temporaryIfBranchTypeInformation.peek() + .computeIfAbsent(entry.getKey(), x -> new LinkedList<>()); + tti.add(t); + } + } + + rhs.keySet().removeAll(lhs.keySet()); + + for (var entry : rhs.entrySet()) { + // edge case: (... || x instanceof B) produces typeof(x)|B type + System.err.println("TODO"); + } + } + + private static ClassNode unionize(final List<ClassNode> a, final List<ClassNode> b) { + List<ClassNode> types = new ArrayList<>(a.size() + b.size()); + for (ClassNode t : a) { + if (t instanceof UnionTypeClassNode u) { + Collections.addAll(types, u.getDelegates()); + } else { + types.add(t); + } + } + for (ClassNode t : b) { + if (t instanceof UnionTypeClassNode u) { + Collections.addAll(types, u.getDelegates()); + } else { + types.add(t); + } + } + assert types.size() > 1; + return new UnionTypeClassNode(types.toArray(ClassNode[]::new)); + } + private void validateResourceInARM(final BinaryExpression expression, final ClassNode lType) { if (expression instanceof DeclarationExpression && TryCatchStatement.isResource(expression) @@ -1170,8 +1235,8 @@ out: if ((samParameterTypes.length == 1 && isOrImplements(samParameterTypes[0 } addStaticTypeError(message, leftExpression); } else { - ClassNode[] tergetTypes = visibleSetters.stream().map(setterType).toArray(ClassNode[]::new); - addAssignmentError(tergetTypes.length == 1 ? tergetTypes[0] : new UnionTypeClassNode(tergetTypes), getType(valueExpression), expression); + ClassNode[] targetTypes = visibleSetters.stream().map(setterType).toArray(ClassNode[]::new); + addAssignmentError(targetTypes.length == 1 ? targetTypes[0] : new UnionTypeClassNode(targetTypes), getType(valueExpression), expression); } return true; } @@ -1492,7 +1557,7 @@ out: if ((samParameterTypes.length == 1 && isOrImplements(samParameterTypes[0 ClassNode valueType = getType(valueExpression); BinaryExpression kv = typeCheckingContext.popEnclosingBinaryExpression(); if (propertyTypes.stream().noneMatch(targetType -> checkCompatibleAssignmentTypes(targetType, getResultType(targetType, ASSIGN, valueType, kv), valueExpression))) { - ClassNode targetType = propertyTypes.size() == 1 ? propertyTypes.iterator().next() : new UnionTypeClassNode(propertyTypes.toArray(ClassNode.EMPTY_ARRAY)); + ClassNode targetType = propertyTypes.size() == 1 ? propertyTypes.iterator().next() : new UnionTypeClassNode(propertyTypes.toArray(ClassNode[]::new)); if (!extension.handleIncompatibleAssignment(targetType, valueType, entryExpression)) { addAssignmentError(targetType, valueType, entryExpression); } @@ -4005,18 +4070,16 @@ out: if (mn.size() != 1) { ClassNode receiver = getType(objectExpression); List<Receiver<String>> owners = new ArrayList<>(); if (typeCheckingContext.delegationMetadata != null - && objectExpression instanceof VariableExpression - && "owner".equals(((Variable) objectExpression).getName()) - && /*isNested:*/typeCheckingContext.delegationMetadata.getParent() != null) { + && /*isNested:*/typeCheckingContext.delegationMetadata.getParent() != null + && objectExpression instanceof VariableExpression v && v.getName().equals("owner")) { owners.add(new Receiver<>(receiver, "owner")); // if nested, Closure is the first owner List<Receiver<String>> thisType = new ArrayList<>(2); // and the enclosing class is the addDelegateReceiver(thisType, makeThis(), null); // end of the closure owner chain addReceivers(owners, thisType, typeCheckingContext.delegationMetadata.getParent(), "owner."); } else { - List<ClassNode> temporaryTypes = getTemporaryTypesForExpression(objectExpression); - int temporaryTypesCount = (temporaryTypes != null ? temporaryTypes.size() : 0); - if (temporaryTypesCount > 0) { // GROOVY-8965, GROOVY-10180, GROOVY-10668 - owners.add(Receiver.make(lowestUpperBound(temporaryTypes))); + // GROOVY-8965, GROOVY-10180, GROOVY-10668: `instanceof` type before declared + for (ClassNode tempType : getTemporaryTypesForExpression(objectExpression)) { + if (tempType != receiver) owners.add(Receiver.make(tempType)); } if (isClassClassNodeWrappingConcreteType(receiver)) { ClassNode staticType = receiver.getGenericsTypes()[0].getType(); @@ -4033,9 +4096,6 @@ out: if (mn.size() != 1) { } } } - if (temporaryTypesCount > 1 && !(objectExpression instanceof VariableExpression)) { - owners.add(Receiver.make(new UnionTypeClassNode(temporaryTypes.toArray(ClassNode.EMPTY_ARRAY)))); - } } return owners; } @@ -4248,9 +4308,10 @@ trying: for (ClassNode[] signature : signatures) { @Override protected void afterSwitchCaseStatementsVisited(final SwitchStatement statement) { + List<ClassNode> caseTypes; // GROOVY-8411: if any "case Type:" then "default:" contributes condition type - if (!statement.getDefaultStatement().isEmpty() && !typeCheckingContext.temporaryIfBranchTypeInformation.peek().isEmpty()) - pushInstanceOfTypeInfo(statement.getExpression(), new ClassExpression(statement.getExpression().getNodeMetaData(TYPE))); + if (!statement.getDefaultStatement().isEmpty() && (caseTypes = typeCheckingContext.temporaryIfBranchTypeInformation.peek().remove(extractTemporaryTypeInfoKey(statement.getExpression()))) != null) + pushInstanceOfTypeInfo(statement.getExpression(), new ClassExpression(unionize(caseTypes, List.of((ClassNode) statement.getExpression().getNodeMetaData(TYPE))))); } @Override @@ -4258,6 +4319,11 @@ trying: for (ClassNode[] signature : signatures) { Expression selectable = typeCheckingContext.getEnclosingSwitchStatement().getExpression(); Expression expression = statement.getExpression(); if (expression instanceof ClassExpression) { // GROOVY-8411: refine the switch type +var types = typeCheckingContext.temporaryIfBranchTypeInformation.peek().remove(extractTemporaryTypeInfoKey(selectable)); +if (types != null) { + ClassNode t = unionize(types, List.of(expression.getType())); + expression = new ClassExpression(t); +} pushInstanceOfTypeInfo(selectable, expression); } else if (expression instanceof ClosureExpression) { // GROOVY-9854: propagate the switch type ClassNode inf = selectable.getNodeMetaData(TYPE); @@ -5341,7 +5407,7 @@ trying: for (ClassNode[] signature : signatures) { if (!selfTypes.isEmpty()) { // TODO: reduce to the most-specific type(s) ClassNode superclass = selfTypes.stream().filter(t -> !t.isInterface()).findFirst().orElse(OBJECT_TYPE); selfTypes.remove(superclass); selfTypes.add(trait); - return new WideningCategories.LowestUpperBoundClassNode("TypesOf$" + trait.getNameWithoutPackage(), superclass, selfTypes.toArray(ClassNode.EMPTY_ARRAY)); + return new WideningCategories.LowestUpperBoundClassNode("TypesOf$" + trait.getNameWithoutPackage(), superclass, selfTypes.toArray(ClassNode[]::new)); } return trait; } @@ -6209,7 +6275,7 @@ out: for (ClassNode type : todo) { } private ClassNode getInferredTypeFromTempInfo(final Expression expression, final ClassNode expressionType) { - if (expression instanceof VariableExpression) { + if (expression instanceof VariableExpression && !isPrimitiveType(expressionType)) { List<ClassNode> tempTypes = getTemporaryTypesForExpression(expression); if (!tempTypes.isEmpty()) { ClassNode superclass; @@ -6249,10 +6315,15 @@ out: for (ClassNode type : todo) { } int typesCount = types.size(); - if (typesCount == 1) { + if (typesCount == 1 || (typesCount != 0 && types.stream().filter(ClassNode::isInterface).count() == 0)) { return types.get(0); - } else if (typesCount > 1) { - return new UnionTypeClassNode(types.toArray(ClassNode.EMPTY_ARRAY)); + } else if (typesCount > 1) { // create disjunction type (A&B) + String names = types.stream().map(StaticTypeCheckingSupport::prettyPrintType).collect(Collectors.joining(" & ", "(", ")")); + Map<Boolean, List<ClassNode>> ifacesAndClasses = types.stream().collect(Collectors.partitioningBy(ClassNode::isInterface)); + ClassNode sc = !ifacesAndClasses.get(Boolean.FALSE).isEmpty() ? ifacesAndClasses.get(Boolean.FALSE).get(0) : OBJECT_TYPE; + ClassNode[] is = ifacesAndClasses.get(Boolean.TRUE).toArray(ClassNode[]::new); +assert !isObjectType(sc) || (is.length > 1) : "expecting specific super class or multiple interfaces"; + return new ClassNode(names, Opcodes.ACC_ABSTRACT, sc, is, null); } } } diff --git a/src/test/groovy/groovy/transform/stc/MethodCallsSTCTest.groovy b/src/test/groovy/groovy/transform/stc/MethodCallsSTCTest.groovy index 2b430400eb..b84c580dab 100644 --- a/src/test/groovy/groovy/transform/stc/MethodCallsSTCTest.groovy +++ b/src/test/groovy/groovy/transform/stc/MethodCallsSTCTest.groovy @@ -875,6 +875,17 @@ class MethodCallsSTCTest extends StaticTypeCheckingTestCase { ''', 'Cannot find matching method','#foo(java.util.Date)', 'Cannot find matching method','#bar(java.util.Date)' + + shouldFailWithMessages foo + ''' + def it = ~/regexp/ + if (it !instanceof String) { + it = 123 + Integer.toHextString(it) + } else { // cannot be String + foo(it) + } + ''', + 'Cannot find matching method','#foo(java.util.regex.Pattern)' } // GROOVY-5226, GROOVY-11290 @@ -903,16 +914,6 @@ class MethodCallsSTCTest extends StaticTypeCheckingTestCase { } } ''' - - assertScript foobar + ''' - def it = ~/regexp/ - if (it !instanceof String) { - it = 123 - foo(it) - } else { - bar(it) - } - ''' } // GROOVY-5226, GROOVY-11290 diff --git a/src/test/groovy/groovy/transform/stc/TypeInferenceSTCTest.groovy b/src/test/groovy/groovy/transform/stc/TypeInferenceSTCTest.groovy index dbf1cf374e..850392fd23 100644 --- a/src/test/groovy/groovy/transform/stc/TypeInferenceSTCTest.groovy +++ b/src/test/groovy/groovy/transform/stc/TypeInferenceSTCTest.groovy @@ -18,7 +18,6 @@ */ package groovy.transform.stc -import groovy.test.NotYetImplemented import org.codehaus.groovy.ast.ClassHelper import org.codehaus.groovy.ast.ClassNode import org.codehaus.groovy.ast.MethodNode @@ -243,7 +242,6 @@ class TypeInferenceSTCTest extends StaticTypeCheckingTestCase { } // GROOVY-10096 - @NotYetImplemented void testInstanceOf10() { shouldFailWithMessages ''' class Foo { @@ -313,7 +311,7 @@ class TypeInferenceSTCTest extends StaticTypeCheckingTestCase { // ^ (AbstractCollection<String> & Serializable & ... & Deque) } ''', - 'Cannot call <UnionType:java.util.AbstractCollection','#add(java.lang.String) with arguments [int]' + 'Cannot call (java.util.AbstractCollection','#add(java.lang.String) with arguments [int]' } // GROOVY-5226 @@ -1249,7 +1247,7 @@ class TypeInferenceSTCTest extends StaticTypeCheckingTestCase { case File: something.toString() default: - something.getCanonicalName() + something.canonicalName // TODO: GROOVY-10702 } } ''',
