Only check sibling config values to dedupe

Currently more config values are being kept than necessesary. For
example, given config values for a string resource:

Config: default     Value: "keep"
Config: ldrtl       Value: "dedupe"
Config: ldrtl-night Value: "dedupe"
Config: land        Value: "keep2"

The ldrtl-night config value will fail to be removed despite being
equivalent to the ldrtl value. This is because the value for the land
configuration is not equivalent to the ldrtl-night value. Instead of
checking that every compatible config value not related by dominance
should have quivalent values, only check sibling config values within
the dominator tree.

Bug: 137230022
Test: aapt2_tests
Change-Id: I965365d1a9433ae595eab48d82837ac102148334

2 files changed, 52 insertions, 4 deletions

diff --git a/tools/aapt2/optimize/ResourceDeduper.cpp b/tools/aapt2/optimize/ResourceDeduper.cpp
index 78ebcb97b811..0278b439cfae 100644
--- a/tools/aapt2/optimize/ResourceDeduper.cpp
+++ b/tools/aapt2/optimize/ResourceDeduper.cpp
@@ -63,13 +63,14 @@ class DominatedKeyValueRemover : public DominatorTree::BottomUpVisitor {
     // Compare compatible configs for this entry and ensure the values are
     // equivalent.
     const ConfigDescription& node_configuration = node_value->config;
-    for (const auto& sibling : entry_->values) {
-      if (!sibling->value) {
+    for (const auto& sibling : parent->children()) {
+      ResourceConfigValue* sibling_value = sibling->value();
+      if (!sibling_value->value) {
         // Sibling was already removed.
         continue;
       }
-      if (node_configuration.IsCompatibleWith(sibling->config) &&
-          !node_value->value->Equals(sibling->value.get())) {
+      if (node_configuration.IsCompatibleWith(sibling_value->config) &&
+          !node_value->value->Equals(sibling_value->value.get())) {
         // The configurations are compatible, but the value is
         // different, so we can't remove this value.
         return;
diff --git a/tools/aapt2/optimize/ResourceDeduper_test.cpp b/tools/aapt2/optimize/ResourceDeduper_test.cpp
index 2e098aec4f8d..048e318d2802 100644
--- a/tools/aapt2/optimize/ResourceDeduper_test.cpp
+++ b/tools/aapt2/optimize/ResourceDeduper_test.cpp
@@ -80,11 +80,58 @@ TEST(ResourceDeduperTest, DifferentValuesAreKept) {
           .Build();
 
   ASSERT_TRUE(ResourceDeduper().Consume(context.get(), table.get()));
+  EXPECT_THAT(table, HasValue("android:string/keep", default_config));
   EXPECT_THAT(table, HasValue("android:string/keep", ldrtl_config));
   EXPECT_THAT(table, HasValue("android:string/keep", ldrtl_v21_config));
   EXPECT_THAT(table, HasValue("android:string/keep", land_config));
 }
 
+TEST(ResourceDeduperTest, SameValuesAreDedupedIncompatibleSiblings) {
+  std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
+  const ConfigDescription default_config = {};
+  const ConfigDescription ldrtl_config = test::ParseConfigOrDie("ldrtl");
+  const ConfigDescription ldrtl_night_config = test::ParseConfigOrDie("ldrtl-night");
+  // Chosen because this configuration is not compatible with ldrtl-night.
+  const ConfigDescription ldrtl_notnight_config = test::ParseConfigOrDie("ldrtl-notnight");
+
+  std::unique_ptr<ResourceTable> table =
+      test::ResourceTableBuilder()
+          .AddString("android:string/keep", ResourceId{}, default_config, "keep")
+          .AddString("android:string/keep", ResourceId{}, ldrtl_config, "dedupe")
+          .AddString("android:string/keep", ResourceId{}, ldrtl_night_config, "dedupe")
+          .AddString("android:string/keep", ResourceId{}, ldrtl_notnight_config, "keep2")
+          .Build();
+
+  ASSERT_TRUE(ResourceDeduper().Consume(context.get(), table.get()));
+  EXPECT_THAT(table, HasValue("android:string/keep", default_config));
+  EXPECT_THAT(table, HasValue("android:string/keep", ldrtl_config));
+  EXPECT_THAT(table, Not(HasValue("android:string/keep", ldrtl_night_config)));
+  EXPECT_THAT(table, HasValue("android:string/keep", ldrtl_notnight_config));
+}
+
+TEST(ResourceDeduperTest, SameValuesAreDedupedCompatibleNonSiblings) {
+  std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
+  const ConfigDescription default_config = {};
+  const ConfigDescription ldrtl_config = test::ParseConfigOrDie("ldrtl");
+  const ConfigDescription ldrtl_night_config = test::ParseConfigOrDie("ldrtl-night");
+  // Chosen because this configuration is compatible with ldrtl.
+  const ConfigDescription land_config = test::ParseConfigOrDie("land");
+
+  std::unique_ptr<ResourceTable> table =
+      test::ResourceTableBuilder()
+          .AddString("android:string/keep", ResourceId{}, default_config, "keep")
+          .AddString("android:string/keep", ResourceId{}, ldrtl_config, "dedupe")
+          .AddString("android:string/keep", ResourceId{}, ldrtl_night_config, "dedupe")
+          .AddString("android:string/keep", ResourceId{}, land_config, "keep2")
+          .Build();
+
+  ASSERT_TRUE(ResourceDeduper().Consume(context.get(), table.get()));
+  EXPECT_THAT(table, HasValue("android:string/keep", default_config));
+  EXPECT_THAT(table, HasValue("android:string/keep", ldrtl_config));
+  EXPECT_THAT(table, Not(HasValue("android:string/keep", ldrtl_night_config)));
+  EXPECT_THAT(table, HasValue("android:string/keep", land_config));
+}
+
 TEST(ResourceDeduperTest, LocalesValuesAreKept) {
   std::unique_ptr<IAaptContext> context = test::ContextBuilder().Build();
   const ConfigDescription default_config = {};