Skip to content

Commit 5f56279

Browse files
committed
GROOVY-12166: STC: scope statement-level instanceof narrowing to the member being visited
Temporary type information from instanceof is keyed by the target variable — for a field access, the shared FieldNode. if/loops/ternaries and expression statements bracket their own frames, but a narrowing recorded at statement level (an explicit `return field instanceof Sub`, or an `assert field instanceof Sub`, whose propagation to subsequent statements is intentional) lands in the enclosing frame, which was the frame pushed for the whole class visit. Keyed by the shared FieldNode, it then applied to every member visited afterwards: the checker stamped the narrowed type on unrelated field reads and the static compiler emitted a spurious checkcast, throwing ClassCastException at runtime whenever the field held a different subtype. Members now push their own frame (methods and constructors, property and field initializers, object initializer blocks), so statement-level narrowing still flows within a member but can never survive into another; narrowing has no meaning past the member boundary. Intra-member flow typing (assert, if-branch) is unchanged. The checker-side leak predates Groovy 5 but was masked by codegen: StaticTypesTypeChooser resolved types from the declared target first until GROOVY-11375 (5.0.0-alpha-9) gave the expression-stamped inferred type priority, exposing the stale narrowing as a checkcast. That change is correct; the fix belongs here.
1 parent e5f9c92 commit 5f56279

2 files changed

Lines changed: 145 additions & 0 deletions

File tree

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2610,6 +2610,7 @@ private void storeInferredTypeForPropertyExpression(final PropertyExpression pex
26102610
@Override
26112611
public void visitProperty(final PropertyNode node) {
26122612
boolean osc = typeCheckingContext.isInStaticContext;
2613+
typeCheckingContext.pushTemporaryTypeInfo(); // GROOVY-12166: member scope
26132614
try {
26142615
typeCheckingContext.isInStaticContext = node.isInStaticContext();
26152616
currentProperty = node;
@@ -2618,6 +2619,7 @@ public void visitProperty(final PropertyNode node) {
26182619
visitClassCodeContainer(node.getSetterBlock());
26192620
} finally {
26202621
currentProperty = null;
2622+
typeCheckingContext.popTemporaryTypeInfo();
26212623
typeCheckingContext.isInStaticContext = osc;
26222624
}
26232625
}
@@ -2626,13 +2628,15 @@ public void visitProperty(final PropertyNode node) {
26262628
@Override
26272629
public void visitField(final FieldNode node) {
26282630
boolean osc = typeCheckingContext.isInStaticContext;
2631+
typeCheckingContext.pushTemporaryTypeInfo(); // GROOVY-12166: member scope
26292632
try {
26302633
typeCheckingContext.isInStaticContext = node.isInStaticContext();
26312634
currentField = node;
26322635
visitAnnotations(node);
26332636
visitInitialExpression(node.getInitialExpression(), new FieldExpression(node), node);
26342637
} finally {
26352638
currentField = null;
2639+
typeCheckingContext.popTemporaryTypeInfo();
26362640
typeCheckingContext.isInStaticContext = osc;
26372641
}
26382642
}
@@ -3467,6 +3471,11 @@ protected void startMethodInference(final MethodNode node, final ErrorCollector
34673471
@Override
34683472
protected void visitConstructorOrMethod(final MethodNode node, final boolean isConstructor) {
34693473
typeCheckingContext.pushEnclosingMethod(node);
3474+
// GROOVY-12166: statement-level instanceof narrowing (return, assert)
3475+
// records into the enclosing temporary-type-info frame; scope it to
3476+
// this member so narrowing keyed by a shared node (e.g. a FieldNode)
3477+
// cannot leak into members visited later
3478+
typeCheckingContext.pushTemporaryTypeInfo();
34703479
final ClassNode returnType = node.getReturnType(); // GROOVY-10660: implicit return case
34713480
if (!isConstructor && (isClosureWithType(returnType) || isFunctionalInterface(returnType))) {
34723481
new ReturnAdder(returnStmt -> applyTargetType(returnType, returnStmt.getExpression())).visitMethod(node);
@@ -3486,6 +3495,7 @@ protected void visitConstructorOrMethod(final MethodNode node, final boolean isC
34863495
if (node.getCode() != null) superCall.setSourcePosition(node.getCode());
34873496
superCall.visit(this);
34883497
}
3498+
typeCheckingContext.popTemporaryTypeInfo();
34893499
typeCheckingContext.popEnclosingMethod();
34903500
}
34913501

@@ -3527,7 +3537,9 @@ protected void visitObjectInitializerStatements(final ClassNode node) {
35273537
// GROOVY-5450: create fake constructor node so final field analysis can allow write within non-static initializer block(s)
35283538
ConstructorNode init = new ConstructorNode(0, null, null, new BlockStatement(node.getObjectInitializerStatements(), null));
35293539
typeCheckingContext.pushEnclosingMethod(init);
3540+
typeCheckingContext.pushTemporaryTypeInfo(); // GROOVY-12166: member scope
35303541
super.visitObjectInitializerStatements(node);
3542+
typeCheckingContext.popTemporaryTypeInfo();
35313543
typeCheckingContext.popEnclosingMethod();
35323544
}
35333545

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,133 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
package bugs
20+
21+
import org.junit.jupiter.api.Test
22+
23+
import static groovy.test.GroovyAssert.assertScript
24+
25+
/**
26+
* GROOVY-12166: statement-level {@code instanceof} narrowing keyed by a shared
27+
* field node must not leak out of the member being visited into other members
28+
* of the class (spurious {@code checkcast} → {@code ClassCastException}).
29+
*/
30+
final class Groovy12166 {
31+
32+
private static final String TYPES = '''
33+
abstract class Base {
34+
String describe() { 'base' }
35+
abstract void unrelated()
36+
}
37+
class Alpha extends Base { void unrelated() {} }
38+
class Beta extends Base { void unrelated() {} }
39+
'''
40+
41+
@Test
42+
void testExplicitReturnInstanceofDoesNotNarrowFieldClassWide() {
43+
assertScript TYPES + '''
44+
@groovy.transform.CompileStatic
45+
class Holder {
46+
private Base field
47+
Holder(Base f) { this.field = f }
48+
boolean isAlpha() { return field instanceof Alpha }
49+
String use() { field.describe() }
50+
}
51+
def h = new Holder(new Beta())
52+
assert !h.isAlpha()
53+
assert h.use() == 'base'
54+
'''
55+
}
56+
57+
@Test
58+
void testAssertInstanceofDoesNotNarrowFieldClassWide() {
59+
assertScript TYPES + '''
60+
@groovy.transform.CompileStatic
61+
class Holder {
62+
private Base field
63+
Holder(Base f) { this.field = f }
64+
void check() { assert field instanceof Alpha }
65+
String use() { field.describe() }
66+
}
67+
// check() is never called; field holds a Beta
68+
assert new Holder(new Beta()).use() == 'base'
69+
'''
70+
}
71+
72+
@Test
73+
void testNarrowingIsIndependentOfMethodDeclarationOrder() {
74+
assertScript TYPES + '''
75+
@groovy.transform.CompileStatic
76+
class Holder {
77+
private Base field
78+
Holder(Base f) { this.field = f }
79+
String use() { field.describe() }
80+
boolean isAlpha() { return field instanceof Alpha }
81+
String useAfter() { field.describe() }
82+
}
83+
def h = new Holder(new Beta())
84+
assert !h.isAlpha()
85+
assert h.use() == 'base'
86+
assert h.useAfter() == 'base'
87+
'''
88+
}
89+
90+
@Test
91+
void testFieldInitializerInstanceofDoesNotNarrowClassWide() {
92+
assertScript TYPES + '''
93+
@groovy.transform.CompileStatic
94+
class Holder {
95+
private Base field
96+
private boolean flag = (field instanceof Alpha)
97+
Holder(Base f) { this.field = f }
98+
String use() { field.describe() }
99+
}
100+
assert new Holder(new Beta()).use() == 'base'
101+
'''
102+
}
103+
104+
@Test
105+
void testIntraMethodNarrowingStillWorks() {
106+
// narrowing within a member must be unaffected: assert flow typing and
107+
// if-branch narrowing still apply to subsequent statements
108+
assertScript TYPES + '''
109+
class Gamma extends Base {
110+
void unrelated() {}
111+
String extra() { 'gamma' }
112+
}
113+
@groovy.transform.CompileStatic
114+
class Holder {
115+
private Base field
116+
Holder(Base f) { this.field = f }
117+
String viaAssert() {
118+
assert field instanceof Gamma
119+
field.extra() // narrowing from assert applies here
120+
}
121+
String viaIf() {
122+
if (field instanceof Gamma) {
123+
return field.extra()
124+
}
125+
'other'
126+
}
127+
}
128+
def h = new Holder(new Gamma())
129+
assert h.viaAssert() == 'gamma'
130+
assert h.viaIf() == 'gamma'
131+
'''
132+
}
133+
}

0 commit comments

Comments
 (0)