[
https://issues.apache.org/jira/browse/GROOVY-12134?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18093943#comment-18093943
]
ASF GitHub Bot commented on GROOVY-12134:
-----------------------------------------
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.
> @Delegate regression: fails to skip a final method inherited from a
> precompiled superclass
> ------------------------------------------------------------------------------------------
>
> Key: GROOVY-12134
> URL: https://issues.apache.org/jira/browse/GROOVY-12134
> Project: Groovy
> Issue Type: Bug
> Affects Versions: 5.x
> Reporter: James Daugherty
> Priority: Major
>
> I'm upgrading the Grails Spring Security plugin to Groovy 5 and noticed
> compilation was failing with an `@Delegate` involving a final method.
>
> Reproducer project is here:
> [https://github.com/jdaugherty/groovy-final-delegate-bug]
>
> As a work around, ignoring the method manually makes compile succeed.
> h1.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
