Remove flag disabled strings from string pool

Test: Automated
Bug: 329436914
Flag: EXEMPT Aconfig not supported on host tools

Change-Id: I627feff5774f44a398a8337733498ede601d07a4

12 files changed, 188 insertions, 73 deletions

diff --git a/core/tests/resourceflaggingtests/Android.bp b/core/tests/resourceflaggingtests/Android.bp
index dd86094127da..efb843735aef 100644
--- a/core/tests/resourceflaggingtests/Android.bp
+++ b/core/tests/resourceflaggingtests/Android.bp
@@ -22,54 +22,6 @@ package {
     default_team: "trendy_team_android_resources",
 }
 
-genrule {
-    name: "resource-flagging-test-app-resources-compile",
-    tools: ["aapt2"],
-    srcs: [
-        "flagged_resources_res/values/bools.xml",
-    ],
-    out: ["values_bools.arsc.flat"],
-    cmd: "$(location aapt2) compile $(in) -o $(genDir) " +
-        "--feature-flags test.package.falseFlag:ro=false,test.package.trueFlag:ro=true",
-}
-
-genrule {
-    name: "resource-flagging-test-app-resources-compile2",
-    tools: ["aapt2"],
-    srcs: [
-        "flagged_resources_res/values/bools2.xml",
-    ],
-    out: ["values_bools2.arsc.flat"],
-    cmd: "$(location aapt2) compile $(in) -o $(genDir) " +
-        "--feature-flags test.package.falseFlag:ro=false,test.package.trueFlag:ro=true",
-}
-
-genrule {
-    name: "resource-flagging-test-app-apk",
-    tools: ["aapt2"],
-    // The first input file in the list must be the manifest
-    srcs: [
-        "TestAppAndroidManifest.xml",
-        ":resource-flagging-test-app-resources-compile",
-        ":resource-flagging-test-app-resources-compile2",
-    ],
-    out: ["resapp.apk"],
-    cmd: "$(location aapt2) link -o $(out) --manifest $(in)",
-}
-
-java_genrule {
-    name: "resource-flagging-apk-as-resource",
-    srcs: [
-        ":resource-flagging-test-app-apk",
-    ],
-    out: ["apks_as_resources.res.zip"],
-    tools: ["soong_zip"],
-
-    cmd: "mkdir -p $(genDir)/res/raw && " +
-        "cp $(in) $(genDir)/res/raw/$$(basename $(in)) && " +
-        "$(location soong_zip) -o $(out) -C $(genDir)/res -D $(genDir)/res",
-}
-
 android_test {
     name: "ResourceFlaggingTests",
     srcs: [
@@ -82,6 +34,6 @@ android_test {
         "testng",
         "compatibility-device-util-axt",
     ],
-    resource_zips: [":resource-flagging-apk-as-resource"],
+    resource_zips: [":resource-flagging-test-app-apk-as-resource"],
     test_suites: ["device-tests"],
 }
diff --git a/core/tests/resourceflaggingtests/src/com/android/resourceflaggingtests/ResourceFlaggingTest.java b/core/tests/resourceflaggingtests/src/com/android/resourceflaggingtests/ResourceFlaggingTest.java
index ad8542e0f6b3..a3d8d3dbf31a 100644
--- a/core/tests/resourceflaggingtests/src/com/android/resourceflaggingtests/ResourceFlaggingTest.java
+++ b/core/tests/resourceflaggingtests/src/com/android/resourceflaggingtests/ResourceFlaggingTest.java
@@ -68,12 +68,29 @@ public class ResourceFlaggingTest {
         assertThat(getBoolean("res3")).isTrue();
     }
 
+    @Test
+    public void testFlagDisabledNoValue() {
+        assertThat(getString("str1")).isEqualTo("");
+    }
+
     private boolean getBoolean(String name) {
-        int resId = mResources.getIdentifier(name, "bool", "com.android.intenal.flaggedresources");
+        int resId = mResources.getIdentifier(
+                name,
+                "bool",
+                "com.android.intenal.flaggedresources");
         assertThat(resId).isNotEqualTo(0);
         return mResources.getBoolean(resId);
     }
 
+    private String getString(String name) {
+        int resId = mResources.getIdentifier(
+                name,
+                "string",
+                "com.android.intenal.flaggedresources");
+        assertThat(resId).isNotEqualTo(0);
+        return mResources.getString(resId);
+    }
+
     private String extractApkAndGetPath(int id) throws Exception {
         final Resources resources = mContext.getResources();
         try (InputStream is = resources.openRawResource(id)) {
diff --git a/tools/aapt2/Android.bp b/tools/aapt2/Android.bp
index 3f9016ba4852..7b227cebe2ae 100644
--- a/tools/aapt2/Android.bp
+++ b/tools/aapt2/Android.bp
@@ -189,6 +189,7 @@ cc_test_host {
         "integration-tests/CommandTests/**/*",
         "integration-tests/ConvertTest/**/*",
         "integration-tests/DumpTest/**/*",
+        ":resource-flagging-test-app-apk",
     ],
 }
 
diff --git a/tools/aapt2/ResourceParser.cpp b/tools/aapt2/ResourceParser.cpp
index 9444dd968f5f..1c85e9ff231b 100644
--- a/tools/aapt2/ResourceParser.cpp
+++ b/tools/aapt2/ResourceParser.cpp
@@ -690,9 +690,7 @@ bool ResourceParser::ParseResource(xml::XmlPullParser* parser,
         resource_format = item_iter->second.format;
       }
 
-      // Don't bother parsing the item if it is behind a disabled flag
-      if (out_resource->flag_status != FlagStatus::Disabled &&
-          !ParseItem(parser, out_resource, resource_format)) {
+      if (!ParseItem(parser, out_resource, resource_format)) {
         return false;
       }
       return true;
diff --git a/tools/aapt2/ResourceParser_test.cpp b/tools/aapt2/ResourceParser_test.cpp
index 2e6ad13d99de..b59b16574c42 100644
--- a/tools/aapt2/ResourceParser_test.cpp
+++ b/tools/aapt2/ResourceParser_test.cpp
@@ -69,13 +69,8 @@ class ResourceParserTest : public ::testing::Test {
     return TestParse(str, ConfigDescription{});
   }
 
-  ::testing::AssertionResult TestParse(StringPiece str, ResourceParserOptions parserOptions) {
-    return TestParse(str, ConfigDescription{}, parserOptions);
-  }
-
-  ::testing::AssertionResult TestParse(
-      StringPiece str, const ConfigDescription& config,
-      ResourceParserOptions parserOptions = ResourceParserOptions()) {
+  ::testing::AssertionResult TestParse(StringPiece str, const ConfigDescription& config) {
+    ResourceParserOptions parserOptions;
     ResourceParser parser(context_->GetDiagnostics(), &table_, android::Source{"test"}, config,
                           parserOptions);
 
@@ -247,19 +242,6 @@ TEST_F(ResourceParserTest, ParseStringTranslatableAttribute) {
   EXPECT_FALSE(TestParse(R"(<string name="foo4" translatable="yes">Translate</string>)"));
 }
 
-TEST_F(ResourceParserTest, ParseStringBehindDisabledFlag) {
-  FeatureFlagProperties flag_properties(true, false);
-  ResourceParserOptions options;
-  options.feature_flag_values = {{"falseFlag", flag_properties}};
-  ASSERT_TRUE(TestParse(
-      R"(<string name="foo" android:featureFlag="falseFlag"
-              xmlns:android="http://schemas.android.com/apk/res/android">foo</string>)",
-      options));
-
-  String* str = test::GetValue<String>(&table_, "string/foo");
-  ASSERT_THAT(str, IsNull());
-}
-
 TEST_F(ResourceParserTest, IgnoreXliffTagsOtherThanG) {
   std::string input = R"(
       <string name="foo" xmlns:xliff="urn:oasis:names:tc:xliff:document:1.2">
diff --git a/tools/aapt2/cmd/Link.cpp b/tools/aapt2/cmd/Link.cpp
index 642a5618b6ad..88cc4dab2a96 100644
--- a/tools/aapt2/cmd/Link.cpp
+++ b/tools/aapt2/cmd/Link.cpp
@@ -1840,11 +1840,51 @@ class Linker {
     return validate(attr->value);
   }
 
+  class FlagDisabledStringVisitor : public DescendingValueVisitor {
+   public:
+    using DescendingValueVisitor::Visit;
+
+    explicit FlagDisabledStringVisitor(android::StringPool& string_pool)
+        : string_pool_(string_pool) {
+    }
+
+    void Visit(RawString* value) override {
+      value->value = string_pool_.MakeRef("");
+    }
+
+    void Visit(String* value) override {
+      value->value = string_pool_.MakeRef("");
+    }
+
+    void Visit(StyledString* value) override {
+      value->value = string_pool_.MakeRef(android::StyleString{{""}, {}});
+    }
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(FlagDisabledStringVisitor);
+    android::StringPool& string_pool_;
+  };
+
   // Writes the AndroidManifest, ResourceTable, and all XML files referenced by the ResourceTable
   // to the IArchiveWriter.
   bool WriteApk(IArchiveWriter* writer, proguard::KeepSet* keep_set, xml::XmlResource* manifest,
                 ResourceTable* table) {
     TRACE_CALL();
+
+    FlagDisabledStringVisitor visitor(table->string_pool);
+
+    for (auto& package : table->packages) {
+      for (auto& type : package->types) {
+        for (auto& entry : type->entries) {
+          for (auto& config_value : entry->values) {
+            if (config_value->flag_status == FlagStatus::Disabled) {
+              config_value->value->Accept(&visitor);
+            }
+          }
+        }
+      }
+    }
+
     const bool keep_raw_values = (context_->GetPackageType() == PackageType::kStaticLib)
                                  || options_.keep_raw_values;
     bool result = FlattenXml(context_, *manifest, kAndroidManifestPath, keep_raw_values,
diff --git a/tools/aapt2/integration-tests/FlaggedResourcesTest/Android.bp b/tools/aapt2/integration-tests/FlaggedResourcesTest/Android.bp
new file mode 100644
index 000000000000..efa8e0494856
--- /dev/null
+++ b/tools/aapt2/integration-tests/FlaggedResourcesTest/Android.bp
@@ -0,0 +1,65 @@
+// Copyright (C) 2024 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package {
+    // See: http://go/android-license-faq
+    // A large-scale-change added 'default_applicable_licenses' to import
+    // all of the 'license_kinds' from "frameworks_base_license"
+    // to get the below license kinds:
+    //   SPDX-license-identifier-Apache-2.0
+    default_applicable_licenses: ["frameworks_base_license"],
+    default_team: "trendy_team_android_resources",
+}
+
+genrule {
+    name: "resource-flagging-test-app-compile",
+    tools: ["aapt2"],
+    srcs: [
+        "res/values/bools.xml",
+        "res/values/bools2.xml",
+        "res/values/strings.xml",
+    ],
+    out: [
+        "values_bools.arsc.flat",
+        "values_bools2.arsc.flat",
+        "values_strings.arsc.flat",
+    ],
+    cmd: "$(location aapt2) compile $(in) -o $(genDir) " +
+        "--feature-flags test.package.falseFlag:ro=false,test.package.trueFlag:ro=true",
+}
+
+genrule {
+    name: "resource-flagging-test-app-apk",
+    tools: ["aapt2"],
+    // The first input file in the list must be the manifest
+    srcs: [
+        "AndroidManifest.xml",
+        ":resource-flagging-test-app-compile",
+    ],
+    out: ["resapp.apk"],
+    cmd: "$(location aapt2) link -o $(out) --manifest $(in)",
+}
+
+java_genrule {
+    name: "resource-flagging-test-app-apk-as-resource",
+    srcs: [
+        ":resource-flagging-test-app-apk",
+    ],
+    out: ["apks_as_resources.res.zip"],
+    tools: ["soong_zip"],
+
+    cmd: "mkdir -p $(genDir)/res/raw && " +
+        "cp $(in) $(genDir)/res/raw/$$(basename $(in)) && " +
+        "$(location soong_zip) -o $(out) -C $(genDir)/res -D $(genDir)/res",
+}
diff --git a/core/tests/resourceflaggingtests/TestAppAndroidManifest.xml b/tools/aapt2/integration-tests/FlaggedResourcesTest/AndroidManifest.xml
index d6cdeb7b5231..d6cdeb7b5231 100644
--- a/core/tests/resourceflaggingtests/TestAppAndroidManifest.xml
+++ b/tools/aapt2/integration-tests/FlaggedResourcesTest/AndroidManifest.xml
diff --git a/core/tests/resourceflaggingtests/flagged_resources_res/values/bools.xml b/tools/aapt2/integration-tests/FlaggedResourcesTest/res/values/bools.xml
index 8d0146511d1d..8d0146511d1d 100644
--- a/core/tests/resourceflaggingtests/flagged_resources_res/values/bools.xml
+++ b/tools/aapt2/integration-tests/FlaggedResourcesTest/res/values/bools.xml
diff --git a/core/tests/resourceflaggingtests/flagged_resources_res/values/bools2.xml b/tools/aapt2/integration-tests/FlaggedResourcesTest/res/values/bools2.xml
index e7563aa0fbdd..e7563aa0fbdd 100644
--- a/core/tests/resourceflaggingtests/flagged_resources_res/values/bools2.xml
+++ b/tools/aapt2/integration-tests/FlaggedResourcesTest/res/values/bools2.xml
diff --git a/tools/aapt2/integration-tests/FlaggedResourcesTest/res/values/strings.xml b/tools/aapt2/integration-tests/FlaggedResourcesTest/res/values/strings.xml
new file mode 100644
index 000000000000..5c0fca16fe39
--- /dev/null
+++ b/tools/aapt2/integration-tests/FlaggedResourcesTest/res/values/strings.xml
@@ -0,0 +1,6 @@
+<?xml version="1.0" encoding="utf-8"?>
+<resources xmlns:android="http://schemas.android.com/apk/res/android">
+    <string name="str">plain string</string>
+
+    <string name="str1" android:featureFlag="test.package.falseFlag">DONTFIND</string>
+</resources>
\ No newline at end of file
diff --git a/tools/aapt2/link/FlaggedResources_test.cpp b/tools/aapt2/link/FlaggedResources_test.cpp
new file mode 100644
index 000000000000..03c4ac9c83a2
--- /dev/null
+++ b/tools/aapt2/link/FlaggedResources_test.cpp
@@ -0,0 +1,54 @@
+/*
+ * Copyright (C) 2022 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "LoadedApk.h"
+#include "cmd/Dump.h"
+#include "io/StringStream.h"
+#include "test/Test.h"
+#include "text/Printer.h"
+
+using ::aapt::io::StringOutputStream;
+using ::aapt::text::Printer;
+using testing::Eq;
+using testing::Ne;
+
+namespace aapt {
+
+using FlaggedResourcesTest = CommandTestFixture;
+
+static android::NoOpDiagnostics noop_diag;
+
+void DumpStringPoolToString(LoadedApk* loaded_apk, std::string* output) {
+  StringOutputStream output_stream(output);
+  Printer printer(&output_stream);
+
+  DumpStringsCommand command(&printer, &noop_diag);
+  ASSERT_EQ(command.Dump(loaded_apk), 0);
+  output_stream.Flush();
+}
+
+TEST_F(FlaggedResourcesTest, DisabledStringRemoved) {
+  auto apk_path = file::BuildPath({android::base::GetExecutableDirectory(), "resapp.apk"});
+  auto loaded_apk = LoadedApk::LoadApkFromPath(apk_path, &noop_diag);
+
+  std::string output;
+  DumpStringPoolToString(loaded_apk.get(), &output);
+
+  std::string excluded = "DONTFIND";
+  ASSERT_EQ(output.find(excluded), std::string::npos);
+}
+
+}  // namespace aapt