[
https://issues.apache.org/jira/browse/GROOVY-12078?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18087805#comment-18087805
]
ASF GitHub Bot commented on GROOVY-12078:
-----------------------------------------
Copilot commented on code in PR #2599:
URL: https://github.com/apache/groovy/pull/2599#discussion_r3384877974
##########
subprojects/groovy-contracts/src/main/java/org/apache/groovy/contracts/generation/PostconditionGenerator.java:
##########
@@ -141,29 +149,64 @@ private void addPostcondition(MethodNode method,
BlockStatement postconditionBlo
localPostconditionBlockStatement.getStatements().add(0,
declS(result, returnStatement.getExpression()));
AssertStatementCreationUtility.injectResultVariableReturnStatementAndAssertionCallStatement(block,
method.getReturnType().redirect(), returnStatement,
localPostconditionBlockStatement);
}
- setOldVariablesIfEnabled(block, contractsEnabled,
method.isStatic());
+ setOldVariablesIfEnabled(block, contractsEnabled, method);
} else if (method instanceof ConstructorNode) {
block.addStatements(postconditionBlockStatement.getStatements());
} else {
- setOldVariablesIfEnabled(block, contractsEnabled,
method.isStatic());
+ setOldVariablesIfEnabled(block, contractsEnabled, method);
block.addStatements(postconditionBlockStatement.getStatements());
}
method.putNodeMetaData(METHOD_PROCESSED, true);
}
}
- private void setOldVariablesIfEnabled(BlockStatement block, Expression
contractsEnabled, boolean isStatic) {
+ private void setOldVariablesIfEnabled(BlockStatement block, Expression
contractsEnabled, MethodNode method) {
final Expression oldVariableExpression = localVarX("old", new
ClassNode(Map.class));
- if (isStatic) {
+ if (method.isStatic()) {
// static methods have no instance state to snapshot; the
synthetic old-variables method is an
- // instance method, so 'old' is simply an empty map (references to
it are rejected at compile time)
+ // instance method, so 'old' starts as an empty map. GROOVY-12078:
it is still populated with
+ // the entry values of any parameters referenced via old.<name>
(field references stay rejected)
block.getStatements().add(0, declS(oldVariableExpression, mapX()));
+ List<Statement> parameterSnapshots =
parameterSnapshotStatements(method);
+ if (!parameterSnapshots.isEmpty()) {
+ block.getStatements().add(1, ifS(boolX(contractsEnabled),
block(new VariableScope(), parameterSnapshots)));
+ }
return;
}
- // Assign the return statement expression to a local variable: Map old
- Statement oldVariableStatement = assignS(oldVariableExpression,
callThisX(OldVariableGenerationUtility.OLD_VARIABLES_METHOD));
+ // Snapshot instance field state into a local variable: Map old =
$_gc_computeOldVariables()
+ final List<Statement> oldSetup = new ArrayList<>();
+ oldSetup.add(assignS(oldVariableExpression,
callThisX(OldVariableGenerationUtility.OLD_VARIABLES_METHOD)));
+ // GROOVY-12078: also snapshot the entry values of parameters
referenced via old.<name>, so a
+ // postcondition can compare against an argument that the method body
reassigns
+ oldSetup.addAll(parameterSnapshotStatements(method));
+
block.getStatements().add(0, declS(oldVariableExpression,
ConstantExpression.NULL));
- block.getStatements().add(1, ifS(boolX(contractsEnabled),
block(oldVariableStatement)));
+ block.getStatements().add(1, ifS(boolX(contractsEnabled), block(new
VariableScope(), oldSetup)));
+ }
+
+ /**
+ * Builds {@code old.put('name', name)} statements for each method
parameter referenced via
+ * {@code old.<name>} in the postcondition. Parameters are snapshotted
after the field state so a
+ * parameter shadowing a field name wins, mirroring lexical scoping.
{@code final} (and {@code val})
+ * parameters are snapshotted too: {@code final} prevents reassignment but
not in-place mutation, so
+ * {@code old.<name>} must still capture the entry state.
+ *
+ * @param method the method whose postcondition is being generated
+ * @return the list of snapshot statements (possibly empty)
+ */
+ private static List<Statement> parameterSnapshotStatements(final
MethodNode method) {
+ final Set<String> oldReferences =
method.getNodeMetaData(AnnotationClosureVisitor.OLD_REFERENCES_KEY);
+ if (oldReferences == null || oldReferences.isEmpty()) return List.of();
+
+ final List<Statement> statements = new ArrayList<>();
+ for (Parameter parameter : method.getParameters()) {
+ if (!oldReferences.contains(parameter.getName())) continue;
+ // old.put('name', name) - defensively copying a mutable value so
an in-place mutation
+ // in the body does not also change the snapshot
Review Comment:
The comment claims the snapshot is always a defensive copy, but
`snapshotExpression(...)` only clones for cloneable value-like types; other
types are stored by reference. Updating the comment avoids overstating the
guarantee and matches the actual behavior.
##########
subprojects/groovy-contracts/src/main/java/org/apache/groovy/contracts/generation/OldVariableGenerationUtility.java:
##########
@@ -126,4 +122,49 @@ public static void addOldVariableMethodNode(final
ClassNode classNode) {
preconditionMethodNode.setSynthetic(true);
}
+
+ /**
+ * Returns an expression that snapshots {@code value} (of declared type
{@code type}) for storage in
+ * the {@code old} map: a null-safe {@code clone()} call for the cloneable
value-like types, otherwise
+ * the value reference itself. Cloning the same set of types as the
instance-field snapshot means an
+ * object that the method body mutates in place is still observed in its
pre-call state via {@code old}.
+ *
+ * @param type the declared type of the value being snapshotted
+ * @param value the expression producing the value to snapshot
+ * @return the snapshot expression
+ */
+ public static Expression snapshotExpression(final ClassNode type, final
Expression value) {
+ final ClassNode wrapperType = ClassHelper.getWrapper(type);
+ if (isOldSnapshotType(wrapperType) && isCloneable(wrapperType)) {
+ final MethodCallExpression clone = callX(value, "clone");
+ clone.setSafe(true); // leave a null value as null
+ return clone;
+ }
+ return value;
+ }
+
+ /**
+ * Reports whether the (wrapper) type participates in {@code old}
snapshotting: the value-like JDK
+ * types ({@code java.lang} / {@code java.math} / {@code java.util} /
{@code java.sql}), {@link String},
+ * {@link groovy.lang.GString} and the primitives.
+ */
+ private static boolean isOldSnapshotType(final ClassNode wrapperType) {
+ final String name = wrapperType.getName();
+ return name.startsWith("java.lang")
+ || ClassHelper.isPrimitiveType(wrapperType)
+ || name.startsWith("java.math")
+ || name.startsWith("java.util")
+ || name.startsWith("java.sql")
+ || "groovy.lang.GString".equals(name)
+ || "java.lang.String".equals(name);
Review Comment:
`name.startsWith("java.lang")` already covers `java.lang.String`, so the
explicit `"java.lang.String".equals(name)` check is redundant and can be
removed to reduce noise.
> support old parameter values in @Ensures postconditions (incl. static methods)
> ------------------------------------------------------------------------------
>
> Key: GROOVY-12078
> URL: https://issues.apache.org/jira/browse/GROOVY-12078
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Priority: Major
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
