Skip to content

Commit a3c5939

Browse files
eric-milleseric-milles
committed
GROOVY-11550: check method for name clash (same erasure, different type)
1 parent fc776ff commit a3c5939

19 files changed

+206
-90
lines changed

‎src/main/java/org/apache/groovy/ast/tools/ClassNodeUtils.java

+8-8
Original file line numberDiff line numberDiff line change
@@ -75,20 +75,20 @@ public class ClassNodeUtils {
7575
*/
7676
public static String formatTypeName(final ClassNode cNode) {
7777
if (cNode.isArray()) {
78-
ClassNode it = cNode;
7978
int dim = 0;
80-
while (it.isArray()) {
81-
dim++;
82-
it = it.getComponentType();
79+
ClassNode cn = cNode;
80+
while (cn.isArray()) {
81+
dim += 1;
82+
cn = cn.getComponentType();
8383
}
84-
StringBuilder sb = new StringBuilder(it.getName().length() + 2 * dim);
85-
sb.append(it.getName());
86-
for (int i = 0; i < dim; i++) {
84+
StringBuilder sb = new StringBuilder(cn.getName().length() + (2 * dim));
85+
sb.append(formatTypeName(cn));
86+
for (int i = 0; i < dim; i += 1) {
8787
sb.append("[]");
8888
}
8989
return sb.toString();
9090
}
91-
return cNode.getName();
91+
return cNode.isGenericsPlaceHolder() ? cNode.getUnresolvedName() : cNode.getName();
9292
}
9393

9494
/**

‎src/main/java/org/apache/groovy/ast/tools/MethodNodeUtils.java

+35-13
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
*/
1919
package org.apache.groovy.ast.tools;
2020

21+
import org.codehaus.groovy.ast.ClassNode;
2122
import org.codehaus.groovy.ast.ConstructorNode;
2223
import org.codehaus.groovy.ast.MethodNode;
2324
import org.codehaus.groovy.ast.Parameter;
@@ -40,6 +41,14 @@ public class MethodNodeUtils {
4041

4142
private MethodNodeUtils() { }
4243

44+
private static void appendTypeName(final StringBuilder sb, ClassNode cn) {
45+
while (cn.isArray()) {
46+
cn = cn.getComponentType();
47+
sb.append('[');
48+
}
49+
sb.append(cn.getName());
50+
}
51+
4352
/**
4453
* Return the method node's descriptor including its
4554
* name and parameter types without generics.
@@ -51,7 +60,8 @@ public static String methodDescriptorWithoutReturnType(final MethodNode mNode) {
5160
StringBuilder sb = new StringBuilder();
5261
sb.append(mNode.getName()).append(':');
5362
for (Parameter p : mNode.getParameters()) {
54-
sb.append(ClassNodeUtils.formatTypeName(p.getType())).append(',');
63+
appendTypeName(sb, p.getType());
64+
sb.append(';');
5565
}
5666
return sb.toString();
5767
}
@@ -75,26 +85,38 @@ public static String methodDescriptor(final MethodNode mNode) {
7585
* @param pretty whether to quote a name with spaces
7686
* @return the method node's descriptor
7787
*/
78-
public static String methodDescriptor(final MethodNode mNode, boolean pretty) {
88+
public static String methodDescriptor(final MethodNode mNode, final boolean pretty) {
7989
String name = mNode.getName();
80-
if (pretty) pretty = name.contains(" ");
8190
Parameter[] parameters = mNode.getParameters();
82-
int nParameters = parameters == null ? 0 : parameters.length;
83-
84-
StringBuilder sb = new StringBuilder(name.length() * 2 + nParameters * 10);
85-
sb.append(ClassNodeUtils.formatTypeName(mNode.getReturnType()));
86-
sb.append(' ');
87-
if (pretty) sb.append('"');
88-
sb.append(name);
89-
if (pretty) sb.append('"');
91+
int nParameters = (parameters == null ? 0 : parameters.length);
92+
93+
StringBuilder sb = new StringBuilder((name.length() * 2) + (nParameters * 10));
94+
if (pretty && !mNode.isConstructor()) {
95+
sb.append(ClassNodeUtils.formatTypeName(mNode.getReturnType()));
96+
sb.append(' ');
97+
}
98+
if (name.contains(" ")) {
99+
sb.append('"').append(name).append('"');
100+
} else {
101+
sb.append(name);
102+
}
90103
sb.append('(');
91104
for (int i = 0; i < nParameters; i += 1) {
92105
if (i > 0) {
93-
sb.append(", ");
106+
sb.append(',');
107+
if (pretty) sb.append(' ');
108+
}
109+
if (pretty) {
110+
sb.append(ClassNodeUtils.formatTypeName(parameters[i].getType()));
111+
} else {
112+
appendTypeName(sb, parameters[i].getType());
94113
}
95-
sb.append(ClassNodeUtils.formatTypeName(parameters[i].getType()));
96114
}
97115
sb.append(')');
116+
if (!pretty && !mNode.isConstructor()) {
117+
sb.append(':');
118+
appendTypeName(sb, mNode.getReturnType());
119+
}
98120
return sb.toString();
99121
}
100122

‎src/main/java/org/codehaus/groovy/ast/tools/GenericsUtils.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -597,7 +597,7 @@ public static void extractSuperClassGenerics(final ClassNode type, final ClassNo
597597
} else {
598598
ClassNode superClass = getSuperClass(type, target);
599599
if (superClass != null) {
600-
if (hasUnresolvedGenerics(superClass)) {
600+
if (type.isRedirectNode() && hasUnresolvedGenerics(superClass)) {
601601
GenericsType[] tp = type.redirect().getGenericsTypes();
602602
if (tp != null) {
603603
GenericsType[] ta = type.getGenericsTypes();

‎src/main/java/org/codehaus/groovy/classgen/ClassCompletionVerifier.java

+85-9
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import groovy.transform.Sealed;
2222
import org.apache.groovy.ast.tools.AnnotatedNodeUtils;
2323
import org.apache.groovy.ast.tools.ClassNodeUtils;
24+
import org.apache.groovy.ast.tools.MethodNodeUtils;
2425
import org.codehaus.groovy.ast.ASTNode;
2526
import org.codehaus.groovy.ast.ClassCodeVisitorSupport;
2627
import org.codehaus.groovy.ast.ClassHelper;
@@ -55,6 +56,7 @@
5556
import java.util.LinkedHashSet;
5657
import java.util.List;
5758
import java.util.Map;
59+
import java.util.Set;
5860

5961
import static java.lang.reflect.Modifier.isFinal;
6062
import static java.lang.reflect.Modifier.isNative;
@@ -65,6 +67,10 @@
6567
import static java.lang.reflect.Modifier.isSynchronized;
6668
import static java.lang.reflect.Modifier.isTransient;
6769
import static java.lang.reflect.Modifier.isVolatile;
70+
import static org.codehaus.groovy.ast.tools.GenericsUtils.addMethodGenerics;
71+
import static org.codehaus.groovy.ast.tools.GenericsUtils.buildWildcardType;
72+
import static org.codehaus.groovy.ast.tools.GenericsUtils.correctToGenericsSpecRecurse;
73+
import static org.codehaus.groovy.ast.tools.GenericsUtils.createGenericsSpec;
6874
import static org.objectweb.asm.Opcodes.ACC_ABSTRACT;
6975
import static org.objectweb.asm.Opcodes.ACC_FINAL;
7076
import static org.objectweb.asm.Opcodes.ACC_NATIVE;
@@ -123,6 +129,7 @@ public void visitClass(final ClassNode node) {
123129
checkMethodsForIncorrectName(node);
124130
checkMethodsForWeakerAccess(node);
125131
checkMethodsForOverridingFinal(node);
132+
checkMethodsForOverridingIssue(node);
126133
checkNoAbstractMethodsNonAbstractClass(node);
127134
checkClassExtendsOrImplementsSelfTypes(node);
128135
checkNoStaticMethodWithSameSignatureAsNonStatic(node);
@@ -279,7 +286,7 @@ private static String getDescription(final ClassNode node) {
279286
}
280287

281288
private static String getDescription(final MethodNode node) {
282-
return "method '" + node.getTypeDescriptor() + "'";
289+
return "method '" + MethodNodeUtils.methodDescriptor(node, true) + "'";
283290
}
284291

285292
private static String getDescription(final FieldNode node) {
@@ -299,7 +306,7 @@ private void checkAbstractDeclaration(final MethodNode methodNode) {
299306

300307
addError("Can't have an abstract method in a non-abstract class." +
301308
" The " + getDescription(currentClass) + " must be declared abstract or the method '" +
302-
methodNode.getTypeDescriptor() + "' must not be abstract.", methodNode);
309+
MethodNodeUtils.methodDescriptor(methodNode, true) + "' must not be abstract.", methodNode);
303310
}
304311

305312
private void checkClassForExtendingFinalOrSealed(final ClassNode cn) {
@@ -404,28 +411,97 @@ private void checkMethodsForWeakerAccess(final ClassNode cn) {
404411
}
405412

406413
private void checkMethodsForOverridingFinal(final ClassNode cn) {
414+
final int skips = ACC_SYNTHETIC | ACC_STATIC | ACC_PRIVATE;
407415
for (MethodNode method : cn.getMethods()) {
408-
if ((method.getModifiers() & ACC_SYNTHETIC) != 0) continue; // GROOVY-11579: bridge method
416+
if ((method.getModifiers() & skips) != 0) continue; // GROOVY-11579
409417

410-
ClassNode sc = cn.getSuperClass();
411418
Parameter[] params = method.getParameters();
412-
for (MethodNode superMethod : sc.getMethods(method.getName())) {
413-
if (superMethod.isFinal()
419+
for (MethodNode superMethod : cn.getSuperClass().getMethods(method.getName())) {
420+
if ((superMethod.getModifiers() & skips + ACC_FINAL) == ACC_FINAL
414421
&& ParameterUtils.parametersEqual(params, superMethod.getParameters())) {
415422
StringBuilder sb = new StringBuilder();
416423
sb.append("You are not allowed to override the final method ");
417-
sb.append(method.getName());
424+
if (method.getName().contains(" ")) {
425+
sb.append('"').append(method.getName()).append('"');
426+
} else {
427+
sb.append(method.getName());
428+
}
418429
appendParamsDescription(params, sb);
419430
sb.append(" from ");
420-
sb.append(getDescription(sc));
431+
sb.append(getDescription(superMethod.getDeclaringClass()));
421432
sb.append(".");
422433

423434
addError(sb.toString(), method.getLineNumber() > 0 ? method : cn);
435+
break;
424436
}
425437
}
426438
}
427439
}
428440

441+
private void checkMethodsForOverridingIssue(final ClassNode cn) {
442+
Set<ClassNode> superTypes = getAllSuperTypes(cn);
443+
superTypes.remove(ClassHelper.GROOVY_OBJECT_TYPE);
444+
superTypes.remove(ClassHelper.OBJECT_TYPE);
445+
if (superTypes.isEmpty()) return;
446+
447+
for (MethodNode mn : cn.getMethods()) {
448+
Parameter[] pa = mn.getParameters();
449+
if (pa.length == 0 || (mn.getModifiers() & ACC_SYNTHETIC + ACC_STATIC + ACC_PRIVATE) != 0) continue;
450+
451+
out: for (ClassNode sc : superTypes) {
452+
Map<String, ClassNode> cspec = createGenericsSpec(sc);
453+
for (MethodNode sm : sc.getDeclaredMethods(mn.getName())) {
454+
if (!sm.isStatic() && !sm.isPrivate() && ParameterUtils.parametersEqual(pa, sm.getParameters())) {
455+
Map<String, ClassNode> mspec = addMethodGenerics(sm, cspec);
456+
for (int i = 0, n = pa.length; i < n; i += 1) {
457+
var t0 = sm.getParameters()[i].getType();
458+
var t1 = pa[i].getType();
459+
if (!t0.isGenericsPlaceHolder() && !ClassHelper.isPrimitiveType(t0)
460+
&& !t1.isGenericsPlaceHolder() && !ClassHelper.isPrimitiveType(t1)
461+
&& !buildWildcardType(t0 = correctToGenericsSpecRecurse(mspec, t0)).isCompatibleWith(t1)) {
462+
StringBuilder sb = new StringBuilder();
463+
sb.append("name clash: ");
464+
if (mn.getName().contains(" ")) {
465+
sb.append('"').append(mn.getName()).append('"');
466+
} else {
467+
sb.append(mn.getName());
468+
}
469+
appendParamsDescription(pa, sb);
470+
sb.append(" in ");
471+
sb.append(getDescription(cn));
472+
sb.append(" and ");
473+
if (sm.getName().contains(" ")) {
474+
sb.append('"').append(sm.getName()).append('"');
475+
} else {
476+
sb.append(sm.getName());
477+
}
478+
appendParamsDescription(sm.getParameters(), sb);
479+
sb.append(" in ");
480+
sb.append(getDescription(sc));
481+
sb.append(" have the same erasure, yet neither overrides the other.");
482+
483+
addError(sb.toString(), mn.getLineNumber() > 0 ? mn : cn);
484+
break;
485+
}
486+
}
487+
break out;
488+
}
489+
}
490+
}
491+
}
492+
}
493+
494+
private static Set<ClassNode> getAllSuperTypes(ClassNode cn) {
495+
Set<ClassNode> interfaces = GeneralUtils.getInterfacesAndSuperInterfaces(cn);
496+
Set<ClassNode> superTypes = new LinkedHashSet<>();
497+
interfaces.remove(cn);
498+
while ((cn = cn.getSuperClass()) != null) {
499+
superTypes.add(cn);
500+
}
501+
superTypes.addAll(interfaces);
502+
return superTypes;
503+
}
504+
429505
private void appendParamsDescription(final Parameter[] parameters, final StringBuilder msg) {
430506
msg.append('(');
431507
boolean needsComma = false;
@@ -435,7 +511,7 @@ private void appendParamsDescription(final Parameter[] parameters, final StringB
435511
} else {
436512
needsComma = true;
437513
}
438-
msg.append(parameter.getType());
514+
msg.append(parameter.getType().toString(false));
439515
}
440516
msg.append(')');
441517
}

‎src/main/java/org/codehaus/groovy/classgen/EnumVisitor.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -285,7 +285,7 @@ private void addInit(final ClassNode enumClass, final FieldNode minValue, final
285285
if (!methodNode.isAbstract()) continue;
286286
MethodNode enumConstMethod = inner.getMethod(methodNode.getName(), methodNode.getParameters());
287287
if (enumConstMethod == null || enumConstMethod.isAbstract()) {
288-
addError(field, "Can't have an abstract method in enum constant " + field.getName() + ". Implement method '" + methodNode.getTypeDescriptor() + "'.");
288+
addError(field, "Can't have an abstract method in enum constant " + field.getName() + ". Implement method '" + methodNode.getTypeDescriptor(true) + "'.");
289289
}
290290
}
291291
if (inner.getVariableScope() == null) {

0 commit comments

Comments
 (0)