Copilot commented on code in PR #2311:
URL: https://github.com/apache/groovy/pull/2311#discussion_r3176226386
##########
src/main/java/org/codehaus/groovy/classgen/asm/sc/StaticTypesTypeChooser.java:
##########
@@ -32,20 +32,23 @@
* type information from node metadata generated by the static type checker.
*/
public class StaticTypesTypeChooser extends StatementMetaTypeChooser {
+
@Override
public ClassNode resolveType(final Expression exp, final ClassNode
current) {
- ASTNode target = getTarget(exp); // see GROOVY-9344, GROOVY-9607
-
- ClassNode inferredType =
target.getNodeMetaData(StaticTypesMarker.DECLARATION_INFERRED_TYPE);
+ ClassNode inferredType =
exp.getNodeMetaData(StaticTypesMarker.DECLARATION_INFERRED_TYPE);
+ if (inferredType == null) {
+ inferredType =
exp.getNodeMetaData(StaticTypesMarker.INFERRED_TYPE);
+ }
if (inferredType == null) {
- inferredType =
target.getNodeMetaData(StaticTypesMarker.INFERRED_TYPE);
+ ASTNode node = getTarget(exp); // GROOVY-9344, GROOVY-9607
+ inferredType =
node.getNodeMetaData(StaticTypesMarker.INFERRED_TYPE);
Review Comment:
This drops the fallback to `DECLARATION_INFERRED_TYPE` on the accessed
target. Fresh `VariableExpression` wrappers such as `varX(variable)` in
`ClosureWriter#getClosureSharedVariables` don't carry metadata on the wrapper
itself, so after this change captured/aliased variables resolve to the target's
current `INFERRED_TYPE` instead of the declaration/LUB type. That can generate
overly narrow bytecode types for closure-shared variables and reintroduce the
kind of regressions the old target lookup was protecting against.
##########
src/test/groovy/groovy/transform/stc/TypeInferenceSTCTest.groovy:
##########
@@ -242,23 +241,29 @@ class TypeInferenceSTCTest extends
StaticTypeCheckingTestCase {
'''
}
- // GROOVY-10096
- @NotYetImplemented
+ // GROOVY-11769
void testInstanceOf10() {
Review Comment:
Repurposing this test removes the only regression coverage for GROOVY-10096
from the suite. A new GROOVY-11769 test should be added under a different name
so we don't stop checking the previous return-type inference bug.
##########
src/test/groovy/groovy/transform/stc/TypeInferenceSTCTest.groovy:
##########
@@ -242,23 +241,29 @@ class TypeInferenceSTCTest extends
StaticTypeCheckingTestCase {
'''
}
- // GROOVY-10096
- @NotYetImplemented
+ // GROOVY-11769
void testInstanceOf10() {
- shouldFailWithMessages '''
- class Foo {
- void foo() {
- }
+ assertScript '''
+ abstract class Foo {
+ abstract boolean isBaz()
}
class Bar extends Foo {
- void bar() {
- }
+ final boolean baz = false
}
- static Bar baz(Foo foo) {
- (false || foo instanceof Bar) ? foo : new Bar()
+ class Baz extends Foo {
+ final boolean baz = true
}
- ''',
- 'Cannot return value of type Foo for method returning Bar'
+
+ void test(Foo foo) {
+ if (foo instanceof Bar || foo.isBaz()) {
+ foo.toString()
Review Comment:
This assertion doesn't actually verify the `instanceof` flow typing change:
`foo.toString()` is available on `Object`, so the test will pass even if `foo`
is never narrowed after the `foo instanceof Bar || foo.isBaz()` condition.
Please assert behavior that requires the narrowed type; otherwise this
regression test won't catch the bug it is meant to cover.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
