AAPT2: Fix R.java styleable + indices ordering

Make sure that Styleables are directly followed by their indices.
If not, Robolectric breaks. This is not strictly incorrect to have
an arbitrary ordering in R.java, but its easier to just support
Robolectric in this case.

Bug: 65837293
Test: make aapt2_tests

(cherry picked from commit af85c4deb667843a227d62275fe6992005f4c38d)

Change-Id: Ia59ba58427ade386d075ca9fc9eb5b53e35beca0

3 files changed, 86 insertions, 33 deletions

diff --git a/tools/aapt2/java/ClassDefinition.cpp b/tools/aapt2/java/ClassDefinition.cpp
index 6ad0dd68cb6a..0c57e7e06128 100644
--- a/tools/aapt2/java/ClassDefinition.cpp
+++ b/tools/aapt2/java/ClassDefinition.cpp
@@ -41,18 +41,21 @@ void MethodDefinition::WriteToStream(const StringPiece& prefix, bool final,
 
 ClassDefinition::Result ClassDefinition::AddMember(std::unique_ptr<ClassMember> member) {
   Result result = Result::kAdded;
-  auto iter = members_.find(member);
-  if (iter != members_.end()) {
-    members_.erase(iter);
+  auto iter = indexed_members_.find(member->GetName());
+  if (iter != indexed_members_.end()) {
+    // Overwrite the entry.
+    ordered_members_[iter->second].reset();
     result = Result::kOverridden;
   }
-  members_.insert(std::move(member));
+
+  indexed_members_[member->GetName()] = ordered_members_.size();
+  ordered_members_.push_back(std::move(member));
   return result;
 }
 
 bool ClassDefinition::empty() const {
-  for (const std::unique_ptr<ClassMember>& member : members_) {
-    if (!member->empty()) {
+  for (const std::unique_ptr<ClassMember>& member : ordered_members_) {
+    if (member != nullptr && !member->empty()) {
       return false;
     }
   }
@@ -61,7 +64,7 @@ bool ClassDefinition::empty() const {
 
 void ClassDefinition::WriteToStream(const StringPiece& prefix, bool final,
                                     std::ostream* out) const {
-  if (members_.empty() && !create_if_empty_) {
+  if (empty() && !create_if_empty_) {
     return;
   }
 
@@ -76,9 +79,14 @@ void ClassDefinition::WriteToStream(const StringPiece& prefix, bool final,
   std::string new_prefix = prefix.to_string();
   new_prefix.append(kIndent);
 
-  for (const std::unique_ptr<ClassMember>& member : members_) {
-    member->WriteToStream(new_prefix, final, out);
-    *out << "\n";
+  for (const std::unique_ptr<ClassMember>& member : ordered_members_) {
+    // There can be nullptr members when a member is added to the ClassDefinition
+    // and takes precedence over a previous member with the same name. The overridden member is
+    // set to nullptr.
+    if (member != nullptr) {
+      member->WriteToStream(new_prefix, final, out);
+      *out << "\n";
+    }
   }
 
   *out << prefix << "}";
diff --git a/tools/aapt2/java/ClassDefinition.h b/tools/aapt2/java/ClassDefinition.h
index 6c4bcad83d2a..28a3489e71a4 100644
--- a/tools/aapt2/java/ClassDefinition.h
+++ b/tools/aapt2/java/ClassDefinition.h
@@ -18,8 +18,9 @@
 #define AAPT_JAVA_CLASSDEFINITION_H
 
 #include <ostream>
-#include <set>
 #include <string>
+#include <unordered_map>
+#include <vector>
 
 #include "android-base/macros.h"
 #include "androidfw/StringPiece.h"
@@ -73,21 +74,18 @@ class PrimitiveMember : public ClassMember {
   void WriteToStream(const android::StringPiece& prefix, bool final,
                      std::ostream* out) const override {
     ClassMember::WriteToStream(prefix, final, out);
-
-    *out << prefix << "public static " << (final ? "final " : "") << "int "
-         << name_ << "=" << val_ << ";";
+    *out << prefix << "public static " << (final ? "final " : "") << "int " << name_ << "=" << val_
+         << ";";
   }
 
  private:
+  DISALLOW_COPY_AND_ASSIGN(PrimitiveMember);
+
   std::string name_;
   T val_;
-
-  DISALLOW_COPY_AND_ASSIGN(PrimitiveMember);
 };
 
-/**
- * Specialization for strings so they get the right type and are quoted with "".
- */
+// Specialization for strings so they get the right type and are quoted with "".
 template <>
 class PrimitiveMember<std::string> : public ClassMember {
  public:
@@ -111,10 +109,10 @@ class PrimitiveMember<std::string> : public ClassMember {
   }
 
  private:
+  DISALLOW_COPY_AND_ASSIGN(PrimitiveMember);
+
   std::string name_;
   std::string val_;
-
-  DISALLOW_COPY_AND_ASSIGN(PrimitiveMember);
 };
 
 using IntMember = PrimitiveMember<uint32_t>;
@@ -160,10 +158,10 @@ class PrimitiveArrayMember : public ClassMember {
   }
 
  private:
+  DISALLOW_COPY_AND_ASSIGN(PrimitiveArrayMember);
+
   std::string name_;
   std::vector<T> elements_;
-
-  DISALLOW_COPY_AND_ASSIGN(PrimitiveArrayMember);
 };
 
 using ResourceArrayMember = PrimitiveArrayMember<ResourceId>;
@@ -185,12 +183,16 @@ class MethodDefinition : public ClassMember {
   }
 
   // Even if the method is empty, we always want to write the method signature.
-  bool empty() const override { return false; }
+  bool empty() const override {
+    return false;
+  }
 
   void WriteToStream(const android::StringPiece& prefix, bool final,
                      std::ostream* out) const override;
 
  private:
+  DISALLOW_COPY_AND_ASSIGN(MethodDefinition);
+
   std::string signature_;
   std::vector<std::string> statements_;
 };
@@ -222,19 +224,13 @@ class ClassDefinition : public ClassMember {
                      std::ostream* out) const override;
 
  private:
-  struct ClassMemberCompare {
-    using T = std::unique_ptr<ClassMember>;
-    bool operator()(const T& a, const T& b) const {
-      return a->GetName() < b->GetName();
-    }
-  };
+  DISALLOW_COPY_AND_ASSIGN(ClassDefinition);
 
   std::string name_;
   ClassQualifier qualifier_;
   bool create_if_empty_;
-  std::set<std::unique_ptr<ClassMember>, ClassMemberCompare> members_;
-
-  DISALLOW_COPY_AND_ASSIGN(ClassDefinition);
+  std::vector<std::unique_ptr<ClassMember>> ordered_members_;
+  std::unordered_map<android::StringPiece, size_t> indexed_members_;
 };
 
 }  // namespace aapt
diff --git a/tools/aapt2/java/JavaClassGenerator_test.cpp b/tools/aapt2/java/JavaClassGenerator_test.cpp
index 4f449f0db41a..668e4340e839 100644
--- a/tools/aapt2/java/JavaClassGenerator_test.cpp
+++ b/tools/aapt2/java/JavaClassGenerator_test.cpp
@@ -24,6 +24,8 @@
 
 using ::android::StringPiece;
 using ::testing::HasSubstr;
+using ::testing::Lt;
+using ::testing::Ne;
 using ::testing::Not;
 
 namespace aapt {
@@ -306,6 +308,53 @@ TEST(JavaClassGeneratorTest, CommentsForStyleablesAndNestedAttributesArePresent)
   EXPECT_THAT(output, HasSubstr(styleable.GetComment()));
 }
 
+TEST(JavaClassGeneratorTest, StyleableAndIndicesAreColocated) {
+  std::unique_ptr<ResourceTable> table =
+      test::ResourceTableBuilder()
+          .SetPackageId("android", 0x01)
+          .AddValue("android:attr/layout_gravity", util::make_unique<Attribute>())
+          .AddValue("android:attr/background", util::make_unique<Attribute>())
+          .AddValue("android:styleable/ActionBar",
+                    test::StyleableBuilder()
+                        .AddItem("android:attr/background", ResourceId(0x01010000))
+                        .Build())
+          .AddValue("android:styleable/ActionBar.LayoutParams",
+                    test::StyleableBuilder()
+                        .AddItem("android:attr/layout_gravity", ResourceId(0x01010001))
+                        .Build())
+          .Build();
+
+  std::unique_ptr<IAaptContext> context =
+      test::ContextBuilder()
+          .AddSymbolSource(util::make_unique<ResourceTableSymbolSource>(table.get()))
+          .SetNameManglerPolicy(NameManglerPolicy{"android"})
+          .Build();
+
+  JavaClassGeneratorOptions options;
+  JavaClassGenerator generator(context.get(), table.get(), {});
+  std::stringstream out;
+  ASSERT_TRUE(generator.Generate("android", &out));
+  std::string output = out.str();
+
+  std::string::size_type actionbar_pos = output.find("int[] ActionBar");
+  ASSERT_THAT(actionbar_pos, Ne(std::string::npos));
+
+  std::string::size_type actionbar_background_pos = output.find("int ActionBar_background");
+  ASSERT_THAT(actionbar_background_pos, Ne(std::string::npos));
+
+  std::string::size_type actionbar_layout_params_pos = output.find("int[] ActionBar_LayoutParams");
+  ASSERT_THAT(actionbar_layout_params_pos, Ne(std::string::npos));
+
+  std::string::size_type actionbar_layout_params_layout_gravity_pos =
+      output.find("int ActionBar_LayoutParams_layout_gravity");
+  ASSERT_THAT(actionbar_layout_params_layout_gravity_pos, Ne(std::string::npos));
+
+  EXPECT_THAT(actionbar_pos, Lt(actionbar_background_pos));
+  EXPECT_THAT(actionbar_pos, Lt(actionbar_layout_params_pos));
+  EXPECT_THAT(actionbar_background_pos, Lt(actionbar_layout_params_pos));
+  EXPECT_THAT(actionbar_layout_params_pos, Lt(actionbar_layout_params_layout_gravity_pos));
+}
+
 TEST(JavaClassGeneratorTest, CommentsForRemovedAttributesAreNotPresentInClass) {
   Attribute attr(false);
   attr.SetComment(StringPiece("removed"));