AAPT2: Warn when positional arguments exist and --legacy is on

This is normally an error, but old AAPT didn't check for it correctly,
so many projects violate this. With --legacy, this becomes a warning.

Change-Id: I23647e029930e11b719591cd38609e1b43247e20

tools/aapt2/ResourceParser.cpp

@@ -582,10 +582,15 @@ bool ResourceParser::parseString(xml::XmlPullParser* parser, ParsedResource* out
 
         if (formatted && translateable) {
             if (!util::verifyJavaStringFormat(*stringValue->value)) {
-                mDiag->error(DiagMessage(outResource->source)
-                             << "multiple substitutions specified in non-positional format; "
-                                "did you mean to add the formatted=\"false\" attribute?");
-                return false;
+                DiagMessage msg(outResource->source);
+                msg << "multiple substitutions specified in non-positional format; "
+                       "did you mean to add the formatted=\"false\" attribute?";
+                if (mOptions.errorOnPositionalArguments) {
+                    mDiag->error(msg);
+                    return false;
+                }
+
+                mDiag->warn(msg);
             }
         }
 

tools/aapt2/ResourceParser.h

@@ -44,6 +44,11 @@ struct ResourceParserOptions {
      * Whether the default setting for this parser is to allow translation.
      */
     bool translatable = true;
+
+    /**
+     * Whether positional arguments in formatted strings are treated as errors or warnings.
+     */
+    bool errorOnPositionalArguments = true;
 };
 
 /*

tools/aapt2/compile/Compile.cpp

@@ -107,6 +107,7 @@ struct CompileOptions {
     Maybe<std::string> resDir;
     std::vector<std::u16string> products;
     bool pseudolocalize = false;
+    bool legacyMode = false;
     bool verbose = false;
 };
 
@@ -192,6 +193,7 @@ static bool compileTable(IAaptContext* context, const CompileOptions& options,
 
         ResourceParserOptions parserOptions;
         parserOptions.products = options.products;
+        parserOptions.errorOnPositionalArguments = !options.legacyMode;
 
         // If the filename includes donottranslate, then the default translatable is false.
         parserOptions.translatable = pathData.name.find(u"donottranslate") == std::string::npos;
@@ -438,6 +440,8 @@ int compile(const std::vector<StringPiece>& args) {
             .optionalFlag("--dir", "Directory to scan for resources", &options.resDir)
             .optionalSwitch("--pseudo-localize", "Generate resources for pseudo-locales "
                             "(en-XA and ar-XB)", &options.pseudolocalize)
+            .optionalSwitch("--legacy", "Treat errors that used to be valid in AAPT as warnings",
+                            &options.legacyMode)
             .optionalSwitch("-v", "Enables verbose logging", &options.verbose);
     if (!flags.parse("aapt2 compile", args, &std::cerr)) {
         return 1;

tools/aapt2/link/Link.cpp

@@ -228,29 +228,53 @@ public:
         return {};
     }
 
+    /**
+     * Precondition: ResourceTable doesn't have any IDs assigned yet, nor is it linked.
+     * Postcondition: ResourceTable has only one package left. All others are stripped, or there
+     *                is an error and false is returned.
+     */
     bool verifyNoExternalPackages() {
+        auto isExtPackageFunc = [&](const std::unique_ptr<ResourceTablePackage>& pkg) -> bool {
+            return mContext.getCompilationPackage() != pkg->name ||
+                    !pkg->id ||
+                    pkg->id.value() != mContext.getPackageId();
+        };
+
         bool error = false;
         for (const auto& package : mFinalTable.packages) {
-            if (mContext.getCompilationPackage() != package->name ||
-                    !package->id || package->id.value() != mContext.getPackageId()) {
+            if (isExtPackageFunc(package)) {
                 // We have a package that is not related to the one we're building!
                 for (const auto& type : package->types) {
                     for (const auto& entry : type->entries) {
+                        ResourceNameRef resName(package->name, type->type, entry->name);
+
                         for (const auto& configValue : entry->values) {
-                            mContext.getDiagnostics()->error(
-                                    DiagMessage(configValue.value->getSource())
-                                                << "defined resource '"
-                                                << ResourceNameRef(package->name,
-                                                                   type->type,
-                                                                   entry->name)
-                                                << "' for external package '"
-                                                << package->name << "'");
-                            error = true;
+                            // Special case the occurrence of an ID that is being generated for the
+                            // 'android' package. This is due to legacy reasons.
+                            if (valueCast<Id>(configValue.value.get()) &&
+                                    package->name == u"android") {
+                                mContext.getDiagnostics()->warn(
+                                        DiagMessage(configValue.value->getSource())
+                                        << "generated id '" << resName
+                                        << "' for external package '" << package->name
+                                        << "'");
+                            } else {
+                                mContext.getDiagnostics()->error(
+                                        DiagMessage(configValue.value->getSource())
+                                        << "defined resource '" << resName
+                                        << "' for external package '" << package->name
+                                        << "'");
+                                error = true;
+                            }
                         }
                     }
                 }
             }
         }
+
+        auto newEndIter = std::remove_if(mFinalTable.packages.begin(), mFinalTable.packages.end(),
+                                         isExtPackageFunc);
+        mFinalTable.packages.erase(newEndIter, mFinalTable.packages.end());
         return !error;
     }