You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@groovy.apache.org by em...@apache.org on 2025/09/20 08:14:42 UTC
(groovy) branch GROOVY-11758 updated (b38d70a4d1 -> b8d9cc32a9)
This is an automated email from the ASF dual-hosted git repository.
emilles pushed a change to branch GROOVY-11758
in repository https://gitbox.apache.org/repos/asf/groovy.git
discard b38d70a4d1 GROOVY-11758: detect final super method that shadows interface method
new b8d9cc32a9 GROOVY-11758: detect final super method that shadows interface method
This update added new revisions after undoing existing revisions.
That is to say, some revisions that were in the old version of the
branch are not in the new version. This situation occurs
when a user --force pushes a change and generates a repository
containing something like this:
* -- * -- B -- O -- O -- O (b38d70a4d1)
\
N -- N -- N refs/heads/GROOVY-11758 (b8d9cc32a9)
You should already have received notification emails for all of the O
revisions, and so the following emails describe only the N revisions
from the common base, B.
Any revisions marked "omit" are not gone; other references still
refer to them. Any revisions marked "discard" are gone forever.
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
.../java/org/codehaus/groovy/classgen/ClassCompletionVerifier.java | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
(groovy) 01/01: GROOVY-11758: detect final super method that shadows interface method
Posted by em...@apache.org.
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 b8d9cc32a98789c1b5bbe50df49eda0db3ecfddc
Author: Eric Milles <er...@thomsonreuters.com>
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 {