Skip to content

Commit

Permalink
Sort class members to ensure deterministic builds
Browse files Browse the repository at this point in the history
Class methods and fields are currently sorted at serialization (see
DescriptorSerializer.sort) and at deserialization (see
DeserializedMemberScope.OptimizedImplementation#addMembers). Therefore,
the contents of the generated stub files are sorted in incremental
builds but not in clean builds.

The consequence is that the contents of the generated stub files may not
be consistent across a clean build and an incremental build, making the
build non-deterministic and dependent tasks run unnecessarily (see
KT-40882).

To work around that, this commit sorts class methods and fields when
outputting stub files.

Bug: KT-40882 (there are actually 2 issues in here; this commit fixes
     the first one)
Test: New DeterministicBuildIT + Updated existing test expectation files
  • Loading branch information
hungvietnguyen authored and nav-nav committed Nov 25, 2020
1 parent 07a797c commit 4bf63a9
Show file tree
Hide file tree
Showing 70 changed files with 1,119 additions and 1,016 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
/*
* Copyright 2010-2020 JetBrains s.r.o. and Kotlin Programming Language contributors.
* Use of this source code is governed by the Apache 2.0 license that can be found in the license/LICENSE.txt file.
*/

package org.jetbrains.kotlin.gradle

import org.jetbrains.kotlin.gradle.util.allJavaFiles
import org.junit.Test
import java.io.File
import kotlin.test.assertEquals

/** Tests that the outputs of a build are deterministic. */
class DeterministicBuildIT : BaseGradleIT() {

@Test
fun `test KaptGenerateStubsTask - KT-40882`() = with(
Project("simple", directoryPrefix = "kapt2")
) {
setupWorkingDir()
projectDir
.resolve("src/main/java/Foo.kt")
.writeText(
"""
class Foo : Bar {
// The fields and methods are ordered such that any sorting by KGP will be detected.
val fooField1 = 1
val fooField3 = 3
val fooField2 = 2
fun fooMethod1() {}
fun fooMethod3() {}
fun fooMethod2() {}
}
""".trimIndent()
)
projectDir
.resolve("src/main/java/Bar.kt")
.writeText(
"""
interface Bar {
val barField1 = 1
val barField3 = 3
val barField2 = 2
fun barMethod1() {}
fun barMethod3() {}
fun barMethod2() {}
}
""".trimIndent()
)

val buildAndSnapshotStubFiles: () -> Map<File, String> = {
lateinit var stubFiles: Map<File, String>
build(":kaptGenerateStubsKotlin") {
assertSuccessful()
assertTasksExecuted(":kaptGenerateStubsKotlin")
stubFiles = fileInWorkingDir("build/tmp/kapt3/stubs").allJavaFiles().map {
it to it.readText()
}.toMap()
}
stubFiles
}

// Run the first build
val stubFilesAfterFirstBuild = buildAndSnapshotStubFiles()

// Make a change
projectDir.resolve("src/main/java/Foo.kt").also {
it.writeText(
"""
class Foo : Bar {
val fooField1 = 1
val fooField3 = 3
val fooField2 = 2
fun fooMethod1() { println("Method body changed!") }
fun fooMethod3() {}
fun fooMethod2() {}
}
""".trimIndent()
)
}

// Run the second build
val stubFilesAfterSecondBuild = buildAndSnapshotStubFiles()

// Check that the build outputs are deterministic
assertEquals(stubFilesAfterFirstBuild.size, stubFilesAfterSecondBuild.size)
for (file in stubFilesAfterFirstBuild.keys) {
val fileContentsAfterFirstBuild = stubFilesAfterFirstBuild[file]
val fileContentsAfterSecondBuild = stubFilesAfterSecondBuild[file]
assertEquals(fileContentsAfterFirstBuild, fileContentsAfterSecondBuild)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -398,9 +398,9 @@ open class Kapt3IT : Kapt3BaseIT() {
val actual = getErrorMessages()
// try as 0 starting lines first, then as 1 starting line
try {
Assert.assertEquals(genJavaErrorString(8, 16), actual)
Assert.assertEquals(genJavaErrorString(8, 15), actual)
} catch (e: AssertionError) {
Assert.assertEquals(genJavaErrorString(9, 17), actual)
Assert.assertEquals(genJavaErrorString(9, 16), actual)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -387,11 +387,21 @@ class ClassFileToSourceStubConverter(val kaptContext: KaptContextForStubGenerati
)
}

val fields = mapJList<FieldNode, JCTree>(clazz.fields) {
// Class methods and fields are currently sorted at serialization (see DescriptorSerializer.sort) and at deserialization (see
// DeserializedMemberScope.OptimizedImplementation#addMembers). Therefore, the contents of the generated stub files are sorted in
// incremental builds but not in clean builds.
// The consequence is that the contents of the generated stub files may not be consistent across a clean build and an incremental
// build, making the build non-deterministic and dependent tasks run unnecessarily (see KT-40882).
// To work around that, we always sort class methods and fields when outputting stub files. Once we remove the sorting at both
// serialization and deserialization (KT-20980), we can remove this workaround.
val sortedFields = clazz.fields.toList().sortedWith(compareBy({ it.name }, { it.desc }))
val sortedMethods = clazz.methods.toList().sortedWith(compareBy({ it.name }, { it.desc }))

val fields = mapJList<FieldNode, JCTree>(sortedFields) {
if (it.isEnumValue()) null else convertField(it, clazz, lineMappings, packageFqName)
}

val methods = mapJList<MethodNode, JCTree>(clazz.methods) {
val methods = mapJList<MethodNode, JCTree>(sortedMethods) {
if (isEnum) {
if (it.name == "values" && it.desc == "()[L${clazz.name};") return@mapJList null
if (it.name == "valueOf" && it.desc == "(Ljava/lang/String;)L${clazz.name};") return@mapJList null
Expand Down
22 changes: 11 additions & 11 deletions plugins/kapt3/kapt3-compiler/testData/converter/abstractEnum.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ public enum E {
/*public static final*/ X /* = new E() */,
/*public static final*/ Y /* = new E() */;

E() {
}

public abstract void a();

public final void b() {
}

E() {
}

@kotlin.Metadata()
public static final class Obj {
@org.jetbrains.annotations.NotNull()
Expand Down Expand Up @@ -42,13 +42,13 @@ public enum E2 {
/*public static final*/ X /* = new E2() */,
/*public static final*/ Y /* = new E2() */;

public abstract void a();

E2(int n) {
}

E2(java.lang.String s) {
}

public abstract void a();
}

////////////////////
Expand All @@ -63,13 +63,13 @@ public enum E3 {
@org.jetbrains.annotations.NotNull()
private final java.lang.String a = null;

E3(java.lang.String a) {
}

@org.jetbrains.annotations.NotNull()
public final java.lang.String getA() {
return null;
}

E3(java.lang.String a) {
}
}

////////////////////
Expand All @@ -86,6 +86,9 @@ public enum E4 {
private final long c = 0L;
private final boolean d = false;

E4(java.lang.String a, int b, long c, boolean d) {
}

@org.jetbrains.annotations.NotNull()
public final java.lang.String getA() {
return null;
Expand All @@ -102,7 +105,4 @@ public enum E4 {
public final boolean getD() {
return false;
}

E4(java.lang.String a, int b, long c, boolean d) {
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,15 @@ import java.lang.System;
@kotlin.Metadata()
public abstract class Base {

public Base() {
super();
}

protected abstract void doJob(@org.jetbrains.annotations.NotNull()
java.lang.String job, int delay);

protected abstract <T extends java.lang.CharSequence>void doJobGeneric(@org.jetbrains.annotations.NotNull()
T job, int delay);

public Base() {
super();
}
}

////////////////////
Expand All @@ -22,6 +22,10 @@ import java.lang.System;
@kotlin.Metadata()
public final class Impl extends Base {

public Impl() {
super();
}

@java.lang.Override()
protected void doJob(@org.jetbrains.annotations.NotNull()
java.lang.String job, int delay) {
Expand All @@ -31,8 +35,4 @@ public final class Impl extends Base {
protected <T extends java.lang.CharSequence>void doJobGeneric(@org.jetbrains.annotations.NotNull()
T job, int delay) {
}

public Impl() {
super();
}
}
62 changes: 31 additions & 31 deletions plugins/kapt3/kapt3-compiler/testData/converter/aliasedImports.txt
Original file line number Diff line number Diff line change
Expand Up @@ -4,65 +4,69 @@ import a.b.ABC;

@kotlin.Metadata()
public final class Test {
public Test.MyDate date;
public java.util.concurrent.TimeUnit timeUnit;
public java.util.concurrent.TimeUnit microseconds;
public a.b.ABC abc;
public bcd bcd;
public Test.MyDate date;
public java.util.concurrent.TimeUnit microseconds;
public java.util.concurrent.TimeUnit timeUnit;

@org.jetbrains.annotations.NotNull()
public final Test.MyDate getDate() {
return null;
public Test() {
super();
}

public final void setDate(@org.jetbrains.annotations.NotNull()
Test.MyDate p0) {
@org.jetbrains.annotations.NotNull()
public final a.b.ABC getAbc() {
return null;
}

@org.jetbrains.annotations.NotNull()
public final java.util.concurrent.TimeUnit getTimeUnit() {
public final bcd getBcd() {
return null;
}

public final void setTimeUnit(@org.jetbrains.annotations.NotNull()
java.util.concurrent.TimeUnit p0) {
@org.jetbrains.annotations.NotNull()
public final Test.MyDate getDate() {
return null;
}

@org.jetbrains.annotations.NotNull()
public final java.util.concurrent.TimeUnit getMicroseconds() {
return null;
}

public final void setMicroseconds(@org.jetbrains.annotations.NotNull()
java.util.concurrent.TimeUnit p0) {
}

@org.jetbrains.annotations.NotNull()
public final a.b.ABC getAbc() {
public final java.util.concurrent.TimeUnit getTimeUnit() {
return null;
}

public final void setAbc(@org.jetbrains.annotations.NotNull()
a.b.ABC p0) {
}

@org.jetbrains.annotations.NotNull()
public final bcd getBcd() {
return null;
}

public final void setBcd(@org.jetbrains.annotations.NotNull()
bcd p0) {
}

public Test() {
super();
public final void setDate(@org.jetbrains.annotations.NotNull()
Test.MyDate p0) {
}

public final void setMicroseconds(@org.jetbrains.annotations.NotNull()
java.util.concurrent.TimeUnit p0) {
}

public final void setTimeUnit(@org.jetbrains.annotations.NotNull()
java.util.concurrent.TimeUnit p0) {
}

@kotlin.Metadata()
public static final class MyDate {
public Test.MyDate date2;

public MyDate() {
super();
}

@org.jetbrains.annotations.NotNull()
public final Test.MyDate getDate2() {
return null;
Expand All @@ -71,10 +75,6 @@ public final class Test {
public final void setDate2(@org.jetbrains.annotations.NotNull()
Test.MyDate p0) {
}

public MyDate() {
super();
}
}
}

Expand All @@ -89,6 +89,10 @@ import a.b.ABC;
public final class Test2 {
public java.util.Date date;

public Test2() {
super();
}

@org.jetbrains.annotations.NotNull()
public final java.util.Date getDate() {
return null;
Expand All @@ -97,8 +101,4 @@ public final class Test2 {
public final void setDate(@org.jetbrains.annotations.NotNull()
java.util.Date p0) {
}

public Test2() {
super();
}
}
Loading

0 comments on commit 4bf63a9

Please sign in to comment.