diff --git a/tools/aapt2/Android.bp b/tools/aapt2/Android.bp index eb3a99ae63317..2ff92e651edab 100644 --- a/tools/aapt2/Android.bp +++ b/tools/aapt2/Android.bp @@ -101,6 +101,7 @@ cc_library_host_static { "io/ZipArchive.cpp", "link/AutoVersioner.cpp", "link/ManifestFixer.cpp", + "link/NoDefaultResourceRemover.cpp", "link/ProductFilter.cpp", "link/PrivateAttributeMover.cpp", "link/ReferenceLinker.cpp", diff --git a/tools/aapt2/ResourceTable.cpp b/tools/aapt2/ResourceTable.cpp index 95bf9210ba97f..d0faac30425a7 100644 --- a/tools/aapt2/ResourceTable.cpp +++ b/tools/aapt2/ResourceTable.cpp @@ -162,7 +162,7 @@ struct ConfigKey { const StringPiece& product; }; -bool ltConfigKeyRef(const std::unique_ptr& lhs, const ConfigKey& rhs) { +bool lt_config_key_ref(const std::unique_ptr& lhs, const ConfigKey& rhs) { int cmp = lhs->config.compare(*rhs.config); if (cmp == 0) { cmp = StringPiece(lhs->product).compare(rhs.product); @@ -172,8 +172,8 @@ bool ltConfigKeyRef(const std::unique_ptr& lhs, const Confi ResourceConfigValue* ResourceEntry::FindValue(const ConfigDescription& config, const StringPiece& product) { - auto iter = - std::lower_bound(values.begin(), values.end(), ConfigKey{&config, product}, ltConfigKeyRef); + auto iter = std::lower_bound(values.begin(), values.end(), ConfigKey{&config, product}, + lt_config_key_ref); if (iter != values.end()) { ResourceConfigValue* value = iter->get(); if (value->config == config && StringPiece(value->product) == product) { @@ -185,8 +185,8 @@ ResourceConfigValue* ResourceEntry::FindValue(const ConfigDescription& config, ResourceConfigValue* ResourceEntry::FindOrCreateValue(const ConfigDescription& config, const StringPiece& product) { - auto iter = - std::lower_bound(values.begin(), values.end(), ConfigKey{&config, product}, ltConfigKeyRef); + auto iter = std::lower_bound(values.begin(), values.end(), ConfigKey{&config, product}, + lt_config_key_ref); if (iter != values.end()) { ResourceConfigValue* value = iter->get(); if (value->config == config && StringPiece(value->product) == product) { @@ -220,15 +220,16 @@ std::vector ResourceEntry::FindAllValues(const ConfigDescr return results; } -std::vector ResourceEntry::FindValuesIf( - const std::function& f) { - std::vector results; - for (auto& configValue : values) { - if (f(configValue.get())) { - results.push_back(configValue.get()); +bool ResourceEntry::HasDefaultValue() const { + const ConfigDescription& default_config = ConfigDescription::DefaultConfig(); + + // The default config should be at the top of the list, since the list is sorted. + for (auto& config_value : values) { + if (config_value->config == default_config) { + return true; } } - return results; + return false; } // The default handler for collisions. diff --git a/tools/aapt2/ResourceTable.h b/tools/aapt2/ResourceTable.h index 374fe1ea66a13..8534eaaf03664 100644 --- a/tools/aapt2/ResourceTable.h +++ b/tools/aapt2/ResourceTable.h @@ -104,13 +104,26 @@ class ResourceEntry { explicit ResourceEntry(const android::StringPiece& name) : name(name.to_string()) {} ResourceConfigValue* FindValue(const ConfigDescription& config); + ResourceConfigValue* FindValue(const ConfigDescription& config, const android::StringPiece& product); + ResourceConfigValue* FindOrCreateValue(const ConfigDescription& config, const android::StringPiece& product); std::vector FindAllValues(const ConfigDescription& config); - std::vector FindValuesIf( - const std::function& f); + + template + std::vector FindValuesIf(Func f) { + std::vector results; + for (auto& config_value : values) { + if (f(config_value.get())) { + results.push_back(config_value.get()); + } + } + return results; + } + + bool HasDefaultValue() const; private: DISALLOW_COPY_AND_ASSIGN(ResourceEntry); diff --git a/tools/aapt2/cmd/Link.cpp b/tools/aapt2/cmd/Link.cpp index 12ab883454111..0839f6ff87ad8 100644 --- a/tools/aapt2/cmd/Link.cpp +++ b/tools/aapt2/cmd/Link.cpp @@ -55,6 +55,7 @@ #include "java/ProguardRules.h" #include "link/Linkers.h" #include "link/ManifestFixer.h" +#include "link/NoDefaultResourceRemover.h" #include "link/ReferenceLinker.h" #include "link/TableMerger.h" #include "link/XmlCompatVersioner.h" @@ -1785,6 +1786,14 @@ class LinkCommand { util::make_unique(context_)); } + // Before we process anything, remove the resources whose default values don't exist. + // We want to force any references to these to fail the build. + if (!NoDefaultResourceRemover{}.Consume(context_, &final_table_)) { + context_->GetDiagnostics()->Error(DiagMessage() + << "failed removing resources with no defaults"); + return 1; + } + ReferenceLinker linker; if (!linker.Consume(context_, &final_table_)) { context_->GetDiagnostics()->Error(DiagMessage() << "failed linking references"); diff --git a/tools/aapt2/link/NoDefaultResourceRemover.cpp b/tools/aapt2/link/NoDefaultResourceRemover.cpp new file mode 100644 index 0000000000000..cfb4b26aba02b --- /dev/null +++ b/tools/aapt2/link/NoDefaultResourceRemover.cpp @@ -0,0 +1,83 @@ +/* + * Copyright (C) 2018 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 "link/NoDefaultResourceRemover.h" + +#include + +#include "ResourceTable.h" + +namespace aapt { + +static bool IsDefaultConfigRequired(const ConfigDescription& config) { + // We don't want to be overzealous with resource removal, so have strict requirements. + // If a resource defines a value for a locale-only configuration, the default configuration is + // required. + if (ConfigDescription::DefaultConfig().diff(config) == ConfigDescription::CONFIG_LOCALE) { + return true; + } + return false; +} + +static bool KeepResource(const std::unique_ptr& entry) { + if (entry->visibility.level == Visibility::Level::kPublic) { + // Removing a public API without the developer knowing is bad, so just leave this here for now. + return true; + } + + if (entry->HasDefaultValue()) { + // There is a default value, no removal needed. + return true; + } + + // There is no default value defined, check if removal is required. + for (const auto& config_value : entry->values) { + if (IsDefaultConfigRequired(config_value->config)) { + return false; + } + } + return true; +} + +bool NoDefaultResourceRemover::Consume(IAaptContext* context, ResourceTable* table) { + const ConfigDescription default_config = ConfigDescription::DefaultConfig(); + for (auto& pkg : table->packages) { + for (auto& type : pkg->types) { + const auto end_iter = type->entries.end(); + const auto new_end_iter = + std::stable_partition(type->entries.begin(), end_iter, KeepResource); + for (auto iter = new_end_iter; iter != end_iter; ++iter) { + const ResourceName name(pkg->name, type->type, (*iter)->name); + IDiagnostics* diag = context->GetDiagnostics(); + diag->Warn(DiagMessage() << "removing resource " << name + << " without required default value"); + if (context->IsVerbose()) { + diag->Note(DiagMessage() << " did you forget to remove all definitions?"); + for (const auto& config_value : (*iter)->values) { + if (config_value->value != nullptr) { + diag->Note(DiagMessage(config_value->value->GetSource()) << "defined here"); + } + } + } + } + + type->entries.erase(new_end_iter, type->entries.end()); + } + } + return true; +} + +} // namespace aapt diff --git a/tools/aapt2/link/NoDefaultResourceRemover.h b/tools/aapt2/link/NoDefaultResourceRemover.h new file mode 100644 index 0000000000000..7582ef3f7516a --- /dev/null +++ b/tools/aapt2/link/NoDefaultResourceRemover.h @@ -0,0 +1,45 @@ +/* + * Copyright (C) 2018 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. + */ + +#ifndef AAPT_LINK_NODEFAULTRESOURCEREMOVER_H +#define AAPT_LINK_NODEFAULTRESOURCEREMOVER_H + +#include "android-base/macros.h" + +#include "process/IResourceTableConsumer.h" + +namespace aapt { + +// Removes any resource for which there exists no definition for the default configuration, where +// for that resource type, a definition is required. +// +// The obvious example is when defining localized strings. If a string in the default configuration +// has its name changed, the translations for that string won't be changed but will still cause +// the generated R class to contain the old string name. This will cause breakages in apps that +// still rely on the old name when the translations are updated. +class NoDefaultResourceRemover : public IResourceTableConsumer { + public: + NoDefaultResourceRemover() = default; + + bool Consume(IAaptContext* context, ResourceTable* table) override; + + private: + DISALLOW_COPY_AND_ASSIGN(NoDefaultResourceRemover); +}; + +} // namespace aapt + +#endif // AAPT_LINK_NODEFAULTRESOURCEREMOVER_H diff --git a/tools/aapt2/link/NoDefaultResourceRemover_test.cpp b/tools/aapt2/link/NoDefaultResourceRemover_test.cpp new file mode 100644 index 0000000000000..943709a2af125 --- /dev/null +++ b/tools/aapt2/link/NoDefaultResourceRemover_test.cpp @@ -0,0 +1,49 @@ +/* + * Copyright (C) 2018 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 "link/NoDefaultResourceRemover.h" + +#include "test/Test.h" + +namespace aapt { + +TEST(NoDefaultResourceRemoverTest, RemoveEntryWithNoDefaultAndOnlyLocales) { + std::unique_ptr context = test::ContextBuilder().Build(); + std::unique_ptr table = + test::ResourceTableBuilder() + .SetPackageId("android", 0x01) + .AddSimple("android:string/foo") + .AddSimple("android:string/foo", test::ParseConfigOrDie("en-rGB")) + .AddSimple("android:string/foo", test::ParseConfigOrDie("fr-rFR")) + .AddSimple("android:string/bar", test::ParseConfigOrDie("en-rGB")) + .AddSimple("android:string/bar", test::ParseConfigOrDie("fr-rFR")) + .AddSimple("android:string/bat", test::ParseConfigOrDie("en-rGB-xhdpi")) + .AddSimple("android:string/bat", test::ParseConfigOrDie("fr-rFR-hdpi")) + .AddSimple("android:string/baz", test::ParseConfigOrDie("en-rGB")) + .AddSimple("android:string/baz", test::ParseConfigOrDie("fr-rFR")) + .SetSymbolState("android:string/baz", ResourceId(0x01020002), Visibility::Level::kPublic) + .Build(); + + NoDefaultResourceRemover remover; + ASSERT_TRUE(remover.Consume(context.get(), table.get())); + + EXPECT_TRUE(table->FindResource(test::ParseNameOrDie("android:string/foo"))); + EXPECT_FALSE(table->FindResource(test::ParseNameOrDie("android:string/bar"))); + EXPECT_TRUE(table->FindResource(test::ParseNameOrDie("android:string/bat"))); + EXPECT_TRUE(table->FindResource(test::ParseNameOrDie("android:string/baz"))); +} + +} // namespace aapt