Copilot commented on code in PR #2916:
URL: https://github.com/apache/groovy/pull/2916#discussion_r3973843805
##########
src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java:
##########
@@ -1192,6 +1192,19 @@ private void propagateTemporaryTypeInfo(final
Map<Object, List<ClassNode>> lhs,
lhs.keySet().removeIf(k -> k instanceof Object[]);
rhs.keySet().removeIf(k -> k instanceof Object[]);
+ // grails-core#16157 (JIRA pending): VOID_TYPE is not an instanceof
type; it marks a write to the
+ // variable (see storeType) that voids earlier instanceof guards.
Carry the
+ // marker to the enclosing scope and keep it out of the union types
below.
+ for (var map : List.of(lhs, rhs)) {
+ for (var it = map.entrySet().iterator(); it.hasNext(); ) {
+ var entry = it.next();
+ if (entry.getValue().removeIf(VOID_TYPE::equals)) {
+
typeCheckingContext.peekTemporaryTypeInfo(entry.getKey()).add(VOID_TYPE);
+ if (entry.getValue().isEmpty()) it.remove();
+ }
+ }
+ }
Review Comment:
This introduces `var` (Java 10) and `List.of` (Java 9) into a core compiler
class, which will not compile if the project source/target level is <= 8 (and
is inconsistent with typical Groovy compiler code style). Use explicit types
and a Java-8-compatible iteration approach (e.g., iterate over the two maps
without `List.of`, and declare concrete types for iterator/entry).
##########
src/test/groovy/groovy/transform/stc/TypeInferenceSTCTest.groovy:
##########
@@ -522,6 +522,45 @@ class TypeInferenceSTCTest extends
StaticTypeCheckingTestCase {
'''
}
+ // grails-core#16157 (JIRA pending): `x == null || x.foo()` guard on a
closure or for-loop
+ // parameter must not merge the "assignment voids instanceof" marker
+ // into a `void` type for the variable in the then block
Review Comment:
The PR metadata/title references `GROOVY-12393`, but the new test comment
only references `grails-core#16157 (JIRA pending)`. To avoid losing
traceability for this Groovy-core change, consider also mentioning
`GROOVY-12393` (or whichever Groovy issue corresponds) in this comment.
##########
src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java:
##########
@@ -1192,6 +1192,19 @@ private void propagateTemporaryTypeInfo(final
Map<Object, List<ClassNode>> lhs,
lhs.keySet().removeIf(k -> k instanceof Object[]);
rhs.keySet().removeIf(k -> k instanceof Object[]);
+ // grails-core#16157 (JIRA pending): VOID_TYPE is not an instanceof
type; it marks a write to the
+ // variable (see storeType) that voids earlier instanceof guards.
Carry the
+ // marker to the enclosing scope and keep it out of the union types
below.
+ for (var map : List.of(lhs, rhs)) {
+ for (var it = map.entrySet().iterator(); it.hasNext(); ) {
+ var entry = it.next();
+ if (entry.getValue().removeIf(VOID_TYPE::equals)) {
+
typeCheckingContext.peekTemporaryTypeInfo(entry.getKey()).add(VOID_TYPE);
Review Comment:
This unconditionally appends `VOID_TYPE` to the enclosing-scope list
whenever it was present in a branch list. If this merge runs multiple times for
the same key, the enclosing list can accumulate duplicate `VOID_TYPE` markers.
Consider adding it only if it’s not already present (or store the marker in a
set-like structure) to keep temporary type info stable.
--
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]
