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 <[email protected]> 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() + } +}
