[
https://issues.apache.org/jira/browse/GROOVY-12384?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18113329#comment-18113329
]
ASF GitHub Bot commented on GROOVY-12384:
-----------------------------------------
Copilot commented on code in PR #2906:
URL: https://github.com/apache/groovy/pull/2906#discussion_r3968790727
##########
src/test/groovy/org/apache/groovy/util/HiddenClassDefinerTest.groovy:
##########
@@ -68,6 +68,17 @@ class HiddenClassDefinerTest {
* Production Java call sites capture {@code MethodHandles.lookup()} in a
* {@code static final} field of the nest-host class itself.
*/
+ @Test
+ void loadingTheDefinerDoesNotResolveLookupClassOption() {
+ // GROOVY-12384: the ClassOption array lives in a nested holder so
that a runtime
+ // without hidden classes (Android's ART) can load the definer and ask
isEnabled()
+ def optionArray = MethodHandles.Lookup.ClassOption[]
+ assert HiddenClassDefiner.declaredFields.every { it.type !=
optionArray }
+ assert HiddenClassDefiner.declaredClasses.any { holder ->
+ holder.declaredFields.any { it.type == optionArray }
+ }
Review Comment:
`def optionArray = MethodHandles.Lookup.ClassOption[]` is not a valid way to
obtain the array `Class` object in Groovy, and it also makes `optionArray`
ambiguous (type vs. value). Use the array class literal instead so `Field.type`
comparisons are correct (e.g., assign
`MethodHandles.Lookup.ClassOption[].class` to `optionArray`).
##########
src/test/groovy/org/apache/groovy/util/HiddenClassDefinerTest.groovy:
##########
@@ -68,6 +68,17 @@ class HiddenClassDefinerTest {
* Production Java call sites capture {@code MethodHandles.lookup()} in a
* {@code static final} field of the nest-host class itself.
*/
+ @Test
+ void loadingTheDefinerDoesNotResolveLookupClassOption() {
+ // GROOVY-12384: the ClassOption array lives in a nested holder so
that a runtime
+ // without hidden classes (Android's ART) can load the definer and ask
isEnabled()
+ def optionArray = MethodHandles.Lookup.ClassOption[]
+ assert HiddenClassDefiner.declaredFields.every { it.type !=
optionArray }
+ assert HiddenClassDefiner.declaredClasses.any { holder ->
+ holder.declaredFields.any { it.type == optionArray }
+ }
Review Comment:
This is a JUnit `@Test`, but it uses Groovy `assert` statements. If
assertions are disabled in the test runtime, this test can become a no-op.
Prefer JUnit Assertions (`assertTrue`/`assertFalse`/`assertAll`) here so the
checks always execute under the test runner.
##########
src/test/groovy/org/codehaus/groovy/reflection/android/AndroidSupportTest.groovy:
##########
@@ -0,0 +1,43 @@
+/*
+ * 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.reflection.android
+
+import org.apache.groovy.internal.runtime.invoke.InvokerFactory
+import org.junit.jupiter.api.Test
+
+import static org.junit.jupiter.api.Assertions.assertFalse
+import static org.junit.jupiter.api.Assertions.assertTrue
+
+/**
+ * GROOVY-12384: Android is recognised by its VM name, so a JVM is never
mistaken
+ * for it, whatever is on the class path.
+ */
+final class AndroidSupportTest {
+
+ @Test
+ void aJvmIsNotAndroid() {
+ assertFalse(System.getProperty('java.vm.name').startsWith('Dalvik'))
Review Comment:
`System.getProperty('java.vm.name')` can be `null` (or access can be
restricted), which would make `.startsWith('Dalvik')` throw at runtime. Make
the assertion resilient by handling `null`/read failures (e.g., coerce to empty
string or use a JUnit assumption to skip when the property can’t be read).
> Direct invokers: check for Android before touching HiddenClassDefiner
> ---------------------------------------------------------------------
>
> Key: GROOVY-12384
> URL: https://issues.apache.org/jira/browse/GROOVY-12384
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Priority: Major
>
> Involves: swapping two lines in InvokerFactory.generationAllowed so the
> existing Android check runs before HiddenClassDefiner.isEnabled(), and moving
> the ClassOption array into a nested holder class so loading the definer does
> not resolve a type ART lacks.
> Impact on normal usage: none. The Android check is a cached boolean.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
