Skip to content

Commit 82c23f9

Browse files
jbartokjbartok
authored
Fix annotation parameter ordering problem in KTS (#38878)
2 parents 336e129 + ef11a72 commit 82c23f9

5 files changed

Lines changed: 269 additions & 165 deletions

File tree

‎platforms/core-configuration/java-api-extractor/src/main/java/org/gradle/internal/tools/api/impl/ApiMemberSelector.java‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,12 @@
2626
import org.objectweb.asm.Opcodes;
2727
import org.objectweb.asm.TypePath;
2828

29+
import java.util.LinkedHashSet;
30+
import java.util.Set;
2931
import java.util.SortedSet;
3032
import java.util.TreeSet;
3133

34+
import static org.objectweb.asm.Opcodes.ACC_ANNOTATION;
3235
import static org.objectweb.asm.Opcodes.ACC_FINAL;
3336
import static org.objectweb.asm.Opcodes.ACC_PRIVATE;
3437
import static org.objectweb.asm.Opcodes.ACC_PROTECTED;
@@ -45,7 +48,7 @@
4548
*/
4649
public class ApiMemberSelector extends ClassVisitor {
4750

48-
private final SortedSet<MethodMember> methods = new TreeSet<>();
51+
private Set<MethodMember> methods;
4952
private final SortedSet<FieldMember> fields = new TreeSet<>();
5053
private final SortedSet<InnerClassMember> innerClasses = new TreeSet<>();
5154

@@ -73,6 +76,12 @@ public void visit(int version, int access, String name, @Nullable String signatu
7376
super.visit(version, access, name, signature, superName, interfaces);
7477
classMember = new ClassMember(version, access, name, signature, superName, interfaces);
7578
isInnerClass = (access & ACC_SUPER) == ACC_SUPER;
79+
if ((access & ACC_ANNOTATION) == ACC_ANNOTATION) {
80+
// Kotlin binds positional annotation arguments by member declaration order
81+
methods = new LinkedHashSet<>();
82+
} else {
83+
methods = new TreeSet<>();
84+
}
7685
}
7786

7887
@Override

‎platforms/core-configuration/java-api-extractor/src/test/groovy/org/gradle/internal/tools/api/ApiClassExtractorAnnotationsTest.groovy‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,10 @@
1616

1717
package org.gradle.internal.tools.api
1818

19+
import org.objectweb.asm.ClassReader
20+
import org.objectweb.asm.ClassVisitor
21+
import org.objectweb.asm.MethodVisitor
22+
import org.objectweb.asm.Opcodes
1923
import spock.lang.Issue
2024

2125
import java.lang.annotation.ElementType
@@ -748,6 +752,38 @@ class ApiClassExtractorAnnotationsTest extends ApiClassExtractorTestSupport {
748752
consumer.classes.Main.clazz.name == "Main"
749753
}
750754

755+
@Issue("https://github.com/gradle/gradle/issues/38873")
756+
def "annotation members keep their declaration order"() {
757+
given:
758+
def api = toApi([
759+
Ann: '''
760+
public @interface Ann {
761+
String option() default "";
762+
String description();
763+
int alpha() default 0;
764+
}
765+
'''
766+
])
767+
768+
when:
769+
def extractedBytes = api.extractApiClassFrom(api.classes.Ann)
770+
771+
then:
772+
methodNamesInOrder(extractedBytes) == ['option', 'description', 'alpha']
773+
}
774+
775+
private static List<String> methodNamesInOrder(byte[] classBytes) {
776+
def names = []
777+
new ClassReader(classBytes).accept(new ClassVisitor(Opcodes.ASM9) {
778+
@Override
779+
MethodVisitor visitMethod(int access, String name, String descriptor, String signature, String[] exceptions) {
780+
names << name
781+
return null
782+
}
783+
}, 0)
784+
names
785+
}
786+
751787
private static Map<String, Method> mapMethods(Method[] methods) {
752788
methods.collectEntries { [it.name, it] }
753789
}

‎platforms/core-configuration/java-api-extractor/src/test/groovy/org/gradle/internal/tools/api/ApiClassExtractorTest.groovy‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -430,6 +430,37 @@ class ApiClassExtractorTest extends ApiClassExtractorTestSupport {
430430

431431
}
432432

433+
def "extracted class does not depend on the order in which members are declared"() {
434+
given: "the same API, with fields and methods declared in different order"
435+
def declaredInOneOrder = compileTo(new File(temporaryFolder, 'one'), ['com.acme.A': '''
436+
package com.acme;
437+
438+
public class A {
439+
public int b = 1;
440+
public int a = 2;
441+
public void bar() {}
442+
public void foo() {}
443+
}
444+
'''], [])
445+
def declaredInAnotherOrder = compileTo(new File(temporaryFolder, 'another'), ['com.acme.A': '''
446+
package com.acme;
447+
448+
public class A {
449+
public int a = 2;
450+
public int b = 1;
451+
public void foo() {}
452+
public void bar() {}
453+
}
454+
'''], [])
455+
456+
when:
457+
def one = declaredInOneOrder.extractApiClassFrom(declaredInOneOrder.classes['com.acme.A'])
458+
def another = declaredInAnotherOrder.extractApiClassFrom(declaredInAnotherOrder.classes['com.acme.A'])
459+
460+
then: "the extracted classes are byte-identical, so reordering members does not invalidate compile avoidance"
461+
one == another
462+
}
463+
433464
def "stubs should not contain any source or debug information"() {
434465
given:
435466
def api = toApi 'com.acme.A': '''

‎subprojects/core/src/integTest/groovy/org/gradle/api/tasks/options/AbstractOptionIntegrationSpec.groovy‎

Lines changed: 81 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,10 @@
1717
package org.gradle.api.tasks.options
1818

1919
import org.gradle.integtests.fixtures.AbstractIntegrationSpec
20+
import org.gradle.integtests.fixtures.polyglot.PolyglotTestFixture
21+
import org.gradle.test.fixtures.dsl.GradleDsl
2022

21-
abstract class AbstractOptionIntegrationSpec extends AbstractIntegrationSpec {
23+
abstract class AbstractOptionIntegrationSpec extends AbstractIntegrationSpec implements PolyglotTestFixture {
2224
String taskWithSingleOption(String optionType) {
2325
"""
2426
import org.gradle.api.DefaultTask;
@@ -98,6 +100,31 @@ abstract class AbstractOptionIntegrationSpec extends AbstractIntegrationSpec {
98100
"""
99101
}
100102

103+
String kotlinTaskWithSingleOption(String optionType) {
104+
String kotlinType = kotlinType(optionType)
105+
String initialValue = optionType == 'boolean' ? 'false' : 'null'
106+
"""
107+
abstract class SampleTask : DefaultTask() {
108+
@get:Internal
109+
@set:Option("myProp", "Configures command line option 'myProp'.")
110+
var myProp: $kotlinType = $initialValue
111+
112+
@TaskAction
113+
fun renderOptionValue() {
114+
println("Value of myProp: " + myProp)
115+
}
116+
117+
enum class TestEnum {
118+
OPT_1, OPT_2, OPT_3
119+
}
120+
}
121+
"""
122+
}
123+
124+
String buildScriptTaskWithSingleOption(String optionType) {
125+
currentDsl() == GradleDsl.KOTLIN ? kotlinTaskWithSingleOption(optionType) : groovyTaskWithSingleOption(optionType)
126+
}
127+
101128
String taskWithSinglePropertyOption(String propertyType, String optionType) {
102129
String methodName = propertyType.substring(0, 1).toLowerCase() + propertyType.substring(1)
103130

@@ -152,6 +179,29 @@ abstract class AbstractOptionIntegrationSpec extends AbstractIntegrationSpec {
152179
"""
153180
}
154181

182+
String kotlinTaskWithSinglePropertyOption(String propertyType, String optionType) {
183+
"""
184+
abstract class SampleTask : DefaultTask() {
185+
@get:Internal
186+
@get:Option("myProp", "Configures command line option 'myProp'.")
187+
abstract val myProp: $propertyType<${kotlinType(optionType, false)}>
188+
189+
@TaskAction
190+
fun renderOptionValue() {
191+
println("Value of myProp: " + myProp.orNull)
192+
}
193+
194+
enum class TestEnum {
195+
OPT_1, OPT_2, OPT_3
196+
}
197+
}
198+
"""
199+
}
200+
201+
String buildScriptTaskWithSinglePropertyOption(String propertyType, String optionType) {
202+
currentDsl() == GradleDsl.KOTLIN ? kotlinTaskWithSinglePropertyOption(propertyType, optionType) : groovyTaskWithSinglePropertyOption(propertyType, optionType)
203+
}
204+
155205
String taskWithUnparameterizedPropertyOption(String propertyType, String methodName) {
156206
"""
157207
import org.gradle.api.DefaultTask;
@@ -194,6 +244,36 @@ abstract class AbstractOptionIntegrationSpec extends AbstractIntegrationSpec {
194244
"""
195245
}
196246

247+
String kotlinTaskWithUnparameterizedPropertyOption(String propertyType) {
248+
"""
249+
abstract class SampleTask : DefaultTask() {
250+
@get:Internal
251+
@get:Option("myProp", "Configures command line option 'myProp'.")
252+
abstract val myProp: $propertyType
253+
254+
@TaskAction
255+
fun renderOptionValue() {
256+
println("Value of myProp: " + myProp.orNull)
257+
}
258+
}
259+
"""
260+
}
261+
262+
String buildScriptTaskWithUnparameterizedPropertyOption(String propertyType, String methodName) {
263+
currentDsl() == GradleDsl.KOTLIN ? kotlinTaskWithUnparameterizedPropertyOption(propertyType) : groovyTaskWithUnparameterizedPropertyOption(propertyType, methodName)
264+
}
265+
266+
/**
267+
* Maps a Java type as used in the Groovy fixtures to its Kotlin spelling.
268+
*/
269+
static String kotlinType(String javaType, boolean nullable = true) {
270+
String type = javaType.replace('Integer', 'Int').replace('Object', 'Any')
271+
if (javaType == 'boolean') {
272+
return 'Boolean'
273+
}
274+
nullable ? "$type?" : type
275+
}
276+
197277
String taskWithMultipleOptions() {
198278
"""
199279
import org.gradle.api.DefaultTask;

0 commit comments

Comments
 (0)