(groovy) 01/02: GROOVY-12131: OptimizerVisitor: detect constant nodes shared across classes

[pa...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

paulk-asert pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/groovy.git

commit cbff4ba02c36a7599ef21c85e6116bf232e14987
Author: Paul King <pa...@asert.com.au>
AuthorDate: Sun Jul 5 14:18:36 2026 +1000

    GROOVY-12131: OptimizerVisitor: detect constant nodes shared across classes
---
 .../codehaus/groovy/control/OptimizerVisitor.java  | 36 ++++++++-
 .../groovy/control/OptimizerVisitorTest.groovy     | 86 ++++++++++++++++++++++
 2 files changed, 120 insertions(+), 2 deletions(-)

diff --git a/src/main/java/org/codehaus/groovy/control/OptimizerVisitor.java b/src/main/java/org/codehaus/groovy/control/OptimizerVisitor.java
index aa5d2005e8..2887e8a9d3 100644
--- a/src/main/java/org/codehaus/groovy/control/OptimizerVisitor.java
+++ b/src/main/java/org/codehaus/groovy/control/OptimizerVisitor.java
@@ -24,6 +24,8 @@ import org.codehaus.groovy.ast.FieldNode;
 import org.codehaus.groovy.ast.expr.ClosureExpression;
 import org.codehaus.groovy.ast.expr.ConstantExpression;
 import org.codehaus.groovy.ast.expr.Expression;
+import org.codehaus.groovy.control.messages.WarningMessage;
+import org.codehaus.groovy.syntax.Token;
 import org.objectweb.asm.Opcodes;
 
 import java.util.ArrayList;
@@ -117,7 +119,7 @@ public class OptimizerVisitor extends ClassCodeExpressionTransformer {
         boolean isPrimitive = isPrimitiveType(constantExpression.getType());
         FieldNode field = isPrimitive ? const2Prims.get(n) : const2Objects.get(n);
         if (field != null) {
-            constantExpression.setConstantName(field.getName());
+            stampConstantName(constantExpression, field.getName());
             return;
         }
         String name;
@@ -132,7 +134,7 @@ public class OptimizerVisitor extends ClassCodeExpressionTransformer {
                 constantExpression);
         field.setSynthetic(true);
         missingFields.add(field);
-        constantExpression.setConstantName(field.getName());
+        stampConstantName(constantExpression, field.getName());
         if (isPrimitive) {
             const2Prims.put(n, field);
         } else {
@@ -140,6 +142,36 @@ public class OptimizerVisitor extends ClassCodeExpressionTransformer {
         }
     }
 
+    /**
+     * Records the cached-constant field name on the expression node, warning when the node was
+     * already stamped with a different name (GROOVY-12131). The field name lives on the node
+     * itself, which assumes each node is reachable from exactly one class — the parser only ever
+     * produces such trees, and closure bodies are exempt from caching (GROOVY-3339). An AST
+     * transform that aliases a constant-bearing subtree into a second class breaks the
+     * assumption: the classes overwrite each other's stamp and whichever class generates
+     * bytecode against the other's numbering silently reads the wrong {@code $const$} field at
+     * runtime. A same-name re-stamp is harmless (each class reads its own field, holding the
+     * same value), so only a diverging name is reported.
+     *
+     * @param constantExpression the constant being cached
+     * @param name the {@code $const$} field name assigned for the current class
+     */
+    private void stampConstantName(ConstantExpression constantExpression, String name) {
+        String previousName = constantExpression.getConstantName();
+        if (previousName != null && !previousName.equals(name)) {
+            source.getErrorCollector().addWarning(WarningMessage.LIKELY_ERRORS,
+                    "Constant expression " + constantExpression.getText() + " is shared between classes: cached as "
+                            + previousName + " in a previously optimized class and now as " + name + " in "
+                            + currentClass.getName() + ". One of the classes will read the wrong cached constant"
+                            + " at runtime; the responsible AST transform should copy expression nodes rather than"
+                            + " alias them across classes.",
+                    Token.newString(constantExpression.getText(),
+                            constantExpression.getLineNumber(), constantExpression.getColumnNumber()),
+                    source);
+        }
+        constantExpression.setConstantName(name);
+    }
+
     /**
      * Rewrites constant expressions that can be cached in synthetic fields.
      *
diff --git a/src/test/groovy/org/codehaus/groovy/control/OptimizerVisitorTest.groovy b/src/test/groovy/org/codehaus/groovy/control/OptimizerVisitorTest.groovy
new file mode 100644
index 0000000000..cf23dc121d
--- /dev/null
+++ b/src/test/groovy/org/codehaus/groovy/control/OptimizerVisitorTest.groovy
@@ -0,0 +1,86 @@
+/*
+ *  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.control
+
+import org.codehaus.groovy.ast.ClassHelper
+import org.codehaus.groovy.ast.ClassNode
+import org.codehaus.groovy.ast.Parameter
+import org.codehaus.groovy.ast.expr.ConstantExpression
+import org.codehaus.groovy.ast.stmt.Statement
+import org.junit.jupiter.api.Test
+import org.objectweb.asm.Opcodes
+
+import static org.codehaus.groovy.ast.tools.GeneralUtils.block
+import static org.codehaus.groovy.ast.tools.GeneralUtils.stmt
+
+/**
+ * GROOVY-12131: constant caching communicates the assigned {@code $const$} field name by
+ * mutating the {@link ConstantExpression}, which assumes each node is reachable from exactly
+ * one class. An AST transform that aliases a node into a second class makes the classes
+ * overwrite each other's stamp, so one class silently reads the wrong cached constant at
+ * runtime. The visitor now warns when it re-stamps a node with a different field name.
+ */
+class OptimizerVisitorTest {
+
+    private static ClassNode classWithCode(String name, Statement... statements) {
+        def cn = new ClassNode(name, Opcodes.ACC_PUBLIC, ClassHelper.OBJECT_TYPE)
+        cn.addMethod('m', Opcodes.ACC_PUBLIC, ClassHelper.VOID_TYPE, Parameter.EMPTY_ARRAY,
+                ClassNode.EMPTY_ARRAY, block(statements))
+        cn
+    }
+
+    private static List<String> optimize(ClassNode... classes) {
+        def sourceUnit = SourceUnit.create('OptimizerVisitorTest', 'placeholder')
+        def visitor = new OptimizerVisitor(null)
+        classes.each { visitor.visitClass(it, sourceUnit) }
+        def collector = sourceUnit.errorCollector
+        (0..<collector.warningCount).collect { collector.getWarning(it).message }
+    }
+
+    @Test
+    void sharedConstantNodeStampedWithDivergingNamesWarns() {
+        // A numbers the shared 2.5 node $const$1 (behind -9.9); B re-stamps it $const$0
+        def shared = new ConstantExpression(2.5G)
+        def a = classWithCode('A', stmt(new ConstantExpression(-9.9G)), stmt(shared))
+        def b = classWithCode('B', stmt(shared))
+
+        def warnings = optimize(a, b)
+        assert warnings.any { it.contains('$const$') && it.contains('B') }
+    }
+
+    @Test
+    void distinctNodesWithEqualValuesDoNotWarn() {
+        // nested/sibling classes share constant values, never nodes — each class caches its own
+        def a = classWithCode('A', stmt(new ConstantExpression(-9.9G)), stmt(new ConstantExpression(2.5G)))
+        def b = classWithCode('B', stmt(new ConstantExpression(2.5G)))
+
+        assert optimize(a, b).isEmpty()
+    }
+
+    @Test
+    void sharedNodeStampedWithSameNameDoesNotWarn() {
+        // coincidentally identical numbering is harmless: each class reads its own field,
+        // which holds the same value
+        def shared = new ConstantExpression(2.5G)
+        def a = classWithCode('A', stmt(shared))
+        def b = classWithCode('B', stmt(shared))
+
+        assert optimize(a, b).isEmpty()
+    }
+}