Skip to content

Commit f33e9b5

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

File tree

9 files changed

+230
-181
lines changed

9 files changed

+230
-181
lines changed

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,9 @@ 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)
47+
// prevent union and intersection types from escaping into classgen
48+
&& !inferredType.getName().startsWith("(") && !inferredType.getName().startsWith("<")) {
4749
return inferredType;
4850
}
4951

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -883,12 +883,12 @@ public static boolean implementsInterfaceOrIsSubclassOf(final ClassNode type, fi
883883
return true;
884884
}
885885
if (superOrInterface instanceof WideningCategories.LowestUpperBoundClassNode) {
886-
if (implementsInterfaceOrIsSubclassOf(type, superOrInterface.getSuperClass())
886+
if (implementsInterfaceOrIsSubclassOf(type, superOrInterface.getUnresolvedSuperClass())
887887
&& Arrays.stream(superOrInterface.getInterfaces()).allMatch(type::implementsInterface)) {
888888
return true;
889889
}
890-
} else if (superOrInterface instanceof UnionTypeClassNode) {
891-
for (ClassNode delegate : ((UnionTypeClassNode) superOrInterface).getDelegates()) {
890+
} else if (superOrInterface instanceof UnionTypeClassNode union) {
891+
for (ClassNode delegate : union.getDelegates()) {
892892
if (implementsInterfaceOrIsSubclassOf(type, delegate)) {
893893
return true;
894894
}

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

Lines changed: 115 additions & 36 deletions
Large diffs are not rendered by default.

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,10 @@ public List<ErrorCollector> getErrorCollectors() {
116116
*/
117117
protected Stack<Map<Object, List<ClassNode>>> temporaryIfBranchTypeInformation = new Stack<>();
118118

119+
List<ClassNode> peekTemporaryTypeInfo(final Object o) {
120+
return temporaryIfBranchTypeInformation.peek().computeIfAbsent(o, x -> new LinkedList<>());
121+
}
122+
119123
public void pushTemporaryTypeInfo() {
120124
temporaryIfBranchTypeInformation.push(new HashMap<>());
121125
}

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

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -51,14 +51,15 @@
5151
/**
5252
* This class node type is very special and should only be used by the static type checker
5353
* to represent types which are the union of other types. This is useful when, for example,
54-
* we enter a section like :
54+
* we enter a section like:
5555
* <pre>if (x instanceof A || x instanceof B)</pre>
5656
* where the type of <i>x</i> can be represented as one of <i>A</i> or <i>B</i>.
57-
*
57+
* <p>
5858
* This class node type should never leak outside of the type checker. More precisely, it should
5959
* only be used to check method call arguments, and nothing more.
6060
*/
6161
class UnionTypeClassNode extends ClassNode {
62+
6263
private final ClassNode[] delegates;
6364

6465
UnionTypeClassNode(final ClassNode... classNodes) {
@@ -68,7 +69,7 @@ class UnionTypeClassNode extends ClassNode {
6869
}
6970

7071
private static String makeName(final ClassNode[] nodes) {
71-
StringJoiner sj = new StringJoiner("+", "<UnionType:", ">");
72+
var sj = new StringJoiner("+", "<UnionType:", ">");
7273
for (ClassNode node : nodes) {
7374
sj.add(node.getText());
7475
}
@@ -78,7 +79,7 @@ private static String makeName(final ClassNode[] nodes) {
7879
private static ClassNode makeSuper(final ClassNode[] nodes) {
7980
ClassNode upper = lowestUpperBound(Arrays.asList(nodes));
8081
if (upper instanceof LowestUpperBoundClassNode) {
81-
upper = upper.getUnresolvedSuperClass();
82+
upper = upper.getUnresolvedSuperClass(false);
8283
} else if (upper.isInterface()) {
8384
upper = OBJECT_TYPE;
8485
}
@@ -87,7 +88,7 @@ private static ClassNode makeSuper(final ClassNode[] nodes) {
8788

8889
//--------------------------------------------------------------------------
8990

90-
public ClassNode[] getDelegates() {
91+
ClassNode[] getDelegates() {
9192
return delegates;
9293
}
9394

@@ -384,10 +385,7 @@ public boolean isAnnotated() {
384385

385386
@Override
386387
public boolean isDerivedFrom(final ClassNode type) {
387-
for (ClassNode delegate : delegates) {
388-
if (delegate.isDerivedFrom(type)) return true;
389-
}
390-
return false;
388+
return getUnresolvedSuperClass(false).isDerivedFrom(type);
391389
}
392390

393391
@Override

‎src/test/groovy/bugs/Groovy8337Bug.groovy‎

Lines changed: 0 additions & 52 deletions
This file was deleted.

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

Lines changed: 33 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -608,9 +608,10 @@ class MethodCallsSTCTest extends StaticTypeCheckingTestCase {
608608
'''
609609
}
610610

611+
// GROOVY-5226
611612
void testMethodCallArgumentUsingInstanceOf() {
612613
assertScript '''
613-
void foo(String str) { 'String' }
614+
void foo(String str) { }
614615
def o
615616
if (o instanceof String) {
616617
foo(o)
@@ -643,7 +644,8 @@ class MethodCallsSTCTest extends StaticTypeCheckingTestCase {
643644
'Cannot find matching method'
644645
}
645646

646-
void testShouldNotFailThanksToInstanceOfChecks() {
647+
// GROOVY-5226
648+
void testShouldNotFailWithExplicitCasts() {
647649
assertScript '''
648650
static String foo(String s) {
649651
'String'
@@ -656,17 +658,18 @@ class MethodCallsSTCTest extends StaticTypeCheckingTestCase {
656658
}
657659
['foo',123,true].each {
658660
if (it instanceof String) {
659-
foo((String)it)
660-
} else if (it instanceof Boolean) {
661-
foo((Boolean)it)
661+
foo((String) it)
662662
} else if (it instanceof Integer) {
663-
foo((Integer)it)
663+
foo((Integer) it)
664+
} else if (it instanceof Boolean) {
665+
foo((Boolean) it)
664666
}
665667
}
666668
'''
667669
}
668670

669-
void testShouldNotFailThanksToInstanceOfChecksAndWithoutExplicitCasts() {
671+
// GROOVY-5226
672+
void testShouldNotFailWithInstanceOfChecks() {
670673
assertScript '''
671674
static String foo(String s) {
672675
'String'
@@ -680,16 +683,17 @@ class MethodCallsSTCTest extends StaticTypeCheckingTestCase {
680683
['foo',123,true].each {
681684
if (it instanceof String) {
682685
foo(it)
683-
} else if (it instanceof Boolean) {
684-
foo(it)
685686
} else if (it instanceof Integer) {
686687
foo(it)
688+
} else if (it instanceof Boolean) {
689+
foo(it)
687690
}
688691
}
689692
'''
690693
}
691694

692-
void testShouldNotFailThanksToInstanceOfChecksAndWithoutExplicitCasts2() {
695+
// GROOVY-5226
696+
void testShouldNotFailWithInstanceOfChecks2() {
693697
assertScript '''
694698
static String foo(String s) {
695699
'String'
@@ -703,15 +707,16 @@ class MethodCallsSTCTest extends StaticTypeCheckingTestCase {
703707
['foo',123,true].each { argument ->
704708
if (argument instanceof String) {
705709
foo(argument)
706-
} else if (argument instanceof Boolean) {
707-
foo(argument)
708710
} else if (argument instanceof Integer) {
709711
foo(argument)
712+
} else if (argument instanceof Boolean) {
713+
foo(argument)
710714
}
711715
}
712716
'''
713717
}
714718

719+
// GROOVY-5226
715720
void testShouldFailWithMultiplePossibleMethods() {
716721
shouldFailWithMessages '''
717722
static String foo(String s) {
@@ -724,14 +729,15 @@ class MethodCallsSTCTest extends StaticTypeCheckingTestCase {
724729
'Boolean'
725730
}
726731
['foo',123,true].each {
727-
if (it instanceof String || it instanceof Boolean || it instanceof Integer) {
732+
if (it instanceof String || it instanceof Integer || it instanceof Boolean) {
728733
foo(it)
729734
}
730735
}
731736
''',
732-
'Reference to method is ambiguous'
737+
'foo'
733738
}
734739

740+
// GROOVY-5226
735741
void testShouldFailWithMultiplePossibleMethods2() {
736742
shouldFailWithMessages '''
737743
static String foo(String s) {
@@ -744,12 +750,12 @@ class MethodCallsSTCTest extends StaticTypeCheckingTestCase {
744750
'Boolean'
745751
}
746752
['foo',123,true].each { argument ->
747-
if (argument instanceof String || argument instanceof Boolean || argument instanceof Integer) {
753+
if (argument instanceof String || argument instanceof Integer || argument instanceof Boolean) {
748754
foo(argument)
749755
}
750756
}
751757
''',
752-
'Reference to method is ambiguous'
758+
'foo'
753759
}
754760

755761
// GROOVY-5703
@@ -875,6 +881,17 @@ class MethodCallsSTCTest extends StaticTypeCheckingTestCase {
875881
''',
876882
'Cannot find matching method','#foo(java.util.Date)',
877883
'Cannot find matching method','#bar(java.util.Date)'
884+
885+
shouldFailWithMessages foo + '''
886+
def it = ~/regexp/
887+
if (it !instanceof String) {
888+
it = 123
889+
Integer.toHextString(it)
890+
} else { // cannot be String
891+
foo(it)
892+
}
893+
''',
894+
'Cannot find matching method','#foo(java.util.regex.Pattern)'
878895
}
879896

880897
// GROOVY-5226, GROOVY-11290
@@ -903,16 +920,6 @@ class MethodCallsSTCTest extends StaticTypeCheckingTestCase {
903920
}
904921
}
905922
'''
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-
'''
916923
}
917924

918925
// GROOVY-5226, GROOVY-11290

0 commit comments

Comments
 (0)