[
https://issues.apache.org/jira/browse/GROOVY-12630?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18136331#comment-18136331
]
ASF GitHub Bot commented on GROOVY-12630:
-----------------------------------------
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.
> static-compilation gaps (`++`/`--` and real setter with extension getter)
> -------------------------------------------------------------------------
>
> Key: GROOVY-12630
> URL: https://issues.apache.org/jira/browse/GROOVY-12630
> Project: Groovy
> Issue Type: Bug
> Components: Static compilation
> Affects Versions: 6.0.0
> Reporter: Paul King
> Priority: Major
>
> h2. Description
> Under {{@CompileStatic}}, GROOVY-12524 made compound assignment ({{+=}},
> {{-=}}, {{?=}}, ...) work on a property whose accessors are extension
> methods. Two related forms still fail to compile. Both work in dynamic and
> {{@TypeChecked}} code, and both failed the same way in 6.0.0.
> # Prefix and postfix increment and decrement ({{++}}, {{--}}) on an extension
> property.
> # Compound assignment to a property with a real setter and an extension
> getter.
> h2. Example
> An extension module declares:
> {code:java}
> public class Box { public int v; }
> public class Mix {
> public String v = "a";
> public void setRs(String x) { v = "RS:" + x; } // real setter
> }
> public class BoxExtension {
> public static int getCount(Box b) { return b.v; }
> public static void setCount(Box b, int x) { b.v = x; }
> public static String getRs(Mix m) { return m.v + "E"; } // extension
> getter
> }
> {code}
> Then:
> {code:groovy}
> @groovy.transform.CompileStatic
> void test(Box b, Mix m) {
> b.count += 1 // compiles since GROOVY-12524
> b.count++ // Access to Box#count is forbidden
> ++b.count // Access to Box#count is forbidden
> b.count-- // Access to Box#count is forbidden
> m.rs += 'x' // Access to Mix#rs is forbidden
> }
> {code}
> The same failure occurs for a static extension property ({{Box.sx++}}) and
> for extension properties supplied by Groovy itself.
> The long forms compile and are the workaround:
> {code:groovy}
> b.count = b.count + 1
> m.rs = m.rs + 'x'
> {code}
> A real getter with an extension setter ({{m.rg += 'x'}}) already works.
> h2. Cause
> * *Increment and decrement.* Static compilation has no rewrite for
> {{PrefixExpression}} or {{PostfixExpression}}. Codegen loads the operand as a
> property, and that load does not consider extension getters. A plain read
> works only because {{PropertyExpressionTransformer}} first turns it into a
> getter call.
> * *Mixed accessors.* The GROOVY-12524 rewrite in
> {{BinaryExpressionTransformer}} applies only when the setter is an
> {{ExtensionMethodNode}}. When the setter is a real method, the compound
> assignment takes the old path, which fails on the extension getter for the
> read.
> h2. Suggested fix
> * Let the compound-assignment rewrite apply when *either* accessor is an
> extension method. The type checker already records the extension getter for
> the read side.
> * Rewrite {{++p}} / {{--p}} on such a property to {{setP(getP().next())}} /
> {{setP(getP().previous())}}, whose value is the new value. Rewrite {{p++}} /
> {{p--}} the same way, keeping the old value in a temporary as the
> expression's value. Reuse the getter lookup and setter-call construction from
> GROOVY-12524.
> * The receiver can be evaluated twice, once for the read and once for the
> write. Dynamic, {{@TypeChecked}} and {{@CompileStatic}} code all do this
> today, for real and extension properties alike.
> h2. Test clean-up in Groovy12524
> Three {{shouldFail}} tests pass without reaching the guard they are named
> after, and none of them checks a message:
> * {{testAttributeCompoundAssignOfExtensionPropertyIsRejected}}: {{file.@text
> += 'b'}} fails with an internal compiler error
> ({{ArrayIndexOutOfBoundsException}}). Plain {{file.@text = 'b'}} fails the
> same way, which is a separate pre-existing bug.
> * {{testSpreadSafeCompoundAssignIsNotRewrittenAsExtensionProperty}}: fails on
> a generics error ({{Cannot assign
> java.util.List<java.util.List<java.lang.String>> to:
> java.util.List<java.lang.String>}}) before the spread-safe guard is reached.
> * {{testPrimitiveExtensionSetterWithoutGetterIsRejected}}: fails with
> {{Cannot find matching method java.lang.Object#plus(int)}}.
> Each test should assert the error it expects. If no spread-safe form gets
> past type checking, that guard is defensive only and its test can be dropped.
> The Javadoc of the private method
> {{transformExtensionPropertyCompoundAssignment}} also opens with the JIRA
> key, which CONTRIBUTING asks to keep out of doc comments.
> h2. Not in scope
> * {{b.count **= 2}} is rejected under {{@TypeChecked}} and
> {{@CompileStatic}}, with the same error as for a real {{int}} property ({{int
> ** int}} is a {{Number}}).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
