Copilot commented on code in PR #3160:
URL: https://github.com/apache/groovy/pull/3160#discussion_r4439995815
##########
src/main/java/org/codehaus/groovy/transform/sc/transformers/BinaryExpressionTransformer.java:
##########
@@ -235,6 +239,114 @@ private Expression
transformExtensionPropertyCompoundAssignment(final BinaryExpr
return setterCall;
}
+ /**
+ * {@code b.count++} or {@code --b.count} on a property with an extension
+ * getter or setter. The operand cannot be read or written as a property,
so
+ * the increment is a getter call, then {@code next()} or {@code
previous()},
+ * then a setter call. The receiver is evaluated once. A prefix expression
+ * yields the new value and a postfix expression the old one. A bare name
is
+ * left alone: in a closure it may resolve through the delegate, which code
+ * generation already handles.
+ *
+ * @param expr a prefix or postfix expression
+ * @return the rewritten expression, or {@code null} when the operand is
not such a property
+ */
+ Expression transformExtensionPropertyIncrement(final Expression expr) {
+ boolean postfix = expr instanceof PostfixExpression;
+ Expression operand = postfix ? ((PostfixExpression)
expr).getExpression() : ((PrefixExpression) expr).getExpression();
+ int op = (postfix ? ((PostfixExpression) expr).getOperation() :
((PrefixExpression) expr).getOperation()).getType();
+ if (op != Types.PLUS_PLUS && op != Types.MINUS_MINUS) return null;
+
+ if (!(operand instanceof PropertyExpression) || operand instanceof
AttributeExpression) return null;
+ PropertyExpression property = (PropertyExpression) operand;
+ String propertyName = property.getPropertyAsString();
+ if (property.isSpreadSafe() || propertyName == null) return null;
+
+ MethodNode setter = operand.getNodeMetaData(DIRECT_METHOD_CALL_TARGET);
+ if (setter == null || setter.getParameters().length != 1) return null;
+
+ MethodNode getter = extensionPropertyGetter(operand, setter);
Review Comment:
With an extension setter, this lookup can select an extension getter before
considering the receiver's real getter. A `File` subclass defining `getText()`
but using the GDK setter will have statically compiled `f.text++` read the
filesystem instead of calling its getter, potentially throwing
`FileNotFoundException`. Preserve the type checker's selected read accessor,
including real getters, and add a regression with a real getter competing with
an extension getter.
##########
src/main/java/org/codehaus/groovy/transform/sc/transformers/BinaryExpressionTransformer.java:
##########
@@ -235,6 +239,114 @@ private Expression
transformExtensionPropertyCompoundAssignment(final BinaryExpr
return setterCall;
}
+ /**
+ * {@code b.count++} or {@code --b.count} on a property with an extension
+ * getter or setter. The operand cannot be read or written as a property,
so
+ * the increment is a getter call, then {@code next()} or {@code
previous()},
+ * then a setter call. The receiver is evaluated once. A prefix expression
+ * yields the new value and a postfix expression the old one. A bare name
is
+ * left alone: in a closure it may resolve through the delegate, which code
+ * generation already handles.
+ *
+ * @param expr a prefix or postfix expression
+ * @return the rewritten expression, or {@code null} when the operand is
not such a property
+ */
+ Expression transformExtensionPropertyIncrement(final Expression expr) {
+ boolean postfix = expr instanceof PostfixExpression;
+ Expression operand = postfix ? ((PostfixExpression)
expr).getExpression() : ((PrefixExpression) expr).getExpression();
+ int op = (postfix ? ((PostfixExpression) expr).getOperation() :
((PrefixExpression) expr).getOperation()).getType();
+ if (op != Types.PLUS_PLUS && op != Types.MINUS_MINUS) return null;
+
+ if (!(operand instanceof PropertyExpression) || operand instanceof
AttributeExpression) return null;
+ PropertyExpression property = (PropertyExpression) operand;
+ String propertyName = property.getPropertyAsString();
+ if (property.isSpreadSafe() || propertyName == null) return null;
+
+ MethodNode setter = operand.getNodeMetaData(DIRECT_METHOD_CALL_TARGET);
+ if (setter == null || setter.getParameters().length != 1) return null;
+
+ MethodNode getter = extensionPropertyGetter(operand, setter);
+ if (getter == null) {
+ ClassNode type = setter.getDeclaringClass();
+ MethodNode realGetter = realPropertyGetter(type, propertyName);
+ if (setter instanceof ExtensionMethodNode) {
+ getter = realGetter; // extension setter, real getter
+ } else if (realGetter == null) {
+ getter = findExtensionPropertyGetter(type, setter.isStatic(),
propertyName); // real setter, extension getter
+ }
+ }
+ if (getter == null) return null;
+
+ Expression receiver = property.getObjectExpression();
+ boolean implicitThis = property.isImplicitThis();
+ boolean safe = property.isSafe();
+
+ receiver = staticCompilationTransformer.transform(receiver);
+ Expression unchanged = receiver;
+ if (!implicitThis) receiver = transformReceiverOnce(receiver);
+ List<TemporaryVariableExpression> temps = new ArrayList<>();
+ if (receiver != unchanged) temps.add((TemporaryVariableExpression)
receiver);
+
+ ClassNode readType = getter.getReturnType();
+ MethodCallExpression getterCall = callX(receiver, getter.getName());
+ getterCall.setImplicitThis(implicitThis);
+ getterCall.setMethodTarget(getter);
+ getterCall.setSafe(safe);
+ getterCall.setSourcePosition(operand);
+ getterCall.putNodeMetaData(INFERRED_TYPE, readType);
+
+ Expression current = getterCall;
+ if (postfix) {
+ TemporaryVariableExpression old = new
TemporaryVariableExpression(getterCall);
+ temps.add(0, old);
+ current = old;
+ }
+ Expression value = incrementValue(expr, op, current, readType);
+ if (value == null) return null;
+
+ Expression setterCall = transformAssignmentToSetterCall(receiver,
setter, value, implicitThis, safe, property.getProperty(), expr);
+ if (postfix) {
+ return new SequenceExpression(setterCall, current, temps);
+ }
+ return temps.isEmpty() ? setterCall : new
ReleasingExpression(setterCall, temps.get(0));
+ }
+
+ /**
+ * The new value of an increment or decrement: {@code next()} or
+ * {@code previous()} where the type checker resolved one, otherwise plus
+ * or minus one for a primitive number.
+ */
+ private Expression incrementValue(final Expression expr, final int op,
final Expression current, final ClassNode type) {
+ MethodNode method = expr.getNodeMetaData(DIRECT_METHOD_CALL_TARGET);
+ if (method != null) {
+ MethodCallExpression call = callX(current, method.getName());
+ call.setImplicitThis(false);
+ call.setMethodTarget(method);
+ call.setSourcePosition(expr);
+ ClassNode returnType = method.getReturnType();
+ call.putNodeMetaData(INFERRED_TYPE, returnType);
+ if
(StaticTypeCheckingSupport.implementsInterfaceOrIsSubclassOf(returnType, type))
{
+ return call;
+ }
+ return staticCompilationTransformer.transform(castX(type, call));
// e.g. next(Number) for a BigDecimal
Review Comment:
Casting the increment result back to the getter's type can narrow a value
that the setter accepts unchanged. For example, with an extension getter
returning `byte` and a setter accepting `Number`, incrementing 127 produces an
Integer 128 through `next(Number)`, but this cast changes it to -128 before
both the setter call and the prefix result. Keep the `next()`/`previous()`
result intact and let setter-argument conversion narrow it only when the
setter's parameter requires that.
##########
src/main/java/org/codehaus/groovy/transform/sc/transformers/BinaryExpressionTransformer.java:
##########
@@ -235,6 +239,114 @@ private Expression
transformExtensionPropertyCompoundAssignment(final BinaryExpr
return setterCall;
}
+ /**
+ * {@code b.count++} or {@code --b.count} on a property with an extension
+ * getter or setter. The operand cannot be read or written as a property,
so
+ * the increment is a getter call, then {@code next()} or {@code
previous()},
+ * then a setter call. The receiver is evaluated once. A prefix expression
+ * yields the new value and a postfix expression the old one. A bare name
is
+ * left alone: in a closure it may resolve through the delegate, which code
+ * generation already handles.
+ *
+ * @param expr a prefix or postfix expression
+ * @return the rewritten expression, or {@code null} when the operand is
not such a property
+ */
+ Expression transformExtensionPropertyIncrement(final Expression expr) {
+ boolean postfix = expr instanceof PostfixExpression;
+ Expression operand = postfix ? ((PostfixExpression)
expr).getExpression() : ((PrefixExpression) expr).getExpression();
+ int op = (postfix ? ((PostfixExpression) expr).getOperation() :
((PrefixExpression) expr).getOperation()).getType();
+ if (op != Types.PLUS_PLUS && op != Types.MINUS_MINUS) return null;
+
+ if (!(operand instanceof PropertyExpression) || operand instanceof
AttributeExpression) return null;
+ PropertyExpression property = (PropertyExpression) operand;
+ String propertyName = property.getPropertyAsString();
+ if (property.isSpreadSafe() || propertyName == null) return null;
+
+ MethodNode setter = operand.getNodeMetaData(DIRECT_METHOD_CALL_TARGET);
+ if (setter == null || setter.getParameters().length != 1) return null;
+
+ MethodNode getter = extensionPropertyGetter(operand, setter);
+ if (getter == null) {
+ ClassNode type = setter.getDeclaringClass();
+ MethodNode realGetter = realPropertyGetter(type, propertyName);
+ if (setter instanceof ExtensionMethodNode) {
+ getter = realGetter; // extension setter, real getter
+ } else if (realGetter == null) {
+ getter = findExtensionPropertyGetter(type, setter.isStatic(),
propertyName); // real setter, extension getter
+ }
+ }
+ if (getter == null) return null;
+
+ Expression receiver = property.getObjectExpression();
+ boolean implicitThis = property.isImplicitThis();
+ boolean safe = property.isSafe();
+
+ receiver = staticCompilationTransformer.transform(receiver);
+ Expression unchanged = receiver;
+ if (!implicitThis) receiver = transformReceiverOnce(receiver);
+ List<TemporaryVariableExpression> temps = new ArrayList<>();
+ if (receiver != unchanged) temps.add((TemporaryVariableExpression)
receiver);
+
+ ClassNode readType = getter.getReturnType();
+ MethodCallExpression getterCall = callX(receiver, getter.getName());
+ getterCall.setImplicitThis(implicitThis);
+ getterCall.setMethodTarget(getter);
+ getterCall.setSafe(safe);
+ getterCall.setSourcePosition(operand);
+ getterCall.putNodeMetaData(INFERRED_TYPE, readType);
+
+ Expression current = getterCall;
+ if (postfix) {
+ TemporaryVariableExpression old = new
TemporaryVariableExpression(getterCall);
+ temps.add(0, old);
+ current = old;
+ }
+ Expression value = incrementValue(expr, op, current, readType);
+ if (value == null) return null;
+
+ Expression setterCall = transformAssignmentToSetterCall(receiver,
setter, value, implicitThis, safe, property.getProperty(), expr);
+ if (postfix) {
+ return new SequenceExpression(setterCall, current, temps);
+ }
+ return temps.isEmpty() ? setterCall : new
ReleasingExpression(setterCall, temps.get(0));
+ }
+
+ /**
+ * The new value of an increment or decrement: {@code next()} or
+ * {@code previous()} where the type checker resolved one, otherwise plus
+ * or minus one for a primitive number.
+ */
+ private Expression incrementValue(final Expression expr, final int op,
final Expression current, final ClassNode type) {
+ MethodNode method = expr.getNodeMetaData(DIRECT_METHOD_CALL_TARGET);
+ if (method != null) {
+ MethodCallExpression call = callX(current, method.getName());
+ call.setImplicitThis(false);
+ call.setMethodTarget(method);
+ call.setSourcePosition(expr);
+ ClassNode returnType = method.getReturnType();
+ call.putNodeMetaData(INFERRED_TYPE, returnType);
+ if
(StaticTypeCheckingSupport.implementsInterfaceOrIsSubclassOf(returnType, type))
{
+ return call;
+ }
+ return staticCompilationTransformer.transform(castX(type, call));
// e.g. next(Number) for a BigDecimal
+ }
+ if (ClassHelper.isPrimitiveType(type) &&
!ClassHelper.isPrimitiveBoolean(type)) {
Review Comment:
A boxed getter paired with a primitive setter, such as `Integer
getTotal(Counter)` and `setTotal(int)`, leaves `c.total++` on the original
forbidden-property path. The type checker records the operand as `int` and
supplies no `next()` target, but this fallback sees the getter's `Integer`
return type and returns `null`. Use the resolved arithmetic type or explicit
unboxing so this valid accessor combination is rewritten, and add a regression
test.
--
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]
