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
diff --git a/tools/aapt2/cmd/Link_test.cpp b/tools/aapt2/cmd/Link_test.cpp
index 28fcc1a..e629baf 100644
--- a/tools/aapt2/cmd/Link_test.cpp
+++ b/tools/aapt2/cmd/Link_test.cpp
@@ -441,8 +441,8 @@
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 @@
<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 @@
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 @@
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 b3f98a9..5421abd 100644
--- a/tools/aapt2/compile/IdAssigner.cpp
+++ b/tools/aapt2/compile/IdAssigner.cpp
@@ -37,6 +37,7 @@
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 @@
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 @@
}
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 @@
}
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 @@
// 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 @@
<< " 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 8911dad..ce45b7c 100644
--- a/tools/aapt2/compile/IdAssigner_test.cpp
+++ b/tools/aapt2/compile/IdAssigner_test.cpp
@@ -117,14 +117,28 @@
}
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()));
}