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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
