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.
--
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]
