Skip to content

Commit af95d66

Browse files
paulk-asertpaulk-asert
committed
GROOVY-11983: STC: unsound smart-cast in else branch of if (cond && !(x instanceof Y))
1 parent 88ca738 commit af95d66

2 files changed

Lines changed: 144 additions & 3 deletions

File tree

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

Lines changed: 51 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@
5656
import org.codehaus.groovy.ast.expr.AttributeExpression;
5757
import org.codehaus.groovy.ast.expr.BinaryExpression;
5858
import org.codehaus.groovy.ast.expr.BitwiseNegationExpression;
59+
import org.codehaus.groovy.ast.expr.BooleanExpression;
5960
import org.codehaus.groovy.ast.expr.CastExpression;
6061
import org.codehaus.groovy.ast.expr.ClassExpression;
6162
import org.codehaus.groovy.ast.expr.ClosureExpression;
@@ -254,6 +255,9 @@
254255
import static org.codehaus.groovy.runtime.ArrayGroovyMethods.init;
255256
import static org.codehaus.groovy.runtime.ArrayGroovyMethods.last;
256257
import static org.codehaus.groovy.syntax.Types.ASSIGN;
258+
import static org.codehaus.groovy.syntax.Types.BITWISE_AND;
259+
import static org.codehaus.groovy.syntax.Types.BITWISE_OR;
260+
import static org.codehaus.groovy.syntax.Types.BITWISE_XOR;
257261
import static org.codehaus.groovy.syntax.Types.COMPARE_EQUAL;
258262
import static org.codehaus.groovy.syntax.Types.COMPARE_NOT_EQUAL;
259263
import static org.codehaus.groovy.syntax.Types.COMPARE_NOT_IN;
@@ -268,6 +272,7 @@
268272
import static org.codehaus.groovy.syntax.Types.INTDIV_EQUAL;
269273
import static org.codehaus.groovy.syntax.Types.KEYWORD_IN;
270274
import static org.codehaus.groovy.syntax.Types.KEYWORD_INSTANCEOF;
275+
import static org.codehaus.groovy.syntax.Types.LOGICAL_AND;
271276
import static org.codehaus.groovy.syntax.Types.LOGICAL_OR;
272277
import static org.codehaus.groovy.syntax.Types.MINUS_MINUS;
273278
import static org.codehaus.groovy.syntax.Types.MOD;
@@ -4636,13 +4641,15 @@ public void visitIfElse(final IfStatement ifElse) {
46364641

46374642
// GROOVY-6429: reverse instanceof tracking
46384643
typeCheckingContext.pushTemporaryTypeInfo();
4639-
tti.forEach(this::putNotInstanceOfTypeInfo);
4644+
// GROOVY-11983: only invert when the boolean's falsity pins down each instanceof
4645+
boolean canInvert = canInvertNarrowingForElseBranch(ifElse.getBooleanExpression());
4646+
if (canInvert) tti.forEach(this::putNotInstanceOfTypeInfo);
46404647

46414648
elsePath.visit(this);
46424649

46434650
typeCheckingContext.popTemporaryTypeInfo();
46444651
// GROOVY-8523: propagate tracking to outer scope; keep simple for now
4645-
if (elsePath.isEmpty() && !GeneralUtils.maybeFallsThrough(thenPath)) {
4652+
if (canInvert && elsePath.isEmpty() && !GeneralUtils.maybeFallsThrough(thenPath)) {
46464653
tti.forEach(this::putNotInstanceOfTypeInfo);
46474654
}
46484655
} finally {
@@ -4837,7 +4844,11 @@ public void visitTernaryExpression(final TernaryExpression expression) {
48374844
Map<Object, List<ClassNode>> tti = typeCheckingContext.temporaryIfBranchTypeInformation.pop();
48384845

48394846
typeCheckingContext.pushTemporaryTypeInfo();
4840-
tti.forEach(this::putNotInstanceOfTypeInfo); // GROOVY-8412
4847+
// GROOVY-8412 / GROOVY-11983: only invert when sound (no LOGICAL_AND in condition)
4848+
if (!(expression instanceof ElvisOperatorExpression)
4849+
&& canInvertNarrowingForElseBranch(expression.getBooleanExpression())) {
4850+
tti.forEach(this::putNotInstanceOfTypeInfo);
4851+
}
48414852
Expression falseExpression = expression.getFalseExpression();
48424853
ClassNode typeOfFalse = visitValueExpression(falseExpression);
48434854
typeCheckingContext.popTemporaryTypeInfo();
@@ -6645,6 +6656,43 @@ private void putNotInstanceOfTypeInfo(final Object key, final Collection<ClassNo
66456656
tti.addAll(types); // stash negative type(s)
66466657
}
66476658

6659+
/**
6660+
* GROOVY-11983: Whether the temporary type info captured while visiting {@code expr}
6661+
* may be soundly inverted for the else branch of {@code if (expr) ... else ...} (or
6662+
* the false branch of a ternary, or the surrounding scope after an early-returning
6663+
* {@code if (expr) return}). Inversion is only sound when {@code !expr} pins down
6664+
* the negation of every individual instanceof / !instanceof check inside it.
6665+
* <p>
6666+
* Only OR-like operators ({@code ||}, and {@code |} on booleans) preserve that
6667+
* property — {@code !(L op R)} is equivalent to {@code !L && !R}, so each operand's
6668+
* negation is pinned down. AND-like and XOR-like operators ({@code &&}, {@code &},
6669+
* {@code ^}) do not: {@code !(L && R)} only requires <em>at least one</em> operand
6670+
* to be false, so a {@code !instanceof} hidden inside one of them would otherwise
6671+
* be unwrapped into a positive smart-cast in the else branch, producing an unsound
6672+
* {@code checkcast} and a runtime ClassCastException.
6673+
*/
6674+
private static boolean canInvertNarrowingForElseBranch(final Expression expr) {
6675+
if (expr instanceof BinaryExpression be) {
6676+
int op = be.getOperation().getType();
6677+
// AND-like / XOR: !(L op R) doesn't pin down each operand's truth value
6678+
if (op == LOGICAL_AND || op == BITWISE_AND || op == BITWISE_XOR) return false;
6679+
// OR-like: !(L op R) <=> !L && !R, so recurse into both operands
6680+
if (op == LOGICAL_OR || op == BITWISE_OR) {
6681+
return canInvertNarrowingForElseBranch(be.getLeftExpression())
6682+
&& canInvertNarrowingForElseBranch(be.getRightExpression());
6683+
}
6684+
return true; // instanceof, ==, comparisons, etc.
6685+
}
6686+
if (expr instanceof BooleanExpression be) return canInvertNarrowingForElseBranch(be.getExpression());
6687+
if (expr instanceof NotExpression ne) return canInvertNarrowingForElseBranch(ne.getExpression());
6688+
if (expr instanceof TernaryExpression te) {
6689+
return canInvertNarrowingForElseBranch(te.getBooleanExpression())
6690+
&& canInvertNarrowingForElseBranch(te.getTrueExpression())
6691+
&& canInvertNarrowingForElseBranch(te.getFalseExpression());
6692+
}
6693+
return true;
6694+
}
6695+
66486696
private boolean optInstanceOfTypeInfo(final Expression expression, final ClassNode type) {
66496697
var tti = typeCheckingContext.temporaryIfBranchTypeInformation.peek().get(extractTemporaryTypeInfoKey(expression));
66506698
if (tti != null) { assert !tti.isEmpty();

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

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1031,6 +1031,99 @@ class TypeInferenceSTCTest extends StaticTypeCheckingTestCase {
10311031
}
10321032
}
10331033

1034+
// GROOVY-11983: !instanceof inside && must not produce a positive smart-cast in the else branch
1035+
@Test
1036+
void testNotInstanceof7() {
1037+
for (test in ['!(x instanceof String)', 'x !instanceof String']) {
1038+
// if/else form: a non-String x with cond=false lands in the else; if x were
1039+
// smart-cast to String, the body would emit a checkcast and throw CCE
1040+
assertScript """
1041+
@groovy.transform.CompileStatic
1042+
class T {
1043+
static int f(Object x, boolean cond) {
1044+
if (cond && $test) {
1045+
return 1
1046+
} else {
1047+
return x.hashCode()
1048+
}
1049+
}
1050+
}
1051+
assert T.f(42, false) == Integer.valueOf(42).hashCode()
1052+
assert T.f('s', false) == 's'.hashCode()
1053+
assert T.f('s', true) == 's'.hashCode()
1054+
assert T.f(42, true) == 1
1055+
"""
1056+
// ternary form
1057+
assertScript """
1058+
@groovy.transform.CompileStatic
1059+
class T {
1060+
static int g(Object x, boolean cond) {
1061+
return (cond && $test) ? 1 : x.hashCode()
1062+
}
1063+
}
1064+
assert T.g(42, false) == Integer.valueOf(42).hashCode()
1065+
assert T.g('s', false) == 's'.hashCode()
1066+
"""
1067+
// early-return surrounding-scope form
1068+
assertScript """
1069+
@groovy.transform.CompileStatic
1070+
class T {
1071+
static int h(Object x, boolean cond) {
1072+
if (cond && $test) return 1
1073+
return x.hashCode()
1074+
}
1075+
}
1076+
assert T.h(42, false) == Integer.valueOf(42).hashCode()
1077+
assert T.h('s', false) == 's'.hashCode()
1078+
"""
1079+
}
1080+
}
1081+
1082+
// GROOVY-11983: bitwise & / ^ between booleans, and bitwise | wrapping a logical-and,
1083+
// must also disable the else-branch instanceof inversion
1084+
@Test
1085+
void testNotInstanceof8() {
1086+
// bitwise-AND: same unsoundness as logical-AND
1087+
assertScript '''
1088+
@groovy.transform.CompileStatic
1089+
class T {
1090+
static int f(Object x, boolean cond) {
1091+
if (cond & !(x instanceof String)) return 1
1092+
return x.hashCode()
1093+
}
1094+
}
1095+
assert T.f(42, false) == Integer.valueOf(42).hashCode()
1096+
assert T.f('s', false) == 's'.hashCode()
1097+
assert T.f('s', true) == 's'.hashCode()
1098+
assert T.f(42, true) == 1
1099+
'''
1100+
// bitwise-XOR: same unsoundness
1101+
assertScript '''
1102+
@groovy.transform.CompileStatic
1103+
class T {
1104+
static int g(Object x, boolean cond) {
1105+
if (cond ^ (x instanceof String)) return 1
1106+
return x.hashCode()
1107+
}
1108+
}
1109+
assert T.g(42, false) == Integer.valueOf(42).hashCode()
1110+
assert T.g('s', true) == 's'.hashCode()
1111+
'''
1112+
// bitwise-OR wrapping a logical-AND: still must disqualify
1113+
assertScript '''
1114+
@groovy.transform.CompileStatic
1115+
class T {
1116+
static int h(Object x, boolean a, boolean b) {
1117+
if (a | (b && !(x instanceof String))) return 1
1118+
return x.hashCode()
1119+
}
1120+
}
1121+
assert T.h(42, false, false) == Integer.valueOf(42).hashCode()
1122+
assert T.h('s', false, false) == 's'.hashCode()
1123+
assert T.h(42, true, false) == 1
1124+
'''
1125+
}
1126+
10341127
// GROOVY-10217
10351128
@Test
10361129
void testInstanceOfThenSubscriptOperator() {

0 commit comments

Comments
 (0)