[
https://issues.apache.org/jira/browse/GROOVY-12388?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18113336#comment-18113336
]
ASF GitHub Bot commented on GROOVY-12388:
-----------------------------------------
Copilot commented on code in PR #2910:
URL: https://github.com/apache/groovy/pull/2910#discussion_r3968895757
##########
src/test/groovy/org/codehaus/groovy/tools/TestDgmConverter.java:
##########
@@ -847,4 +849,48 @@ public void testDisabledFactoryFallsBackToReflection()
throws Exception {
else System.setProperty(DgmProxyFactoryConfig.PROPERTY, previous);
}
}
+
+ /**
+ * GROOVY-12388: the adapters' static initialisers must not carry a
MethodType
+ * constant, which Android's D8 accepts only from API 28; they build the
type
+ * from class constants instead.
+ */
+ public void testAdapterInitialisersUseNoMethodTypeConstants() throws
IOException {
+ ClassLoader loader = DefaultGroovyMethods.class.getClassLoader();
+ int checked = 0;
+ for (int i = 0; ; i++) {
+ URL adapter =
loader.getResource("org/codehaus/groovy/runtime/dgm$" + i + ".class");
+ if (adapter == null) {
+ if (i > 0) break;
+ continue;
+ }
Review Comment:
This assumes `dgm$<n>.class` resources exist as a contiguous sequence
starting at 0/1 and stops at the first missing index, which could silently miss
adapters if numbering ever has gaps. Also, `checked > 1000` is a brittle
threshold that can fail if the adapter count changes. Consider iterating until
a configurable number of consecutive misses is reached (or enumerating
resources from the containing JAR), and relax the final assertion to something
less version-sensitive (e.g., `checked > 0` or a small lower bound).
##########
src/main/java/org/codehaus/groovy/tools/DgmConverter.java:
##########
@@ -469,6 +486,30 @@ private static void
createTargetMethodHandleField(ClassWriter cw, CachedMethod m
mv.visitEnd();
}
+ /**
+ * Pushes the {@code Class} for a type: the wrapper's {@code TYPE} field
for a
+ * primitive or {@code void}, a class constant otherwise.
+ */
+ private static void pushClassConstant(MethodVisitor mv, Type type) {
+ String wrapper = switch (type.getSort()) {
+ case Type.VOID -> "java/lang/Void";
+ case Type.BOOLEAN -> "java/lang/Boolean";
+ case Type.CHAR -> "java/lang/Character";
+ case Type.BYTE -> "java/lang/Byte";
+ case Type.SHORT -> "java/lang/Short";
+ case Type.INT -> "java/lang/Integer";
+ case Type.FLOAT -> "java/lang/Float";
+ case Type.LONG -> "java/lang/Long";
+ case Type.DOUBLE -> "java/lang/Double";
+ default -> null;
+ };
Review Comment:
This uses a Java switch expression (`switch (...) { case ... -> ... }`),
which requires newer Java language levels (>= 14). If this module is compiled
with an older source/target (common for Groovy builds), this will fail
compilation. Use a traditional `switch` statement (or an `if/else` chain) to
keep the helper compatible with older Java compilers.
##########
src/test/groovy/org/codehaus/groovy/tools/TestDgmConverter.java:
##########
@@ -847,4 +849,48 @@ public void testDisabledFactoryFallsBackToReflection()
throws Exception {
else System.setProperty(DgmProxyFactoryConfig.PROPERTY, previous);
}
}
+
+ /**
+ * GROOVY-12388: the adapters' static initialisers must not carry a
MethodType
+ * constant, which Android's D8 accepts only from API 28; they build the
type
+ * from class constants instead.
+ */
+ public void testAdapterInitialisersUseNoMethodTypeConstants() throws
IOException {
+ ClassLoader loader = DefaultGroovyMethods.class.getClassLoader();
+ int checked = 0;
+ for (int i = 0; ; i++) {
+ URL adapter =
loader.getResource("org/codehaus/groovy/runtime/dgm$" + i + ".class");
+ if (adapter == null) {
+ if (i > 0) break;
+ continue;
+ }
+ byte[] bytes;
+ try (InputStream in = adapter.openStream()) {
+ bytes = in.readAllBytes();
+ }
Review Comment:
`InputStream.readAllBytes()` is only available starting in Java 9. If tests
are intended to run on Java 8 (or other older toolchains), this will fail to
compile/run. Consider replacing with a Java-8-compatible stream-to-byte[] read
(e.g., using a loop into a `ByteArrayOutputStream`).
##########
src/test/groovy/org/codehaus/groovy/tools/TestDgmConverter.java:
##########
@@ -847,4 +849,48 @@ public void testDisabledFactoryFallsBackToReflection()
throws Exception {
else System.setProperty(DgmProxyFactoryConfig.PROPERTY, previous);
}
}
+
+ /**
+ * GROOVY-12388: the adapters' static initialisers must not carry a
MethodType
+ * constant, which Android's D8 accepts only from API 28; they build the
type
+ * from class constants instead.
+ */
+ public void testAdapterInitialisersUseNoMethodTypeConstants() throws
IOException {
+ ClassLoader loader = DefaultGroovyMethods.class.getClassLoader();
+ int checked = 0;
+ for (int i = 0; ; i++) {
+ URL adapter =
loader.getResource("org/codehaus/groovy/runtime/dgm$" + i + ".class");
+ if (adapter == null) {
+ if (i > 0) break;
+ continue;
+ }
+ byte[] bytes;
+ try (InputStream in = adapter.openStream()) {
+ bytes = in.readAllBytes();
+ }
+ List<String> methodTypeConstants = new ArrayList<>();
+ boolean[] buildsMethodType = {false};
+ new ClassReader(bytes).accept(new ClassVisitor(Opcodes.ASM9) {
+ @Override
+ public MethodVisitor visitMethod(int access, String name,
String descriptor, String signature, String[] exceptions) {
+ return new MethodVisitor(Opcodes.ASM9) {
+ @Override
+ public void visitLdcInsn(Object value) {
+ if (value instanceof Type type && type.getSort()
== Type.METHOD) {
+ methodTypeConstants.add(name + ": " + type);
+ }
+ }
+ @Override
+ public void visitMethodInsn(int opcode, String owner,
String mname, String mdesc, boolean isInterface) {
+ if ("java/lang/invoke/MethodType".equals(owner) &&
"methodType".equals(mname)) buildsMethodType[0] = true;
+ }
+ };
+ }
+ }, 0);
+ assertTrue("dgm$" + i + " has MethodType constants: " +
methodTypeConstants, methodTypeConstants.isEmpty());
+ assertTrue("dgm$" + i + " does not build its MethodType",
buildsMethodType[0]);
+ checked++;
+ }
+ assertTrue("no adapters found", checked > 1000);
+ }
Review Comment:
This assumes `dgm$<n>.class` resources exist as a contiguous sequence
starting at 0/1 and stops at the first missing index, which could silently miss
adapters if numbering ever has gaps. Also, `checked > 1000` is a brittle
threshold that can fail if the adapter count changes. Consider iterating until
a configurable number of consecutive misses is reached (or enumerating
resources from the containing JAR), and relax the final assertion to something
less version-sensitive (e.g., `checked > 0` or a small lower bound).
##########
src/test/groovy/org/codehaus/groovy/tools/TestDgmConverter.java:
##########
@@ -847,4 +849,48 @@ public void testDisabledFactoryFallsBackToReflection()
throws Exception {
else System.setProperty(DgmProxyFactoryConfig.PROPERTY, previous);
}
}
+
+ /**
+ * GROOVY-12388: the adapters' static initialisers must not carry a
MethodType
+ * constant, which Android's D8 accepts only from API 28; they build the
type
+ * from class constants instead.
+ */
+ public void testAdapterInitialisersUseNoMethodTypeConstants() throws
IOException {
+ ClassLoader loader = DefaultGroovyMethods.class.getClassLoader();
+ int checked = 0;
+ for (int i = 0; ; i++) {
+ URL adapter =
loader.getResource("org/codehaus/groovy/runtime/dgm$" + i + ".class");
+ if (adapter == null) {
+ if (i > 0) break;
+ continue;
+ }
+ byte[] bytes;
+ try (InputStream in = adapter.openStream()) {
+ bytes = in.readAllBytes();
+ }
+ List<String> methodTypeConstants = new ArrayList<>();
+ boolean[] buildsMethodType = {false};
+ new ClassReader(bytes).accept(new ClassVisitor(Opcodes.ASM9) {
+ @Override
+ public MethodVisitor visitMethod(int access, String name,
String descriptor, String signature, String[] exceptions) {
+ return new MethodVisitor(Opcodes.ASM9) {
+ @Override
+ public void visitLdcInsn(Object value) {
+ if (value instanceof Type type && type.getSort()
== Type.METHOD) {
+ methodTypeConstants.add(name + ": " + type);
+ }
+ }
+ @Override
+ public void visitMethodInsn(int opcode, String owner,
String mname, String mdesc, boolean isInterface) {
+ if ("java/lang/invoke/MethodType".equals(owner) &&
"methodType".equals(mname)) buildsMethodType[0] = true;
+ }
+ };
+ }
+ }, 0);
Review Comment:
The test description says it’s asserting about the adapters’ static
initialisers, but the visitor currently flags `MethodType` constants and
`MethodType.methodType` calls in *any* method. This can cause false failures
(if another method legitimately has a MethodType constant) or false passes (if
`methodType` is called outside `<clinit>`). Filter the analysis to
`name.equals(\"<clinit>\")` so the test accurately enforces the intended
constraint.
> DgmConverter: avoid MethodType constants in the adapter initialisers
> --------------------------------------------------------------------
>
> Key: GROOVY-12388
> URL: https://issues.apache.org/jira/browse/GROOVY-12388
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> Involves: the generated dgm$N classes load their target handle with an ldc
> MethodType, which D8 accepts only from API 28. Emitting a
> MethodType.methodType(Class, Class[]) call instead brings the floor back to
> API 26, where indy starts.
> Impact on normal usage: slightly larger adapter bytecode and a method call
> instead of a constant-pool resolution in each adapter's static initialiser,
> both negligible. The value is small too: Android 8 and 8.1 devices are around
> one percent of the installed base, so this is the one I would skip.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
