Repository: groovy Updated Branches: refs/heads/master 4738970c6 -> 3d60dbd79
GROOVY-7620: No error if abstract getter is not implemented but static field exists (closes #342) Project: http://git-wip-us.apache.org/repos/asf/groovy/repo Commit: http://git-wip-us.apache.org/repos/asf/groovy/commit/973197fd Tree: http://git-wip-us.apache.org/repos/asf/groovy/tree/973197fd Diff: http://git-wip-us.apache.org/repos/asf/groovy/diff/973197fd Branch: refs/heads/master Commit: 973197fdb5e27df579f5ae9b918d3fa13213f482 Parents: 4738970 Author: paulk <pa...@asert.com.au> Authored: Tue May 31 21:14:54 2016 +1000 Committer: paulk <pa...@asert.com.au> Committed: Wed Jun 1 13:20:06 2016 +1000 ---------------------------------------------------------------------- src/main/org/codehaus/groovy/ast/ClassNode.java | 23 ++++--- .../classgen/ClassCompletionVerifier.java | 71 +++++++++++--------- src/test/groovy/bugs/Groovy7081Bug.groovy | 2 +- src/test/groovy/bugs/Groovy7620Bug.groovy | 59 ++++++++++++++++ 4 files changed, 114 insertions(+), 41 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/groovy/blob/973197fd/src/main/org/codehaus/groovy/ast/ClassNode.java ---------------------------------------------------------------------- diff --git a/src/main/org/codehaus/groovy/ast/ClassNode.java b/src/main/org/codehaus/groovy/ast/ClassNode.java index 6601ac7..68ac22d 100644 --- a/src/main/org/codehaus/groovy/ast/ClassNode.java +++ b/src/main/org/codehaus/groovy/ast/ClassNode.java @@ -425,30 +425,33 @@ public class ClassNode extends AnnotatedNode implements Opcodes { public Map<String, MethodNode> getDeclaredMethodsMap() { // Start off with the methods from the superclass. ClassNode parent = getSuperClass(); - Map<String, MethodNode> result = null; + Map<String, MethodNode> result; if (parent != null) { result = parent.getDeclaredMethodsMap(); } else { result = new HashMap<String, MethodNode>(); } + addInterfaceMethods(result); + // And add in the methods implemented in this class. + for (MethodNode method : getMethods()) { + String sig = method.getTypeDescriptor(); + result.put(sig, method); + } + return result; + } + + public void addInterfaceMethods(Map<String, MethodNode> methodsMap) { // add in unimplemented abstract methods from the interfaces for (ClassNode iface : getInterfaces()) { Map<String, MethodNode> ifaceMethodsMap = iface.getDeclaredMethodsMap(); for (String methSig : ifaceMethodsMap.keySet()) { - if (!result.containsKey(methSig)) { + if (!methodsMap.containsKey(methSig)) { MethodNode methNode = ifaceMethodsMap.get(methSig); - result.put(methSig, methNode); + methodsMap.put(methSig, methNode); } } } - - // And add in the methods implemented in this class. - for (MethodNode method : getMethods()) { - String sig = method.getTypeDescriptor(); - result.put(sig, method); - } - return result; } public String getName() { http://git-wip-us.apache.org/repos/asf/groovy/blob/973197fd/src/main/org/codehaus/groovy/classgen/ClassCompletionVerifier.java ---------------------------------------------------------------------- diff --git a/src/main/org/codehaus/groovy/classgen/ClassCompletionVerifier.java b/src/main/org/codehaus/groovy/classgen/ClassCompletionVerifier.java index 47e3175..c29c705 100644 --- a/src/main/org/codehaus/groovy/classgen/ClassCompletionVerifier.java +++ b/src/main/org/codehaus/groovy/classgen/ClassCompletionVerifier.java @@ -84,33 +84,48 @@ public class ClassCompletionVerifier extends ClassCodeVisitorSupport { } private void checkNoStaticMethodWithSameSignatureAsNonStatic(final ClassNode node) { - Map<String, MethodNode> result = new HashMap<String, MethodNode>(); - // add in unimplemented abstract methods from the interfaces - for (ClassNode iface : node.getInterfaces()) { - Map<String, MethodNode> ifaceMethodsMap = iface.getDeclaredMethodsMap(); - for (String methSig : ifaceMethodsMap.keySet()) { - if (!result.containsKey(methSig)) { - MethodNode methNode = ifaceMethodsMap.get(methSig); - result.put(methSig, methNode); - } - } + ClassNode parent = node.getSuperClass(); + Map<String, MethodNode> result; + // start with methods from the parent if any + if (parent != null) { + result = parent.getDeclaredMethodsMap(); + } else { + result = new HashMap<String, MethodNode>(); } + // add in unimplemented abstract methods from the interfaces + node.addInterfaceMethods(result); for (MethodNode methodNode : node.getMethods()) { MethodNode mn = result.get(methodNode.getTypeDescriptor()); - if (mn!=null && methodNode.isStatic() && !methodNode.isStaticConstructor()) { + if (mn != null && (mn.isStatic() ^ methodNode.isStatic()) && !methodNode.isStaticConstructor()) { + if (!mn.isAbstract()) continue; ClassNode declaringClass = mn.getDeclaringClass(); ClassNode cn = declaringClass.getOuterClass(); - if (cn==null && declaringClass.isResolved()) { + if (cn == null && declaringClass.isResolved()) { // in case of a precompiled class, the outerclass is unknown Class typeClass = declaringClass.getTypeClass(); typeClass = typeClass.getEnclosingClass(); - if (typeClass!=null) { + if (typeClass != null) { cn = ClassHelper.make(typeClass); } } - if (cn==null || !Traits.isTrait(cn)) { - addError("Method '" + mn.getName() + "' is already defined in " + getDescription(node) + ". You cannot have " + - "both a static and a non static method with the same signature", methodNode); + if (cn == null || !Traits.isTrait(cn)) { + ASTNode errorNode = methodNode; + String name = mn.getName(); + if (errorNode.getLineNumber() == -1) { + // try to get a better error message location based on the property + for (PropertyNode propertyNode : node.getProperties()) { + if (name.startsWith("set") || name.startsWith("get") || name.startsWith("is")) { + String propName = Verifier.capitalize(propertyNode.getField().getName()); + String shortName = name.substring(name.startsWith("is") ? 2 : 3); + if (propName.equals(shortName)) { + errorNode = propertyNode; + break; + } + } + } + } + addError("The " + getDescription(methodNode) + " is already defined in " + getDescription(node) + + ". You cannot have both a static and an instance method with the same signature", errorNode); } } result.put(methodNode.getTypeDescriptor(), methodNode); @@ -307,6 +322,13 @@ public class ClassCompletionVerifier extends ClassCodeVisitorSupport { private void addInvalidUseOfFinalError(MethodNode method, Parameter[] parameters, ClassNode superCN) { StringBuilder msg = new StringBuilder(); msg.append("You are not allowed to override the final method ").append(method.getName()); + appendParamsDescription(parameters, msg); + msg.append(" from ").append(getDescription(superCN)); + msg.append("."); + addError(msg.toString(), method); + } + + private void appendParamsDescription(Parameter[] parameters, StringBuilder msg) { msg.append("("); boolean needsComma = false; for (Parameter parameter : parameters) { @@ -317,25 +339,14 @@ public class ClassCompletionVerifier extends ClassCodeVisitorSupport { } msg.append(parameter.getType()); } - msg.append(") from ").append(getDescription(superCN)); - msg.append("."); - addError(msg.toString(), method); + msg.append(")"); } private void addWeakerAccessError(ClassNode cn, MethodNode method, Parameter[] parameters, MethodNode superMethod) { StringBuilder msg = new StringBuilder(); msg.append(method.getName()); - msg.append("("); - boolean needsComma = false; - for (Parameter parameter : parameters) { - if (needsComma) { - msg.append(","); - } else { - needsComma = true; - } - msg.append(parameter.getType()); - } - msg.append(") in "); + appendParamsDescription(parameters, msg); + msg.append(" in "); msg.append(cn.getName()); msg.append(" cannot override "); msg.append(superMethod.getName()); http://git-wip-us.apache.org/repos/asf/groovy/blob/973197fd/src/test/groovy/bugs/Groovy7081Bug.groovy ---------------------------------------------------------------------- diff --git a/src/test/groovy/bugs/Groovy7081Bug.groovy b/src/test/groovy/bugs/Groovy7081Bug.groovy index ba6c44e..1415318 100644 --- a/src/test/groovy/bugs/Groovy7081Bug.groovy +++ b/src/test/groovy/bugs/Groovy7081Bug.groovy @@ -31,7 +31,7 @@ class Groovy7081Bug extends GroovyTestCase { new Foo().name ''' - assert msg.contains("Method 'getName' is already defined in class 'Foo'") + assert msg.contains("The method 'java.lang.String getName()' is already defined in class 'Foo'") } void testShouldSeeConflictInTypeSignature() { http://git-wip-us.apache.org/repos/asf/groovy/blob/973197fd/src/test/groovy/bugs/Groovy7620Bug.groovy ---------------------------------------------------------------------- diff --git a/src/test/groovy/bugs/Groovy7620Bug.groovy b/src/test/groovy/bugs/Groovy7620Bug.groovy new file mode 100644 index 0000000..44525ef --- /dev/null +++ b/src/test/groovy/bugs/Groovy7620Bug.groovy @@ -0,0 +1,59 @@ +/* + * 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 + +class Groovy7620Bug extends GroovyTestCase { + void testShouldSeeThatMethodIsNotImplemented() { + def msg = shouldFail ''' + abstract class A { + abstract Object getFoo() + + void test() { + println getFoo() + } + } + + class B extends A { + static Object foo + } + + new B().test() + ''' + + assert msg.contains("The method 'java.lang.Object getFoo()' is already defined in class 'B'") + } + + void testShouldSeeConflictInTypeSignature() { + def msg = shouldFail ''' + interface C { + Object getFoo() + } + + class D implements C { + static Object foo + } + + new B().test() + ''' + + assert msg.contains("The method 'java.lang.Object getFoo()' is already defined in class 'D'") + } +}
