Fail framework build when staged type id overlap public type id

Added more elegant error message to fail the build when staged type id
overlap public type id. And also added logic to handle the case when
staging type is assigned the same type with same id as public resource.

Bug: b/268793902
Test: Added and verified affected atests pass
Change-Id: Ic245d703bc34a8f7c2c4c266085a7a09c89918bf

3 files changed, 63 insertions, 18 deletions

diff --git a/tools/aapt2/cmd/Link_test.cpp b/tools/aapt2/cmd/Link_test.cpp
index 28fcc1a4800e..e629bafbdd25 100644
--- a/tools/aapt2/cmd/Link_test.cpp
+++ b/tools/aapt2/cmd/Link_test.cpp
@@ -441,8 +441,8 @@ static void BuildNonFinalizedSDK(const std::string& apk_path, const std::string&
       R"(<resources>
           <public type="attr" name="finalized_res" id="0x01010001"/>
 
-          <!-- S staged attributes (support staged resources in the same type id) -->
-          <staging-public-group type="attr" first-id="0x01010050">
+          <!-- S staged attributes (Not support staged resources in the same type id) -->
+          <staging-public-group type="attr" first-id="0x01fc0050">
             <public name="staged_s_res" />
           </staging-public-group>
 
@@ -480,8 +480,8 @@ static void BuildFinalizedSDK(const std::string& apk_path, const std::string& ja
           <public type="attr" name="staged_s2_res" id="0x01010003"/>
           <public type="string" name="staged_s_string" id="0x01020000"/>
 
-          <!-- S staged attributes (support staged resources in the same type id) -->
-          <staging-public-group-final type="attr" first-id="0x01010050">
+          <!-- S staged attributes (Not support staged resources in the same type id) -->
+          <staging-public-group-final type="attr" first-id="0x01fc0050">
             <public name="staged_s_res" />
           </staging-public-group-final>
 
@@ -551,7 +551,7 @@ TEST_F(LinkTest, StagedAndroidApi) {
   EXPECT_THAT(android_r_contents, HasSubstr("public static final int finalized_res=0x01010001;"));
   EXPECT_THAT(
       android_r_contents,
-      HasSubstr("public static final int staged_s_res; static { staged_s_res=0x01010050; }"));
+      HasSubstr("public static final int staged_s_res; static { staged_s_res=0x01fc0050; }"));
   EXPECT_THAT(
       android_r_contents,
       HasSubstr("public static final int staged_s_string; static { staged_s_string=0x01fd0080; }"));
@@ -583,7 +583,7 @@ TEST_F(LinkTest, StagedAndroidApi) {
 
   result = am.GetResourceId("android:attr/staged_s_res");
   ASSERT_TRUE(result.has_value());
-  EXPECT_THAT(*result, Eq(0x01010050));
+  EXPECT_THAT(*result, Eq(0x01fc0050));
 
   result = am.GetResourceId("android:string/staged_s_string");
   ASSERT_TRUE(result.has_value());
diff --git a/tools/aapt2/compile/IdAssigner.cpp b/tools/aapt2/compile/IdAssigner.cpp
index b3f98a9d3e30..5421abde3689 100644
--- a/tools/aapt2/compile/IdAssigner.cpp
+++ b/tools/aapt2/compile/IdAssigner.cpp
@@ -37,6 +37,7 @@ using Result = expected<T, std::string>;
 
 template <typename Id, typename Key>
 struct NextIdFinder {
+  std::map<Id, Key> pre_assigned_ids_;
   explicit NextIdFinder(Id start_id = 0u) : next_id_(start_id){};
 
   // Attempts to reserve an identifier for the specified key.
@@ -55,7 +56,6 @@ struct NextIdFinder {
   Id next_id_;
   bool next_id_called_ = false;
   bool exhausted_ = false;
-  std::map<Id, Key> pre_assigned_ids_;
   typename std::map<Id, Key>::iterator next_preassigned_id_;
 };
 
@@ -158,7 +158,7 @@ bool IdAssigner::Consume(IAaptContext* context, ResourceTable* table) {
   }
 
   if (assigned_id_map_) {
-    // Reserve all the IDs mentioned in the stable ID map. That way we won't assig IDs that were
+    // Reserve all the IDs mentioned in the stable ID map. That way we won't assign IDs that were
     // listed in the map if they don't exist in the table.
     for (const auto& stable_id_entry : *assigned_id_map_) {
       const ResourceName& pre_assigned_name = stable_id_entry.first;
@@ -191,6 +191,11 @@ bool IdAssigner::Consume(IAaptContext* context, ResourceTable* table) {
 }
 
 namespace {
+static const std::string_view staged_type_overlap_error =
+    "Staged public resource type IDs have conflict with non staged public resources type "
+    "IDs, please restart staged resource type ID assignment at 0xff in public-staging.xml "
+    "and also delete all the overlapping groups in public-final.xml";
+
 template <typename Id, typename Key>
 Result<Id> NextIdFinder<Id, Key>::ReserveId(Key key, Id id) {
   CHECK(!next_id_called_) << "ReserveId cannot be called after NextId";
@@ -282,8 +287,20 @@ bool IdAssignerContext::ReserveId(const ResourceName& name, ResourceId id,
     // another type.
     auto assign_result = type_id_finder_.ReserveId(key, id.type_id());
     if (!assign_result.has_value()) {
-      diag->Error(android::DiagMessage() << "can't assign ID " << id << " to resource " << name
-                                         << " because type " << assign_result.error());
+      auto pre_assigned_type = type_id_finder_.pre_assigned_ids_[id.type_id()].type;
+      bool pre_assigned_type_staged =
+          non_staged_type_ids_.find(pre_assigned_type) == non_staged_type_ids_.end();
+      auto hex_type_id = fmt::format("{:#04x}", (int)id.type_id());
+      bool current_type_staged = visibility.staged_api;
+      diag->Error(android::DiagMessage()
+                  << "can't assign type ID " << hex_type_id << " to "
+                  << (current_type_staged ? "staged type " : "non staged type ") << name.type.type
+                  << " because this type ID have been assigned to "
+                  << (pre_assigned_type_staged ? "staged type " : "non staged type ")
+                  << pre_assigned_type);
+      if (pre_assigned_type_staged || current_type_staged) {
+        diag->Error(android::DiagMessage() << staged_type_overlap_error);
+      }
       return false;
     }
     type = types_.emplace(key, TypeGroup(package_id_, id.type_id())).first;
@@ -298,6 +315,20 @@ bool IdAssignerContext::ReserveId(const ResourceName& name, ResourceId id,
                   << " because type already has ID " << std::hex << (int)id.type_id());
       return false;
     }
+  } else {
+    // Ensure that staged public resources cannot have the same type name and type id with
+    // non staged public resources.
+    auto non_staged_type = non_staged_type_ids_.find(name.type.type);
+    if (non_staged_type != non_staged_type_ids_.end() && non_staged_type->second == id.type_id()) {
+      diag->Error(
+          android::DiagMessage()
+          << "can`t assign type ID " << fmt::format("{:#04x}", (int)id.type_id())
+          << " to staged type " << name.type.type << " because type ID "
+          << fmt::format("{:#04x}", (int)id.type_id())
+          << " already has been assigned to a non staged resource type with the same type name");
+      diag->Error(android::DiagMessage() << staged_type_overlap_error);
+      return false;
+    }
   }
 
   auto assign_result = type->second.ReserveId(name, id);
diff --git a/tools/aapt2/compile/IdAssigner_test.cpp b/tools/aapt2/compile/IdAssigner_test.cpp
index 8911dad39470..ce45b7c1df04 100644
--- a/tools/aapt2/compile/IdAssigner_test.cpp
+++ b/tools/aapt2/compile/IdAssigner_test.cpp
@@ -117,14 +117,28 @@ TEST_F(IdAssignerTests, FailWhenTypeHasTwoNonStagedIds) {
 }
 
 TEST_F(IdAssignerTests, FailWhenTypeHasTwoNonStagedIdsRegardlessOfStagedId) {
-  auto table = test::ResourceTableBuilder()
-                   .AddSimple("android:attr/foo", ResourceId(0x01050000))
-                   .AddSimple("android:attr/bar", ResourceId(0x01ff0006))
-                   .Add(NewResourceBuilder("android:attr/staged_baz")
-                            .SetId(0x01ff0000)
-                            .SetVisibility({.staged_api = true})
-                            .Build())
-                   .Build();
+  auto table =
+      test::ResourceTableBuilder()
+          .AddSimple("android:attr/foo", ResourceId(0x01050000))
+          .AddSimple("android:attr/bar", ResourceId(0x01ff0006))
+          .Add(NewResourceBuilder("android:attr/staged_baz")
+                   .SetId(0x01ff0000)
+                   .SetVisibility({.staged_api = true, .level = Visibility::Level::kPublic})
+                   .Build())
+          .Build();
+  IdAssigner assigner;
+  ASSERT_FALSE(assigner.Consume(context.get(), table.get()));
+}
+
+TEST_F(IdAssignerTests, FailWhenTypeHaveBothStagedAndNonStagedIds) {
+  auto table =
+      test::ResourceTableBuilder()
+          .AddSimple("android:attr/foo", ResourceId(0x01010000))
+          .Add(NewResourceBuilder("android:bool/staged_baz")
+                   .SetId(0x01010001)
+                   .SetVisibility({.staged_api = true, .level = Visibility::Level::kPublic})
+                   .Build())
+          .Build();
   IdAssigner assigner;
   ASSERT_FALSE(assigner.Consume(context.get(), table.get()));
 }