Copilot commented on code in PR #2670:
URL: https://github.com/apache/groovy/pull/2670#discussion_r3527599451
##########
src/main/java/org/codehaus/groovy/transform/DelegateASTTransformation.java:
##########
@@ -424,7 +424,10 @@ private void addDelegateMethod(final MethodNode candidate,
final DelegateDescrip
break;
}
}
- if (existingNode == null || existingNode.getCode() == null) {
+ // GROOVY-12134: a non-abstract method from a pre-compiled class is an
+ // implementation despite having no code; only a primary class node can
+ // supply a bodiless placeholder that should be overwritten
+ if (existingNode == null || (existingNode.getCode() == null &&
existingNode.getDeclaringClass().isPrimaryClassNode())) {
Review Comment:
This condition will also prevent generating delegate methods that are
intended to implement *abstract* methods inherited from non-primary (e.g.,
precompiled) super types, since abstract methods also commonly have `getCode()
== null` and `isPrimaryClassNode() == false`. Consider explicitly allowing
overwrite when `existingNode.isAbstract()` (or equivalent check) so delegation
can still provide concrete implementations for abstract inherited signatures.
##########
src/test/groovy/org/codehaus/groovy/transform/DelegateTransformTest.groovy:
##########
@@ -69,22 +70,74 @@ final class DelegateTransformTest {
// GROOVY-10439
@Test
void testDelegateImplementingInterfaceWithDifferentTypeArgumentThanOwner()
{
- def err = shouldFail '''
+ assertScript '''
class C extends ArrayList<String> {
@Delegate List<Number> numbers // List<String> takes precedence
}
new C(numbers:[1,2,3])
'''
- assert err.message =~ /The return type of java.lang.Number get\(int\)
in C is incompatible with java.lang.String in java.util.ArrayList/
- err = shouldFail '''
+ def err = shouldFail '''
class C extends ArrayList<String> {
@Delegate HashSet<Number> numbers // Set<Number> added;
Verifier checks
}
'''
assert err.message =~ /The interface Collection cannot be implemented
more than once with different arguments: java.util.Collection<java.lang.Number>
\(via Set\) and java.util.Collection<java.lang.String> \(via ArrayList\)/
}
+ // GROOVY-12134
+ @Test
+ void testDelegateSkipsMethodsInheritedFromPrecompiledSuperclass() {
+ // final methods format(Object) and
format(Object,StringBuffer,FieldPosition)
+ // are inherited by C from binary (pre-compiled) super types
+ assertScript '''
+ import java.text.SimpleDateFormat
+ class C extends SimpleDateFormat {
+ @Delegate SimpleDateFormat delegate = new
SimpleDateFormat('yyyy')
+ }
+ new C()
+ '''
+
+ // same shape with Groovy-compiled classes loaded from the classpath
+ def config = new CompilerConfiguration(targetDirectory:
File.createTempDir())
+ def parentDir = File.createTempDir()
+ try {
+ def a = new File(parentDir, 'Sup.groovy')
+ a.write '''
+ class Sup {
+ final String describe() { 'sup' }
+ }
+ '''
+ def b = new File(parentDir, 'Base.groovy')
+ b.write '''
+ class Base extends Sup {
+ String hello() { 'hello' }
+ }
+ '''
+ def unit = new CompilationUnit(config)
+ unit.addSources(a, b)
+ unit.compile()
+
+ def loader = new GroovyClassLoader(this.class.classLoader)
+ loader.addClasspath(config.targetDirectory.path)
+ def result = new GroovyShell(loader).evaluate '''
Review Comment:
`GroovyClassLoader` is `Closeable` and can hold onto resources/classes
across test runs. To avoid test-suite resource leakage (especially in
long-running/parallel builds), close the classloader in a `finally` block or
use a try-with-resources pattern with `GroovyClassLoader`.
##########
src/test/groovy/org/codehaus/groovy/transform/DelegateTransformTest.groovy:
##########
@@ -69,22 +70,74 @@ final class DelegateTransformTest {
// GROOVY-10439
@Test
void testDelegateImplementingInterfaceWithDifferentTypeArgumentThanOwner()
{
- def err = shouldFail '''
+ assertScript '''
class C extends ArrayList<String> {
@Delegate List<Number> numbers // List<String> takes precedence
}
new C(numbers:[1,2,3])
'''
- assert err.message =~ /The return type of java.lang.Number get\(int\)
in C is incompatible with java.lang.String in java.util.ArrayList/
- err = shouldFail '''
+ def err = shouldFail '''
class C extends ArrayList<String> {
@Delegate HashSet<Number> numbers // Set<Number> added;
Verifier checks
}
'''
assert err.message =~ /The interface Collection cannot be implemented
more than once with different arguments: java.util.Collection<java.lang.Number>
\(via Set\) and java.util.Collection<java.lang.String> \(via ArrayList\)/
}
+ // GROOVY-12134
+ @Test
+ void testDelegateSkipsMethodsInheritedFromPrecompiledSuperclass() {
+ // final methods format(Object) and
format(Object,StringBuffer,FieldPosition)
+ // are inherited by C from binary (pre-compiled) super types
+ assertScript '''
+ import java.text.SimpleDateFormat
+ class C extends SimpleDateFormat {
+ @Delegate SimpleDateFormat delegate = new
SimpleDateFormat('yyyy')
+ }
+ new C()
+ '''
+
+ // same shape with Groovy-compiled classes loaded from the classpath
+ def config = new CompilerConfiguration(targetDirectory:
File.createTempDir())
+ def parentDir = File.createTempDir()
+ try {
+ def a = new File(parentDir, 'Sup.groovy')
+ a.write '''
+ class Sup {
+ final String describe() { 'sup' }
+ }
+ '''
+ def b = new File(parentDir, 'Base.groovy')
+ b.write '''
+ class Base extends Sup {
Review Comment:
The variables `a` and `b` are not descriptive in this multi-step setup
(writing sources, compiling, then loading from classpath). Renaming them to
something like `supSource`/`baseSource` (or similar) would make the test easier
to follow and maintain.
##########
src/main/java/org/codehaus/groovy/transform/DelegateASTTransformation.java:
##########
@@ -424,7 +424,10 @@ private void addDelegateMethod(final MethodNode candidate,
final DelegateDescrip
break;
}
}
- if (existingNode == null || existingNode.getCode() == null) {
+ // GROOVY-12134: a non-abstract method from a pre-compiled class is an
+ // implementation despite having no code; only a primary class node can
+ // supply a bodiless placeholder that should be overwritten
+ if (existingNode == null || (existingNode.getCode() == null &&
existingNode.getDeclaringClass().isPrimaryClassNode())) {
Review Comment:
The new overwrite rule is covered for inherited *final* methods, but it
doesn’t appear to cover the flip side: inherited *abstract* methods from a
precompiled superclass that should be implemented via `@Delegate`. Adding a
regression test for an abstract method inherited from a classpath-loaded
(precompiled) superclass would help ensure the new condition doesn’t block
valid delegate method generation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
