This is an automated email from the ASF dual-hosted git repository.
emilles pushed a commit to branch GROOVY_5_0_X
in repository https://gitbox.apache.org/repos/asf/groovy.git
The following commit(s) were added to refs/heads/GROOVY_5_0_X by this push:
new 1f001e6fba GROOVY-11758: detect final super method that shadows
interface method
1f001e6fba is described below
commit 1f001e6fba40fa40e6f86b9fef98043dc6490e02
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 | 96 ++++++++++++++--------
.../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, 101 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..7caca8c80b 100644
--- a/src/main/java/org/codehaus/groovy/classgen/ClassCompletionVerifier.java
+++ b/src/main/java/org/codehaus/groovy/classgen/ClassCompletionVerifier.java
@@ -396,11 +396,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());
+ }
+ }
+ for (MethodNode publicMethod : interfaceMethods.values()) {
+ for (MethodNode scMethod :
cn.getSuperClass().getMethods(publicMethod.getName())) {
+ if (scMethod.isFinal() && !scMethod.isPublic() &&
!scMethod.isPrivate() && !scMethod.isStatic()
+ &&
ParameterUtils.parametersEqual(scMethod.getParameters(),
publicMethod.getParameters())) {
+ 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 +568,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 +612,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 {
