Merge "Fix persisting which resources use feature flags" into main

9 files changed, 86 insertions, 57 deletions

diff --git a/tools/aapt2/ResourcesInternal.proto b/tools/aapt2/ResourcesInternal.proto
index f4735a2f6ce7..380c5f21103c 100644
--- a/tools/aapt2/ResourcesInternal.proto
+++ b/tools/aapt2/ResourcesInternal.proto
@@ -50,8 +50,11 @@ message CompiledFile {
   // Any symbols this file auto-generates/exports (eg. @+id/foo in an XML file).
   repeated Symbol exported_symbol = 5;
 
-  // The status of the flag the file is behind if any
+  // The status of the read only flag the file is behind if any
   uint32 flag_status = 6;
   bool flag_negated = 7;
   string flag_name = 8;
+
+  // Whether the file uses read/write feature flags
+  bool uses_readwrite_feature_flags = 9;
 }
diff --git a/tools/aapt2/cmd/Compile.cpp b/tools/aapt2/cmd/Compile.cpp
index a5e18d35a256..3b4f5429f254 100644
--- a/tools/aapt2/cmd/Compile.cpp
+++ b/tools/aapt2/cmd/Compile.cpp
@@ -407,6 +407,45 @@ static bool IsValidFile(IAaptContext* context, const std::string& input_path) {
   return true;
 }
 
+class FindReadWriteFlagsVisitor : public xml::Visitor {
+ public:
+  FindReadWriteFlagsVisitor(const FeatureFlagValues& feature_flag_values)
+      : feature_flag_values_(feature_flag_values) {
+  }
+
+  void Visit(xml::Element* node) override {
+    if (had_flags_) {
+      return;
+    }
+    auto* attr = node->FindAttribute(xml::kSchemaAndroid, xml::kAttrFeatureFlag);
+    if (attr != nullptr) {
+      std::string_view flag_name = util::TrimWhitespace(attr->value);
+      if (flag_name.starts_with('!')) {
+        flag_name = flag_name.substr(1);
+      }
+      if (auto it = feature_flag_values_.find(flag_name); it != feature_flag_values_.end()) {
+        if (!it->second.read_only) {
+          had_flags_ = true;
+          return;
+        }
+      } else {
+        // Flag not passed to aapt2, must evaluate at runtime
+        had_flags_ = true;
+        return;
+      }
+    }
+    VisitChildren(node);
+  }
+
+  bool HadFlags() const {
+    return had_flags_;
+  }
+
+ private:
+  bool had_flags_ = false;
+  const FeatureFlagValues& feature_flag_values_;
+};
+
 static bool CompileXml(IAaptContext* context, const CompileOptions& options,
                        const ResourcePathData& path_data, io::IFile* file, IArchiveWriter* writer,
                        const std::string& output_path) {
@@ -436,6 +475,10 @@ static bool CompileXml(IAaptContext* context, const CompileOptions& options,
   xmlres->file.type = ResourceFile::Type::kProtoXml;
   xmlres->file.flag = ParseFlag(path_data.flag_name);
 
+  FindReadWriteFlagsVisitor visitor(options.feature_flag_values);
+  xmlres->root->Accept(&visitor);
+  xmlres->file.uses_readwrite_feature_flags = visitor.HadFlags();
+
   if (xmlres->file.flag) {
     std::string error;
     auto flag_status = GetFlagStatus(xmlres->file.flag, options.feature_flag_values, &error);
diff --git a/tools/aapt2/cmd/Link.cpp b/tools/aapt2/cmd/Link.cpp
index 755dbb6f8e42..0e18ee250993 100644
--- a/tools/aapt2/cmd/Link.cpp
+++ b/tools/aapt2/cmd/Link.cpp
@@ -615,6 +615,8 @@ bool ResourceFileFlattener::Flatten(ResourceTable* table, IArchiveWriter* archiv
             file_op.xml_to_flatten->file.source = file_ref->GetSource();
             file_op.xml_to_flatten->file.name =
                 ResourceName(pkg->name, type->named_type, entry->name);
+            file_op.xml_to_flatten->file.uses_readwrite_feature_flags =
+                config_value->uses_readwrite_feature_flags;
           }
 
           // NOTE(adamlesinski): Explicitly construct a StringPiece here, or
@@ -647,6 +649,17 @@ bool ResourceFileFlattener::Flatten(ResourceTable* table, IArchiveWriter* archiv
             }
           }
 
+          FeatureFlagsFilterOptions flags_filter_options;
+          // Don't fail on unrecognized flags or flags without values as these flags might be
+          // defined and have a value by the time they are evaluated at runtime.
+          flags_filter_options.fail_on_unrecognized_flags = false;
+          flags_filter_options.flags_must_have_value = false;
+          flags_filter_options.remove_disabled_elements = true;
+          FeatureFlagsFilter flags_filter(options_.feature_flag_values, flags_filter_options);
+          if (!flags_filter.Consume(context_, file_op.xml_to_flatten.get())) {
+            return 1;
+          }
+
           std::vector<std::unique_ptr<xml::XmlResource>> versioned_docs =
               LinkAndVersionXmlFile(table, &file_op);
           if (versioned_docs.empty()) {
@@ -673,6 +686,7 @@ bool ResourceFileFlattener::Flatten(ResourceTable* table, IArchiveWriter* archiv
 
               // Update the output format of this XML file.
               file_ref->type = XmlFileTypeForOutputFormat(options_.output_format);
+
               bool result = table->AddResource(
                   NewResourceBuilder(file.name)
                       .SetValue(std::move(file_ref), file.config)
@@ -685,14 +699,6 @@ bool ResourceFileFlattener::Flatten(ResourceTable* table, IArchiveWriter* archiv
               }
             }
 
-            FeatureFlagsFilterOptions flags_filter_options;
-            flags_filter_options.fail_on_unrecognized_flags = false;
-            flags_filter_options.flags_must_have_value = false;
-            FeatureFlagsFilter flags_filter(options_.feature_flag_values, flags_filter_options);
-            if (!flags_filter.Consume(context_, doc.get())) {
-              return 1;
-            }
-
             error |= !FlattenXml(context_, *doc, dst_path, options_.keep_raw_values,
                                  false /*utf16*/, options_.output_format, archive_writer);
           }
diff --git a/tools/aapt2/format/proto/ProtoDeserialize.cpp b/tools/aapt2/format/proto/ProtoDeserialize.cpp
index 91ec3485ac3b..b8936553a193 100644
--- a/tools/aapt2/format/proto/ProtoDeserialize.cpp
+++ b/tools/aapt2/format/proto/ProtoDeserialize.cpp
@@ -640,6 +640,7 @@ bool DeserializeCompiledFileFromPb(const pb::internal::CompiledFile& pb_file,
   out_file->name = name_ref.ToResourceName();
   out_file->source.path = pb_file.source_path();
   out_file->type = DeserializeFileReferenceTypeFromPb(pb_file.type());
+  out_file->uses_readwrite_feature_flags = pb_file.uses_readwrite_feature_flags();
 
   out_file->flag_status = (FlagStatus)pb_file.flag_status();
   if (!pb_file.flag_name().empty()) {
diff --git a/tools/aapt2/format/proto/ProtoSerialize.cpp b/tools/aapt2/format/proto/ProtoSerialize.cpp
index fcc77d5a9d6d..da99c4f5917c 100644
--- a/tools/aapt2/format/proto/ProtoSerialize.cpp
+++ b/tools/aapt2/format/proto/ProtoSerialize.cpp
@@ -767,6 +767,7 @@ void SerializeCompiledFileToPb(const ResourceFile& file, pb::internal::CompiledF
     out_file->set_flag_negated(file.flag->negated);
     out_file->set_flag_name(file.flag->name);
   }
+  out_file->set_uses_readwrite_feature_flags(file.uses_readwrite_feature_flags);
 
   for (const SourcedResourceName& exported : file.exported_symbols) {
     pb::internal::CompiledFile_Symbol* pb_symbol = out_file->add_exported_symbol();
diff --git a/tools/aapt2/link/FlaggedResources_test.cpp b/tools/aapt2/link/FlaggedResources_test.cpp
index 47a71fe36e9f..4dcb8507fa45 100644
--- a/tools/aapt2/link/FlaggedResources_test.cpp
+++ b/tools/aapt2/link/FlaggedResources_test.cpp
@@ -226,9 +226,11 @@ TEST_F(FlaggedResourcesTest, ReadWriteFlagInXmlGetsFlagged) {
       }
     }
   }
+
   ASSERT_TRUE(found) << "No entry for layout1 at v36 with FLAG_USES_FEATURE_FLAGS bit set";
-  // There should only be 1 entry that has the FLAG_USES_FEATURE_FLAGS bit of flags set to 1
-  ASSERT_EQ(fields_flagged, 1);
+  // There should only be 2 entry that has the FLAG_USES_FEATURE_FLAGS bit of flags set to 1, the
+  // three versions of the layout file that has flags
+  ASSERT_EQ(fields_flagged, 3);
 }
 
 }  // namespace aapt
diff --git a/tools/aapt2/link/FlaggedXmlVersioner.cpp b/tools/aapt2/link/FlaggedXmlVersioner.cpp
index 8a3337c446cb..626cae73bfa2 100644
--- a/tools/aapt2/link/FlaggedXmlVersioner.cpp
+++ b/tools/aapt2/link/FlaggedXmlVersioner.cpp
@@ -35,10 +35,6 @@ class AllDisabledFlagsVisitor : public xml::Visitor {
     VisitChildren(node);
   }
 
-  bool HadFlags() const {
-    return had_flags_;
-  }
-
  private:
   bool FixupOrShouldRemove(const std::unique_ptr<xml::Node>& node) {
     if (auto* el = NodeCast<Element>(node.get())) {
@@ -47,7 +43,6 @@ class AllDisabledFlagsVisitor : public xml::Visitor {
         return false;
       }
 
-      had_flags_ = true;
       // This class assumes all flags are disabled so we want to remove any elements behind flags
       // unless the flag specification is negated. In the negated case we remove the featureFlag
       // attribute because we have already determined whether we are keeping the element or not.
@@ -62,56 +57,27 @@ class AllDisabledFlagsVisitor : public xml::Visitor {
 
     return false;
   }
-
-  bool had_flags_ = false;
-};
-
-// An xml visitor that goes through the a doc and determines if any elements are behind a flag.
-class FindFlagsVisitor : public xml::Visitor {
- public:
-  void Visit(xml::Element* node) override {
-    if (had_flags_) {
-      return;
-    }
-    auto* attr = node->FindAttribute(xml::kSchemaAndroid, xml::kAttrFeatureFlag);
-    if (attr != nullptr) {
-      had_flags_ = true;
-      return;
-    }
-    VisitChildren(node);
-  }
-
-  bool HadFlags() const {
-    return had_flags_;
-  }
-
-  bool had_flags_ = false;
 };
 
 std::vector<std::unique_ptr<xml::XmlResource>> FlaggedXmlVersioner::Process(IAaptContext* context,
                                                                             xml::XmlResource* doc) {
   std::vector<std::unique_ptr<xml::XmlResource>> docs;
-  if ((static_cast<ApiVersion>(doc->file.config.sdkVersion) >= SDK_BAKLAVA) ||
-      (static_cast<ApiVersion>(context->GetMinSdkVersion()) >= SDK_BAKLAVA)) {
+  if (!doc->file.uses_readwrite_feature_flags) {
+    docs.push_back(doc->Clone());
+  } else if ((static_cast<ApiVersion>(doc->file.config.sdkVersion) >= SDK_BAKLAVA) ||
+             (static_cast<ApiVersion>(context->GetMinSdkVersion()) >= SDK_BAKLAVA)) {
     // Support for read/write flags was added in baklava so if the doc will only get used on
     // baklava or later we can just return the original doc.
     docs.push_back(doc->Clone());
-    FindFlagsVisitor visitor;
-    doc->root->Accept(&visitor);
-    docs.back()->file.uses_readwrite_feature_flags = visitor.HadFlags();
   } else {
     auto preBaklavaVersion = doc->Clone();
     AllDisabledFlagsVisitor visitor;
     preBaklavaVersion->root->Accept(&visitor);
-    preBaklavaVersion->file.uses_readwrite_feature_flags = false;
     docs.push_back(std::move(preBaklavaVersion));
 
-    if (visitor.HadFlags()) {
-      auto baklavaVersion = doc->Clone();
-      baklavaVersion->file.config.sdkVersion = SDK_BAKLAVA;
-      baklavaVersion->file.uses_readwrite_feature_flags = true;
-      docs.push_back(std::move(baklavaVersion));
-    }
+    auto baklavaVersion = doc->Clone();
+    baklavaVersion->file.config.sdkVersion = SDK_BAKLAVA;
+    docs.push_back(std::move(baklavaVersion));
   }
   return docs;
 }
diff --git a/tools/aapt2/link/FlaggedXmlVersioner_test.cpp b/tools/aapt2/link/FlaggedXmlVersioner_test.cpp
index 0c1314f165cc..0dc464253385 100644
--- a/tools/aapt2/link/FlaggedXmlVersioner_test.cpp
+++ b/tools/aapt2/link/FlaggedXmlVersioner_test.cpp
@@ -101,6 +101,7 @@ TEST_F(FlaggedXmlVersionerTest, PreBaklavaGetsSplit) {
         <TextView android:featureFlag="package.flag" /><TextView /><TextView />
       </LinearLayout>)");
   doc->file.config.sdkVersion = SDK_GINGERBREAD;
+  doc->file.uses_readwrite_feature_flags = true;
 
   FlaggedXmlVersioner versioner;
   auto results = versioner.Process(context_.get(), doc.get());
@@ -131,6 +132,7 @@ TEST_F(FlaggedXmlVersionerTest, NoVersionGetsSplit) {
       <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android">
         <TextView android:featureFlag="package.flag" /><TextView /><TextView />
       </LinearLayout>)");
+  doc->file.uses_readwrite_feature_flags = true;
 
   FlaggedXmlVersioner versioner;
   auto results = versioner.Process(context_.get(), doc.get());
@@ -162,6 +164,7 @@ TEST_F(FlaggedXmlVersionerTest, NegatedFlagAttributeRemoved) {
         <TextView android:featureFlag="!package.flag" /><TextView /><TextView />
       </LinearLayout>)");
   doc->file.config.sdkVersion = SDK_GINGERBREAD;
+  doc->file.uses_readwrite_feature_flags = true;
 
   FlaggedXmlVersioner versioner;
   auto results = versioner.Process(context_.get(), doc.get());
@@ -192,6 +195,7 @@ TEST_F(FlaggedXmlVersionerTest, NegatedFlagAttributeRemovedNoSpecifiedVersion) {
       <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android">
         <TextView android:featureFlag="!package.flag" /><TextView /><TextView />
       </LinearLayout>)");
+  doc->file.uses_readwrite_feature_flags = true;
 
   FlaggedXmlVersioner versioner;
   auto results = versioner.Process(context_.get(), doc.get());
diff --git a/tools/aapt2/link/TableMerger.cpp b/tools/aapt2/link/TableMerger.cpp
index 1d4adc4a57d8..17f332397317 100644
--- a/tools/aapt2/link/TableMerger.cpp
+++ b/tools/aapt2/link/TableMerger.cpp
@@ -295,6 +295,8 @@ bool TableMerger::DoMerge(const android::Source& src, ResourceTablePackage* src_
           dst_config_value =
               dst_entry->FindOrCreateValue(src_config_value->config, src_config_value->product);
         }
+        dst_config_value->uses_readwrite_feature_flags |=
+            src_config_value->uses_readwrite_feature_flags;
 
         // Continue if we're taking the new resource.
         CloningValueTransformer cloner(&main_table_->string_pool);
@@ -378,12 +380,13 @@ bool TableMerger::MergeFile(const ResourceFile& file_desc, bool overlay, io::IFi
   file_ref->file = file;
   file_ref->SetFlagStatus(file_desc.flag_status);
   file_ref->SetFlag(file_desc.flag);
-
   ResourceTablePackage* pkg = table.FindOrCreatePackage(file_desc.name.package);
-  pkg->FindOrCreateType(file_desc.name.type)
-      ->FindOrCreateEntry(file_desc.name.entry)
-      ->FindOrCreateValue(file_desc.config, {})
-      ->value = std::move(file_ref);
+  ResourceConfigValue* config_value = pkg->FindOrCreateType(file_desc.name.type)
+                                          ->FindOrCreateEntry(file_desc.name.entry)
+                                          ->FindOrCreateValue(file_desc.config, {});
+
+  config_value->value = std::move(file_ref);
+  config_value->uses_readwrite_feature_flags = file_desc.uses_readwrite_feature_flags;
 
   return DoMerge(file->GetSource(), pkg, false /*mangle*/, overlay /*overlay*/, true /*allow_new*/);
 }