Ensure BR files have consistent output

This CL fixes an incrementality problem where we could've generated different BR
classes w/o any actual code change on the developer side.

I've moved usages of BR to a separate class to ensure ordering as multiple
things access it ~yay v1 compat~. This is still not perfect for caching because the
BR data uses a java serializable class w/ a regular map. We need to move it to
json as well to ensure caching is consistent too (thought this change should help
with incremental javac)

Bug: 131659806
Test: existing tests
Change-Id: I3412845f121798bb474fb503fe2bfdfa93034da1

6 files changed, 60 insertions, 35 deletions

diff --git a/compiler/src/main/java/android/databinding/annotationprocessor/BindableBag.kt b/compiler/src/main/java/android/databinding/annotationprocessor/BindableBag.kt
index 3615c2fd..b33f8ddb 100644
--- a/compiler/src/main/java/android/databinding/annotationprocessor/BindableBag.kt
+++ b/compiler/src/main/java/android/databinding/annotationprocessor/BindableBag.kt
@@ -43,13 +43,13 @@ class BindableBag(
         moduleProperties: Set<String>,
         private val env: ProcessingEnvironment) {
     // The BR fields we will generate.
-    val toBeGenerated: List<BRWithValues>
+    val toBeGenerated: List<ModuleBR>
     // The list of package names that are generated. We usually generate for all dependencies
     // as well as the current module but it may change in Features depending on where the dependency
     // is coming from.
     var writtenPackages: List<String>
     // A lookup file to map Field names into integer values.
-    val variableIdLookup: Map<String, Int>
+    val variableIdLookup: BRMapping
 
     // Information about the Feature module. Currently we only use the package offset id assigned
     // by gradle. That id allows us to avoid id conflicts between different features.
@@ -87,20 +87,22 @@ class BindableBag(
                 }
             }
         }
-        // now assign ids
-        brFiles.forEach {
-            it.properties.values.forEach {
-                if (it.value == null) {
-                    idBag.findId(it.name)
-                }
+        // now assign ids to the remaining ones.
+        // we sort them to keep ids as stable as possible between runs
+        val brsWithoutValues = brFiles.flatMap { packageProps ->
+            packageProps.properties.values.filter {
+                it.value == null
+            }.map {
+                it.name
             }
         }
+        idBag.assignIds(brsWithoutValues)
 
         writtenPackages = brPackagesToGenerate.toList()
-        val pairs = idBag.getLookup().map { Pair(it.key, it.value) }
+        val brMapping = idBag.buildMapping()
         // now assign id to all stuff we'll generate
         toBeGenerated = brPackagesToGenerate.map { brPkg ->
-            BRWithValues(brPkg, pairs)
+            ModuleBR(brPkg, brMapping)
             // WHEN we have BR per module, the code below should be executed instead
 //            val packageProps = brFiles.firstOrNull {
 //                it.pkg == brPkg
@@ -113,10 +115,10 @@ class BindableBag(
 //                val pairs = packageProps.properties.values.map {
 //                    Pair(it.name, it.value ?: idBag.findId(it.name))
 //                } + Pair("_all", 0)
-//                BRWithValues(brPkg, pairs)
+//                ModuleBR(brPkg, pairs)
 //            }
         }
-        variableIdLookup = idBag.getLookup()
+        variableIdLookup = brMapping
     }
 
     /**
@@ -206,9 +208,18 @@ class BindableBag(
     data class Property(val name: String, val value: Int?)
 
     /**
-     * Represents the metadata for a BR class with its ids already assigned.
+     * Represents the metadata for a specific module BR class with its ids already assigned.
      */
-    data class BRWithValues(val pkg: String, val props: List<Pair<String, Int>>)
+    data class ModuleBR(val pkg: String, val br: BRMapping)
+
+    /**
+     * Keeps the final mapping for a BR class.
+     * Props are ordered by key so that re-runs of the same code generates the same output.
+     */
+    data class BRMapping(val props: List<Pair<String, Int>>) {
+        val size
+            get() = props.size
+    }
 
     /**
      * helper class to generate ids and ensure that same key receives the same id.
@@ -227,7 +238,13 @@ class BindableBag(
             idMapping[key] = value
         }
 
-        fun findId(key: String): Int {
+        fun assignIds(keys : List<String>) {
+            keys.sorted().forEach {
+                findId(it)
+            }
+        }
+
+        private fun findId(key: String): Int {
             return idMapping.getOrPut(key) {
                 var i = newIdOffset
                 while (usedIds.contains(i)) {
@@ -238,6 +255,12 @@ class BindableBag(
             }
         }
 
-        fun getLookup() = idMapping
+        fun buildMapping() = BRMapping(
+                idMapping.map {
+                    Pair(it.key, it.value)
+                }.sortedBy {
+                    it.first
+                }
+        )
     }
 }
diff --git a/compiler/src/main/java/android/databinding/annotationprocessor/ProcessDataBinding.java b/compiler/src/main/java/android/databinding/annotationprocessor/ProcessDataBinding.java
index 33d7b58b..ad69ff03 100644
--- a/compiler/src/main/java/android/databinding/annotationprocessor/ProcessDataBinding.java
+++ b/compiler/src/main/java/android/databinding/annotationprocessor/ProcessDataBinding.java
@@ -39,7 +39,6 @@ import javax.lang.model.element.TypeElement;
 import javax.xml.bind.JAXBException;
 import java.util.Arrays;
 import java.util.List;
-import java.util.Map;
 import java.util.Set;
 import java.util.stream.Collectors;
 
@@ -141,7 +140,7 @@ public class ProcessDataBinding extends AbstractProcessor {
         Callback dataBinderWriterCallback = new Callback() {
             CompilerChef mChef;
             List<String> mModulePackages;
-            Map<String, Integer> mBRVariableLookup;
+            BindableBag.BRMapping mBRVariableLookup;
             boolean mWrittenMapper = false;
 
             @Override
@@ -169,9 +168,9 @@ public class ProcessDataBinding extends AbstractProcessor {
             }
 
             @Override
-            public void onBrWriterReady(Map<String, Integer> brLookup, List<String> brPackages) {
+            public void onBrWriterReady(BindableBag.BRMapping brWithValues, List<String> brPackages) {
                 Preconditions.checkNull(mBRVariableLookup, "Cannot set br writer twice");
-                mBRVariableLookup = brLookup;
+                mBRVariableLookup = brWithValues;
                 mModulePackages = brPackages;
                 considerWritingMapper();
             }
@@ -268,6 +267,6 @@ public class ProcessDataBinding extends AbstractProcessor {
 
     interface Callback {
         void onChefReady(CompilerChef chef, GenClassInfoLog classInfoLog);
-        void onBrWriterReady(Map<String, Integer> brWriter, List<String> brPackages);
+        void onBrWriterReady(BindableBag.BRMapping brWithValues, List<String> brPackages);
     }
 }
diff --git a/compiler/src/main/java/android/databinding/tool/CompilerChef.java b/compiler/src/main/java/android/databinding/tool/CompilerChef.java
index 23421604..83dd9b6e 100644
--- a/compiler/src/main/java/android/databinding/tool/CompilerChef.java
+++ b/compiler/src/main/java/android/databinding/tool/CompilerChef.java
@@ -13,6 +13,7 @@
 
 package android.databinding.tool;
 
+import android.databinding.annotationprocessor.BindableBag;
 import android.databinding.tool.processing.Scope;
 import android.databinding.tool.processing.ScopedException;
 import android.databinding.tool.reflection.InjectedClass;
@@ -186,7 +187,7 @@ public class CompilerChef {
     public void writeDataBinderMapper(
             ProcessingEnvironment processingEnv,
             CompilerArguments compilerArgs,
-            Map<String, Integer> brValueLookup,
+            BindableBag.BRMapping brValueLookup,
             List<String> modulePackages) {
         if (compilerArgs.isEnableV2()) {
             // figure out which mappers exists as they may not exist for v1 libs.
@@ -265,7 +266,7 @@ public class CompilerChef {
 
     private void writeMapperForV1Compat(
             CompilerArguments compilerArgs,
-            Map<String, Integer> brValueLookup) {
+            BindableBag.BRMapping brValueLookup) {
         LibTypes libTypes = ModelAnalyzer.getInstance().libTypes;
         BindingMapperWriter dbr = new BindingMapperWriter(
                 BindingMapperWriter.v1CompatMapperPkg(useAndroidX()),
@@ -326,7 +327,7 @@ public class CompilerChef {
      */
     private void writeMapperForModule(
             CompilerArguments compilerArgs,
-            Map<String, Integer> brValueLookup,
+            BindableBag.BRMapping brValueLookup,
             Set<String> availableDependencyModules) {
         GenClassInfoLog infoLog;
         try {
diff --git a/compiler/src/main/kotlin/android/databinding/tool/writer/BRWriter.kt b/compiler/src/main/kotlin/android/databinding/tool/writer/BRWriter.kt
index e6796aa0..f5beb20b 100644
--- a/compiler/src/main/kotlin/android/databinding/tool/writer/BRWriter.kt
+++ b/compiler/src/main/kotlin/android/databinding/tool/writer/BRWriter.kt
@@ -29,14 +29,14 @@ import com.squareup.javapoet.TypeSpec
 import javax.lang.model.element.Modifier
 
 class BRWriter(private val useFinal : Boolean) {
-    fun write(values : BindableBag.BRWithValues): String {
+    fun write(values : BindableBag.ModuleBR): String {
         val spec = TypeSpec.classBuilder("BR").apply {
             addModifiers(Modifier.PUBLIC)
             if (ModelAnalyzer.getInstance().hasGeneratedAnnotation) {
                 addAnnotation(AnnotationSpec.builder(ClassName.get("javax.annotation", "Generated"))
                         .addMember("value", S,"Android Data Binding").build())
             }
-            values.props.forEach {
+            values.br.props.forEach {
                 addField(
                         FieldSpec.builder(TypeName.INT, it.first, Modifier.PUBLIC,
                                 Modifier.STATIC).apply {
diff --git a/compiler/src/main/kotlin/android/databinding/tool/writer/BindingMapperWriter.kt b/compiler/src/main/kotlin/android/databinding/tool/writer/BindingMapperWriter.kt
index 8a01cda1..2392cd2d 100644
--- a/compiler/src/main/kotlin/android/databinding/tool/writer/BindingMapperWriter.kt
+++ b/compiler/src/main/kotlin/android/databinding/tool/writer/BindingMapperWriter.kt
@@ -13,6 +13,7 @@
 
 package android.databinding.tool.writer
 
+import android.databinding.annotationprocessor.BindableBag
 import android.databinding.tool.CompilerArguments
 import android.databinding.tool.LayoutBinder
 import android.databinding.tool.LibTypes
@@ -33,7 +34,7 @@ class BindingMapperWriter(
             className = "Test${className}"
         }
     }
-    fun write(brValueLookup: MutableMap<String, Int>) = kcode("") {
+    fun write(brValueLookup: BindableBag.BRMapping) = kcode("") {
         nl("package $pkg;")
         nl("import ${compilerArgs.modulePackage}.BR;")
         nl("import android.util.SparseArray;")
@@ -156,8 +157,8 @@ class BindingMapperWriter(
                 nl("static final SparseArray<String> sKeys = new SparseArray<>();")
                 block("static") {
                     tab("sKeys.put(0, \"_all\");")
-                    brValueLookup.forEach {
-                        tab("sKeys.put(${it.value}, \"${it.key}\");")
+                    brValueLookup.props.forEach {
+                        tab("sKeys.put(${it.second}, \"${it.first}\");")
                     }
                 }
             }
diff --git a/compiler/src/main/kotlin/android/databinding/tool/writer/BindingMapperWriterV2.kt b/compiler/src/main/kotlin/android/databinding/tool/writer/BindingMapperWriterV2.kt
index 52e2e8b4..346c03ac 100644
--- a/compiler/src/main/kotlin/android/databinding/tool/writer/BindingMapperWriterV2.kt
+++ b/compiler/src/main/kotlin/android/databinding/tool/writer/BindingMapperWriterV2.kt
@@ -16,6 +16,7 @@
 
 package android.databinding.tool.writer
 
+import android.databinding.annotationprocessor.BindableBag
 import android.databinding.tool.CompilerArguments
 import android.databinding.tool.LibTypes
 import android.databinding.tool.ext.L
@@ -157,7 +158,7 @@ class BindingMapperWriterV2(genClassInfoLog: GenClassInfoLog,
                                 val genClass: GenClassInfoLog.GenClass)
 
 
-    fun write(brValueLookup: MutableMap<String, Int>): TypeSpec
+    fun write(brValueLookup: BindableBag.BRMapping): TypeSpec
             = TypeSpec.classBuilder(className).apply {
         superclass(dataBinderMapper)
         addModifiers(Modifier.PUBLIC)
@@ -211,7 +212,7 @@ class BindingMapperWriterV2(genClassInfoLog: GenClassInfoLog,
         }
     }
 
-    private fun generateInnerBrLookup(brValueLookup: MutableMap<String, Int>) = TypeSpec
+    private fun generateInnerBrLookup(brValueLookup: BindableBag.BRMapping) = TypeSpec
             .classBuilder("InnerBrLookup").apply {
                 /**
                  * generated code looks like:
@@ -227,15 +228,15 @@ class BindingMapperWriterV2(genClassInfoLog: GenClassInfoLog,
                 )
                 val keysField = FieldSpec.builder(keysTypeName, "sKeys").apply {
                     addModifiers(Modifier.STATIC, Modifier.FINAL)
-                    initializer("new $T($L)", keysTypeName, brValueLookup.size + 1)
+                    initializer("new $T($L)", keysTypeName, brValueLookup.size)
                 }.build()
                 addField(keysField)
                 addStaticBlock(CodeBlock.builder().apply {
-                    brValueLookup.forEach {
+                    brValueLookup.props.forEach {
                         addStatement("$N.put($L, $S)",
                                 keysField,
-                                it.value,
-                                it.key)
+                                it.second,
+                                it.first)
                     }
                 }.build())
             }.build()