Copilot commented on code in PR #2905:
URL: https://github.com/apache/groovy/pull/2905#discussion_r3967737736
##########
src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java:
##########
@@ -294,25 +297,43 @@ private static Set<String> exportedPackageList(final
Module module) {
private static final Map<String, Set<String>> CONCEALED_PACKAGES_TO_OPEN;
private static final Map<String, Set<String>> EXPORTED_PACKAGES_TO_OPEN;
- static {
- ModuleFinder finder = ModuleFinder.ofSystem();
- Map<String, ModuleDescriptor> packages = new HashMap<>(1024);
- finder.findAll().stream()
- .map(ModuleReference::descriptor)
- .forEach(md -> md.packages().forEach(pn ->
packages.putIfAbsent(pn, md)));
+ /**
+ * Whether the running VM has the Java Platform Module System. Runtimes
built
+ * on the JDK class library without it (Android's ART) lack {@code Module},
+ * {@code ModuleFinder} and {@code trySetAccessible}; there the Java 8
+ * accessibility rules apply and the package tables below stay empty
+ * (GROOVY-12383).
+ */
+ private static final boolean MODULES_AVAILABLE = modulesAvailable();
+
+ static boolean modulesAvailable() {
+ try {
+ Class.class.getMethod("getModule");
+ return true;
+ } catch (Throwable t) {
+ return false;
+ }
+ }
Review Comment:
`MODULES_AVAILABLE` is currently inferred only from `Class#getModule`
existence, but the guarded code also depends on other JPMS APIs
(`ModuleFinder`, and in `trySetAccessible`,
`AccessibleObject#trySetAccessible`). If a runtime provides `getModule` but is
missing other JPMS pieces, this can still crash with
`NoClassDefFoundError`/`NoSuchMethodError`. Consider making
`modulesAvailable()` validate all required symbols (e.g., `java.lang.Module`,
`java.lang.module.ModuleFinder`, and `AccessibleObject.trySetAccessible`) to
ensure the guard matches the APIs actually used.
##########
src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java:
##########
@@ -294,25 +297,43 @@ private static Set<String> exportedPackageList(final
Module module) {
private static final Map<String, Set<String>> CONCEALED_PACKAGES_TO_OPEN;
private static final Map<String, Set<String>> EXPORTED_PACKAGES_TO_OPEN;
- static {
- ModuleFinder finder = ModuleFinder.ofSystem();
- Map<String, ModuleDescriptor> packages = new HashMap<>(1024);
- finder.findAll().stream()
- .map(ModuleReference::descriptor)
- .forEach(md -> md.packages().forEach(pn ->
packages.putIfAbsent(pn, md)));
+ /**
+ * Whether the running VM has the Java Platform Module System. Runtimes
built
+ * on the JDK class library without it (Android's ART) lack {@code Module},
+ * {@code ModuleFinder} and {@code trySetAccessible}; there the Java 8
+ * accessibility rules apply and the package tables below stay empty
+ * (GROOVY-12383).
+ */
+ private static final boolean MODULES_AVAILABLE = modulesAvailable();
+
+ static boolean modulesAvailable() {
+ try {
+ Class.class.getMethod("getModule");
+ return true;
+ } catch (Throwable t) {
Review Comment:
Catching `Throwable` will also swallow serious JVM errors (e.g.,
`OutOfMemoryError`, `StackOverflowError`, `ThreadDeath`) that should generally
propagate. Prefer catching the narrowest set of exceptions relevant to
reflection/linkage checks (e.g., `NoSuchMethodException`, `LinkageError`,
`SecurityException`) so unexpected VM failures aren't silently converted into
'modules unavailable'.
##########
src/test/groovy/org/codehaus/groovy/vmplugin/v9/Java9AccessibilityTest.groovy:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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 org.codehaus.groovy.vmplugin.v9
+
+import org.codehaus.groovy.vmplugin.VMPlugin
+import org.codehaus.groovy.vmplugin.VMPluginFactory
+import org.junit.jupiter.api.Test
+
+import java.lang.reflect.Modifier
+
+import static org.junit.jupiter.api.Assertions.assertFalse
+import static org.junit.jupiter.api.Assertions.assertTrue
+
+/**
+ * GROOVY-12383: the module-system accessibility rules apply on a JVM, and the
+ * plugin initialises with them; where the module system is absent the Java 8
+ * rules take over, which only a runtime such as Android's ART can exercise.
+ */
+final class Java9AccessibilityTest {
+
+ private final VMPlugin plugin = VMPluginFactory.plugin
+
+ @Test
+ void moduleSystemIsDetectedOnAJvm() {
+ assertTrue(Java9.modulesAvailable())
+ assertTrue(plugin instanceof Java9)
+ }
+
+ @Test
+ void publicMemberOfAnExportedJdkPackageIsAccessible() {
+ def method = ArrayList.getMethod('size')
+ assertTrue(plugin.checkCanSetAccessible(method,
Java9AccessibilityTest))
+ assertTrue(plugin.checkAccessible(Java9AccessibilityTest, ArrayList,
method.modifiers, false))
+ }
+
+ @Test
+ void concealedJdkMemberIsNotAccessibleWithoutIllegalAccess() {
+ def stringCoding = Class.forName('java.lang.StringCoding')
+ assertFalse(plugin.checkAccessible(Java9AccessibilityTest,
stringCoding, Modifier.PUBLIC | Modifier.STATIC, false))
+ }
Review Comment:
`java.lang.StringCoding` is an internal JDK class and may not exist (or may
be strongly encapsulated/renamed) across different Java versions, which can
make the test fail with `ClassNotFoundException` rather than asserting
accessibility behavior. To keep the test stable, either (a) use an API/class
that is reliably present but non-exported, or (b) guard with a JUnit assumption
and skip when the target class cannot be loaded.
##########
src/test/groovy/org/codehaus/groovy/vmplugin/v9/Java9AccessibilityTest.groovy:
##########
@@ -0,0 +1,63 @@
+/*
+ * 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 org.codehaus.groovy.vmplugin.v9
+
+import org.codehaus.groovy.vmplugin.VMPlugin
+import org.codehaus.groovy.vmplugin.VMPluginFactory
+import org.junit.jupiter.api.Test
+
+import java.lang.reflect.Modifier
+
+import static org.junit.jupiter.api.Assertions.assertFalse
+import static org.junit.jupiter.api.Assertions.assertTrue
+
+/**
+ * GROOVY-12383: the module-system accessibility rules apply on a JVM, and the
+ * plugin initialises with them; where the module system is absent the Java 8
+ * rules take over, which only a runtime such as Android's ART can exercise.
+ */
+final class Java9AccessibilityTest {
+
+ private final VMPlugin plugin = VMPluginFactory.plugin
+
+ @Test
+ void moduleSystemIsDetectedOnAJvm() {
+ assertTrue(Java9.modulesAvailable())
+ assertTrue(plugin instanceof Java9)
+ }
+
+ @Test
+ void publicMemberOfAnExportedJdkPackageIsAccessible() {
+ def method = ArrayList.getMethod('size')
+ assertTrue(plugin.checkCanSetAccessible(method,
Java9AccessibilityTest))
+ assertTrue(plugin.checkAccessible(Java9AccessibilityTest, ArrayList,
method.modifiers, false))
+ }
+
+ @Test
+ void concealedJdkMemberIsNotAccessibleWithoutIllegalAccess() {
+ def stringCoding = Class.forName('java.lang.StringCoding')
+ assertFalse(plugin.checkAccessible(Java9AccessibilityTest,
stringCoding, Modifier.PUBLIC | Modifier.STATIC, false))
+ }
+
+ @Test
+ void classConstructorIsNeverMadeAccessible() {
+ def ctor = Class.getDeclaredConstructors()[0]
Review Comment:
`getDeclaredConstructors()[0]` is order-dependent and can be
non-deterministic across JVMs/vendors/versions, making this test potentially
flaky. Select a specific constructor deterministically (e.g., match by
parameter types/count) and assert on that.
##########
src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java:
##########
@@ -294,25 +297,43 @@ private static Set<String> exportedPackageList(final
Module module) {
private static final Map<String, Set<String>> CONCEALED_PACKAGES_TO_OPEN;
private static final Map<String, Set<String>> EXPORTED_PACKAGES_TO_OPEN;
- static {
- ModuleFinder finder = ModuleFinder.ofSystem();
- Map<String, ModuleDescriptor> packages = new HashMap<>(1024);
- finder.findAll().stream()
- .map(ModuleReference::descriptor)
- .forEach(md -> md.packages().forEach(pn ->
packages.putIfAbsent(pn, md)));
+ /**
+ * Whether the running VM has the Java Platform Module System. Runtimes
built
+ * on the JDK class library without it (Android's ART) lack {@code Module},
+ * {@code ModuleFinder} and {@code trySetAccessible}; there the Java 8
+ * accessibility rules apply and the package tables below stay empty
+ * (GROOVY-12383).
+ */
+ private static final boolean MODULES_AVAILABLE = modulesAvailable();
+
+ static boolean modulesAvailable() {
+ try {
+ Class.class.getMethod("getModule");
+ return true;
+ } catch (Throwable t) {
+ return false;
+ }
+ }
+ static {
Map<String, Set<String>> concealedPackagesToOpen = new
ConcurrentHashMap<>(64);
Map<String, Set<String>> exportedPackagesToOpen = new
ConcurrentHashMap<>(64);
+ if (MODULES_AVAILABLE) {
+ ModuleFinder finder = ModuleFinder.ofSystem();
Review Comment:
`MODULES_AVAILABLE` is currently inferred only from `Class#getModule`
existence, but the guarded code also depends on other JPMS APIs
(`ModuleFinder`, and in `trySetAccessible`,
`AccessibleObject#trySetAccessible`). If a runtime provides `getModule` but is
missing other JPMS pieces, this can still crash with
`NoClassDefFoundError`/`NoSuchMethodError`. Consider making
`modulesAvailable()` validate all required symbols (e.g., `java.lang.Module`,
`java.lang.module.ModuleFinder`, and `AccessibleObject.trySetAccessible`) to
ensure the guard matches the APIs actually used.
##########
src/main/java/org/codehaus/groovy/vmplugin/v9/Java9.java:
##########
@@ -219,6 +219,7 @@ private static class BigIntegerMultiplyMethodHolder {
/** {@inheritDoc} */
@Override
public boolean trySetAccessible(final AccessibleObject ao) {
+ if (!MODULES_AVAILABLE) return super.trySetAccessible(ao);
return ao.trySetAccessible();
Review Comment:
`MODULES_AVAILABLE` is currently inferred only from `Class#getModule`
existence, but the guarded code also depends on other JPMS APIs
(`ModuleFinder`, and in `trySetAccessible`,
`AccessibleObject#trySetAccessible`). If a runtime provides `getModule` but is
missing other JPMS pieces, this can still crash with
`NoClassDefFoundError`/`NoSuchMethodError`. Consider making
`modulesAvailable()` validate all required symbols (e.g., `java.lang.Module`,
`java.lang.module.ModuleFinder`, and `AccessibleObject.trySetAccessible`) to
ensure the guard matches the APIs actually used.
--
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]
