You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by em...@apache.org on 2024/06/03 17:56:07 UTC
(groovy) branch master updated: GROOVY-11339: `@AutoImplement`: don't implement default argument methods
This is an automated email from the ASF dual-hosted git repository.
emilles pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/groovy.git
The following commit(s) were added to refs/heads/master by this push:
new 98b0722836 GROOVY-11339: `@AutoImplement`: don't implement default argument methods
98b0722836 is described below
commit 98b07228366ded3390d9306a73013e547b610ba6
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Mon Jun 3 12:46:54 2024 -0500
GROOVY-11339: `@AutoImplement`: don't implement default argument methods
---
.../apache/groovy/ast/tools/MethodNodeUtils.java | 51 +++++++++++++++
.../transform/AutoImplementASTTransformation.java | 76 +++++++++++++++-------
.../transform/stc/StaticTypeCheckingVisitor.java | 60 +----------------
.../transform/AutoImplementTransformTest.groovy | 34 ++++++++--
4 files changed, 135 insertions(+), 86 deletions(-)
diff --git a/src/main/java/org/apache/groovy/ast/tools/MethodNodeUtils.java b/src/main/java/org/apache/groovy/ast/tools/MethodNodeUtils.java
index 1a74e013a2..40b7f83fb2 100644
--- a/src/main/java/org/apache/groovy/ast/tools/MethodNodeUtils.java
+++ b/src/main/java/org/apache/groovy/ast/tools/MethodNodeUtils.java
@@ -18,13 +18,20 @@
*/
package org.apache.groovy.ast.tools;
+import org.codehaus.groovy.ast.ConstructorNode;
import org.codehaus.groovy.ast.MethodNode;
import org.codehaus.groovy.ast.Parameter;
import org.codehaus.groovy.ast.stmt.BlockStatement;
+import org.codehaus.groovy.ast.stmt.EmptyStatement;
import org.codehaus.groovy.ast.stmt.Statement;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
import static org.apache.groovy.util.BeanUtils.decapitalize;
import static org.codehaus.groovy.ast.ClassHelper.isPrimitiveBoolean;
+import static org.objectweb.asm.Opcodes.ACC_ABSTRACT;
/**
* Utility class for working with MethodNodes
@@ -156,4 +163,48 @@ public class MethodNodeUtils {
return (parameters == null || parameters.length == 0)
&& mNode.isPublic() && !mNode.isStatic() && !mNode.isAbstract() && !mNode.isVoidMethod();
}
+
+ /**
+ * Returns new list that includes methods that will be generated for default
+ * argument expressions.
+ *
+ * @since 5.0.0
+ */
+ public static List<MethodNode> withDefaultArgumentMethods(final List<? extends MethodNode> methods) {
+ List<MethodNode> result = new ArrayList<>(methods.size());
+
+ for (MethodNode method : methods) {
+ result.add(method);
+
+ if (!method.hasDefaultValue()) continue;
+
+ Parameter[] parameters = method.getParameters();
+ var n = Arrays.stream(parameters).filter(Parameter::hasInitialExpression).count();
+ for (int i = 1; i <= n; i += 1) { // drop parameters with value from right to left
+ Parameter[] newParams = new Parameter[parameters.length - i];
+ int j = 1, index = 0;
+ for (Parameter parameter : parameters) {
+ if (j > n - i && parameter.hasInitialExpression()) {
+ ;
+ } else {
+ newParams[index++] = parameter;
+ }
+ if (parameter.hasInitialExpression()) j += 1;
+ }
+
+ MethodNode stub;
+ if (method.isConstructor()) {
+ stub = new ConstructorNode(method.getModifiers(), newParams, method.getExceptions(), EmptyStatement.INSTANCE);
+ } else {
+ stub = new MethodNode(method.getName(), method.getModifiers() & ~ACC_ABSTRACT, method.getReturnType(), newParams, method.getExceptions(), EmptyStatement.INSTANCE);
+ }
+ stub.setDeclaringClass(method.getDeclaringClass());
+ stub.setGenericsTypes(method.getGenericsTypes());
+ stub.setSynthetic(true);
+ result.add(stub);
+ }
+ }
+
+ return result;
+ }
}
diff --git a/src/main/java/org/codehaus/groovy/transform/AutoImplementASTTransformation.java b/src/main/java/org/codehaus/groovy/transform/AutoImplementASTTransformation.java
index b870016f12..8feec9e82b 100644
--- a/src/main/java/org/codehaus/groovy/transform/AutoImplementASTTransformation.java
+++ b/src/main/java/org/codehaus/groovy/transform/AutoImplementASTTransformation.java
@@ -45,7 +45,7 @@ import static org.apache.groovy.ast.tools.ClassNodeUtils.addGeneratedMethod;
import static org.apache.groovy.ast.tools.ClassNodeUtils.isSubtype;
import static org.apache.groovy.ast.tools.MethodNodeUtils.getPropertyName;
import static org.apache.groovy.ast.tools.MethodNodeUtils.methodDescriptorWithoutReturnType;
-import static org.apache.groovy.util.BeanUtils.capitalize;
+import static org.apache.groovy.ast.tools.MethodNodeUtils.withDefaultArgumentMethods;
import static org.codehaus.groovy.ast.tools.GeneralUtils.callX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.constX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.ctorX;
@@ -119,7 +119,7 @@ public class AutoImplementASTTransformation extends AbstractASTTransformation {
if (mNode.getParameters().length == 0) {
String propertyName = getPropertyName(mNode);
if (propertyName != null) {
- String accessorName = mNode.getName().startsWith("is") ? getGetterName(propertyName) : "is" + capitalize(propertyName);
+ String accessorName = mNode.getName().startsWith("is") ? getGetterName(propertyName) : getGetterName(propertyName, Boolean.TYPE);
if (cNode.hasMethod(accessorName, Parameter.EMPTY_ARRAY)) {
// delegate to existing accessor to reduce the surprise
return returnS(callX(varX("this"), accessorName));
@@ -148,14 +148,14 @@ public class AutoImplementASTTransformation extends AbstractASTTransformation {
*/
private static Map<String, MethodNode> getAllCorrectedMethodsMap(final ClassNode cNode) {
Map<String, MethodNode> result = new HashMap<>();
- for (MethodNode mn : cNode.getMethods()) {
+ for (MethodNode mn : getMethodsWithGenerated(cNode)) {
result.put(methodDescriptorWithoutReturnType(mn), mn);
}
ClassNode next = cNode;
while (true) {
Map<String, ClassNode> genericsSpec = createGenericsSpec(next);
if (next != cNode) {
- for (MethodNode mn : next.getMethods()) {
+ for (MethodNode mn : getMethodsWithGenerated(next)) {
MethodNode correctedMethod = correctToGenericsSpec(genericsSpec, mn);
ClassNode correctedClass = correctToGenericsSpecRecurse(genericsSpec, next);
MethodNode found = getDeclaredMethodCorrected(genericsSpec, correctedMethod, correctedClass);
@@ -194,33 +194,59 @@ public class AutoImplementASTTransformation extends AbstractASTTransformation {
next = correctToGenericsSpecRecurse(updatedGenericsSpec, superClass);
}
- // GROOVY-9816: remove entries for to-be-generated property access and mutate methods
- for (ClassNode cn = cNode; cn != null && !ClassHelper.isObjectType(cn); cn = cn.getSuperClass()) {
- for (PropertyNode pn : cn.getProperties()) {
- if (!pn.getField().isFinal()) {
- result.remove(pn.getSetterNameOrDefault() + ":" + pn.getType().getText() + ",");
+ return result;
+ }
+
+ private static List<MethodNode> getMethodsWithGenerated(final ClassNode cNode) {
+ List<MethodNode> methods = cNode.getMethods();
+
+ // GROOVY-11339:
+ methods = withDefaultArgumentMethods(methods);
+
+ // GROOVY-9816:
+ for (PropertyNode pn : cNode.getProperties()) {
+ int modifiers = pn.isStatic() ? 9 : 1;
+ if (!pn.isFinal()) {
+ String setterName = pn.getSetterNameOrDefault();
+ Parameter[] oneParameter = {new Parameter(pn.getType(), "value")};
+ if (!cNode.hasMethod(setterName, oneParameter)) {
+ MethodNode mn = new MethodNode(setterName, modifiers, ClassHelper.VOID_TYPE, oneParameter, null, null);
+ mn.setDeclaringClass(cNode);
+ mn.setSynthetic(true);
+ methods.add(mn);
}
- if (!ClassHelper.isPrimitiveBoolean(pn.getType())) {
- result.remove(pn.getGetterNameOrDefault() + ":");
- } else if (pn.getGetterName() != null) {
- result.remove(pn.getGetterName() + ":");
- } else {
- // getter generated only if no explicit isser and vice versa
- String isserName = "is" + capitalize(pn.getName());
- String getterName = getGetterName(pn.getName());
- if (!cNode.hasMethod(isserName, Parameter.EMPTY_ARRAY)) {
- result.remove(getterName + ":");
- }
- if (!cNode.hasMethod(getterName, Parameter.EMPTY_ARRAY)) {
- result.remove(isserName + ":");
- }
+ }
+ if (pn.getGetterName() != null || !ClassHelper.isPrimitiveBoolean(pn.getType())) {
+ String getterName = pn.getGetterNameOrDefault();
+ if (!cNode.hasMethod(getterName, Parameter.EMPTY_ARRAY)) {
+ MethodNode mn = new MethodNode(getterName, modifiers, pn.getType(), Parameter.EMPTY_ARRAY, null, null);
+ mn.setDeclaringClass(cNode);
+ mn.setSynthetic(true);
+ methods.add(mn);
+ }
+ } else {
+ // getter generated only if no explicit isser and vice versa
+ String isserName = getGetterName(pn.getName(), Boolean.TYPE);
+ String getterName = getGetterName(pn.getName());
+ if (!cNode.hasMethod(isserName, Parameter.EMPTY_ARRAY)) {
+ MethodNode mn = new MethodNode(getterName, modifiers, pn.getType(), Parameter.EMPTY_ARRAY, null, null);
+ mn.setDeclaringClass(cNode);
+ mn.setSynthetic(true);
+ methods.add(mn);
+ }
+ if (!cNode.hasMethod(getterName, Parameter.EMPTY_ARRAY)) {
+ MethodNode mn = new MethodNode(isserName, modifiers, pn.getType(), Parameter.EMPTY_ARRAY, null, null);
+ mn.setDeclaringClass(cNode);
+ mn.setSynthetic(true);
+ methods.add(mn);
}
}
}
- return result;
+
+ return methods;
}
- private static boolean isWeakerCandidate(MethodNode existing, MethodNode found) {
+ private static boolean isWeakerCandidate(final MethodNode existing, final MethodNode found) {
return !(existing.isAbstract() && !found.isAbstract()) &&
// GROOVY-10472: prefer covariant method with more concrete type
isSubtype(found.getReturnType(), existing.getReturnType());
diff --git a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
index acf7c9f730..b5619cc1a9 100644
--- a/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
+++ b/src/main/java/org/codehaus/groovy/transform/stc/StaticTypeCheckingVisitor.java
@@ -145,6 +145,7 @@ import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.IntStream;
+import static org.apache.groovy.ast.tools.MethodNodeUtils.withDefaultArgumentMethods;
import static org.apache.groovy.util.BeanUtils.capitalize;
import static org.apache.groovy.util.BeanUtils.decapitalize;
import static org.codehaus.groovy.ast.ClassHelper.AUTOCLOSEABLE_TYPE;
@@ -4968,73 +4969,18 @@ trying: for (ClassNode[] signature : signatures) {
}
if (!receiver.isResolved() && !methods.isEmpty()) {
- methods = addGeneratedMethods(receiver, methods);
+ methods = withDefaultArgumentMethods(methods);
}
return methods;
}
- private static List<MethodNode> addGeneratedMethods(final ClassNode receiver, final List<? extends MethodNode> methods) {
- // using a comparator of parameters
- List<MethodNode> result = new LinkedList<>();
- for (MethodNode method : methods) {
- result.add(method);
- Parameter[] parameters = method.getParameters();
- int counter = 0;
- int size = parameters.length;
- for (int i = size - 1; i >= 0; i--) {
- Parameter parameter = parameters[i];
- if (parameter != null && parameter.hasInitialExpression()) {
- counter++;
- }
- }
-
- for (int j = 1; j <= counter; j++) {
- Parameter[] newParams = new Parameter[parameters.length - j];
- int index = 0;
- int k = 1;
- for (Parameter parameter : parameters) {
- if (k > counter - j && parameter != null && parameter.hasInitialExpression()) {
- k++;
- } else if (parameter != null && parameter.hasInitialExpression()) {
- newParams[index++] = parameter;
- k++;
- } else {
- newParams[index++] = parameter;
- }
- }
- MethodNode stubbed;
- if (method.isConstructor()) {
- stubbed = new ConstructorNode(
- method.getModifiers(),
- newParams,
- method.getExceptions(),
- GENERATED_EMPTY_STATEMENT
- );
- } else {
- stubbed = new MethodNode(
- method.getName(),
- method.getModifiers(),
- method.getReturnType(),
- newParams,
- method.getExceptions(),
- GENERATED_EMPTY_STATEMENT
- );
- stubbed.setGenericsTypes(method.getGenericsTypes());
- }
- stubbed.setDeclaringClass(method.getDeclaringClass());
- result.add(stubbed);
- }
- }
- return result;
- }
-
protected List<MethodNode> findMethod(ClassNode receiver, final String name, final ClassNode... args) {
if (isPrimitiveType(receiver)) receiver = getWrapper(receiver);
List<MethodNode> methods;
if ("<init>".equals(name) && !receiver.isInterface()) {
- methods = addGeneratedMethods(receiver, receiver.getDeclaredConstructors());
+ methods = withDefaultArgumentMethods(receiver.getDeclaredConstructors());
if (methods.isEmpty()) {
MethodNode node = new ConstructorNode(Opcodes.ACC_PUBLIC, Parameter.EMPTY_ARRAY, ClassNode.EMPTY_ARRAY, GENERATED_EMPTY_STATEMENT);
node.setDeclaringClass(receiver);
diff --git a/src/test/org/codehaus/groovy/transform/AutoImplementTransformTest.groovy b/src/test/org/codehaus/groovy/transform/AutoImplementTransformTest.groovy
index 536d881c5d..a9821fb1ab 100644
--- a/src/test/org/codehaus/groovy/transform/AutoImplementTransformTest.groovy
+++ b/src/test/org/codehaus/groovy/transform/AutoImplementTransformTest.groovy
@@ -88,7 +88,8 @@ final class AutoImplementTransformTest {
'''
}
- @Test // GROOVY-9816
+ // GROOVY-9816
+ @Test
void testPropertyMethodsNotOverwritten() {
assertScript shell, '''
interface Bar {
@@ -205,7 +206,8 @@ final class AutoImplementTransformTest {
'''
}
- @Test // GROOVY-8270
+ // GROOVY-8270
+ @Test
void testGenericParameterTypes() {
assertScript shell, '''
@AutoImplement
@@ -217,7 +219,8 @@ final class AutoImplementTransformTest {
'''
}
- @Test // GROOVY-10472
+ // GROOVY-10472
+ @Test
void testCovariantReturnTypes() {
assertScript shell, '''
interface Super { List findAll() }
@@ -300,7 +303,8 @@ final class AutoImplementTransformTest {
'''
}
- @Test // GROOVY-10552
+ // GROOVY-10552
+ @Test
void testMethodWithTypeParameter() {
assertScript shell, '''
interface I {
@@ -323,4 +327,26 @@ final class AutoImplementTransformTest {
new C().commit()
'''
}
+
+ // GROOVY-11339
+ @Test
+ void testMethodWithDefaultArgument() {
+ assertScript shell, '''
+ interface I {
+ def foo()
+ def foo(bar)
+ def foo(bar,baz)
+ }
+ @AutoImplement
+ class C implements I {
+ @Override // foo(bar) and foo(bar,baz)
+ def foo(bar, baz = null) {
+ return bar
+ }
+ }
+ def c = new C()
+ assert c.foo() == null
+ assert c.foo(12) == 12
+ '''
+ }
}