You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by pa...@apache.org on 2019/11/12 04:19:19 UTC
[groovy] 01/03: GROOVY-9244: prevent ambiguous constructor if using
@InheritConstructors
This is an automated email from the ASF dual-hosted git repository.
paulk pushed a commit to branch GROOVY_2_5_X
in repository https://gitbox.apache.org/repos/asf/groovy.git
commit e9e20f795a67724b571ebaf8738c51ac548fbecc
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Fri Oct 4 11:18:56 2019 -0500
GROOVY-9244: prevent ambiguous constructor if using @InheritConstructors
class Base {
Base(Number n) {}
Base(String s) {}
}
@groovy.transform.InheritConstructors
class Impl {
}
new Impl((String) null) // should trigger Base(String)
---
.../InheritConstructorsASTTransformation.java | 38 +++----
src/test/groovy/bugs/Groovy9244.groovy | 117 +++++++++++++++++++++
2 files changed, 133 insertions(+), 22 deletions(-)
diff --git a/src/main/java/org/codehaus/groovy/transform/InheritConstructorsASTTransformation.java b/src/main/java/org/codehaus/groovy/transform/InheritConstructorsASTTransformation.java
index 42f95f9..8333930 100644
--- a/src/main/java/org/codehaus/groovy/transform/InheritConstructorsASTTransformation.java
+++ b/src/main/java/org/codehaus/groovy/transform/InheritConstructorsASTTransformation.java
@@ -38,6 +38,7 @@ import static org.apache.groovy.ast.tools.ClassNodeUtils.addGeneratedConstructor
import static org.codehaus.groovy.ast.ClassHelper.make;
import static org.codehaus.groovy.ast.tools.GeneralUtils.args;
import static org.codehaus.groovy.ast.tools.GeneralUtils.block;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.castX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.ctorSuperS;
import static org.codehaus.groovy.ast.tools.GeneralUtils.param;
import static org.codehaus.groovy.ast.tools.GeneralUtils.varX;
@@ -51,31 +52,28 @@ import static org.codehaus.groovy.ast.tools.GenericsUtils.extractSuperClassGener
@GroovyASTTransformation(phase = CompilePhase.CANONICALIZATION)
public class InheritConstructorsASTTransformation extends AbstractASTTransformation {
- private static final Class MY_CLASS = InheritConstructors.class;
- private static final ClassNode MY_TYPE = make(MY_CLASS);
- private static final String MY_TYPE_NAME = "@" + MY_TYPE.getNameWithoutPackage();
+ private static final ClassNode INHERIT_CONSTRUCTORS_TYPE = make(InheritConstructors.class);
+ private static final String ANNOTATION = "@" + INHERIT_CONSTRUCTORS_TYPE.getNameWithoutPackage();
public void visit(ASTNode[] nodes, SourceUnit source) {
init(nodes, source);
- AnnotatedNode parent = (AnnotatedNode) nodes[1];
- AnnotationNode node = (AnnotationNode) nodes[0];
- if (!MY_TYPE.equals(node.getClassNode())) return;
-
- if (parent instanceof ClassNode) {
- processClass((ClassNode) parent, node);
+ AnnotationNode anno = (AnnotationNode) nodes[0];
+ AnnotatedNode target = (AnnotatedNode) nodes[1];
+ if (INHERIT_CONSTRUCTORS_TYPE.equals(anno.getClassNode()) && target instanceof ClassNode) {
+ processClass((ClassNode) target, anno);
}
}
private void processClass(ClassNode cNode, AnnotationNode node) {
if (cNode.isInterface()) {
addError("Error processing interface '" + cNode.getName() +
- "'. " + MY_TYPE_NAME + " only allowed for classes.", cNode);
+ "'. " + ANNOTATION + " only allowed for classes.", cNode);
return;
}
boolean copyConstructorAnnotations = memberHasValue(node, "constructorAnnotations", true);
boolean copyParameterAnnotations = memberHasValue(node, "parameterAnnotations", true);
ClassNode sNode = cNode.getSuperClass();
- List<AnnotationNode> superAnnotations = sNode.getAnnotations(MY_TYPE);
+ List<AnnotationNode> superAnnotations = sNode.getAnnotations(INHERIT_CONSTRUCTORS_TYPE);
if (superAnnotations.size() == 1) {
// We need @InheritConstructors from parent classes processed first
// so force that order here. The transformation is benign on an already
@@ -98,31 +96,27 @@ public class InheritConstructorsASTTransformation extends AbstractASTTransformat
if (isExisting(classNode, params)) return;
ConstructorNode added = addGeneratedConstructor(classNode, consNode.getModifiers(), params, consNode.getExceptions(), block(ctorSuperS(args(theArgs))));
if (copyConstructorAnnotations) {
- added.addAnnotations(copyAnnotatedNodeAnnotations(consNode, MY_TYPE_NAME));
+ added.addAnnotations(copyAnnotatedNodeAnnotations(consNode, ANNOTATION));
}
}
private List<Expression> buildParams(Parameter[] origParams, Parameter[] params, Map<String, ClassNode> genericsSpec, boolean copyParameterAnnotations) {
- List<Expression> theArgs = new ArrayList<Expression>();
- for (int i = 0; i < origParams.length; i++) {
+ List<Expression> theArgs = new ArrayList<>();
+ for (int i = 0, n = origParams.length; i < n; i += 1) {
Parameter p = origParams[i];
ClassNode newType = correctToGenericsSpecRecurse(genericsSpec, p.getType());
params[i] = p.hasInitialExpression() ? param(newType, p.getName(), p.getInitialExpression()) : param(newType, p.getName());
if (copyParameterAnnotations) {
- params[i].addAnnotations(copyAnnotatedNodeAnnotations(origParams[i], MY_TYPE_NAME));
+ params[i].addAnnotations(copyAnnotatedNodeAnnotations(origParams[i], ANNOTATION));
}
- theArgs.add(varX(p.getName(), newType));
+ // cast argument to parameter type in case the value is null
+ theArgs.add(castX(p.getType(), varX(p.getName(), newType)));
}
return theArgs;
}
private static boolean isExisting(ClassNode classNode, Parameter[] params) {
- for (ConstructorNode consNode : classNode.getDeclaredConstructors()) {
- if (matchingTypes(params, consNode.getParameters())) {
- return true;
- }
- }
- return false;
+ return classNode.getDeclaredConstructors().stream().anyMatch(ctor -> matchingTypes(params, ctor.getParameters()));
}
private static boolean matchingTypes(Parameter[] params, Parameter[] existingParams) {
diff --git a/src/test/groovy/bugs/Groovy9244.groovy b/src/test/groovy/bugs/Groovy9244.groovy
new file mode 100644
index 0000000..3ce3235
--- /dev/null
+++ b/src/test/groovy/bugs/Groovy9244.groovy
@@ -0,0 +1,117 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package groovy.bugs
+
+import groovy.test.NotYetImplemented
+import org.junit.Test
+
+import static groovy.test.GroovyAssert.assertScript
+
+final class Groovy9244 {
+
+ @Test
+ void testSuperConstructorsAndNullArgument1() {
+ assertScript '''
+ abstract class Base {
+ Base(Number n) { which = 'Number' }
+ Base(String s) { which = 'String' }
+ public String which
+ }
+
+ class Impl extends Base {
+ Impl(Number n) { super((Number) n) }
+ Impl(String s) { super((String) s) }
+ }
+
+ def impl = new Impl((String) null)
+ assert impl.which == 'String'
+ '''
+ }
+
+ @Test
+ void testSuperConstructorsAndNullArgument2() {
+ assertScript '''
+ abstract class Base {
+ Base(Number n) { which = 'Number' }
+ Base(String s) { which = 'String' }
+ public String which
+ }
+
+ @groovy.transform.CompileStatic
+ class Impl extends Base {
+ Impl(Number n) { super(n) }
+ Impl(String s) { super(s) }
+ }
+
+ def impl = new Impl((String) null)
+ assert impl.which == 'String'
+ '''
+ }
+
+ @Test
+ void testSuperConstructorsAndNullArgument3() {
+ assertScript '''
+ abstract class Base {
+ Base(Number n) { which = 'Number' }
+ Base(String s) { which = 'String' }
+ public String which
+ }
+
+ @groovy.transform.InheritConstructors
+ class Impl extends Base {
+ }
+
+ def impl = new Impl((String) null)
+ assert impl.which == 'String'
+ '''
+ }
+
+ @Test @NotYetImplemented
+ void testSuperConstructorsAndNullArgument4() {
+ assertScript '''
+ abstract class Base {
+ Base(Number n) { which = 'Number' }
+ Base(String s) { which = 'String' }
+ public String which
+ }
+
+ class Impl extends Base {
+ Impl(Number n) { super(n) }
+ Impl(String s) { super(s) }
+ }
+
+ def impl = new Impl((String) null)
+ assert impl.which == 'String'
+ '''
+ }
+
+ @Test @NotYetImplemented
+ void testSuperConstructorsAndNullArgument5() {
+ assertScript '''
+ abstract class Base {
+ Base(Number n) { which = 'Number' }
+ Base(String s) { which = 'String' }
+ public String which
+ }
+
+ def impl = new Base((String) null) {}
+ assert iompl.which == 'String'
+ '''
+ }
+}