Fix for combining resources so that the transitive, direct, and primary resources are combined for the primary writing.

Example:
transitive resource: styleable/RubADubDub has an attr of butcher
direct resource: styleable/RubADubDub has an attr of baker
primary resource: styleable/RubADubDub has an attr of candlestickmaker

The merged resources should have:
transitive resource: <no matching key>
primary resource: styleable/RubADubDub has attrs of butcher, baker, candlestickmaker

--
MOS_MIGRATED_REVID=122453026

2 files changed, 72 insertions, 42 deletions

diff --git a/src/tools/android/java/com/google/devtools/build/android/AndroidDataMerger.java b/src/tools/android/java/com/google/devtools/build/android/AndroidDataMerger.java
index 3c2cde8666..32c9a23155 100644
--- a/src/tools/android/java/com/google/devtools/build/android/AndroidDataMerger.java
+++ b/src/tools/android/java/com/google/devtools/build/android/AndroidDataMerger.java
@@ -15,8 +15,6 @@ package com.google.devtools.build.android;
 
 import com.google.common.base.Joiner;
 import com.google.common.base.Stopwatch;
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.common.util.concurrent.ListeningExecutorService;
@@ -33,10 +31,10 @@ import java.nio.file.Files;
 import java.nio.file.Path;
 import java.nio.file.attribute.BasicFileAttributeView;
 import java.util.ArrayList;
-import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 import java.util.Set;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutorService;
@@ -244,7 +242,7 @@ public class AndroidDataMerger {
    * The UnwrittenMergedAndroidData contains only one of each DataKey in both the direct and
    * transitive closure.
    *
-   * The merge semantics are as follows: <pre>
+   * The merge semantics for overwriting resources (non id and styleable) are as follows: <pre>
    *   Key:
    *     A(): package A
    *     A(foo): package A with resource symbol foo
@@ -275,7 +273,13 @@ public class AndroidDataMerger {
    *     A(foo),B(foo) -> C() -> D(foo) == Valid
    *     A() -> B(foo),C(foo) -> D(foo) == Valid
    * </pre>
-   *
+   * <p>
+   * Combining resources are much simpler -- since a combining (id and styleable) resource does not
+   * get replaced when redefined, they are simply combined: <pre>
+   *     A(foo) -> B(foo) -> C(foo) == Valid
+   *     
+   * </pre>
+   * 
    * @param transitive The transitive dependencies to merge.
    * @param direct The direct dependencies to merge.
    * @param primaryData The primary data to merge against.
@@ -295,75 +299,108 @@ public class AndroidDataMerger {
 
     try {
       // Extract the primary resources.
-      ParsedAndroidData primary = ParsedAndroidData.from(primaryData);
+      ParsedAndroidData parsedPrimary = ParsedAndroidData.from(primaryData);
 
-      Map<DataKey, DataResource> overwritableDeps = new HashMap<>();
-      Map<DataKey, DataAsset> assets = new HashMap<>();
+      // Create the builders for the final parsed data.
+      final ParsedAndroidData.Builder primaryBuilder = ParsedAndroidData.Builder.newBuilder();
+      final ParsedAndroidData.Builder transitiveBuilder = ParsedAndroidData.Builder.newBuilder();
+      final KeyValueConsumers transitiveConsumers = transitiveBuilder.consumers();
+      final KeyValueConsumers primaryConsumers = primaryBuilder.consumers();
 
-      Set<MergeConflict> conflicts = new HashSet<>();
-      conflicts.addAll(primary.conflicts());
+      
+      final Set<MergeConflict> conflicts = new HashSet<>();
+      conflicts.addAll(parsedPrimary.conflicts());
       for (MergeConflict conflict : Iterables.concat(direct.conflicts(), transitive.conflicts())) {
         if (allowPrimaryOverrideAll
-            && (primary.containsOverwritable(conflict.dataKey())
-                || primary.containsAsset(conflict.dataKey()))) {
+            && (parsedPrimary.containsOverwritable(conflict.dataKey())
+                || parsedPrimary.containsAsset(conflict.dataKey()))) {
           continue;
         }
         conflicts.add(conflict);
       }
 
-      // resources
+      // overwriting resources
+      for (Entry<DataKey, DataResource> entry : parsedPrimary.iterateOverwritableEntries()) {
+        primaryConsumers.overwritingConsumer.consume(entry.getKey(), entry.getValue());
+      }
+
       for (Map.Entry<DataKey, DataResource> entry : direct.iterateOverwritableEntries()) {
         // Direct dependencies are simply overwritten, no conflict.
-        if (!primary.containsOverwritable(entry.getKey())) {
-          overwritableDeps.put(entry.getKey(), entry.getValue());
+        if (!parsedPrimary.containsOverwritable(entry.getKey())) {
+          transitiveConsumers.overwritingConsumer.consume(entry.getKey(), entry.getValue());
         }
       }
       for (Map.Entry<DataKey, DataResource> entry : transitive.iterateOverwritableEntries()) {
         // If the primary is considered to be intentional (usually at the binary level),
         // skip.
-        if (primary.containsOverwritable(entry.getKey()) && allowPrimaryOverrideAll) {
+        if (allowPrimaryOverrideAll && parsedPrimary.containsOverwritable(entry.getKey())) {
           continue;
         }
         // If a transitive value is in the direct map report a conflict, as it is commonly
         // unintentional.
         if (direct.containsOverwritable(entry.getKey())) {
           conflicts.add(direct.foundResourceConflict(entry.getKey(), entry.getValue()));
-        } else if (primary.containsOverwritable(entry.getKey())) {
+        } else if (parsedPrimary.containsOverwritable(entry.getKey())) {
           // If overwriting a transitive value with a primary map, assume it's an unintentional
           // override, unless allowPrimaryOverrideAll is set. At which point, this code path
           // should not be reached.
-          conflicts.add(primary.foundResourceConflict(entry.getKey(), entry.getValue()));
+          conflicts.add(parsedPrimary.foundResourceConflict(entry.getKey(), entry.getValue()));
         } else {
           // If it's in none of the of sources, add it.
-          overwritableDeps.put(entry.getKey(), entry.getValue());
+          transitiveConsumers.overwritingConsumer.consume(entry.getKey(), entry.getValue());
+        }
+      }
+
+      // combining resources
+      for (Entry<DataKey, DataResource> entry : parsedPrimary.iterateCombiningEntries()) {
+        primaryConsumers.combiningConsumer.consume(entry.getKey(), entry.getValue());
+      }
+      for (Map.Entry<DataKey, DataResource> entry : direct.iterateCombiningEntries()) {
+        if (parsedPrimary.containsCombineable(entry.getKey())) {
+          // If it is in the primary, add it to the primary to be combined.
+          primaryConsumers.combiningConsumer.consume(entry.getKey(), entry.getValue());
+        } else {
+          // If the combining asset is not in the primary, put it into the transitive.
+          transitiveConsumers.combiningConsumer.consume(entry.getKey(), entry.getValue());
+        }
+      }
+      for (Map.Entry<DataKey, DataResource> entry : transitive.iterateCombiningEntries()) {
+        if (parsedPrimary.containsCombineable(entry.getKey())) {
+          primaryConsumers.combiningConsumer.consume(entry.getKey(), entry.getValue());
+        } else {
+          transitiveConsumers.combiningConsumer.consume(entry.getKey(), entry.getValue());
         }
       }
 
       // assets
+      for (Entry<DataKey, DataAsset> entry : parsedPrimary.iterateAssetEntries()) {
+        primaryConsumers.assetConsumer.consume(entry.getKey(), entry.getValue());
+      }
+
       for (Map.Entry<DataKey, DataAsset> entry : direct.iterateAssetEntries()) {
         // Direct dependencies are simply overwritten, no conflict.
-        if (!primary.containsAsset(entry.getKey())) {
-          assets.put(entry.getKey(), entry.getValue());
+        if (!parsedPrimary.containsAsset(entry.getKey())) {
+          transitiveConsumers.assetConsumer.consume(entry.getKey(), entry.getValue());
         }
       }
       for (Map.Entry<DataKey, DataAsset> entry : transitive.iterateAssetEntries()) {
         // If the primary is considered to be intentional (usually at the binary level),
         // skip.
-        if (primary.containsAsset(entry.getKey()) && allowPrimaryOverrideAll) {
+        if (allowPrimaryOverrideAll && parsedPrimary.containsAsset(entry.getKey())) {
           continue;
         }
         // If a transitive value is in the direct map report a conflict, as it is commonly
         // unintentional.
         if (direct.containsAsset(entry.getKey())) {
           conflicts.add(direct.foundAssetConflict(entry.getKey(), entry.getValue()));
-        } else if (primary.containsAsset(entry.getKey())) {
+        } else if (parsedPrimary.containsAsset(entry.getKey())) {
           // If overwriting a transitive value with a primary map, assume it's an unintentional
           // override, unless allowPrimaryOverrideAll is set. At which point, this code path
           // should not be reached.
-          conflicts.add(primary.foundAssetConflict(entry.getKey(), entry.getValue()));
+          conflicts.add(parsedPrimary.foundAssetConflict(entry.getKey(), entry.getValue()));
         } else {
           // If it's in none of the of sources, add it.
-          assets.put(entry.getKey(), entry.getValue());
+          transitiveConsumers.assetConsumer.consume(entry.getKey(), entry.getValue());
         }
       }
 
@@ -384,12 +421,8 @@ public class AndroidDataMerger {
 
       return UnwrittenMergedAndroidData.of(
           primaryData.getManifest(),
-          primary,
-          ParsedAndroidData.of(
-              ImmutableSet.<MergeConflict>of(),
-              ImmutableMap.copyOf(overwritableDeps),
-              direct.mergeCombining(transitive),
-              ImmutableMap.copyOf(assets)));
+          primaryBuilder.build(),
+          transitiveBuilder.build());
     } catch (IOException e) {
       throw new MergingException(e);
     }
diff --git a/src/tools/android/java/com/google/devtools/build/android/ParsedAndroidData.java b/src/tools/android/java/com/google/devtools/build/android/ParsedAndroidData.java
index 3e13e61ad3..0ec40d8aa3 100644
--- a/src/tools/android/java/com/google/devtools/build/android/ParsedAndroidData.java
+++ b/src/tools/android/java/com/google/devtools/build/android/ParsedAndroidData.java
@@ -30,7 +30,6 @@ import java.nio.file.Path;
 import java.nio.file.SimpleFileVisitor;
 import java.nio.file.attribute.BasicFileAttributes;
 import java.util.ArrayList;
-import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -447,6 +446,10 @@ public class ParsedAndroidData {
     return overwritingResources.containsKey(name);
   }
 
+  public boolean containsCombineable(DataKey key) {
+    return combiningResources.containsKey(key);
+  }
+
   Iterable<Entry<DataKey, DataResource>> iterateOverwritableEntries() {
     return overwritingResources.entrySet();
   }
@@ -455,6 +458,10 @@ public class ParsedAndroidData {
     return Iterables.concat(overwritingResources.entrySet(), combiningResources.entrySet());
   }
 
+  public Iterable<Entry<DataKey, DataResource>> iterateCombiningEntries() {
+    return combiningResources.entrySet();
+  }
+
   boolean containsAsset(DataKey name) {
     return assets.containsKey(name);
   }
@@ -471,16 +478,6 @@ public class ParsedAndroidData {
     return MergeConflict.between(key, assets.get(key), value);
   }
 
-  ImmutableMap<DataKey, DataResource> mergeCombining(ParsedAndroidData other) {
-    Map<DataKey, DataResource> merged = new HashMap<>();
-    CombiningConsumer consumer = new CombiningConsumer(merged);
-    for (Entry<DataKey, DataResource> entry :
-        Iterables.concat(
-            combiningResources.entrySet(), other.combiningResources.entrySet())) {
-      consumer.consume(entry.getKey(), entry.getValue());
-    }
-    return ImmutableMap.copyOf(merged);
-  }
 
   ImmutableSet<MergeConflict> conflicts() {
     return conflicts;