[
https://issues.apache.org/jira/browse/GROOVY-11967?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18076346#comment-18076346
]
ASF GitHub Bot commented on GROOVY-11967:
-----------------------------------------
Copilot commented on code in PR #2493:
URL: https://github.com/apache/groovy/pull/2493#discussion_r3143665471
##########
src/main/java/org/codehaus/groovy/transform/sc/transformers/ListExpressionTransformer.java:
##########
@@ -111,6 +113,13 @@ public void visit(final GroovyCodeVisitor visitor) {
var list = new ConstructorCallExpression(ArrayList_TYPE, new
ConstantExpression(getExpressions().size(), true));
list.putNodeMetaData(StaticTypesMarker.DIRECT_METHOD_CALL_TARGET,
ArrayList_NEW);
list.visit(visitor);
+ // GROOVY-11967: indy-mode constructor call leaves Object on
the JVM
+ // stack, so the following INVOKEVIRTUAL ArrayList.add needs a
CHECKCAST.
+ // Non-indy mode emits INVOKESPECIAL which already leaves
ArrayList on
+ // the stack, so the cast would be redundant — skip it there.
+ if (g.getController().getInvocationWriter() instanceof
InvokeDynamicWriter) {
+ mv.visitTypeInsn(CHECKCAST, "java/util/ArrayList");
+ }
Review Comment:
The CAST is currently gated on `instanceof InvokeDynamicWriter`, but a
constructor call can also be emitted via the non-indy
`CallSiteWriter.callConstructor(...)` path (declared to return `Object`) when
the controller is using `InvocationWriter` (non-indy) and not in a
statically-checked method. In that case the following `INVOKEVIRTUAL
java/util/ArrayList.add` would still see `Object` on the stack and can
VerifyError as well. Consider making the cast unconditional, or key it off the
actual stack/top operand type after `list.visit(visitor)` (e.g., if the top
operand isn’t `ArrayList_TYPE`, emit the CHECKCAST).
##########
src/test/groovy/bugs/Groovy11967.groovy:
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package bugs
+
+import org.junit.jupiter.api.Test
+
+import static groovy.test.GroovyAssert.assertScript
+
+/**
+ * Regression coverage for the synthesised lower-arity bridge constructor that
+ * {@code @CompileStatic} emits for a constructor with a default-valued list
+ * parameter. The bridge inlines the default {@code [...]} literal as
+ * {@code new ArrayList(n) + .add(...)}; in indy mode the constructor call
+ * returns {@code Object} on the JVM stack, so the {@code INVOKEVIRTUAL
+ * ArrayList.add} that follows must be preceded by a {@code CHECKCAST
ArrayList}
+ * to satisfy the verifier.
Review Comment:
The explanatory pseudo-code in the class Javadoc reads `new ArrayList(n) +
.add(...)`, which looks like an accidental `+` and could confuse readers.
Consider rephrasing it to reflect the actual bytecode shape (allocate `new
ArrayList(n)` then invoke one or more `add` calls).
```suggestion
* parameter. The bridge inlines the default {@code [...]} literal by
* allocating {@code new ArrayList(n)} and then invoking one or more
* {@code add(...)} calls; in indy mode the constructor call returns
* {@code Object} on the JVM stack, so the {@code INVOKEVIRTUAL
ArrayList.add}
* that follows must be preceded by a {@code CHECKCAST ArrayList} to satisfy
* the verifier.
```
##########
src/main/java/org/codehaus/groovy/transform/sc/transformers/ListExpressionTransformer.java:
##########
@@ -111,6 +113,13 @@ public void visit(final GroovyCodeVisitor visitor) {
var list = new ConstructorCallExpression(ArrayList_TYPE, new
ConstantExpression(getExpressions().size(), true));
list.putNodeMetaData(StaticTypesMarker.DIRECT_METHOD_CALL_TARGET,
ArrayList_NEW);
list.visit(visitor);
+ // GROOVY-11967: indy-mode constructor call leaves Object on
the JVM
+ // stack, so the following INVOKEVIRTUAL ArrayList.add needs a
CHECKCAST.
+ // Non-indy mode emits INVOKESPECIAL which already leaves
ArrayList on
+ // the stack, so the cast would be redundant — skip it there.
+ if (g.getController().getInvocationWriter() instanceof
InvokeDynamicWriter) {
+ mv.visitTypeInsn(CHECKCAST, "java/util/ArrayList");
Review Comment:
After emitting the `CHECKCAST`, the `OperandStack` model is still whatever
`list.visit(visitor)` left behind (likely `Object` when routed through
indy/call-site code). To keep subsequent static compilation/type-driven codegen
consistent, update the operand stack type to `ArrayList` (e.g. replace the top
operand) when the cast is inserted.
```suggestion
mv.visitTypeInsn(CHECKCAST, "java/util/ArrayList");
os.replace(ArrayList_TYPE);
```
> VerifyError in @CompileStatic constructor with default-valued list parameter
> ----------------------------------------------------------------------------
>
> Key: GROOVY-11967
> URL: https://issues.apache.org/jira/browse/GROOVY-11967
> Project: Groovy
> Issue Type: Bug
> Reporter: Paul King
> Priority: Major
>
> h3. Summary
> A {{@CompileStatic}} class with a constructor that has a default-valued
> list-literal parameter triggers {{VerifyError}} when the synthesised
> lower-arity bridge constructor is loaded.
> h3. Repro
> {code:java}
> @CompileStatic
> class Foo {
> Foo(Class<?> a, MessageSource b, List<Class> targetTypes = [Object]) {
> }
> }
> new Foo(String, ms) // boom
> {code}
> h3. Symptom
> {noformat}
> java.lang.VerifyError: Bad type on operand stack
> Location: Foo.<init>(Ljava/lang/Class;LMessageSource;)V @20: invokevirtual
> Reason: Type 'java/lang/Object' (current frame, stack[4]) is not
> assignable to 'java/util/ArrayList'
> {noformat}
> h3. Root cause
> Introduced by GROOVY-8699 (commit {{7b18440c4f}}, master only). That change
> replaced the {{ScriptBytecodeAdapter.createList(Object[])}} call (returns
> {{List}}) with direct {{new ArrayList\(n) + .add(...)}} bytecode, generated
> by {{ListExpressionTransformer$NewListExpression.visit()}}.
> In indy mode the constructor call lowers to {{invokedynamic
> init:(Class;I)Object}} -- the JVM stack value is typed as {{Object}}, so the
> subsequent {{INVOKEVIRTUAL java/util/ArrayList.add(Object)Z}} fails
> verification because its receiver is not an {{ArrayList}}. The {{CHECKCAST}}
> to {{List}} added later by the transformer happens after the {{add}} call,
> too late.
> h3. Scope
> Not specific to {{List<Class>}} -- any default {{[...]}} literal in a
> {{@CompileStatic}} constructor is affected ({{List<String>}},
> {{List<Integer>}}, {{List<List<String>>}}, etc.). Map defaults go through a
> different transformer and are not affected.
> h3. Fix
> Insert {{CHECKCAST java/util/ArrayList}} immediately after the constructor
> call in {{ListExpressionTransformer$NewListExpression.visit()}}, before the
> loop that emits the per-element {{DUP / add / pop}}.
> h3. Affects
> Master only. Groovy 5.0.x and earlier are fine (used the
> {{createList(Object[])}} helper).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
