[
https://issues.apache.org/jira/browse/GROOVY-12390?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18113333#comment-18113333
]
ASF GitHub Bot commented on GROOVY-12390:
-----------------------------------------
Copilot commented on code in PR #2912:
URL: https://github.com/apache/groovy/pull/2912#discussion_r3968801371
##########
src/test/groovy/org/codehaus/groovy/runtime/GStringImplRuntimeFootprintTest.groovy:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.runtime
+
+import groovy.transform.KnownImmutable
+import org.apache.groovy.ast.tools.ImmutablePropertyUtils
+import org.codehaus.groovy.ast.ClassHelper
+import org.apache.groovy.internal.util.ImmutableTypes
+import org.junit.jupiter.api.Test
+
+import static org.junit.jupiter.api.Assertions.assertEquals
+import static org.junit.jupiter.api.Assertions.assertFalse
+import static org.junit.jupiter.api.Assertions.assertTrue
+
+/**
+ * GROOVY-12390: building a GString must not initialise the compiler's AST
+ * model; the immutable-type check it needs lives in a runtime utility.
+ */
+final class GStringImplRuntimeFootprintTest {
+
+ private static final class RecordingLoader extends URLClassLoader {
+ final List<String> loaded = []
+ final Map<String, String> firstUse = [:]
+ RecordingLoader(URL[] urls) { super(urls,
ClassLoader.platformClassLoader) }
Review Comment:
`ClassLoader.platformClassLoader` relies on
`ClassLoader.getPlatformClassLoader()` (Java 9+). If this project still
compiles/tests on Java 8, this test won’t compile. Consider using a
Java-8-compatible parent (e.g.,
`ClassLoader.getSystemClassLoader().getParent()`) or selecting the platform
classloader via reflection with a fallback.
##########
src/main/java/org/apache/groovy/internal/util/ImmutableTypes.java:
##########
@@ -0,0 +1,160 @@
+/*
+ * 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.apache.groovy.internal.util;
+
+import org.apache.groovy.lang.annotation.Incubating;
+
+import java.lang.annotation.Annotation;
+import java.util.Set;
+
+/**
+ * The types and marker annotations Groovy treats as immutable, shared by the
+ * {@code @Immutable} family of transforms and by the runtime (GROOVY-12390).
+ * <p>
+ * Lives outside the AST packages on purpose: {@code GStringImpl} consults it
+ * for every GString with a non-trivial value, and reaching it through the
+ * transform utilities would initialise the compiler's {@code ClassHelper},
+ * with its reflective model of the JDK types, in an application that never
+ * compiles anything. Not part of the public API.
Review Comment:
The Javadoc states “Not part of the public API” while the class is `public`
and annotated `@Incubating`, which typically signals a (new) supported API
surface. To avoid mixed signals for consumers, either (a) remove `@Incubating`
and clearly mark it as internal-only (if there is an internal
annotation/convention), or (b) adjust the Javadoc to match the intended API
status (e.g., ‘public but internal/subject to change’).
##########
src/test/groovy/org/codehaus/groovy/runtime/GStringImplRuntimeFootprintTest.groovy:
##########
@@ -0,0 +1,89 @@
+/*
+ * 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.runtime
+
+import groovy.transform.KnownImmutable
+import org.apache.groovy.ast.tools.ImmutablePropertyUtils
+import org.codehaus.groovy.ast.ClassHelper
+import org.apache.groovy.internal.util.ImmutableTypes
+import org.junit.jupiter.api.Test
+
+import static org.junit.jupiter.api.Assertions.assertEquals
+import static org.junit.jupiter.api.Assertions.assertFalse
+import static org.junit.jupiter.api.Assertions.assertTrue
+
+/**
+ * GROOVY-12390: building a GString must not initialise the compiler's AST
+ * model; the immutable-type check it needs lives in a runtime utility.
+ */
+final class GStringImplRuntimeFootprintTest {
+
+ private static final class RecordingLoader extends URLClassLoader {
+ final List<String> loaded = []
+ final Map<String, String> firstUse = [:]
+ RecordingLoader(URL[] urls) { super(urls,
ClassLoader.platformClassLoader) }
+ @Override
+ protected Class<?> loadClass(String name, boolean resolve) {
+ if (name.startsWith('org.codehaus.groovy.') ||
name.startsWith('org.apache.groovy.') || name.startsWith('groovy.')) loaded <<
name
+ if (name in WATCHED && !firstUse.containsKey(name)) {
+ firstUse[name] = new Throwable().stackTrace.findAll {
!it.className.startsWith('java.') && !it.className.contains('RecordingLoader')
}.take(12).join('\n')
+ }
+ super.loadClass(name, resolve)
Review Comment:
Overriding `ClassLoader#loadClass` without preserving the usual per-class
loading lock can introduce race conditions (e.g.,
duplicate-definition/LinkageError) if classloading ever becomes concurrent.
Even though this is test code, it’s safer to wrap the logging/recording +
`super.loadClass` call with `synchronized (getClassLoadingLock(name)) { ... }`
(or make the method `synchronized`) to match the standard classloading contract.
> GStringImpl: constant-value check pulls the compiler's ClassHelper into the
> runtime
> -----------------------------------------------------------------------------------
>
> Key: GROOVY-12390
> URL: https://issues.apache.org/jira/browse/GROOVY-12390
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Assignee: Paul King
> Priority: Major
>
> Involves: checkValuesStringConstant calls
> ImmutablePropertyUtils.builtinOrMarkedImmutableClass, whose class initialiser
> builds ClassNodes through ClassHelper, which reflects over the JDK types. The
> two checks it needs, a name set and an annotation test, belong in a runtime
> utility that the AST utility delegates to.
> Impact on normal usage: positive. The first GString with a non-trivial value
> stops loading a few dozen compiler classes, and native-image reachability
> shrinks accordingly. Behaviour is unchanged. This one is worth doing
> regardless of Android.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
