This is an automated email from the ASF dual-hosted git repository. emilles pushed a commit to branch GROOVY-11758 in repository https://gitbox.apache.org/repos/asf/groovy.git
commit b38d70a4d17cdb2f0cf92a6d23282966618f9fe3 Author: Eric Milles <[email protected]> AuthorDate: Fri Sep 19 19:19:48 2025 -0500 GROOVY-11758: detect final super method that shadows interface method --- .../groovy/classgen/ClassCompletionVerifier.java | 97 ++++++++++++++-------- .../transform/trait/TraitASTTransformation.java | 8 +- .../groovy/transform/trait/TraitComposer.java | 42 +++++----- src/test/groovy/groovy/InterfaceTest.groovy | 16 ++++ .../traitx/TraitASTTransformationTest.groovy | 2 +- 5 files changed, 102 insertions(+), 63 deletions(-) diff --git a/src/main/java/org/codehaus/groovy/classgen/ClassCompletionVerifier.java b/src/main/java/org/codehaus/groovy/classgen/ClassCompletionVerifier.java index c470489b99..9c96e6ffba 100644 --- a/src/main/java/org/codehaus/groovy/classgen/ClassCompletionVerifier.java +++ b/src/main/java/org/codehaus/groovy/classgen/ClassCompletionVerifier.java @@ -77,6 +77,7 @@ import static org.codehaus.groovy.ast.tools.GenericsUtils.createGenericsSpec; import static org.codehaus.groovy.transform.sc.StaticCompilationVisitor.isStaticallyCompiled; import static org.objectweb.asm.Opcodes.ACC_FINAL; import static org.objectweb.asm.Opcodes.ACC_PRIVATE; +import static org.objectweb.asm.Opcodes.ACC_PUBLIC; import static org.objectweb.asm.Opcodes.ACC_STATIC; import static org.objectweb.asm.Opcodes.ACC_SYNTHETIC; import static org.objectweb.asm.Opcodes.ACC_VOLATILE; @@ -396,11 +397,72 @@ public class ClassCompletionVerifier extends ClassCodeVisitorSupport { } private void checkMethodsForWeakerAccess(final ClassNode cn) { - for (MethodNode method : cn.getMethods()) { - checkMethodForWeakerAccessPrivileges(method, cn); + for (MethodNode cnMethod : cn.getMethods()) { + if (!cnMethod.isPublic() && !cnMethod.isStatic()) { + sc: for (MethodNode scMethod : cn.getSuperClass().getMethods(cnMethod.getName())) { + if (!scMethod.isStatic() + && !scMethod.isPrivate() + && (cnMethod.isPrivate() + || (cnMethod.isProtected() && scMethod.isPublic()) + || (cnMethod.isPackageScope() && (scMethod.isPublic() || scMethod.isProtected())))) { + if (ParameterUtils.parametersEqual(cnMethod.getParameters(), scMethod.getParameters())) { + addWeakerAccessError(cn, cnMethod, scMethod); + break sc; + } + } + } + } + } + + // Verifier: checks weaker access of cn's methods against abstract or default interface methods + + // GROOVY-11758: check for non-public final super class method that shadows an interface method + Map<String, MethodNode> interfaceMethods = ClassNodeUtils.getDeclaredMethodsFromInterfaces(cn); + if (!interfaceMethods.isEmpty()) { + for (MethodNode cnMethod : cn.getMethods()) { + if (!cnMethod.isPrivate() && !cnMethod.isStatic()) { + interfaceMethods.remove(cnMethod.getTypeDescriptor()); + } + } + final int finalStaticPrivatePublic = ACC_FINAL | ACC_STATIC | ACC_PRIVATE | ACC_PUBLIC; + for (MethodNode publicMethod : interfaceMethods.values()) { + for (MethodNode scMethod : cn.getSuperClass().getMethods(publicMethod.getName())) { + if ((scMethod.getModifiers() & finalStaticPrivatePublic) == ACC_FINAL) { + addWeakerAccessError2(cn, scMethod, publicMethod); + } + } + } } } + private void addWeakerAccessError(final ClassNode cn, final MethodNode cnMethod, final MethodNode scMethod) { + StringBuilder msg = new StringBuilder(); + msg.append(cnMethod.getName()); + appendParamsDescription(cnMethod.getParameters(), msg); + msg.append(" in "); + msg.append(cn.getName()); + msg.append(" cannot override "); + msg.append(scMethod.getName()); + msg.append(" in "); + msg.append(scMethod.getDeclaringClass().getName()); + msg.append("; attempting to assign weaker access privileges; was "); + msg.append(scMethod.isPublic() ? "public" : (scMethod.isProtected() ? "protected" : "package-private")); + + addError(msg.toString(), cnMethod); + } + + private void addWeakerAccessError2(final ClassNode cn, final MethodNode scMethod, final MethodNode ifMethod) { + StringBuilder msg = new StringBuilder(); + msg.append("inherited final method "); + msg.append(scMethod.getName()); + appendParamsDescription(scMethod.getParameters(), msg); + msg.append(" from "); + msg.append(scMethod.getDeclaringClass().getName()); + msg.append(" cannot shadow the public method in "); + msg.append(ifMethod.getDeclaringClass().getName()); + addError(msg.toString(), cn); + } + private void checkMethodsForOverridingFinal(final ClassNode cn) { final int skips = ACC_SYNTHETIC | ACC_STATIC | ACC_PRIVATE; for (MethodNode method : cn.getMethods()) { @@ -507,22 +569,6 @@ out: for (ClassNode sc : superTypes) { msg.append(')'); } - private void addWeakerAccessError(final ClassNode cn, final MethodNode method, final Parameter[] parameters, final MethodNode superMethod) { - StringBuilder msg = new StringBuilder(); - msg.append(method.getName()); - appendParamsDescription(parameters, msg); - msg.append(" in "); - msg.append(cn.getName()); - msg.append(" cannot override "); - msg.append(superMethod.getName()); - msg.append(" in "); - msg.append(superMethod.getDeclaringClass().getName()); - msg.append("; attempting to assign weaker access privileges; was "); - msg.append(superMethod.isPublic() ? "public" : (superMethod.isProtected() ? "protected" : "package-private")); - - addError(msg.toString(), method); - } - @Override public void visitMethod(final MethodNode node) { inConstructor = false; @@ -567,21 +613,6 @@ out: for (ClassNode sc : superTypes) { } } - private void checkMethodForWeakerAccessPrivileges(final MethodNode mn, final ClassNode cn) { - if (mn.isPublic()) return; - Parameter[] params = mn.getParameters(); - for (MethodNode superMethod : cn.getSuperClass().getMethods(mn.getName())) { - Parameter[] superParams = superMethod.getParameters(); - if (!ParameterUtils.parametersEqual(params, superParams)) continue; - if ((mn.isPrivate() && !superMethod.isPrivate()) - || (mn.isProtected() && !superMethod.isProtected() && !superMethod.isPackageScope() && !superMethod.isPrivate()) - || (!mn.isPrivate() && !mn.isProtected() && !mn.isPublic() && (superMethod.isPublic() || superMethod.isProtected()))) { - addWeakerAccessError(cn, mn, params, superMethod); - return; - } - } - } - private void checkOverloadingPrivateAndPublic(final MethodNode node) { if (isStaticallyCompiled(currentClass)) return; // GROOVY-11627 boolean mixed = false; diff --git a/src/main/java/org/codehaus/groovy/transform/trait/TraitASTTransformation.java b/src/main/java/org/codehaus/groovy/transform/trait/TraitASTTransformation.java index 50cf1eac1f..aea4019036 100644 --- a/src/main/java/org/codehaus/groovy/transform/trait/TraitASTTransformation.java +++ b/src/main/java/org/codehaus/groovy/transform/trait/TraitASTTransformation.java @@ -507,13 +507,7 @@ public class TraitASTTransformation extends AbstractASTTransformation implements } private static boolean methodNeedsReplacement(final ClassNode cNode, final MethodNode mNode) { - // no method found, we need to replace - if (mNode == null) return true; - // method is in current class, nothing to be done - if (mNode.getDeclaringClass() == cNode) return false; - // do not overwrite final - if ((mNode.getModifiers() & ACC_FINAL) != 0) return false; - return true; + return mNode == null || mNode.getDeclaringClass() != cNode; // do not replace trait method } private void processField(final FieldNode field, final MethodNode initializer, final MethodNode staticInitializer, diff --git a/src/main/java/org/codehaus/groovy/transform/trait/TraitComposer.java b/src/main/java/org/codehaus/groovy/transform/trait/TraitComposer.java index c0dc22ba09..a8e6f10769 100644 --- a/src/main/java/org/codehaus/groovy/transform/trait/TraitComposer.java +++ b/src/main/java/org/codehaus/groovy/transform/trait/TraitComposer.java @@ -93,12 +93,14 @@ public abstract class TraitComposer { * to call this method on a class node which does not implement a trait. */ public static void doExtendTraits(final ClassNode cn, final SourceUnit su, final CompilationUnit cu) { - if (cn.isInterface()) return; + if (cn.isInterface()) { + return; + } if (Traits.isTrait(cn)) { checkTraitAllowed(cn, su); return; } - if (!cn.getNameWithoutPackage().endsWith(Traits.TRAIT_HELPER)) { + if (!cn.getName().endsWith(Traits.TRAIT_HELPER)) { GroovyClassVisitor visitor = new SuperCallTraitTransformer(su); for (ClassNode trait : Traits.findTraits(cn)) { applyTrait(trait, cn, Traits.findHelpers(trait), su); @@ -119,13 +121,13 @@ public abstract class TraitComposer { } } - private static void applyTrait(final ClassNode trait, final ClassNode cNode, final TraitHelpersTuple helpers, SourceUnit unit) { + private static void applyTrait(final ClassNode trait, final ClassNode cNode, final TraitHelpersTuple helpers, final SourceUnit unit) { ClassNode helperClassNode = helpers.getHelper(); ClassNode fieldHelperClassNode = helpers.getFieldHelper(); ClassNode staticFieldHelperClassNode = helpers.getStaticFieldHelper(); Map<String, ClassNode> genericsSpec = GenericsUtils.createGenericsSpec(trait, GenericsUtils.createGenericsSpec(cNode)); - List<MethodNode> hMethods = new ArrayList<>(helperClassNode.getMethods()); + var hMethods = new ArrayList<MethodNode>(helperClassNode.getMethods()); if (hMethods.size() > 1) { hMethods.sort(Comparator.comparing(MethodNodeUtils::methodDescriptorWithoutReturnType)); } @@ -371,19 +373,15 @@ public abstract class TraitComposer { forwarder.addAnnotation(bridgeAnnotation); MethodNode existingMethod = findExistingMethod(targetNode, forwarder); - if (existingMethod != null) { - if (!forwarder.isStatic() && existingMethod.isStatic()) { - // found an existing static method that is going to conflict with interface - unit.addError(createException(trait, targetNode, forwarder, existingMethod)); - return; + if (existingMethod != null && existingMethod.isStatic() && !forwarder.isStatic()) { + // found an existing static method that is going to conflict with interface + unit.addError(createException(trait, targetNode, forwarder, existingMethod)); + } else { + if (!shouldSkipMethod(targetNode, forwarder.getName(), forwarderParams)) { + targetNode.addMethod(forwarder); } + createSuperForwarder(targetNode, forwarder, genericsSpec); } - - if (!shouldSkipMethod(targetNode, forwarder.getName(), forwarderParams)) { - targetNode.addMethod(forwarder); - } - - createSuperForwarder(targetNode, forwarder, genericsSpec); } private static SyntaxException createException(ClassNode trait, ClassNode targetNode, MethodNode forwarder, MethodNode existingMethod) { @@ -414,10 +412,10 @@ public abstract class TraitComposer { return new SyntaxException(message, errorTarget); } - private static GenericsType[] removeNonPlaceHolders(GenericsType[] oldTypes) { - if (oldTypes==null || oldTypes.length==0) return oldTypes; - ArrayList<GenericsType> l = new ArrayList<>(Arrays.asList(oldTypes)); - Iterator<GenericsType> it = l.iterator(); + private static GenericsType[] removeNonPlaceHolders(final GenericsType[] oldTypes) { + if (oldTypes == null || oldTypes.length == 0) return oldTypes; + var list = new ArrayList<GenericsType>(Arrays.asList(oldTypes)); + Iterator<GenericsType> it = list.iterator(); boolean modified = false; while (it.hasNext()) { GenericsType gt = it.next(); @@ -427,8 +425,8 @@ public abstract class TraitComposer { } } if (!modified) return oldTypes; - if (l.isEmpty()) return null; - return l.toArray(GenericsType.EMPTY_ARRAY); + if (list.isEmpty()) return null; + return list.toArray(GenericsType[]::new); } /** @@ -436,7 +434,7 @@ public abstract class TraitComposer { * @param forwarder a forwarder method * @param genericsSpec */ - private static void createSuperForwarder(ClassNode targetNode, MethodNode forwarder, final Map<String,ClassNode> genericsSpec) { + private static void createSuperForwarder(final ClassNode targetNode, final MethodNode forwarder, final Map<String,ClassNode> genericsSpec) { List<ClassNode> interfaces = new ArrayList<>(Traits.collectAllInterfacesReverseOrder(targetNode, new LinkedHashSet<>())); String name = forwarder.getName(); Parameter[] forwarderParameters = forwarder.getParameters(); diff --git a/src/test/groovy/groovy/InterfaceTest.groovy b/src/test/groovy/groovy/InterfaceTest.groovy index 2b55ab0cf0..629c141552 100644 --- a/src/test/groovy/groovy/InterfaceTest.groovy +++ b/src/test/groovy/groovy/InterfaceTest.groovy @@ -135,6 +135,22 @@ final class InterfaceTest extends CompilableTestSupport { ''' } + // GROOVY-11758 + void testSuperClassFinalMethodAndInterfaceMethod() { + def err = shouldNotCompile ''' + interface A { + def m() + } + class B { + protected final m() { + } + } + class C extends B implements A { + } + ''' + assert err =~ /inherited final method m\(\) from B cannot shadow the public method in A/ + } + // GROOVY-11753 void testSuperClassCovariantOfParameterizedInterface() { assertScript ''' diff --git a/src/test/groovy/org/codehaus/groovy/transform/traitx/TraitASTTransformationTest.groovy b/src/test/groovy/org/codehaus/groovy/transform/traitx/TraitASTTransformationTest.groovy index b697369a4f..194d8fbf22 100644 --- a/src/test/groovy/org/codehaus/groovy/transform/traitx/TraitASTTransformationTest.groovy +++ b/src/test/groovy/org/codehaus/groovy/transform/traitx/TraitASTTransformationTest.groovy @@ -441,7 +441,7 @@ final class TraitASTTransformationTest { } @ParameterizedTest - @ValueSource(strings=['public',/*TODO:'protected',*/'private']) + @ValueSource(strings=['public','private']) void testFinalPropertyGetterDefinedInSuperClass(String modifier) { assertScript shell, """ trait Foo {
