[
https://issues.apache.org/jira/browse/GROOVY-12393?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18113576#comment-18113576
]
ASF GitHub Bot commented on GROOVY-12393:
-----------------------------------------
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.
> STC: logical-or guard on a closure or loop parameter infers the variable as
> void
> ---------------------------------------------------------------------------------
>
> Key: GROOVY-12393
> URL: https://issues.apache.org/jira/browse/GROOVY-12393
> Project: Groovy
> Issue Type: Bug
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
