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/16 15:37:58 UTC
(groovy) branch GROOVY-8659 created (now 99a19ebe79)
This is an automated email from the ASF dual-hosted git repository.
emilles pushed a change to branch GROOVY-8659
in repository https://gitbox.apache.org/repos/asf/groovy.git
at 99a19ebe79 GROOVY-8659: warning for inability to generate property access method
This branch includes the following new commits:
new aab4136d61 new property access method when super's is private or package-private
new 99a19ebe79 GROOVY-8659: warning for inability to generate property access method
The 2 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.
(groovy) 02/02: GROOVY-8659: warning for inability to generate property access 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-8659
in repository https://gitbox.apache.org/repos/asf/groovy.git
commit 99a19ebe79666ee5ae74bdda58065a95546f12a4
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Tue Sep 16 10:37:36 2025 -0500
GROOVY-8659: warning for inability to generate property access method
---
.../org/codehaus/groovy/classgen/Verifier.java | 18 ++++-
src/test/groovy/groovy/PropertyTest.groovy | 90 ++++++++++++++++++++++
2 files changed, 105 insertions(+), 3 deletions(-)
diff --git a/src/main/java/org/codehaus/groovy/classgen/Verifier.java b/src/main/java/org/codehaus/groovy/classgen/Verifier.java
index beac6f0125..0a37b824de 100644
--- a/src/main/java/org/codehaus/groovy/classgen/Verifier.java
+++ b/src/main/java/org/codehaus/groovy/classgen/Verifier.java
@@ -86,6 +86,7 @@ import java.util.List;
import java.util.ListIterator;
import java.util.Map;
import java.util.Set;
+import java.util.function.Consumer;
import java.util.function.IntFunction;
import java.util.stream.Collectors;
@@ -827,14 +828,16 @@ public class Verifier implements GroovyClassVisitor, Opcodes {
if (getter == null && isPrimitiveBoolean(node.getType())) {
getter = classNode.getGetterMethod("is" + capitalize(name), !node.isStatic());
}
- if (methodNeedsReplacement(getter)) {
+ if (methodNeedsReplacement(getter,
+ (mn) -> cannotReplaceMethod(node, mn))) {
getterBlock = createGetterBlock(node, field);
}
}
Statement setterBlock = node.getSetterBlock();
if (setterBlock == null && !node.isPrivate() && !node.isFinal()) {
MethodNode setter = classNode.getSetterMethod(setterName, false); // atypical: allow setter with non-void return type
- if (methodNeedsReplacement(setter)) {
+ if (methodNeedsReplacement(setter,
+ (mn) -> cannotReplaceMethod(node, mn))) {
setterBlock = createSetterBlock(node, field);
}
}
@@ -859,13 +862,22 @@ public class Verifier implements GroovyClassVisitor, Opcodes {
}
}
- private boolean methodNeedsReplacement(final MethodNode mn) {
+ private void cannotReplaceMethod(final PropertyNode pn, final MethodNode mn) {
+ String message = MethodNodeUtils.methodDescriptor(mn, true);
+ message = message.substring(message.indexOf(' ') + 1); // strip return type
+ message = String.format("Property %s cannot override final method %s of class %s", pn.getName(), message, mn.getDeclaringClass().getName());
+
+ classNode.getModule().getContext().addWarning(message, pn);
+ }
+
+ private boolean methodNeedsReplacement(final MethodNode mn, final Consumer<MethodNode> cannotReplace) {
if (mn != null) {
ClassNode declaringClass = mn.getDeclaringClass();
// nothing to be done for method of current class
if (declaringClass == classNode) return false;
// cannot overwrite a non-private final method
if (mn.isFinal() && !mn.isPrivate() && (!mn.isPackageScope() || inSamePackage(declaringClass, classNode))) {
+ cannotReplace.accept(mn); // GROOVY-8659
return false;
}
}
diff --git a/src/test/groovy/groovy/PropertyTest.groovy b/src/test/groovy/groovy/PropertyTest.groovy
index 88c3493600..2a9db0a400 100644
--- a/src/test/groovy/groovy/PropertyTest.groovy
+++ b/src/test/groovy/groovy/PropertyTest.groovy
@@ -18,6 +18,9 @@
*/
package groovy
+import org.codehaus.groovy.control.CompilationUnit
+import org.codehaus.groovy.control.CompilerConfiguration
+import org.codehaus.groovy.control.messages.WarningMessage
import org.junit.jupiter.api.Test
import static groovy.test.GroovyAssert.assertScript
@@ -352,6 +355,93 @@ final class PropertyTest {
'''
}
+ // GROOVY-8659
+ @Test
+ void testPropertyCannotOverrideFinalGetter() {
+ File parentDir = File.createTempDir()
+ def config = new CompilerConfiguration()
+ config.targetDirectory = File.createTempDir()
+ config.warningLevel = WarningMessage.POSSIBLE_ERRORS
+ try {
+ def a = new File(parentDir, 'A.groovy')
+ a.write '''
+ abstract class A {
+ final String getFoo() { 'A' }
+ }
+ '''
+ def c = new File(parentDir, 'C.groovy')
+ c.write '''
+ class C extends A {
+ final String foo = 'C'
+ }
+ '''
+ def m = new File(parentDir, 'Main.groovy')
+ m.write '''
+ def pogo = new C()
+ assert pogo.foo == 'A'
+ assert pogo.getFoo() == 'A'
+ '''
+
+ def loader = new GroovyClassLoader(this.class.classLoader)
+ def cu = new CompilationUnit(config, null, loader)
+ cu.addSources(a, c, m)
+ cu.compile()
+
+ assert cu.errorCollector.warnings*.message == [
+ 'Property foo cannot override final method getFoo() of class A'
+ ]
+
+ loader.addClasspath(config.targetDirectory.absolutePath)
+ loader.loadClass('Main').main()
+ } finally {
+ parentDir.deleteDir()
+ config.targetDirectory.deleteDir()
+ }
+ }
+
+ // GROOVY-8659
+ @Test
+ void testPropertyCannotOverrideFinalSetter() {
+ File parentDir = File.createTempDir()
+ def config = new CompilerConfiguration()
+ config.targetDirectory = File.createTempDir()
+ config.warningLevel = WarningMessage.POSSIBLE_ERRORS
+ try {
+ def a = new File(parentDir, 'A.groovy')
+ a.write '''
+ abstract class A {
+ final void setFoo(String foo) { }
+ }
+ '''
+ def c = new File(parentDir, 'C.groovy')
+ c.write '''
+ class C extends A {
+ String foo
+ }
+ '''
+ def m = new File(parentDir, 'Main.groovy')
+ m.write '''
+ def pogo = new C(foo: 'C')
+ assert pogo.foo == null
+ '''
+
+ def loader = new GroovyClassLoader(this.class.classLoader)
+ def cu = new CompilationUnit(config, null, loader)
+ cu.addSources(a, c, m)
+ cu.compile()
+
+ assert cu.errorCollector.warnings*.message == [
+ 'Property foo cannot override final method setFoo(java.lang.String) of class A'
+ ]
+
+ loader.addClasspath(config.targetDirectory.absolutePath)
+ loader.loadClass('Main').main()
+ } finally {
+ parentDir.deleteDir()
+ config.targetDirectory.deleteDir()
+ }
+ }
+
@Test
void testPropertyWithOverrideGetterAndSetter() {
assertScript '''
(groovy) 01/02: new property access method when super's is private or package-private
Posted by em...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
emilles pushed a commit to branch GROOVY-8659
in repository https://gitbox.apache.org/repos/asf/groovy.git
commit aab4136d61873aaf53931a7acbe074ae858f072a
Author: Eric Milles <er...@thomsonreuters.com>
AuthorDate: Tue Sep 16 10:12:56 2025 -0500
new property access method when super's is private or package-private
---
.../org/codehaus/groovy/classgen/Verifier.java | 24 +++++++------
src/test/groovy/groovy/PropertyTest.groovy | 42 ++++++++++++++++++----
2 files changed, 49 insertions(+), 17 deletions(-)
diff --git a/src/main/java/org/codehaus/groovy/classgen/Verifier.java b/src/main/java/org/codehaus/groovy/classgen/Verifier.java
index a7b08f0310..beac6f0125 100644
--- a/src/main/java/org/codehaus/groovy/classgen/Verifier.java
+++ b/src/main/java/org/codehaus/groovy/classgen/Verifier.java
@@ -114,6 +114,7 @@ import static org.codehaus.groovy.ast.tools.GeneralUtils.ctorX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.declS;
import static org.codehaus.groovy.ast.tools.GeneralUtils.fieldX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.getInterfacesAndSuperInterfaces;
+import static org.codehaus.groovy.ast.tools.GeneralUtils.inSamePackage;
import static org.codehaus.groovy.ast.tools.GeneralUtils.localVarX;
import static org.codehaus.groovy.ast.tools.GeneralUtils.param;
import static org.codehaus.groovy.ast.tools.GeneralUtils.params;
@@ -809,16 +810,6 @@ public class Verifier implements GroovyClassVisitor, Opcodes {
public void visitField(final FieldNode node) {
}
- private boolean methodNeedsReplacement(final MethodNode m) {
- // no method found, we need to replace
- if (m == null) return true;
- // method is in current class, nothing to be done
- if (m.getDeclaringClass() == getClassNode()) return false;
- // do not overwrite final
- if (isFinal(m.getModifiers())) return false;
- return true;
- }
-
@Override
public void visitProperty(final PropertyNode node) {
String name = node.getName();
@@ -868,6 +859,19 @@ public class Verifier implements GroovyClassVisitor, Opcodes {
}
}
+ private boolean methodNeedsReplacement(final MethodNode mn) {
+ if (mn != null) {
+ ClassNode declaringClass = mn.getDeclaringClass();
+ // nothing to be done for method of current class
+ if (declaringClass == classNode) return false;
+ // cannot overwrite a non-private final method
+ if (mn.isFinal() && !mn.isPrivate() && (!mn.isPackageScope() || inSamePackage(declaringClass, classNode))) {
+ return false;
+ }
+ }
+ return true;
+ }
+
private void visitGetter(final PropertyNode node, final FieldNode field, final Statement getterBlock, final int getterModifiers, final String getterName) {
MethodNode getter = new MethodNode(getterName, getterModifiers, node.getType(), Parameter.EMPTY_ARRAY, ClassNode.EMPTY_ARRAY, getterBlock);
getter.setSynthetic(true);
diff --git a/src/test/groovy/groovy/PropertyTest.groovy b/src/test/groovy/groovy/PropertyTest.groovy
index 30f6653370..88c3493600 100644
--- a/src/test/groovy/groovy/PropertyTest.groovy
+++ b/src/test/groovy/groovy/PropertyTest.groovy
@@ -325,15 +325,42 @@ final class PropertyTest {
'''
}
+ @Test
+ void testPropertyOverridesGetterAndSetter2() {
+ assertScript '''
+ abstract class A {
+ private final String getFoo() { 'A' }
+ }
+ class C extends A {
+ final String foo = 'C'
+ }
+
+ assert new C().getFoo() == 'C'
+ assert C.getMethod('getFoo').declaringClass.name == 'C'
+ '''
+
+ assertScript '''
+ abstract class A {
+ private final void setFoo(String foo) { }
+ }
+ class C extends A {
+ String foo
+ }
+
+ assert new C(foo: 'C').getFoo() == 'C'
+ assert C.getMethod('setFoo', String).declaringClass.name == 'C'
+ '''
+ }
+
@Test
void testPropertyWithOverrideGetterAndSetter() {
assertScript '''
- abstract class Base {
+ abstract class A {
abstract String getName()
abstract void setName(String name)
}
- class A extends Base {
- private String name = 'AA'
+ class C extends A {
+ private String name = 'C'
@Override
String getName() {
@@ -344,10 +371,11 @@ final class PropertyTest {
this.name = name
}
}
- Base a = new A()
- assert 'AA' == a.name
- a.name = 'BB'
- assert 'BB' == a.name
+
+ A a = new C()
+ assert a.name == 'C'
+ a.name = 'X'
+ assert a.name == 'X'
'''
}